Skip to content

Commit

Permalink
fix(eks): nested OCI repository names for private ECR helmchart deplo…
Browse files Browse the repository at this point in the history
…yments are not properly handled (#23378)

Fixes #22340

I've changed the way of extracting components (registry domain & region) from repository to regexp with named capture groups. 

I've tested it by extending `integ.eks-helm-asset.ts` with second deployment from manually created private ECR. But I didn't provide coverage for this issue, because before deployment private ECR has to be created and filled by an example chart - i'm not aware of any simple way to automate a second part in tests.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
  • Loading branch information
psemeniuk authored Feb 16, 2023
1 parent 462a42b commit 72f2a95
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 12 deletions.
20 changes: 9 additions & 11 deletions packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import shutil
import tempfile
import zipfile
from urllib.parse import urlparse, unquote

logger = logging.getLogger()
logger.setLevel(logging.INFO)
Expand Down Expand Up @@ -95,26 +94,25 @@ def helm_handler(event, context):

def get_oci_cmd(repository, version):
# Generates OCI command based on pattern. Public ECR vs Private ECR are treated differently.
cmnd = []
private_ecr_pattern = '\d+.dkr.ecr.[a-z]+-[a-z]+-\d.amazonaws.com'
public_ecr = 'public.ecr.aws'
private_ecr_pattern = 'oci://(?P<registry>\d+.dkr.ecr.(?P<region>[a-z]+-[a-z]+-\d).amazonaws.com)*'
public_ecr_pattern = 'oci://(?P<registry>public.ecr.aws)*'

registry = repository.rsplit('/', 1)[0].replace('oci://', '')
private_registry = re.match(private_ecr_pattern, repository).groupdict()
public_registry = re.match(public_ecr_pattern, repository).groupdict()

if re.fullmatch(private_ecr_pattern, registry) is not None:
if private_registry['registry'] is not None:
logger.info("Found AWS private repository")
region = registry.replace('.amazonaws.com', '').split('.')[-1]
cmnd = [
f"aws ecr get-login-password --region {region} | " \
f"helm registry login --username AWS --password-stdin {registry}; helm pull {repository} --version {version} --untar"
f"aws ecr get-login-password --region {private_registry['region']} | " \
f"helm registry login --username AWS --password-stdin {private_registry['registry']}; helm pull {repository} --version {version} --untar"
]
elif registry.startswith(public_ecr):
elif public_registry['registry'] is not None:
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"
f"helm registry login --username AWS --password-stdin {public_registry['registry']}; helm pull {repository} --version {version} --untar"
]
else:
logger.error("OCI repository format not recognized, falling back to helm pull")
Expand Down
9 changes: 8 additions & 1 deletion packages/@aws-cdk/aws-eks/test/integ.eks-helm-asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ class EksClusterStack extends Stack {
createNamespace: true,
});

// there is no opinionated way of testing charts from private ECR, so there is description of manual steps needed to reproduce:
// 1. `export AWS_PROFILE=youraccountprofile; aws ecr create-repository --repository-name helm-charts-test/s3-chart --region YOUR_REGION`
// 2. `helm pull oci://public.ecr.aws/aws-controllers-k8s/s3-chart --version v0.1.0`
// 3. Login to ECR (howto: https://docs.aws.amazon.com/AmazonECR/latest/userguide/push-oci-artifact.html )
// 4. `helm push s3-chart-v0.1.0.tgz oci://YOUR_ACCOUNT_ID.dkr.ecr.YOUR_REGION.amazonaws.com/helm-charts-test/`
// 5. Change `repository` in above test to oci://YOUR_ACCOUNT_ID.dkr.ecr.YOUR_REGION.amazonaws.com/helm-charts-test
// 6. Run integration tests as usual

this.cluster.addHelmChart('test-oci-chart-different-release-name', {
chart: 'lambda-chart',
release: 'lambda-chart-release',
Expand All @@ -79,4 +87,3 @@ new integ.IntegTest(app, 'aws-cdk-eks-helm', {
});

app.synth();

0 comments on commit 72f2a95

Please sign in to comment.