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

(aws_eks): HelmChart doesn't support repository pointing to OCI uri #32187

Closed
1 task
michelesr opened this issue Nov 19, 2024 · 13 comments
Closed
1 task

(aws_eks): HelmChart doesn't support repository pointing to OCI uri #32187

michelesr opened this issue Nov 19, 2024 · 13 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 p3 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@michelesr
Copy link

Describe the bug

I'm trying to deploy the latest external-dns chart using CDK, however, now it moved to OCI, as in the index.yaml has a reference to the OCI url:

    urls:
    - oci://registry-1.docker.io/bitnamicharts/external-dns:8.6.0

This is causing the helm layer to try to run something like:

$ helm upgrade -n kube-system external-dns oci://registry-1.docker.io/bitnamicharts/external-dns:8.6.0 --version 8.6.0
Error: invalid_reference: invalid tag

It doesn't work because helm doesn't want the tag to be in the URL from what I can see, as this command will work fine:

$ helm upgrade -n kube-system external-dns oci://registry-1.docker.io/bitnamicharts/external-dns --version 8.6.0
Pulled: registry-1.docker.io/bitnamicharts/external-dns:8.6.0
Digest: sha256:aa292106d3ff5c3114f249d8b687a63960b1c9bce9eafff217bbf590b3b1daa3

So in short I believe the lambda layer for helm should check if the URL exposed in the repository index.yaml is in fact an OCI url and remove the tag from it because it causes helm to fail (version can be still specified with --version) .

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

It should be possible to deploy an helm chart if the index.yaml is referencing a OCI uri.

Current Behavior

Helm fails with Error: invalid_reference: invalid tag because the supplied OCI url contains the tag that Helm doesn't want.

Reproduction Steps

Add the following construct to a chart:

        aws_eks.HelmChart(
            self,
            "ExternalDnsControllerChart",
            cluster=eks_cluster,
            repository="https://charts.bitnami.com/bitnami",
            namespace="kube-system",
            release="external-dns",
            chart="external-dns",
            version="8.6.0",
        )

Possible Solution

Modify the layer so that checks for the presence of an OCI url and strips the tag from the url.

Additional Information/Context

No response

CDK CLI Version

2.167.0

Framework Version

No response

Node.js Version

All

OS

All

Language

TypeScript, Python, .NET, Java, Go

Language Version

No response

Other information

No response

@michelesr michelesr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 19, 2024
@github-actions github-actions bot added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label Nov 19, 2024
@pahud pahud self-assigned this Nov 19, 2024
@pahud
Copy link
Contributor

pahud commented Nov 19, 2024

Yes you are right. I am not super familiar with helm with OCI but from what I've learned from Amazon Q, yes,

it should be
oci://registry-1.docker.io/bitnamicharts/external-dns
with an explicit --version 8.6.0

I guess this should be related in https://github.com/aws/aws-cdk/blob/3e7ba32a79da23096501ee3e78eb6190c908118c/packages/%40aws-cdk/custom-resource-handlers/lib/aws-eks/kubectl-handler/helm/__init__.py

My questions:

Given

aws_eks.HelmChart(
            self,
            "ExternalDnsControllerChart",
            cluster=eks_cluster,
            repository="https://charts.bitnami.com/bitnami",
            namespace="kube-system",
            release="external-dns",
            chart="external-dns",
            version="8.6.0",
        )

Did you see this from the lambda logs?

This is causing the helm layer to try to run something like:

$ helm upgrade -n kube-system external-dns oci://registry-1.docker.io/bitnamicharts/external-dns:8.6.0 --version 8.6.0
Error: invalid_reference: invalid tag

How should CDK figure out the OCI URLs from above? From I've learned here, I assume it should just helm upgrade with https://charts.bitnami.com/bitnami as the repository argument, no?

@michelesr
Copy link
Author

This was in the logs:

