Repository: google/go-containerregistry
Pull request #971 (2021-03-20 14:29:14)

crane digest: fallback to GET when HEAD fails
### Before

```
$ crane digest registry.access.redhat.com/ubi8/ubi-minimal
2021/03/20 10:19:50 No matching credentials were found for "registry.access.redhat.com/ubi8/ubi-minimal", falling back on anonymous
2021/03/20 10:19:51 computing digest: too many parts in hash: 
```

_My, what a very unhelpful error message that is!_ 🤔 

### After

```
$ go run ./cmd/crane digest registry.access.redhat.com/ubi8/ubi-minimal
2021/03/20 10:20:10 No matching credentials were found for "registry.access.redhat.com/ubi8/ubi-minimal", falling back on anonymous
2021/03/20 10:20:10 HEAD request failed, falling back on GET: response did not include Docker-Content-Digest header
2021/03/20 10:20:10 No matching credentials were found for "registry.access.redhat.com/ubi8/ubi-minimal", falling back on anonymous
sha256:fdfb0770bff33e0f97d78583efd68b546a19d0a4b0ac23eef25ef261bca3e975
```

This should also make `crane digest` work against registries that don't support `HEAD` at all, or that don't include all the necessary descriptor information in response headers, though neglecting `Docker-Content-Digest` is probably by far the most common omission.

If this lgty in general I can add some tests.

codecov-io: # [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/971?src=pr&el=h1) Report
> Merging [#971](https://codecov.io/gh/google/go-containerregistry/pull/971?src=pr&el=desc) (b504b62) into [main](https://codecov.io/gh/google/go-containerregistry/commit/70c58c0e4786822fee41860c0c235f14563cd868?el=desc) (70c58c0) will **decrease** coverage by `0.07%`.
> The diff coverage is `58.82%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-containerregistry/pull/971/graphs/tree.svg?width=650&height=150&src=pr&token=wG7iiDCdZ4)](https://codecov.io/gh/google/go-containerregistry/pull/971?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
- Coverage   75.19%   75.11%   -0.08%     
==========================================
  Files         107      107              
  Lines        4837     4850      +13     
==========================================
+ Hits         3637     3643       +6     
- Misses        669      673       +4     
- Partials      531      534       +3     
```


| [Impacted Files](https://codecov.io/gh/google/go-containerregistry/pull/971?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/v1/remote/descriptor.go](https://codecov.io/gh/google/go-containerregistry/pull/971/diff?src=pr&el=tree#diff-cGtnL3YxL3JlbW90ZS9kZXNjcmlwdG9yLmdv) | `71.67% <50.00%> (-2.11%)` | :arrow_down: |
| [pkg/crane/digest.go](https://codecov.io/gh/google/go-containerregistry/pull/971/diff?src=pr&el=tree#diff-cGtnL2NyYW5lL2RpZ2VzdC5nbw==) | `68.18% <80.00%> (+1.51%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-containerregistry/pull/971?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-containerregistry/pull/971?src=pr&el=footer). Last update [70c58c0...b504b62](https://codecov.io/gh/google/go-containerregistry/pull/971?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

jonjohnsonjr: LGTM in general. One thing I'm slightly worried about is the headManifest errors not making sense out of context. WDYT about prepending them with `HEAD <url>: `?
ImJasonH: > LGTM in general. One thing I'm slightly worried about is the headManifest errors not making sense out of context. WDYT about prepending them with `HEAD <url>: `?

Good call, done.

Added tests.
jonjohnsonjr: One final nit is that the reason this error was particularly confusing is here: https://github.com/google/go-containerregistry/blob/4a92f6cd028daf021b9f29b0b2bc9f10b422e2fb/pkg/v1/hash.go#L90

In particular, "too many" is definitely wrong for `NewHash("")`.

Can we make that slightly better?
ImJasonH: 
> Can we make that slightly better?

Updated to `return fmt.Errorf("cannot parse hash: %q", unquoted)`

Patch:
diff --git a/pkg/crane/digest.go b/pkg/crane/digest.go
index 7fa774b6..697cc4af 100644
--- a/pkg/crane/digest.go
+++ b/pkg/crane/digest.go
@@ -14,6 +14,8 @@
 
 package crane
 
+import "github.com/google/go-containerregistry/pkg/logs"
+
 // Digest returns the sha256 hash of the remote image at ref.
 func Digest(ref string, opt ...Option) (string, error) {
 	o := makeOptions(opt...)
@@ -39,7 +41,12 @@ func Digest(ref string, opt ...Option) (string, error) {
 	}
 	desc, err := head(ref, opt...)
 	if err != nil {
-		return "", err
+		logs.Warn.Printf("HEAD request failed, falling back on GET: %v", err)
+		rdesc, err := getManifest(ref, opt...)
+		if err != nil {
+			return "", err
+		}
+		return rdesc.Digest.String(), nil
 	}
 	return desc.Digest.String(), nil
 }
diff --git a/pkg/v1/hash.go b/pkg/v1/hash.go
index 7fdf22e9..e9630087 100644
--- a/pkg/v1/hash.go
+++ b/pkg/v1/hash.go
@@ -87,7 +87,7 @@ func Hasher(name string) (hash.Hash, error) {
 func (h *Hash) parse(unquoted string) error {
 	parts := strings.Split(unquoted, ":")
 	if len(parts) != 2 {
-		return fmt.Errorf("too many parts in hash: %s", unquoted)
+		return fmt.Errorf("cannot parse hash: %q", unquoted)
 	}
 
 	rest := strings.TrimLeft(parts[1], "0123456789abcdef")
diff --git a/pkg/v1/remote/descriptor.go b/pkg/v1/remote/descriptor.go
index 9c248cfe..6d7ecb71 100644
--- a/pkg/v1/remote/descriptor.go
+++ b/pkg/v1/remote/descriptor.go
@@ -324,14 +324,26 @@ func (f *fetcher) headManifest(ref name.Reference, acceptable []types.MediaType)
 		return nil, err
 	}
 
-	mediaType := types.MediaType(resp.Header.Get("Content-Type"))
+	mth := resp.Header.Get("Content-Type")
+	if mth == "" {
+		return nil, fmt.Errorf("HEAD %s: response did not include Content-Type header", u.String())
+	}
+	mediaType := types.MediaType(mth)
 
-	size, err := strconv.ParseInt(resp.Header.Get("Content-Length"), 10, 64)
+	lh := resp.Header.Get("Content-Length")
+	if lh == "" {
+		return nil, fmt.Errorf("HEAD %s: response did not include Content-Length header", u.String())
+	}
+	size, err := strconv.ParseInt(lh, 10, 64)
 	if err != nil {
 		return nil, err
 	}
 
-	digest, err := v1.NewHash(resp.Header.Get("Docker-Content-Digest"))
+	dh := resp.Header.Get("Docker-Content-Digest")
+	if dh == "" {
+		return nil, fmt.Errorf("HEAD %s: response did not include Docker-Content-Digest header", u.String())
+	}
+	digest, err := v1.NewHash(dh)
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/pkg/crane/digest_test.go b/pkg/crane/digest_test.go
new file mode 100644
index 00000000..ac215d33
--- /dev/null
+++ b/pkg/crane/digest_test.go
@@ -0,0 +1,61 @@
+// Copyright 2021 Google LLC All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package crane
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+
+	"github.com/google/go-containerregistry/pkg/v1/types"
+)
+
+func TestDigest_MissingDigest(t *testing.T) {
+	response := []byte("doesn't matter")
+	digest := "sha256:477c34d98f9e090a4441cf82d2f1f03e64c8eb730e8c1ef39a8595e685d4df65" // Digest of "doesn't matter"
+	getCalled := false
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/v2/" {
+			w.WriteHeader(http.StatusOK)
+			return
+		}
+		w.Header().Set("Content-Type", string(types.DockerManifestSchema2))
+		if r.Method == http.MethodGet {
+			getCalled = true
+			w.Header().Set("Docker-Content-Digest", digest)
+		}
+		// This will automatically set the Content-Length header.
+		w.Write(response)
+	}))
+	defer server.Close()
+	u, err := url.Parse(server.URL)
+	if err != nil {
+		t.Fatalf("url.Parse(%v) = %v", server.URL, err)
+	}
+
+	got, err := Digest(fmt.Sprintf("%s/repo:latest", u.Host))
+	if err != nil {
+		t.Fatalf("Digest: %v", err)
+	}
+	if got != digest {
+		t.Errorf("Digest: got %q, want %q", got, digest)
+	}
+	if !getCalled {
+		t.Errorf("Digest: expected GET to be called")
+	}
+}
diff --git a/pkg/v1/remote/descriptor_test.go b/pkg/v1/remote/descriptor_test.go
index dd4791da..9b093b2b 100644
--- a/pkg/v1/remote/descriptor_test.go
+++ b/pkg/v1/remote/descriptor_test.go
@@ -20,6 +20,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -174,3 +175,42 @@ func TestHeadSchema1(t *testing.T) {
 		t.Errorf("Descriptor.Size = %q, expected %q", desc.Size, len(response))
 	}
 }
