fix(eks): policy does not exist or is not attachable in China and GovCloud regions #25170

pahud · 2023-04-18T04:31:14Z

Reopening this PR and closing the previous one.

As ECR Public is not available in China regions and GovCloud, AmazonElasticContainerRegistryPublicReadOnly IAM managed policy would not be available in those affected regions and should not be attached to the role. This PR implements a RegionInfo method to determine if ECR public is available in the deploying region and conditionally attach AmazonElasticContainerRegistryPublicReadOnly to the role. If region can't be determined in synth time, we assume ECR Public is not available to avoid potential failure.

Closes #24743 #24808 #25178

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

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

iliapolo · 2023-04-19T09:37:10Z

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

+                f"helm registry login --username AWS --password-stdin {public_registry['registry']}; helm pull {repository} --version {version} --untar"
+                ]
+        else:
+            cmnd = [f"helm pull {repository} --version {version} --untar"] 


So running helm registry login isn't necessary then? Do you know why we run it in the normal flow? is it because of service quotas?

iliapolo · 2023-04-19T09:52:57Z

packages/aws-cdk-lib/aws-eks/README.md

@@ -1494,6 +1495,11 @@ const cluster = new eks.Cluster(this, 'Cluster', {
 });
 ```

+## Working with ECR Public


Feels like this should be in the Helm Charts section.

iliapolo · 2023-04-19T09:55:47Z

packages/aws-cdk-lib/aws-eks/lib/private/service-available.ts

+import { Token } from '../../../core';
+import { RegionInfo } from '../../../region-info';
+
+export function EcrPublicAvailable(region: any): boolean {


Suggested change

export function EcrPublicAvailable(region: any): boolean {

export function ecrPublicAvailable(region: any): boolean {

Also move this function to the kubectl-provider.ts file.

iliapolo · 2023-04-19T09:58:26Z

packages/aws-cdk-lib/aws-eks/test/service-available.test.ts

@@ -0,0 +1,17 @@
+import { EcrPublicAvailable } from '../lib/private/service-available';
+
+describe('EcrPublicAvailable', () => {


This is why you had to export the EcrPublicAvailable function, even though it is clearly private. Instead unit testing the function, just test that the result of the function is what you expect. That is, instantiate a cluster in a specific region, and assert that the environment of the handler is correct, and that the policy doe / doesn't exist.

iliapolo · 2023-04-19T10:02:48Z

packages/aws-cdk-lib/region-info/build-tools/generate-static-data.ts

@@ -56,6 +56,7 @@ export async function main(): Promise<void> {
    registerFact(region, 'DOMAIN_SUFFIX', domainSuffix);

    registerFact(region, 'CDK_METADATA_RESOURCE_AVAILABLE', AWS_CDK_METADATA.has(region) ? 'YES' : 'NO');
+    registerFact(region, 'ECR_PUBLIC_AVAILABLE', partition === 'aws' ? 'YES' : 'NO');


Did you consider using the AWS::Partition pseudo param in conjunction with Fn::If to make this more environment agnostic? its a bummer the user must specify a region in the stack in order to get this functionality.

iliapolo · 2023-04-19T10:06:21Z

packages/aws-cdk-lib/region-info/lib/region-info.ts

+   * Whether Amazon ECR Public is available in this region or not.
+   */
+  public get ecrPublicAvailable(): boolean {
+    return Fact.find(this.name, FactName.ECR_PUBLIC_AVAILABLE) === 'YES';


So this would mean if a user doesn't specify a region in the stack, which is the common case, we now change the behavior and remove the policy and remove the ecr authorization - this might have a some implications no? We should default to the common case, and make the user "work" a little harder for the uncommon one.

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

aws-cdk-automation · 2023-04-19T16:55:56Z

The pull request linter fails with the following errors:

❌ Fixes must contain a change to a test file.
❌ 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.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

aws-cdk-automation · 2023-04-19T17:17:04Z

AWS CodeBuild CI Report

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

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

…Cloud regions (#25215) Reopening this PR because #25170 was closed by accident. As ECR Public is not available in China regions and GovCloud, `AmazonElasticContainerRegistryPublicReadOnly` IAM managed policy would not be available in those affected regions and should not be attached to the role. This PR implements a CfnCondition to determine if ECR public is available based on `Aws.Partition` of the deploying region and conditionally attach `AmazonElasticContainerRegistryPublicReadOnly` to the kubectl-provider handler role. This PR has been tested in the following regions: - [x] *cn-north-1 - [x] *cn-northwest-1 - [x] us-east-1 * I can confirm the role is created correctly in cn regions but due to - #24358 - #24696 The cluster and nodegroup are still failing to create in CN. Closes #24743 #24808 #25178

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Apr 18, 2023

aws-cdk-automation requested a review from a team April 18, 2023 04:31

mergify bot added the contribution/core This is a PR that came from AWS. label Apr 18, 2023

pahud mentioned this pull request Apr 18, 2023

fix(eks): policy does not exist or is not attachable in China regions #24759

Closed

aws-cdk-automation previously requested changes Apr 18, 2023

View reviewed changes

pahud marked this pull request as ready for review April 18, 2023 17:18

iliapolo reviewed Apr 19, 2023

View reviewed changes

pahud closed this Apr 19, 2023

pahud force-pushed the fix-eks-policy-not-exist branch from 27753ad to d8267b7 Compare April 19, 2023 16:54

aws-cdk-automation requested changes Apr 19, 2023

View reviewed changes

This was referenced Apr 20, 2023

fix(eks): policy does not exist or is not attachable in China and GovCloud regions #25215

Merged

aws-eks: KubectlProvider fails to create/update in govcloud #24808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eks): policy does not exist or is not attachable in China and GovCloud regions #25170

fix(eks): policy does not exist or is not attachable in China and GovCloud regions #25170

pahud commented Apr 18, 2023 •

edited

Loading

aws-cdk-automation left a comment

iliapolo Apr 19, 2023

iliapolo Apr 19, 2023

iliapolo Apr 19, 2023

iliapolo Apr 19, 2023

iliapolo Apr 19, 2023

iliapolo Apr 19, 2023

iliapolo Apr 19, 2023

aws-cdk-automation left a comment

aws-cdk-automation commented Apr 19, 2023

aws-cdk-automation commented Apr 19, 2023

	export function EcrPublicAvailable(region: any): boolean {
	export function ecrPublicAvailable(region: any): boolean {

		@@ -0,0 +1,17 @@
		import { EcrPublicAvailable } from '../lib/private/service-available';

		describe('EcrPublicAvailable', () => {

fix(eks): policy does not exist or is not attachable in China and GovCloud regions #25170

fix(eks): policy does not exist or is not attachable in China and GovCloud regions #25170

Conversation

pahud commented Apr 18, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

iliapolo Apr 19, 2023

Choose a reason for hiding this comment

iliapolo Apr 19, 2023

Choose a reason for hiding this comment

iliapolo Apr 19, 2023

Choose a reason for hiding this comment

iliapolo Apr 19, 2023

Choose a reason for hiding this comment

iliapolo Apr 19, 2023

Choose a reason for hiding this comment

iliapolo Apr 19, 2023

Choose a reason for hiding this comment

iliapolo Apr 19, 2023

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 19, 2023

aws-cdk-automation commented Apr 19, 2023

AWS CodeBuild CI Report

pahud commented Apr 18, 2023 •

edited

Loading