Merge pull request #8861 from jedevc/docker-resolver-relevant-error

docker: return most relevant error from docker resolution

remotes/docker/resolver.go

@@ -242,10 +242,9 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 	}
 
 	var (
-		firstErr error
-		paths    [][]string
-		dgst     = refspec.Digest()
-		caps     = HostCapabilityPull
+		paths [][]string
+		dgst  = refspec.Digest()
+		caps  = HostCapabilityPull
 	)
 
 	if dgst != "" {
@@ -276,6 +275,13 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 		return "", ocispec.Descriptor{}, err
 	}
 
+	var (
+		// firstErr is the most relevant error encountered during resolution.
+		// We use this to determine the error to return, making sure that the
+		// error created furthest through the resolution process is returned.
+		firstErr         error
+		firstErrPriority int
+	)
 	for _, u := range paths {
 		for _, host := range hosts {
 			ctx := log.WithLogger(ctx, log.G(ctx).WithField("host", host.Host))
@@ -295,9 +301,9 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 				if errors.Is(err, ErrInvalidAuthorization) {
 					err = fmt.Errorf("pull access denied, repository does not exist or may require authorization: %w", err)
 				}
-				// Store the error for referencing later
-				if firstErr == nil {
+				if firstErrPriority < 1 {
 					firstErr = err
+					firstErrPriority = 1
 				}
 				log.G(ctx).WithError(err).Info("trying next host")
 				continue // try another host
@@ -306,14 +312,19 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 
 			if resp.StatusCode > 299 {
 				if resp.StatusCode == http.StatusNotFound {
+					if firstErrPriority < 2 {
+						firstErr = fmt.Errorf("%s: %w", ref, errdefs.ErrNotFound)
+						firstErrPriority = 2
+					}
 					log.G(ctx).Info("trying next host - response was http.StatusNotFound")
 					continue
 				}
 				if resp.StatusCode > 399 {
-					// Set firstErr when encountering the first non-404 status code.
-					if firstErr == nil {
+					if firstErrPriority < 3 {
 						firstErr = remoteerrors.NewUnexpectedStatusErr(resp)
+						firstErrPriority = 3
 					}
+					log.G(ctx).Infof("trying next host - response was %s", resp.Status)
 					continue // try another host
 				}
 				return "", ocispec.Descriptor{}, remoteerrors.NewUnexpectedStatusErr(resp)
@@ -384,8 +395,9 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 			}
 			// Prevent resolving to excessively large manifests
 			if size > MaxManifestSize {
-				if firstErr == nil {
+				if firstErrPriority < 4 {
 					firstErr = fmt.Errorf("rejecting %d byte manifest for %s: %w", size, ref, errdefs.ErrNotFound)
+					firstErrPriority = 4
 				}
 				continue
 			}
@@ -401,10 +413,8 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 		}
 	}
 
-	// If above loop terminates without return, then there was an error.
-	// "firstErr" contains the first non-404 error. That is, "firstErr == nil"
-	// means that either no registries were given or each registry returned 404.
-
+	// If above loop terminates without return or error, then no registries
+	// were provided.
 	if firstErr == nil {
 		firstErr = fmt.Errorf("%s: %w", ref, errdefs.ErrNotFound)
 	}

remotes/docker/resolver_test.go

@@ -32,6 +32,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/remotes"
 	"github.com/containerd/containerd/remotes/docker/auth"
 	digest "github.com/opencontainers/go-digest"
@@ -331,6 +332,7 @@ func TestHostTLSFailureFallbackResolver(t *testing.T) {
 	}
 
 	runBasicTest(t, "testname", sf)
+	runNotFoundTest(t, "testname", sf)
 }
 
 func TestHTTPFallbackResolver(t *testing.T) {
@@ -638,6 +640,28 @@ func runBasicTest(t *testing.T, name string, sf func(h http.Handler) (string, Re
 	}
 }
 
+func runNotFoundTest(t *testing.T, name string, sf func(h http.Handler) (string, ResolverOptions, func())) {
+	var (
+		ctx = context.Background()
+		tag = "latest"
+		r   = http.NewServeMux()
+	)
+
+	base, ro, close := sf(logHandler{t, r})
+	defer close()
+
+	resolver := NewResolver(ro)
+	image := fmt.Sprintf("%s/%s:%s", base, name, tag)
+
+	_, _, err := resolver.Resolve(ctx, image)
+	if err == nil {
+		t.Fatalf("Expected error resolving %s, got nil", image)
+	}
+	if !errors.Is(err, errdefs.ErrNotFound) {
+		t.Fatalf("Expected error resolving %s to be ErrNotFound, got %v", image, err)
+	}
+}
+
 func testFetch(ctx context.Context, f remotes.Fetcher, desc ocispec.Descriptor) error {
 	r, err := f.Fetch(ctx, desc)
 	if err != nil {