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

HannesBBR · 2022-11-30T14:19:28Z

Using the default region causes aws ecr-public get-login-password --region {region} to fail, which then breaks the further steps of the helm deploy process. See the issue #23052 for more details.

According to the ECR docs the region should be either us-east-1 or us-west-2 for public-ecr.

Note that the command also requires additional permissions to succeed, for which I added the Managed Policy AmazonElasticContainerRegistryPublicReadOnly to the handler lambda role.

I've tested the solution in our environment by updating the lambda-handler code and permissions accordingly with the proposed changes here, and now the helm deploy step succeeds, e.g.:

[INFO]	2022-11-30T10:44:02.061Z	6a4afdfa-0505-4d5f-b670-bc5734eee730	b'Login Succeeded\nPulled: public.ecr.aws/kubecost/cost-analyzer:1.97.0\nDigest: sha256:40886873c16ebba885bbb179e38aaea006ed42cd965e6995701dc127e1c80c9e\n'
[INFO]	2022-11-30T10:44:02.061Z	6a4afdfa-0505-4d5f-b670-bc5734eee730	b'Login Succeeded\nPulled: public.ecr.aws/kubecost/cost-analyzer:1.97.0\nDigest: sha256:40886873c16ebba885bbb179e38aaea006ed42cd965e6995701dc127e1c80c9e\n'

I also updated the integration tests to check for the addition of the managed policy, but did not run the actual integration tests yet. I tried running them from gitpod, but since the accounts in our environment have SCPs that only allow deploying to a single region, I wasn't able to complete all integration tests in time before gitpod closes me off. If someone could help out running the integration tests, that would be great 🙏

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-11-30T14:19:37Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

HannesBBR · 2022-11-30T14:30:49Z

As mentioned above, I wasn't able to run the integration tests timely on our own environment using gitpod. If somebody could run them for me, it would be greatly appreciated 🙏

comcalvi · 2022-11-30T16:47:39Z

Could you add a new integration test, ideally for the reproduction code in the first post of the linked issue? I can run the integ test.

comcalvi · 2022-11-30T16:49:06Z

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

-        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'


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

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?

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

HannesBBR · 2022-12-05T09:50:58Z

Could you add a new integration test, ideally for the reproduction code in the first post of the linked issue? I can run the integ test.

I think the integration test defined here should already cover this change https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-eks/test/integ.eks-helm-asset.ts#L54.

Note that the original issue normally would only occur when running this test in a region not us-east-1 (as then the authentication to the public ecr would fail with the original lambda code). Is there a way to force an integration test to run in a specific region?

comcalvi

You're correct that the integ test covers this, but it must have been run in us-east-1 by change for it to work. Please update it to force it to run twice (once in us-east-1 and once in us-west-2) by either making two Apps or two IntegTests.

comcalvi · 2022-12-07T20:15:17Z

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

-        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'


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?

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?

@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']

aws-cdk-automation · 2022-12-22T00:08:24Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2022-12-30T00:08:10Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

aws-cdk-automation · 2023-02-04T00:07:54Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

pahud · 2023-02-06T17:51:42Z

reopening this PR with branch update.

aws-cdk-automation · 2023-02-06T18:10:45Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: ab79ebb
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

pahud · 2023-02-06T18:15:17Z

Hi @HannesBBR

Do you have the capacity to update the failed tests with integ-runner --update-on-failed ? We need pass the build before we are able to advance this PR. Thanks.

aws-cdk-automation · 2023-02-07T00:09:34Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

aws-cdk-automation · 2023-02-07T00:11:36Z

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

HannesBBR added 2 commits November 30, 2022 10:52

Add ecr-public permissions to handler + use us-east-1

51fb897

Add managed policy to snapshot test

b86519e

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Nov 30, 2022

aws-cdk-automation requested a review from a team November 30, 2022 14:19

aws-cdk-automation requested changes Nov 30, 2022

View reviewed changes

HannesBBR marked this pull request as ready for review November 30, 2022 14:26

HannesBBR mentioned this pull request Nov 30, 2022

aws-eks: addHelmChart() fails with public.ecr.aws #23052

Closed

comcalvi requested changes Nov 30, 2022

View reviewed changes

comcalvi requested changes Dec 7, 2022

View reviewed changes

comcalvi self-assigned this Dec 7, 2022

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Dec 30, 2022

aws-cdk-automation closed this Dec 30, 2022

pahud mentioned this pull request Feb 2, 2023

aws-eks: OCI HelmChart from public.ecr.aws only works in us-east-1 #23977

Closed

pahud reopened this Feb 3, 2023

aws-cdk-automation closed this Feb 4, 2023

pahud reopened this Feb 6, 2023

pahud removed the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Feb 6, 2023

Merge branch 'main' into hannesbbr/fix-helm-public-ecr-region

ab79ebb

pahud added the pr-linter/do-not-close The PR linter will not close this PR while this label is present label Feb 6, 2023

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Feb 7, 2023

aws-cdk-automation closed this Feb 7, 2023

pahud mentioned this pull request Feb 10, 2023

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

Closed

zorrofox mentioned this pull request Feb 13, 2023

fix(eks): fix helm deploy login for public ECR repositories #24104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HannesBBR commented Nov 30, 2022 •

edited

Loading

gitpod-io bot commented Nov 30, 2022

aws-cdk-automation left a comment

HannesBBR commented Nov 30, 2022

comcalvi commented Nov 30, 2022

comcalvi Nov 30, 2022

HannesBBR Dec 5, 2022

comcalvi Dec 7, 2022

HannesBBR commented Dec 5, 2022

comcalvi left a comment

comcalvi Dec 7, 2022

andskli Jan 10, 2023

pahud Feb 2, 2023

aws-cdk-automation commented Dec 22, 2022

aws-cdk-automation commented Dec 30, 2022

aws-cdk-automation commented Feb 4, 2023

pahud commented Feb 6, 2023

aws-cdk-automation commented Feb 6, 2023

pahud commented Feb 6, 2023

aws-cdk-automation commented Feb 7, 2023

aws-cdk-automation commented Feb 7, 2023

	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']

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

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

Conversation

HannesBBR commented Nov 30, 2022 • edited Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Nov 30, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

HannesBBR commented Nov 30, 2022

comcalvi commented Nov 30, 2022

comcalvi Nov 30, 2022

Choose a reason for hiding this comment

HannesBBR Dec 5, 2022

Choose a reason for hiding this comment

comcalvi Dec 7, 2022

Choose a reason for hiding this comment

HannesBBR commented Dec 5, 2022

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Dec 7, 2022

Choose a reason for hiding this comment

andskli Jan 10, 2023

Choose a reason for hiding this comment

pahud Feb 2, 2023

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 22, 2022

aws-cdk-automation commented Dec 30, 2022

aws-cdk-automation commented Feb 4, 2023

pahud commented Feb 6, 2023

aws-cdk-automation commented Feb 6, 2023

AWS CodeBuild CI Report

pahud commented Feb 6, 2023

aws-cdk-automation commented Feb 7, 2023

aws-cdk-automation commented Feb 7, 2023

HannesBBR commented Nov 30, 2022 •

edited

Loading