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: support alibaba cloud rrsa store auth provider #1909

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

DahuK
Copy link
Contributor

@DahuK DahuK commented Nov 3, 2024

Description

What this PR does / why we need it:

Support using Alibaba Cloud RAM Roles for Service Accounts(RRSA) to pull artifacts from a private Alibaba Cloud Container Registry (ACR) with an ACR specific auth token.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • basic unit test cases to create/enable/provide alibabacloud acr basic auth provider

In addition, I have local built the testing image contains the changes in this PR,and tested in the Alibaba Cloud ACK cluster to verify fetch the signature metadata of a private images in the Aliababa Cloud ACR repository by using the store authProvider based on the new RRSA configuration. The more instructions will be added in the subsequent document PR.

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

Post Merge Requirements

  • MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

Copy link

codecov bot commented Nov 3, 2024

Codecov Report

Attention: Patch coverage is 87.60331% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../authprovider/alibabacloud/alibabacloudacrbasic.go 86.23% 11 Missing and 4 partials ⚠️
Files with missing lines Coverage Δ
...kg/common/oras/authprovider/alibabacloud/helper.go 100.00% <100.00%> (ø)
...kg/common/oras/authprovider/authproviderfactory.go 62.85% <ø> (ø)
pkg/referrerstore/oras/oras.go 73.91% <ø> (-6.22%) ⬇️
.../authprovider/alibabacloud/alibabacloudacrbasic.go 86.23% <86.23%> (ø)

... and 28 files with indirect coverage changes

@DahuK DahuK force-pushed the alibabacloud-rrsa branch 4 times, most recently from 9338d47 to af6bfa4 Compare November 4, 2024 12:50
Copy link
Collaborator

@susanshi susanshi left a comment

Choose a reason for hiding this comment

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

Hi @DahuK ,
thanks so much for adding the alibaba auth provider. The group of us have discussed during the PR review meeting, and here are the notes:

  • we are aiming for unit test coverage at 80%, would it be possible to add more coverage for alibabacloudacrauthtypes.go and alibabacloudacrbasic.go
  • maybe as a sperate PR, are you looking to add e2e test similar to azure-test.bats
  • also as a seperate PR, might be good to add to auth provider docs at supported auth providers

@susanshi
Copy link
Collaborator

susanshi commented Nov 5, 2024

Hi @DahuK ,please also review the lint report and resolve the merge conflict. let us know if you have any questions

charts/ratify/README.md Outdated Show resolved Hide resolved
charts/ratify/templates/store.yaml Outdated Show resolved Hide resolved
pkg/common/oras/authprovider/authproviderfactory.go Outdated Show resolved Hide resolved
pkg/common/oras/authprovider/alibabacloud/helper.go Outdated Show resolved Hide resolved
pkg/common/oras/authprovider/alibabacloud/helper.go Outdated Show resolved Hide resolved
@DahuK DahuK closed this Nov 8, 2024
@DahuK DahuK reopened this Nov 8, 2024
@DahuK DahuK force-pushed the alibabacloud-rrsa branch 2 times, most recently from ff17ccc to 3426cde Compare November 8, 2024 06:18
@DahuK
Copy link
Contributor Author

DahuK commented Nov 8, 2024

Hi @DahuK , thanks so much for adding the alibaba auth provider. The group of us have discussed during the PR review meeting, and here are the notes:

  • we are aiming for unit test coverage at 80%, would it be possible to add more coverage for alibabacloudacrauthtypes.go and alibabacloudacrbasic.go
  • maybe as a sperate PR, are you looking to add e2e test similar to azure-test.bats
  • also as a seperate PR, might be good to add to auth provider docs at supported auth providers

Thanks your reviewing and sorry for the late response!

  • I have added some ut for the coverage
  • For company compliance policies we may not be able to provide our credentials as accesskey/secret, however we may be able to add some e2e bats for local testing in another PR
  • Yes, I will add the provider docs this weekend in another PR

Copy link
Collaborator

@binbin-li binbin-li left a comment

Choose a reason for hiding this comment

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

left some minor comments, lgtm overall

Copy link
Collaborator

@susanshi susanshi left a comment

Choose a reason for hiding this comment

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

thanks for the update! left some clarification questions

@DahuK DahuK force-pushed the alibabacloud-rrsa branch 2 times, most recently from 8dcc3f1 to 3766140 Compare November 13, 2024 13:15
@susanshi
Copy link
Collaborator

susanshi commented Nov 14, 2024

Hi @DahuK , thanks for the latest updates, the change looks good. Please resolve the latest go.mod conflicts , this PR is almost ready to merge. @junczhu , @binbin-li please take a look if conversation should be resolved. thanks all

Copy link
Collaborator

@binbin-li binbin-li 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 adding support for alibaba and unit tests!

@binbin-li
Copy link
Collaborator

@DahuK the PR is good to go, could you sign the commit following it: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@DahuK DahuK force-pushed the alibabacloud-rrsa branch 3 times, most recently from 2ad167f to a883ce3 Compare November 14, 2024 11:22
@DahuK
Copy link
Contributor Author

DahuK commented Nov 14, 2024

@DahuK the PR is good to go, could you sign the commit following it: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Thanks! the commit is signed.

@binbin-li binbin-li enabled auto-merge (squash) November 14, 2024 12:40
@binbin-li binbin-li merged commit 0837c02 into ratify-project:dev Nov 14, 2024
20 checks passed
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.

4 participants