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(controller): add rudimentary ecr and artifact registry support #2037

Merged
merged 4 commits into from
May 21, 2024

Conversation

krancour
Copy link
Member

@krancour krancour commented May 17, 2024

Follow up to #2018. Does not quite solve #1885 or #2029 because this does not use workload identity.

As noted in #1885, a possible workaround was to use a CronJob to periodically exchange AWS access key ID and secret access key for a short-lived ECR authorization token (12 hours). The token is of the form base64(AWS:<temporary-passsword>).

Similarly, a GCP service account key (which is structured data) can be exchanged for a GCP access token (one hour) for Artifact Registry. (I believe it works for GCR as well, although GCR is deprecated and as of a few days ago, new repos cannot be created in GCR.) This is an oauth2 access token, which can also be presented as a password (along with username oauth2accesstoken) when using basic auth.

This PR introduces functionality that seamlessly makes these exchanges. It also caches tokens to avoid excessive calls to the AWS and GCP APIs.

Why are we doing this first and not jumping straight to using workload identity?

Good question. Workload identity will not help a Kargo instance running outside of AWS to access ECR and it will not help a Kargo instance running outside of GCP to access Artifact Registry -- which makes this feature a lowest common denominator / universal solution for ECR and Artifact Registry integration.

Note: ACR (Azure Container Registry) does not require anything special because ACR provides for long-lived, user-managed tokens, with or without an expiration date.

I will circle back to add workload identity-based options for all three major cloud providers in a follow-up(s). See #1885 and #2029.

Docs to be included in a follow-up as well.

Copy link

netlify bot commented May 17, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit 1d3d95d
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/664bb81d3a0a7f00085b6734
😎 Deploy Preview https://deploy-preview-2037.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@krancour krancour force-pushed the krancour/ecr-support branch from 5359c47 to adf3b76 Compare May 17, 2024 19:27
@krancour krancour added this to the v0.7.0 milestone May 17, 2024
Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 60.17699% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 45.71%. Comparing base (1180bef) to head (1d3d95d).

Files Patch % Lines
internal/credentials/credentials.go 22.72% 15 Missing and 2 partials ⚠️
internal/credentials/ecr/ecr.go 70.90% 14 Missing and 2 partials ⚠️
internal/credentials/gcp/gcp.go 66.66% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2037      +/-   ##
==========================================
+ Coverage   45.62%   45.71%   +0.09%     
==========================================
  Files         234      236       +2     
  Lines       15998    16109     +111     
==========================================
+ Hits         7299     7365      +66     
- Misses       8342     8383      +41     
- Partials      357      361       +4     

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

@krancour krancour marked this pull request as ready for review May 17, 2024 19:54
@krancour krancour requested a review from a team as a code owner May 17, 2024 19:54
@krancour krancour force-pushed the krancour/ecr-support branch 3 times, most recently from 7422b31 to e115f55 Compare May 18, 2024 18:08
krancour added 3 commits May 18, 2024 14:52
…horization token

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
… token

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
@krancour krancour force-pushed the krancour/ecr-support branch from e115f55 to 870c338 Compare May 18, 2024 18:55
@krancour krancour changed the title feat(controller): add rudimentary ecr support feat(controller): add rudimentary ecr and artifact registry support May 18, 2024
@krancour
Copy link
Member Author

krancour commented May 18, 2024

@hiddeco note that I've amended what you already reviewed to add Google Artifact Registry (and GCR?) support.

ctx context.Context, credType Type, secret *corev1.Secret,
) (Credentials, error) {
if credType == TypeImage {
// If the cred type is image, we'll try to derive username and password
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if, at some point, it may be better to identify the type of credentials we are dealing with based on some user-defined value. Instead of the brute-force approach that's being taken now.

@krancour krancour added this pull request to the merge queue May 21, 2024
Merged via the queue into akuity:main with commit 3e59bc8 May 21, 2024
17 checks passed
@krancour krancour deleted the krancour/ecr-support branch May 21, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants