Repository: vmware-tanzu/carvel-imgpkg (go)

Issue:
`describe` command should fetch all the information possible and display failures in the end
**Describe the problem/challenge you have**
Sometimes when trying to get more information about a Bundle an error can occur while talking with the registry or the user might not have access to some particular Image. Today, when `imgpkg` fails to retrieve information about some image it displays the error message and stops processing. Nevertheless, `imgpkg` could still provide information about the rest of the Images that are part of the Bundle. 


**Describe the solution you'd like**

When running the command `imgpkg describe` the command should run until the end without falling and in the end provide information about the failures

```
Bundle SHA: sha256:3cd8f0d917a92fafeef2d44eb63195d5351c35cf46b8b7448cb126c7d68c6291

Images:
  - Image: localhost:5000/test@sha256:4c8b96d4fffdfae29258d94a22ae4ad1fe36139d47288b8960d9958d1e63a9d0
    Type: Image
    Origin: index.docker.io/dkalinin/k8s-simple-app@sha256:4c8b96d4fffdfae29258d94a22ae4ad1fe36139d47288b8960d9958d1e63a9d0
    Annotations:
      kbld.carvel.dev/id: index.docker.io/dkalinin/k8s-simple-app:latest
      kbld.carvel.dev/origins: - resolved:
    tag: latest
    url: index.docker.io/dkalinin/k8s-simple-app:latest


  - Image: localhost:5000/test@sha256:510482df49db421542ae10142e8ce99572b23ef72675c37e92e4a12b541a3f6a
    Type: Internal

  - Image: localhost:5000/test@sha256:ecc10da4f480b1ba263d96197151d76bc787104cbcf7f1b1f42b14340b58bae7
    Type: Signature
    Error: Access Denied

Unable to describe the full bundle, please review the previous output to understand the error
Failed
```

When the error happens while reading an Image from ImagesLock, `imgpkg` should assume that the type of the Image is `Image` and should provide the following output

```
    Image: localhost:5000/test@sha256:4c8b96d4fffdfae29258d94a22ae4ad1fe36139d47288b8960d9958d1e63a9d0
    Type: Image
    Origin: index.docker.io/dkalinin/k8s-simple-app@sha256:4c8b96d4fffdfae29258d94a22ae4ad1fe36139d47288b8960d9958d1e63a9d0
    Error: Image Not Found
```

**Anything else you would like to add:**
For signatures/sboms/internal Images we should ignore 404 errors since they might not be present, nevertheless, if we get a different error it should be logged.
This behavior should apply to all the output types.

---
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 work on this issue.


Pull request:
Collect access denied errors on describe
Fixes #347 

Patch:
diff --git a/pkg/imgpkg/bundle/fetch_images.go b/pkg/imgpkg/bundle/fetch_images.go
index ad2705c5..59c76094 100644
--- a/pkg/imgpkg/bundle/fetch_images.go
+++ b/pkg/imgpkg/bundle/fetch_images.go
@@ -8,6 +8,7 @@ import (
 
 	regname "github.com/google/go-containerregistry/pkg/name"
 	"github.com/vmware-tanzu/carvel-imgpkg/pkg/imgpkg/lockconfig"
+	"github.com/vmware-tanzu/carvel-imgpkg/pkg/imgpkg/signature"
 )
 
 // SignatureFetcher Interface to retrieve signatures associated with Images
