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

add remote.Head #770

Merged
merged 2 commits into from
Sep 16, 2020
Merged

add remote.Head #770

merged 2 commits into from
Sep 16, 2020

Conversation

vito
Copy link
Contributor

@vito vito commented Sep 16, 2020

fixes #769

Lend me your nitpicks! There's a bit of copypasta here but I didn't want to over-aggressively DRY things up.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the example test failed -- it talks to dockerhub, which we should fix.

You can just skip it to fix travis for now.

pkg/v1/remote/descriptor.go Show resolved Hide resolved
pkg/v1/remote/descriptor.go Outdated Show resolved Hide resolved
@vito
Copy link
Contributor Author

vito commented Sep 16, 2020

One thing I noticed when trying to use this: for multi-platform images, the digest you'll get back from remote.Head(ref) will be different from the digest you'll get from remote.Image(ref).Digest(), which is what we used before.

The former is the digest of the "manifest list", while the latter is the digest of the manifest for your particular platform, contained in said list.

I'm guessing there's no real way around this, since the platforms are discovered through the manifest, and the whole point of this is to not fetch the manifest. It's probably fine for our use case - it just means that the digest will change when any of the platforms change.

@jonjohnsonjr
Copy link
Collaborator

I'm guessing there's no real way around this, since the platforms are discovered through the manifest, and the whole point of this is to not fetch the manifest.

I don't believe there is a general solution to this. As you said, the platform resolution fundamentally requires parsing the manifest.

It's possible to request the amd64/linux image from the registry by not including manifest lists in the accept headers, but that only solves a subset of the problem and (IMO) makes things messier... I'm also pretty sure it only works if you request schema 1 images, which are deprecated, so probably not even a partial solution here.

If the manifest being requested uses the new format, and the appropriate media type is not present in an Accept header, the registry will assume that the client cannot handle the manifest as-is, and rewrite it on the fly into the old format. If the object that would otherwise be returned is a manifest list, the registry will look up the appropriate manifest for the amd64 platform and linux OS, rewrite that manifest into the old format if necessary, and return the result to the client. If no suitable manifest is found in the manifest list, the registry will return a 404 error.

https://docs.docker.com/registry/spec/manifest-v2-2/#backward-compatibility

It's probably fine for our use case - it just means that the digest will change when any of the platforms change.

In my experience, child manifests usually change together, so I imagine the false positive rate would be low. If you wanted, you could check the mediaType and resolve the list to whichever image you care about for manifest lists, but you need to know the desired platform ahead of time. I'm not familiar enough with your system to know if that's possible.

If possible, I'd just use the digest of the manifest list everywhere. Runtimes should know how to pull manifest lists, so the end result tends to be the same. I ran into issues with this on cri-o last year but they've since been resolved: knative/serving#3997

cc @julz you may want to pick up this PR for knative digest resolution once it merges

this is useful for fetching a reference's digest/size/type via a HEAD
request, which does not count towards Docker Hub image pull rate limits

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@jonjohnsonjr
Copy link
Collaborator

err: exit status 1: stderr: go: inconsistent vendoring in /home/travis/gopath/src/github.com/google/go-containerregistry:
	golang.org/x/tools@v0.0.0-20200916195026-c9a70fc28ce3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	golang.org/x/tools@v0.0.0-20200911153331-7ad463ce66dd: is marked as explicit in vendor/modules.txt, but not explicitly required in go.mod
run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory

./hack/presubmit.sh is your friend here.

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@vito
Copy link
Contributor Author

vito commented Sep 16, 2020

./hack/presubmit.sh is your friend here.

Woops! I ran it and saw the same failures but didn't expect my changes to affect any dependencies, so I assumed something was wrong on my machine. Pushed a commit to resolve this.

Side note: I haven't skipped the failing test from before but I'll push a commit for that too if it flakes out again.

Thanks for your help!

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2020

Codecov Report

Merging #770 into master will decrease coverage by 0.16%.
The diff coverage is 60.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #770      +/-   ##
==========================================
- Coverage   79.02%   78.86%   -0.17%     
==========================================
  Files         102      102              
  Lines        4735     4778      +43     
==========================================
+ Hits         3742     3768      +26     
- Misses        550      559       +9     
- Partials      443      451       +8     
Impacted Files Coverage Δ
pkg/v1/remote/descriptor.go 76.92% <60.46%> (-5.10%) ⬇️

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 d7f8d06...710b911. Read the comment docs.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this!

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.

Ability to send a HEAD request to get a manifest's digest
3 participants