From 741e5228fc1b77e817b3700c81e14d718fdca6f7 Mon Sep 17 00:00:00 2001 From: Nana Kwadwo Date: Sun, 30 Jul 2023 13:41:24 +0000 Subject: [PATCH] fix: resolve incorrect tag selection in helm chart upgrade Signed-off-by: Nana Kwadwo --- cmd/chart/chart.go | 2 +- cmd/chart/upgrade.go | 83 +++++++++++++++++--- cmd/chart/upgrade_test.go | 155 ++++++++++++++++++++++++++++++++++++++ pkg/helm/io.go | 28 ++++++- pkg/helm/io_test.go | 72 ++++++++++++++++++ 5 files changed, 323 insertions(+), 17 deletions(-) create mode 100644 cmd/chart/upgrade_test.go create mode 100644 pkg/helm/io_test.go diff --git a/cmd/chart/chart.go b/cmd/chart/chart.go index 3ac5bbd2d..d65ec089e 100644 --- a/cmd/chart/chart.go +++ b/cmd/chart/chart.go @@ -25,7 +25,7 @@ func MakeChart() *cobra.Command { } command.AddCommand(MakeVerify()) - command.AddCommand(MakeUpgrade()) + command.AddCommand(MakeUpgrade(craneLister{})) return command } diff --git a/cmd/chart/upgrade.go b/cmd/chart/upgrade.go index 15a6bb279..291a87acb 100644 --- a/cmd/chart/upgrade.go +++ b/cmd/chart/upgrade.go @@ -15,7 +15,17 @@ import ( "github.com/spf13/cobra" ) -func MakeUpgrade() *cobra.Command { +type tagLister interface { + listTags(string) ([]string, error) +} + +type craneLister struct{} + +func (c craneLister) listTags(image string) ([]string, error) { + return crane.ListTags(image) +} + +func MakeUpgrade(lister tagLister) *cobra.Command { var command = &cobra.Command{ Use: "upgrade", Short: "Upgrade all images in a values.yaml file to the latest version", @@ -87,23 +97,16 @@ Otherwise, it returns a non-zero exit code and the updated values.yaml file.`, for k := range filtered { imageName, tag := splitImageName(k) - ref, err := crane.ListTags(imageName) + ref, err := lister.listTags(imageName) if err != nil { return errors.New("unable to list tags for " + imageName) } - var vs []*semver.Version - for _, r := range ref { - v, err := semver.NewVersion(r) - if err == nil { - vs = append(vs, v) - } + latestTag, err := getLatestTag(tag, ref) + if err != nil { + return err } - sort.Sort(sort.Reverse(semver.Collection(vs))) - - latestTag := vs[0].String() - if latestTag != tag { updated++ // Semver is "eating" the "v" prefix, so we need to add it back, if it was there in first place @@ -144,3 +147,59 @@ func splitImageName(reposName string) (string, string) { nameParts := strings.SplitN(reposName, ":", 2) return nameParts[0], nameParts[1] } + +func getLatestTag(tag string, refs []string) (string, error) { + current, err := semver.NewVersion(tag) + if err != nil { + return "", err + } + + hasRootlessSuffix := strings.Contains(current.Prerelease(), "rootless") + + var vs []*semver.Version + for _, r := range refs { + // Strip out '-rootless' metadata from version string as semver treats it as prerelease + if hasRootlessSuffix { + r = strings.TrimSuffix(r, "-rootless") + r = r + "+rootless" + } + + v, err := semver.NewVersion(r) + if err == nil && v.GreaterThan(current) { + vs = append(vs, v) + } + } + + if len(vs) == 0 { + return tag, err + } + + latestStableVersion := getLatestStableVersion(vs) + + return latestStableVersion, nil +} + +func getLatestStableVersion(vs []*semver.Version) string { + sort.Sort(sort.Reverse(semver.Collection(vs))) + latest := vs[0] + + // Finding the latest stable version by selecting the first version without a prerelease tag + if latest.Prerelease() != "" { + for _, ver := range vs { + if ver.Prerelease() == "" { + latest = ver + break + } + } + } + + latestStr := latest.String() + + // Adding the '-rootless' build tag back + if strings.HasSuffix(latestStr, "+rootless") { + latestStr = strings.TrimSuffix(latestStr, "+rootless") + latestStr = latestStr + "-rootless" + } + + return latestStr +} diff --git a/cmd/chart/upgrade_test.go b/cmd/chart/upgrade_test.go new file mode 100644 index 000000000..e2c2c0339 --- /dev/null +++ b/cmd/chart/upgrade_test.go @@ -0,0 +1,155 @@ +package chart + +import ( + "fmt" + "os" + "testing" + + "gopkg.in/yaml.v2" +) + +type test struct { + title string + current string + tags []string + want string +} + +type mockLister struct { + imageTags map[string][]string +} + +func (ml mockLister) listTags(image string) ([]string, error) { + tags, ok := ml.imageTags[image] + if !ok { + return nil, fmt.Errorf("Image %s not found", image) + } + + return tags, nil +} + +func newMockLister(t *testing.T, tests []test) *mockLister { + tagsMap := make(map[string][]string) + lister := mockLister{imageTags: tagsMap} + + if len(tests) == 0 { + t.Fatal("Tests cannot be empty, please provide test cases") + } + + for _, tt := range tests { + key, _ := splitImageName(tt.current) + // Ensuring that a test entry with the same key doesn't already exist to prevent overwriting + if _, ok := lister.imageTags[key]; !ok { + lister.imageTags[key] = tt.tags + } else { + t.Fatalf("Duplicate image name found which might break tests: %s", tt.current) + } + } + + return &lister +} + +func Test_ChartUpgrade(t *testing.T) { + tests := []test{ + { + title: "Promote from a rootless build to the latest rootless build", + current: "moby/buildkit:v0.11.5-rootless", + want: "moby/buildkit:v0.11.6-rootless", + tags: []string{"0.10.6-rootless", "v0.11.5", "0.11.6", "0.11.6-rootless", "0.12.0-rc1", "0.12.0-rc1-rootless"}, + }, + { + title: "Promote from a rootless build to the latest rootless build", + current: "moby1/buildkit:v0.10.0-rootless", + want: "moby1/buildkit:v0.11.6-rootless", + tags: []string{"0.10.0", "0.10.6", "0.10.6-rootless", "0.11.6", "0.11.6-rootless", "0.12.0-rc1", "0.12.0-rc1-rootless"}, + }, + { + title: "Promote from non-rootless build to the latest stable build", + current: "moby2/buildkit:v0.8.0", + want: "moby2/buildkit:v0.11.6", + tags: []string{"v0.8.0", "0.10.6-rootless", "0.11.6", "0.11.6-rootless", "0.12.0-rc1", "0.12.0-rc1-rootless"}, + }, + { + title: "Promote from stable release to the latest stable release", + current: "prom/prometheus:v2.42.0", + want: "prom/prometheus:v2.44.0", + tags: []string{"2.41.5", "2.42.0", "2.43.0", "2.44.0", "2.45.0-rc.0", "2.45.0-rc.1"}, + }, + { + title: "Promote from stable release to the latest stable release", + current: "prom1/prometheus:v2.2.0", + want: "prom1/prometheus:v2.44.0", + tags: []string{"v2.1.0", "v2.2.0", "2.41.5", "2.42.0", "2.43.0", "2.44.0", "2.45.0-rc.0", "2.45.0-rc.1"}, + }, + { + title: "Promote from release condidate to the latest stable release", + current: "bitnami/postgresql:v2.42.0-rc.2", + want: "bitnami/postgresql:v2.44.0", + tags: []string{"v2.42.0-rc.5", "2.41.5", "2.42.0", "2.43.0", "2.44.0", "2.45.0-rc.0", "2.45.0-rc.1"}, + }, + { + title: "Promote from prerelease version to the latest prerelease version", + current: "bitnami/rabbitmq:v2.22.0-rc1", + want: "bitnami/rabbitmq:v2.22.0-rc3", + tags: []string{"v2.21.0", "v2.22.0-rc1", "v2.22.0-rc2", "v2.22.0-rc3"}, + }, + { + title: "Remain on current version when there isn't a later version", + current: "openfaas/openfaas:v2.23.3", + want: "openfaas/openfaas:v2.23.3", + tags: []string{"v2.23.3", "v2.21.0", "v2.15.6"}, + }, + } + + testFile, err := os.CreateTemp(os.TempDir(), "arkade_*.yml") + if err != nil { + t.Fatal(err) + } + + defer os.Remove(testFile.Name()) + + testData := make(map[string]map[string]string) + for i, t := range tests { + title := fmt.Sprintf("test%d", i) + testData[title] = map[string]string{"image": t.current} + } + + yamlBytes, err := yaml.Marshal(testData) + if err != nil { + t.Fatal(err) + } + + if _, err := testFile.Write(yamlBytes); err != nil { + t.Fatal(err) + } + + lister := newMockLister(t, tests) + + cmd := MakeUpgrade(lister) + cmd.SetArgs([]string{"--write", "--file", testFile.Name()}) + + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + yamlData, err := os.ReadFile(testFile.Name()) + if err != nil { + t.Fatal(err) + } + + var final map[string]map[string]string + err = yaml.Unmarshal(yamlData, &final) + if err != nil { + t.Fatal(err) + } + + for i, tc := range tests { + t.Run(tc.title, func(t *testing.T) { + title := fmt.Sprintf("test%d", i) + got := final[title]["image"] + if got != tc.want { + t.Fatalf("want: %s, got: %s", tc.want, got) + } + }) + } +} diff --git a/pkg/helm/io.go b/pkg/helm/io.go index d6db3a71d..c849abc98 100644 --- a/pkg/helm/io.go +++ b/pkg/helm/io.go @@ -1,10 +1,12 @@ package helm import ( + "bufio" + "bytes" "fmt" "os" "reflect" - "strings" + "regexp" "gopkg.in/yaml.v2" ) @@ -38,10 +40,28 @@ func ReplaceValuesInHelmValuesFile(values map[string]string, yamlPath string) (s return "", err } - fileContent := string(readFile) - for k, v := range values { - fileContent = strings.ReplaceAll(fileContent, k, v) + var buffer bytes.Buffer + + scanner := bufio.NewScanner(bytes.NewReader(readFile)) + for scanner.Scan() { + line := scanner.Text() + + for k, v := range values { + // The regex 're' is constructed to match the exact key 'k' as a whole word at the end of + // the line to prevent unintended replacement of partial matches. + re := regexp.MustCompile(`\b` + regexp.QuoteMeta(k) + `$`) + line = re.ReplaceAllString(line, v) + } + + buffer.WriteString(line) + buffer.WriteByte('\n') } + + if err := scanner.Err(); err != nil { + return "", err + } + + fileContent := buffer.String() return fileContent, nil } diff --git a/pkg/helm/io_test.go b/pkg/helm/io_test.go new file mode 100644 index 000000000..ae26509f7 --- /dev/null +++ b/pkg/helm/io_test.go @@ -0,0 +1,72 @@ +package helm + +import ( + "fmt" + "os" + "testing" + + "gopkg.in/yaml.v2" +) + +func Test_ReplaceValuesInHelmValuesFile(t *testing.T) { + tests := []struct { + current string + update string + }{ + {current: "ghcr.io/openfaas/faas-netes:0.1.0", update: "ghcr.io/openfaas/faas-netes:0.2.0"}, + {current: "ghcr.io/openfaas/faas-netes:0.1.0-rc", update: "ghcr.io/openfaas/faas-netes:0.2.0"}, + {current: "prom/prometheus:v2.43.0", update: "prom/prometheus:v2.45.0"}, + {current: "prom/prometheus:v2.43.0-rc.0", update: "prom/prometheus:v2.45.0"}, + {current: "ghcr.io/openfaas/faas-netes:0.1.0-rc.1", update: "ghcr.io/openfaas/faas-netes:0.2.0"}, + {current: "ghcr.io/openfaas/faas-netes:0.1.0-rc.1-rc.2", update: "ghcr.io/openfaas/faas-netes:0.2.0"}, + } + + testFile, err := os.CreateTemp(os.TempDir(), "arkade_*.yml") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(testFile.Name()) + + yamlData := make(map[string]map[string]string) + for i, t := range tests { + title := fmt.Sprintf("test%d", i) + yamlData[title] = map[string]string{"image": t.current} + } + + yamlBytes, err := yaml.Marshal(yamlData) + if err != nil { + t.Fatal(err) + } + + if _, err := testFile.Write(yamlBytes); err != nil { + t.Fatal(err) + } + + input := make(map[string]string) + for _, t := range tests { + input[t.current] = t.update + } + + // Test is run multiple times to verify consistent and correct output independent of the + // input map's iteration order, addressing potential non-deterministic behaviour + count := 10 + for i := 0; i < count; i++ { + out, err := ReplaceValuesInHelmValuesFile(input, testFile.Name()) + if err != nil { + t.Fatal(err) + } + + var results map[string]map[string]string + if err := yaml.Unmarshal([]byte(out), &results); err != nil { + t.Fatal(err) + } + + for i, tc := range tests { + title := fmt.Sprintf("test%d", i) + got := results[title]["image"] + if got != tc.update { + t.Fatalf("want: %s, got: %s", tc.update, got) + } + } + } +}