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 <[email protected]>

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
 	}
 

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()

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())