Merge pull request #6736 from hangpark/fix/calculate-digest-from-chart-yaml-n-chart-lock

fix(pkg/downloader): Calculate digest from both Chart.lock & Chart.yaml deps

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
 	}

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)
+			}
+		})
 	}
 }

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")
 	}
 

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"},
+	})
+}