Merge eb8fb41837 into a71eb99db4

internal/chart/v3/lint/rules/chartfile.go

@@ -120,6 +120,9 @@ func validateChartName(cf *chart.Metadata) error {
 	if name != cf.Name {
 		return fmt.Errorf("chart name %q is invalid", cf.Name)
 	}
+	if err := chartutil.ValidateChartName(cf.Name); err != nil {
+		return fmt.Errorf("chart name %q is invalid; %w", cf.Name, err)
+	}
 	return nil
 }
 

internal/chart/v3/lint/rules/chartfile_test.go

@@ -29,19 +29,16 @@ import (
 )
 
 const (
-	badChartNameDir    = "testdata/badchartname"
 	badChartDir        = "testdata/badchartfile"
 	anotherBadChartDir = "testdata/anotherbadchartfile"
 )
 
 var (
-	badChartNamePath         = filepath.Join(badChartNameDir, "Chart.yaml")
 	badChartFilePath         = filepath.Join(badChartDir, "Chart.yaml")
 	nonExistingChartFilePath = filepath.Join(os.TempDir(), "Chart.yaml")
 )
 
 var badChart, _ = chartutil.LoadChartfile(badChartFilePath)
-var badChartName, _ = chartutil.LoadChartfile(badChartNamePath)
 
 // Validation functions Test
 func TestValidateChartYamlNotDirectory(t *testing.T) {
@@ -67,14 +64,67 @@ func TestValidateChartYamlFormat(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
-	err := validateChartName(badChart)
-	if err == nil {
-		t.Error("validateChartName to return a linter error, got no error")
+	const chartNameRule = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
+
+	tests := []struct {
+		name    string
+		wantErr string
+	}{
+		{
+			name:    "",
+			wantErr: "name is required",
+		},
+		{
+			name:    "../badchartname",
+			wantErr: `chart name "../badchartname" is invalid`,
+		},
+		{
+			name:    "BadChart",
+			wantErr: `chart name "BadChart" is invalid; ` + chartNameRule,
+		},
+		{
+			name:    "bad.chart",
+			wantErr: `chart name "bad.chart" is invalid`,
+		},
+		{
+			name:    "bad_chart",
+			wantErr: `chart name "bad_chart" is invalid`,
+		},
+		{
+			name:    "-badchart",
+			wantErr: `chart name "-badchart" is invalid`,
+		},
+		{
+			name:    "badchart-",
+			wantErr: `chart name "badchart-" is invalid`,
+		},
+		{
+			name: "badchart",
+		},
+		{
+			name: "bad-chart-1",
+		},
+		{
+			name: "bad--chart",
+		},
 	}
 
-	err = validateChartName(badChartName)
-	if err == nil {
-		t.Error("expected validateChartName to return a linter error for an invalid name, got no error")
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateChartName(&chart.Metadata{Name: tt.name})
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Fatalf("validateChartName(%q) returned unexpected error: %s", tt.name, err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("validateChartName(%q) returned no error, want %q", tt.name, tt.wantErr)
+			}
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Fatalf("validateChartName(%q) returned %q, want to contain %q", tt.name, err, tt.wantErr)
+			}
+		})
 	}
 }
 

internal/chart/v3/util/create.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -32,10 +33,9 @@ import (
 )
 
 // chartName is a regular expression for testing the supplied name of a chart.
