-
Notifications
You must be signed in to change notification settings - Fork 40k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Forbid modifying cronjob with CRON_TZ and TZ in .spec.schedule #129510
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Maciej Szulik <soltysh@gmail.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/priority important-longterm |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/label api-review |
As well as this, I suggest we blog about it (“mid-cycle deprecations and removals blog” specifically) and that we recommend a ValidatingAdmissionPolicy. The ValidatingAdmissionPolicy would be one that people can apply to older clusters so that they are ready to upgrade. |
Changelog suggestion -Using `TZ` or `CRON_TZ` in `.spec.schedule`, is now forbidden, use the `.spec.timeZone` field instead.
+Making a change that specifies `TZ` or `CRON_TZ` in the `.spec.schedule` of a CronJob is now forbidden; use the `.spec.timeZone` field instead. |
@@ -743,11 +743,7 @@ func validateCronJobSpec(spec, oldSpec *batch.CronJobSpec, fldPath *field.Path, | |||
if len(spec.Schedule) == 0 { | |||
allErrs = append(allErrs, field.Required(fldPath.Child("schedule"), "")) | |||
} else { | |||
allowTZInSchedule := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't expecting to drop this ratcheting validation ever... removing this toleration makes unrelated updates to cronjobs containing a TZ (like annotating them or removing a finalizer) fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already requiring use of the field in new cronjobs, I don't see a reason to break updates of old cronjobs
What type of PR is this?
/kind cleanup
/kind api-change
What this PR does / why we need it:
We've been warning about not using
CRON_TZ
andTZ
variables in schedule since k8s 1.23 (#106455), since 1.29 we only allowed updates (#116252) .
The information has been around for long enough for us to entirely restrict the usage of
CRON_TZ
andTZ
in.spec.schedule
, which is what this PR does.Which issue(s) this PR fixes:
NONE
Special notes for your reviewer:
/assign @liggitt
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: