From 9e9999b6714b1dc53ced5140815de387aa85bd21 Mon Sep 17 00:00:00 2001 From: Hang Park Date: Mon, 14 Oct 2019 19:29:06 +0900 Subject: [PATCH 1/2] fix(pkg/downloader): Add failing tests for #6416 and bugs due to #5874 This commit includes failing tests for a bug reported by #6416 and several bugs due to #5874. `helm dependency build` command fails if one of subcharts has optional dependency fields (e.g. Alias / Condition / Tags) or SemVer ranges. Signed-off-by: Hang Park --- pkg/downloader/manager_test.go | 107 ++++++++++++++++++ .../testdata/local-subchart-0.1.0.tgz | Bin 0 -> 259 bytes 2 files changed, 107 insertions(+) create mode 100644 pkg/downloader/testdata/local-subchart-0.1.0.tgz 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 0000000000000000000000000000000000000000..4853121056a718bf02392623cf46a826f7908752 GIT binary patch literal 259 zcmV+e0sQ_SiwG0|00000|0w_~VMtOiV@ORlOnEsqVl!4SWK%V1T2nbTPgYhoO;>Dc zVQyr3R8em|NM&qo0PNJuio!4y2H>vq6nTN^{F#I Date: Tue, 22 Oct 2019 12:38:49 +0900 Subject: [PATCH 2/2] fix(pkg/downloader): Calculate digest from both Chart.lock & Chart.yaml deps To make digests include information about Chart.yaml dependencies, not only the lock file, digest calculation is changed to accept both contents. This terminates the `dep build` command if Chart.yaml dependencies have been updated so that `dep up` should be executed properly, to prevent downloading wrong versions or mismatched subcharts. Note that previous Helm cannot know whether Chart.yaml dependencies were changed or not since the Chart.lock's digest is calculated by only Chart.lock contents, which don't include information about SemVer ranges and extra dependency fields such as aliases, conditions, and tags. Specially, SemVer can be written as a version range in Chart.yaml, but Chart.lock has the specific, resolved version of that range. Signed-off-by: Hang Park --- internal/resolver/resolver.go | 6 +-- internal/resolver/resolver_test.go | 75 +++++++++++++++++++++++------- pkg/downloader/manager.go | 2 +- 3 files changed, 61 insertions(+), 22 deletions(-) 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") }