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

feat: integrate Alibaba Cloud Container Registry cred helper #2008

Merged
merged 7 commits into from Aug 29, 2022

Conversation

mozillazg
Copy link
Contributor

Summary

Integrate a new keychain to support Alibaba Cloud Container Registry.

Ticket Link

Fixes #2007

Release Note

feat: integrate Alibaba Cloud Container Registry cred helper

@mozillazg mozillazg force-pushed the support-alibaba-acr-keychain branch from 85b58a1 to 1024cf4 Compare June 18, 2022 13:46
Integrate a new keychain to support Alibaba Cloud Container Registry
when run with the `--k8s-keychain` flag.

Resolves: sigstore#2007

Signed-off-by: mozillazg <mozillazg101@gmail.com>
@mozillazg mozillazg force-pushed the support-alibaba-acr-keychain branch from 1024cf4 to d1ac93c Compare June 18, 2022 14:33
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2022

Codecov Report

Merging #2008 (e4fbac0) into main (8f29f03) will increase coverage by 0.65%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2008      +/-   ##
==========================================
+ Coverage   26.03%   26.69%   +0.65%     
==========================================
  Files         131      131              
  Lines        7689     7690       +1     
==========================================
+ Hits         2002     2053      +51     
+ Misses       5445     5376      -69     
- Partials      242      261      +19     
Impacted Files Coverage Δ
cmd/cosign/cli/options/registry.go 0.00% <0.00%> (ø)
pkg/blob/load.go 74.28% <0.00%> (+5.71%) ⬆️
pkg/oci/layout/index.go 28.57% <0.00%> (+28.57%) ⬆️
pkg/oci/layout/write.go 37.50% <0.00%> (+37.50%) ⬆️
pkg/oci/layout/signatures.go 53.84% <0.00%> (+53.84%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: mozillazg <mozillazg101@gmail.com>
@mozillazg
Copy link
Contributor Author

@imjasonh Anything I can help with to get this PR approved?

@@ -85,6 +86,7 @@ func (o *RegistryOptions) GetRegistryClientOpts(ctx context.Context) []remote.Op
google.Keychain,
authn.NewKeychainFromHelper(ecr.NewECRHelper(ecr.WithLogger(ioutil.Discard))),
authn.NewKeychainFromHelper(credhelper.NewACRCredentialsHelper()),
authn.NewKeychainFromHelper(alibabaacr.NewACRHelper().WithLoggerOut(ioutil.Discard)),
Copy link
Member

Choose a reason for hiding this comment

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

It's not really your responsibility, but I'd like to document this behavior a little better in general.

Along with that, the --k8s-keychain flag that controls this is somewhat poorly named -- it always has been, but it's especially confusing if we add a cred helper that isn't included in the standard k8s cred helpers, and it'll only get worse if we add more in the future.

Again, this isn't really your fault, and I don't think we need to change anything in this PR, just noting it for whomever decides to take this on in the future (likely me).

imjasonh
imjasonh previously approved these changes Jun 29, 2022
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

LGTM, we'll need approval from owners to get it merged though.

…-keychain

Signed-off-by: mozillazg <mozillazg101@gmail.com>
@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

Signed-off-by: mozillazg <mozillazg101@gmail.com>
@mozillazg
Copy link
Contributor Author

LGTM, we'll need approval from owners to get it merged though.

@dlorenc ping~

dlorenc
dlorenc previously approved these changes Aug 20, 2022
Signed-off-by: mozillazg <mozillazg101@gmail.com>
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.

feat: integrate Alibaba Cloud Container Registry cred helper
4 participants