From 9072b09145822cd1cc6c6963aac8ecc8fd6592a2 Mon Sep 17 00:00:00 2001 From: Tianon Gravi Date: Mon, 28 Dec 2020 14:07:55 -0800 Subject: [PATCH] Refactor platforms.Only with a "platformVector" helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This improves the hard-coded list of ARM fallbacks in the `platform.Only` implementation (by doing a descending loop over variant numbers instead, which is all the hard-coded list was doing). Making this a separate function can then more easily be recursive later for handling an `arm64`->`arm` fallback (or similar), but I think it makes the code a lot more clear too (so we're calculating a vector of platforms separately from building a matcher object). This also makes a minor adjustment in `TestImagePullWithDistSourceLabel` which had an implicit assumption that `platforms.Only` would only ever result in a single suitable manifest, which isn't strictly true (and is likely failing as-is when run on any 32bit `arm` system that's `v6` or higher, which this fixes 😅). Signed-off-by: Tianon Gravi --- image_test.go | 2 +- platforms/compare.go | 135 +++++++++++-------------------------------- 2 files changed, 34 insertions(+), 103 deletions(-) diff --git a/image_test.go b/image_test.go index 939e4b4d4..99aa44db8 100644 --- a/image_test.go +++ b/image_test.go @@ -113,7 +113,7 @@ func TestImagePullWithDistSourceLabel(t *testing.T) { key := fmt.Sprintf("containerd.io/distribution.source.%s", source) // only check the target platform - childrenHandler := images.FilterPlatforms(images.ChildrenHandler(cs), pMatcher) + childrenHandler := images.LimitManifests(images.ChildrenHandler(cs), pMatcher, 1) checkLabelHandler := func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) { children, err := childrenHandler(ctx, desc) diff --git a/platforms/compare.go b/platforms/compare.go index 3ad22a10d..2db6b7cb2 100644 --- a/platforms/compare.go +++ b/platforms/compare.go @@ -16,7 +16,12 @@ package platforms -import specs "github.com/opencontainers/image-spec/specs-go/v1" +import ( + "strconv" + "strings" + + specs "github.com/opencontainers/image-spec/specs-go/v1" +) // MatchComparer is able to match and compare platforms to // filter and sort platforms. @@ -26,103 +31,37 @@ type MatchComparer interface { Less(specs.Platform, specs.Platform) bool } -// Only returns a match comparer for a single platform -// using default resolution logic for the platform. -// -// For ARMv8, will also match ARMv7, ARMv6 and ARMv5 (for 32bit runtimes) -// For ARMv7, will also match ARMv6 and ARMv5 -// For ARMv6, will also match ARMv5 -func Only(platform specs.Platform) MatchComparer { - platform = Normalize(platform) - if platform.Architecture == "arm" { - if platform.Variant == "v8" { - return orderedPlatformComparer{ - matchers: []Matcher{ - &matcher{ - Platform: platform, - }, - &matcher{ - Platform: specs.Platform{ - Architecture: platform.Architecture, - OS: platform.OS, - OSVersion: platform.OSVersion, - OSFeatures: platform.OSFeatures, - Variant: "v7", - }, - }, - &matcher{ - Platform: specs.Platform{ - Architecture: platform.Architecture, - OS: platform.OS, - OSVersion: platform.OSVersion, - OSFeatures: platform.OSFeatures, - Variant: "v6", - }, - }, - &matcher{ - Platform: specs.Platform{ - Architecture: platform.Architecture, - OS: platform.OS, - OSVersion: platform.OSVersion, - OSFeatures: platform.OSFeatures, - Variant: "v5", - }, - }, - }, - } - } - if platform.Variant == "v7" { - return orderedPlatformComparer{ - matchers: []Matcher{ - &matcher{ - Platform: platform, - }, - &matcher{ - Platform: specs.Platform{ - Architecture: platform.Architecture, - OS: platform.OS, - OSVersion: platform.OSVersion, - OSFeatures: platform.OSFeatures, - Variant: "v6", - }, - }, - &matcher{ - Platform: specs.Platform{ - Architecture: platform.Architecture, - OS: platform.OS, - OSVersion: platform.OSVersion, - OSFeatures: platform.OSFeatures, - Variant: "v5", - }, - }, - }, - } - } - if platform.Variant == "v6" { - return orderedPlatformComparer{ - matchers: []Matcher{ - &matcher{ - Platform: platform, - }, - &matcher{ - Platform: specs.Platform{ - Architecture: platform.Architecture, - OS: platform.OS, - OSVersion: platform.OSVersion, - OSFeatures: platform.OSFeatures, - Variant: "v5", - }, - }, - }, +// platformVector returns an (ordered) vector of appropriate specs.Platform +// objects to try matching for the given platform object (see platforms.Only). +func platformVector(platform specs.Platform) []specs.Platform { + vector := []specs.Platform{platform} + + switch platform.Architecture { + case "arm": + if armVersion, err := strconv.Atoi(strings.TrimPrefix(platform.Variant, "v")); err == nil && armVersion > 5 { + for armVersion--; armVersion >= 5; armVersion-- { + vector = append(vector, specs.Platform{ + Architecture: platform.Architecture, + OS: platform.OS, + OSVersion: platform.OSVersion, + OSFeatures: platform.OSFeatures, + Variant: "v" + strconv.Itoa(armVersion), + }) } } } - return singlePlatformComparer{ - Matcher: &matcher{ - Platform: platform, - }, - } + return vector +} + +// Only returns a match comparer for a single platform +// using default resolution logic for the platform. +// +// For arm/v8, will also match arm/v7, arm/v6 and arm/v5 +// For arm/v7, will also match arm/v6 and arm/v5 +// For arm/v6, will also match arm/v5 +func Only(platform specs.Platform) MatchComparer { + return Ordered(platformVector(Normalize(platform))...) } // Ordered returns a platform MatchComparer which matches any of the platforms @@ -153,14 +92,6 @@ func Any(platforms ...specs.Platform) MatchComparer { // with preference for ordering. var All MatchComparer = allPlatformComparer{} -type singlePlatformComparer struct { - Matcher -} - -func (c singlePlatformComparer) Less(p1, p2 specs.Platform) bool { - return c.Match(p1) && !c.Match(p2) -} - type orderedPlatformComparer struct { matchers []Matcher }