Skip to content

Commit

Permalink
lint and validate dependency metadata to reference dependencies with …
Browse files Browse the repository at this point in the history 
…a unique key (name or alias)

Report charts with the following bad dependency specifications as bad charts:

    dependencies:
    - name: foo
      alias: baz # ← baz used twice
      version: 1.0.0
    - name: bar
      alias: baz # ← baz used twice
      version: 1.0.0

    dependencies:
    - name: foo
      alias: bar # ← shadows chart below
      version: 1.0.0
    - name: bar
      version: 1.0.0

    dependencies:
    - name: foo
      version: 1.0.0
    - name: foo # ← chart with same name as above (although version or repo will be different, this will not work currently)
      version: 1.2.3

Closes #9169

Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>
  • Loading branch information
@dastrobu
dastrobu authored and Daniel Strobusch (FG-41) committed May 23, 2023
1 parent 2398830 commit 6a4035a
Show file tree
Hide file tree
Showing 4 changed files with 166 additions and 3 deletions.
9 changes: 9 additions & 0 deletions pkg/chart/metadata.go
View file
Expand Up @@ -128,10 +128,19 @@ func (md *Metadata) Validate() error {

// Aliases need to be validated here to make sure that the alias name does
// not contain any illegal characters.
dependencies := map[string]*Dependency{}
for _, dependency := range md.Dependencies {
if err := dependency.Validate(); err != nil {
return err
}
key := dependency.Name
if dependency.Alias != "" {
key = dependency.Alias
}
if dependencies[key] != nil {
return ValidationErrorf("more than one dependency with name or alias %q", key)
}
dependencies[key] = dependency
}
return nil
}
Expand Down
78 changes: 75 additions & 3 deletions pkg/chart/metadata_test.go
View file
Expand Up @@ -21,34 +21,42 @@ import (

func TestValidate(t *testing.T) {
tests := []struct {
md *Metadata
err error
name string
md *Metadata
err error
}{
{
"chart without metadata",
nil,
ValidationError("chart.metadata is required"),
},
{
"chart without apiVersion",
&Metadata{Name: "test", Version: "1.0"},
ValidationError("chart.metadata.apiVersion is required"),
},
{
"chart without name",
&Metadata{APIVersion: "v2", Version: "1.0"},
ValidationError("chart.metadata.name is required"),
},
{
"chart without version",
&Metadata{Name: "test", APIVersion: "v2"},
ValidationError("chart.metadata.version is required"),
},
{
"chart with bad type",
&Metadata{Name: "test", APIVersion: "v2", Version: "1.0", Type: "test"},
ValidationError("chart.metadata.type must be application or library"),
},
{
"chart without dependency",
&Metadata{Name: "test", APIVersion: "v2", Version: "1.0", Type: "application"},
nil,
},
{
"dependency with valid alias",
&Metadata{
Name: "test",
APIVersion: "v2",
Expand All @@ -61,6 +69,7 @@ func TestValidate(t *testing.T) {
nil,
},
{
"dependency with bad characters in alias",
&Metadata{
Name: "test",
APIVersion: "v2",
Expand All @@ -73,6 +82,67 @@ func TestValidate(t *testing.T) {
ValidationError("dependency \"bad\" has disallowed characters in the alias"),
},
{
"same dependency twice",
&Metadata{
Name: "test",
APIVersion: "v2",
Version: "1.0",
Type: "application",
Dependencies: []*Dependency{
{Name: "foo", Alias: ""},
{Name: "foo", Alias: ""},
},
},
ValidationError("more than one dependency with name or alias \"foo\""),
},
{
"two dependencies with alias from second dependency shadowing first one",
&Metadata{
Name: "test",
APIVersion: "v2",
Version: "1.0",
Type: "application",
Dependencies: []*Dependency{
{Name: "foo", Alias: ""},
{Name: "bar", Alias: "foo"},
},
},
ValidationError("more than one dependency with name or alias \"foo\""),
},
{
// this case would make sense and could work in future versions of Helm, currently template rendering would
// result in undefined behaviour
"same dependency twice with different version",
&Metadata{
Name: "test",
APIVersion: "v2",
Version: "1.0",
Type: "application",
Dependencies: []*Dependency{
{Name: "foo", Alias: "", Version: "1.2.3"},
{Name: "foo", Alias: "", Version: "1.0.0"},
},
},
ValidationError("more than one dependency with name or alias \"foo\""),
},
{
// this case would make sense and could work in future versions of Helm, currently template rendering would
// result in undefined behaviour
"two dependencies with same name but different repos",
&Metadata{
Name: "test",
APIVersion: "v2",
Version: "1.0",
Type: "application",
Dependencies: []*Dependency{
{Name: "foo", Repository: "repo-0"},
{Name: "foo", Repository: "repo-1"},
},
},
ValidationError("more than one dependency with name or alias \"foo\""),
},
{
"dependencies has nil",
&Metadata{
Name: "test",
APIVersion: "v2",
Expand All @@ -85,6 +155,7 @@ func TestValidate(t *testing.T) {
ValidationError("dependencies must not contain empty or null nodes"),
},
{
"maintainer not empty",
&Metadata{
Name: "test",
APIVersion: "v2",
Expand All @@ -97,6 +168,7 @@ func TestValidate(t *testing.T) {
ValidationError("maintainers must not contain empty or null nodes"),
},
{
"version invalid",
&Metadata{APIVersion: "v2", Name: "test", Version: "1.2.3.4"},
ValidationError("chart.metadata.version \"1.2.3.4\" is invalid"),
},
Expand All @@ -105,7 +177,7 @@ func TestValidate(t *testing.T) {
for _, tt := range tests {
result := tt.md.Validate()
if result != tt.err {
t.Errorf("expected '%s', got '%s'", tt.err, result)
t.Errorf("expected %q, got %q in test %q", tt.err, result, tt.name)
}
}
}
Expand Down
21 changes: 21 additions & 0 deletions pkg/lint/rules/dependencies.go
View file
Expand Up @@ -37,6 +37,7 @@ func Dependencies(linter *support.Linter) {
}

linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependencyInMetadata(c))
linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependenciesUnique(c))
linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyInChartsDir(c))
}

Expand Down Expand Up @@ -80,3 +81,23 @@ func validateDependencyInMetadata(c *chart.Chart) (err error) {
}
return err
}

func validateDependenciesUnique(c *chart.Chart) (err error) {
dependencies := map[string]*chart.Dependency{}
shadowing := []string{}

for _, dep := range c.Metadata.Dependencies {
key := dep.Name
if dep.Alias != "" {
key = dep.Alias
}
if dependencies[key] != nil {
shadowing = append(shadowing, key)
}
dependencies[key] = dep
}
if len(shadowing) > 0 {
err = fmt.Errorf("multiple dependencies with name or alias: %s", strings.Join(shadowing, ","))
}
return err
}
61 changes: 61 additions & 0 deletions pkg/lint/rules/dependencies_test.go
View file
Expand Up @@ -78,6 +78,67 @@ func TestValidateDependencyInMetadata(t *testing.T) {
}
}

func TestValidateDependenciesUnique(t *testing.T) {
tests := []struct {
chart chart.Chart
}{
{chart.Chart{
Metadata: &chart.Metadata{
Name: "badchart",
Version: "0.1.0",
APIVersion: "v2",
Dependencies: []*chart.Dependency{
{
Name: "foo",
},
{
Name: "foo",
},
},
},
}},
{chart.Chart{
Metadata: &chart.Metadata{
Name: "badchart",
Version: "0.1.0",
APIVersion: "v2",
Dependencies: []*chart.Dependency{
{
Name: "foo",
Alias: "bar",
},
{
Name: "bar",
},
},
},
}},
{chart.Chart{
Metadata: &chart.Metadata{
Name: "badchart",
Version: "0.1.0",
APIVersion: "v2",
Dependencies: []*chart.Dependency{
{
Name: "foo",
Alias: "baz",
},
{
Name: "bar",
Alias: "baz",
},
},
},
}},
}

for _, tt := range tests {
if err := validateDependenciesUnique(&tt.chart); err == nil {
t.Errorf("chart should have been flagged for dependency shadowing")
}
}
}

func TestDependencies(t *testing.T) {
tmp := ensure.TempDir(t)
defer os.RemoveAll(tmp)
Expand Down

0 comments on commit 6a4035a

Please sign in to comment.