Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crane digest: fallback to GET when HEAD fails #971

Merged
merged 2 commits into from
Mar 22, 2021

Conversation

imjasonh
Copy link
Collaborator

@imjasonh imjasonh commented Mar 20, 2021

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
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #971 (5e8022c) into main (4a92f6c) will increase coverage by 0.04%.
The diff coverage is 94.11%.

❗ Current head 5e8022c differs from pull request most recent head def730a. Consider uploading reports for the commit def730a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
+ Coverage   75.19%   75.23%   +0.04%     
==========================================
  Files         107      107              
  Lines        4837     4850      +13     
==========================================
+ Hits         3637     3649      +12     
- Misses        669      670       +1     
  Partials      531      531              
Impacted Files Coverage Δ
pkg/crane/digest.go 68.18% <80.00%> (+1.51%) ⬆️
pkg/v1/remote/descriptor.go 75.14% <100.00%> (+1.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a92f6c...def730a. Read the comment docs.

@jonjohnsonjr
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

One final nit is that the reason this error was particularly confusing is here:

return fmt.Errorf("too many parts in hash: %s", unquoted)

In particular, "too many" is definitely wrong for NewHash("").

Can we make that slightly better?

@imjasonh
Copy link
Collaborator Author

Can we make that slightly better?

Updated to return fmt.Errorf("cannot parse hash: %q", unquoted)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants