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

Fix cross account issue #17533

Merged
merged 1 commit into from
Sep 15, 2022
Merged

Conversation

vincentni
Copy link
Contributor

@vincentni vincentni commented Sep 9, 2022

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Pass in the correct endpoint to AWS ECR client config, so at the time of fetching token, the token will be valid for the target endpoint.

Issue being fixed

Fixes #(issue)
Currently, when AWS ECR client is created, it defaults the endpoint to whatever AWS_ACCESS_KEY_ID belongs to and thus falls short in the cross account case. The proposed fix sets the endpoint explicitly so it works in all cases.

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@vincentni vincentni requested a review from a team as a code owner September 9, 2022 23:20
@vincentni vincentni force-pushed the aws-ecr-cross-account-fix branch 2 times, most recently from 7385a82 to 41ffea1 Compare September 9, 2022 23:32
@vincentni
Copy link
Contributor Author

vincentni commented Sep 9, 2022

@bitsf @stonezdj @steven-zou @ywk253100 Could you please give it a look? Thank you!

Signed-off-by: Vincent Ni <[email protected]>
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #17533 (6714d59) into main (9573cd7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17533      +/-   ##
==========================================
+ Coverage   67.25%   67.28%   +0.02%     
==========================================
  Files         990      990              
  Lines       84048    84048              
  Branches     2625     2625              
==========================================
+ Hits        56526    56548      +22     
+ Misses      23674    23650      -24     
- Partials     3848     3850       +2     
Flag Coverage Δ
unittests 67.28% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pkg/reg/adapter/awsecr/adapter.go 70.24% <100.00%> (ø)
...es/vulnerability/vulnerability-config.component.ts 58.51% <0.00%> (-4.45%) ⬇️
src/common/utils/passports.go 89.74% <0.00%> (+5.12%) ⬆️
src/controller/event/topic.go 9.00% <0.00%> (+7.20%) ⬆️
src/lib/cache/helper.go 82.60% <0.00%> (+8.69%) ⬆️
src/core/api/internal.go 46.80% <0.00%> (+10.63%) ⬆️
src/controller/event/handler/auditlog/auditlog.go 56.52% <0.00%> (+47.82%) ⬆️

@vincentni
Copy link
Contributor Author

@bitsf @chlins all tests passed

Copy link
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

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

lgtm

@vincentni
Copy link
Contributor Author

@bitsf @chlins thanks for the approvals! please feel free to merge it as I don't have write permission.

@chlins chlins merged commit a3d9600 into goharbor:main Sep 15, 2022
@vincentni vincentni deleted the aws-ecr-cross-account-fix branch September 15, 2022 06:28
@vincentni vincentni mentioned this pull request Sep 20, 2022
5 tasks
sluetze pushed a commit to sluetze/harbor that referenced this pull request Oct 29, 2022
mcsage pushed a commit to mcsage/harbor that referenced this pull request Feb 16, 2023
Signed-off-by: Vincent Ni <[email protected]>
Signed-off-by: Stephan Hohn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/update Update or Fix replication/adapters related to replication adapters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replication to Cross-account AWS ECR
3 participants