diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index 20b62dc66..e4dbab227 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -132,7 +132,7 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string return nil, errors.Errorf("can't get a valid version for repositories %s. Try changing the version constraint in Chart.yaml", strings.Join(missing, ", ")) } - digest, err := HashReq(locked) + digest, err := HashReq(reqs, locked) if err != nil { return nil, err } @@ -148,8 +148,8 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string // // This should be used only to compare against another hash generated by this // function. -func HashReq(req []*chart.Dependency) (string, error) { - data, err := json.Marshal(req) +func HashReq(req, lock []*chart.Dependency) (string, error) { + data, err := json.Marshal([2][]*chart.Dependency{req, lock}) if err != nil { return "", err } diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 3af62b811..3828771cc 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -130,7 +130,7 @@ func TestResolve(t *testing.T) { t.Fatalf("Expected error in test %q", tt.name) } - if h, err := HashReq(tt.expect.Dependencies); err != nil { + if h, err := HashReq(tt.req, tt.expect.Dependencies); err != nil { t.Fatal(err) } else if h != l.Digest { t.Errorf("%q: hashes don't match.", tt.name) @@ -156,24 +156,63 @@ func TestResolve(t *testing.T) { } func TestHashReq(t *testing.T) { - expect := "sha256:d661820b01ed7bcf26eed8f01cf16380e0a76326ba33058d3150f919d9b15bc0" - req := []*chart.Dependency{ - {Name: "alpine", Version: "0.1.0", Repository: "http://localhost:8879/charts"}, - } - h, err := HashReq(req) - if err != nil { - t.Fatal(err) - } - if expect != h { - t.Errorf("Expected %q, got %q", expect, h) - } + expect := "sha256:fb239e836325c5fa14b29d1540a13b7d3ba13151b67fe719f820e0ef6d66aaaf" - req = []*chart.Dependency{} - h, err = HashReq(req) - if err != nil { - t.Fatal(err) + tests := []struct { + name string + chartVersion string + lockVersion string + wantError bool + }{ + { + name: "chart with the expected digest", + chartVersion: "0.1.0", + lockVersion: "0.1.0", + wantError: false, + }, + { + name: "ranged version but same resolved lock version", + chartVersion: "^0.1.0", + lockVersion: "0.1.0", + wantError: true, + }, + { + name: "ranged version resolved as higher version", + chartVersion: "^0.1.0", + lockVersion: "0.1.2", + wantError: true, + }, + { + name: "different version", + chartVersion: "0.1.2", + lockVersion: "0.1.2", + wantError: true, + }, + { + name: "different version with a range", + chartVersion: "^0.1.2", + lockVersion: "0.1.2", + wantError: true, + }, } - if expect == h { - t.Errorf("Expected %q != %q", expect, h) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := []*chart.Dependency{ + {Name: "alpine", Version: tt.chartVersion, Repository: "http://localhost:8879/charts"}, + } + lock := []*chart.Dependency{ + {Name: "alpine", Version: tt.lockVersion, Repository: "http://localhost:8879/charts"}, + } + h, err := HashReq(req, lock) + if err != nil { + t.Fatal(err) + } + if !tt.wantError && expect != h { + t.Errorf("Expected %q, got %q", expect, h) + } else if tt.wantError && expect == h { + t.Errorf("Expected not %q, but same", expect) + } + }) } } diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index cc3af74db..ac61ac5ac 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -79,7 +79,7 @@ func (m *Manager) Build() error { } req := c.Metadata.Dependencies - if sum, err := resolver.HashReq(req); err != nil || sum != lock.Digest { + if sum, err := resolver.HashReq(req, lock.Dependencies); err != nil || sum != lock.Digest { return errors.New("Chart.lock is out of sync with Chart.yaml") } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index b21106fea..0c5c08615 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -17,10 +17,14 @@ package downloader import ( "bytes" + "path/filepath" "reflect" "testing" "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chartutil" + "helm.sh/helm/v3/pkg/getter" + "helm.sh/helm/v3/pkg/repo/repotest" ) func TestVersionEquals(t *testing.T) { @@ -176,3 +180,106 @@ func TestGetRepoNames(t *testing.T) { } } } + +// This function is the skeleton test code of failing tests for #6416 and bugs due to #5874. +// This function is used by below tests that ensures success of build operation +// with optional fields, alias, condition, tags, and even with ranged version. +// Parent chart includes local-subchart 0.1.0 subchart from a fake repository, by default. +// If each of these main fields (name, version, repository) is not supplied by dep param, default value will be used. +func checkBuildWithOptionalFields(t *testing.T, chartName string, dep chart.Dependency) { + // Set up a fake repo + srv, err := repotest.NewTempServer("testdata/*.tgz*") + if err != nil { + t.Fatal(err) + } + defer srv.Stop() + if err := srv.LinkIndices(); err != nil { + t.Fatal(err) + } + dir := func(p ...string) string { + return filepath.Join(append([]string{srv.Root()}, p...)...) + } + + // Set main fields if not exist + if dep.Name == "" { + dep.Name = "local-subchart" + } + if dep.Version == "" { + dep.Version = "0.1.0" + } + if dep.Repository == "" { + dep.Repository = srv.URL() + } + + // Save a chart + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: chartName, + Version: "0.1.0", + APIVersion: "v1", + Dependencies: []*chart.Dependency{&dep}, + }, + } + if err := chartutil.SaveDir(c, dir()); err != nil { + t.Fatal(err) + } + + // Set-up a manager + b := bytes.NewBuffer(nil) + g := getter.Providers{getter.Provider{ + Schemes: []string{"http", "https"}, + New: getter.NewHTTPGetter, + }} + m := &Manager{ + ChartPath: dir(chartName), + Out: b, + Getters: g, + RepositoryConfig: dir("repositories.yaml"), + RepositoryCache: dir(), + } + + // First build will update dependencies and create Chart.lock file. + err = m.Build() + if err != nil { + t.Fatal(err) + } + + // Second build should be passed. See PR #6655. + err = m.Build() + if err != nil { + t.Fatal(err) + } +} + +func TestBuild_WithoutOptionalFields(t *testing.T) { + // Dependency has main fields only (name/version/repository) + checkBuildWithOptionalFields(t, "without-optional-fields", chart.Dependency{}) +} + +func TestBuild_WithSemVerRange(t *testing.T) { + // Dependency version is the form of SemVer range + checkBuildWithOptionalFields(t, "with-semver-range", chart.Dependency{ + Version: ">=0.1.0", + }) +} + +func TestBuild_WithAlias(t *testing.T) { + // Dependency has an alias + checkBuildWithOptionalFields(t, "with-alias", chart.Dependency{ + Alias: "local-subchart-alias", + }) +} + +func TestBuild_WithCondition(t *testing.T) { + // Dependency has a condition + checkBuildWithOptionalFields(t, "with-condition", chart.Dependency{ + Condition: "some.condition", + }) +} + +func TestBuild_WithTags(t *testing.T) { + // Dependency has several tags + checkBuildWithOptionalFields(t, "with-tags", chart.Dependency{ + Tags: []string{"tag1", "tag2"}, + }) +} diff --git a/pkg/downloader/testdata/local-subchart-0.1.0.tgz b/pkg/downloader/testdata/local-subchart-0.1.0.tgz new file mode 100644 index 000000000..485312105 Binary files /dev/null and b/pkg/downloader/testdata/local-subchart-0.1.0.tgz differ