-// This regular expression is probably stricter than it needs to be. We can relax it
-// somewhat. Newline characters, as well as $, quotes, +, parens, and % are known to be
-// problematic.
-var chartName = regexp.MustCompile("^[a-zA-Z0-9._-]+$")
+var chartName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
+
+const validChartNameMessage = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
 
 const (
 	// ChartfileName is the default Chart file name.
@@ -650,6 +650,10 @@ var Stderr io.Writer = os.Stderr
 
 // CreateFrom creates a new chart, but scaffolds it from the src chart.
 func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
+	if err := ValidateChartName(chartfile.Name); err != nil {
+		return err
+	}
+
 	schart, err := loader.Load(src)
 	if err != nil {
 		return fmt.Errorf("could not load %s: %w", src, err)
@@ -703,7 +707,7 @@ func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
 // new chart directory.
 func Create(name, dir string) (string, error) {
 	// Sanity-check the name of a chart so user doesn't create one that causes problems.
-	if err := validateChartName(name); err != nil {
+	if err := ValidateChartName(name); err != nil {
 		return "", err
 	}
 
@@ -822,12 +826,13 @@ func writeFile(name string, content []byte) error {
 	return os.WriteFile(name, content, 0o644)
 }
 
-func validateChartName(name string) error {
+// ValidateChartName validates a chart name used by chart creation and linting.
+func ValidateChartName(name string) error {
 	if name == "" || len(name) > maxChartNameLength {
 		return fmt.Errorf("chart name must be between 1 and %d characters", maxChartNameLength)
 	}
 	if !chartName.MatchString(name) {
-		return fmt.Errorf("chart name must match the regular expression %q", chartName.String())
+		return errors.New(validChartNameMessage)
 	}
 	return nil
 }

internal/chart/v3/util/create_test.go

@@ -107,6 +107,13 @@ func TestCreateFrom(t *testing.T) {
 			t.Errorf("File %s contains <CHARTNAME>", f)
 		}
 	}
+
+	cf.Name = "Bad.Chart"
+	if err := CreateFrom(cf, t.TempDir(), srcdir); err == nil {
+		t.Fatal("CreateFrom with invalid chart name returned no error")
+	} else if err.Error() != validChartNameMessage {
+		t.Fatalf("CreateFrom returned %q, want %q", err, validChartNameMessage)
+	}
 }
 
 // TestCreate_Overwrite is a regression test for making sure that files are overwritten.
@@ -145,28 +152,89 @@ func TestCreate_Overwrite(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
-	for name, shouldPass := range map[string]bool{
-		"":                              false,
-		"abcdefghijklmnopqrstuvwxyz-_.": true,
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": true,
-		"$hello":                        false,
-		"Hellô":                         false,
-		"he%%o":                         false,
-		"he\nllo":                       false,
-
-		"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": false,
-	} {
-		if err := validateChartName(name); (err != nil) == shouldPass {
-			t.Errorf("test for %q failed", name)
-		}
+	tests := []struct {
+		name    string
+		wantErr string
+	}{
+		{
+			name:    "",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz",
+		},
+		{
+			name: "bad-chart-1",
+		},
+		{
+			name: "bad--chart",
+		},
+		{
+			name:    "BadChart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad.chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad_chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "-badchart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "badchart-",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "$hello",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "Hellô",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he%%o",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he\nllo",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateChartName(tt.name)
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Fatalf("ValidateChartName(%q) returned unexpected error: %s", tt.name, err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("ValidateChartName(%q) returned no error, want %q", tt.name, tt.wantErr)
+			}
+			if err.Error() != tt.wantErr {
+				t.Fatalf("ValidateChartName(%q) returned %q, want %q", tt.name, err, tt.wantErr)
+			}
+		})
 	}
 }

pkg/chart/v2/lint/rules/chartfile.go

@@ -121,6 +121,9 @@ func validateChartName(cf *chart.Metadata) error {
 	if name != cf.Name {
 		return fmt.Errorf("chart name %q is invalid", cf.Name)
 	}
+	if err := chartutil.ValidateChartName(cf.Name); err != nil {
+		return fmt.Errorf("chart name %q is invalid; %w", cf.Name, err)
+	}
 	return nil
 }
 

pkg/chart/v2/lint/rules/chartfile_test.go

@@ -29,19 +29,16 @@ import (
 )
 
 const (
-	badChartNameDir    = "testdata/badchartname"
 	badChartDir        = "testdata/badchartfile"
 	anotherBadChartDir = "testdata/anotherbadchartfile"
 )
 
 var (
-	badChartNamePath         = filepath.Join(badChartNameDir, "Chart.yaml")
 	badChartFilePath         = filepath.Join(badChartDir, "Chart.yaml")
 	nonExistingChartFilePath = filepath.Join(os.TempDir(), "Chart.yaml")
 )
 
 var badChart, _ = chartutil.LoadChartfile(badChartFilePath)
-var badChartName, _ = chartutil.LoadChartfile(badChartNamePath)
 
 // Validation functions Test
 func TestValidateChartYamlNotDirectory(t *testing.T) {
@@ -67,14 +64,67 @@ func TestValidateChartYamlFormat(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
-	err := validateChartName(badChart)
-	if err == nil {
-		t.Error("validateChartName to return a linter error, got no error")
+	const chartNameRule = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
+
+	tests := []struct {
+		name    string
+		wantErr string
+	}{
+		{
+			name:    "",
+			wantErr: "name is required",
+		},
+		{
+			name:    "../badchartname",
+			wantErr: `chart name "../badchartname" is invalid`,
+		},
+		{
+			name:    "BadChart",
+			wantErr: `chart name "BadChart" is invalid; ` + chartNameRule,
+		},
+		{
+			name:    "bad.chart",
+			wantErr: `chart name "bad.chart" is invalid`,
+		},
+		{
+			name:    "bad_chart",
+			wantErr: `chart name "bad_chart" is invalid`,
+		},
+		{
+			name:    "-badchart",
+			wantErr: `chart name "-badchart" is invalid`,
+		},
+		{
+			name:    "badchart-",
+			wantErr: `chart name "badchart-" is invalid`,
+		},
+		{
+			name: "badchart",
+		},
+		{
+			name: "bad-chart-1",
+		},
+		{
+			name: "bad--chart",
+		},
 	}
 
-	err = validateChartName(badChartName)
-	if err == nil {
-		t.Error("expected validateChartName to return a linter error for an invalid name, got no error")
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateChartName(&chart.Metadata{Name: tt.name})
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Fatalf("validateChartName(%q) returned unexpected error: %s", tt.name, err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("validateChartName(%q) returned no error, want %q", tt.name, tt.wantErr)
+			}
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Fatalf("validateChartName(%q) returned %q, want to contain %q", tt.name, err, tt.wantErr)
+			}
+		})
 	}
 }
 

pkg/chart/v2/util/create.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -32,10 +33,9 @@ import (
 )
 
 // chartName is a regular expression for testing the supplied name of a chart.
-// This regular expression is probably stricter than it needs to be. We can relax it
-// somewhat. Newline characters, as well as $, quotes, +, parens, and % are known to be
-// problematic.
-var chartName = regexp.MustCompile("^[a-zA-Z0-9._-]+$")
+var chartName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
+
+const validChartNameMessage = "chart names must contain lowercase letters, numbers, and dashes, and must start and end with a lowercase letter or number"
 
 const (
 	// ChartfileName is the default Chart file name.
@@ -649,6 +649,10 @@ var Stderr io.Writer = os.Stderr
 
 // CreateFrom creates a new chart, but scaffolds it from the src chart.
 func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
+	if err := ValidateChartName(chartfile.Name); err != nil {
+		return err
+	}
+
 	schart, err := loader.Load(src)
 	if err != nil {
 		return fmt.Errorf("could not load %s: %w", src, err)
@@ -702,7 +706,7 @@ func CreateFrom(chartfile *chart.Metadata, dest, src string) error {
 // new chart directory.
 func Create(name, dir string) (string, error) {
 	// Sanity-check the name of a chart so user doesn't create one that causes problems.
-	if err := validateChartName(name); err != nil {
+	if err := ValidateChartName(name); err != nil {
 		return "", err
 	}
 
@@ -821,12 +825,13 @@ func writeFile(name string, content []byte) error {
 	return os.WriteFile(name, content, 0o644)
 }
 
-func validateChartName(name string) error {
+// ValidateChartName validates a chart name used by chart creation and linting.
+func ValidateChartName(name string) error {
 	if name == "" || len(name) > maxChartNameLength {
 		return fmt.Errorf("chart name must be between 1 and %d characters", maxChartNameLength)
 	}
 	if !chartName.MatchString(name) {
-		return fmt.Errorf("chart name must match the regular expression %q", chartName.String())
+		return errors.New(validChartNameMessage)
 	}
 	return nil
 }

pkg/chart/v2/util/create_test.go

@@ -107,6 +107,13 @@ func TestCreateFrom(t *testing.T) {
 			t.Errorf("File %s contains <CHARTNAME>", f)
 		}
 	}
+
+	cf.Name = "Bad.Chart"
+	if err := CreateFrom(cf, t.TempDir(), srcdir); err == nil {
+		t.Fatal("CreateFrom with invalid chart name returned no error")
+	} else if err.Error() != validChartNameMessage {
+		t.Fatalf("CreateFrom returned %q, want %q", err, validChartNameMessage)
+	}
 }
 
 // TestCreate_Overwrite is a regression test for making sure that files are overwritten.
@@ -145,28 +152,89 @@ func TestCreate_Overwrite(t *testing.T) {
 }
 
 func TestValidateChartName(t *testing.T) {
-	for name, shouldPass := range map[string]bool{
-		"":                              false,
-		"abcdefghijklmnopqrstuvwxyz-_.": true,
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": true,
-		"$hello":                        false,
-		"Hellô":                         false,
-		"he%%o":                         false,
-		"he\nllo":                       false,
-
-		"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"abcdefghijklmnopqrstuvwxyz-_." +
-			"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": false,
-	} {
-		if err := validateChartName(name); (err != nil) == shouldPass {
-			t.Errorf("test for %q failed", name)
-		}
+	tests := []struct {
+		name    string
+		wantErr string
+	}{
+		{
+			name:    "",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz",
+		},
+		{
+			name: "bad-chart-1",
+		},
+		{
+			name: "bad--chart",
+		},
+		{
+			name:    "BadChart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad.chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "bad_chart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "-badchart",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "badchart-",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "$hello",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "Hellô",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he%%o",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name:    "he\nllo",
+			wantErr: validChartNameMessage,
+		},
+		{
+			name: "abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"abcdefghijklmnopqrstuvwxyz-_." +
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.",
+			wantErr: "chart name must be between 1 and 250 characters",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateChartName(tt.name)
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Fatalf("ValidateChartName(%q) returned unexpected error: %s", tt.name, err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("ValidateChartName(%q) returned no error, want %q", tt.name, tt.wantErr)
+			}
+			if err.Error() != tt.wantErr {
+				t.Fatalf("ValidateChartName(%q) returned %q, want %q", tt.name, err, tt.wantErr)
+			}
+		})
 	}
 }