@@ -31,7 +32,15 @@ func (o *Bundle) FetchAllImagesRefs(concurrency int, ui Logger, sigFetcher Signa
 		}
 		refs, err := sigFetcher.FetchForImageRefs(imgs)
 		if err != nil {
-			return nil, err
+			fetchErr, ok := err.(*signature.FetchError)
+			if !ok {
+				return nil, err
+			}
+			for _, err := range fetchErr.AllErrors {
+				bundle.cachedImageRefs.StoreImageRef(
+					NewImageRefWithTypeAndError(
+						lockconfig.ImageRef{Image: err.ImageRef()}, SignatureImage, err.Error()))
+			}
 		}
 
 		for _, ref := range refs {
diff --git a/pkg/imgpkg/bundle/images_refs.go b/pkg/imgpkg/bundle/images_refs.go
index 08cb3c61..b9653e59 100644
--- a/pkg/imgpkg/bundle/images_refs.go
+++ b/pkg/imgpkg/bundle/images_refs.go
@@ -20,6 +20,7 @@ type ImageRef struct {
 	IsBundle  *bool
 	Copiable  bool
 	ImageType ImageType
+	Error     string
 }
 
 // ImageType defines the type of Image. This is an evolving list that might grow with time
@@ -48,18 +49,20 @@ func NewContentImageRef(imgRef lockconfig.ImageRef) ImageRef {
 
 // NewImageRefWithType Constructs a new ImageRef based on the ImageType
 func NewImageRefWithType(imgRef lockconfig.ImageRef, imageType ImageType) ImageRef {
-	copiable := true
-	if imageType == InternalImage {
-		copiable = false
-	}
-	isBundle := false
-	if imageType == BundleImage {
-		isBundle = true
-	}
+	copiable := imageType == InternalImage
+	isBundle := imageType == BundleImage
 
 	return ImageRef{ImageRef: imgRef, IsBundle: &isBundle, Copiable: copiable, ImageType: imageType}
 }
 
+// NewImageRefWithTypeAndError Constructs a new ImageRef based on the ImageType that failed to fetch
+func NewImageRefWithTypeAndError(imgRef lockconfig.ImageRef, imageType ImageType, err string) ImageRef {
+	copiable := imageType == InternalImage
+	isBundle := imageType == BundleImage
+
+	return ImageRef{ImageRef: imgRef, IsBundle: &isBundle, Copiable: copiable, ImageType: imageType, Error: err}
+}
+
 // Digest Image Digest
 func (i ImageRef) Digest() string {
 	digest, err := name.NewDigest(i.Image)
@@ -81,6 +84,7 @@ func (i ImageRef) DeepCopy() ImageRef {
 		IsBundle:  isBundle,
 		Copiable:  i.Copiable,
 		ImageType: i.ImageType,
+		Error:     i.Error,
 	}
 }
 
diff --git a/pkg/imgpkg/signature/cosign.go b/pkg/imgpkg/signature/cosign.go
index bfa72f7b..61c6b650 100644
--- a/pkg/imgpkg/signature/cosign.go
+++ b/pkg/imgpkg/signature/cosign.go
@@ -42,6 +42,9 @@ func (c Cosign) Signature(imageRef regname.Digest) (imageset.UnprocessedImageRef
 			if transportErr.StatusCode == http.StatusNotFound {
 				return imageset.UnprocessedImageRef{}, NotFoundErr{}
 			}
+			if transportErr.StatusCode == http.StatusForbidden {
+				return imageset.UnprocessedImageRef{}, AccessDeniedErr{imageRef: sigTagRef.Identifier()}
+			}
 		}
 		return imageset.UnprocessedImageRef{}, err
 	}
diff --git a/pkg/imgpkg/signature/fetch_signatures.go b/pkg/imgpkg/signature/fetch_signatures.go
index f6659945..adbfb981 100644
--- a/pkg/imgpkg/signature/fetch_signatures.go
+++ b/pkg/imgpkg/signature/fetch_signatures.go
@@ -19,14 +19,66 @@ type Finder interface {
 	Signature(reference name.Digest) (imageset.UnprocessedImageRef, error)
 }
 
+// FetchingError Error type that happen when fetching signatures
+type FetchingError interface {
+	error
+	ImageRef() string
+}
+
 // NotFoundErr specific not found error
-type NotFoundErr struct{}
+type NotFoundErr struct {
+	imageRef string
+}
+
+// ImageRef Image Reference and associated to the error
+func (n NotFoundErr) ImageRef() string {
+	return n.imageRef
+}
 
 // Error Not Found Error message
-func (s NotFoundErr) Error() string {
+func (n NotFoundErr) Error() string {
 	return "signature not found"
 }
 
+// AccessDeniedErr specific access denied error
+type AccessDeniedErr struct {
+	imageRef string
+}
+
+// ImageRef Image Reference and associated to the error
+func (a AccessDeniedErr) ImageRef() string {
+	return a.imageRef
+}
+
+// Error Access Denied message
+func (a AccessDeniedErr) Error() string {
+	return "access denied"
+}
+
+// FetchError Struct that will contain all the errors found while fetching signatures
+type FetchError struct {
+	AllErrors []FetchingError
+}
+
+// Error message that contains all errors
+func (f *FetchError) Error() string {
+	msg := "Unable to retrieve the following images:\n"
+	for _, err := range f.AllErrors {
+		msg = fmt.Sprintf("%sImage: '%s'\nError:%s", msg, err.ImageRef(), err.Error())
+	}
+	return msg
+}
+
+// HasErrors check if any error happened
+func (f *FetchError) HasErrors() bool {
+	return len(f.AllErrors) > 0
+}
+
+// Add a new error to the list of errors
+func (f *FetchError) Add(err FetchingError) {
+	f.AllErrors = append(f.AllErrors, err)
+}
+
 // Signatures Signature fetcher
 type Signatures struct {
 	signatureFinder Finder
@@ -71,6 +123,7 @@ func (s *Signatures) FetchForImageRefs(images []lockconfig.ImageRef) ([]lockconf
 
 	throttle := util.NewThrottle(s.concurrency)
 	var wg errgroup.Group
+	allErrs := &FetchError{}
 
 	for _, ref := range images {
 		ref := ref //copy
@@ -85,10 +138,16 @@ func (s *Signatures) FetchForImageRefs(images []lockconfig.ImageRef) ([]lockconf
 
 			signature, err := s.signatureFinder.Signature(imgDigest)
 			if err != nil {
-				if _, ok := err.(NotFoundErr); !ok {
-					return fmt.Errorf("Fetching signature for image '%s': %s", imgDigest.Name(), err)
+				if _, ok := err.(NotFoundErr); ok {
+					return nil
+				}
+				if deniedErr, ok := err.(AccessDeniedErr); ok {
+					lock.Lock()
+					defer lock.Unlock()
+					allErrs.Add(deniedErr)
+					return nil
 				}
-				return nil
+				return fmt.Errorf("Fetching signature for image '%s': %s", imgDigest.Name(), err)
 			}
 
 			lock.Lock()
@@ -103,7 +162,15 @@ func (s *Signatures) FetchForImageRefs(images []lockconfig.ImageRef) ([]lockconf
 
 	err := wg.Wait()
 
-	return signatures, err
+	if err != nil {
+		return signatures, err
+	}
+
+	if allErrs.HasErrors() {
+		return signatures, allErrs
+	}
+
+	return signatures, nil
 }
 
 // Noop No Operation signature fetcher
diff --git a/pkg/imgpkg/v1/describe.go b/pkg/imgpkg/v1/describe.go
index cc7b1983..37bc6bed 100644
--- a/pkg/imgpkg/v1/describe.go
+++ b/pkg/imgpkg/v1/describe.go
@@ -34,10 +34,11 @@ type Metadata struct {
 
 // ImageInfo URLs where the image can be found as well as annotations provided in the Images Lock
 type ImageInfo struct {
-	Image       string            `json:"image"`
-	Origin      string            `json:"origin"`
+	Image       string            `json:"image,omitempty"`
+	Origin      string            `json:"origin,omitempty"`
 	Annotations map[string]string `json:"annotations,omitempty"`
 	ImageType   bundle.ImageType  `json:"imageType"`
+	Error       string            `json:"error,omitempty"`
 }
 
 // Content Contents present in a Bundle
@@ -164,15 +165,22 @@ func (r *refWithDescription) describeBundleRec(visitedImgs map[string]refWithDes
 			}
 			desc.bundle.Content.Bundles[digest.DigestStr()] = bundleDesc
 		} else {
-			digest, err := name.NewDigest(ref.Image)
-			if err != nil {
-				panic(fmt.Sprintf("Internal inconsistency: image %s should be fully resolved", ref.Image))
-			}
-			desc.bundle.Content.Images[digest.DigestStr()] = ImageInfo{
-				Image:       ref.PrimaryLocation(),
-				Origin:      ref.Image,
-				Annotations: ref.Annotations,
-				ImageType:   ref.ImageType,
+			if ref.Error == "" {
+				digest, err := name.NewDigest(ref.Image)
+				if err != nil {
+					panic(fmt.Sprintf("Internal inconsistency: image %s should be fully resolved", ref.Image))
+				}
+				desc.bundle.Content.Images[digest.DigestStr()] = ImageInfo{
+					Image:       ref.PrimaryLocation(),
+					Origin:      ref.Image,
+					Annotations: ref.Annotations,
+					ImageType:   ref.ImageType,
+				}
+			} else {
+				desc.bundle.Content.Images[ref.Image] = ImageInfo{
+					ImageType: ref.ImageType,
+					Error:     ref.Error,
+				}
 			}
 		}
 	}


Test patch:
diff --git a/pkg/imgpkg/signature/fetch_signatures_test.go b/pkg/imgpkg/signature/fetch_signatures_test.go
index 787f72cc..c42edadc 100644
--- a/pkg/imgpkg/signature/fetch_signatures_test.go
+++ b/pkg/imgpkg/signature/fetch_signatures_test.go
@@ -11,6 +11,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/vmware-tanzu/carvel-imgpkg/pkg/imgpkg/imageset"
+	"github.com/vmware-tanzu/carvel-imgpkg/pkg/imgpkg/lockconfig"
 	"github.com/vmware-tanzu/carvel-imgpkg/pkg/imgpkg/signature"
 	"github.com/vmware-tanzu/carvel-imgpkg/pkg/imgpkg/signature/signaturefakes"
 )
@@ -44,6 +45,36 @@ func TestSignatureRetriever_Signatures(t *testing.T) {
 		assert.Equal(t, imageset.UnprocessedImageRef{DigestRef: "registry.io/img@sha256:cf31af331f38d1d7158470e095b132acd126a7180a54f263d386da88eb681d93", Tag: "some-tag"}, sign2)
 	})
 
+	t.Run("denied errors are provided as part of the error", func(t *testing.T) {
+		fakeSignatureFinder := &signaturefakes.FakeFinder{}
+		subject := signature.NewSignatures(fakeSignatureFinder, 2)
+		fakeSignatureFinder.SignatureCalls(func(digest regname.Digest) (imageset.UnprocessedImageRef, error) {
+			availableResults := map[string]imageset.UnprocessedImageRef{
+				"sha256:4c8b96d4fffdfae29258d94a22ae4ad1fe36139d47288b8960d9958d1e63a9d0": {DigestRef: "registry.io/img@sha256:cf31af331f38d1d7158470e095b132acd126a7180a54f263d386da88eb681d93", Tag: "some-tag"},
+				"sha256:56cb33b3b4bc45509c5ff7513ddc6ed78764f9ad5165cc32826e04da49d5462b": {DigestRef: "registry.io/img2@sha256:be154cc2b1211a9f98f4d708f4266650c9129784d0485d4507d9b0fa05d928b6", Tag: "some-other-tag"},
+			}
+			if res, ok := availableResults[digest.DigestStr()]; ok {
+				return res, nil
+			}
+			return imageset.UnprocessedImageRef{}, signature.AccessDeniedErr{}
+		})
+
+		var args []lockconfig.ImageRef
+		args = append(args, lockconfig.ImageRef{Image: "registry.io/img@sha256:4c8b96d4fffdfae29258d94a22ae4ad1fe36139d47288b8960d9958d1e63a9d0"})
+		args = append(args, lockconfig.ImageRef{Image: "registry.io/img1@sha256:6716afd7a68262a37d3f67681ed9dedf3b882938ad777f268f44d68894531f7a"})
+		args = append(args, lockconfig.ImageRef{Image: "registry.io/img2@sha256:56cb33b3b4bc45509c5ff7513ddc6ed78764f9ad5165cc32826e04da49d5462b"})
+		args = append(args, lockconfig.ImageRef{Image: "registry.io/img3@sha256:a40a266ca606d8dcbac60b4bb1ec42128ba7063f5eed3a997ec4546edc6cf209"})
+		signatures, err := subject.FetchForImageRefs(args)
+		require.Error(t, err)
+		errs, ok := err.(*signature.FetchError)
+		require.True(t, ok, "Unexpected error found %+v, while expecting a FetchError", err)
+		require.Len(t, errs.AllErrors, 2)
+		require.EqualError(t, errs.AllErrors[0], "access denied")
+		require.EqualError(t, errs.AllErrors[1], "access denied")
+
+		require.Len(t, signatures, 2)
+	})
+
 	t.Run("it returns error when returned error is not sign.NotFound", func(t *testing.T) {
 		fakeSignatureFinder := &signaturefakes.FakeFinder{}
 		subject := signature.NewSignatures(fakeSignatureFinder, 2)
diff --git a/pkg/imgpkg/v1/describe_test.go b/pkg/imgpkg/v1/describe_test.go
index 2163dba7..66950d10 100644
--- a/pkg/imgpkg/v1/describe_test.go
+++ b/pkg/imgpkg/v1/describe_test.go
@@ -5,6 +5,7 @@ package v1_test
 
 import (
 	"fmt"
+	"net/http"
 	"os"
 	"strings"
 	"testing"
@@ -320,6 +321,50 @@ func TestDescribeBundle(t *testing.T) {
 			assertBundleResult(t, topBundle, bundleDescription)
 		})
 	}
+
+	t.Run("When denied error occur retrieving a signature, it provide the error information for the signature", func(t *testing.T) {
+		logger.LogLevel = helpers.LogTrace
+		fakeRegBuilder := helpers.NewFakeRegistry(t, logger)
+		img1 := fakeRegBuilder.WithRandomImage("other-repo/some-random-img")
+		hash, err := regv1.NewHash(img1.Digest)
+		require.NoError(t, err)
+		b := fakeRegBuilder.
+			WithRandomBundle("repo/bundle-with-sig-error").
+			WithImageRefs([]lockconfig.ImageRef{{Image: img1.RefDigest}})
+		signToDeny := fakeRegBuilder.WithRandomTaggedImage(b.RefDigest, cosign.Munge(regv1.Descriptor{Digest: hash}))
+
+		fakeRegBuilder.Build()
+		fakeRegBuilder.WithHandlerFunc(func(writer http.ResponseWriter, request *http.Request) bool {
+			if strings.HasSuffix(request.URL.String(), "/v2/") {
+				return false
+			}
+
+			if request.Method == "GET" || request.Method == "HEAD" {
+				if strings.Contains(request.URL.String(), cosign.Munge(regv1.Descriptor{Digest: hash})) {
+					writer.WriteHeader(403)
+					writer.Write([]byte("{\"errors\":[{\"code\":\"UNKNOWN\",\"message\":\"denied access\"}]}"))
+					return true
+				}
+			}
+			return false
+		})
+
+		bundleDescription, err := v1.Describe(b.RefDigest, v1.DescribeOpts{
+			Logger:                 logger,
+			Concurrency:            1,
+			IncludeCosignArtifacts: true,
+		},
+			registry.Opts{
+				EnvironFunc: os.Environ,
+				RetryCount:  3,
+			},
+		)
+		require.NoError(t, err)
+
+		require.Len(t, bundleDescription.Content.Images, 2)
+		require.Equal(t, ctlbundle.ImageType("Signature"), bundleDescription.Content.Images[signToDeny.Tag].ImageType)
+		require.Equal(t, "access denied", bundleDescription.Content.Images[signToDeny.Tag].Error)
+	})
 }
 
 type testImage struct {
diff --git a/test/helpers/fake_registry.go b/test/helpers/fake_registry.go
index 9add5533..64c610da 100644
--- a/test/helpers/fake_registry.go
+++ b/test/helpers/fake_registry.go
@@ -90,16 +90,18 @@ func (r *FakeTestRegistryBuilder) BuildWithRegistryOpts(opts registry.Opts) regi
 		auth := regremote.WithAuth(r.auth)
 
 		if val.Image != nil {
-			r.logger.Tracef("build: creating image on registry: %s\n", fmt.Sprintf("%s/%s", u.Host, imageRef))
+			r.logger.Tracef("build: creating image on registry: %s", fmt.Sprintf("%s/%s", u.Host, imageRef))
 			err = regremote.Write(imageRefWithTestRegistry, val.Image, regremote.WithNondistributable, auth)
 			assert.NoError(r.t, err)
 			if val.Tag != "" {
+				r.logger.Tracef(" with tag: %s", val.Tag)
 				err = regremote.Tag(imageRefWithTestRegistry.Context().Tag(val.Tag), val.Image, auth)
 				assert.NoError(r.t, err)
 			} else {
 				err = regremote.Tag(imageRefWithTestRegistry.Context().Tag("latest"), val.Image, auth)
 				assert.NoError(r.t, err)
 			}
+			r.logger.Tracef("\n")
 		}
 
 		if val.ImageIndex != nil {