imgpkg v0.40.0 breaks with relocated image

[mstergianis]

What steps did you take:

1. Create a bundle that references an image. It could be anything, I chose nginx for my example
2. Relocate (imgpkg copy) that bundle to a second private registry. The scenario I'm simulating here is moving something from an company's internal registry, to a company's public yet authenticated registry.
3. log out of the first registry. So as to simulate a customer that only has access to the public registry
4. imgpkg describe the image that is on the public registry

Here's a script that roughly sums up what I did to replicate this from scratch

mkdir -p bundle/.imgpkg/
cat >bundle/.imgpkg/images.yml <<EOF
apiVersion: imgpkg.carvel.dev/v1alpha1
images:
- annotations:
    kbld.carvel.dev/id: tdp-configurator
  image: FIRST-PRIVATE-REGISTRY/SOME-URI@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef

kind: ImagesLock
EOF

imgpkg push -b FIRST-PRIVATE-REGISTRY/SOME-BUNDLE-URI -f bundle/

imgpkg copy -b FIRST-PRIVATE-REGISTRY/SOME-BUNDLE-URI@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef --to-repo SECOND-PRIVATE-REGISTRY/SOME-BUNDLE-URI

docker logout FIRST-PRIVATE-REGISTRY

imgpkg describe -b SECOND-PRIVATE-REGISTRY/SOME-BUNDLE-URI@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
# imgpkg: Error: Error:
#   GET https://FIRST-PRIVATE-REGISTRY/v2/SOME-BUNDLE-URI/manifests/sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef:
#     UNAUTHORIZED: unauthorized to access repository: SOME-BUNDLE-URI, action: pull: unauthorized to access repository: SOME-BUNDLE-URI, action: pull in getting remote access of image FIRST-PRIVATE-REGISTRY/SOME-BUNDLE-URI@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef

What happened:
imgpkg describe fails because it tries to list the layers of an image it can't access

What did you expect:
imgpkg describe succeeds

Anything else you would like to add:

• I figure this is the fix (when I tested my limited example it worked) but I felt bad creating a PR that didn't improve the tests as well. When I timeboxed myself and tried to write a test I got lost in the FakeRegistry and how to describe this particular case.

  diff --git a/pkg/imgpkg/v1/describe.go b/pkg/imgpkg/v1/describe.go
  index ac0359ca..bc93e04f 100644
  --- a/pkg/imgpkg/v1/describe.go
  +++ b/pkg/imgpkg/v1/describe.go
  @@ -188,11 +188,11 @@ func (r *refWithDescription) describeBundleRec(visitedImgs map[string]refWithDes
   			desc.bundle.Content.Bundles[digest.DigestStr()] = bundleDesc
   		} else {
   			if ref.Error == "" {
  -				digest, err := name.NewDigest(ref.Image)
  +				digest, err := name.NewDigest(ref.PrimaryLocation())
   				if err != nil {
   					return desc.bundle, fmt.Errorf("Internal inconsistency: image %s should be fully resolved", ref.Image)
   				}
  -				layers, err = getImageLayersInfo(ref.Image)
  +				layers, err = getImageLayersInfo(ref.PrimaryLocation())
   				if err != nil {
   					return desc.bundle, err
   				}

• I find it a little bit odd that the default behaviour now includes fetching these layers. It roughly doubles the execution time of my example, and I'd imagine for many real world cases where a bundle contains more than one image it might be worse. Maybe consider putting the new feature behind an option?
• Cross linking Updated describe command to show layer's digest of each image #622

Environment:

• imgpkg version (use imgpkg --version):
  • imgpkg version 0.41.0 - broken
  • 0.40.0 - broken
  • 0.39.0 - working
• Docker registry used (e.g. Docker HUB): I used a harbor instance and azurecr in my example
• OS (e.g. from /etc/os-release): linux

---

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

[joaopapereira]

Great catch.
That bug has been around since 2022 😄 not sure how we did not catch that earlier.

I find it a little bit odd that the default behaviour now includes fetching these layers. It roughly doubles the execution time of my example, and I'd imagine for many real world cases where a bundle contains more than one image it might be worse. Maybe consider putting the new feature behind an option?

This sounds like an oversight. We also have a flag for cosign artifacts, which defaults to true. Would it be ok if we have a flag called --layers that also defaults to true, and you can set it to false?

When I timeboxed myself and tried to write a test I got lost in the FakeRegistry and how to describe this particular case.

This particular fake only supports one registry, which would make it complicated to test. The easier thing to do here is to create an e2e test where the setup might be easier.

[renuy]

PR merged