From 6f769bc0e9879e981fc4b8bc0d474b7ad4049431 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 21 Jun 2021 10:51:51 +0200 Subject: [PATCH] create: support images with invalid platform Much to my regret, there is a number of images in the wild with invalid platforms breaking the platform checks in libimage that want to make sure that a local image is matching the expected platform. Imagine a `podman run --arch=arm64 fedora` with a local amd64 fedora image. We really shouldn't use the local one in this case and pull down the arm64 one. The strict platform checks in libimage in combination with invalid platforms in images surfaced in Podman being able to pull an image but failing to look it up in subsequent presence checks. A `podman run` would hence pull such an image but fail to create the container. Support images with invalid platforms by vendoring the latest HEAD from containers/common. Also remove the partially implemented pull-policy logic from Podman and let libimage handle that entirely. However, whenever --arch, --os or --platform are specified, the pull policy will be forced to "newer". This way, we pessimistically assume that the local image has an invalid platform and we reach out to the registry. If there's a newer image (i.e., one with a different digest), we'll pull it down. Please note that most of the logic has either already been implemented in libimage or been moved down which allows for removing some clutter from Podman. [NO TESTS NEEDED] since c/common has new tests. Podman can rely on the existing tests. Fixes: #10648 Fixes: #10682 Signed-off-by: Valentin Rothberg --- cmd/podman/containers/create.go | 58 ++++++-------------- pkg/api/handlers/compat/containers_create.go | 5 +- pkg/api/handlers/libpod/images.go | 2 +- pkg/api/handlers/libpod/images_pull.go | 26 ++++++--- pkg/api/handlers/libpod/manifests.go | 2 +- pkg/api/handlers/utils/images.go | 25 ++++----- pkg/api/server/register_images.go | 4 ++ pkg/bindings/images/pull.go | 10 ++-- pkg/bindings/images/types.go | 3 + pkg/bindings/images/types_pull_options.go | 16 ++++++ pkg/domain/infra/tunnel/images.go | 2 +- pkg/errorhandling/errorhandling.go | 9 ++- pkg/specgen/generate/container.go | 3 +- pkg/specgen/generate/container_create.go | 3 +- 14 files changed, 88 insertions(+), 80 deletions(-) diff --git a/cmd/podman/containers/create.go b/cmd/podman/containers/create.go index f06869c4eb..52cf8fea41 100644 --- a/cmd/podman/containers/create.go +++ b/cmd/podman/containers/create.go @@ -8,7 +8,6 @@ import ( "strings" "github.com/containers/common/pkg/config" - storageTransport "github.com/containers/image/v5/storage" "github.com/containers/image/v5/transports/alltransports" "github.com/containers/podman/v3/cmd/podman/common" "github.com/containers/podman/v3/cmd/podman/registry" @@ -16,9 +15,7 @@ import ( "github.com/containers/podman/v3/pkg/domain/entities" "github.com/containers/podman/v3/pkg/specgen" "github.com/containers/podman/v3/pkg/util" - "github.com/containers/storage" "github.com/pkg/errors" - "github.com/sirupsen/logrus" "github.com/spf13/cobra" ) @@ -238,30 +235,12 @@ func createInit(c *cobra.Command) error { return nil } -// TODO: we should let the backend take care of the pull policy (which it -// does!). The code below is at risk of causing regression and code divergence. func pullImage(imageName string) (string, error) { pullPolicy, err := config.ValidatePullPolicy(cliVals.Pull) if err != nil { 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() == storageTransport.Transport.Name(): - br, err := registry.ImageEngine().Exists(registry.GetContext(), imageName) - if err != nil { - return "", err - } - imageMissing = !br.Value - } - if cliVals.Platform != "" || cliVals.Arch != "" || cliVals.OS != "" { if cliVals.Platform != "" { if cliVals.Arch != "" || cliVals.OS != "" { @@ -273,31 +252,28 @@ func pullImage(imageName string) (string, error) { cliVals.Arch = split[1] } } + } - if pullPolicy != config.PullPolicyAlways { - logrus.Info("--platform --arch and --os causes the pull policy to be \"always\"") - pullPolicy = config.PullPolicyAlways - } + pullReport, pullErr := registry.ImageEngine().Pull(registry.GetContext(), imageName, entities.ImagePullOptions{ + Authfile: cliVals.Authfile, + Quiet: cliVals.Quiet, + Arch: cliVals.Arch, + OS: cliVals.OS, + Variant: cliVals.Variant, + SignaturePolicy: cliVals.SignaturePolicy, + PullPolicy: pullPolicy, + }) + if pullErr != nil { + return "", pullErr } - if imageMissing || pullPolicy == config.PullPolicyAlways { - if pullPolicy == config.PullPolicyNever { - return "", errors.Wrap(storage.ErrImageUnknown, imageName) - } - pullReport, pullErr := registry.ImageEngine().Pull(registry.GetContext(), imageName, entities.ImagePullOptions{ - Authfile: cliVals.Authfile, - Quiet: cliVals.Quiet, - Arch: cliVals.Arch, - OS: cliVals.OS, - Variant: cliVals.Variant, - SignaturePolicy: cliVals.SignaturePolicy, - PullPolicy: pullPolicy, - }) - if pullErr != nil { - return "", pullErr - } + // Return the input name such that the image resolves to correct + // repo/tag in the backend (see #8082). Unless we're referring to + // the image via a transport. + if _, err := alltransports.ParseImageName(imageName); err == nil { imageName = pullReport.Images[0] } + return imageName, nil } diff --git a/pkg/api/handlers/compat/containers_create.go b/pkg/api/handlers/compat/containers_create.go index 162a98135b..15eb86422d 100644 --- a/pkg/api/handlers/compat/containers_create.go +++ b/pkg/api/handlers/compat/containers_create.go @@ -71,13 +71,12 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { imgNameOrID := newImage.ID() // if the img had multi names with the same sha256 ID, should use the InputName, not the ID if len(newImage.Names()) > 1 { - imageRef, err := utils.ParseDockerReference(resolvedName) - if err != nil { + if err := utils.IsRegistryReference(resolvedName); err != nil { utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest, err) return } // maybe the InputName has no tag, so use full name to display - imgNameOrID = imageRef.DockerReference().String() + imgNameOrID = resolvedName } sg := specgen.NewSpecGenerator(imgNameOrID, cliOpts.RootFS) diff --git a/pkg/api/handlers/libpod/images.go b/pkg/api/handlers/libpod/images.go index a90408bfdd..fc6ab4b4c4 100644 --- a/pkg/api/handlers/libpod/images.go +++ b/pkg/api/handlers/libpod/images.go @@ -482,7 +482,7 @@ func PushImage(w http.ResponseWriter, r *http.Request) { destination = source } - if _, err := utils.ParseDockerReference(destination); err != nil { + if err := utils.IsRegistryReference(destination); err != nil { utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest, err) return } diff --git a/pkg/api/handlers/libpod/images_pull.go b/pkg/api/handlers/libpod/images_pull.go index 73d08a26e1..04b4156386 100644 --- a/pkg/api/handlers/libpod/images_pull.go +++ b/pkg/api/handlers/libpod/images_pull.go @@ -26,14 +26,16 @@ func ImagesPull(w http.ResponseWriter, r *http.Request) { runtime := r.Context().Value("runtime").(*libpod.Runtime) decoder := r.Context().Value("decoder").(*schema.Decoder) query := struct { - Reference string `schema:"reference"` - OS string `schema:"OS"` - Arch string `schema:"Arch"` - Variant string `schema:"Variant"` - TLSVerify bool `schema:"tlsVerify"` - AllTags bool `schema:"allTags"` + Reference string `schema:"reference"` + OS string `schema:"OS"` + Arch string `schema:"Arch"` + Variant string `schema:"Variant"` + TLSVerify bool `schema:"tlsVerify"` + AllTags bool `schema:"allTags"` + PullPolicy string `schema:"policy"` }{ - TLSVerify: true, + TLSVerify: true, + PullPolicy: "always", } if err := decoder.Decode(&query, r.URL.Query()); err != nil { @@ -48,7 +50,7 @@ func ImagesPull(w http.ResponseWriter, r *http.Request) { } // Make sure that the reference has no transport or the docker one. - if _, err := utils.ParseDockerReference(query.Reference); err != nil { + if err := utils.IsRegistryReference(query.Reference); err != nil { utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest, err) return } @@ -83,12 +85,18 @@ func ImagesPull(w http.ResponseWriter, r *http.Request) { pullOptions.Writer = writer + pullPolicy, err := config.ParsePullPolicy(query.PullPolicy) + if err != nil { + utils.Error(w, "failed to parse pull policy", http.StatusBadRequest, err) + return + } + var pulledImages []*libimage.Image var pullError error runCtx, cancel := context.WithCancel(r.Context()) go func() { defer cancel() - pulledImages, pullError = runtime.LibimageRuntime().Pull(runCtx, query.Reference, config.PullPolicyAlways, pullOptions) + pulledImages, pullError = runtime.LibimageRuntime().Pull(runCtx, query.Reference, pullPolicy, pullOptions) }() flush := func() { diff --git a/pkg/api/handlers/libpod/manifests.go b/pkg/api/handlers/libpod/manifests.go index f21eb2e80f..2f36db583a 100644 --- a/pkg/api/handlers/libpod/manifests.go +++ b/pkg/api/handlers/libpod/manifests.go @@ -169,7 +169,7 @@ func ManifestPush(w http.ResponseWriter, r *http.Request) { errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String())) return } - if _, err := utils.ParseDockerReference(query.Destination); err != nil { + if err := utils.IsRegistryReference(query.Destination); err != nil { utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest, err) return } diff --git a/pkg/api/handlers/utils/images.go b/pkg/api/handlers/utils/images.go index 2662cd368b..2a1908d63e 100644 --- a/pkg/api/handlers/utils/images.go +++ b/pkg/api/handlers/utils/images.go @@ -15,22 +15,19 @@ import ( "github.com/pkg/errors" ) -// ParseDockerReference parses the specified image name to a -// `types.ImageReference` and enforces it to refer to a docker-transport -// reference. -func ParseDockerReference(name string) (types.ImageReference, error) { - dockerPrefix := fmt.Sprintf("%s://", docker.Transport.Name()) +// IsRegistryReference checks if the specified name points to the "docker://" +// transport. If it points to no supported transport, we'll assume a +// non-transport reference pointing to an image (e.g., "fedora:latest"). +func IsRegistryReference(name string) error { imageRef, err := alltransports.ParseImageName(name) - if err == nil && imageRef.Transport().Name() != docker.Transport.Name() { - return nil, errors.Errorf("reference %q must be a docker reference", name) - } else if err != nil { - origErr := err - imageRef, err = alltransports.ParseImageName(fmt.Sprintf("%s%s", dockerPrefix, name)) - if err != nil { - return nil, errors.Wrapf(origErr, "reference %q must be a docker reference", name) - } + if err != nil { + // No supported transport -> assume a docker-stype reference. + return nil } - return imageRef, nil + if imageRef.Transport().Name() == docker.Transport.Name() { + return nil + } + return errors.Errorf("unsupport transport %s in %q: only docker transport is supported", imageRef.Transport().Name(), name) } // ParseStorageReference parses the specified image name to a diff --git a/pkg/api/server/register_images.go b/pkg/api/server/register_images.go index b32c0df206..2641809ee5 100644 --- a/pkg/api/server/register_images.go +++ b/pkg/api/server/register_images.go @@ -964,6 +964,10 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error { // description: Pull image for the specified variant. // type: string // - in: query + // name: policy + // description: Pull policy, "always" (default), "missing", "newer", "never". + // type: string + // - in: query // name: tlsVerify // description: Require TLS verification. // type: boolean diff --git a/pkg/bindings/images/pull.go b/pkg/bindings/images/pull.go index 9780c3bfff..7dfe9560c2 100644 --- a/pkg/bindings/images/pull.go +++ b/pkg/bindings/images/pull.go @@ -13,7 +13,7 @@ import ( "github.com/containers/podman/v3/pkg/auth" "github.com/containers/podman/v3/pkg/bindings" "github.com/containers/podman/v3/pkg/domain/entities" - "github.com/hashicorp/go-multierror" + "github.com/containers/podman/v3/pkg/errorhandling" "github.com/pkg/errors" ) @@ -65,7 +65,7 @@ func Pull(ctx context.Context, rawImage string, options *PullOptions) ([]string, dec := json.NewDecoder(response.Body) var images []string - var mErr error + var pullErrors []error for { var report entities.ImagePullReport if err := dec.Decode(&report); err != nil { @@ -77,7 +77,7 @@ func Pull(ctx context.Context, rawImage string, options *PullOptions) ([]string, select { case <-response.Request.Context().Done(): - return images, mErr + break default: // non-blocking select } @@ -86,7 +86,7 @@ func Pull(ctx context.Context, rawImage string, options *PullOptions) ([]string, case report.Stream != "": fmt.Fprint(stderr, report.Stream) case report.Error != "": - mErr = multierror.Append(mErr, errors.New(report.Error)) + pullErrors = append(pullErrors, errors.New(report.Error)) case len(report.Images) > 0: images = report.Images case report.ID != "": @@ -94,5 +94,5 @@ func Pull(ctx context.Context, rawImage string, options *PullOptions) ([]string, return images, errors.Errorf("failed to parse pull results stream, unexpected input: %v", report) } } - return images, mErr + return images, errorhandling.JoinErrors(pullErrors) } diff --git a/pkg/bindings/images/types.go b/pkg/bindings/images/types.go index 1f3e46729f..0aa75a81e1 100644 --- a/pkg/bindings/images/types.go +++ b/pkg/bindings/images/types.go @@ -147,6 +147,9 @@ type PullOptions struct { // OS will overwrite the local operating system (OS) for image // pulls. OS *string + // Policy is the pull policy. Supported values are "missing", "never", + // "newer", "always". An empty string defaults to "always". + Policy *string // Password for authenticating against the registry. Password *string // Quiet can be specified to suppress pull progress when pulling. Ignored diff --git a/pkg/bindings/images/types_pull_options.go b/pkg/bindings/images/types_pull_options.go index 0611c4447d..8fcf499eb9 100644 --- a/pkg/bindings/images/types_pull_options.go +++ b/pkg/bindings/images/types_pull_options.go @@ -84,6 +84,22 @@ func (o *PullOptions) GetOS() string { return *o.OS } +// WithPolicy +func (o *PullOptions) WithPolicy(value string) *PullOptions { + v := &value + o.Policy = v + return o +} + +// GetPolicy +func (o *PullOptions) GetPolicy() string { + var policy string + if o.Policy == nil { + return policy + } + return *o.Policy +} + // WithPassword func (o *PullOptions) WithPassword(value string) *PullOptions { v := &value diff --git a/pkg/domain/infra/tunnel/images.go b/pkg/domain/infra/tunnel/images.go index 3fd9a755d3..42027a2dc3 100644 --- a/pkg/domain/infra/tunnel/images.go +++ b/pkg/domain/infra/tunnel/images.go @@ -107,7 +107,7 @@ func (ir *ImageEngine) Pull(ctx context.Context, rawImage string, opts entities. options := new(images.PullOptions) options.WithAllTags(opts.AllTags).WithAuthfile(opts.Authfile).WithArch(opts.Arch).WithOS(opts.OS) options.WithVariant(opts.Variant).WithPassword(opts.Password) - options.WithQuiet(opts.Quiet).WithUsername(opts.Username) + options.WithQuiet(opts.Quiet).WithUsername(opts.Username).WithPolicy(opts.PullPolicy.String()) if s := opts.SkipTLSVerify; s != types.OptionalBoolUndefined { if s == types.OptionalBoolTrue { options.WithSkipTLSVerify(true) diff --git a/pkg/errorhandling/errorhandling.go b/pkg/errorhandling/errorhandling.go index 9b1740006f..6adbc9f344 100644 --- a/pkg/errorhandling/errorhandling.go +++ b/pkg/errorhandling/errorhandling.go @@ -15,6 +15,12 @@ func JoinErrors(errs []error) error { return nil } + // If there's just one error, return it. This prevents the "%d errors + // occurred:" header plus list from the multierror package. + if len(errs) == 1 { + return errs[0] + } + // `multierror` appends new lines which we need to remove to prevent // blank lines when printing the error. var multiE *multierror.Error @@ -24,9 +30,6 @@ func JoinErrors(errs []error) error { if finalErr == nil { return finalErr } - if len(multiE.WrappedErrors()) == 1 && logrus.IsLevelEnabled(logrus.TraceLevel) { - return multiE.WrappedErrors()[0] - } return errors.New(strings.TrimSpace(finalErr.Error())) } diff --git a/pkg/specgen/generate/container.go b/pkg/specgen/generate/container.go index d00e51e825..e7276892d8 100644 --- a/pkg/specgen/generate/container.go +++ b/pkg/specgen/generate/container.go @@ -24,7 +24,8 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat var inspectData *libimage.ImageData var err error if s.Image != "" { - newImage, _, err = r.LibimageRuntime().LookupImage(s.Image, nil) + lookupOptions := &libimage.LookupImageOptions{IgnorePlatform: true} + newImage, _, err = r.LibimageRuntime().LookupImage(s.Image, lookupOptions) if err != nil { return nil, err } diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 7682367b7c..e53032ebea 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -92,7 +92,8 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener options = append(options, libpod.WithRootFS(s.Rootfs)) } else { var resolvedImageName string - newImage, resolvedImageName, err = rt.LibimageRuntime().LookupImage(s.Image, nil) + lookupOptions := &libimage.LookupImageOptions{IgnorePlatform: true} + newImage, resolvedImageName, err = rt.LibimageRuntime().LookupImage(s.Image, lookupOptions) if err != nil { return nil, err }