From f497feb13a5612379ae118bf2b18781cea339b2a Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 31 May 2022 16:41:10 +0200 Subject: [PATCH] libimage: image lookup: check platform Check the platform when looking up images locally. When the user requested a custom platform and a local image doesn't match, the image will be discarded. Otherwise a warning will be emitted. Also refactor the code to make it more maintainable in the future. Fixes: containers/podman/issues/12682 Signed-off-by: Valentin Rothberg --- libimage/inspect.go | 2 +- libimage/platform.go | 53 ++++++++++++++++++++++++++++++ libimage/platform_test.go | 20 ++++++++++++ libimage/pull.go | 25 ++++++++++---- libimage/runtime.go | 69 ++++++++------------------------------- 5 files changed, 106 insertions(+), 63 deletions(-) create mode 100644 libimage/platform.go create mode 100644 libimage/platform_test.go diff --git a/libimage/inspect.go b/libimage/inspect.go index ae06acd2c..5da8df1bf 100644 --- a/libimage/inspect.go +++ b/libimage/inspect.go @@ -216,7 +216,7 @@ func (i *Image) inspectInfo(ctx context.Context) (*types.ImageInspectInfo, error return nil, err } - img, err := ref.NewImage(ctx, i.runtime.systemContextCopy()) + img, err := ref.NewImage(ctx, &i.runtime.systemContext) if err != nil { return nil, err } diff --git a/libimage/platform.go b/libimage/platform.go new file mode 100644 index 000000000..c5e192a93 --- /dev/null +++ b/libimage/platform.go @@ -0,0 +1,53 @@ +package libimage + +import ( + "context" + "fmt" + "runtime" +) + +func toPlatformString(architecture, os, variant string) string { + if variant == "" { + return fmt.Sprintf("%s/%s", os, architecture) + } + return fmt.Sprintf("%s/%s/%s", os, architecture, variant) +} + +// Checks whether the image matches the specified platform. +// Returns +// * 1) a matching error that can be used for logging (or returning) what does not match +// * 2) a bool indicating whether architecture, os or variant were set (some callers need that to decide whether they need to throw an error) +// * 3) a fatal error that occured prior to check for matches (e.g., storage errors etc.) +func (i *Image) matchesPlatform(ctx context.Context, architecture, os, variant string) (error, bool, error) { + customPlatform := len(architecture)+len(os)+len(variant) != 0 + + if len(architecture) == 0 { + architecture = runtime.GOARCH + } + if len(os) == 0 { + os = runtime.GOOS + } + + inspectInfo, err := i.inspectInfo(ctx) + if err != nil { + return nil, customPlatform, fmt.Errorf("inspecting image: %w", err) + } + + matches := true + switch { + case architecture != inspectInfo.Architecture: + matches = false + case os != inspectInfo.Os: + matches = false + case variant != "" && variant != inspectInfo.Variant: + matches = false + } + + if matches { + return nil, customPlatform, nil + } + + imagePlatform := toPlatformString(inspectInfo.Architecture, inspectInfo.Os, inspectInfo.Variant) + expectedPlatform := toPlatformString(architecture, os, variant) + return fmt.Errorf("image platform (%s) does not match the expected platform (%s)", imagePlatform, expectedPlatform), customPlatform, nil +} diff --git a/libimage/platform_test.go b/libimage/platform_test.go new file mode 100644 index 000000000..8b93a75c0 --- /dev/null +++ b/libimage/platform_test.go @@ -0,0 +1,20 @@ +package libimage + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestToPlatformString(t *testing.T) { + for _, test := range []struct { + arch, os, variant, expected string + }{ + {"a", "b", "", "b/a"}, + {"a", "b", "c", "b/a/c"}, + {"", "", "c", "//c"}, // callers are responsible for the input + } { + platform := toPlatformString(test.arch, test.os, test.variant) + require.Equal(t, platform, test.expected) + } +} diff --git a/libimage/pull.go b/libimage/pull.go index 21c33fb2b..5e743574c 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -160,20 +160,31 @@ func (r *Runtime) Pull(ctx context.Context, name string, pullPolicy config.PullP } localImages := []*Image{} - lookupOptions := &LookupImageOptions{Architecture: options.Architecture, OS: options.OS, Variant: options.Variant} for _, name := range pulledImages { - local, _, err := r.LookupImage(name, nil) + image, _, err := r.LookupImage(name, nil) if err != nil { return nil, errors.Wrapf(err, "error locating pulled image %q name in containers storage", name) } - ref, err := local.StorageReference() + + // Note that we can ignore the 2nd return value here. Some + // images may ship with "wrong" platform, but we already warn + // about it. Throwing an error is not (yet) the plan. + matchError, _, err := image.matchesPlatform(ctx, options.Architecture, options.OS, options.Variant) if err != nil { - return nil, fmt.Errorf("creating storage reference for pulled image %q: %w", name, err) + return nil, fmt.Errorf("checking platform of image %s: %w", name, err) } - if _, err := r.imageReferenceMatchesContext(ref, name, lookupOptions, options.Writer); err != nil { - return nil, fmt.Errorf("checking platform for pulled image %q: %w", name, err) + + // If the image does not match the expected/requested platform, + // make sure to leave some breadcrumbs for the user. + if matchError != nil { + if options.Writer == nil { + logrus.Warnf("%v", matchError) + } else { + fmt.Fprintf(options.Writer, "WARNING: %v\n", matchError) + } } - localImages = append(localImages, local) + + localImages = append(localImages, image) } return localImages, pullError diff --git a/libimage/runtime.go b/libimage/runtime.go index d98ecf5cc..c2f2c9d6d 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -3,7 +3,6 @@ package libimage import ( "context" "fmt" - "io" "os" "strings" @@ -379,21 +378,24 @@ func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *Loo image = instance } - matches, err := r.imageReferenceMatchesContext(ref, name, options, nil) - if err != nil { - return nil, err - } - - // NOTE: if the user referenced by ID we must optimistically assume - // that they know what they're doing. Given, we already did the - // manifest limbo above, we may already have resolved it. - if !matches && !strings.HasPrefix(image.ID(), candidate) { - return nil, nil - } // Also print the string within the storage transport. That may aid in // debugging when using additional stores since we see explicitly where // the store is and which driver (options) are used. logrus.Debugf("Found image %q as %q in local containers storage (%s)", name, candidate, ref.StringWithinTransport()) + + // Ignore the (fatal) error since the image may be corrupted, which + // will bubble up at other places. During lookup, we just return it as + // is. + if matchError, customPlatform, _ := image.matchesPlatform(context.Background(), options.Architecture, options.OS, options.Variant); matchError != nil { + if customPlatform { + logrus.Debugf("%v", matchError) + // Return nil if the user clearly requested a custom + // platform and the located image does not match. + return nil, nil + } + logrus.Warnf("%v", matchError) + } + return image, nil } @@ -498,49 +500,6 @@ func (r *Runtime) ResolveName(name string) (string, error) { return normalized.String(), nil } -// imageReferenceMatchesContext return true if the specified reference matches -// the platform (os, arch, variant) as specified by the lookup options. -func (r *Runtime) imageReferenceMatchesContext(ref types.ImageReference, name string, options *LookupImageOptions, writer io.Writer) (bool, error) { - if options.Architecture+options.OS+options.Variant == "" { - return true, nil - } - - ctx := context.Background() - img, err := ref.NewImage(ctx, &r.systemContext) - if err != nil { - return false, err - } - defer img.Close() - data, err := img.Inspect(ctx) - if err != nil { - return false, err - } - - writeMessage := func(msg string) { - if writer == nil { - logrus.Warn(msg) - } else { - fmt.Fprintf(writer, "WARNING: %s\n", msg) - } - } - - matches := true - if options.Architecture != "" && options.Architecture != data.Architecture { - writeMessage(fmt.Sprintf("requested architecture %q does not match architecture %q of image %s", options.Architecture, data.Architecture, name)) - matches = false - } - if options.OS != "" && options.OS != data.Os { - writeMessage(fmt.Sprintf("requested OS %q does not match OS %q of image %s", options.OS, data.Os, name)) - matches = false - } - if options.Variant != "" && options.Variant != data.Variant { - writeMessage(fmt.Sprintf("requested variant %q does not match variant %q of image %s", options.Variant, data.Variant, name)) - matches = false - } - - return matches, nil -} - // IsExternalContainerFunc allows for checking whether the specified container // is an external one. The definition of an external container can be set by // callers.