Pass docker auth when retrieving bundle metadata #2366
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carolynvs
force-pushed
the
fix-bundle-check-auth
branch
from
a05f8f0 to
4367243
Compare
VinozzZ
approved these changes
Sep 20, 2022
|
Do you mean the part about what crane does by default vs go-container registry? |
|
When I first implemented this function, I used go-containerregistry instead of crane because I was returning more than just the digest. Now that the function only returns digest, I can just use crane. I've added another commit that re-implements the function using just crane. That way we don't need to explain the differences between how the two libraries authenticate. What do you think? |
carolynvs
force-pushed
the
fix-bundle-check-auth
branch
2 times, most recently
from
d9ff9e7 to
d73d0d0
Compare
Before we publish or copy a bundle, we retrieve the destination bundle's metadata as a way to check if it already exists, and requires --force to overwrite. By default crane passes the current keychain when interacting with registries. Crane uses go-containerregistry under the hood and at the time I didn't realize that it was crane, and not go-containerregistry specifying the authentication. The go-containerregistry library authenticates as anonymous by default. So when a bundle was published to a new location, we would get a 401 auth error because authentication was not supplied, when we expected a 404 to tell that the bundle didn't exist at the destination location. I have updated the implementation of GetBundleMetadata to use crane for consistency and consolidated the set up of the crane options. Signed-off-by: Carolyn Van Slyck <[email protected]>
TestRegistry requires that the current user is logged into ghcr.io in order to test that we are passing authentication properly. Signed-off-by: Carolyn Van Slyck <[email protected]>
These tests are slow, speed them up! Signed-off-by: Carolyn Van Slyck <[email protected]>
carolynvs
force-pushed
the
fix-bundle-check-auth
branch
from
f060cbe to
b579f27
Compare
|
@VinozzZ I have updated the solution to use crane only. Can you take another look? |
Looks good to me |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change
Before we publish or copy a bundle, we retrieve the destination bundle's metadata as a way to check if it already exists, and requires --force to overwrite. By default crane passes the current keychain when interacting with registries. Crane uses go-containerregistry under the hood and at the time I didn't realize that it was crane, and not go-containerregistry specifying the authentication. The go-containerregistry library authenticates as anonymous by default.
So when a bundle was published to a new location, we would get a 401 auth error because authentication was not supplied, when we expected a 404 to tell that the bundle didn't exist at the destination location. I have fixed our call to get the bundle metadata so that we pass in the keychain.
What issue does it fix
Closes #2365
Notes for the reviewer
N/A
Checklist
Reviewer Checklist