From 9a58f6c4fb4ce7faf4ecd81dd3e9b471e59aa1e1 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Fri, 3 Mar 2023 14:14:35 +0100 Subject: [PATCH] Forbid creating CronJob with TZ or CRON_TZ, but allow updates --- pkg/apis/batch/validation/validation.go | 13 +- pkg/apis/batch/validation/validation_test.go | 136 ++++++++++++++++--- pkg/registry/batch/cronjob/strategy.go | 5 +- pkg/registry/batch/cronjob/strategy_test.go | 14 -- 4 files changed, 130 insertions(+), 38 deletions(-) diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index 23650fb7481..e8c22ba7be6 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/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 diff --git a/pkg/apis/batch/validation/validation_test.go b/pkg/apis/batch/validation/validation_test.go index 084f06567bc..52882195512 100644 --- a/pkg/apis/batch/validation/validation_test.go +++ b/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{} diff --git a/pkg/registry/batch/cronjob/strategy.go b/pkg/registry/batch/cronjob/strategy.go index 62233ba2ae6..8c7eebc6d80 100644 --- a/pkg/registry/batch/cronjob/strategy.go +++ b/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 } diff --git a/pkg/registry/batch/cronjob/strategy_test.go b/pkg/registry/batch/cronjob/strategy_test.go index 86e8e3b090c..f9b0098eaae 100644 --- a/pkg/registry/batch/cronjob/strategy_test.go +++ b/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) {