From 237acc1f98f71d127e677b1b5ff1efeda37c302f Mon Sep 17 00:00:00 2001 From: Hang Park Date: Tue, 15 Oct 2019 06:50:35 +0900 Subject: [PATCH] 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 --- pkg/downloader/manager.go | 82 +++++++++++++++++ pkg/downloader/manager_test.go | 164 +++++++++++++++++++++++++++++++++ 2 files changed, 246 insertions(+) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 812997ea7..ce6743b05 100644 --- a/pkg/downloader/manager.go +++ b/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) diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index e7955b093..f1ea3e2a6 100644 --- a/pkg/downloader/manager_test.go +++ b/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) {