Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EKS: Pull helm/chart using oci #20402

Closed
mburket opened this issue May 18, 2022 · 10 comments
Closed

EKS: Pull helm/chart using oci #20402

mburket opened this issue May 18, 2022 · 10 comments
Labels
@aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@mburket
Copy link

mburket commented May 18, 2022

Describe the bug

Hi,

I am trying to install helm/chart from the AWS ECR, but ran into error when the kubectl_handler pulls the helm/chart. Looks like it is reading the entire ecr login and helm pull commands as a file name. Please see error below (I masked our aws account id and the repo name):

[ERROR] FileNotFoundError: [Errno 2] No such file or directory: 'aws ecr get-login-password --region us-east-1 | helm registry login --username AWS --password-stdin xxx.dkr.ecr.us-east-1.amazonaws.com; helm pull oci://xxx.dkr.ecr.us-east-1.amazonaws.com/xxx --version 0.1.0 --untar': 'aws ecr get-login-password --region us-east-1 | helm registry login --username AWS --password-stdin xxx.dkr.ecr.us-east-1.amazonaws.com; helm pull oci://xxx.dkr.ecr.us-east-1.amazonaws.com/xxx --version 0.1.0 --untar'
Traceback (most recent call last):
  File "/var/task/index.py", line 17, in handler
    return helm_handler(event, context)
  File "/var/task/helm/init.py", line 85, in helm_handler
    chart_dir = get_chart_from_oci(tmpdir.name, release, repository, version)
  File "/var/task/helm/init.py", line 126, in get_chart_from_oci
    output = subprocess.check_output(cmnd, stderr=subprocess.STDOUT, cwd=tmpdir, env=env)
  File "/var/lang/lib/python3.7/subprocess.py", line 411, in check_output
    **kwargs).stdout
  File "/var/lang/lib/python3.7/subprocess.py", line 488, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/var/lang/lib/python3.7/subprocess.py", line 800, in init
    restore_signals, start_new_session)
  File "/var/lang/lib/python3.7/subprocess.py", line 1551, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)

Expected Behavior

helm/chart pull and installation should work when chart is stored in AWS ECR.

Current Behavior

kubectl_handler throws FileNotFoundError.

Reproduction Steps

Add a helm/chart as below in code:

new eks.HelmChart(scope, 'xxx', {
cluster: cluster,
chart: 'xxx',
release: 'xxx',
repository: oci://${accountId}.dkr.ecr.${region}.amazonaws.com/xxx,
namespace: 'kube-system',
version: '0.1.0'
});

Possible Solution

A good discussion from stackoverflow: https://stackoverflow.com/questions/24306205/file-not-found-error-when-launching-a-subprocess-containing-piped-commands

Additional Information/Context

No response

CDK CLI Version

2.21.1

Framework Version

No response

Node.js Version

1.14

OS

MacOs

Language

Typescript

Language Version

No response

Other information

No response

@mburket mburket added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 18, 2022
@github-actions github-actions bot added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label May 18, 2022
@Naumel Naumel added p2 effort/medium Medium work item – several days of effort labels May 25, 2022
@peterwoodworth peterwoodworth removed the needs-triage This issue or PR still needs to be triaged. label May 25, 2022
@calebpalmer
Copy link

I'm also seeing this same error.

@harshadbhatia
Copy link
Contributor

harshadbhatia commented Jun 10, 2022

I wrote the first implementation after testing the changes. Which was patched by this PR #19778

I have raised some comments with author to clarify. @mburket , feel free to raise PR if you like. Otherwise, i will put PR for reverting that change.

@otaviomacedo , What are your thoughts around the shell=True ? We could move conversation on this issue ?

@hbhatia-demyst
Copy link

@otaviomacedo , have put a PR to fix this and also support public ECR as well. #20724

@otaviomacedo otaviomacedo removed their assignment Jun 27, 2022
mergify bot pushed a commit that referenced this issue Jun 27, 2022
This fixes the change made by the following PR. #19778

`shell=True` caused regression observed in the following issue: [20402](#20402)

The code should now allow Public and Private AWS ECR repositories to work with oci prefix.

----

### All Submissions:

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

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-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`)? No

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

craigjam commented Jul 5, 2022

is this working for anyone?
In 2.30.0, which should contain the PR above, I get the following error - but this command runs fine standalone:
[ERROR] FileNotFoundError: [Errno 2] No such file or directory: 'aws ecr get-login-password --region us-west-2 | helm registry login --username AWS --password-stdin account.dkr.ecr.us-west-2.amazonaws.com; helm pull oci://account.dkr.ecr.us-west-2.amazonaws.com/my-helm-app --version v0.1.0 --untar': 'aws ecr get-login-password --region us-west-2 | helm registry login --username AWS --password-stdin account.dkr.ecr.us-west-2.amazonaws.com; helm pull oci://account.dkr.ecr.us-west-2.amazonaws.com/my-helm-app --version v0.1.0 --untar'

@harshadbhatia
Copy link
Contributor

I personally have been waiting for it merged. Is this a private or a public repository which you are using ?

If you stack was previously created i.e. prior to cdk version upgrade you will notice that CDK doesnt actually update the lambda code, so you might have to recreate the stack.

@craigjam
Copy link

craigjam commented Jul 5, 2022

I destroyed and recreated the stack as I suspected that might be the case!
It does work with (my) public ECR repository - presumably this is because the private ECR case uses | (pipe) to set the ECR credentials and doesn't work unless Shell=True (as mentioned in #19778 ) ?

daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
This fixes the change made by the following PR. aws#19778

`shell=True` caused regression observed in the following issue: [20402](aws#20402)

The code should now allow Public and Private AWS ECR repositories to work with oci prefix.

----

### All Submissions:

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

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-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`)? No

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

Hey everyone!
I experienced the same issue on version 1.65.0. I saw that the bug was resolved on version 2.21.0 but at v1 it's just not...
Once I added Shell=True it works but it's a manual process. Please fix the bug on v1 too.

@Obirah
Copy link

Obirah commented Jul 25, 2022

I'm witnessing another weird behavior: when I try to pull a Helm Chart from an OCI repo (Non-ECR-repo; aws-cdk 2.33.0), instead of doing the pull, Helm only prints its help in the kubectl-layer as if the issued command only was helm and after that the Chart deployment of course fails because there are no Chart files.

@pahud
Copy link
Contributor

pahud commented Mar 21, 2023

As #20724 has been merged, I presume this should be fixed. I'm closing it for now. Feel free to re-open if it's still relevant.

@pahud pahud closed this as completed Mar 21, 2023
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests