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

Error pulling latest chart from OCI registry if semver version has a v prefix #11107

Closed
wallrj opened this issue Jul 2, 2022 · 12 comments
Closed
Labels
bug Categorizes issue or PR as related to a bug. oci Related to Helm OCI feature Stale

Comments

@wallrj
Copy link

wallrj commented Jul 2, 2022

If a Helm chart is in an OCI registry and has a version with v prefix before the semver, many of the helm subcommands can not find it.

I think the problem is caused by the use of StrictNewVersion vs NewVersion here in the registry/client.go file:

helm/pkg/registry/client.go

Lines 622 to 630 in 79cd4dd

var tagVersions []*semver.Version
for _, tag := range registryTags {
// Change underscore (_) back to plus (+) for Helm
// See https://github.com/helm/helm/issues/10166
tagVersion, err := semver.StrictNewVersion(strings.ReplaceAll(tag, "_", "+"))
if err == nil {
tagVersions = append(tagVersions, tagVersion)
}
}

Elsewhere in the Helm code, the more lenient semver.NewVersion is used:

$ git grep 'semver\.\(Strict\)\?NewVersion'
cmd/helm/search/search.go:              v1, err := semver.NewVersion(first.Chart.Version)
cmd/helm/search/search.go:              v2, err := semver.NewVersion(second.Chart.Version)
cmd/helm/search_repo.go:                v, err := semver.NewVersion(r.Chart.Version)
internal/resolver/resolver.go:                  v, err := semver.NewVersion(ch.Metadata.Version)
internal/resolver/resolver.go:                  _, err := semver.NewVersion(version)
internal/resolver/resolver.go:                  v, err := semver.NewVersion(ver.Version)
pkg/action/dependency.go:                       if _, err := semver.StrictNewVersion(maybeVersion); err == nil {
pkg/action/dependency.go:               v, err := semver.NewVersion(depChart.Metadata.Version)
pkg/action/dependency.go:                       v, err := semver.NewVersion(c.Metadata.Version)
pkg/action/package.go:  if _, err := semver.NewVersion(ver); err != nil {
pkg/chart/metadata.go:  _, err := semver.NewVersion(v)
pkg/chartutil/capabilities.go:  sv, err := semver.NewVersion(version)
pkg/chartutil/compatible.go:    sv, err := semver.NewVersion(ver)
pkg/downloader/chart_downloader.go:     _, errSemVer := semver.NewVersion(version)
pkg/downloader/manager.go:                      v, err := semver.NewVersion(ch.Metadata.Version)
pkg/downloader/manager.go:      sv1, err := semver.NewVersion(v1)
pkg/downloader/manager.go:      sv2, err := semver.NewVersion(v2)
pkg/downloader/manager.go:      v, err := semver.NewVersion(ch.Metadata.Version)
pkg/lint/rules/chartfile.go:    version, err := semver.NewVersion(cf.Version)
pkg/plugin/installer/vcs_installer.go:          if v, err := semver.NewVersion(r); err == nil {
pkg/registry/client.go:         tagVersion, err := semver.StrictNewVersion(strings.ReplaceAll(tag, "_", "+"))
pkg/registry/util.go:           test, err := semver.NewVersion(v)
pkg/repo/index.go:      i, err := semver.NewVersion(c[a].Version)
pkg/repo/index.go:      j, err := semver.NewVersion(c[b].Version)
pkg/repo/index.go:              test, err := semver.NewVersion(ver.Version)

For example:

With a strict semver versioned chart, as used for testing pkg/registry/client.go, both push and pull work with an OCI registry:

$ helm push pkg/downloader/testdata/local-subchart-0.1.0.tgz oci://gcr.io/jetstack-richard/test-chart
Pushed: gcr.io/jetstack-richard/test-chart/local-subchart:0.1.0
Digest: sha256:7cfc000b6a983861357c02832022aa907d5fa614730f3e4a2daf8872f00c32c7

$ helm pull oci://gcr.io/jetstack-richard/test-chart/local-subchart
Pulled: gcr.io/jetstack-richard/test-chart/local-subchart:0.1.0
Digest: sha256:7cfc000b6a983861357c02832022aa907d5fa614730f3e4a2daf8872f00c32c7

But with the Helm chart for cert-manager which uses a vX.Y.Z version number:

$ helm show chart ./cert-manager-v1.8.2.tgz 
...
apiVersion: v1
appVersion: v1.8.2
version: v1.8.2
...

Push it to an OCI registry:

$ helm push cert-manager-v1.8.2.tgz oci://gcr.io/jetstack-richard/cert-manager-chart
Pushed: gcr.io/jetstack-richard/cert-manager-chart/cert-manager:v1.8.2
Digest: sha256:5dd707dbffebc478d1005016674bdb8484a52b64355486c3afd8d350bf8c0578

helm pull fails, without specifying a version:

$ helm pull oci://gcr.io/jetstack-richard/cert-manager-chart/cert-manager
Error: Unable to locate any tags in provided repository: oci://gcr.io/jetstack-richard/cert-manager-chart/cert-manager

helm pull succeeds when a --version is supplied:

$ helm pull oci://gcr.io/jetstack-richard/cert-manager-chart/cert-manager --version v1.8.2
Pulled: gcr.io/jetstack-richard/cert-manager-chart/cert-manager:v1.8.2
Digest: sha256:5dd707dbffebc478d1005016674bdb8484a52b64355486c3afd8d350bf8c0578

xref:

Helm version info

$ helm version
version.BuildInfo{Version:"v3.9.0", GitCommit:"7ceeda6c585217a19a1131663d8cd1f7d641b2a7", GitTreeState:"clean", GoVersion:"go1.17.5"}
$ kubectl version --output=yaml
clientVersion:
  buildDate: "2022-05-03T13:46:05Z"
  compiler: gc
  gitCommit: 4ce5a8954017644c5420bae81d72b09b735c21f0
  gitTreeState: clean
  gitVersion: v1.24.0
  goVersion: go1.18.1
  major: "1"
  minor: "24"
  platform: linux/amd64
kustomizeVersion: v4.5.4
@bacongobbler bacongobbler added the bug Categorizes issue or PR as related to a bug. label Jul 6, 2022
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Oct 5, 2022
@wallrj
Copy link
Author

wallrj commented Oct 11, 2022

/lifecycle frozen

@github-actions github-actions bot removed the Stale label Oct 12, 2022
@github-actions
Copy link

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Jan 10, 2023
@fredgate
Copy link

fredgate commented Jan 10, 2023

This is still a problem. For example with chart of cert-mnager : cert-manager/cert-manager#2380

To see if Helm keeps requirement about Semver version, or if it can accomodate with a v prefix.

@github-actions
Copy link

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Apr 26, 2023
@joejulian joejulian removed the Stale label May 1, 2023
@github-actions
Copy link

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Jul 31, 2023
@fredgate
Copy link

fredgate commented Aug 1, 2023

This is still a problem. For example with chart of cert-mnager : cert-manager/cert-manager#2380

To see if Helm keeps requirement about Semver version, or if it can accomodate with a v prefix.

@morremeyer
Copy link

morremeyer commented Sep 18, 2023

@fredgate I would not consider this a bug in helm. This is a problem about cert-manager having chart versions that violate the specification. As you suggested in cert-manager/cert-manager#2380, this has to be fixed on the cert-manager side.

Edit: I do consider this a bug, see #12403

@fredgate
Copy link

I tend to agree with you.
Unfortunately cert-manager is not very responsive

@wallrj
Copy link
Author

wallrj commented Dec 18, 2023

I still consider this a bug in Helm and wrote the following in cert-manager/cert-manager#2380 (comment):

The helm binary (which is surely the reference implementation of the official spec) does (mostly) work with charts where the version has a v prefix.
For example we use helm package to create the cert-manager chart archive, and we use helm lint --strict to check the chart for standards compliance.

Neither of those tools emits any error or warning about the v in the cert-manager Chart version.

helm push can push the cert-manager chart to an OCI registry.
helm install can pull the cert-manager chart from an OCI registry using a --version argument with or without the v prefix.
The only part that doesn't work with OCI charts is the use of semver version ranges or the discovery of latest.

BTW semver ranges and discovery of latest do work when using a standard chart repository.

The Helm Charts and Versioning documentation says:

Every chart must have a version number. A version must follow the SemVer 2 standard. Unlike Helm Classic, Helm v2 and later uses version numbers as release markers. Packages in repositories are identified by name plus version.
More complex SemVer 2 names are also supported, such as version: 1.2.3-alpha.1+ef365. But non-SemVer names are explicitly disallowed by the system.

Given that helm package and helm lint --strict and helm install do not "explicitly disallow" the v prefix, it seems reasonable to assume that it is acceptable.

Neither Migrating Helm v2 to v3 nor Changes Since Helm 2 say anything about stricter version parsing.

So it seems to me that it's a bug in helm, which could be fixed by:
removing the v prefix when computing the OCI tag before pushing the chart, or by
using the non-strict semver parser when helm filters the tags in the OCI repo.

@morremeyer
Copy link

@wallrj Thinking about that even back then, I changed my opinion. TL;DR: I agree with you.

I opened #12403 a while ago for that exact reason.

Copy link

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug. oci Related to Helm OCI feature Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants