helm / helm

The Kubernetes Package Manager
https://helm.sh
Apache License 2.0
26.88k stars 7.09k forks source link

Tags with version containing + character is broken #13368

Open nicholasSUSE opened 1 week ago

nicholasSUSE commented 1 week ago

Output of helm version:

version.BuildInfo{Version:"v3.16.1", GitCommit:"5a5449dc42be07001fd5771d56429132984ab3ab", GitTreeState:"clean", GoVersion:"go1.22.7"}

Output of kubectl version:

Client Version: v1.30.0

Problem

At: func (c *Client) Tags(ref string) ([]string, error)

registry.ParseReference(ref) will throw an error for a version like 100.0.0+up0.0.0.

The + character will break the regular expression at the tagRegexp in the oras-project.

As stated on https://helm.sh/docs/chart_best_practices/conventions/#version-numbers The 100.0.0+up0.0.0 should comply with SemVer2 and thus be accepted by Helm.

At the Tags function, there is a special handling for + characters after the ParseReference(ref) function: https://github.com/helm/helm/blob/main/pkg/registry/client.go#L686

This will never be reached since the error has been thrown before.

mkulke commented 5 days ago

hey, I tried to reproduce the problem, and I wasn't able to do so. I packaged and pushed a version w/ + in it and it seems to survive the roundtrip. Can you elaborate which operation fails for you specifally?

$ helm package ./my-chart/
Successfully packaged chart and saved it to: .../myapp-100.0.0+up0.0.0.tgz

$ helm push myapp-100.0.0+up0.0.0.tgz oci://ghcr.io/mkulke/helm-charts
Pushed: ghcr.io/mkulke/helm-charts/myapp:100.0.0_up0.0.0
Digest: sha256:c1d65ddd012070e97489c5e026e11d56ef80c85a0d276cd16b14063a0e60075a
ghcr.io/mkulke/helm-charts/myapp:100.0.0_up0.0.0 contains an underscore.

OCI artifact references (e.g. tags) do not support the plus sign (+). To support
storing semantic versions, Helm adopts the convention of changing plus (+) to
an underscore (_) in chart version tags when pushing to a registry and back to
a plus (+) when pulling from a registry.

$ helm upgrade myapp oci://ghcr.io/mkulke/helm-charts/myapp --version 100.0.0+up0.0.0
Pulled: ghcr.io/mkulke/helm-charts/myapp:100.0.0_up0.0.0
Digest: sha256:c1d65ddd012070e97489c5e026e11d56ef80c85a0d276cd16b14063a0e60075a
ghcr.io/mkulke/helm-charts/myapp:100.0.0_up0.0.0 contains an underscore.

OCI artifact references (e.g. tags) do not support the plus sign (+). To support
storing semantic versions, Helm adopts the convention of changing plus (+) to
an underscore (_) in chart version tags when pushing to a registry and back to
a plus (+) when pulling from a registry.
Release "myapp" has been upgraded. Happy Helming!
NAME: myapp
LAST DEPLOYED: Mon Oct  7 16:08:35 2024
NAMESPACE: default
STATUS: deployed
REVISION: 14
TEST SUITE: None

$ helm list -o json | jq .[].chart
"myapp-100.0.0+up0.0.0"
nicholasSUSE commented 4 days ago

Hello @mkulke, the helm push works fine.

I don't know which helm cli command fails. I was implementing an automation to push charts to an OCI registry and using the helm project directly in go. Before the push, I want to check if the chart is already in the OCI registry.

Here is my code:

// checkAsset checks if a specific asset version exists in the OCI registry
func checkAsset(helmClient *registry.Client, ociDNS, chart, version string) (bool, error) {
    res, err := helmClient.Tags(ociDNS + "/" + chart + ":" + version)
    if err != nil {
        if strings.Contains(err.Error(), "unexpected status code 404: name unknown: repository name not known to registry") {
            return false, nil
        }
        return false, err
    }

If you try to specify the version like that, it will throw the error.

So currently I made a workaround like this that does not fail:

// checkAsset checks if a specific asset version exists in the OCI registry
func checkAsset(helmClient *registry.Client, ociDNS, chart, version string) (bool, error) {
    // Once issue is resolved: https://github.com/helm/helm/issues/13368
    // Replace by: helmClient.Tags(ociDNS + "/" + chart + ":" + version)
    existingVersions, err := helmClient.Tags(ociDNS + "/" + chart)
    if err != nil {
        if strings.Contains(err.Error(), "unexpected status code 404: name unknown: repository name not known to registry") {
            return false, nil
        }
        return false, err
    }

    for _, existingVersion := range existingVersions {
        if existingVersion == version {
            return true, nil
        }
    }

    return false, nil
}
mkulke commented 4 days ago

are you able to drop the version tag from your call to Tags()?

res, err := helmClient.Tags(ociDNS + "/" + chart)

I think it should produce the same results (list of tags for the image) but the oras verifier will not stumble over a + in the provided version.

I suppose the implicit assumption is that the Tags will be called without the tags suffix in the provided reference. the function is called for example by helm show all oci://ghcr.io/some-user/my-chart