Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(eks): fix helm deploy for public-ecr repositories #23176

Closed
wants to merge 3 commits into from
Closed

fix(eks): fix helm deploy for public-ecr repositories #23176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51fb897
Add ecr-public permissions to handler + use us-east-1
HannesBBR Nov 30, 2022
b86519e
Add managed policy to snapshot test
HannesBBR Nov 30, 2022
ab79ebb
Merge branch 'main' into hannesbbr/fix-helm-public-ecr-region
pahud Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ def get_oci_cmd(repository, version):
f"helm registry login --username AWS --password-stdin {registry}; helm pull {repository} --version {version} --untar"
]
elif registry.startswith(public_ecr):
logger.info("Found AWS public repository, will use default region as deployment")
region = os.environ.get('AWS_REGION', 'us-east-1')
logger.info("Found AWS public repository, will use region 'us-east-1' as deployment")
region = 'us-east-1'
Comment on lines -112 to +113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a change to the unit tests for this; could you add a unit test for this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the previous commits for this lambda handler but didn't see any unit tests (unless I missed those) for these functions, so not sure what to add exactly here. What kind of unit test are you looking for?

Note that the integration test https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-eks/test/integ.eks-helm-asset.ts#L54 should already cover the behaviour change here as well, which I think makes more sense than a unit test in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, this is a custom resource lambda. We have no unit tests for these, so disregard this.

Comment on lines -112 to +113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's two valid regions, us-east-1 and us-west-2. If someone today is using us-west-2, then this change will force them to redeploy to update to us-east-1. We try to avoid forcing new deployments in new releases, so can you update this to check if AWS_REGION is us-west-2, and if it is, continue to use us-west-2?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like region is only used for grabbing temp ECR password and pass it to helm, and not subsequent commands or any SDK calls, so shouldn't impact rendered resources?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comcalvi

According to the Amazon ECR public registries documentation the region should always be us-east-1 for the ecr-public get-login-password operation.

https://docs.aws.amazon.com/AmazonECR/latest/public/public-registries.html#public-registry-auth
When authenticating to a public registry, always authenticate to the us-east-1 Region when using the AWS CLI.

so I think we should always make it us-east-1 here, even deploying in us-east-2.

aws-cdk/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py

Lines 111 to 121 in c0b1019

elif registry.startswith(public_ecr):
logger.info("Found AWS public repository, will use default region as deployment")
region = os.environ.get('AWS_REGION', 'us-east-1')
cmnd = [
f"aws ecr-public get-login-password --region {region} | " \
f"helm registry login --username AWS --password-stdin {public_ecr}; helm pull {repository} --version {version} --untar"
]
else:
logger.error("OCI repository format not recognized, falling back to helm pull")
cmnd = ['helm', 'pull', repository, '--version', version, '--untar']


cmnd = [
f"aws ecr-public get-login-password --region {region} | " \
Expand Down
3 changes: 3 additions & 0 deletions packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {
this.handlerRole.addManagedPolicy(
iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonEC2ContainerRegistryReadOnly'),
);
this.handlerRole.addManagedPolicy(
iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonElasticContainerRegistryPublicReadOnly'),
); // allow handler to authorize to the public-ecr registry

// allow this handler to assume the kubectl role
cluster.kubectlRole.grant(this.handlerRole, 'sts:AssumeRole');
Expand Down
14 changes: 14 additions & 0 deletions packages/@aws-cdk/aws-eks/test/cluster.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2103,6 +2103,13 @@ describe('cluster', () => {
':iam::aws:policy/AmazonEC2ContainerRegistryReadOnly',
]],
},
{
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':iam::aws:policy/AmazonElasticContainerRegistryPublicReadOnly',
]],
},
],
});
});
Expand Down Expand Up @@ -2297,6 +2304,13 @@ describe('cluster', () => {
':iam::aws:policy/AmazonEC2ContainerRegistryReadOnly',
]],
},
{
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':iam::aws:policy/AmazonElasticContainerRegistryPublicReadOnly',
]],
},
],
});
});
Expand Down