From 82c6ef56d9d7076b4d02a6f4b75e54eaf3815979 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 18 Jun 2021 14:45:29 +0200 Subject: [PATCH 1/3] libimage: pull: ignore platform for local image lookup We must ignore the platform of a local image when doing lookups. Some images set an incorrect or even invalid platform (see containers/podman/issues/10682). Doing the lookup while ignoring the platform checks prevents redundantly downloading the same image. Note that this has the consequence that a `--pull-never --arch=hurz` may chose a local image of another architecture. However, I estimate the benefit of continuing to allow potentially invalid images higher than not running them (and breaking workloads). The changes required to touch the corrupted checks. I used the occasion to make the corrupted checks a bit cheaper. Signed-off-by: Valentin Rothberg --- libimage/corrupted_test.go | 5 ++++- libimage/image.go | 18 ++++++++++++++++++ libimage/pull.go | 19 +++++++++++++++---- libimage/runtime.go | 5 ++--- 4 files changed, 39 insertions(+), 8 deletions(-) diff --git a/libimage/corrupted_test.go b/libimage/corrupted_test.go index b39473ea4..e683c1fa3 100644 --- a/libimage/corrupted_test.go +++ b/libimage/corrupted_test.go @@ -64,6 +64,9 @@ func TestCorruptedImage(t *testing.T) { _, err = image.Inspect(ctx, false) require.Error(t, err, "inspecting corrupted image should fail") + err = image.isCorrupted(imageName) + require.Error(t, err, "image is corrupted") + exists, err = runtime.Exists(imageName) require.NoError(t, err, "corrupted image exists should not fail") require.False(t, exists, "corrupted image should not be marked to exist") @@ -75,7 +78,7 @@ func TestCorruptedImage(t *testing.T) { require.Len(t, pulledImages, 1) image = pulledImages[0] - // Inpsecting a repaired image should work. + // Inspecting a repaired image should work. _, err = image.Inspect(ctx, false) require.NoError(t, err, "inspecting repaired image should work") } diff --git a/libimage/image.go b/libimage/image.go index 3bcdbabec..f1272f507 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -61,6 +61,24 @@ func (i *Image) reload() error { return nil } +// isCorrupted returns an error if the image may be corrupted. +func (i *Image) isCorrupted(name string) error { + // If it's a manifest list, we're good for now. + if _, err := i.getManifestList(); err == nil { + return nil + } + + ref, err := i.StorageReference() + if err != nil { + return err + } + + if _, err := ref.NewImage(context.Background(), nil); err != nil { + return errors.Errorf("Image %s exists in local storage but may be corrupted: %v", name, err) + } + return nil +} + // Names returns associated names with the image which may be a mix of tags and // digests. func (i *Image) Names() []string { diff --git a/libimage/pull.go b/libimage/pull.go index 22cb1a567..0a6dd51e6 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -333,7 +333,7 @@ func (r *Runtime) copyFromRegistry(ctx context.Context, ref types.ImageReference // from a registry. On successful pull it returns the used fully-qualified // name that can later be used to look up the image in the local containers // storage. -func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName string, pullPolicy config.PullPolicy, options *PullOptions) ([]string, error) { +func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName string, pullPolicy config.PullPolicy, options *PullOptions) ([]string, error) { //nolint:gocyclo // Sanity check. if err := pullPolicy.Validate(); err != nil { return nil, err @@ -350,15 +350,26 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str // If there's already a local image "localhost/foo", then we should // attempt pulling that instead of doing the full short-name dance. lookupOptions := &LookupImageOptions{ - Architecture: options.Architecture, - OS: options.OS, - Variant: options.Variant, + // NOTE: we must ignore the platform of a local image when + // doing lookups. Some images set an incorrect or even invalid + // platform (see containers/podman/issues/10682). Doing the + // lookup while ignoring the platform checks prevents + // redundantly downloading the same image. + IgnorePlatform: true, } localImage, resolvedImageName, err = r.LookupImage(imageName, lookupOptions) if err != nil && errors.Cause(err) != storage.ErrImageUnknown { logrus.Errorf("Looking up %s in local storage: %v", imageName, err) } + // If the local image is corrupted, we need to repull it. + if localImage != nil { + if err := localImage.isCorrupted(imageName); err != nil { + logrus.Error(err) + localImage = nil + } + } + if pullPolicy == config.PullPolicyNever { if localImage != nil { logrus.Debugf("Pull policy %q but no local image has been found for %s", pullPolicy, imageName) diff --git a/libimage/runtime.go b/libimage/runtime.go index 58ac5864a..3cbd3dcf4 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -144,9 +144,8 @@ func (r *Runtime) Exists(name string) (bool, error) { if image == nil { return false, nil } - // Inspect the image to make sure if it's corrupted or not. - if _, err := image.Inspect(context.Background(), false); err != nil { - logrus.Errorf("Image %s exists in local storage but may be corrupted: %v", name, err) + if err := image.isCorrupted(name); err != nil { + logrus.Error(err) return false, nil } return true, nil From d3d122312790178e77608675cccc55a78b042969 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 21 Jun 2021 11:19:16 +0200 Subject: [PATCH 2/3] libimage: pull: enforce pull policy for custom platforms Enforce the pull policy to always if a custom platform is requested by the user. Some images ship with invalid platforms which we must pessimistically assume, see containers/podman/issues/10682. Signed-off-by: Valentin Rothberg --- libimage/pull.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/libimage/pull.go b/libimage/pull.go index 0a6dd51e6..9f40cf70b 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -379,6 +379,16 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str return nil, errors.Wrap(storage.ErrImageUnknown, imageName) } + // 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. + if pullPolicy != config.PullPolicyAlways && len(options.Architecture)+len(options.OS)+len(options.Variant) > 0 { + 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 + } + if pullPolicy == config.PullPolicyMissing && localImage != nil { return []string{resolvedImageName}, nil } From f38374fa4c60ebabfe8e9ad664fe630685f7f96e Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 21 Jun 2021 15:01:03 +0200 Subject: [PATCH 3/3] libimage: pull: override even --pull=never with custom platform As it turned out in Podman CI (containers/podman/pull/10739), the policy is overridden via --arch/os/platform/variant even when the policy is set to never. While I think this is a bug, it is a separate one and must tackled separately. Signed-off-by: Valentin Rothberg --- libimage/pull.go | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/libimage/pull.go b/libimage/pull.go index 9f40cf70b..0a5e49fd2 100644 --- a/libimage/pull.go +++ b/libimage/pull.go @@ -105,6 +105,20 @@ func (r *Runtime) Pull(ctx context.Context, name string, pullPolicy config.PullP r.writeEvent(&Event{ID: "", Name: name, Time: time.Now(), Type: EventTypeImagePull}) } + // Some callers may set the platform via the system context at creation + // time of the runtime. We need this information to decide whether we + // need to enforce pulling from a registry (see + // containers/podman/issues/10682). + if options.Architecture == "" { + options.Architecture = r.systemContext.ArchitectureChoice + } + if options.OS == "" { + options.OS = r.systemContext.OSChoice + } + if options.Variant == "" { + options.Variant = r.systemContext.VariantChoice + } + var ( pulledImages []string pullError error @@ -370,25 +384,29 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str } } - if pullPolicy == config.PullPolicyNever { - if localImage != nil { - logrus.Debugf("Pull policy %q but no local image has been found for %s", pullPolicy, imageName) - return []string{resolvedImageName}, nil - } - logrus.Debugf("Pull policy %q and %s resolved to local image %s", pullPolicy, imageName, resolvedImageName) - return nil, errors.Wrap(storage.ErrImageUnknown, imageName) - } - // 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. + // + // 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 && len(options.Architecture)+len(options.OS)+len(options.Variant) > 0 { 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 } + if pullPolicy == config.PullPolicyNever { + if localImage != nil { + logrus.Debugf("Pull policy %q but no local image has been found for %s", pullPolicy, imageName) + return []string{resolvedImageName}, nil + } + logrus.Debugf("Pull policy %q and %s resolved to local image %s", pullPolicy, imageName, resolvedImageName) + return nil, errors.Wrap(storage.ErrImageUnknown, imageName) + } + if pullPolicy == config.PullPolicyMissing && localImage != nil { return []string{resolvedImageName}, nil }