fix(pkg/downloader): Implement ensureLock() instead of digest comparison

The functionality of checking whether the lock file is out-of-date or not is
quite important to end-users. But digest comparison is not an answer for it.
This commit implements it in an alternative way, which compares names, repos,
and version resolutions in each pair of Chart.yaml and Chart.lock dependencies.

Signed-off-by: Hang Park <hangpark@kaist.ac.kr>

pkg/downloader/manager.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"sort"
 	"strings"
 	"sync"
 
@@ -78,6 +79,11 @@ func (m *Manager) Build() error {
 		return m.Update()
 	}
 
+	// Ensure that the lock file is up-to-date with Chart.yaml dependencies.
+	if err := ensureLock(c.Metadata.Dependencies, lock.Dependencies); err != nil {
+		return err
+	}
+
 	// Check that all of the repos we're dependent on actually exist.
 	if err := m.hasAllRepos(lock.Dependencies); err != nil {
 		return err
@@ -603,6 +609,82 @@ func (m *Manager) loadChartRepositories() (map[string]*repo.ChartRepository, err
 	return indices, nil
 }
 
+// ensureLock compares Chart.yaml and Chart.lock dependencies, and then ensures
+// the lock file is still up-to-date.
+func ensureLock(chartDeps []*chart.Dependency, lockDeps []*chart.Dependency) error {
+	var (
+		vSorted  = make([]*semver.Version, 0, len(lockDeps))
+		vm       = make(map[*semver.Version]*chart.Dependency, len(lockDeps))
+		lm       = make(map[*chart.Dependency]bool, len(lockDeps))
+		cMissing []string
+		lMissing []string
+	)
+
+	// Filter & sort Chart.lock dependencies in desc order.
+	for _, ld := range lockDeps {
+		v, err := semver.NewVersion(ld.Version)
+		if err != nil || ld.Repository == "" {
+			continue
+		}
+		vm[v] = ld
+		vSorted = append(vSorted, v)
+	}
+	sort.Sort(sort.Reverse(semver.Collection(vSorted)))
+
+	lSorted := make([]*chart.Dependency, len(vSorted))
+	for i, v := range vSorted {
+		lSorted[i] = vm[v]
+	}
+
+	// Filter dependencies in Chart.yaml which is not in Chart.lock.
+	for _, cd := range chartDeps {
+		if cd.Repository == "" {
+			// Don't care about local subcharts during building dependencies.
+			continue
+		}
+
+		// Chart.yaml can have SemVer range.
+		constraint, err := semver.NewConstraint(cd.Version)
+		if err != nil {
+			return errors.Wrapf(err, "dependency %q in Chart.yaml has an invalid version/constraint format", cd.Name)
+		}
+
+		found := false
+		for _, ld := range lSorted {
+			if cd.Name != ld.Name || cd.Repository != ld.Repository {
+				continue
+			}
+			ldVer, err := semver.NewVersion(ld.Version)
+			if err != nil {
+				return errors.Wrapf(err, "dependency %q in Chart.lock has an invalid version format", ld.Name)
+			} else if constraint.Check(ldVer) {
+				// Lock dependencies are sorted, so the matched version is the highest version among available candidates.
+				found = true
+				lm[ld] = true
+				break
+			}
+		}
+		if !found {
+			cMissing = append(cMissing, cd.Name)
+		}
+	}
+	if len(cMissing) > 0 {
+		return errors.Errorf("dependencies %s in Chart.yaml don't have a valid version resolution in Chart.lock. Try updating Chart.lock", strings.Join(cMissing, ", "))
+	}
+
+	// All Chart.yaml dependencies are resolved in Chart.lock.
+	// But we should check whether all Chart.lock dependencies are needed by Chart.yaml.
+	for _, ld := range lSorted {
+		if _, ok := lm[ld]; !ok {
+			lMissing = append(lMissing, ld.Name)
+		}
+	}
+	if len(lMissing) > 0 {
+		return errors.Errorf("dependencies %s in Chart.lock are not required by Chart.yaml. Try updating Chart.lock", strings.Join(lMissing, ", "))
+	}
+	return nil
+}
+
 // writeLock writes a lockfile to disk
 func writeLock(chartpath string, lock *chart.Lock) error {
 	data, err := yaml.Marshal(lock)

pkg/downloader/manager_test.go

@@ -17,6 +17,7 @@ package downloader
 
 import (
 	"bytes"
+	"fmt"
 	"path/filepath"
 	"reflect"
 	"testing"
@@ -181,9 +182,172 @@ func TestGetRepoNames(t *testing.T) {
 	}
 }
 
+func TestEnsureLock(t *testing.T) {
+	repo := "http://example.com/charts"
+	tests := []struct {
+		name  string
+		cDeps []*chart.Dependency
+		lDeps []*chart.Dependency
+		err   bool
+	}{
+		{
+			name: "no dep",
+			err:  false,
+		},
+		{
+			name: "extra lock dep",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+				{Name: "extra-subchart", Version: "0.1.0", Repository: repo},
+			},
+			err: true,
+		},
+		{
+			name: "extra lock dep without repo",
+			cDeps: []*chart.Dependency{
+				{Name: "with-repo", Version: "0.1.0", Repository: repo},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "with-repo", Version: "0.1.0", Repository: repo},
+				{Name: "without-repo", Version: "0.1.0"},
+			},
+			err: false,
+		},
+		{
+			name: "extra chart dep",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+				{Name: "extra-subchart", Version: "0.1.0", Repository: repo},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+			},
+			err: true,
+		},
+		{
+			name: "extra chart dep without repo",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+				{Name: "extra-subchart", Version: "0.1.0"},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+			},
+			err: false,
+		},
+		{
+			name: "lock version outdated",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "^1.3.4", Repository: repo},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "1.2.3", Repository: repo},
+			},
+			err: true,
+		},
+		{
+			name: "resolution matched",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: ">=0.1.0", Repository: repo},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.2", Repository: repo},
+			},
+			err: false,
+		},
+		{
+			name: "chart deps of same chart that only one resolved",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo, Alias: "subchart1"},
+				{Name: "subchart", Version: "0.2.0", Repository: repo, Alias: "subchart2"},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+			},
+			err: true,
+		},
+		{
+			name: "chart deps of same chart resolved to common version",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: ">=0.1.0", Repository: repo, Alias: "subchart1"},
+				{Name: "subchart", Version: ">=0.2.0", Repository: repo, Alias: "subchart2"},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.3.0", Repository: repo},
+			},
+			err: false,
+		},
+		{
+			name: "chart deps of same chart with intersected version as smaller version of lock deps",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.2.0", Repository: repo, Alias: "subchart1"},
+				{Name: "subchart", Version: "~0.2.0", Repository: repo, Alias: "subchart2"},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.2.0", Repository: repo},
+				{Name: "subchart", Version: "0.2.1", Repository: repo},
+			},
+			err: false,
+		},
+		{
+			name: "chart deps of same chart with intersected version as bigger version of lock deps",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "~0.2.0", Repository: repo, Alias: "subchart2"},
+				{Name: "subchart", Version: "~0.2.1", Repository: repo, Alias: "subchart2"},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.2.0", Repository: repo},
+				{Name: "subchart", Version: "0.2.1", Repository: repo},
+			},
+			err: true,
+		},
+		{
+			name: "multiple lock deps of single chart dep resolutions",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: ">=0.1.0", Repository: repo},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "0.1.0", Repository: repo},
+				{Name: "subchart", Version: "0.2.0", Repository: repo},
+			},
+			err: true,
+		},
+		{
+			name: "wrong version format",
+			cDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "wrong-version", Repository: repo},
+			},
+			lDeps: []*chart.Dependency{
+				{Name: "subchart", Version: "wrong-version", Repository: repo},
+			},
+			err: true,
+		},
+	}
+
+	for _, tt := range tests {
+		err := ensureLock(tt.cDeps, tt.lDeps)
+
+		if err != nil {
+			if tt.err {
+				continue
+			}
+			t.Fatal(fmt.Sprintf("Expected no error in test %q but:", tt.name), err)
+		}
+
+		if tt.err {
+			t.Fatalf("Expected error in test %q", tt.name)
+		}
+	}
+}
+
 // 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) {