kubernetes / cloud-provider-aws

Cloud provider for AWS
https://cloud-provider-aws.sigs.k8s.io/
Apache License 2.0
395 stars 302 forks source link

Handle private ECR image references containing public.ecr.aws #667

Closed cartermckinnon closed 1 year ago

cartermckinnon commented 1 year ago

What type of PR is this?

/kind bug

What this PR does / why we need it:

When support was added for public.ecr.aws images, a simple string.Contains was used to determine whether a requested image reference was a private or public ECR repository. If you use public.ecr.aws anywhere in your image reference, such as:

123456789123.dkr.ecr.us-west-2.amazonaws.com/public.ecr.aws/foo:latest

Then public ECR creds will be retrieved, and image pulls will fail (or you'll fail to call ecr-public::GetAuthorizationToken if your IAM creds don't have permissions for it).

Which issue(s) this PR fixes:

Fixes #651

Special notes for your reviewer:

I did some cleanup while I was at it:

Does this PR introduce a user-facing change?:

Fixes an issue with private ECR image references that contain `public.ecr.aws` being misidentified as public ECR repositories.
k8s-ci-robot commented 1 year ago

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
cartermckinnon commented 1 year ago

/assign mmerkes

k8s-ci-robot commented 1 year ago

@cartermckinnon: GitHub didn't allow me to assign the following users: mmerkes.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide

In response to [this](https://github.com/kubernetes/cloud-provider-aws/pull/667#issuecomment-1725929305): >/assign mmerkes Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
cartermckinnon commented 1 year ago

@mmerkes please review?

cartermckinnon commented 1 year ago

/assign tzneal

cartermckinnon commented 1 year ago

I sanity checked this with the log output:

> echo '{
  "kind":"CredentialProviderRequest",
  "apiVersion":"credentialprovider.kubelet.k8s.io/v1",
  "image":"123456789123.dkr.ecr.us-west-2.amazonaws.com/public.ecr.aws/foo"
}' | ./ecr-credential-provider

I0919 09:33:41.036509   63515 main.go:125] Getting creds for private image 123456789123.dkr.ecr.us-west-2.amazonaws.com/public.ecr.aws/foo
{"kind":"CredentialProviderResponse","apiVersion":"credentialprovider.kubelet.k8s.io/v1","cacheKeyType":"Registry","cacheDuration":"6h0m0s","auth":{"123456789123.dkr.ecr.us-west-2.amazonaws.com":{"username":"AWS","password":"<secret>"}}}

> echo '{
  "kind":"CredentialProviderRequest",
  "apiVersion":"credentialprovider.kubelet.k8s.io/v1",
  "image":"public.ecr.aws/foo"
}' | ./ecr-credential-provider

I0919 09:34:36.638610   63660 main.go:94] Getting creds for public registry
{"kind":"CredentialProviderResponse","apiVersion":"credentialprovider.kubelet.k8s.io/v1","cacheKeyType":"Registry","cacheDuration":"6h0m0s","auth":{"public.ecr.aws":{"username":"AWS","password":"<secret>"}}}
k8s-ci-robot commented 1 year ago

@mmerkes: changing LGTM is restricted to collaborators

In response to [this](https://github.com/kubernetes/cloud-provider-aws/pull/667#pullrequestreview-1634004724): >LGTM Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
cartermckinnon commented 1 year ago

/assign @nckturner

can you give this a look Nick?

cartermckinnon commented 1 year ago

@dims PTAL?

dims commented 1 year ago

/approve /lgtm

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, mmerkes, tzneal

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: - ~~[OWNERS](https://github.com/kubernetes/cloud-provider-aws/blob/master/OWNERS)~~ [dims] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
matty-rose commented 1 year ago

hey @cartermckinnon we ran into this issue quite recently, super nice to see the fix merged in already 🎉 - do you have an idea on the cadence/ETA that the ecr-credential-provider binary with this fix will be incorporated into the EKS AMIs?