feat(eks): Allow helm pull from OCI repositories #18547

harshadbhatia · 2022-01-20T01:38:51Z

The feature allows lambda to install charts from OCI repositories. This also adds login capabilities when the AWS registry is used.

Fixes - #18001

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

gitpod-io · 2022-01-20T01:38:56Z

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

harshadbhatia · 2022-01-23T23:34:24Z

@otaviomacedo , Any updates on this ?

otaviomacedo · 2022-01-24T11:52:44Z

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.


What are the required permissions? Can we automate that for the user?

They would be ecr repository permissions to be able to log in. Ideally, it
should be present on the lambda.

The handler role is this one:

aws-cdk/packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts

Line 164 in 19a287f

this.handlerRole = handler.role!;

You will need to add the ecr:GetAuthorizationToken permission to it

@otaviomacedo , Done. please review

Looks good. But the tests have to be updated now.

@otaviomacedo , When I am trying to run tests from aws-eks directory. It appears to be not picking up tests at all or giving other errors. Could you advise me on the correct steps to execute tests?

yarn build && yarn test should be the only thing you need. Another thing you can try is to build the whole transitive closure before running the test:

aws-eks $ ../../../scripts/buildup

@otaviomacedo , the tests are now complete. Could we please merge this ?

@otaviomacedo , Could you please review?. Thanks

Pull request has been modified.

mergify · 2022-02-23T17:14:33Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2022-02-24T12:29:16Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-02-24T13:09:42Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 8980950
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-02-24T13:10:01Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

harshadbhatia · 2022-02-24T13:54:21Z

@otaviomacedo , thanks a lot for your help !

tsahiduek · 2022-04-04T19:52:42Z

Hi there,
Is this available at CDK V2 2.19.0?
How should I use it?
I've tried the following but it fails:

    cluster.addHelmChart(
      'AckS3', {
      chart: 'ack-s3',
      repository: 'oci://public.ecr.aws/aws-controllers-k8s/s3-chart',
      version: 'v0.0.19',
      namespace: 'ack-system',
      createNamespace: true,
    });

TIA

tsahiduek · 2022-04-05T12:50:27Z

Did someone tested this feature?
I've copied the python code locally to understand why it's not working, and it seems that the helm pull command is executed inside the subprocess, but it isn't really working. When tested locally the output directory is empty (I've modified the tempfile.TemporaryDirectory() creation to tempfile.mkdtemp() in order to persist the directory after the program finished to run

Any help here will be much appreciated

tsahiduek · 2022-04-05T13:56:59Z

I think I've understood why:
The check_output uses shell=True. That means that all arguments of the commands being passed to the check_output are basically been passed to the shell and not to the helm pull command. Using shell is also discouraged from security perspective
References:
https://docs.python.org/3/library/subprocess.html
https://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess

I'll open a PR to fix that (tested locally for now)

