change Ping to check for Docker header

Change Registry.Ping() to:
  - respect http 401 response code as valid
  - conform to the Specs by requiring header
    Docker-Distribution-API-Version

Change the TokenTransport unit tests to emit that header.

Remove the naive workaround inside Registry.Pingable() as
iterating over all sites that do not support the old
ping would require huge and constant effort. (Not only GCR.)

I add separate Registry.PingClient, because the Client handles 401
by closing the body so it disables the possibility of disinguishing
whether the 401 contained Docker-Distribution-API-Version header.

Refactor: simplify current TestPingable
Signed-off-by: Jakub Bielecki <47531708+jabielecki@users.noreply.github.com>

registry/ping.go

@@ -2,27 +2,35 @@ package registry
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"strings"
 )
 
-// Pingable checks pingable
+// Pingable returns false for some specific registries that can be never successfuly pinged.
+//
+// Currently it always returns true.
 func (r *Registry) Pingable() bool {
-	// Currently *.gcr.io/v2 can't be ping if users have each projects auth
-	return !strings.HasSuffix(r.URL, "gcr.io")
+	return true
 }
 
-// Ping tries to contact a registry URL to make sure it is up and accessible.
+var ErrNoDockerHeader = errors.New("site does not return http(s) header Docker-Distribution-API-Version: registry/2.0")
+
+// Ping tries to contact a registry URL to make sure it is up and it supports Docker v2 Registry Specification.
 func (r *Registry) Ping(ctx context.Context) error {
 	url := r.url("/v2/")
 	r.Logf("registry.ping url=%s", url)
 	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
 		return err
 	}
-	resp, err := r.Client.Do(req.WithContext(ctx))
-	if resp != nil {
-		defer resp.Body.Close()
+	resp, err := r.PingClient.Do(req.WithContext(ctx))
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+	if !strings.HasPrefix(resp.Header.Get("Docker-Distribution-API-Version"), "registry/2.") {
+		return ErrNoDockerHeader
 	}
-	return err
+	return nil
 }

registry/ping_test.go

@@ -1,31 +1,94 @@
 package registry
 
 import (
+	"context"
+	"net/http"
+	"net/http/httptest"
 	"testing"
+
+	"github.com/docker/docker/api/types"
 )
 
+func createClientAndPing(httpCode int, headerName string, headerValue string) (*Registry, func(), error) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set(headerName, headerValue)
+		w.WriteHeader(httpCode)
+	}))
+
+	auth := types.AuthConfig{ServerAddress: ts.URL}
+	r, err := New(context.Background(), auth, Opt{Insecure: true})
+	return r, ts.Close, err
+}
+
+func TestPing(t *testing.T) {
+	testcases := []struct {
+		httpCode    int
+		headerName  string
+		headerValue string
+		wantErr     error
+	}{
+		{
+			httpCode:    200,
+			headerName:  "whatever",
+			headerValue: "whatever",
+			wantErr:     ErrNoDockerHeader,
+		},
+		{
+			httpCode:    401,
+			headerName:  "wrong",
+			headerValue: "whatever",
+			wantErr:     ErrNoDockerHeader,
+		},
+		{
+			httpCode:    200,
+			headerName:  "docker-distribution-api-version",
+			headerValue: "registry/2.0",
+			wantErr:     nil,
+		},
+		{
+			httpCode:    401,
+			headerName:  "Docker-Distribution-API-Version",
+			headerValue: "registry/2.1",
+			// Many popular servers do allow unauthenticated image pulls, but require authentication to visit the base url.
+			// This conforms to Docker Registry v2 API Specification https://docs.docker.com/registry/spec/api/
+			// Thus `401 Unauthorized` is as good response as `200 OK`, if only it has the proper Docker header.
+			wantErr: nil,
+		},
+	}
+	for _, tc := range testcases {
+		r, closeFunc, err := createClientAndPing(tc.httpCode, tc.headerName, tc.headerValue)
+		defer closeFunc()
+		if err != tc.wantErr {
+			t.Fatalf("when creating client and performing ping for (%v, %q, %q), got error %#v but expected %#v", tc.httpCode, tc.headerName, tc.headerValue, err, tc.wantErr)
+		}
+		if err != nil {
+			continue
+		}
+		err = r.Ping(context.Background())
+		if err != tc.wantErr {
+			t.Fatalf("when repeating ping for (%v, %q, %q), got error %#v but expected %#v", tc.httpCode, tc.headerName, tc.headerValue, err, tc.wantErr)
+		}
+	}
+}
+
 func TestPingable(t *testing.T) {
-	testcases := map[string]struct {
+	testcases := []struct {
 		registry Registry
-		expect   bool
+		want     bool
 	}{
-		"Docker": {
+		{
 			registry: Registry{URL: "https://registry-1.docker.io"},
-			expect:   true,
-		},
-		"GCR_global": {
-			registry: Registry{URL: "https://gcr.io"},
-			expect:   false,
+			want:     true,
 		},
-		"GCR_asia": {
+		{
 			registry: Registry{URL: "https://asia.gcr.io"},
-			expect:   false,
+			want:     true,
 		},
 	}
-	for label, testcase := range testcases {
-		actual := testcase.registry.Pingable()
-		if testcase.expect != actual {
-			t.Fatalf("%s: expected (%v), got (%v)", label, testcase.expect, actual)
+	for _, testcase := range testcases {
+		got := testcase.registry.Pingable()
+		if testcase.want != got {
+			t.Fatalf("%s pingable: expected (%v), got (%v)", testcase.registry.URL, testcase.want, got)
 		}
 	}
 }

registry/registry.go

@@ -26,13 +26,14 @@ var ErrUnexpectedHttpStatusCode = errors.New("unexpected http status code")
 
 // Registry defines the client for retrieving information from the registry API.
 type Registry struct {
-	URL      string
-	Domain   string
-	Username string
-	Password string
-	Client   *http.Client
-	Logf     LogfCallback
-	Opt      Opt
+	URL        string
+	Domain     string
+	Username   string
+	Password   string
+	Client     *http.Client
+	PingClient *http.Client
+	Logf       LogfCallback
+	Opt        Opt
 }
 
 var reProtocol = regexp.MustCompile("^https?://")
@@ -129,6 +130,13 @@ func newFromTransport(ctx context.Context, auth types.AuthConfig, transport http
 			Timeout:   opt.Timeout,
 			Transport: customTransport,
 		},
+		PingClient: &http.Client{
+			Timeout: opt.Timeout,
+			Transport: &CustomTransport{
+				Transport: transport,
+				Headers:   opt.Headers,
+			},
+		},
 		Username: auth.Username,
 		Password: auth.Password,
 		Logf:     logf,

registry/tokentransport_test.go

@@ -14,6 +14,7 @@ import (
 func TestErrBasicAuth(t *testing.T) {
 	ctx := context.Background()
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Docker-Distribution-API-Version", "registry/2.0")
 		if r.URL.Path == "/" {
 			w.Header().Set("www-authenticate", `Basic realm="Registry Realm",service="Docker registry"`)
 			w.WriteHeader(http.StatusUnauthorized)
@@ -44,6 +45,7 @@ func TestErrBasicAuth(t *testing.T) {
 var authURI string
 
 func oauthFlow(w http.ResponseWriter, r *http.Request) {
+	w.Header().Set("Docker-Distribution-API-Version", "registry/2.0")
 	if strings.HasPrefix(r.URL.Path, "/oauth2/accesstoken") {
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusOK)