Skip to content
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

Validate format of timeZone before calling system LoadLocation method #115375

Merged
merged 1 commit into from Jan 31, 2023

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Jan 28, 2023

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Adds format validation of time zones ahead of calling the LoadLocation system-dependent method.

This insulates us from platform differences in handling of edge case values (xref #114508 (review))

A unit test ensuring all listed IANA time zones pass this validation is added.

Updates of CronJobs which do not change time zone values are already tolerated.

Which issue(s) this PR fixes:

Makes CronJob validation behavior more consistent across platforms.

Does this PR introduce a user-facing change?

NONE

cc @soltysh @claudiubelu

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

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:

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 28, 2023
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 28, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 28, 2023
@liggitt liggitt changed the title Validate format of timeZone Validate format of timeZone before calling system LoadLocation method Jan 28, 2023
@claudiubelu
Copy link
Contributor

It seems there are a few cases which are failing:

{Failed  === RUN   TestTimeZones
    validation_test.go:2789: America/Ciudad_Juarez failed: [<nil>: Invalid value: "America/Ciudad_Juarez": unknown time zone America/Ciudad_Juarez]
    validation_test.go:2789: Europe/Kyiv failed: [<nil>: Invalid value: "Europe/Kyiv": unknown time zone Europe/Kyiv]
--- FAIL: TestTimeZones (0.18s)

@claudiubelu
Copy link
Contributor

/test pull-ci-kubernetes-unit-windows

@pacoxu
Copy link
Member

pacoxu commented Jan 29, 2023

validation_test.go:2789: America/Ciudad_Juarez failed: [: Invalid value: "America/Ciudad_Juarez": unknown time zone America/Ciudad_Juarez]
validation_test.go:2789: Europe/Kyiv failed: [: Invalid value: "Europe/Kyiv": unknown time zone Europe/Kyiv]

The test failed on Linux and passed on Mac.

@pacoxu
Copy link
Member

pacoxu commented Jan 30, 2023

root@16a10d869e1f:/usr/src/kubernetes# cat /usr/local/go/src/time/zoneinfo_test.go | grep TestBadLocationErrMsg -A 10
func TestBadLocationErrMsg(t *testing.T) {
        time.ResetZoneinfoForTesting()
        loc := "Europe/Kyiv"
        want := errors.New("unknown time zone " + loc)
        _, err := time.LoadLocation(loc)
        if err.Error() == want.Error() {
                t.Errorf("LoadLocation(%q) error = %v; want %v", loc, err, want)
        }
}

func TestLoadLocationValidatesNames(t *testing.T) {
root@16a10d869e1f:/usr/src/kubernetes# /usr/local/go/bin/go test -timeout 30s -run ^TestBadLocationErrMsg$ time
--- FAIL: TestBadLocationErrMsg (0.00s)
    zoneinfo_test.go:46: LoadLocation("Europe/Kyiv") error = unknown time zone Europe/Kyiv; want unknown time zone Europe/Kyiv
FAIL
FAIL    time    0.003s
FAIL

The reason seems to be that America/Ciudad_Juarez and Europe/Kyiv are valid on Mac and not Valid in linux.

@pacoxu
Copy link
Member

pacoxu commented Jan 30, 2023

America/Ciudad_Juarez
http://mm.icann.org/pipermail/tz-announce/2022-November/000076.html

  • A new Zone America/Ciudad_Juarez splits from America/Ojinaga. It was added recently.

Europe/Kyiv
The suggestion to rename the time zone ID Europe/Kiev to Europe/Kyiv has been made and accepted! The change was introduced in release 2022b (Released 2022-08-11).
It can be found in https://data.iana.org/time-zones/tzdb-2022b/backward.

@liggitt
Copy link
Member Author

liggitt commented Jan 30, 2023

The reason seems to be that America/Ciudad_Juarez and Europe/Kyiv are valid on Mac and not Valid in linux.

those were added after go1.19 was released, pruned the unit test to just valid timezones as of go1.19 release, and imported "time/tzdata" to the unit test to mitigate cross-platform differences

@liggitt
Copy link
Member Author

liggitt commented Jan 30, 2023

/test pull-ci-kubernetes-unit-windows

@pacoxu
Copy link
Member

pacoxu commented Jan 31, 2023

/lgtm
/assign @claudiubelu

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 0bd154075557473648178cf7082c6ea85db304e0

@liggitt
Copy link
Member Author

liggitt commented Jan 31, 2023

Unit tests passed on Mac, windows, and Linux

@k8s-ci-robot k8s-ci-robot merged commit e150be6 into kubernetes:master Jan 31, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Jan 31, 2023
@liggitt liggitt deleted the validate-timezone branch May 9, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants