-
Notifications
You must be signed in to change notification settings - Fork 319
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] ecr-credential-provider support to authenticate public registries #603
Conversation
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @mmerkes. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
cc @jlbutler |
cmd/ecr-credential-provider/main.go
Outdated
expiresAt *time.Time | ||
} | ||
|
||
func (e *ecrPlugin) getPublicCredsData() (credsData, error) { |
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.
Could we switch to the following signature to return nil
instead of empty data structure? credsData{}
func (e *ecrPlugin) getPublicCredsData() (*credsData, error)
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.
Ya. I'll update that everywhere.
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.
thanks!
cmd/ecr-credential-provider/main.go
Outdated
}, nil | ||
} | ||
|
||
func (e *ecrPlugin) getPrivateCredsData(image string) (credsData, error) { |
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.
Same as above. Could we switch to the following signature to return nil
instead of empty data structure? credsData{}
func (e *ecrPlugin) getPublicCredsData() (*credsData, error)
|
||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/klog/v2" | ||
"k8s.io/kubelet/pkg/apis/credentialprovider/v1" | ||
) | ||
|
||
const ecrPublicRegion string = "us-east-1" |
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.
@jlbutler any comments about reachability of ECR public?
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.
AFAIK, this is the only region that has an ECR public endpoint. While the endpoint would be reachable from any non-isolated partitions, it won't be able to make authenticated calls from other partitions because you'd need a partition-specific endpoint so that the IAM creds work correctly. If there are endpoints in any other partitions, I can add support for those 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.
Correct, for the purposes here (using the SDK to make API calls for auth) us-east-1 is the region
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.
thanks @jlbutler
var creds credsData | ||
var err error | ||
|
||
if strings.Contains(image, ecrPublicURL) { |
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.
HasPrefix
?
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.
Ya, I had considered that. However, this logic made me this that it could have https://
in the image string:
func parseRepoURL(image string) (string, string, string, error) {
if !strings.Contains(image, "https://") {
image = "https://" + image
}
It could be that the if statement is overly defensive, but I took the same approach 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.
👍🏾
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, nckturner 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
ecr-credential-provider can now authenticate public registries, which allows users to access larger ECR data transfer limits. See #602 for more details. This will not work outside of the
aws
partition as the ECR public endpoint is only inus-east-1
and it requires IAM authentication.To enable this, you may need a couple of changes in your nodes:
public.ecr.aws
tomatchImages
in yourCredentialProviderConfig
. See below for an example.ecr-public:GetAuthorizationToken
andsts:GetServiceBearerToken
permissions. See below for an example policy.Which issue(s) this PR fixes:
Fixes #602
Special notes for your reviewer:
For testing, I created a 1.26 EKS cluster with an AL2 nodegroup in
us-west-2
, built theecr-credential-provider
, uploaded to the nodes and used the below image credential provider config:I deployed the EKS sample app and verified the following:
GetAuthorizationToken
to private ECR registry endpoint showed up inus-west-2
GetAuthorizationToken
to public ECR registry endpoint showed up inus-east-1
Does this PR introduce a user-facing change?: