-
Notifications
You must be signed in to change notification settings - Fork 28
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 ecr-cred-injector #978
Conversation
9e9e04f
to
56a3636
Compare
FROM gcr.io/distroless/static:debug-nonroot | ||
WORKDIR / | ||
COPY --from=builder /workspace/ecr-token-refresh . | ||
USER 65532:65532 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a comment where this group/user number comes from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
} | ||
|
||
if !IsECRRegistry(registry) { | ||
a.log.Info("defaultRegistry is not ECR registry, skip injecting credential to docker config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we missing a return here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good caught
@@ -146,6 +148,10 @@ spec: | |||
defaultMode: 420 | |||
secretName: ecr-token | |||
optional: true | |||
- name: aws-secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about aws-config-secret
? or is aws-secret
the convention elsewhere?
checking my understanding: we can't use the existing ecr-token secret here because we don't have control over its renewal lifecycle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"aws-secret" contains a field called "config", calling it "aws-config-secret" may imply that the volume is the "config" of "aws-secret".
ecr-token stores "username:password", it's not refreshed anymore perhaps because we have migrated to use credential-provider.
56a3636
to
3794f70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: d8660091, jonathanmeier5 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #, if available:
Description of changes: This change will allow packages controller to be able to pull charts from private ECR. It injects the credential(username:password) to the docker config file of the package controller container.
Manually tested that the injector works for regional ECR
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.