From 6b3823c3672d1fd4379e87ae92782236c6d41148 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 10 Jan 2022 15:13:30 +0100 Subject: [PATCH] libimage: refine pull-policy enforcement for custom platforms When pulling down an image with a user-specified custom platform, we try to make sure that user gets what they are asking for. An inherent issue with multi-arch images is that there are many images in the wild which do not get the platform right (see containers/podman/issues/10682). That means we need to pessimistically assume that the local image is wrong and pull the "correct" one down from the registry; in the worst case that is redundant work but we have a guarantee of correctness. Motivated by containers/podman/issues/12707 I had another look at the code and found some space for optimizations. Previously, we enforced the pull policy to "always" but that may be too aggressive since we may be running in an airgapped environment and the local image is correct. With this change, we enforce the pull policy to "newer" which makes errors non-fatal in case a local image has been found; this seems like a good middleground between making sure we are serving the "correct" image and user friendliness. Signed-off-by: Valentin Rothberg --- libimage/pull.go | 47 +++++++++++++++------------------------- libimage/pull_test.go | 30 +++++++++++++++++++++++++ libimage/runtime_test.go | 10 ++++++++- 3 files changed, 56 insertions(+), 31 deletions(-) diff --git a/libimage/pull.go b/libimage/pull.go index e9ba5293c..fa7bd28c5 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -458,37 +458,18 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str } } - customPlatform := false - if len(options.Architecture)+len(options.OS)+len(options.Variant) > 0 { - customPlatform = true - // Unless the pull policy is "always", we must pessimistically assume - // that the local image has an invalid architecture (see - // containers/podman/issues/10682). Hence, whenever the user requests - // a custom platform, set the pull policy to "always" to make sure - // we're pulling down the image. + customPlatform := len(options.Architecture)+len(options.OS)+len(options.Variant) > 0 + if customPlatform && pullPolicy != config.PullPolicyAlways && pullPolicy != config.PullPolicyNever { + // Unless the pull policy is always/never, we must + // pessimistically assume that the local image has an invalid + // architecture (see containers/podman/issues/10682). Hence, + // whenever the user requests a custom platform, set the pull + // policy to "newer" to make sure we're pulling down the + // correct image. // - // NOTE that this is will even override --pull={false,never}. This is - // very likely a bug but a consistent one in Podman/Buildah and should - // be addressed at a later point. - if pullPolicy != config.PullPolicyAlways && pullPolicy != config.PullPolicyNever { - switch { - // User input clearly refer to a local image. - case strings.HasPrefix(imageName, "localhost/"): - logrus.Debugf("Enforcing pull policy to %q to support custom platform (arch: %q, os: %q, variant: %q)", "never", options.Architecture, options.OS, options.Variant) - pullPolicy = config.PullPolicyNever - - // Image resolved to a local one, so let's still have a - // look at the registries or aliases but use it - // otherwise. - case strings.HasPrefix(resolvedImageName, "localhost/"): - logrus.Debugf("Enforcing pull policy to %q to support custom platform (arch: %q, os: %q, variant: %q)", "newer", options.Architecture, options.OS, options.Variant) - pullPolicy = config.PullPolicyNewer - - default: - logrus.Debugf("Enforcing pull policy to %q to support custom platform (arch: %q, os: %q, variant: %q)", "always", options.Architecture, options.OS, options.Variant) - pullPolicy = config.PullPolicyAlways - } - } + // NOTE that this is will even override --pull={false,never}. + pullPolicy = config.PullPolicyNewer + logrus.Debugf("Enforcing pull policy to %q to pull custom platform (arch: %q, os: %q, variant: %q) - local image may mistakenly specify wrong platform", pullPolicy, options.Architecture, options.OS, options.Variant) } if pullPolicy == config.PullPolicyNever { @@ -534,6 +515,12 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str sys := r.systemContextCopy() resolved, err := shortnames.Resolve(sys, imageName) if err != nil { + // TODO: that is a too big of a hammer since we should only + // ignore errors that indicate that there's no alias and no + // USRs. Must be addressed in c/image first. + if localImage != nil && pullPolicy == config.PullPolicyNewer { + return []string{resolvedImageName}, nil + } return nil, err } diff --git a/libimage/pull_test.go b/libimage/pull_test.go index 1aebd35f4..1a48e0201 100644 --- a/libimage/pull_test.go +++ b/libimage/pull_test.go @@ -128,6 +128,36 @@ func TestPullPlatforms(t *testing.T) { require.Len(t, pulledImages, 1) } +func TestPullPlatformsWithEmptyRegistriesConf(t *testing.T) { + runtime, cleanup := testNewRuntime(t, testNewRuntimeOptions{registriesConfPath: "/dev/null"}) + defer cleanup() + ctx := context.Background() + pullOptions := &PullOptions{} + pullOptions.Writer = os.Stdout + + localArch := goruntime.GOARCH + localOS := goruntime.GOOS + + imageName := "quay.io/libpod/busybox" + newTag := "crazy:train" + + pulledImages, err := runtime.Pull(ctx, imageName, config.PullPolicyAlways, pullOptions) + require.NoError(t, err, "pull "+imageName) + require.Len(t, pulledImages, 1) + + err = pulledImages[0].Tag(newTag) + require.NoError(t, err, "tag") + + // See containers/podman/issues/12707: a custom platform will enforce + // pulling via newer. Older versions enforced always which can lead to + // errors. + pullOptions.OS = localOS + pullOptions.Architecture = localArch + pulledImages, err = runtime.Pull(ctx, newTag, config.PullPolicyMissing, pullOptions) + require.NoError(t, err, "pull "+newTag) + require.Len(t, pulledImages, 1) +} + func TestPullPolicy(t *testing.T) { runtime, cleanup := testNewRuntime(t) defer cleanup() diff --git a/libimage/runtime_test.go b/libimage/runtime_test.go index f8e679cfc..e24552cad 100644 --- a/libimage/runtime_test.go +++ b/libimage/runtime_test.go @@ -18,10 +18,14 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +type testNewRuntimeOptions struct { + registriesConfPath string +} + // Create a new Runtime that can be used for testing. The second return value // is a clean-up function that should be called by users to make sure all // temporary test data gets removed. -func testNewRuntime(t *testing.T) (runtime *Runtime, cleanup func()) { +func testNewRuntime(t *testing.T, options ...testNewRuntimeOptions) (runtime *Runtime, cleanup func()) { workdir, err := ioutil.TempDir("", "testStorageRuntime") require.NoError(t, err) storeOptions := &storage.StoreOptions{ @@ -36,6 +40,10 @@ func testNewRuntime(t *testing.T) (runtime *Runtime, cleanup func()) { SystemRegistriesConfDirPath: "/dev/null", } + if len(options) == 1 && options[0].registriesConfPath != "" { + systemContext.SystemRegistriesConfPath = options[0].registriesConfPath + } + runtime, err = RuntimeFromStoreOptions(&RuntimeOptions{SystemContext: systemContext}, storeOptions) require.NoError(t, err) require.Equal(t, runtime.systemContext.BigFilesTemporaryDir, tmpdir())