pull: account for platform

When pulling an image, account for the specified OS, architecture and variant when looking up local images. While a local image may be found based on the specified name, the platform may be different from what the user desires. In that case, do not use the local image but continue pulling. Also remove pull-policy logic from the client. That'll reduce one roundtrip for the remote client and reduces code scattering. The backend should be the single source of truth for pull-policy handling. Fixes: containers#8001 Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg · Nov 24, 2020 · 6aa9622 · 6aa9622
1 parent 4fe7c3f
commit 6aa9622
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 46 deletions.
diff --git a/cmd/podman/containers/create.go b/cmd/podman/containers/create.go
@@ -8,12 +8,9 @@ import (
 	"strings"
 
 	"github.com/containers/common/pkg/config"
-	"github.com/containers/image/v5/storage"
-	"github.com/containers/image/v5/transports/alltransports"
 	"github.com/containers/podman/v2/cmd/podman/common"
 	"github.com/containers/podman/v2/cmd/podman/registry"
 	"github.com/containers/podman/v2/cmd/podman/utils"
-	"github.com/containers/podman/v2/libpod/define"
 	"github.com/containers/podman/v2/pkg/domain/entities"
 	"github.com/containers/podman/v2/pkg/specgen"
 	"github.com/containers/podman/v2/pkg/util"
@@ -220,41 +217,19 @@ func pullImage(imageName string) (string, error) {
 		return "", err
 	}
 
-	// Check if the image is missing and hence if we need to pull it.
-	imageMissing := true
-	imageRef, err := alltransports.ParseImageName(imageName)
-	switch {
-	case err != nil:
-		// Assume we specified a local image without the explicit storage transport.
-		fallthrough
-
-	case imageRef.Transport().Name() == storage.Transport.Name():
-		br, err := registry.ImageEngine().Exists(registry.GetContext(), imageName)
-		if err != nil {
-			return "", err
-		}
-		imageMissing = !br.Value
-	}
-
-	if imageMissing || pullPolicy == config.PullImageAlways {
-		if pullPolicy == config.PullImageNever {
-			return "", errors.Wrapf(define.ErrNoSuchImage, "unable to find a name and tag match for %s in repotags", imageName)
-		}
-		pullReport, pullErr := registry.ImageEngine().Pull(registry.GetContext(), imageName, entities.ImagePullOptions{
-			Authfile:        cliVals.Authfile,
-			Quiet:           cliVals.Quiet,
-			OverrideArch:    cliVals.OverrideArch,
-			OverrideOS:      cliVals.OverrideOS,
-			OverrideVariant: cliVals.OverrideVariant,
-			SignaturePolicy: cliVals.SignaturePolicy,
-			PullPolicy:      pullPolicy,
-		})
-		if pullErr != nil {
-			return "", pullErr
-		}
-		imageName = pullReport.Images[0]
+	pullReport, pullErr := registry.ImageEngine().Pull(registry.GetContext(), imageName, entities.ImagePullOptions{
+		Authfile:        cliVals.Authfile,
+		Quiet:           cliVals.Quiet,
+		OverrideArch:    cliVals.OverrideArch,
+		OverrideOS:      cliVals.OverrideOS,
+		OverrideVariant: cliVals.OverrideVariant,
+		SignaturePolicy: cliVals.SignaturePolicy,
+		PullPolicy:      pullPolicy,
+	})
+	if pullErr != nil {
+		return "", pullErr
 	}
-	return imageName, nil
+	return pullReport.Images[0], nil
 }
 
 // createPodIfNecessary automatically creates a pod when requested.  if the pod name

diff --git a/libpod/image/image.go b/libpod/image/image.go
@@ -142,6 +142,25 @@ func (ir *Runtime) NewFromLocal(name string) (*Image, error) {
 	return ir.newImage(updatedInputName, localImage), nil
 }
 
+// matchesPlatform returns true if the image matches the specified os, arch and
+// variant.  Note that os, arch and variant are ignored if they are empty.
+func (i *Image) matchesPlatform(ctx context.Context, os, arch, variant string) (bool, error) {
+	data, err := i.inspect(ctx, false)
+	if err != nil {
+		return false, errors.Wrap(err, "error determining image's platform")
+	}
+	if os != "" && data.Os != os {
+		return false, nil
+	}
+	if arch != "" && data.Architecture != arch {
+		return false, nil
+	}
+	if variant != "" && data.Variant != variant {
+		return false, nil
+	}
+	return true, nil
+}
+
 // New creates a new image object where the image could be local
 // or remote
 func (ir *Runtime) New(ctx context.Context, name, signaturePolicyPath, authfile string, writer io.Writer, dockeroptions *DockerRegistryOptions, signingoptions SigningOptions, label *string, pullType util.PullType) (*Image, error) {
@@ -153,8 +172,24 @@ func (ir *Runtime) New(ctx context.Context, name, signaturePolicyPath, authfile
 	if pullType != util.PullImageAlways {
 		newImage, err := ir.NewFromLocal(name)
 		if err == nil {
-			return newImage, nil
-		} else if pullType == util.PullImageNever {
+			// Make sure that the local image matches the
+			// user-specified OS and arch.  In case of a
+			// mismatch, continue and attempt to pull the
+			// image.
+			var os, arch, variant string
+			if dockeroptions != nil {
+				os = dockeroptions.OSChoice
+				arch = dockeroptions.ArchitectureChoice
+				variant = dockeroptions.VariantChoice
+			}
+			matches, err := newImage.matchesPlatform(ctx, os, arch, variant)
+			if matches || err != nil {
+				return newImage, err
+			}
+			logrus.Debugf("Found local image %q for %q but it does not match the platform (os: %q, arch:%q)", newImage.ID(), name, os, arch)
+		}
+
+		if pullType == util.PullImageNever {
 			return nil, err
 		}
 	}
@@ -1237,7 +1272,7 @@ func (i *Image) inspect(ctx context.Context, calculateSize bool) (*inspect.Image
 		return nil, err
 	}
 
-	_, manifestType, err := i.GetManifest(ctx, nil)
+	manifestData, manifestType, err := i.GetManifest(ctx, nil)
 	if err != nil {
 		return nil, errors.Wrapf(err, "unable to determine manifest type")
 	}
@@ -1272,14 +1307,26 @@ func (i *Image) inspect(ctx context.Context, calculateSize bool) (*inspect.Image
 		History:      ociv1Img.History,
 		NamesHistory: i.NamesHistory(),
 	}
-	if manifestType == manifest.DockerV2Schema2MediaType {
-		hc, err := i.GetHealthCheck(ctx)
+	switch manifestType {
+	case manifest.DockerV2Schema2MediaType:
+		v2Image, err := i.GetConfigBlob(ctx)
 		if err != nil {
 			return nil, err
 		}
-		if hc != nil {
+		if hc := v2Image.ContainerConfig.Healthcheck; hc != nil {
 			data.HealthCheck = hc
 		}
+		data.Variant = v2Image.Variant
+
+	case ociv1.MediaTypeImageManifest:
+		var m ociv1.Manifest
+		if err := json.Unmarshal(manifestData, &m); err != nil {
+			return nil, err
+		}
+		if m.Config.Platform != nil {
+			data.Variant = m.Config.Platform.Variant
+		}
+	default:
 	}
 	return data, nil
 }

diff --git a/pkg/inspect/inspect.go b/pkg/inspect/inspect.go
@@ -22,6 +22,7 @@ type ImageData struct {
 	Version      string                        `json:"Version"`
 	Author       string                        `json:"Author"`
 	Architecture string                        `json:"Architecture"`
+	Variant      string                        `json:"Variant"`
 	Os           string                        `json:"Os"`
 	Size         int64                         `json:"Size"`
 	VirtualSize  int64                         `json:"VirtualSize"`

diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go
@@ -88,7 +88,7 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener
 	if s.Rootfs != "" {
 		options = append(options, libpod.WithRootFS(s.Rootfs))
 	} else {
-		newImage, err = rt.ImageRuntime().NewFromLocal(s.Image)
+		newImage, err = rt.ImageRuntime().NewFromLocal(s.RawImageName)
 		if err != nil {
 			return nil, err
 		}
@@ -97,7 +97,7 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener
 		// defaulting to the first name or an empty one if no names are
 		// present.
 		imgName := newImage.InputName
-		if s.Image == newImage.InputName && strings.HasPrefix(newImage.ID(), s.Image) {
+		if strings.HasPrefix(newImage.ID(), s.RawImageName) {
 			imgName = ""
 			names := newImage.Names()
 			if len(names) > 0 {

diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go
@@ -6,6 +6,7 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strconv"
 	"strings"
 	"syscall"
@@ -14,6 +15,7 @@ import (
 	. "github.com/containers/podman/v2/test/utils"
 	"github.com/containers/storage/pkg/stringid"
 	"github.com/mrunalp/fileutils"
+	"github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
@@ -47,6 +49,22 @@ var _ = Describe("Podman run", func() {
 		Expect(session.ExitCode()).To(Equal(0))
 	})
 
+	It("podman run a container based on local image but with --override-arch", func() {
+		if runtime.GOARCH == "arm64" {
+			ginkgo.Skip("test does not run on arm64")
+		}
+		session := podmanTest.Podman([]string{"run", "--override-arch", "arm64", ALPINE, "ls"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(1))
+	})
+
+	It("podman run a container based on local image but with --override-os", func() {
+		session := podmanTest.Podman([]string{"run", "--override-os", "bogus", ALPINE, "ls"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(125))
+		Expect(session.ErrorToString()).To(ContainSubstring("Error choosing an image from manifest list"))
+	})
+
 	It("podman run a container based on a complex local image name", func() {
 		imageName := strings.TrimPrefix(nginx, "quay.io/")
 		session := podmanTest.Podman([]string{"run", imageName, "ls"})

diff --git a/test/system/030-run.bats b/test/system/030-run.bats
@@ -142,7 +142,7 @@ echo $rand        |   0 | $rand
     NONLOCAL_IMAGE="$PODMAN_TEST_IMAGE_REGISTRY/$PODMAN_TEST_IMAGE_USER/$PODMAN_TEST_IMAGE_NAME:00000000"
 
     run_podman 125 run --pull=never $NONLOCAL_IMAGE true
-    is "$output" "Error: unable to find a name and tag match for $NONLOCAL_IMAGE in repotags: no such image" "--pull=never [with image not present]: error"
+    is "$output" "Error: unable to find $NONLOCAL_IMAGE in local storage: no such image" "--pull=never [with image not present]: error"
 
     run_podman run --pull=missing $NONLOCAL_IMAGE true
     is "$output" "Trying to pull .*" "--pull=missing [with image NOT PRESENT]: fetches"