/aws/lambda/TestEksPlatform-awscdkawseksKubect-Handler886CB40B-9GJ3gwLzWVpi 2024/11/19/[$LATEST]fc7ca74fe3234639a184971e8590a038 [ERROR] Exception: b'Error: invalid_reference: invalid tag\n'

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 94, in helm_handler
    helm('upgrade', release, chart, repository, values_file, namespace, version, wait, timeout, create_namespace, atomic=atomic)
  File "/var/task/helm/__init__.py", line 202, in helm
    raise Exception(output)```

@pahud
Copy link
Contributor

pahud commented Nov 19, 2024

I will validate that later as creating a cluster needs some time but I guess this should work for you.

Are you able to have a quick test on this?

aws_eks.HelmChart(
    self,
    "ExternalDnsControllerChart",
    cluster=eks_cluster,
    repository="oci://registry-1.docker.io/bitnamicharts",
    namespace="kube-system",
    release="external-dns",
    chart="external-dns",
    version="8.6.0",
)

@michelesr
Copy link
Author

michelesr commented Nov 19, 2024

I will validate that later as creating a cluster needs some time but I guess this should work for you.

Are you able to have a quick test on this?

aws_eks.HelmChart(
    self,
    "ExternalDnsControllerChart",
    cluster=eks_cluster,
    repository="oci://registry-1.docker.io/bitnamicharts",
    namespace="kube-system",
    release="external-dns",
    chart="external-dns",
    version="8.6.0",
)

No

17:10:45 | UPDATE_FAILED        | Custom::AWSCDK-EKS-HelmChart          | ExternalDnsControl...t/Resource/Default
Received response status [FAILED] from custom resource. Message returned: Error: b'Error: pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization
failed\n'

Logs: /aws/lambda/TestEksPlatform-awscdkawseksKubect-Handler886CB40B-9GJ3gwLzWVpi

But, for some reason it works if you set oci://registry-1.docker.io/bitnamicharts/external-dns as repository. However, that is more a workaround, as it's bypassing the helm repository. I made a custom solution where we keep our charts versions and repositories in a yaml file and we have a python script to update them when there are new versions, and while it's easy to check for updates for regular helm repositories with an index.yaml, doing that for oci is a lot more complicated so I'd rather keep the original url rather than moving to a direct OCI one.

@michelesr
Copy link
Author

How should CDK figure out the OCI URLs from above? From I've learned here, I assume it should just helm upgrade with https://charts.bitnami.com/bitnami as the repository argument, no?

I think you're right:

 helm template --repo https://charts.bitnami.com/bitnami external-dns --version 8.6.0
Error: invalid_reference: invalid tag
 helm template --repo https://charts.bitnami.com/bitnami external-dns --version 8.5.1 | head
---
# Source: external-dns/templates/networkpolicy.yaml
kind: NetworkPolicy
apiVersion: networking.k8s.io/v1
metadata:
  name: release-name-external-dns
  namespace: "default"
  labels:
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/managed-by: Helm

So this is an helm issue rather than a CDK issue I think.

@pahud
Copy link
Contributor

pahud commented Nov 19, 2024

Thank you!

I think we should have a pytest for our kubectl/helm handler python and list all known use cases/scenarios in the tests. Without that, it would be very difficult to maintain and subject to errors. I will bring it up to the team.

From your provided suggested code:

aws_eks.HelmChart(
            self,
            "ExternalDnsControllerChart",
            cluster=eks_cluster,
            repository="https://charts.bitnami.com/bitnami",
            namespace="kube-system",
            release="external-dns",
            chart="external-dns",
            version="8.6.0",
        )

It seems the helm handler has to figure out the real oci URL from https://charts.bitnami.com/bitnami and eventually construct the full helm upgrade command with oci://registry-1.docker.io/bitnamicharts/external-dns as the URL instead of oci://registry-1.docker.io/bitnamicharts/external-dns:8.6.0 right? I checked that python code but I didn't figure out how to find out the correct oci URL from there. As I am not a helm expert, can you share some insight on how to figure out the correct OCI path if repository="https://charts.bitnami.com/bitnami" is provided?

@michelesr
Copy link
Author

As I am not a helm expert, can you share some insight on how to figure out the correct OCI path if repository="https://charts.bitnami.com/bitnami" is provided?

Well, you would have to look into the index.yaml, parse it and extract the url, but I don't think this is the way to go. It looks like helm is returning the error when --repo is used in combination with a chart that is exposed as OCI uri, so it might be better to raise it upstream rather than hacking around the lamdba handler code.

@michelesr
Copy link
Author

Reported upstream

@pahud
Copy link
Contributor

pahud commented Nov 19, 2024

so it might be better to raise it upstream rather than hacking around the lamdba handler code.

Good callout. Actually, I was thinking maybe we should introduce a method like

eks.HelmChart.fromCommand("upgrade", "arg1", "arg2", "arg3")

just as how you run helm upgrade arg1 arg2 arg3

I am not a big fan that CDK to maintain a translation layer in between as it's very unintuitive and prompt to error.

Generally, when we validate a helm chart, we should first run it via helm CLI against the cluster and if it works, we just use eks.HelmChart.fromCommand() to delegate lambda to run this for us. It could be an escape hatches as well.

cc @xazhao @GavinZZ for visibility

@pahud pahud added p3 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 19, 2024
@pahud pahud removed their assignment Nov 19, 2024
@pahud
Copy link
Contributor

pahud commented Nov 19, 2024

OK let me sharing the debugging here.

My code:

    const cluster = new eks.Cluster(this, 'EKSCluster', {
      vpc,
      version: eks.KubernetesVersion.V1_31,
      defaultCapacity: 1,
      kubectlLayer: new KubectlV31Layer(this, 'KubectlLayer'),
    });

    cluster.addHelmChart('ExternalDnsControllerChart', {
      repository: 'https://charts.bitnami.com/bitnami',
      namespace: 'kube-system',
      release: 'external-dns', 
      chart: 'external-dns',
      version: '8.6.0'
    });

And I updated the helm/__init__.py from

/node_modules/aws-cdk-lib/custom-resource-handlers/dist/aws-eks/kubectl-handler/helm/init.py

adding the two lines as below:

image

Now I deploy and I can see this in the cloudwatch logs

[INFO]	2024-11-19T18:32:50.604Z	41584a41-7c94-45d4-b1e1-56acc20167a9	helm command: ['helm', 'upgrade', 'external-dns', 'external-dns', '--install', '--create-namespace', '--repo', 'https://charts.bitnami.com/bitnami', '--version', '8.6.0', '--namespace', 'kube-system', '--kubeconfig', '/tmp/kubeconfig']

Now, let's join this array

>>> ' '.join(['helm', 'upgrade', 'external-dns', 'external-dns', '--install', '--create-namespace', '--repo', 'https://charts.bitnami.com/bitnami', '--version', '8.6.0', '--namespace', 'kube-system', '--kubeconfig', '/tmp/kubeconfig'])

'helm upgrade external-dns external-dns --install --create-namespace --repo https://charts.bitnami.com/bitnami --version 8.6.0 --namespace kube-system --kubeconfig /tmp/kubeconfig'

So what's happening in Lambda is actually running this helm command rather than

helm upgrade -n kube-system external-dns oci://registry-1.docker.io/bitnamicharts/external-dns:8.6.0 --version 8.6.0

as you mentioned in the original description.

Obviously, the CDK helm handler would NOT find out the OCI URI from repo and we need to figure out what is the correct helm command without specifying the OCI URI.

Can you share a working helm command for that?

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 19, 2024
@michelesr
Copy link
Author

michelesr commented Nov 19, 2024

Yes, my original assumption was wrong. The handler doesn't transform the repository uri and it just passes it to the --repo argument. I believe the handler is behaving as intended, and the problem is in how Helm handles the command when the chart url is using the OCI protocol, as I reported upstream.

@michelesr
Copy link
Author

I'm going to close this as I don't think there's anything that needs to happen on the CDK side.

Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 p3 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

2 participants