From bd6da7ae80d608a5941c37635108181dc2017925 Mon Sep 17 00:00:00 2001 From: Harshad Bhatia Date: Thu, 20 Jan 2022 12:37:28 +1100 Subject: [PATCH 1/9] feat(eks): Allow helm pull from OCI repositories --- .../lib/kubectl-handler/helm/__init__.py | 64 ++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py b/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py index f83fc204b73e3..e7fe6ffc5dae8 100644 --- a/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py +++ b/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py @@ -1,8 +1,10 @@ import json import logging import os +import re import subprocess import shutil +import tempfile import zipfile from urllib.parse import urlparse, unquote @@ -78,6 +80,12 @@ def helm_handler(event, context): # future work: support versions from s3 assets chart = get_chart_asset_from_url(chart_asset_url) + if repository.startswith('oci://'): + assert(repository is not None) + tmpdir = tempfile.TemporaryDirectory() + chart_dir = get_chart_from_oci(tmpdir.name, release, repository, version) + chart = chart_dir + helm('upgrade', release, chart, repository, values_file, namespace, version, wait, timeout, create_namespace) elif request_type == "Delete": try: @@ -85,6 +93,59 @@ def helm_handler(event, context): except Exception as e: logger.info("delete error: %s" % e) + +def get_oci_cmd(repository, version): + + cmnd = [] + pattern = '\d+.dkr.ecr.[a-z]+-[a-z]+-\d.amazonaws.com' + + registry = repository.rsplit('/', 1)[0].replace('oci://', '') + + if re.fullmatch(pattern, registry) is not None: + region = registry.replace('.amazonaws.com', '').split('.')[-1] + cmnd = [ + "HELM_EXPERIMENTAL_OCI=1;" \ + f"aws ecr get-login-password --region {region} | " \ + f"helm registry login --username AWS --password-stdin {registry}; helm pull {repository} --version {version} --untar" + ] + else: + logger.info("Non AWS OCI repository found") + cmnd = ['HELM_EXPERIMENTAL_OCI=1', 'helm', 'pull', repository, '--version', version, '--untar'] + + return cmnd + + +def get_chart_from_oci(tmpdir, release, repository = None, version = None): + + cmnd = get_oci_cmd(repository, version) + + maxAttempts = 3 + retry = maxAttempts + while retry > 0: + try: + logger.info(cmnd) + env = get_env_with_oci_flag() + output = subprocess.check_output(cmnd, stderr=subprocess.STDOUT, cwd=tmpdir, env=env, shell=True) + logger.info(output) + + return os.path.join(tmpdir, release) + except subprocess.CalledProcessError as exc: + output = exc.output + if b'Broken pipe' in output: + retry = retry - 1 + logger.info("Broken pipe, retries left: %s" % retry) + else: + raise Exception(output) + raise Exception(f'Operation failed after {maxAttempts} attempts: {output}') + + +def get_env_with_oci_flag(): + env = os.environ.copy() + env['HELM_EXPERIMENTAL_OCI'] = '1' + + return env + + def helm(verb, release, chart = None, repo = None, file = None, namespace = None, version = None, wait = False, timeout = None, create_namespace = None): import subprocess @@ -113,7 +174,8 @@ def helm(verb, release, chart = None, repo = None, file = None, namespace = None retry = maxAttempts while retry > 0: try: - output = subprocess.check_output(cmnd, stderr=subprocess.STDOUT, cwd=outdir) + env = get_env_with_oci_flag() + output = subprocess.check_output(cmnd, stderr=subprocess.STDOUT, cwd=outdir, env=env) logger.info(output) return except subprocess.CalledProcessError as exc: From 378854666a3797cf1e1a4b864efd6de10600d7bd Mon Sep 17 00:00:00 2001 From: Harshad Bhatia Date: Fri, 21 Jan 2022 16:27:12 +1100 Subject: [PATCH 2/9] Added README and removed other variable --- packages/@aws-cdk/aws-eks/README.md | 18 ++++++++++++++++++ .../lib/kubectl-handler/helm/__init__.py | 1 - 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-eks/README.md b/packages/@aws-cdk/aws-eks/README.md index 1403e56d817fe..f8e348ab3dd12 100644 --- a/packages/@aws-cdk/aws-eks/README.md +++ b/packages/@aws-cdk/aws-eks/README.md @@ -1144,6 +1144,24 @@ cluster.addHelmChart('test-chart', { }); ``` +### OCI Charts + +OCI charts are also supported. Before executing ensure the handler lambda has the required ECR IAM permissions. +Also replace the `${VARS}` with appropriate values. + +```ts +declare const cluster: eks.Cluster; +// option 1: use a construct +new eks.HelmChart(this, 'MyOCIChart', { + cluster, + chart: 'some-chart', + repository: 'oci://${ACCOUNT_ID}.dkr.ecr.${ACCOUNT_REGION}.amazonaws.com/${REPO_NAME}', + namespace: 'oci', + version: '0.0.1' +}); + +``` + Helm charts are implemented as CloudFormation resources in CDK. This means that if the chart is deleted from your code (or the stack is deleted), the next `cdk deploy` will issue a `helm uninstall` command and the diff --git a/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py b/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py index e7fe6ffc5dae8..7d51e26d7d09b 100644 --- a/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py +++ b/packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py @@ -104,7 +104,6 @@ def get_oci_cmd(repository, version): if re.fullmatch(pattern, registry) is not None: region = registry.replace('.amazonaws.com', '').split('.')[-1] cmnd = [ - "HELM_EXPERIMENTAL_OCI=1;" \ f"aws ecr get-login-password --region {region} | " \ f"helm registry login --username AWS --password-stdin {registry}; helm pull {repository} --version {version} --untar" ] From 1eff1d59839adeafdcc49117ecc5d2a95a732366 Mon Sep 17 00:00:00 2001 From: Harshad Date: Tue, 25 Jan 2022 10:04:44 +1100 Subject: [PATCH 3/9] Update README.md --- packages/@aws-cdk/aws-eks/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-eks/README.md b/packages/@aws-cdk/aws-eks/README.md index f8e348ab3dd12..71f8fcad24127 100644 --- a/packages/@aws-cdk/aws-eks/README.md +++ b/packages/@aws-cdk/aws-eks/README.md @@ -1146,7 +1146,7 @@ cluster.addHelmChart('test-chart', { ### OCI Charts -OCI charts are also supported. Before executing ensure the handler lambda has the required ECR IAM permissions. +OCI charts are also supported. Also replace the `${VARS}` with appropriate values. ```ts From 493956aa058ad1e189274eb37a39b5db77c16689 Mon Sep 17 00:00:00 2001 From: Harshad Bhatia Date: Mon, 31 Jan 2022 15:18:09 +1100 Subject: [PATCH 4/9] Added ecr permissions to handler role --- packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts b/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts index 161ef77cfe3f3..1f0ac27be1deb 100644 --- a/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts +++ b/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts @@ -168,6 +168,12 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider { resources: [cluster.clusterArn], })); + // For OCI helm chart authorization. + this.handlerRole.addToPrincipalPolicy(new iam.PolicyStatement({ + actions: ['ecr:GetAuthorizationToken'], + resources: ["*"], + })); + // allow this handler to assume the kubectl role cluster.kubectlRole.grant(this.handlerRole, 'sts:AssumeRole'); From 9c2efa246e40241b1a7fb59d9b4e6b7c29e0c3e3 Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Mon, 31 Jan 2022 10:02:03 +0000 Subject: [PATCH 5/9] Update kubectl-provider.ts --- packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts b/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts index 1f0ac27be1deb..49298ea54ecd7 100644 --- a/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts +++ b/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts @@ -171,7 +171,7 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider { // For OCI helm chart authorization. this.handlerRole.addToPrincipalPolicy(new iam.PolicyStatement({ actions: ['ecr:GetAuthorizationToken'], - resources: ["*"], + resources: ['*'], })); // allow this handler to assume the kubectl role From edce07f7c8d686089e21d331527209f54d9398b0 Mon Sep 17 00:00:00 2001 From: Zoey Bendef Date: Mon, 21 Feb 2022 12:09:32 +1100 Subject: [PATCH 6/9] Add ECR policy to kubectl provider tests --- packages/@aws-cdk/aws-eks/test/cluster.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/@aws-cdk/aws-eks/test/cluster.test.ts b/packages/@aws-cdk/aws-eks/test/cluster.test.ts index e3c32b1e345c2..7f10b9307e0ec 100644 --- a/packages/@aws-cdk/aws-eks/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-eks/test/cluster.test.ts @@ -2056,6 +2056,11 @@ describe('cluster', () => { Ref: 'referencetoStackMyClusterD33CAEABArn', }, }, + { + Action: 'ecr:GetAuthorizationToken', + Effect: 'Allow', + Resource: '*', + }, { Action: 'sts:AssumeRole', Effect: 'Allow', @@ -2229,6 +2234,11 @@ describe('cluster', () => { Ref: 'referencetoStackCluster18DFEAC17Arn', }, }, + { + Action: 'ecr:GetAuthorizationToken', + Effect: 'Allow', + Resource: '*', + }, { Action: 'sts:AssumeRole', Effect: 'Allow', From 4def9bc941b18367d31ab1926c17a6bd42cb98af Mon Sep 17 00:00:00 2001 From: Zoey Bendef Date: Mon, 21 Feb 2022 16:08:34 +1100 Subject: [PATCH 7/9] Use managed policy for cross-account ECR --- packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts | 7 +++---- packages/@aws-cdk/aws-eks/test/cluster.test.ts | 12 ------------ 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts b/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts index 49298ea54ecd7..eef07598abf27 100644 --- a/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts +++ b/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts @@ -169,10 +169,9 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider { })); // For OCI helm chart authorization. - this.handlerRole.addToPrincipalPolicy(new iam.PolicyStatement({ - actions: ['ecr:GetAuthorizationToken'], - resources: ['*'], - })); + this.handlerRole.addManagedPolicy( + iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonEC2ContainerRegistryReadOnly'), + ); // allow this handler to assume the kubectl role cluster.kubectlRole.grant(this.handlerRole, 'sts:AssumeRole'); diff --git a/packages/@aws-cdk/aws-eks/test/cluster.test.ts b/packages/@aws-cdk/aws-eks/test/cluster.test.ts index 7f10b9307e0ec..38e73ec1eb4bc 100644 --- a/packages/@aws-cdk/aws-eks/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-eks/test/cluster.test.ts @@ -2056,11 +2056,6 @@ describe('cluster', () => { Ref: 'referencetoStackMyClusterD33CAEABArn', }, }, - { - Action: 'ecr:GetAuthorizationToken', - Effect: 'Allow', - Resource: '*', - }, { Action: 'sts:AssumeRole', Effect: 'Allow', @@ -2078,8 +2073,6 @@ describe('cluster', () => { }, ], }); - - }); test('coreDnsComputeType will patch the coreDNS configuration to use a "fargate" compute type and restore to "ec2" upon removal', () => { @@ -2234,11 +2227,6 @@ describe('cluster', () => { Ref: 'referencetoStackCluster18DFEAC17Arn', }, }, - { - Action: 'ecr:GetAuthorizationToken', - Effect: 'Allow', - Resource: '*', - }, { Action: 'sts:AssumeRole', Effect: 'Allow', From ccb521434c68ec0a586d116d7d245520239658fe Mon Sep 17 00:00:00 2001 From: Zoey Bendef Date: Tue, 22 Feb 2022 10:36:16 +1100 Subject: [PATCH 8/9] Add check for attached managed policies --- .../@aws-cdk/aws-eks/test/cluster.test.ts | 72 ++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-eks/test/cluster.test.ts b/packages/@aws-cdk/aws-eks/test/cluster.test.ts index 38e73ec1eb4bc..1bd91e135ca90 100644 --- a/packages/@aws-cdk/aws-eks/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-eks/test/cluster.test.ts @@ -2073,6 +2073,42 @@ describe('cluster', () => { }, ], }); + + expect(providerStack).toHaveResource('AWS::IAM::Role', { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { Service: 'lambda.amazonaws.com' }, + }, + ], + Version: '2012-10-17', + }, + ManagedPolicyArns: [ + { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/service-role/AWSLambdaBasicExecutionRole', + ]], + }, + { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole', + ]], + }, + { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/AmazonEC2ContainerRegistryReadOnly', + ]], + }, + ], + }); }); test('coreDnsComputeType will patch the coreDNS configuration to use a "fargate" compute type and restore to "ec2" upon removal', () => { @@ -2239,8 +2275,42 @@ describe('cluster', () => { }, }); + expect(providerStack).toHaveResource('AWS::IAM::Role', { + AssumeRolePolicyDocument: { + Statement: [ + { + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { Service: 'lambda.amazonaws.com' }, + }, + ], + Version: '2012-10-17', + }, + ManagedPolicyArns: [ + { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/service-role/AWSLambdaBasicExecutionRole', + ]], + }, + { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/service-role/AWSLambdaVPCAccessExecutionRole', + ]], + }, + { + 'Fn::Join': ['', [ + 'arn:', + { Ref: 'AWS::Partition' }, + ':iam::aws:policy/AmazonEC2ContainerRegistryReadOnly', + ]], + }, + ], + }); }); - }); test('kubectl provider passes security group to provider', () => { From e465959ca342a2490d551acccd3035279066bcfa Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Thu, 24 Feb 2022 11:24:11 +0000 Subject: [PATCH 9/9] Migrated tests to the assertions library --- packages/@aws-cdk/aws-eks/test/cluster.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-eks/test/cluster.test.ts b/packages/@aws-cdk/aws-eks/test/cluster.test.ts index fd81dd910180e..5e579c4f2a247 100644 --- a/packages/@aws-cdk/aws-eks/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-eks/test/cluster.test.ts @@ -2107,7 +2107,7 @@ describe('cluster', () => { ], }); - expect(providerStack).toHaveResource('AWS::IAM::Role', { + Template.fromStack(providerStack).hasResourceProperties('AWS::IAM::Role', { AssumeRolePolicyDocument: { Statement: [ { @@ -2308,7 +2308,7 @@ describe('cluster', () => { }, }); - expect(providerStack).toHaveResource('AWS::IAM::Role', { + Template.fromStack(providerStack).hasResourceProperties('AWS::IAM::Role', { AssumeRolePolicyDocument: { Statement: [ {