From a022c218194c05449ad69b69c48fc6cac9d6f0b3 Mon Sep 17 00:00:00 2001 From: Alex Price Date: Tue, 3 Dec 2019 11:38:46 +1100 Subject: [PATCH] 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 --- remotes/docker/fetcher.go | 30 ++++++----- remotes/docker/resolver.go | 6 ++- remotes/docker/resolver_test.go | 88 +++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/remotes/docker/fetcher.go b/remotes/docker/fetcher.go index 2bd295ea6..55c01beaf 100644 --- a/remotes/docker/fetcher.go +++ b/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, - "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 index f126449c3..90a0e34de 100644 --- 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 index 0929e1e12..d75740f6d 100644 --- 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)