fix panic in cron.ParseStandard

Signed-off-by: PersistentJZH <zhihao.kan17@gmail.com>

fix

optimize logic

fix unit test

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 {

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)
+				}
+			}
+		})
+	}
+}

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

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
+}

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)
+			}
+		})
+	}
+}