nit: I am not sure if saying must be enabled isn't too strong considering it is on by default.

examples what we use in other places>

// This is beta field and enabled/disabled by DaemonSetUpdateSurge feature gate.

// This is a beta field and requires enabling GRPCContainerProbe feature gate.

I copied that from some other place, so I guess we use both interchangeably 😉

wouldn't it be better to compare the lowercase strings and have a case for darwin in the for loop?

not needed the values are guaranteed by the language https://pkg.go.dev/runtime#pkg-constants

I meant comparing the timezones by lowercase for darwin

Yeah, but our tests explicitly verify the case correctness, as you see in validation_tests.go so the whole point is to make sure that the case exactly matches what we have in the database file. If you check https://go.dev/src/time/zoneinfo_unix.go, specifically this fragment:

var zoneSources = []string{
	"/usr/share/zoneinfo/",
	"/usr/share/lib/zoneinfo/",
	"/usr/lib/locale/TZ/",
	runtime.GOROOT() + "/lib/time/zoneinfo.zip",
}

you'll notice that the mechanism first checks OS-provided time zone database, which is just a list of directories with files, where the name matches exactly the name of the time zone. The problem is that for MacOS with case insensitive file system America/NewYork is equal to AMERICA/Newyork which causes a problem for our validation tests. Thus to make sure the case is exactly we fallback to verifying the golang provided database which is a zip file, thus we can rely on casing for that. I hope that makes more sense now 🤓

Thanks for explanation :)

maybe I am missing something, but I am a bit confused when I read this description.

I've improved the description a bit.

would it make sense to have some unit tests for this function and passing the OS as a parameter?

Seems like an overkill, but I could.

ack, I will let it up to you

Wait, don't all operating systems have the potential to have case insensitive filesystems?

Yeah, after some research it turns out that all can, but linux usually defaults to case sensitive, whereas mac insensitive (https://en.wikipedia.org/wiki/Case_sensitivity#In_filesystems).

Sorry for nitpicking, but the following is not still clear to me. I mean This( from a sentence This only applies to MacOS) seems to reference ensure the case of the timeZone matches exactly, but we do the opposite in the function.

I meant comparing the timezones by lowercase for darwin

ack, I will let it up to you

Is the reason we have the MacOS guard because we believe on other systems there may be a valid usecase for having case-insensitive timezone changes?

If all OS's can have case-insensitive filesystems, then this doesn't make sense.

Also, this is validation from the server process... that feels... very hacky, to put that check in here, since we don't typically have OS specific checks in server side code.

This also has the side effect of forcing the server process to only accept timezones in the GOROOT, not the ones in the system. That feels wrong, and I am very uncomfortable having server code having different behavior across OSes.

What is the alternate fix for this that doesn't alter the validation path? Or makes this a "symptom based" detection, not a "IF general environment THEN workaround" detection?

This is to fulfill the requirement we found after merging alpha, see https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/3140-TimeZone-support-in-CronJob/README.md#beta which points to both #109218 which added skip for a case-sensitive TZ on a mac, and golang/go#21512 explaining why golang doesn't care about case in TZ database.

I'm saying the requirement shouldn't be solved like this.

At a first read this code doesn't belong here (it's mac specific, not "insensitive filesystem specific", and it works around a problem by identifying the environment, rather than resolving the problem). If you're on a Mac, and you provide "foo" to CronJob timezone, and the system database says "i've got foo", given how we've defined the field, we must accept that input.

The linked test is wrong - based on the defined behavior of the field, the database could have AMERICA/new_york in it, which means our validation automatically accepts whatever values are in that file. In effect, we are not validating the locale name, we're consulting a random third party (the host fs) for info that usually, but not always, is consistent.

I don't think this is something that should be fixed in validation. This is instead something we try to verify via our tests - that a third party TZ data entry can be loaded based on the current environment of the process. I.e. create a synthetic file on Mac, try to load it. Create a synthetic timezone DB on Linux, set the process lookup so that time.LoadLocation checks that, verify we find the arbitrary thing we look for.

I tweaked this to suggest The time zone name (since we're specifying a name, not an offset etc. Minor clarity point.

Use https://pkg.go.dev/testing#T.Setenv instead (in go1.17)