Merge pull request #116252 from soltysh/tz_validation

Forbid creating CronJob with TZ or CRON_TZ, but allow updates

pkg/apis/batch/validation/validation.go

@@ -523,7 +523,11 @@ func validateCronJobSpec(spec, oldSpec *batch.CronJobSpec, fldPath *field.Path,
 	if len(spec.Schedule) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("schedule"), ""))
 	} else {
-		allErrs = append(allErrs, validateScheduleFormat(spec.Schedule, spec.TimeZone, fldPath.Child("schedule"))...)
+		allowTZInSchedule := false
+		if oldSpec != nil {
+			allowTZInSchedule = strings.Contains(oldSpec.Schedule, "TZ")
+		}
+		allErrs = append(allErrs, validateScheduleFormat(spec.Schedule, allowTZInSchedule, spec.TimeZone, fldPath.Child("schedule"))...)
 	}
 
 	if spec.StartingDeadlineSeconds != nil {
@@ -564,13 +568,16 @@ func validateConcurrencyPolicy(concurrencyPolicy *batch.ConcurrencyPolicy, fldPa
 	return allErrs
 }
 
-func validateScheduleFormat(schedule string, timeZone *string, fldPath *field.Path) field.ErrorList {
+func validateScheduleFormat(schedule string, allowTZInSchedule bool, timeZone *string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if _, err := cron.ParseStandard(schedule); err != nil {
 		allErrs = append(allErrs, field.Invalid(fldPath, schedule, err.Error()))
 	}
-	if strings.Contains(schedule, "TZ") && timeZone != nil {
+	switch {
+	case allowTZInSchedule && strings.Contains(schedule, "TZ") && timeZone != nil:
 		allErrs = append(allErrs, field.Invalid(fldPath, schedule, "cannot use both timeZone field and TZ or CRON_TZ in schedule"))
+	case !allowTZInSchedule && strings.Contains(schedule, "TZ"):
+		allErrs = append(allErrs, field.Invalid(fldPath, schedule, "cannot use TZ or CRON_TZ in schedule, use timeZone field instead"))
 	}
 
 	return allErrs

pkg/apis/batch/validation/validation_test.go

@@ -2284,23 +2284,6 @@ func TestValidateCronJob(t *testing.T) {
 				},
 			},
 		},
-		"spec.schedule: cannot use both timeZone field and TZ or CRON_TZ in schedule": {
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "mycronjob",
-				Namespace: metav1.NamespaceDefault,
-				UID:       types.UID("1a2b3c"),
-			},
-			Spec: batch.CronJobSpec{
-				Schedule:          "TZ=UTC 0 * * * *",
-				TimeZone:          &timeZoneUTC,
-				ConcurrencyPolicy: batch.AllowConcurrent,
-				JobTemplate: batch.JobTemplateSpec{
-					Spec: batch.JobSpec{
-						Template: validPodTemplateSpec,
-					},
-				},
-			},
-		},
 		"spec.timeZone: timeZone must be nil or non-empty string": {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "mycronjob",
@@ -2673,6 +2656,125 @@ func TestValidateCronJob(t *testing.T) {
 	}
 }
 
+func TestValidateCronJobScheduleTZ(t *testing.T) {
+	validPodTemplateSpec := getValidPodTemplateSpecForGenerated(getValidGeneratedSelector())
+	validPodTemplateSpec.Labels = map[string]string{}
+	validSchedule := "0 * * * *"
+	invalidSchedule := "TZ=UTC 0 * * * *"
+	invalidCronJob := &batch.CronJob{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "mycronjob",
+			Namespace: metav1.NamespaceDefault,
+			UID:       types.UID("1a2b3c"),
+		},
+		Spec: batch.CronJobSpec{
+			Schedule:          invalidSchedule,
+			ConcurrencyPolicy: batch.AllowConcurrent,
+			JobTemplate: batch.JobTemplateSpec{
+				Spec: batch.JobSpec{
+					Template: validPodTemplateSpec,
+				},
+			},
+		},
+	}
+	validCronJob := &batch.CronJob{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "mycronjob",
+			Namespace: metav1.NamespaceDefault,
+			UID:       types.UID("1a2b3c"),
+		},
+		Spec: batch.CronJobSpec{
+			Schedule:          validSchedule,
+			ConcurrencyPolicy: batch.AllowConcurrent,
+			JobTemplate: batch.JobTemplateSpec{
+				Spec: batch.JobSpec{
+					Template: validPodTemplateSpec,
+				},
+			},
+		},
+	}
+
+	testCases := map[string]struct {
+		cronJob   *batch.CronJob
+		createErr string
+		update    func(*batch.CronJob)
+		updateErr string
+	}{
+		"update removing TZ should work": {
+			cronJob:   invalidCronJob,
+			createErr: "cannot use TZ or CRON_TZ in schedule",
+			update: func(cj *batch.CronJob) {
+				cj.Spec.Schedule = validSchedule
+			},
+		},
+		"update not modifying TZ should work": {
+			cronJob:   invalidCronJob,
+			createErr: "cannot use TZ or CRON_TZ in schedule, use timeZone field instead",
+			update: func(cj *batch.CronJob) {
+				cj.Spec.Schedule = invalidSchedule
+			},
+		},
+		"update not modifying TZ but adding .spec.timeZone should fail": {
+			cronJob:   invalidCronJob,
+			createErr: "cannot use TZ or CRON_TZ in schedule, use timeZone field instead",
+			update: func(cj *batch.CronJob) {
+				cj.Spec.TimeZone = &timeZoneUTC
+			},
+			updateErr: "cannot use both timeZone field and TZ or CRON_TZ in schedule",
+		},
+		"update adding TZ should fail": {
+			cronJob: validCronJob,
+			update: func(cj *batch.CronJob) {
+				cj.Spec.Schedule = invalidSchedule
+			},
+			updateErr: "cannot use TZ or CRON_TZ in schedule",
+		},
+	}
+
+	for k, v := range testCases {
+		t.Run(k, func(t *testing.T) {
+			errs := ValidateCronJobCreate(v.cronJob, corevalidation.PodValidationOptions{})
+			if len(errs) > 0 {
+				err := errs[0]
+				if len(v.createErr) == 0 {
+					t.Errorf("unexpected error: %#v, none expected", err)
+					return
+				}
+				if !strings.Contains(err.Error(), v.createErr) {
+					t.Errorf("unexpected error: %v, expected: %s", err, v.createErr)
+				}
+			} else if len(v.createErr) != 0 {
+				t.Errorf("no error, expected %v", v.createErr)
+				return
+			}
+
+			oldSpec := v.cronJob.DeepCopy()
+			oldSpec.ResourceVersion = "1"
+
+			newSpec := v.cronJob.DeepCopy()
+			newSpec.ResourceVersion = "2"
+			if v.update != nil {
+				v.update(newSpec)
+			}
+
+			errs = ValidateCronJobUpdate(newSpec, oldSpec, corevalidation.PodValidationOptions{})
+			if len(errs) > 0 {
+				err := errs[0]
+				if len(v.updateErr) == 0 {
+					t.Errorf("unexpected error: %#v, none expected", err)
+					return
+				}
+				if !strings.Contains(err.Error(), v.updateErr) {
+					t.Errorf("unexpected error: %v, expected: %s", err, v.updateErr)
+				}
+			} else if len(v.updateErr) != 0 {
+				t.Errorf("no error, expected %v", v.updateErr)
+				return
+			}
+		})
+	}
+}
+
 func TestValidateCronJobSpec(t *testing.T) {
 	validPodTemplateSpec := getValidPodTemplateSpecForGenerated(getValidGeneratedSelector())
 	validPodTemplateSpec.Labels = map[string]string{}

pkg/registry/batch/cronjob/strategy.go

@@ -123,9 +123,6 @@ func (cronJobStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object)
 		warnings = append(warnings, fmt.Sprintf("metadata.name: this is used in Pod names and hostnames, which can result in surprising behavior; a DNS label is recommended: %v", msgs))
 	}
 	warnings = append(warnings, job.WarningsForJobSpec(ctx, field.NewPath("spec", "jobTemplate", "spec"), &newCronJob.Spec.JobTemplate.Spec, nil)...)
