osversion: add "Unmanifested" const

[thaJeztah]

• relates to osversion.Get() returns invalid OS Version #1080

This const can (in most situations, unless you're expecting obsolete versions of Windows) used to check if a binary hasn't been manifested, so the OSVersion cannot be used.

[thaJeztah]

So no idea if this makes sense;

• Is (the REAL) 9200 still something that people may use?
• Can this "unmanifested" version change? (was the version picked as "versions after this support manifesting, so if it's not manifested, assume the "last version before manifesting was introduced"?) <-- if so, perhaps that's worth including in the const's description.

Thought I'd open it as a PR, in case it makes sense

/cc @tianon @kevpar @jterry75

[jterry75]

9200 didnt support containers. IMO we should panic. If you are calling this and get 9200 you are either A) unsupported Windows OS, B) unmanifested in which case you will not hit the code paths you expect. Both need to be handled with a quick "panic(STOP DOING THIS)" :)

[jterry75]

^ btw, I think we can solve this with a quick once shell to cmd to obtain it which is manifested and only needs to be run once per process. Seems like an easy fix.

[thaJeztah]

^ btw, I think we can solve this with a quick once shell to cmd

You mean for version.Get() ? Need to check if there's no repos using this in (integration) test code (which usually isn't manifested) 🤔

[jterry75]

Yea basically osversion.Get(), if 9200 do a cmd ver and get the ver cmd thinks it is. If thats a real ver like 18363 then store the 18363 etc. Do this in a once so that after we know for the process it never changes and is never checked again

[thaJeztah]

Yea basically osversion.Get(), if 9200 do a cmd ver and get the ver cmd thinks it is.

Does cmd have a stable "API" to get this info?

FWIW; this is what we use as fallback in docker (perhaps could be an alternative);
https://github.com/moby/moby/blob/2400bc66ef6955760a331a2bff767d4fec244844/pkg/parsers/kernel/kernel_windows.go

[thaJeztah]

oh, actually, I think that's the wrong one; that also requires manifested 🤔 let me check.

edit: ah, yes, so we DO use that one, but probably depend on the first part to be working;
https://github.com/moby/moby/blob/0a3336fd7d19f7114fce2ff849a8989ed33e2059/pkg/archive/changes_test.go#L253-L263

We should probably update that 🤔

[TBBle]

"Shell out to cmd /c ver in once" gets an eww from me, but I recall the "Read the registry key" approach (that containerd is using for code that wrapped a the hcsshim calls that needed different parameters on different builds) was the one that MS-internal querying rejected.

Either way, I agree that 9200 is fine to be special. There's no way code that is checking this version will have a sensible action for 6.2.0.9200 apart from "fail noisily and fatally". I am assuming that's the full version returned by an unmanifested binary, i.e. the precise Server 2012/Windows 8 version, otherwise we'd be able to distinguish "Unmanifested" from "really Windows 8" and that would make things simpler.

Whether we should panic when we see 9200, or fail and give the caller an "Binary probably not manifested" error, is an interesting discussion. If all relevant code-paths that hit this inside hcsshim can expose an err, then I'd probably not panic inside hcsshim, so the caller can produce a more-useful "Did you build this binary yourself?" complaint, including checking non-canonical sources (Registry, FILEINFO of kernel32.dll, etc) to give better reports. panic in a library is very much not a friend-shaped solution.

(There was also a separate discussion about changing hcsshim APIs to either be version-specific, so that the caller must check the OS version when it matters, or allowing APIs to have the OS version injected in case the caller wants to override hcsshim's choice of canonical version info).

[jterry75]

"Shell out to cmd /c ver in once" gets an eww from me

LOL! Yea, it's just a special case for 9200 I agree. At the end of the day even though when we asked @dcantah to ask internally and we were told Registry is not supported the truth is that we are doing it all over the place and it 100% works. So if we don't do cmd /c ver in a once, then let's just read the build number from the registry in a once when we get 9200 and then use it. It's going to work.

Whether we should panic when we see 9200, or fail and give the caller an "Binary probably not manifested" error, is an interesting discussion.

Reason I suggested panic is that the API for Get() today isn't an error API. So the only option we really have is to panic right? I suppose we could add a TryGet() pattern that returns an error and update Docker/containerd to it?

[TBBle]

I (still) don't have the code in front of me but it's possible that Docker and containerd only use osversion.Get or osversion.Build explicitly in their test-suites, to pick container image bases to use. I have a vague recollection that I was surprised to discover this was the case, assuming I'm remembering the correct API, and the actually-problematic uses were inside hcsshim APIs that were directly used.

In that case, breaking the Get API to support (build, err) seems reasonable, and even if I'm misremembering, this part of hcsshim probably isn't really API-stable in any important way, given that containerd.exe isn't manifested today, so it can't be using it directly.

[jterry75]

Doing less than 5 minutes of research both are used.

Containerd is mostly absent of osversion outside of test cases. BTW the tests are manifested (or at least they used to be) (yours truly did it...). However they also use the reg key read workaround for CurrentBuild as proposed here.

For Docker they appear to use both. They use Get() and Build() and they do things like Get().MajorVersion > ... as well as Build() == osversion.V....

Given the usages here maybe we should add a new func Is(version) or IsAtLeast(version) or IsInRange(version, version)` etc so the caller can have simpler checks?

But I still propose we dont return an error even if we do the above. I want to write code like this:

if osversion.IsAtLeast(osversion.19H1) { 
   // Do cool stuff
}

I dont want to have to write:

v, err := osversion.IsAtLeast(osversion.19H1)
if err != nil {
   // unmanifested
} else if v
   // Do cool stuff
}