Merge pull request #52967 from liggitt/cronjob-validate-update

Automatic merge from submit-queue (batch tested with PRs 53263, 52967, 53262, 52654, 53187). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Limit 52-character cronjob name validation to create

Follow up to https://github.com/kubernetes/kubernetes/pull/52733
Related to #50850

Needed to allow old cronjobs to be updated/migrated/deleted (with foregroundPropagation)

pkg/apis/batch/validation/validation.go

@@ -176,6 +176,14 @@ func ValidateCronJob(scheduledJob *batch.CronJob) field.ErrorList {
 	return allErrs
 }
 
+func ValidateCronJobUpdate(job, oldJob *batch.CronJob) field.ErrorList {
+	allErrs := apivalidation.ValidateObjectMetaUpdate(&job.ObjectMeta, &oldJob.ObjectMeta, field.NewPath("metadata"))
+	allErrs = append(allErrs, ValidateCronJobSpec(&job.Spec, field.NewPath("spec"))...)
+	// skip the 52-character name validation limit on update validation
+	// to allow old cronjobs with names > 52 chars to be updated/deleted
+	return allErrs
+}
+
 func ValidateCronJobSpec(spec *batch.CronJobSpec, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 

pkg/apis/batch/validation/validation_test.go

@@ -347,6 +347,14 @@ func TestValidateCronJob(t *testing.T) {
 		if errs := ValidateCronJob(&v); len(errs) != 0 {
 			t.Errorf("expected success for %s: %v", k, errs)
 		}
+
+		// Update validation should pass same success cases
+		// copy to avoid polluting the testcase object, set a resourceVersion to allow validating update, and test a no-op update
+		v = *v.DeepCopy()
+		v.ResourceVersion = "1"
+		if errs := ValidateCronJobUpdate(&v, &v); len(errs) != 0 {
+			t.Errorf("expected success for %s: %v", k, errs)
+		}
 	}
 
 	negative := int32(-1)
@@ -588,6 +596,24 @@ func TestValidateCronJob(t *testing.T) {
 				t.Errorf("unexpected error: %v, expected: %s", err, k)
 			}
 		}
+
+		// Update validation should fail all failure cases other than the 52 character name limit
+		// copy to avoid polluting the testcase object, set a resourceVersion to allow validating update, and test a no-op update
+		v = *v.DeepCopy()
+		v.ResourceVersion = "1"
+		errs = ValidateCronJobUpdate(&v, &v)
+		if len(errs) == 0 {
+			if k == "metadata.name: must be no more than 52 characters" {
+				continue
+			}
+			t.Errorf("expected failure for %s", k)
+		} else {
+			s := strings.Split(k, ":")
+			err := errs[0]
+			if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) {
+				t.Errorf("unexpected error: %v, expected: %s", err, k)
+			}
+		}
 	}
 }
 

pkg/registry/batch/cronjob/strategy.go

@@ -87,7 +87,7 @@ func (cronJobStrategy) AllowCreateOnUpdate() bool {
 
 // ValidateUpdate is the default update validation for an end user.
 func (cronJobStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
-	return validation.ValidateCronJob(obj.(*batch.CronJob))
+	return validation.ValidateCronJobUpdate(obj.(*batch.CronJob), old.(*batch.CronJob))
 }
 
 type cronJobStatusStrategy struct {