…act (#19778) When using helm to pull OCI artifacts, the helm pull command doesn't works well. The [check_output](https://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess) uses shell=True. That means that all arguments of the commands being passed to the check_output are basically been passed to the shell and not to the helm pull command. Using shell is also discouraged from [security perspective](https://docs.python.org/3/library/subprocess.html#security-considerations) References: https://docs.python.org/3/library/subprocess.html > On POSIX with shell=True, the shell defaults to /bin/sh. If args is a string, the string specifies the command to execute through the shell. This means that the string must be formatted exactly as it would be when typed at the shell prompt. This includes, for example, quoting or backslash escaping filenames with spaces in them. If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself. https://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess The previous change that used the `Shell=True` was introduced in commit - #18547 (comment) EDIT: Adding commit for the following items: - Adding integration test for helm OCI support in aws-eks - Upgrading helm version to 3.8.1 in `aws-lambda-layer` because of issues with the current version of helm that is been used, for OCI chart supports - update `integ.eks-helm-asset.expected.json` file ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) - NO new unconventional dependencies ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? - NO * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? - NO *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…act (aws#19778) When using helm to pull OCI artifacts, the helm pull command doesn't works well. The [check_output](https://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess) uses shell=True. That means that all arguments of the commands being passed to the check_output are basically been passed to the shell and not to the helm pull command. Using shell is also discouraged from [security perspective](https://docs.python.org/3/library/subprocess.html#security-considerations) References: https://docs.python.org/3/library/subprocess.html > On POSIX with shell=True, the shell defaults to /bin/sh. If args is a string, the string specifies the command to execute through the shell. This means that the string must be formatted exactly as it would be when typed at the shell prompt. This includes, for example, quoting or backslash escaping filenames with spaces in them. If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself. https://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess The previous change that used the `Shell=True` was introduced in commit - aws#18547 (comment) EDIT: Adding commit for the following items: - Adding integration test for helm OCI support in aws-eks - Upgrading helm version to 3.8.1 in `aws-lambda-layer` because of issues with the current version of helm that is been used, for OCI chart supports - update `integ.eks-helm-asset.expected.json` file ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) - NO new unconventional dependencies ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? - NO * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? - NO *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(eks): Allow helm pull from OCI repositories

bd6da7a

github-actions bot added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label Jan 20, 2022

github-actions bot assigned otaviomacedo Jan 20, 2022

harshadbhatia mentioned this pull request Jan 20, 2022

(aws-eks): Installing helm charts from e.g. public.ecr.aws in the OCI format #18001

Closed

2 tasks

otaviomacedo reviewed Jan 20, 2022

View reviewed changes

packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py Outdated Show resolved Hide resolved

Added README and removed other variable

3788546

Merge branch 'master' into master

11bf4ba

otaviomacedo reviewed Jan 24, 2022

View reviewed changes

harshadbhatia added 3 commits January 25, 2022 10:04

Update README.md

1eff1d5

Added ecr permissions to handler role

493956a

Merge branch 'master' of github.com:harshadbhatia/aws-cdk

e210396

otaviomacedo previously approved these changes Jan 31, 2022

View reviewed changes

Update kubectl-provider.ts

9c2efa2

otaviomacedo added the pr/do-not-merge This PR should not be merged at this time. label Jan 31, 2022

zoeybendef added 3 commits February 21, 2022 12:09

Add ECR policy to kubectl provider tests

edce07f

Use managed policy for cross-account ECR

4def9bc

Add check for attached managed policies

ccb5214

otaviomacedo removed the pr/do-not-merge This PR should not be merged at this time. label Feb 23, 2022

otaviomacedo previously approved these changes Feb 23, 2022

View reviewed changes

mergify bot and others added 2 commits February 23, 2022 18:15

Merge branch 'master' into master

425b407

Migrated tests to the assertions library

e465959

otaviomacedo approved these changes Feb 24, 2022

View reviewed changes

Merge branch 'master' into master

8980950

mergify bot merged commit 7e624d9 into aws:master Feb 24, 2022

tsahiduek mentioned this pull request Apr 6, 2022

fix(eks): malformed command when installing helm chart from OCI artifact #19778

Merged

4 tasks

pahud mentioned this pull request Jan 26, 2023

(eks oci repository): Installing from OCI repository causing helm seg fault #19167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eks): Allow helm pull from OCI repositories #18547

feat(eks): Allow helm pull from OCI repositories #18547

harshadbhatia commented Jan 20, 2022 •

edited

gitpod-io bot commented Jan 20, 2022

harshadbhatia commented Jan 23, 2022

otaviomacedo Jan 24, 2022

harshadbhatia Jan 24, 2022

otaviomacedo Jan 25, 2022

harshadbhatia Jan 31, 2022

otaviomacedo Jan 31, 2022

harshadbhatia Feb 1, 2022

otaviomacedo Feb 4, 2022

harshadbhatia Feb 21, 2022

hbhatia-demyst Feb 23, 2022 •

edited

mergify bot commented Feb 23, 2022

mergify bot commented Feb 24, 2022

aws-cdk-automation commented Feb 24, 2022

mergify bot commented Feb 24, 2022

harshadbhatia commented Feb 24, 2022

tsahiduek commented Apr 4, 2022 •

edited

tsahiduek commented Apr 5, 2022 •

edited

tsahiduek commented Apr 5, 2022 •

edited

feat(eks): Allow helm pull from OCI repositories #18547

feat(eks): Allow helm pull from OCI repositories #18547

Conversation

harshadbhatia commented Jan 20, 2022 • edited

gitpod-io bot commented Jan 20, 2022

harshadbhatia commented Jan 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbhatia-demyst Feb 23, 2022 • edited

Choose a reason for hiding this comment

mergify bot commented Feb 23, 2022

mergify bot commented Feb 24, 2022

aws-cdk-automation commented Feb 24, 2022

AWS CodeBuild CI Report

mergify bot commented Feb 24, 2022

harshadbhatia commented Feb 24, 2022

tsahiduek commented Apr 4, 2022 • edited

tsahiduek commented Apr 5, 2022 • edited

tsahiduek commented Apr 5, 2022 • edited

harshadbhatia commented Jan 20, 2022 •

edited

hbhatia-demyst Feb 23, 2022 •

edited

tsahiduek commented Apr 4, 2022 •

edited

tsahiduek commented Apr 5, 2022 •

edited

tsahiduek commented Apr 5, 2022 •

edited