Repository: kubernetes/cloud-provider-aws (go)

Issue:
Private ECR repository contains "public.ecr.aws" breaks the GetCredentials
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!-->


**What happened**:

In our company, we intend to do image copy from public repository to our private ECR repository, just because we don't want to encounter network issue that may cause the image pulling to be slow/failed.

Currently our naming convention of the repository looks like: `$ecr_repository_url$public_repository_name:$image_tag.`

A concrete example would be, for image `public.ecr.aws/aws-observability/aws-for-fluent-bit:2.31.12`, we'll do a replication as `112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn/public.ecr.aws/aws-observability/aws-for-fluent-bit:2.31.12`

So as for kubelet's image pulling, we're facing `Failed getting credential from external registry credential provider: error execing credential provider plugin ecr-credential-provider for image 112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn/public.ecr.aws/aws-observability/aws-for-fluent-bit: exit status 1`.

The above error results to image pulling failed with 401 Unauthorized issue.

After digging the source code, I've noticed that the `ecrPlugin.GetCredentials` uses a keyword `public.ecr.aws` to determine if the given image is a public repository or not.

Thus in our case, a credential request was falsely made to publilc registry instead of private registry.

<img width="650" alt="Screen Shot 2023-09-04 at 20 19 27" src="https://github.com/kubernetes/cloud-provider-aws/assets/6973092/8d1a4c8e-202f-494f-8904-1608b1f0b06a">

**What you expected to happen**:

Even with the keyword `public.ecr.aws` present in the image url, the `ecr-credential-provider` shall still be able to determin if the image is in private registry or not.

**How to reproduce it (as minimally and precisely as possible)**:

Create a private repository and image with keyword `public.ecr.aws`, for example, `112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn/public.ecr.aws/aws-observability/aws-for-fluent-bit:2.31.12`.

Then invoke directly the `ecr-credential-provider` with:

```
echo '{"kind":"CredentialProviderRequest","apiVersion":"credentialprovider.kubelet.k8s.io/v1","image":"112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn/public.ecr.aws/aws-observability/aws-for-fluent-bit:2.31.12"}' | /etc/eks/image-credential-provider/ecr-credential-provider
I0904 12:24:55.935639   30684 main.go:95] Getting creds for public registry
E0904 12:24:57.143893   30684 main.go:261] Error running credential provider plugin: UnrecognizedClientException: The security token included in the request is invalid
	status code: 400, request id: 05cad298-32ce-4775-85b2-d23ce746f26e
```

For comparaision, use another repository and image, `112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn/aws-observability/aws-for-fluent-bit:2.31.12`:

```
echo '{"kind":"CredentialProviderRequest","apiVersion":"credentialprovider.kubelet.k8s.io/v1","image":"112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn/aws-observability/aws-for-fluent-bit:2.31.12"}' | /etc/eks/image-credential-provider/ecr-credential-provider
I0904 12:26:23.827086   31239 main.go:126] Getting creds for private registry 112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn/aws-observability/aws-for-fluent-bit:2.31.12
{"kind":"CredentialProviderResponse","apiVersion":"credentialprovider.kubelet.k8s.io/v1","cacheKeyType":"Registry","cacheDuration":"6h0m0s","auth":{"112233445566.dkr.ecr.cn-northwest-1.amazonaws.com.cn":{"username":"AWS","password":"REDACTED"}}}
```

**Anything else we need to know?**:

Nein.

**Environment**:
- Kubernetes version (use `kubectl version`): `Server Version: version.Info{Major:"1", Minor:"27+", GitVersion:"v1.27.4-eks-2d98532", GitCommit:"3d90c097c72493c2f1a9dd641e4a22d24d15be68", GitTreeState:"clean", BuildDate:"2023-07-28T16:51:44Z", GoVersion:"go1.20.6", Compiler:"gc", Platform:"linux/amd64"}`
- Cloud provider or hardware configuration:
- OS (e.g. from /etc/os-release): 
  ```
  NAME="Amazon Linux"
  VERSION="2"
  ID="amzn"
  ID_LIKE="centos rhel fedora"
  VERSION_ID="2"
  PRETTY_NAME="Amazon Linux 2"
  ANSI_COLOR="0;33"
  CPE_NAME="cpe:2.3:o:amazon:amazon_linux:2"
  HOME_URL="https://amazonlinux.com/"
  ```
