tag service: properly handle error responses on HEAD requests by (#1918)

* tag service: properly handle error responses on HEAD requests by
re-issuing requests as GET for proper error details.

Fixes #1911.

Signed-off-by: dmitri <deemok@gmail.com>

* Simplify handling of failing HEAD requests in TagService and
make a GET request for cases:
  - if the server does not handle HEAD
  - if the response was an error to get error details

Signed-off-by: dmitri <deemok@gmail.com>

* Add a missing http.Response.Body.Close call for the GET request.

Signed-off-by: dmitri <deemok@gmail.com>

registry/client/repository.go

@@ -301,18 +301,20 @@ func (t *tags) Get(ctx context.Context, tag string) (distribution.Descriptor, er
 		return distribution.Descriptor{}, err
 	}
 
-	req, err := http.NewRequest("HEAD", u, nil)
+	newRequest := func(method string) (*http.Response, error) {
+		req, err := http.NewRequest(method, u, nil)
 		if err != nil {
-		return distribution.Descriptor{}, err
+			return nil, err
 		}
 
 		for _, t := range distribution.ManifestMediaTypes() {
 			req.Header.Add("Accept", t)
 		}
-
-	var attempts int
 		resp, err := t.client.Do(req)
-check:
+		return resp, err
+	}
+
+	resp, err := newRequest("HEAD")
 	if err != nil {
 		return distribution.Descriptor{}, err
 	}
@@ -321,23 +323,20 @@ check:
 	switch {
 	case resp.StatusCode >= 200 && resp.StatusCode < 400:
 		return descriptorFromResponse(resp)
-	case resp.StatusCode == http.StatusMethodNotAllowed:
-		req, err = http.NewRequest("GET", u, nil)
+	default:
+		// if the response is an error - there will be no body to decode.
+		// Issue a GET request:
+		//   - for data from a server that does not handle HEAD
+		//   - to get error details in case of a failure
+		resp, err = newRequest("GET")
 		if err != nil {
 			return distribution.Descriptor{}, err
 		}
+		defer resp.Body.Close()
 
-		for _, t := range distribution.ManifestMediaTypes() {
-			req.Header.Add("Accept", t)
+		if resp.StatusCode >= 200 && resp.StatusCode < 400 {
+			return descriptorFromResponse(resp)
 		}
-
-		resp, err = t.client.Do(req)
-		attempts++
-		if attempts > 1 {
-			return distribution.Descriptor{}, err
-		}
-		goto check
-	default:
 		return distribution.Descriptor{}, HandleErrorResponse(resp)
 	}
 }

registry/client/repository_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/docker/distribution/manifest/schema1"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/distribution/registry/api/errcode"
+	"github.com/docker/distribution/registry/api/v2"
 	"github.com/docker/distribution/testutil"
 	"github.com/docker/distribution/uuid"
 	"github.com/docker/libtrust"
@@ -950,6 +951,49 @@ func TestManifestTags(t *testing.T) {
 	// TODO(dmcgowan): Check for error cases
 }
 
+func TestObtainsErrorForMissingTag(t *testing.T) {
+	repo, _ := reference.ParseNamed("test.example.com/repo")
+
+	var m testutil.RequestResponseMap
+	var errors errcode.Errors
+	errors = append(errors, v2.ErrorCodeManifestUnknown.WithDetail("unknown manifest"))
+	errBytes, err := json.Marshal(errors)
+	if err != nil {
+		t.Fatal(err)
+	}
+	m = append(m, testutil.RequestResponseMapping{
+		Request: testutil.Request{
+			Method: "GET",
+			Route:  "/v2/" + repo.Name() + "/manifests/1.0.0",
+		},
+		Response: testutil.Response{
+			StatusCode: http.StatusNotFound,
+			Body:       errBytes,
+			Headers: http.Header(map[string][]string{
+				"Content-Type": {"application/json; charset=utf-8"},
+			}),
+		},
+	})
+	e, c := testServer(m)
+	defer c()
+
+	ctx := context.Background()
+	r, err := NewRepository(ctx, repo, e, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tagService := r.Tags(ctx)
+
+	_, err = tagService.Get(ctx, "1.0.0")
+	if err == nil {
+		t.Fatalf("Expected an error")
+	}
+	if !strings.Contains(err.Error(), "manifest unknown") {
+		t.Fatalf("Expected unknown manifest error message")
+	}
+}
+
 func TestManifestTagsPaginated(t *testing.T) {
 	s := httptest.NewServer(http.NotFoundHandler())
 	defer s.Close()