From c2347f91fb16002567b938ef2fbf2bb8b5f86090 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 7 Jan 2025 17:41:08 -0800 Subject: [PATCH] Move windows matcher logic so all platforms can use There shouldn't be any need to make the platform matcher stuff for Windows to only be available on Windows since none of it is dependent on the machine running it. Signed-off-by: Brian Goff --- defaults_test.go | 204 ++++++++++++++++ defaults_windows.go | 76 ------ defaults_windows_test.go | 227 +----------------- ...t_windows.go => platform_windows_compat.go | 82 ++++++- ...test.go => platform_windows_compat_test.go | 6 +- platforms.go | 37 ++- platforms_other.go | 30 --- platforms_windows.go | 34 --- 8 files changed, 330 insertions(+), 366 deletions(-) create mode 100644 defaults_test.go rename platform_compat_windows.go => platform_windows_compat.go (60%) rename platform_compat_windows_test.go => platform_windows_compat_test.go (92%) delete mode 100644 platforms_other.go delete mode 100644 platforms_windows.go diff --git a/defaults_test.go b/defaults_test.go new file mode 100644 index 0000000..d0e1163 --- /dev/null +++ b/defaults_test.go @@ -0,0 +1,204 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package platforms + +import ( + "reflect" + "runtime" + "sort" + "testing" + + imagespec "github.com/opencontainers/image-spec/specs-go/v1" +) + +// TestMatchComparerMatch_ABICheckWCOW checks windows platform matcher +// behavior for stable ABI and non-stable ABI compliant versions +func TestMatchComparerMatch_ABICheckWCOW(t *testing.T) { + platformNoVersion := imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + } + platformWS2019 := imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763", + } + platformWS2022 := imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.20348", + } + platformWindows11 := imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.22621", + } + + for _, test := range []struct { + hostPlatform imagespec.Platform + testPlatform imagespec.Platform + match bool + }{ + { + hostPlatform: platformWS2019, + testPlatform: imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763", + }, + match: true, + }, + { + hostPlatform: platformWS2019, + testPlatform: imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.20348", + }, + match: false, + }, + { + hostPlatform: platformWS2022, + testPlatform: imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763", + }, + match: false, + }, + { + hostPlatform: platformWS2022, + testPlatform: imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.20348", + }, + match: true, + }, + { + hostPlatform: platformWindows11, + testPlatform: imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763", + }, + match: false, + }, + { + hostPlatform: platformWindows11, + testPlatform: imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.20348", + }, + match: true, + }, + { + hostPlatform: platformNoVersion, + testPlatform: platformWS2019, + match: true, + }, + { + hostPlatform: platformNoVersion, + testPlatform: platformNoVersion, + match: true, + }, + { + hostPlatform: platformNoVersion, + testPlatform: platformWindows11, + match: true, + }, + } { + matcher := NewMatcher(test.hostPlatform) + if actual := matcher.Match(test.testPlatform); actual != test.match { + t.Errorf("should match: %t, test(%s) to host(%s)", test.match, test.hostPlatform, test.testPlatform) + } + } +} + +func TestWindowsMatchComparerLess(t *testing.T) { + p := imagespec.Platform{ + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763.1", + } + + m := NewMatcher(p) + if runtime.GOOS != "windows" { + // By default NewMatcher only returns the MatchComparer interface on Windows (which is only for backwards compatibility). + // On other platforms, we need to wrap the matcher in a windowsMatchComparer since the test is using it. + m = &windowsMatchComparer{m} + } + platforms := []imagespec.Platform{ + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17764.1", + }, + { + Architecture: "amd64", + OS: "windows", + }, + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763.1", + }, + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763.2", + }, + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17762.1", + }, + } + expected := []imagespec.Platform{ + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763.2", + }, + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17763.1", + }, + { + Architecture: "amd64", + OS: "windows", + }, + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17764.1", + }, + { + Architecture: "amd64", + OS: "windows", + OSVersion: "10.0.17762.1", + }, + } + sort.SliceStable(platforms, func(i, j int) bool { + return m.(MatchComparer).Less(platforms[i], platforms[j]) + }) + if !reflect.DeepEqual(platforms, expected) { + t.Errorf("expected: %s\nactual : %s", expected, platforms) + } +} diff --git a/defaults_windows.go b/defaults_windows.go index 8bae4eb..0165ade 100644 --- a/defaults_windows.go +++ b/defaults_windows.go @@ -19,8 +19,6 @@ package platforms import ( "fmt" "runtime" - "strconv" - "strings" specs "github.com/opencontainers/image-spec/specs-go/v1" "golang.org/x/sys/windows" @@ -38,80 +36,6 @@ func DefaultSpec() specs.Platform { } } -type windowsmatcher struct { - specs.Platform - osVersionPrefix string - defaultMatcher Matcher -} - -// Match matches platform with the same windows major, minor -// and build version. -func (m windowsmatcher) Match(p specs.Platform) bool { - match := m.defaultMatcher.Match(p) - - if match && m.OS == "windows" { - // HPC containers do not have OS version filled - if m.OSVersion == "" || p.OSVersion == "" { - return true - } - - hostOsVersion := getOSVersion(m.osVersionPrefix) - ctrOsVersion := getOSVersion(p.OSVersion) - return checkHostAndContainerCompat(hostOsVersion, ctrOsVersion) - } - - return match -} - -func getOSVersion(osVersionPrefix string) osVersion { - parts := strings.Split(osVersionPrefix, ".") - if len(parts) < 3 { - return osVersion{} - } - - majorVersion, _ := strconv.ParseUint(parts[0], 10, 8) - minorVersion, _ := strconv.ParseUint(parts[1], 10, 8) - buildNumber, _ := strconv.ParseUint(parts[2], 10, 16) - - return osVersion{ - MajorVersion: uint8(majorVersion), - MinorVersion: uint8(minorVersion), - Build: uint16(buildNumber), - } -} - -// Less sorts matched platforms in front of other platforms. -// For matched platforms, it puts platforms with larger revision -// number in front. -func (m windowsmatcher) Less(p1, p2 specs.Platform) bool { - m1, m2 := m.Match(p1), m.Match(p2) - if m1 && m2 { - r1, r2 := revision(p1.OSVersion), revision(p2.OSVersion) - return r1 > r2 - } - return m1 && !m2 -} - -func revision(v string) int { - parts := strings.Split(v, ".") - if len(parts) < 4 { - return 0 - } - r, err := strconv.Atoi(parts[3]) - if err != nil { - return 0 - } - return r -} - -func prefix(v string) string { - parts := strings.Split(v, ".") - if len(parts) < 4 { - return v - } - return strings.Join(parts[0:3], ".") -} - // Default returns the current platform's default platform specification. func Default() MatchComparer { return Only(DefaultSpec()) diff --git a/defaults_windows_test.go b/defaults_windows_test.go index 8d459e3..c3193a5 100644 --- a/defaults_windows_test.go +++ b/defaults_windows_test.go @@ -20,7 +20,6 @@ import ( "fmt" "reflect" "runtime" - "sort" "testing" imagespec "github.com/opencontainers/image-spec/specs-go/v1" @@ -69,19 +68,14 @@ func TestDefaultMatchComparer(t *testing.T) { t.Errorf("expected: %v, actual: %v", test.match, actual) } } - } func TestMatchComparerMatch_WCOW(t *testing.T) { major, minor, build := windows.RtlGetNtVersionNumbers() buildStr := fmt.Sprintf("%d.%d.%d", major, minor, build) - m := windowsmatcher{ - Platform: DefaultSpec(), - osVersionPrefix: buildStr, - defaultMatcher: &matcher{ - Platform: Normalize(DefaultSpec()), - }, - } + platform := DefaultSpec() + m := NewMatcher(platform) + for _, test := range []struct { platform imagespec.Platform match bool @@ -140,133 +134,7 @@ func TestMatchComparerMatch_WCOW(t *testing.T) { }, } { if actual := m.Match(test.platform); actual != test.match { - t.Errorf("should match: %t, %s to %s", test.match, m.Platform, test.platform) - } - } -} - -// TestMatchComparerMatch_ABICheckWCOW checks windows platform matcher -// behavior for stable ABI and non-stable ABI compliant versions -func TestMatchComparerMatch_ABICheckWCOW(t *testing.T) { - platformNoVersion := imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - } - platformWS2019 := imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763", - } - platformWS2022 := imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.20348", - } - platformWindows11 := imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.22621", - } - matcherNoVersion := NewMatcher(platformNoVersion).(windowsmatcher) - matcherWS2019 := windowsmatcher{ - Platform: platformWS2019, - osVersionPrefix: platformWS2019.OSVersion, - defaultMatcher: &matcher{ - Platform: Normalize(platformWS2019), - }, - } - matcherWS2022 := windowsmatcher{ - Platform: platformWS2022, - osVersionPrefix: platformWS2022.OSVersion, - defaultMatcher: &matcher{ - Platform: Normalize(platformWS2022), - }, - } - matcherWindows11 := windowsmatcher{ - Platform: platformWindows11, - osVersionPrefix: platformWindows11.OSVersion, - defaultMatcher: &matcher{ - Platform: Normalize(platformWindows11), - }, - } - - for _, test := range []struct { - hostPlatformMatcher windowsmatcher - testPlatform imagespec.Platform - match bool - }{ - { - hostPlatformMatcher: matcherWS2019, - testPlatform: imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763", - }, - match: true, - }, - { - hostPlatformMatcher: matcherWS2019, - testPlatform: imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.20348", - }, - match: false, - }, - { - hostPlatformMatcher: matcherWS2022, - testPlatform: imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763", - }, - match: false, - }, - { - hostPlatformMatcher: matcherWS2022, - testPlatform: imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.20348", - }, - match: true, - }, - { - hostPlatformMatcher: matcherWindows11, - testPlatform: imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763", - }, - match: false, - }, - { - hostPlatformMatcher: matcherWindows11, - testPlatform: imagespec.Platform{ - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.20348", - }, - match: true, - }, - { - hostPlatformMatcher: matcherNoVersion, - testPlatform: platformWS2019, - match: true, - }, - { - hostPlatformMatcher: matcherNoVersion, - testPlatform: platformNoVersion, - match: true, - }, - { - hostPlatformMatcher: matcherNoVersion, - testPlatform: platformWindows11, - match: true, - }, - } { - if actual := test.hostPlatformMatcher.Match(test.testPlatform); actual != test.match { - t.Errorf("should match: %t, %s to %s", test.match, test.hostPlatformMatcher.Platform, test.testPlatform) + t.Errorf("should match: %t, %s to %s", test.match, platform, test.platform) } } } @@ -274,20 +142,9 @@ func TestMatchComparerMatch_ABICheckWCOW(t *testing.T) { func TestMatchComparerMatch_LCOW(t *testing.T) { major, minor, build := windows.RtlGetNtVersionNumbers() buildStr := fmt.Sprintf("%d.%d.%d", major, minor, build) - m := windowsmatcher{ - Platform: imagespec.Platform{ - OS: "linux", - Architecture: "amd64", - }, - osVersionPrefix: "", - defaultMatcher: &matcher{ - Platform: Normalize(imagespec.Platform{ - OS: "linux", - Architecture: "amd64", - }, - ), - }, - } + + pLinux := imagespec.Platform{OS: "linux", Architecture: "amd64"} + m := NewMatcher(pLinux) for _, test := range []struct { platform imagespec.Platform match bool @@ -329,75 +186,7 @@ func TestMatchComparerMatch_LCOW(t *testing.T) { }, } { if actual := m.Match(test.platform); actual != test.match { - t.Errorf("should match: %t, %s to %s", test.match, m.Platform, test.platform) + t.Errorf("should match: %t, %s to %s", test.match, pLinux, test.platform) } } } - -func TestMatchComparerLess(t *testing.T) { - m := windowsmatcher{ - Platform: DefaultSpec(), - osVersionPrefix: "10.0.17763", - defaultMatcher: &matcher{ - Platform: Normalize(DefaultSpec()), - }, - } - platforms := []imagespec.Platform{ - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17764.1", - }, - { - Architecture: "amd64", - OS: "windows", - }, - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763.1", - }, - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763.2", - }, - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17762.1", - }, - } - expected := []imagespec.Platform{ - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763.2", - }, - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17763.1", - }, - { - Architecture: "amd64", - OS: "windows", - }, - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17764.1", - }, - { - Architecture: "amd64", - OS: "windows", - OSVersion: "10.0.17762.1", - }, - } - sort.SliceStable(platforms, func(i, j int) bool { - return m.Less(platforms[i], platforms[j]) - }) - if !reflect.DeepEqual(platforms, expected) { - t.Errorf("expected: %s\nactual : %s", expected, platforms) - } -} diff --git a/platform_compat_windows.go b/platform_windows_compat.go similarity index 60% rename from platform_compat_windows.go rename to platform_windows_compat.go index 89e66f0..1fe37f9 100644 --- a/platform_compat_windows.go +++ b/platform_windows_compat.go @@ -16,9 +16,16 @@ package platforms +import ( + "strconv" + "strings" + + specs "github.com/opencontainers/image-spec/specs-go/v1" +) + // osVersion is a wrapper for Windows version information // https://msdn.microsoft.com/en-us/library/windows/desktop/ms724439(v=vs.85).aspx -type osVersion struct { +type windowsOSVersion struct { Version uint32 MajorVersion uint8 MinorVersion uint8 @@ -55,7 +62,7 @@ var compatLTSCReleases = []uint16{ // Every release after WS 2022 will support the previous ltsc // container image. Stable ABI is in preview mode for windows 11 client. // Refer: https://learn.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility?tabs=windows-server-2022%2Cwindows-10#windows-server-host-os-compatibility -func checkHostAndContainerCompat(host, ctr osVersion) bool { +func checkWindowsHostAndContainerCompat(host, ctr windowsOSVersion) bool { // check major minor versions of host and guest if host.MajorVersion != ctr.MajorVersion || host.MinorVersion != ctr.MinorVersion { @@ -76,3 +83,74 @@ func checkHostAndContainerCompat(host, ctr osVersion) bool { } return ctr.Build >= supportedLtscRelease && ctr.Build <= host.Build } + +func getWindowsOSVersion(osVersionPrefix string) windowsOSVersion { + if strings.Count(osVersionPrefix, ".") < 2 { + return windowsOSVersion{} + } + + major, extra, _ := strings.Cut(osVersionPrefix, ".") + minor, extra, _ := strings.Cut(extra, ".") + build, _, _ := strings.Cut(extra, ".") + + majorVersion, err := strconv.ParseUint(major, 10, 8) + if err != nil { + return windowsOSVersion{} + } + + minorVersion, err := strconv.ParseUint(minor, 10, 8) + if err != nil { + return windowsOSVersion{} + } + buildNumber, err := strconv.ParseUint(build, 10, 16) + if err != nil { + return windowsOSVersion{} + } + + return windowsOSVersion{ + MajorVersion: uint8(majorVersion), + MinorVersion: uint8(minorVersion), + Build: uint16(buildNumber), + } +} + +func winRevision(v string) int { + parts := strings.Split(v, ".") + if len(parts) < 4 { + return 0 + } + r, err := strconv.Atoi(parts[3]) + if err != nil { + return 0 + } + return r +} + +type windowsVersionMatcher struct { + windowsOSVersion +} + +func (m windowsVersionMatcher) Match(v string) bool { + if m.isEmpty() || v == "" { + return true + } + osv := getWindowsOSVersion(v) + return checkWindowsHostAndContainerCompat(m.windowsOSVersion, osv) +} + +func (m windowsVersionMatcher) isEmpty() bool { + return m.MajorVersion == 0 && m.MinorVersion == 0 && m.Build == 0 +} + +type windowsMatchComparer struct { + Matcher +} + +func (c *windowsMatchComparer) Less(p1, p2 specs.Platform) bool { + m1, m2 := c.Match(p1), c.Match(p2) + if m1 && m2 { + r1, r2 := winRevision(p1.OSVersion), winRevision(p2.OSVersion) + return r1 > r2 + } + return m1 && !m2 +} diff --git a/platform_compat_windows_test.go b/platform_windows_compat_test.go similarity index 92% rename from platform_compat_windows_test.go rename to platform_windows_compat_test.go index 482c181..6f687e1 100644 --- a/platform_compat_windows_test.go +++ b/platform_windows_compat_test.go @@ -63,17 +63,17 @@ func Test_PlatformCompat(t *testing.T) { // Check if ltsc2019/ltsc2022 guest images are compatible on // the given host OS versions // - hostOSVersion := osVersion{ + hostOSVersion := windowsOSVersion{ MajorVersion: 10, MinorVersion: 0, Build: tc.hostOs, } - ctrOSVersion := osVersion{ + ctrOSVersion := windowsOSVersion{ MajorVersion: 10, MinorVersion: 0, Build: tc.ctrOs, } - if checkHostAndContainerCompat(hostOSVersion, ctrOSVersion) != tc.shouldRun { + if checkWindowsHostAndContainerCompat(hostOSVersion, ctrOSVersion) != tc.shouldRun { var expectedResultStr string if !tc.shouldRun { expectedResultStr = " NOT" diff --git a/platforms.go b/platforms.go index 7a84449..14d65ab 100644 --- a/platforms.go +++ b/platforms.go @@ -144,18 +144,51 @@ type Matcher interface { // // Applications should opt to use `Match` over directly parsing specifiers. func NewMatcher(platform specs.Platform) Matcher { - return newDefaultMatcher(platform) + m := &matcher{ + Platform: Normalize(platform), + } + + if platform.OS == "windows" { + m.osvM = &windowsVersionMatcher{ + windowsOSVersion: getWindowsOSVersion(platform.OSVersion), + } + // In prior versions, on windows, the returned matcher implements a + // MatchComprarer interface. + // This preserves that behavior for backwards compatibility. + // + // TODO: This isn't actually used in this package, except for a test case, + // which may have been an unintended side of some refactor. + // It was likely intended to be used in `Ordered` but it is not since + // `Less` that is implemented here ends up getting masked due to wrapping. + if runtime.GOOS == "windows" { + return &windowsMatchComparer{m} + } + } + return m +} + +type osVerMatcher interface { + Match(string) bool } type matcher struct { specs.Platform + osvM osVerMatcher } func (m *matcher) Match(platform specs.Platform) bool { normalized := Normalize(platform) return m.OS == normalized.OS && m.Architecture == normalized.Architecture && - m.Variant == normalized.Variant + m.Variant == normalized.Variant && + m.matchOSVersion(platform) +} + +func (m *matcher) matchOSVersion(platform specs.Platform) bool { + if m.osvM != nil { + return m.osvM.Match(platform.OSVersion) + } + return true } func (m *matcher) String() string { diff --git a/platforms_other.go b/platforms_other.go deleted file mode 100644 index 03f4dcd..0000000 --- a/platforms_other.go +++ /dev/null @@ -1,30 +0,0 @@ -//go:build !windows - -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package platforms - -import ( - specs "github.com/opencontainers/image-spec/specs-go/v1" -) - -// NewMatcher returns the default Matcher for containerd -func newDefaultMatcher(platform specs.Platform) Matcher { - return &matcher{ - Platform: Normalize(platform), - } -} diff --git a/platforms_windows.go b/platforms_windows.go deleted file mode 100644 index 950e2a2..0000000 --- a/platforms_windows.go +++ /dev/null @@ -1,34 +0,0 @@ -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package platforms - -import ( - specs "github.com/opencontainers/image-spec/specs-go/v1" -) - -// NewMatcher returns a Windows matcher that will match on osVersionPrefix if -// the platform is Windows otherwise use the default matcher -func newDefaultMatcher(platform specs.Platform) Matcher { - prefix := prefix(platform.OSVersion) - return windowsmatcher{ - Platform: platform, - osVersionPrefix: prefix, - defaultMatcher: &matcher{ - Platform: Normalize(platform), - }, - } -}