- Kernel (e.g. `uname -a`): `Linux ip-172-24-155-214.cn-northwest-1.compute.internal 5.10.186-179.751.amzn2.x86_64 #1 SMP Tue Aug 1 20:51:38 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux`
- Install tools:
- Others:

<!-- DO NOT EDIT BELOW THIS LINE -->
/kind bug

Pull request:
Handle private ECR image references containing public.ecr.aws
**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:
- I removed the `RegistryIds` parameter to `GetAuthorizationTokenInput`. This field is deprecated and has no effect on the token returned.

**Does this PR introduce a user-facing change?**:

```release-note
Fixes an issue with private ECR image references that contain `public.ecr.aws` being misidentified as public ECR repositories.
```


Patch:
diff --git a/cmd/ecr-credential-provider/main.go b/cmd/ecr-credential-provider/main.go
index 94f7509..eb2bcde 100644
--- a/cmd/ecr-credential-provider/main.go
+++ b/cmd/ecr-credential-provider/main.go
@@ -40,9 +40,9 @@ import (
 )
 
 const ecrPublicRegion string = "us-east-1"
-const ecrPublicURL string = "public.ecr.aws"
+const ecrPublicHost string = "public.ecr.aws"
 
-var ecrPattern = regexp.MustCompile(`^(\d{12})\.dkr\.ecr(\-fips)?\.([a-zA-Z0-9][a-zA-Z0-9-_]*)\.(amazonaws\.com(\.cn)?|sc2s\.sgov\.gov|c2s\.ic\.gov)$`)
+var ecrPrivateHostPattern = regexp.MustCompile(`^(\d{12})\.dkr\.ecr(\-fips)?\.([a-zA-Z0-9][a-zA-Z0-9-_]*)\.(amazonaws\.com(\.cn)?|sc2s\.sgov\.gov|c2s\.ic\.gov)$`)
 
 // ECR abstracts the calls we make to aws-sdk for testing purposes
 type ECR interface {
@@ -86,7 +86,6 @@ func publicECRProvider() (*ecrpublic.ECRPublic, error) {
 }
 
 type credsData struct {
-	registry  string
 	authToken *string
 	expiresAt *time.Time
 }
@@ -116,43 +115,34 @@ func (e *ecrPlugin) getPublicCredsData() (*credsData, error) {
 	}
 
 	return &credsData{
-		registry:  ecrPublicURL,
 		authToken: output.AuthorizationData.AuthorizationToken,
 		expiresAt: output.AuthorizationData.ExpiresAt,
 	}, nil
 }
 
