From b738e8c3cafb59febf2bfe585e1de614838ac8ef Mon Sep 17 00:00:00 2001 From: PersistentJZH Date: Fri, 10 Oct 2025 22:00:15 +0800 Subject: [PATCH] fix panic in cron.ParseStandard Signed-off-by: PersistentJZH fix optimize logic fix unit test --- pkg/apis/batch/validation/validation.go | 6 +- pkg/apis/batch/validation/validation_test.go | 132 +++++++++++++++++- .../cronjob/cronjob_controllerv2.go | 7 +- pkg/util/parsers/parsers.go | 15 ++ pkg/util/parsers/parsers_test.go | 130 +++++++++++++++++ 5 files changed, 281 insertions(+), 9 deletions(-) diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index b718ee4a0e3..cc8b72584fe 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/pkg/apis/batch/validation/validation.go @@ -23,8 +23,6 @@ import ( "strings" "time" - "github.com/robfig/cron/v3" - apiequality "k8s.io/apimachinery/pkg/api/equality" apimachineryapivalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -37,6 +35,7 @@ import ( "k8s.io/kubernetes/pkg/apis/batch" api "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" + "k8s.io/kubernetes/pkg/util/parsers" "k8s.io/utils/ptr" ) @@ -791,7 +790,8 @@ func validateConcurrencyPolicy(concurrencyPolicy *batch.ConcurrencyPolicy, fldPa func validateScheduleFormat(schedule string, allowTZInSchedule bool, timeZone *string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if _, err := cron.ParseStandard(schedule); err != nil { + + if _, err := parsers.ParseCronScheduleWithPanicRecovery(schedule); err != nil { allErrs = append(allErrs, field.Invalid(fldPath, schedule, err.Error())) } switch { diff --git a/pkg/apis/batch/validation/validation_test.go b/pkg/apis/batch/validation/validation_test.go index 2e30d0e1998..b9bc05ffe0b 100644 --- a/pkg/apis/batch/validation/validation_test.go +++ b/pkg/apis/batch/validation/validation_test.go @@ -18,11 +18,10 @@ package validation import ( "errors" - _ "time/tzdata" - "fmt" "strings" "testing" + _ "time/tzdata" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -4002,3 +4001,132 @@ func TestValidateFailedIndexesNotOverlapCompleted(t *testing.T) { }) } } + +// TestValidateScheduleFormatPanicRecovery tests that validateScheduleFormat +// properly recovers from panics in cron.ParseStandard and returns validation errors +func TestValidateScheduleFormatPanicRecovery(t *testing.T) { + testCases := []struct { + name string + schedule string + expected string + }{ + { + name: "TZ=0 without space should not panic", + schedule: "TZ=0", + expected: "invalid schedule format", + }, + { + name: "TZ= without value should not panic", + schedule: "TZ=", + expected: "invalid schedule format", + }, + { + name: "CRON_TZ= without space should not panic", + schedule: "CRON_TZ=UTC", + expected: "invalid schedule format", + }, + { + name: "malformed timezone spec should not panic", + schedule: "TZ=Invalid/Timezone", + expected: "invalid schedule format", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // This should not panic and should return a validation error + errs := validateScheduleFormat(tc.schedule, false, nil, field.NewPath("spec", "schedule")) + + if len(errs) == 0 { + t.Errorf("Expected validation error for schedule %q, but got none", tc.schedule) + return + } + + // Check that the error message contains expected text + errMsg := errs[0].Error() + if !strings.Contains(errMsg, tc.expected) { + t.Errorf("Expected error message to contain %q, but got: %s", tc.expected, errMsg) + } + + // Verify it's an Invalid field error + if errs[0].Type != field.ErrorTypeInvalid { + t.Errorf("Expected ErrorTypeInvalid, but got: %v", errs[0].Type) + } + }) + } +} + +// TestValidateScheduleFormatNormalCases tests normal validation cases +// to ensure panic recovery doesn't interfere with normal operation +func TestValidateScheduleFormatNormalCases(t *testing.T) { + testCases := []struct { + name string + schedule string + allowTZInSchedule bool + timeZone *string + expectError bool + expectedError string + }{ + { + name: "valid schedule without timezone", + schedule: "0 0 * * *", + allowTZInSchedule: false, + timeZone: nil, + expectError: false, + }, + { + name: "valid schedule with timezone field", + schedule: "0 0 * * *", + allowTZInSchedule: false, + timeZone: &timeZoneUTC, + expectError: false, + }, + { + name: "TZ in schedule when not allowed", + schedule: "TZ=UTC 0 0 * * *", + allowTZInSchedule: false, + timeZone: nil, + expectError: true, + expectedError: "cannot use TZ or CRON_TZ in schedule", + }, + { + name: "TZ in schedule when allowed", + schedule: "TZ=UTC 0 0 * * *", + allowTZInSchedule: true, + timeZone: nil, + expectError: false, + }, + { + name: "TZ in schedule with timezone field", + schedule: "TZ=UTC 0 0 * * *", + allowTZInSchedule: true, + timeZone: &timeZoneUTC, + expectError: true, + expectedError: "cannot use both timeZone field and TZ or CRON_TZ in schedule", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + errs := validateScheduleFormat(tc.schedule, tc.allowTZInSchedule, tc.timeZone, field.NewPath("spec", "schedule")) + + if tc.expectError { + if len(errs) == 0 { + t.Errorf("Expected validation error for schedule %q, but got none", tc.schedule) + return + } + + if tc.expectedError != "" { + errMsg := errs[0].Error() + if !strings.Contains(errMsg, tc.expectedError) { + t.Errorf("Expected error message to contain %q, but got: %s", tc.expectedError, errMsg) + } + } + } else { + if len(errs) > 0 { + t.Errorf("Expected no validation errors for schedule %q, but got: %v", tc.schedule, errs) + } + } + }) + } +} diff --git a/pkg/controller/cronjob/cronjob_controllerv2.go b/pkg/controller/cronjob/cronjob_controllerv2.go index 643d6f7c554..a0152184300 100644 --- a/pkg/controller/cronjob/cronjob_controllerv2.go +++ b/pkg/controller/cronjob/cronjob_controllerv2.go @@ -24,8 +24,6 @@ import ( "strings" "time" - "github.com/robfig/cron/v3" - batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -48,6 +46,7 @@ import ( "k8s.io/kubernetes/pkg/controller" "k8s.io/kubernetes/pkg/controller/cronjob/metrics" jobutil "k8s.io/kubernetes/pkg/controller/job/util" + "k8s.io/kubernetes/pkg/util/parsers" "k8s.io/utils/ptr" ) @@ -391,7 +390,7 @@ func (jm *ControllerV2) updateCronJob(logger klog.Logger, old interface{}, curr // the sync loop will essentially be a no-op for the already queued key with old schedule. if oldCJ.Spec.Schedule != newCJ.Spec.Schedule || !ptr.Equal(oldCJ.Spec.TimeZone, newCJ.Spec.TimeZone) { // schedule changed, change the requeue time, pass nil recorder so that syncCronJob will output any warnings - sched, err := cron.ParseStandard(formatSchedule(newCJ, nil)) + sched, err := parsers.ParseCronScheduleWithPanicRecovery(formatSchedule(newCJ, nil)) if err != nil { // this is likely a user error in defining the spec value // we should log the error and not reconcile this cronjob until an update to spec @@ -511,7 +510,7 @@ func (jm *ControllerV2) syncCronJob( return nil, updateStatus, nil } - sched, err := cron.ParseStandard(formatSchedule(cronJob, jm.recorder)) + sched, err := parsers.ParseCronScheduleWithPanicRecovery(formatSchedule(cronJob, jm.recorder)) if err != nil { // this is likely a user error in defining the spec value // we should log the error and not reconcile this cronjob until an update to spec diff --git a/pkg/util/parsers/parsers.go b/pkg/util/parsers/parsers.go index 80a5f32a877..73916fda304 100644 --- a/pkg/util/parsers/parsers.go +++ b/pkg/util/parsers/parsers.go @@ -24,6 +24,7 @@ import ( _ "crypto/sha512" dockerref "github.com/distribution/reference" + "github.com/robfig/cron/v3" ) // ParseImageName parses a docker image string into three parts: repo, tag and digest. @@ -52,3 +53,17 @@ func ParseImageName(image string) (string, string, string, error) { } return repoToPull, tag, digest, nil } + +// ParseCronScheduleWithPanicRecovery safely parses a cron schedule, recovering from panics +// that can occur in cron.ParseStandard for malformed schedules like "TZ=0". +func ParseCronScheduleWithPanicRecovery(schedule string) (sched cron.Schedule, err error) { + defer func() { + if r := recover(); r != nil { + sched = nil + err = fmt.Errorf("invalid schedule format: %v", r) + } + }() + + sched, err = cron.ParseStandard(schedule) + return +} diff --git a/pkg/util/parsers/parsers_test.go b/pkg/util/parsers/parsers_test.go index 82a75e6a258..96ead1c96cd 100644 --- a/pkg/util/parsers/parsers_test.go +++ b/pkg/util/parsers/parsers_test.go @@ -19,6 +19,7 @@ package parsers import ( "strings" "testing" + "time" ) // Based on Docker test case removed in: @@ -58,3 +59,132 @@ func TestParseImageName(t *testing.T) { } } } + +func TestParseCronSchedule(t *testing.T) { + testCases := []struct { + name string + schedule string + expectError bool + expectedError string + expectPanic bool + }{ + { + name: "valid schedule without timezone", + schedule: "0 0 * * *", + expectError: false, + }, + { + name: "valid schedule with timezone", + schedule: "TZ=UTC 0 0 * * *", + expectError: false, + }, + { + name: "valid schedule with CRON_TZ", + schedule: "CRON_TZ=America/New_York 0 0 * * *", + expectError: false, + }, + { + name: "TZ=0 without space should panic and be recovered", + schedule: "TZ=0", + expectError: true, + expectedError: "invalid schedule format", + expectPanic: true, + }, + { + name: "TZ= without value should panic and be recovered", + schedule: "TZ=", + expectError: true, + expectedError: "invalid schedule format", + expectPanic: true, + }, + { + name: "CRON_TZ= without space should panic and be recovered", + schedule: "CRON_TZ=UTC", + expectError: true, + expectedError: "invalid schedule format", + expectPanic: true, + }, + { + name: "malformed timezone spec should panic and be recovered", + schedule: "TZ=Invalid/Timezone", + expectError: true, + expectedError: "invalid schedule format", + expectPanic: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // This should not panic even with malformed schedules + sched, err := ParseCronScheduleWithPanicRecovery(tc.schedule) + + if tc.expectError { + if err == nil { + t.Errorf("Expected error for schedule %q, but got none", tc.schedule) + return + } + + if tc.expectedError != "" { + errMsg := err.Error() + if !strings.Contains(errMsg, tc.expectedError) { + t.Errorf("Expected error message to contain %q, but got: %s", tc.expectedError, errMsg) + } + } + + if sched != nil { + t.Errorf("Expected nil schedule when error occurs, but got: %v", sched) + } + } else { + if err != nil { + t.Errorf("Expected no error for schedule %q, but got: %v", tc.schedule, err) + return + } + + if sched == nil { + t.Errorf("Expected valid schedule for %q, but got nil", tc.schedule) + return + } + + // Verify that the schedule is actually valid by calling Next + // This ensures we got a real cron.Schedule, not just a non-nil value + next := sched.Next(time.Now()) + if next.IsZero() { + t.Errorf("Expected valid next execution time for schedule %q, but got zero time", tc.schedule) + } + } + }) + } +} + +func TestParseCronSchedulePanicRecovery(t *testing.T) { + // Test that panics are properly recovered and converted to errors + panicSchedules := []string{ + "TZ=0", + "TZ=", + "CRON_TZ=UTC", + "TZ=Invalid/Timezone", + } + + for _, schedule := range panicSchedules { + t.Run("panic_recovery_"+schedule, func(t *testing.T) { + // This should not panic + sched, err := ParseCronScheduleWithPanicRecovery(schedule) + + // Should get an error + if err == nil { + t.Errorf("Expected error for panic-causing schedule %q, but got none", schedule) + } + + // Should get nil schedule + if sched != nil { + t.Errorf("Expected nil schedule for panic-causing schedule %q, but got: %v", schedule, sched) + } + + // Error message should contain "invalid schedule format" + errMsg := err.Error() + if !strings.Contains(errMsg, "invalid schedule format") { + t.Errorf("Expected error message to contain 'invalid schedule format', but got: %s", errMsg) + } + }) + } +}