From b4814a29d41accf6339910db6ea424d4b570dd30 Mon Sep 17 00:00:00 2001
From: Justin Chadwell <me@jedevc.com>
Date: Mon, 24 Jul 2023 15:29:49 +0100
Subject: [PATCH] docker: return most relevant error from docker resolution

Previously, we would return the first non-404 error from a host.

This is logical, however, it can result in confusing errors to the
user:

- e.g. we have an HTTP host, and an HTTPS host.

  If the image does not exist, we return "http: server gave HTTP
  response to HTTPS client". This is technically correct, however, the
  user is easily confused - the most relevant error in this case is the
  404 error.

- e.g. we have a broken HTTP host that returns 5XX errors, and a HTTP
  host with authentication.

  On the request for an image, we return the 5XX error directly.
  However, we have a host later on which returned an authentication
  error which is now hidden from the user.

Note: this *can* be resolved by changing the order of hosts passed in,
however this requires 1. knowing ahead of time which hosts are going to
return certain errors and 2. this is often not desirable, we'd prefer
to use HTTPS if it's available, and only then fallback to HTTP.

To resolve this, we assign each possible error during resolution a
"priority" that marks how far through the image resolution process a
host/path combo got. Then we return the error with the highest priority,
which is much more likely to be the most relevant error to the user.

The ranking of priority then is (from lowest to highest):
- Underlying transport errors (TLS, TCP, etc)
- 404 errors
- Other 4XX/5XX errors
- Manifest rejection (due to max size exceeded)

Signed-off-by: Justin Chadwell <me@jedevc.com>
---
 remotes/docker/resolver.go | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/remotes/docker/resolver.go b/remotes/docker/resolver.go
index b0ff710d8..5808a5847 100644
--- a/remotes/docker/resolver.go
+++ b/remotes/docker/resolver.go
@@ -241,10 +241,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 != "" {
@@ -275,6 +274,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))
@@ -294,9 +300,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
@@ -305,13 +311,17 @@ 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
@@ -384,8 +394,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 +412,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)
 	}