-func (e *ecrPlugin) getPrivateCredsData(image string) (*credsData, error) {
-	klog.Infof("Getting creds for private registry %s", image)
-	registryID, region, registry, err := parseRepoURL(image)
+func (e *ecrPlugin) getPrivateCredsData(imageHost string, image string) (*credsData, error) {
+	klog.Infof("Getting creds for private image %s", image)
+	region, err := parseRegionFromECRPrivateHost(imageHost)
 	if err != nil {
 		return nil, err
 	}
-
 	if e.ecr == nil {
 		e.ecr, err = defaultECRProvider(region)
 		if err != nil {
 			return nil, err
 		}
 	}
-
-	output, err := e.ecr.GetAuthorizationToken(&ecr.GetAuthorizationTokenInput{
-		RegistryIds: []*string{aws.String(registryID)},
-	})
+	output, err := e.ecr.GetAuthorizationToken(&ecr.GetAuthorizationTokenInput{})
 	if err != nil {
 		return nil, err
 	}
-
 	if output == nil {
 		return nil, errors.New("response output from ECR was nil")
 	}
-
 	if len(output.AuthorizationData) == 0 {
 		return nil, errors.New("authorization data was empty")
 	}
-
 	return &credsData{
-		registry:  registry,
 		authToken: output.AuthorizationData[0].AuthorizationToken,
 		expiresAt: output.AuthorizationData[0].ExpiresAt,
 	}, nil
@@ -162,10 +152,15 @@ func (e *ecrPlugin) GetCredentials(ctx context.Context, image string, args []str
 	var creds *credsData
 	var err error
 
-	if strings.Contains(image, ecrPublicURL) {
+	imageHost, err := parseHostFromImageReference(image)
+	if err != nil {
+		return nil, err
+	}
+
+	if imageHost == ecrPublicHost {
 		creds, err = e.getPublicCredsData()
 	} else {
-		creds, err = e.getPrivateCredsData(image)
+		creds, err = e.getPrivateCredsData(imageHost, image)
 	}
 
 	if err != nil {
@@ -192,7 +187,7 @@ func (e *ecrPlugin) GetCredentials(ctx context.Context, image string, args []str
 		CacheKeyType:  v1.RegistryPluginCacheKeyType,
 		CacheDuration: cacheDuration,
 		Auth: map[string]v1.AuthConfig{
-			creds.registry: {
+			imageHost: {
 				Username: parts[0],
 				Password: parts[1],
 			},
@@ -219,24 +214,25 @@ func getCacheDuration(expiresAt *time.Time) *metav1.Duration {
 	return cacheDuration
 }
 
-// parseRepoURL parses and splits the registry URL
-// returns (registryID, region, registry).
-// <registryID>.dkr.ecr(-fips).<region>.amazonaws.com(.cn)
-func parseRepoURL(image string) (string, string, string, error) {
-	if !strings.Contains(image, "https://") {
+// parseHostFromImageReference parses the hostname from an image reference
+func parseHostFromImageReference(image string) (string, error) {
+	// a URL needs a scheme to be parsed correctly
+	if !strings.Contains(image, "://") {
 		image = "https://" + image
 	}
 	parsed, err := url.Parse(image)
 	if err != nil {
-		return "", "", "", fmt.Errorf("error parsing image %s: %v", image, err)
+		return "", fmt.Errorf("error parsing image reference %s: %v", image, err)
 	}
+	return parsed.Hostname(), nil
+}
 
-	splitURL := ecrPattern.FindStringSubmatch(parsed.Hostname())
-	if len(splitURL) < 4 {
-		return "", "", "", fmt.Errorf("%s is not a valid ECR repository URL", parsed.Hostname())
+func parseRegionFromECRPrivateHost(host string) (string, error) {
+	splitHost := ecrPrivateHostPattern.FindStringSubmatch(host)
+	if len(splitHost) != 6 {
+		return "", fmt.Errorf("invalid private ECR host: %s", host)
 	}
-
-	return splitURL[1], splitURL[3], parsed.Hostname(), nil
+	return splitHost[3], nil
 }
 
 func main() {


Test patch:
diff --git a/cmd/ecr-credential-provider/main_test.go b/cmd/ecr-credential-provider/main_test.go
index b52a382..f403388 100644
--- a/cmd/ecr-credential-provider/main_test.go
+++ b/cmd/ecr-credential-provider/main_test.go
@@ -30,7 +30,7 @@ import (
 	"github.com/golang/mock/gomock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/cloud-provider-aws/pkg/providers/v2/mocks"
-	"k8s.io/kubelet/pkg/apis/credentialprovider/v1"
+	v1 "k8s.io/kubelet/pkg/apis/credentialprovider/v1"
 )
 
 func generatePrivateGetAuthorizationTokenOutput(user string, password string, proxy string, expiration *time.Time) *ecr.GetAuthorizationTokenOutput {
@@ -80,6 +80,12 @@ func Test_GetCredentials_Private(t *testing.T) {
 			getAuthorizationTokenOutput: generatePrivateGetAuthorizationTokenOutput("user", "pass", "", nil),
 			response:                    generateResponse("123456789123.dkr.ecr.us-west-2.amazonaws.com", "user", "pass"),
 		},
+		{
+			name:                        "image reference containing public ECR host",
+			image:                       "123456789123.dkr.ecr.us-west-2.amazonaws.com/public.ecr.aws/foo:latest",
+			getAuthorizationTokenOutput: generatePrivateGetAuthorizationTokenOutput("user", "pass", "", nil),
+			response:                    generateResponse("123456789123.dkr.ecr.us-west-2.amazonaws.com", "user", "pass"),
+		},
 		{
 			name:                        "empty authorization data",
 			image:                       "123456789123.dkr.ecr.us-west-2.amazonaws.com",
@@ -251,59 +257,85 @@ func Test_GetCredentials_Public(t *testing.T) {
 	}
 }
 
-func Test_ParseURL(t *testing.T) {
+func Test_parseHostFromImageReference(t *testing.T) {
 	testcases := []struct {
-		name       string
-		image      string
-		registryID string
-		region     string
-		registry   string
-		err        error
+		name  string
+		image string
+		host  string
+		err   error
 	}{
 		{
-			name:       "success",
-			image:      "123456789123.dkr.ecr.us-west-2.amazonaws.com",
-			registryID: "123456789123",
-			region:     "us-west-2",
-			registry:   "123456789123.dkr.ecr.us-west-2.amazonaws.com",
-			err:        nil,
+			name:  "success",
+			image: "123456789123.dkr.ecr.us-west-2.amazonaws.com/foo/bar:1.0",
+			host:  "123456789123.dkr.ecr.us-west-2.amazonaws.com",
+			err:   nil,
 		},
 		{
-			name:       "invalid registry",
-			image:      "foobar",
-			registryID: "",
-			region:     "",
-			registry:   "",
-			err:        errors.New("foobar is not a valid ECR repository URL"),
+			name:  "existing scheme",
+			image: "http://foobar",
+			host:  "foobar",
+			err:   nil,
 		},
 		{
-			name:       "invalid URL",
-			image:      "foobar  ",
-			registryID: "",
-			region:     "",
-			registry:   "",
-			err:        errors.New("error parsing image https://foobar  : parse \"https://foobar  \": invalid character \" \" in host name"),
+			name:  "invalid URL",
+			image: "foobar  ",
+			host:  "",
+			err:   errors.New("error parsing image reference https://foobar  : parse \"https://foobar  \": invalid character \" \" in host name"),
 		},
 	}
 
 	for _, testcase := range testcases {
 		t.Run(testcase.name, func(t *testing.T) {
-			registryID, region, registry, err := parseRepoURL(testcase.image)
+			host, err := parseHostFromImageReference(testcase.image)
 
 			if testcase.err != nil && (testcase.err.Error() != err.Error()) {
 				t.Fatalf("expected error %s, got %s", testcase.err, err)
 			}
 
-			if registryID != testcase.registryID {
-				t.Fatalf("registryID mismatch. Expected %s, got %s", testcase.registryID, registryID)
+			if host != testcase.host {
+				t.Fatalf("registry mismatch. Expected %s, got %s", testcase.host, host)
 			}
+		})
+	}
+}
 
-			if region != testcase.region {
-				t.Fatalf("region mismatch. Expected %s, got %s", testcase.region, region)
+func Test_parseRegionFromECRPrivateHost(t *testing.T) {
+	testcases := []struct {
+		name   string
+		host   string
+		region string
+		err    error
+	}{
+		{
+			name:   "success",
+			host:   "123456789123.dkr.ecr.us-west-2.amazonaws.com",
+			region: "us-west-2",
+			err:    nil,
+		},
+		{
+			name:   "invalid registry",
+			host:   "foobar",
+			region: "",
+			err:    errors.New("invalid private ECR host: foobar"),
+		},
+		{
+			name:   "invalid host",
+			host:   "foobar ",
+			region: "",
+			err:    errors.New("invalid private ECR host: foobar "),
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			region, err := parseRegionFromECRPrivateHost(testcase.host)
+
+			if testcase.err != nil && (testcase.err.Error() != err.Error()) {
+				t.Fatalf("expected error %s, got %s", testcase.err, err)
 			}
 
-			if registry != testcase.registry {
-				t.Fatalf("registry mismatch. Expected %s, got %s", testcase.registry, registry)
+			if region != testcase.region {
+				t.Fatalf("region mismatch. Expected %s, got %s", testcase.region, region)
 			}
 		})
 	}
@@ -345,7 +377,7 @@ func TestRegistryPatternMatch(t *testing.T) {
 		{"123456789012.lala-land-1.amazonaws.com", false},
 	}
 	for _, g := range grid {
-		actual := ecrPattern.MatchString(g.Registry)
+		actual := ecrPrivateHostPattern.MatchString(g.Registry)
 		if actual != g.Expected {
 			t.Errorf("unexpected pattern match value, want %v for %s", g.Expected, g.Registry)
 		}