Repository: helm/helm
Pull request #30887 (2025-05-21 00:39:41)

Fix 3.18.0 regression: registry login with scheme
fixes https://github.com/helm/helm/issues/30873

**What this PR does / why we need it**:

Helm 3.18.0 released an upgrade of ORAS from v1 to v2.

ORAS v2 correctly does not accept http/https scheme for registry login, while ORAS v1 previously did. Even if v1 should not have, we want to preserve backwards compatibility for Helm 3 users who pass the scheme.

This will be removed in Helm 4, where registry login will not accept http/https scheme.

**Special notes for your reviewer:**

edit: @sabre1041 tracked down that here is where the URL scheme gets stripped in older Helm versions: https://github.com/moby/moby/blob/v25.0.6/registry/config.go#L203-L205

**If applicable**:
- [ ] this PR contains user facing changes (the `docs needed` label should be applied if so)
- [ ] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility


TerryHowe: Thanks, looks good other than lint issue
scottrigby: > URL parse considers both `<protocol>:` and `<protocol>://` to be schemes. As a result, uniformly removing the scheme will result in undesirable results.

Ah, this is what was throwing the tests earlier when I initially added to [registry.Client.Login]. Nice sleuthing!

> would recommend that the logic be lower down in the call to capture additional use cases where the `action` package is not used.
> 
> Proposed enhancement would be to include the logic in the `login` method of the `client.go` in the `registry` package and align closer to the prior functionality.

> Tests appear to pass with this type of logic

Yep, added back with the stripping change and all is well! Ready for re-review

Patch:
diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go
index a68a05aa9..622dac4b8 100644
--- a/pkg/chart/loader/load.go
+++ b/pkg/chart/loader/load.go
@@ -18,6 +18,7 @@ package loader
 
 import (
 	"bytes"
+	"encoding/json"
 	"log"
 	"os"
 	"path/filepath"
@@ -104,7 +105,10 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) {
 			}
 		case f.Name == "values.yaml":
 			c.Values = make(map[string]interface{})
