kubelet has broken ECR auth configuration

[aws]

kubelet has broken ECR auth configuration

[TerryHowe]

Sounds like you've done a lot of research on this. Do you want to do a PR? I think taking any action 1.17 is a bit late, but it isn't too late to get any changes in 1.19.

eks-distro/projects/kubernetes/kubernetes/1-19/patches/0006-EKS-PATCH-using-regional-sts-endpoint-for-assume-ecr.patch

Lines 74 to 100 in 91374b7

	}
	+
	+ // TODO: Remove this once aws sdk is updated to latest version.
	+ var provider credentials.Provider
	+ ecrPullRoleArn := os.Getenv("ECR_PULL_ROLE_ARN")
	+ assumeRoleRegion := os.Getenv("AWS_DEFAULT_REGION")
	+ if ecrPullRoleArn != "" && assumeRoleRegion != "" {
	+ stsEndpoint, err := k8saws.ConstructStsEndpoint(ecrPullRoleArn, assumeRoleRegion)
	+ if err != nil {
	+ return nil, err
	+ }
	+ klog.Infof("Using AWS assumed role, %v:%v:%v", ecrPullRoleArn, assumeRoleRegion, stsEndpoint)
	+ provider = &stscreds.AssumeRoleProvider{
	+ Client: sts.New(sess, aws.NewConfig().WithRegion(assumeRoleRegion).WithEndpoint(stsEndpoint)),
	+ RoleARN: ecrPullRoleArn,
	+ }
	+ } else {
	+ provider = &ec2rolecreds.EC2RoleProvider{
	+ Client: ec2metadata.New(sess),
	+ }
	+ }
	+
	+ creds := credentials.NewChainCredentials(
	+ []credentials.Provider{
	+ &credentials.EnvProvider{},
	+ provider,
	+ &credentials.SharedCredentialsProvider{},

I can't say what would make all parties happy with a change there though, but let me know your thoughts.

[nairb774]

What isn't clear, and mainly why I didn't propose a PR was there doesn't seem to be any documentation as to why this feature exists, or why it might be needed. Lacking that context, and documentation would it be possible to just remove the patch entirely? With https://github.com/aws/eks-distro/blob/45bbfb32b8b52c6acefcd8b431227aa0d2bfa10e/projects/kubernetes/kubernetes/1-19/patches/0008-EKS-PATCH-Update-aws-sdk-go-to-v1.34.24.patch it looks like the AWS SDK supports the feature directly now: https://github.com/aws/aws-sdk-go/blob/v1.34.24/aws/session/shared_config.go#L285-L292

If the sole purpose was to support regional STS, it looks like this is now addressed. That said, getting any more detail on the reason for the patch would go a long way in knowing the steps forward. The two patches adding the code are the only two files I can find, which makes me suspicious that there aren't any tests covering this feature. If they exist, and anyone knows where they might be, that would be greatly appreciated.

[TerryHowe]

Good question, I'm not sure of the answer, but I'll see if anyone else does.

[nairb774]

I posted #132 to just remove the patch in the off chance that path is possible. If we are able to locate and document a need for the existing code, I can change the PR, or make a new one, to only change the credential loading behavior in the presence of the env vars.

[SaranBalaji90]

@nairb774 you were right that this is not required anymore given that aws-sdk-go dependency was updated (kubernetes/kubernetes#87253) from 1.18 onwards and was only added to support pulling images using different role for ECR to support other computes like Fargate and also to support regional STS regional endpoint for image pulling which are now addressed. Having said that, we will remove this in the next release. Sorry for the inconvenience this has caused.

[nairb774]

Thanks for the background and great to know the history behind the change.

[TerryHowe]

Sorry, after a lot of arm twisting I still couldn't get this in v1.19, but it will definitely be in v1.20

[nairb774]

It happens. On the other hand, excited to see 1.19 coming.

[nairb774]

To follow up, this was fixed in 1.20? I haven't yet been able to test, but curious for when we get there.

[TerryHowe]

This was fixed for 1.20