Merge pull request #880 from vrothberg/platform-pull-policy

libimage: refine pull-policy enforcement for custom platforms

libimage/pull.go

@@ -464,37 +464,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 {
@@ -540,6 +521,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

@@ -131,6 +131,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())