-	if strings.Contains(newCronJob.Spec.Schedule, "TZ") {
-		warnings = append(warnings, fmt.Sprintf("CRON_TZ or TZ used in %s is not officially supported, see https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/ for more details", field.NewPath("spec", "spec", "schedule")))
-	}
 	return warnings
 }
 
@@ -160,7 +157,7 @@ func (cronJobStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Ob
 		warnings = job.WarningsForJobSpec(ctx, field.NewPath("spec", "jobTemplate", "spec"), &newCronJob.Spec.JobTemplate.Spec, &oldCronJob.Spec.JobTemplate.Spec)
 	}
 	if strings.Contains(newCronJob.Spec.Schedule, "TZ") {
-		warnings = append(warnings, fmt.Sprintf("CRON_TZ or TZ used in %s is not officially supported, see https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/ for more details", field.NewPath("spec", "spec", "schedule")))
+		warnings = append(warnings, fmt.Sprintf("cannot use TZ or CRON_TZ in %s, use timeZone instead, see https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/ for more details", field.NewPath("spec", "spec", "schedule")))
 	}
 	return warnings
 }

pkg/registry/batch/cronjob/strategy_test.go

@@ -273,20 +273,6 @@ func TestCronJobStrategy_WarningsOnCreate(t *testing.T) {
 				},
 			},
 		},
-		"timezone invalid": {
-			wantWarningsCount: 1,
-			cronjob: &batch.CronJob{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:            "mycronjob",
-					Namespace:       metav1.NamespaceDefault,
-					ResourceVersion: "9",
-				},
-				Spec: cronjobSpecWithTZinSchedule,
-				Status: batch.CronJobStatus{
-					LastScheduleTime: &now,
-				},
-			},
-		},
 	}
 	for name, tc := range testcases {
 		t.Run(name, func(t *testing.T) {