+
+// TestHead_MissingHeaders tests that HEAD responses missing necessary headers
+// result in errors.
+func TestHead_MissingHeaders(t *testing.T) {
+	missingType := "missing-type"
+	missingLength := "missing-length"
+	missingDigest := "missing-digest"
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/v2/" {
+			w.WriteHeader(http.StatusOK)
+			return
+		}
+		if r.Method != http.MethodHead {
+			t.Errorf("Method; got %v, want %v", r.Method, http.MethodHead)
+		}
+		if !strings.Contains(r.URL.Path, missingType) {
+			w.Header().Set("Content-Type", "My-Media-Type")
+		}
+		if !strings.Contains(r.URL.Path, missingLength) {
+			w.Header().Set("Content-Length", "10")
+		}
+		if !strings.Contains(r.URL.Path, missingDigest) {
+			w.Header().Set("Docker-Content-Digest", "sha256:0000000000000000000000000000000000000000000000000000000000000000")
+		}
+	}))
+	defer server.Close()
+	u, err := url.Parse(server.URL)
+	if err != nil {
+		t.Fatalf("url.Parse(%v) = %v", server.URL, err)
+	}
+
+	for _, repo := range []string{missingType, missingLength, missingDigest} {
+		tag := mustNewTag(t, fmt.Sprintf("%s/%s:latest", u.Host, repo))
+		if _, err := Head(tag); err == nil {
+			t.Errorf("Head(%q): expected error, got nil", tag)
+		}
+	}
+}