Improve host fallback behaviour in docker remote

This commit improves the fallback behaviour when resolving and
fetching images with multiple hosts. If an error is encountered
when resolving and fetching images, and more than one host is being
used, we will try the same operation on the next host. The error
from the first host is preserved so that if all hosts fail, we can
display the error from the first host.

fixes #3850

Signed-off-by: Alex Price <aprice@atlassian.com>

remotes/docker/fetcher.go

@@ -95,41 +95,49 @@ func (r dockerFetcher) Fetch(ctx context.Context, desc ocispec.Descriptor) (io.R
 			images.MediaTypeDockerSchema1Manifest,
 			ocispec.MediaTypeImageManifest, ocispec.MediaTypeImageIndex:
 
+			var firstErr error
 			for _, host := range r.hosts {
 				req := r.request(host, http.MethodGet, "manifests", desc.Digest.String())
 
 				rc, err := r.open(ctx, req, desc.MediaType, offset)
 				if err != nil {
-					if errdefs.IsNotFound(err) {
-						continue // try another host
+					// Store the error for referencing later
+					if firstErr == nil {
+						firstErr = err
 					}
-
-					return nil, err
+					continue // try another host
 				}
 
 				return rc, nil
 			}
+
+			return nil, firstErr
 		}
 
 		// Finally use blobs endpoints
+		var firstErr error
 		for _, host := range r.hosts {
 			req := r.request(host, http.MethodGet, "blobs", desc.Digest.String())
 
 			rc, err := r.open(ctx, req, desc.MediaType, offset)
 			if err != nil {
-				if errdefs.IsNotFound(err) {
-					continue // try another host
+				// Store the error for referencing later
+				if firstErr == nil {
+					firstErr = err
 				}
-
-				return nil, err
+				continue // try another host
 			}
 
 			return rc, nil
 		}
 
-		return nil, errors.Wrapf(errdefs.ErrNotFound,
+		if errdefs.IsNotFound(firstErr) {
+			firstErr = errors.Wrapf(errdefs.ErrNotFound,
 				"could not fetch content descriptor %v (%v) from remote",
 				desc.Digest, desc.MediaType)
+		}
+
+		return nil, firstErr
 
 	})
 }

remotes/docker/resolver.go

@@ -286,7 +286,11 @@ func (r *dockerResolver) Resolve(ctx context.Context, ref string) (string, ocisp
 				if errors.Cause(err) == ErrInvalidAuthorization {
 					err = errors.Wrapf(err, "pull access denied, repository does not exist or may require authorization")
 				}
-				return "", ocispec.Descriptor{}, err
+				// Store the error for referencing later
+				if lastErr == nil {
+					lastErr = err
+				}
+				continue // try another host
 			}
 			resp.Body.Close() // don't care about body contents.
 

remotes/docker/resolver_test.go

@@ -29,6 +29,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/containerd/containerd/remotes"
 	digest "github.com/opencontainers/go-digest"
@@ -192,6 +193,93 @@ func TestBadTokenResolver(t *testing.T) {
 	}
 }
 
+func TestHostFailureFallbackResolver(t *testing.T) {
+	sf := func(h http.Handler) (string, ResolverOptions, func()) {
+		s := httptest.NewServer(h)
+		base := s.URL[7:] // strip "http://"
+
+		options := ResolverOptions{}
+		createHost := func(host string) RegistryHost {
+			return RegistryHost{
+				Client: &http.Client{
+					// Set the timeout so we timeout waiting for the non-responsive HTTP server
+					Timeout: 500 * time.Millisecond,
+				},
+				Host:         host,
+				Scheme:       "http",
+				Path:         "/v2",
+				Capabilities: HostCapabilityPull | HostCapabilityResolve | HostCapabilityPush,
+			}
+		}
+
+		// Create an unstarted HTTP server. We use this to generate a random port.
+		notRunning := httptest.NewUnstartedServer(nil)
+		notRunningBase := notRunning.Listener.Addr().String()
+
+		// Override hosts with two hosts
+		options.Hosts = func(host string) ([]RegistryHost, error) {
+			return []RegistryHost{
+				createHost(notRunningBase), // This host IS running, but with a non-responsive HTTP server
+				createHost(base),           // This host IS running
+			}, nil
+		}
+
+		return base, options, s.Close
+	}
+
+	runBasicTest(t, "testname", sf)
+}
+
+func TestHostTLSFailureFallbackResolver(t *testing.T) {
+	sf := func(h http.Handler) (string, ResolverOptions, func()) {
+		// Start up two servers
+		server := httptest.NewServer(h)
+		httpBase := server.URL[7:] // strip "http://"
+
+		tlsServer := httptest.NewUnstartedServer(h)
+		tlsServer.StartTLS()
+		httpsBase := tlsServer.URL[8:] // strip "https://"
+
+		capool := x509.NewCertPool()
+		cert, _ := x509.ParseCertificate(tlsServer.TLS.Certificates[0].Certificate[0])
+		capool.AddCert(cert)
+
+		client := &http.Client{
+			Transport: &http.Transport{
+				TLSClientConfig: &tls.Config{
+					RootCAs: capool,
+				},
+			},
+		}
+
+		options := ResolverOptions{}
+		createHost := func(host string) RegistryHost {
+			return RegistryHost{
+				Client:       client,
+				Host:         host,
+				Scheme:       "https",
+				Path:         "/v2",
+				Capabilities: HostCapabilityPull | HostCapabilityResolve | HostCapabilityPush,
+			}
+		}
+
+		// Override hosts with two hosts
+		options.Hosts = func(host string) ([]RegistryHost, error) {
+			return []RegistryHost{
+				createHost(httpBase),  // This host is serving plain HTTP
+				createHost(httpsBase), // This host is serving TLS
+			}, nil
+		}
+
+		return httpBase, options, func() {
+			server.Close()
+			tlsServer.Close()
+		}
+	}
+
+	runBasicTest(t, "testname", sf)
+}
+
 func withTokenServer(th http.Handler, creds func(string) (string, string, error)) func(h http.Handler) (string, ResolverOptions, func()) {
 	return func(h http.Handler) (string, ResolverOptions, func()) {
 		s := httptest.NewUnstartedServer(th)