Repository: helm/helm
Pull request #30894 (2025-05-21 19:39:16)

Prevent push cmd failure in 3.18 by handling version tag resolution in ORAS memory store
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/main/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:

Fix: https://github.com/helm/helm/issues/30881

Initial work for `dev-v3` in https://github.com/helm/helm/pull/30889#issuecomment-2898909440 but as requested by @gjenkins8, we should backport after on v3. :)

The issue is:
- The `newReference()` function transforms version tags by replacing + with _ for OCI compatibility (as we can see in the output message)
- But the code was using the original ref (with +) for `TagBytes()`
- Then it tries to find the tagged reference using `parsedRef.String()` (with _)
- This mismatch causes the `Resolve` method to fail with "not found"
- By using `parsedRef.String()` consistently in both places, the references will match and the lookup will succeed.

I extracted the `TagBytes` function to improve testability. `Push()` includes several external calls that are hard to mock, so isolating this logic makes testing more manageable.

The patch could be summarized as:
```diff
- oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, manifestData, ref)
+ oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, manifestData, parsedRef.String())
```

The initial regression is something that was missed in https://github.com/helm/helm/commit/5a7046b9bfaf1d7e97c555e33fa8744bba758004#diff-acbc1e889c3e91003a88f18e21c3073b64937801b20f663fcf2601540b9e3cdaR709

Before :
```sh
» ../../helm/bin/helm push ./test-0.0.0-snapshot+dirty.tgz oci://ghcr.io/benoittgt/helm-charts/test --debug
Error: failed to perform "Resolve" on source: ghcr.io/benoittgt/helm-charts/test/test:0.0.0-snapshot_dirty: not found
helm.go:92: 2025-05-21 16:41:49.581662 +0200 CEST m=+0.041508960 [debug] failed to perform "Resolve" on source: ghcr.io/benoittgt/helm-charts/test/test:0.0.0-snapshot_dirty: not found
```

After :

```sh
» ../../helm/bin/helm push ./test-0.0.0-snapshot+dirty.tgz oci://ghcr.io/benoittgt/helm-charts/test --debug
Pushed: ghcr.io/benoittgt/helm-charts/test/test:0.0.0-snapshot_dirty
Digest: sha256:680f23c71cb6727ae8845daf01f019dc11bdac4b652289c9a970b270c56ba79d
ghcr.io/benoittgt/helm-charts/test/test:0.0.0-snapshot_dirty 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.
```

**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains user facing changes (the `docs needed` label should be applied if so)
- [x] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility (it will be backported)



Patch:
diff --git a/.golangci.yml b/.golangci.yml
index 156dd0509..a1deb3aeb 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -20,7 +20,6 @@ linters:
   enable:
     - depguard
     - dupl
-    - gomodguard
     - govet
     - ineffassign
     - misspell
@@ -57,13 +56,6 @@ linters:
     dupl:
       threshold: 400
 
-    gomodguard:
-      blocked:
-        modules:
-          - github.com/evanphx/json-patch:
-              recommendations:
-                - github.com/evanphx/json-patch/v5
-
 run:
   timeout: 10m
 
