Forbid creating CronJob with TZ or CRON_TZ, but allow updates #116252

soltysh · 2023-03-03T13:23:47Z

What type of PR is this?

/kind cleanup
/kind api-change

What this PR does / why we need it:

Back in #106455 (k8s 1.23) after we've upgraded github.com/robfig/cron (in #102735) we've accidentally exposed internal details of the library allowing users to set TZ or CRON_TZ variable in .spec.schedule to affect TimeZone of a CronJob.

Now that kubernetes/enhancements#3140 is becoming GA we're proposing failing creating a new CronJobs with TZ or CRON_TZ in .spec.Schedule in favor of setting .spec.timeZone, but leave the warning and allow updates not to break users.

Special notes for your reviewer:

/assign @liggitt @sftim

Does this PR introduce a user-facing change?

Creation of new CronJob objects containing `TZ` or `CRON_TZ` in `.spec.schedule`, accidentally enabled in 1.22, is now disallowed. Use the `.spec.timeZone` field instead, supported in 1.25+ clusters in default configurations. See https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/#unsupported-timezone-specification for more information.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/3140

soltysh · 2023-03-03T13:24:09Z

/hold
for reaching consensus
/triage accepted
/priority backlog

soltysh · 2023-03-03T13:24:19Z

/sig apps

sftim · 2023-03-03T13:41:31Z

-Fail CronJob creation with TZ or CRON_TZ set in .spec.schedule
+Fail CronJob creation with TZ or CRON_TZ set in `.spec.schedule`

?

We can use Markdown in the changelog.
Do we want to mention that we also block adding TZ or CRON_TZ to existing CronJobs (which I think we do)?

sftim · 2023-03-03T13:55:30Z

We could have an on-by-default feature gate for this, and for one release allow people to force it off (to help people migrate). Even though the “wrong” way was never official.

liggitt · 2023-03-03T16:06:20Z

should we wait until TimeZone is enabled by default or GA in all supported releases before dropping this? rejecting this now means you can't write a manifest that uses time zone and works with all supported kubernetes versions

soltysh · 2023-03-03T16:45:24Z

should we wait until TimeZone is enabled by default or GA in all supported releases before dropping this? rejecting this now means you can't write a manifest that uses time zone and works with all supported kubernetes versions

I think that's a reasonable approach.

/milestone v1.28

sftim · 2023-03-03T16:52:28Z

I suppose the genie is out of the bottle.

should we wait until TimeZone is enabled by default or GA in all supported releases before dropping this? rejecting this now means you can't write a manifest that uses time zone and works with all supported kubernetes versions

I mean, I guess, but: how much do we want to tacitly endorse TZ and CRON_TZ which are explicitly unsupported? AIUI the timeZone field is the only way that we commit to support.

k8s-triage-robot · 2023-06-01T17:25:29Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

soltysh · 2023-09-06T13:26:28Z

@liggitt this should be ready for a re-review, unfortunately I had to leave that mutation tag, since the verification mechanism is rather simple and will always match that line, and I don't want to change the entire method's body just in case someone will in the future try to modify.

pacoxu

/lgtm
/assign @liggitt
for api change review

k8s-ci-robot · 2023-09-12T02:48:12Z

LGTM label has been added.

Git tree hash: cc896e36c026a8542ab074fbe78868f86c4fd278

liggitt · 2023-10-11T13:44:33Z

pkg/apis/batch/validation/validation.go

@@ -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, oldSchedule string, timeZone *string, fldPath *field.Path) field.ErrorList {


rather than passing in the whole schedule, I think it would be easier to understand and unit test to make the parameter here as narrow as possible, like allowTZInSchedule bool

could invoke it like this:

allowTZInSchedule := false if oldSpec != nil { allowTZInSchedule = strings.Contains(oldSpec.Schedule, "TZ") } allErrs = append(allErrs, validateScheduleFormat(spec.Schedule, allowTZInSchedule, spec.TimeZone, fldPath.Child("schedule"))...)

and use it like this:

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

Great suggestion - updated.

sftim · 2023-10-20T15:30:15Z

/uncc

liggitt

/lgtm
/approve
/kind deprecation

k8s-ci-robot · 2023-10-20T15:48:52Z

LGTM label has been added.

Git tree hash: 9d10a1efab75a10a906f33f8e760d01a56f7fefc

k8s-ci-robot · 2023-10-20T15:49:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pacoxu, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2023-10-20T15:55:00Z

be sure to open a docs PR to update the https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/#unsupported-timezone-specification to say

The implementation of the CronJob API from Kubernetes 1.22-1.28 lets you ...

soltysh · 2023-10-23T09:30:26Z

Done in kubernetes/website#43654

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 3, 2023

k8s-ci-robot assigned liggitt Mar 3, 2023

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2023

k8s-ci-robot assigned sftim Mar 3, 2023

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 3, 2023

k8s-ci-robot requested review from lavalamp and liggitt March 3, 2023 13:24

k8s-ci-robot added this to the v1.28 milestone Mar 3, 2023

soltysh mentioned this pull request Mar 7, 2023

Promote CronJobTimeZone to stable kubernetes/website#39835

Merged

marosset mentioned this pull request Mar 13, 2023

TimeZone support in CronJob kubernetes/enhancements#3140

Closed

13 tasks

soltysh force-pushed the tz_validation branch 2 times, most recently from 827d128 to a372f36 Compare September 6, 2023 13:24

pacoxu approved these changes Sep 12, 2023

View reviewed changes

k8s-ci-robot assigned pacoxu Sep 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2023

liggitt reviewed Oct 11, 2023

View reviewed changes

liggitt moved this from In progress to Changes requested in API Reviews Oct 12, 2023

Forbid creating CronJob with TZ or CRON_TZ, but allow updates

9a58f6c

soltysh force-pushed the tz_validation branch from a372f36 to 9a58f6c Compare October 20, 2023 15:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2023

k8s-ci-robot requested review from liggitt, pacoxu and sftim October 20, 2023 15:14

k8s-ci-robot removed the request for review from sftim October 20, 2023 15:30

liggitt reviewed Oct 20, 2023

View reviewed changes

k8s-ci-robot added kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 20, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2023

k8s-ci-robot merged commit f5d7c34 into kubernetes:master Oct 20, 2023
14 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 20, 2023

soltysh mentioned this pull request Oct 23, 2023

Update information about CronJob's unsupported time zone field kubernetes/website#43654

Merged

soltysh deleted the tz_validation branch October 23, 2023 09:30

sveinpj mentioned this pull request Apr 26, 2024

Upgrade AKS to 1.29 equinor/radix-platform#1318

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forbid creating CronJob with TZ or CRON_TZ, but allow updates #116252

Forbid creating CronJob with TZ or CRON_TZ, but allow updates #116252

soltysh commented Mar 3, 2023 •

edited by liggitt

soltysh commented Mar 3, 2023

soltysh commented Mar 3, 2023

sftim commented Mar 3, 2023

sftim commented Mar 3, 2023

liggitt commented Mar 3, 2023

soltysh commented Mar 3, 2023

sftim commented Mar 3, 2023

k8s-triage-robot commented Jun 1, 2023

soltysh commented Sep 6, 2023

pacoxu left a comment

k8s-ci-robot commented Sep 12, 2023

liggitt Oct 11, 2023

soltysh Oct 20, 2023

sftim commented Oct 20, 2023

liggitt left a comment

k8s-ci-robot commented Oct 20, 2023

k8s-ci-robot commented Oct 20, 2023

liggitt commented Oct 20, 2023

soltysh commented Oct 23, 2023

Forbid creating CronJob with TZ or CRON_TZ, but allow updates #116252

Forbid creating CronJob with TZ or CRON_TZ, but allow updates #116252

Conversation

soltysh commented Mar 3, 2023 • edited by liggitt

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

soltysh commented Mar 3, 2023

soltysh commented Mar 3, 2023

sftim commented Mar 3, 2023

sftim commented Mar 3, 2023

liggitt commented Mar 3, 2023

soltysh commented Mar 3, 2023

sftim commented Mar 3, 2023

k8s-triage-robot commented Jun 1, 2023

soltysh commented Sep 6, 2023

pacoxu left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 12, 2023

liggitt Oct 11, 2023

Choose a reason for hiding this comment

soltysh Oct 20, 2023

Choose a reason for hiding this comment

sftim commented Oct 20, 2023

liggitt left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 20, 2023

k8s-ci-robot commented Oct 20, 2023

liggitt commented Oct 20, 2023

soltysh commented Oct 23, 2023

soltysh commented Mar 3, 2023 •

edited by liggitt