-			if err := yaml.Unmarshal(f.Data, &c.Values); err != nil {
+			if err := yaml.Unmarshal(f.Data, &c.Values, func(d *json.Decoder) *json.Decoder {
+				d.UseNumber()
+				return d
+			}); err != nil {
 				return c, errors.Wrap(err, "cannot load values.yaml")
 			}
 		case f.Name == "values.schema.json":
diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go
index 61c633a6d..566951ec5 100644
--- a/pkg/chartutil/values.go
+++ b/pkg/chartutil/values.go
@@ -17,6 +17,7 @@ limitations under the License.
 package chartutil
 
 import (
+	"encoding/json"
 	"fmt"
 	"io"
 	"os"
@@ -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/plugin/plugin.go b/pkg/plugin/plugin.go
index e2fb78672..8eac1f74d 100644
--- a/pkg/plugin/plugin.go
+++ b/pkg/plugin/plugin.go
@@ -84,7 +84,7 @@ type Metadata struct {
 	PlatformCommand []PlatformCommand `json:"platformCommand"`
 
 	// Command is the plugin command, as a single string.
-	// Providing a command will result in an deprecation warning if PlatformCommand is also set.
+	// Providing a command will result in an error if PlatformCommand is also set.
 	//
 	// The command will be passed through environment expansion, so env vars can
 	// be present in this command. Unless IgnoreFlags is set, this will
@@ -266,11 +266,11 @@ func validatePluginData(plug *Plugin, filepath string) error {
 	plug.Metadata.Usage = sanitizeString(plug.Metadata.Usage)
 
 	if len(plug.Metadata.PlatformCommand) > 0 && len(plug.Metadata.Command) > 0 {
-		fmt.Printf("WARNING: both 'platformCommand' and 'command' are set in %q (this will become an error in a future Helm version)\n", filepath)
+		return fmt.Errorf("both platformCommand and command are set in %q", filepath)
 	}
 
 	if len(plug.Metadata.PlatformHooks) > 0 && len(plug.Metadata.Hooks) > 0 {
-		fmt.Printf("WARNING: both 'platformHooks' and 'hooks' are set in %q (this will become an error in a future Helm version)\n", filepath)
+		return fmt.Errorf("both platformHooks and hooks are set in %q", filepath)
 	}
 
 	// We could also validate SemVer, executable, and other fields should we so choose.
diff --git a/pkg/registry/client.go b/pkg/registry/client.go
index de7e636e1..2c85cd3b6 100644
--- a/pkg/registry/client.go
+++ b/pkg/registry/client.go
@@ -229,8 +229,39 @@ type (
 	}
 )
 
+// Deprecated: will be removed in Helm 4
+// Added for backwards compatibility for Helm < 3.18.0 after moving to ORAS v2
+// ref: https://github.com/helm/helm/issues/30873
+// TODO: document that Helm 4 `registry login` does accept full URLs
+func (c *Client) stripURL(host string) string {
+	// strip scheme from host in URL
+	for _, s := range []string{"oci://", "http://", "https://"} {
+		if strings.HasPrefix(host, s) {
+			plain := strings.TrimPrefix(host, s)
+			if c.debug {
+				fmt.Fprintf(c.out, "[WARNING] Invalid registry passed: registries should NOT be prefixed with a URL scheme. Use %q instead\n", plain)
+			}
+			host = plain
+			break
+		}
+	}
+	// strip repo from registry in URL
+	if idx := strings.Index(host, "/"); idx != -1 {
+		host = host[:idx]
+		if c.debug {
+			fmt.Fprintf(c.out, "[WARNING] Invalid registry passed: registries should NOT include a repository. Use %q instead\n", host)
+		}
+		return host
+	}
+
+	return host
+}
+
 // Login logs into a registry
 func (c *Client) Login(host string, options ...LoginOption) error {
+	// This is the lowest available point to strip incorrect URL parts
+	host = c.stripURL(host)
+
 	for _, option := range options {
 		option(&loginOperation{host, c})
 	}


Test patch:
diff --git a/cmd/helm/template_test.go b/cmd/helm/template_test.go
index e5b939879..d13f96057 100644
--- a/cmd/helm/template_test.go
+++ b/cmd/helm/template_test.go
@@ -161,6 +161,11 @@ func TestTemplateCmd(t *testing.T) {
 			cmd:    fmt.Sprintf("template '%s' -f %s/extra_values.yaml", chartPath, chartPath),
 			golden: "output/template-subchart-cm-set-file.txt",
 		},
+		{
+			name:   "check toToml function rendering",
+			cmd:    fmt.Sprintf("template '%s'", "testdata/testcharts/issue-totoml"),
+			golden: "output/issue-totoml.txt",
+		},
 	}
 	runTestCmd(t, tests)
 }
diff --git a/cmd/helm/testdata/output/issue-totoml.txt b/cmd/helm/testdata/output/issue-totoml.txt
new file mode 100644
index 000000000..06cf4bb8d
--- /dev/null
+++ b/cmd/helm/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/cmd/helm/testdata/testcharts/issue-totoml/Chart.yaml b/cmd/helm/testdata/testcharts/issue-totoml/Chart.yaml
new file mode 100644
index 000000000..2815d0d21
--- /dev/null
+++ b/cmd/helm/testdata/testcharts/issue-totoml/Chart.yaml
@@ -0,0 +1,3 @@
+apiVersion: v2
+name: issue-totoml
+version: 0.1.0
\ No newline at end of file
diff --git a/cmd/helm/testdata/testcharts/issue-totoml/templates/configmap.yaml b/cmd/helm/testdata/testcharts/issue-totoml/templates/configmap.yaml
new file mode 100644
index 000000000..2db705b63
--- /dev/null
+++ b/cmd/helm/testdata/testcharts/issue-totoml/templates/configmap.yaml
@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: issue-totoml
+data: |
+  {{ .Values.global | toToml }}
\ No newline at end of file
diff --git a/cmd/helm/testdata/testcharts/issue-totoml/values.yaml b/cmd/helm/testdata/testcharts/issue-totoml/values.yaml
new file mode 100644
index 000000000..dd0140449
--- /dev/null
+++ b/cmd/helm/testdata/testcharts/issue-totoml/values.yaml
@@ -0,0 +1,2 @@
+global:
+  key: 13
\ No newline at end of file
diff --git a/pkg/chartutil/dependencies_test.go b/pkg/chartutil/dependencies_test.go
index 7e035be5f..bf8b1a9b5 100644
--- a/pkg/chartutil/dependencies_test.go
+++ b/pkg/chartutil/dependencies_test.go
@@ -15,6 +15,7 @@ limitations under the License.
 package chartutil
 
 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)
@@ -309,6 +324,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/plugin/plugin_test.go b/pkg/plugin/plugin_test.go
index 7447bdb50..770d701fa 100644
--- a/pkg/plugin/plugin_test.go
+++ b/pkg/plugin/plugin_test.go
@@ -496,8 +496,8 @@ func TestValidatePluginData(t *testing.T) {
 		{false, mockMissingMeta},         // Test if the metadata section missing
 		{true, mockNoCommand},            // Test no command metadata works
 		{true, mockLegacyCommand},        // Test legacy command metadata works
-		{true, mockWithCommand},          // Test platformCommand and command both set pass until v4
-		{true, mockWithHooks},            // Test platformHooks and hooks both set pass until v4
+		{false, mockWithCommand},         // Test platformCommand and command both set fails
+		{false, mockWithHooks},           // Test platformHooks and hooks both set fails
 	} {
 		err := validatePluginData(item.plug, fmt.Sprintf("test-%d", i))
 		if item.pass && err != nil {
diff --git a/pkg/registry/client_test.go b/pkg/registry/client_test.go
index 4c5a78849..dcc54c0c4 100644
--- a/pkg/registry/client_test.go
+++ b/pkg/registry/client_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package registry
 
 import (
+	"io"
 	"testing"
 
 	"github.com/containerd/containerd/remotes"
@@ -31,3 +32,23 @@ func TestNewClientResolverNotSupported(t *testing.T) {
 	require.Equal(t, err, errDeprecatedRemote)
 	assert.Nil(t, client)
 }
+
+func TestStripURL(t *testing.T) {
+	client := &Client{
+		out: io.Discard,
+	}
+	// no change with supported host formats
+	assert.Equal(t, "username@localhost:8000", client.stripURL("username@localhost:8000"))
+	assert.Equal(t, "localhost:8000", client.stripURL("localhost:8000"))
+	assert.Equal(t, "docker.pkg.dev", client.stripURL("docker.pkg.dev"))
+
+	// test strip scheme from host in URL
+	assert.Equal(t, "docker.pkg.dev", client.stripURL("oci://docker.pkg.dev"))
+	assert.Equal(t, "docker.pkg.dev", client.stripURL("http://docker.pkg.dev"))
+	assert.Equal(t, "docker.pkg.dev", client.stripURL("https://docker.pkg.dev"))
+
+	// test strip repo from Registry in URL
+	assert.Equal(t, "127.0.0.1:15000", client.stripURL("127.0.0.1:15000/asdf"))
+	assert.Equal(t, "127.0.0.1:15000", client.stripURL("127.0.0.1:15000/asdf/asdf"))
+	assert.Equal(t, "127.0.0.1:15000", client.stripURL("127.0.0.1:15000"))
+}