diff --git a/go.mod b/go.mod
index dc8ffab39..46d9ce97a 100644
--- a/go.mod
+++ b/go.mod
@@ -13,7 +13,7 @@ require (
 	github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2
 	github.com/cyphar/filepath-securejoin v0.4.1
 	github.com/distribution/distribution/v3 v3.0.0
-	github.com/evanphx/json-patch/v5 v5.9.11
+	github.com/evanphx/json-patch v5.9.11+incompatible
 	github.com/fluxcd/cli-utils v0.36.0-flux.13
 	github.com/foxcpp/go-mockdns v1.1.0
 	github.com/gobwas/glob v0.2.3
@@ -68,6 +68,7 @@ require (
 	github.com/docker/go-events v0.0.0-20190806004212-e31b211e4f1c // indirect
 	github.com/docker/go-metrics v0.0.1 // indirect
 	github.com/emicklei/go-restful/v3 v3.12.1 // indirect
+	github.com/evanphx/json-patch/v5 v5.9.11 // indirect
 	github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f // indirect
 	github.com/fatih/color v1.13.0 // indirect
 	github.com/felixge/httpsnoop v1.0.4 // indirect
diff --git a/go.sum b/go.sum
index ebc7ed7f2..b98d3165d 100644
--- a/go.sum
+++ b/go.sum
@@ -77,6 +77,8 @@ github.com/docker/go-metrics v0.0.1 h1:AgB/0SvBxihN0X8OR4SjsblXkbMvalQ8cjmtKQ2rQ
 github.com/docker/go-metrics v0.0.1/go.mod h1:cG1hvH2utMXtqgqqYE9plW6lDxS3/5ayHzueweSI3Vw=
 github.com/emicklei/go-restful/v3 v3.12.1 h1:PJMDIM/ak7btuL8Ex0iYET9hxM3CI2sjZtzpL63nKAU=
 github.com/emicklei/go-restful/v3 v3.12.1/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc=
+github.com/evanphx/json-patch v5.9.11+incompatible h1:ixHHqfcGvxhWkniF1tWxBHA0yb4Z+d1UQi45df52xW8=
+github.com/evanphx/json-patch v5.9.11+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk=
 github.com/evanphx/json-patch/v5 v5.9.11 h1:/8HVnzMq13/3x9TPvjG08wUGqBTmZBsCWzjTM0wiaDU=
 github.com/evanphx/json-patch/v5 v5.9.11/go.mod h1:3j+LviiESTElxA4p3EMKAB9HXj3/XEtnUf6OZxqIQTM=
 github.com/exponent-io/jsonpath v0.0.0-20210407135951-1de76d718b3f h1:Wl78ApPPB2Wvf/TIe2xdyJxTlb6obmF18d8QdkxNDu4=
diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go
index 75c73e959..f0905e508 100644
--- a/pkg/chart/v2/loader/load.go
+++ b/pkg/chart/v2/loader/load.go
@@ -19,6 +19,7 @@ package loader
 import (
 	"bufio"
 	"bytes"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -223,7 +224,10 @@ func LoadValues(data io.Reader) (map[string]interface{}, error) {
 			}
 			return nil, fmt.Errorf("error reading yaml document: %w", err)
 		}
-		if err := yaml.Unmarshal(raw, &currentMap); err != nil {
+		if err := yaml.Unmarshal(raw, &currentMap, func(d *json.Decoder) *json.Decoder {
+			d.UseNumber()
+			return d
+		}); err != nil {
 			return nil, fmt.Errorf("cannot unmarshal yaml document: %w", err)
 		}
 		values = MergeMaps(values, currentMap)
diff --git a/pkg/chart/v2/util/values.go b/pkg/chart/v2/util/values.go
index 6850e8b9b..42b1a28e8 100644
--- a/pkg/chart/v2/util/values.go
+++ b/pkg/chart/v2/util/values.go
@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -105,7 +106,10 @@ func tableLookup(v Values, simple string) (Values, error) {
 
 // ReadValues will parse YAML byte data into a Values.
 func ReadValues(data []byte) (vals Values, err error) {
-	err = yaml.Unmarshal(data, &vals)
+	err = yaml.Unmarshal(data, &vals, func(d *json.Decoder) *json.Decoder {
+		d.UseNumber()
+		return d
+	})
 	if len(vals) == 0 {
 		vals = Values{}
 	}
diff --git a/pkg/kube/client.go b/pkg/kube/client.go
index 9bbd4d9ba..a812fc198 100644
--- a/pkg/kube/client.go
+++ b/pkg/kube/client.go
@@ -30,7 +30,7 @@ import (
 	"strings"
 	"sync"
 
-	jsonpatch "github.com/evanphx/json-patch/v5"
+	jsonpatch "github.com/evanphx/json-patch"
 	v1 "k8s.io/api/core/v1"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
diff --git a/pkg/registry/client.go b/pkg/registry/client.go
index 2d131dc47..f31821166 100644
--- a/pkg/registry/client.go
+++ b/pkg/registry/client.go
@@ -685,19 +685,9 @@ func (c *Client) Push(data []byte, ref string, options ...PushOption) (*PushResu
 	})
 
 	ociAnnotations := generateOCIAnnotations(meta, operation.creationTime)
-	manifest := ocispec.Manifest{
-		Versioned:   specs.Versioned{SchemaVersion: 2},
-		Config:      configDescriptor,
-		Layers:      layers,
-		Annotations: ociAnnotations,
-	}
 
-	manifestData, err := json.Marshal(manifest)
-	if err != nil {
-		return nil, err
-	}
-
-	manifestDescriptor, err := oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, manifestData, ref)
+	manifestDescriptor, err := c.tagManifest(ctx, memoryStore, configDescriptor,
+		layers, ociAnnotations, parsedRef)
 	if err != nil {
 		return nil, err
 	}
@@ -898,3 +888,24 @@ func (c *Client) ValidateReference(ref, version string, u *url.URL) (*url.URL, e
 
 	return u, err
 }
+
+// tagManifest prepares and tags a manifest in memory storage
+func (c *Client) tagManifest(ctx context.Context, memoryStore *memory.Store,
+	configDescriptor ocispec.Descriptor, layers []ocispec.Descriptor,
+	ociAnnotations map[string]string, parsedRef reference) (ocispec.Descriptor, error) {
+
+	manifest := ocispec.Manifest{
+		Versioned:   specs.Versioned{SchemaVersion: 2},
+		Config:      configDescriptor,
+		Layers:      layers,
+		Annotations: ociAnnotations,
+	}
+
+	manifestData, err := json.Marshal(manifest)
+	if err != nil {
+		return ocispec.Descriptor{}, err
+	}
+
+	return oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest,
+		manifestData, parsedRef.String())
+}


Test patch:
diff --git a/pkg/chart/v2/util/dependencies_test.go b/pkg/chart/v2/util/dependencies_test.go
index 5947eac69..07b2441e2 100644
--- a/pkg/chart/v2/util/dependencies_test.go
+++ b/pkg/chart/v2/util/dependencies_test.go
@@ -15,6 +15,7 @@ limitations under the License.
 package util
 
 import (
+	"encoding/json"
 	"os"
 	"path/filepath"
 	"sort"
@@ -237,6 +238,20 @@ func TestProcessDependencyImportValues(t *testing.T) {
 			if b := strconv.FormatBool(pv); b != vv {
 				t.Errorf("failed to match imported bool value %v with expected %v for key %q", b, vv, kk)
 			}
+		case json.Number:
+			if fv, err := pv.Float64(); err == nil {
+				if sfv := strconv.FormatFloat(fv, 'f', -1, 64); sfv != vv {
+					t.Errorf("failed to match imported float value %v with expected %v for key %q", sfv, vv, kk)
+				}
+			}
+			if iv, err := pv.Int64(); err == nil {
+				if siv := strconv.FormatInt(iv, 10); siv != vv {
+					t.Errorf("failed to match imported int value %v with expected %v for key %q", siv, vv, kk)
+				}
+			}
+			if pv.String() != vv {
+				t.Errorf("failed to match imported string value %q with expected %q for key %q", pv, vv, kk)
+			}
 		default:
 			if pv != vv {
 				t.Errorf("failed to match imported string value %q with expected %q for key %q", pv, vv, kk)
@@ -341,6 +356,10 @@ func TestProcessDependencyImportValuesMultiLevelPrecedence(t *testing.T) {
 			if s := strconv.FormatFloat(pv, 'f', -1, 64); s != vv {
 				t.Errorf("failed to match imported float value %v with expected %v", s, vv)
 			}
+		case json.Number:
+			if pv.String() != vv {
+				t.Errorf("failed to match imported string value %q with expected %q", pv, vv)
+			}
 		default:
 			if pv != vv {
 				t.Errorf("failed to match imported string value %q with expected %q", pv, vv)
diff --git a/pkg/cmd/template_test.go b/pkg/cmd/template_test.go
index a6c848e08..c478fced4 100644
--- a/pkg/cmd/template_test.go
+++ b/pkg/cmd/template_test.go
@@ -22,6 +22,18 @@ import (
 	"testing"
 )
 
+func TestTemplateCmdWithToml(t *testing.T) {
+
+	tests := []cmdTestCase{
+		{
+			name:   "check toToml function rendering",
+			cmd:    fmt.Sprintf("template '%s'", "testdata/testcharts/issue-totoml"),
+			golden: "output/issue-totoml.txt",
+		},
+	}
+	runTestCmd(t, tests)
+}
+
 var chartPath = "testdata/testcharts/subchart"
 
 func TestTemplateCmd(t *testing.T) {
diff --git a/pkg/cmd/testdata/output/issue-totoml.txt b/pkg/cmd/testdata/output/issue-totoml.txt
new file mode 100644
index 000000000..06cf4bb8d
--- /dev/null
+++ b/pkg/cmd/testdata/output/issue-totoml.txt
@@ -0,0 +1,8 @@
+---
+# Source: issue-totoml/templates/configmap.yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: issue-totoml
+data: |
+  key = 13
diff --git a/pkg/cmd/testdata/testcharts/issue-totoml/Chart.yaml b/pkg/cmd/testdata/testcharts/issue-totoml/Chart.yaml
new file mode 100644
index 000000000..f4be7a213
--- /dev/null
+++ b/pkg/cmd/testdata/testcharts/issue-totoml/Chart.yaml
@@ -0,0 +1,3 @@
+apiVersion: v2
+name: issue-totoml
+version: 0.1.0
diff --git a/pkg/cmd/testdata/testcharts/issue-totoml/templates/configmap.yaml b/pkg/cmd/testdata/testcharts/issue-totoml/templates/configmap.yaml
new file mode 100644
index 000000000..621e70d48
--- /dev/null
+++ b/pkg/cmd/testdata/testcharts/issue-totoml/templates/configmap.yaml
@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: issue-totoml
+data: |
+  {{ .Values.global | toToml }}
diff --git a/pkg/cmd/testdata/testcharts/issue-totoml/values.yaml b/pkg/cmd/testdata/testcharts/issue-totoml/values.yaml
new file mode 100644
index 000000000..dd0140449
--- /dev/null
+++ b/pkg/cmd/testdata/testcharts/issue-totoml/values.yaml
@@ -0,0 +1,2 @@
+global:
+  key: 13
\ No newline at end of file
diff --git a/pkg/registry/client_test.go b/pkg/registry/client_test.go
new file mode 100644
index 000000000..8fc392336
--- /dev/null
+++ b/pkg/registry/client_test.go
@@ -0,0 +1,54 @@
+/*
+Copyright The Helm Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package registry
+
+import (
+	"context"
+	"io"
+	"testing"
+
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/stretchr/testify/require"
+	"oras.land/oras-go/v2/content/memory"
+)
+
+// Inspired by oras test
+// https://github.com/oras-project/oras-go/blob/05a2b09cbf2eab1df691411884dc4df741ec56ab/content_test.go#L1802
+func TestTagManifestTransformsReferences(t *testing.T) {
+	memStore := memory.New()
+	client := &Client{out: io.Discard}
+	ctx := context.Background()
+
+	refWithPlus := "test-registry.io/charts/test:1.0.0+metadata"
+	expectedRef := "test-registry.io/charts/test:1.0.0_metadata" // + becomes _
+
+	configDesc := ocispec.Descriptor{MediaType: ConfigMediaType, Digest: "sha256:config", Size: 100}
+	layers := []ocispec.Descriptor{{MediaType: ChartLayerMediaType, Digest: "sha256:layer", Size: 200}}
+
+	parsedRef, err := newReference(refWithPlus)
+	require.NoError(t, err)
+
+	desc, err := client.tagManifest(ctx, memStore, configDesc, layers, nil, parsedRef)
+	require.NoError(t, err)
+
+	transformedDesc, err := memStore.Resolve(ctx, expectedRef)
+	require.NoError(t, err, "Should find the reference with _ instead of +")
+	require.Equal(t, desc.Digest, transformedDesc.Digest)
+
+	_, err = memStore.Resolve(ctx, refWithPlus)
+	require.Error(t, err, "Should NOT find the reference with the original +")
+}