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

Add AWS credential provider #825

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marcelobartsch-jt
Copy link

This code allows argo image updater to use ECR repos natively.

it is very basic code but get the job done, feel free to comment what is missing so I can update and improve this code.

thanks!

Signed-off-by: Marcelo Bartsch <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2024

Codecov Report

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

Project coverage is 73.20%. Comparing base (65698c5) to head (0c101c9).

Files Patch % Lines
pkg/image/credentials.go 16.66% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #825      +/-   ##
==========================================
- Coverage   73.53%   73.20%   -0.34%     
==========================================
  Files          31       31              
  Lines        3140     3157      +17     
==========================================
+ Hits         2309     2311       +2     
- Misses        695      710      +15     
  Partials      136      136              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jannfis
Copy link
Contributor

jannfis commented Aug 12, 2024

Thank you, I really do like this PR and the intent behind it.

However, I do have slight concerns about maintainability here. To maintain this, one needs to have an aws account and ECR set up. How would this be integrated into a potential end-to-end test? Who will be the one to take care of issues and bugs reported to this particular functionality?

Also, when we start incorporating proprietary authentication for vendor no. 1, then we're not far away from requests to incorporating similar mechanisms for vendor no. 2, vendor no. 3 and so on. This would multiply the impact described in the previous paragraph.

All that being said, please don't close or abandon this PR. I just want to raise these concerns to open up the discussion. Thank you!

@marcelobartsch-jt
Copy link
Author

About E2E test, they can be automated, of course you will need some infrastructure , or tools like localstack which emulate AWS, for other cloud platforms, no idea ,other option is to use mocked services,sadly I'm not that knowledged on testing in golang to implement , but is doable.
Now the big question is , no new features because we don't have the infra for testing, put those features as second class citizens.

Now in the case of new ones, you can just say 'Sure, provide a PR' , but I think is a natural path for people wanting to use this with less complications as possible, because, I could use the 'ext:/scripts/aws.sh' , for example, and just use that, but then again I need to modify the base image or mount it, but that might need aws cli, python and its depedencies.
Now another approach is to put those 'auth' as 'external' and provide a way to compile them apart, and instead of use 'aws' for the credentials, just use 'ext:/local/aws-ecr-auth', so users , like me, don't need to add python, and aws cli, and all it's dependencies to the base image.

Again, just thinking out loud looking to expand the discussion!

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.

3 participants