[go: nahoru, domu]

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

OCI Distribution Improvements #682

Merged
merged 7 commits into from
Oct 8, 2021

Conversation

kflansburg
Copy link
Collaborator
@kflansburg kflansburg commented Sep 24, 2021

This PR includes a number of improvements for OCI distribution:

  • HEAD requests are attempted for fetching manifest digest, with a fallback to GET (some providers to do not include the digest in the HEAD response). This appears to be preferred by registry providers such as DockerHub, because they do not apply rate limits to HEAD requests. I have confirmed that fetch_manifest_digest, including any authentication API calls, does not incur ratelimits.
  • JWT tokens are now cached by (registry, repository, operation), instead of just registry. Previously, requests using the same client to perform different operations, or access different repositories in the same registry would fail, because tokens are scoped to the repository.
  • Tokens are now stored in an expiring cache, which inspects the JWT for its expiration timestamp, and returns None if a token is in the cache but is expired.

Combined, these changes should make it possible to reuse the OCI client over long periods of time, with more efficient registry API usage.

Other optimizations which could be made:

  • If a pull request is made, it can use a push token.
  • We may want to scope tokens by auth as well, you could have a token obtained anonymously, and one obtained with auth, and they would conflict.

Signed-off-by: Kevin Flansburg <kevin.flansburg@gmail.com>
Signed-off-by: Kevin Flansburg <kevin.flansburg@gmail.com>
Signed-off-by: Kevin Flansburg <kevin.flansburg@gmail.com>
Signed-off-by: Kevin Flansburg <kevin.flansburg@gmail.com>
@bacongobbler
Copy link
Collaborator
bacongobbler commented Oct 4, 2021

These are all great changes, and they map back to OCI's expectations as to how these JWTs should be treated.

Have you had a chance to look at the failing CI checks yet? Looks like legitimate type type mismatch and compiler errors.

@flavio
Copy link
Contributor
flavio commented Oct 5, 2021

I looked at the changes too, this looks good 👏

Signed-off-by: Kevin Flansburg <kevin.flansburg@gmail.com>
Signed-off-by: Kevin Flansburg <kevin.flansburg@gmail.com>
@kflansburg
Copy link
Collaborator Author

@bacongobbler Ok, I merged main to reconcile the changes introduced from #670 . I've also addressed the type errors in the test.

I dont believe the license error from cargo deny is related to this PR.

Signed-off-by: Kevin Flansburg <kevin.flansburg@gmail.com>
Copy link
Contributor
@flavio flavio left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for having fixed the tests.

I agree with you about the license check, the failure doesn't seem to be related with this PR

@kflansburg kflansburg merged commit 2275956 into krustlet:main Oct 8, 2021
@kflansburg kflansburg deleted the kflansburg/oci-head branch October 8, 2021 11:39
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