Improve host fallback behaviour in docker remote

awprice · awprice · commit 37b9a347a274 · 2020-02-10T15:16:39.000+11:00
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>
diff --git a/remotes/docker/fetcher.go b/remotes/docker/fetcher.go
@@ -96,41 +96,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,
-			"could not fetch content descriptor %v (%v) from remote",
-			desc.Digest, desc.MediaType)
+		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
 
 	})
 }
diff --git a/remotes/docker/resolver.go b/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.
 
diff --git a/remotes/docker/resolver_test.go b/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)

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