Merge pull request #31491 from benoittgt/31490-plugin-name-helper

feat: improve plugin name validation error messages and field name detection (v1)

internal/plugin/loader.go

@@ -47,6 +47,7 @@ func loadMetadataLegacy(metadataData []byte) (*Metadata, error) {
 
 	var ml MetadataLegacy
 	d := yaml.NewDecoder(bytes.NewReader(metadataData))
+	// NOTE: No strict unmarshalling for legacy plugins - maintain backwards compatibility
 	if err := d.Decode(&ml); err != nil {
 		return nil, err
 	}
@@ -66,6 +67,7 @@ func loadMetadataV1(metadataData []byte) (*Metadata, error) {
 
 	var mv1 MetadataV1
 	d := yaml.NewDecoder(bytes.NewReader(metadataData))
+	d.KnownFields(true)
 	if err := d.Decode(&mv1); err != nil {
 		return nil, err
 	}

internal/plugin/loader_test.go

@@ -268,3 +268,96 @@ func TestFindPlugins(t *testing.T) {
 		})
 	}
 }
+
+func TestLoadMetadataLegacy(t *testing.T) {
+	testCases := map[string]struct {
+		yaml          string
+		expectError   bool
+		errorContains string
+		expectedName  string
+		logNote       string
+	}{
+		"capital name field": {
+			yaml: `Name: my-plugin
+version: 1.0.0
+usage: test plugin
+description: test description
+command: echo test`,
+			expectError:   true,
+			errorContains: `invalid plugin name "": must contain only a-z, A-Z, 0-9, _ and -`,
+			// Legacy plugins: No strict unmarshalling (backwards compatibility)
+			// YAML decoder silently ignores "Name:", then validation catches empty name
+			logNote: "NOTE: V1 plugins use strict unmarshalling and would get: yaml: field Name not found",
+		},
+		"correct name field": {
+			yaml: `name: my-plugin
+version: 1.0.0
+usage: test plugin
+description: test description
+command: echo test`,
+			expectError:  false,
+			expectedName: "my-plugin",
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			m, err := loadMetadataLegacy([]byte(tc.yaml))
+
+			if tc.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tc.errorContains)
+				t.Logf("Legacy error (validation catches empty name): %v", err)
+				if tc.logNote != "" {
+					t.Log(tc.logNote)
+				}
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tc.expectedName, m.Name)
+			}
+		})
+	}
+}
+
+func TestLoadMetadataV1(t *testing.T) {
+	testCases := map[string]struct {
+		yaml          string
+		expectError   bool
+		errorContains string
+		expectedName  string
+	}{
+		"capital name field": {
+			yaml: `apiVersion: v1
+Name: my-plugin
+type: cli/v1
+runtime: subprocess
+`,
+			expectError:   true,
+			errorContains: "field Name not found in type plugin.MetadataV1",
+		},
+		"correct name field": {
+			yaml: `apiVersion: v1
+name: my-plugin
+type: cli/v1
+runtime: subprocess
+`,
+			expectError:  false,
+			expectedName: "my-plugin",
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			m, err := loadMetadataV1([]byte(tc.yaml))
+
+			if tc.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tc.errorContains)
+				t.Logf("V1 error (strict unmarshalling): %v", err)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tc.expectedName, m.Name)
+			}
+		})
+	}
+}

internal/plugin/metadata.go

@@ -54,7 +54,7 @@ func (m Metadata) Validate() error {
 	var errs []error
 
 	if !validPluginName.MatchString(m.Name) {
-		errs = append(errs, fmt.Errorf("invalid name"))
+		errs = append(errs, fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name))
 	}
 
 	if m.APIVersion == "" {

internal/plugin/metadata_legacy.go

@@ -69,7 +69,7 @@ type MetadataLegacy struct {
 
 func (m *MetadataLegacy) Validate() error {
 	if !validPluginName.MatchString(m.Name) {
-		return fmt.Errorf("invalid plugin name")
+		return fmt.Errorf("invalid plugin name %q: must contain only a-z, A-Z, 0-9, _ and -", m.Name)
 	}
 	m.Usage = sanitizeString(m.Usage)
 

internal/plugin/metadata_legacy_test.go

@@ -0,0 +1,126 @@
+/*
+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 plugin
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMetadataLegacyValidate(t *testing.T) {
+	testsValid := map[string]MetadataLegacy{
+		"valid metadata": {
+			Name: "myplugin",
+		},
+		"valid with command": {
+			Name:    "myplugin",
+			Command: "echo hello",
+		},
+		"valid with platformCommand": {
+			Name: "myplugin",
+			PlatformCommand: []PlatformCommand{
+				{OperatingSystem: "linux", Architecture: "amd64", Command: "echo hello"},
+			},
+		},
+		"valid with hooks": {
+			Name: "myplugin",
+			Hooks: Hooks{
+				"install": "echo install",
+			},
+		},
+		"valid with platformHooks": {
+			Name: "myplugin",
+			PlatformHooks: PlatformHooks{
+				"install": []PlatformCommand{
+					{OperatingSystem: "linux", Architecture: "amd64", Command: "echo install"},
+				},
+			},
+		},
+		"valid with downloaders": {
+			Name: "myplugin",
+			Downloaders: []Downloaders{
+				{
+					Protocols: []string{"myproto"},
+					Command:   "echo download",
+				},
+			},
+		},
+	}
+
+	for testName, metadata := range testsValid {
+		t.Run(testName, func(t *testing.T) {
+			assert.NoError(t, metadata.Validate())
+		})
+	}
+
+	testsInvalid := map[string]MetadataLegacy{
+		"invalid name": {
+			Name: "my plugin", // further tested in TestValidPluginName
+		},
+		"both command and platformCommand": {
+			Name:    "myplugin",
+			Command: "echo hello",
+			PlatformCommand: []PlatformCommand{
+				{OperatingSystem: "linux", Architecture: "amd64", Command: "echo hello"},
+			},
+		},
+		"both hooks and platformHooks": {
+			Name: "myplugin",
+			Hooks: Hooks{
+				"install": "echo install",
+			},
+			PlatformHooks: PlatformHooks{
+				"install": []PlatformCommand{
+					{OperatingSystem: "linux", Architecture: "amd64", Command: "echo install"},
+				},
+			},
+		},
+		"downloader with empty command": {
+			Name: "myplugin",
+			Downloaders: []Downloaders{
+				{
+					Protocols: []string{"myproto"},
+					Command:   "",
+				},
+			},
+		},
+		"downloader with no protocols": {
+			Name: "myplugin",
+			Downloaders: []Downloaders{
+				{
+					Protocols: []string{},
+					Command:   "echo download",
+				},
+			},
+		},
+		"downloader with empty protocol": {
+			Name: "myplugin",
+			Downloaders: []Downloaders{
+				{
+					Protocols: []string{""},
+					Command:   "echo download",
+				},
+			},
+		},
+	}
+
+	for testName, metadata := range testsInvalid {
+		t.Run(testName, func(t *testing.T) {
+			assert.Error(t, metadata.Validate())
+		})
+	}
+}

internal/plugin/metadata_test.go

@@ -53,10 +53,10 @@ func TestValidatePluginData(t *testing.T) {
 	}{
 		{true, mockSubprocessCLIPlugin(t, "abcdefghijklmnopqrstuvwxyz0123456789_-ABC"), ""},
 		{true, mockSubprocessCLIPlugin(t, "foo-bar-FOO-BAR_1234"), ""},
-		{false, mockSubprocessCLIPlugin(t, "foo -bar"), "invalid name"},
-		{false, mockSubprocessCLIPlugin(t, "$foo -bar"), "invalid name"}, // Test leading chars
-		{false, mockSubprocessCLIPlugin(t, "foo -bar "), "invalid name"}, // Test trailing chars
-		{false, mockSubprocessCLIPlugin(t, "foo\nbar"), "invalid name"},  // Test newline
+		{false, mockSubprocessCLIPlugin(t, "foo -bar"), "invalid plugin name"},
+		{false, mockSubprocessCLIPlugin(t, "$foo -bar"), "invalid plugin name"}, // Test leading chars
+		{false, mockSubprocessCLIPlugin(t, "foo -bar "), "invalid plugin name"}, // Test trailing chars
+		{false, mockSubprocessCLIPlugin(t, "foo\nbar"), "invalid plugin name"},  // Test newline
 		{true, mockNoCommand, ""},     // Test no command metadata works
 		{true, mockLegacyCommand, ""}, // Test legacy command metadata works
 	} {
@@ -66,8 +66,8 @@ func TestValidatePluginData(t *testing.T) {
 		} else if !item.pass && err == nil {
 			t.Errorf("expected case %d to fail", i)
 		}
-		if !item.pass && err.Error() != item.errString {
-			t.Errorf("index [%d]: expected the following error: %s, but got: %s", i, item.errString, err.Error())
+		if !item.pass && !strings.Contains(err.Error(), item.errString) {
+			t.Errorf("index [%d]: expected error to contain: %s, but got: %s", i, item.errString, err.Error())
 		}
 	}
 }
@@ -92,7 +92,7 @@ func TestMetadataValidateMultipleErrors(t *testing.T) {
 
 	// Check that all expected errors are present in the joined error
 	expectedErrors := []string{
-		"invalid name",
+		"invalid plugin name",
 		"empty APIVersion",
 		"empty type field",
 		"empty runtime field",

internal/plugin/plugin_test.go

@@ -21,6 +21,44 @@ import (
 	"helm.sh/helm/v4/internal/plugin/schema"
 )
 
+func TestValidPluginName(t *testing.T) {
+	validNames := map[string]string{
+		"lowercase":       "myplugin",
+		"uppercase":       "MYPLUGIN",
+		"mixed case":      "MyPlugin",
+		"with digits":     "plugin123",
+		"with hyphen":     "my-plugin",
+		"with underscore": "my_plugin",
+		"mixed chars":     "my-awesome_plugin_123",
+	}
+
+	for name, pluginName := range validNames {
+		t.Run("valid/"+name, func(t *testing.T) {
+			if !validPluginName.MatchString(pluginName) {
+				t.Errorf("expected %q to match validPluginName regex", pluginName)
+			}
+		})
+	}
+
+	invalidNames := map[string]string{
+		"empty":   "",
+		"space":   "my plugin",
+		"colon":   "plugin:",
+		"period":  "my.plugin",
+		"slash":   "my/plugin",
+		"dollar":  "$plugin",
+		"unicode": "plügîn",
+	}
+
+	for name, pluginName := range invalidNames {
+		t.Run("invalid/"+name, func(t *testing.T) {
+			if validPluginName.MatchString(pluginName) {
+				t.Errorf("expected %q to not match validPluginName regex", pluginName)
+			}
+		})
+	}
+}
+
 func mockSubprocessCLIPlugin(t *testing.T, pluginName string) *SubprocessPluginRuntime {
 	t.Helper()