From 5600b129efa290a35fade6ce6766108f277b0f7b Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Thu, 6 Oct 2016 16:07:22 -0600 Subject: [PATCH] fix(helm): resolve URLs and SemVers correctly The original dependency resolution did not correctly resolve version or URL of a dependency. Version was tracked by filename, and URL was assumed to be absolute. This fixes both of those. Closes #1277 --- cmd/helm/downloader/manager.go | 57 ++++++++++-- cmd/helm/downloader/manager_test.go | 87 +++++++++++++++++++ .../cache/kubernetes-charts-index.yaml | 9 +- .../repository/cache/testing-index.yaml | 1 + .../helmhome/repository/repositories.yaml | 2 + 5 files changed, 144 insertions(+), 12 deletions(-) create mode 100644 cmd/helm/downloader/manager_test.go create mode 100644 cmd/helm/downloader/testdata/helmhome/repository/cache/testing-index.yaml diff --git a/cmd/helm/downloader/manager.go b/cmd/helm/downloader/manager.go index 17ef1efd2..373714cee 100644 --- a/cmd/helm/downloader/manager.go +++ b/cmd/helm/downloader/manager.go @@ -22,10 +22,12 @@ import ( "io/ioutil" "net/url" "os" + "path" "path/filepath" "strings" "sync" + "github.com/Masterminds/semver" "github.com/ghodss/yaml" "k8s.io/helm/cmd/helm/helmpath" @@ -175,7 +177,7 @@ func (m *Manager) downloadAll(deps []*chartutil.Dependency) error { for _, dep := range deps { fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository) - churl, err := findChartURL(dep.Name, dep.Repository, repos) + churl, err := findChartURL(dep.Name, dep.Version, dep.Repository, repos) if err != nil { fmt.Fprintf(m.Out, "WARNING: %s (skipped)", err) continue @@ -270,21 +272,28 @@ func urlsAreEqual(a, b string) bool { // findChartURL searches the cache of repo data for a chart that has the name and the repourl specified. // -// In this current version, name is of the form 'foo-1.2.3'. This will change when -// the repository index stucture changes. -func findChartURL(name, repourl string, repos map[string]*repo.ChartRepository) (string, error) { +// 'name' is the name of the chart. Version is an exact semver, or an empty string. If empty, the +// newest version will be returned. +// +// repourl is the repository to search +// +// If it finds a URL that is "relative", it will prepend the repourl. +func findChartURL(name, version, repourl string, repos map[string]*repo.ChartRepository) (string, error) { for _, cr := range repos { if urlsAreEqual(repourl, cr.URL) { for ename, entry := range cr.IndexFile.Entries { if ename == name { for _, verEntry := range entry { if len(verEntry.URLs) == 0 { - // Not totally sure what to do here. Returning an - // error is the strictest option. Skipping it might - // be preferable. - return "", fmt.Errorf("chart %q has no download URL", name) + // Not a legit entry. + continue } - return verEntry.URLs[0], nil + + if version == "" || versionEquals(version, verEntry.Version) { + return normalizeURL(repourl, verEntry.URLs[0]) + } + + return normalizeURL(repourl, verEntry.URLs[0]) } } } @@ -293,6 +302,36 @@ func findChartURL(name, repourl string, repos map[string]*repo.ChartRepository) return "", fmt.Errorf("chart %s not found in %s", name, repourl) } +func versionEquals(v1, v2 string) bool { + sv1, err := semver.NewVersion(v1) + if err != nil { + // Fallback to string comparison. + return v1 == v2 + } + sv2, err := semver.NewVersion(v2) + if err != nil { + return false + } + return sv1.Equal(sv2) +} + +func normalizeURL(baseURL, urlOrPath string) (string, error) { + u, err := url.Parse(urlOrPath) + if err != nil { + return urlOrPath, err + } + if u.IsAbs() { + return u.String(), nil + } + u2, err := url.Parse(baseURL) + if err != nil { + return urlOrPath, fmt.Errorf("Base URL failed to parse: %s", err) + } + + u2.Path = path.Join(u2.Path, urlOrPath) + return u2.String(), nil +} + // loadChartRepositories reads the repositories.yaml, and then builds a map of // ChartRepositories. // diff --git a/cmd/helm/downloader/manager_test.go b/cmd/helm/downloader/manager_test.go new file mode 100644 index 000000000..3910d1684 --- /dev/null +++ b/cmd/helm/downloader/manager_test.go @@ -0,0 +1,87 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package downloader + +import ( + "bytes" + "testing" + + "k8s.io/helm/cmd/helm/helmpath" + //"k8s.io/helm/pkg/repo" +) + +func TestVersionEquals(t *testing.T) { + tests := []struct { + name, v1, v2 string + expect bool + }{ + {name: "semver match", v1: "1.2.3-beta.11", v2: "1.2.3-beta.11", expect: true}, + {name: "semver match, build info", v1: "1.2.3-beta.11+a", v2: "1.2.3-beta.11+b", expect: true}, + {name: "string match", v1: "abcdef123", v2: "abcdef123", expect: true}, + {name: "semver mismatch", v1: "1.2.3-beta.11", v2: "1.2.3-beta.22", expect: false}, + {name: "semver mismatch, invalid semver", v1: "1.2.3-beta.11", v2: "stinkycheese", expect: false}, + } + + for _, tt := range tests { + if versionEquals(tt.v1, tt.v2) != tt.expect { + t.Errorf("%s: failed comparison of %q and %q (expect equal: %t)", tt.name, tt.v1, tt.v2, tt.expect) + } + } +} + +func TestNormalizeURL(t *testing.T) { + tests := []struct { + name, base, path, expect string + }{ + {name: "basic URL", base: "https://example.com", path: "http://helm.sh/foo", expect: "http://helm.sh/foo"}, + {name: "relative path", base: "https://helm.sh/charts", path: "foo", expect: "https://helm.sh/charts/foo"}, + } + + for _, tt := range tests { + got, err := normalizeURL(tt.base, tt.path) + if err != nil { + t.Errorf("%s: error %s", tt.name, err) + continue + } else if got != tt.expect { + t.Errorf("%s: expected %q, got %q", tt.name, tt.expect, got) + } + } +} + +func TestFindChartURL(t *testing.T) { + b := bytes.NewBuffer(nil) + m := &Manager{ + Out: b, + HelmHome: helmpath.Home("testdata/helmhome"), + } + repos, err := m.loadChartRepositories() + if err != nil { + t.Fatal(err) + } + + name := "alpine" + version := "0.1.0" + repoURL := "http://example.com/charts" + + churl, err := findChartURL(name, version, repoURL, repos) + if err != nil { + t.Fatal(err) + } + if churl != "http://storage.googleapis.com/kubernetes-charts/alpine-0.1.0.tgz" { + t.Errorf("Unexpected URL %q", churl) + } + +} diff --git a/cmd/helm/downloader/testdata/helmhome/repository/cache/kubernetes-charts-index.yaml b/cmd/helm/downloader/testdata/helmhome/repository/cache/kubernetes-charts-index.yaml index 26ce97423..ec7283685 100644 --- a/cmd/helm/downloader/testdata/helmhome/repository/cache/kubernetes-charts-index.yaml +++ b/cmd/helm/downloader/testdata/helmhome/repository/cache/kubernetes-charts-index.yaml @@ -2,7 +2,8 @@ apiVersion: v1 entries: alpine: - name: alpine - url: http://storage.googleapis.com/kubernetes-charts/alpine-0.1.0.tgz + urls: + - http://storage.googleapis.com/kubernetes-charts/alpine-0.1.0.tgz checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d home: https://k8s.io/helm sources: @@ -14,7 +15,8 @@ entries: engine: "" icon: "" - name: alpine - url: http://storage.googleapis.com/kubernetes-charts/alpine-0.2.0.tgz + urls: + - http://storage.googleapis.com/kubernetes-charts/alpine-0.2.0.tgz checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d home: https://k8s.io/helm sources: @@ -27,7 +29,8 @@ entries: icon: "" mariadb: - name: mariadb - url: http://storage.googleapis.com/kubernetes-charts/mariadb-0.3.0.tgz + urls: + - http://storage.googleapis.com/kubernetes-charts/mariadb-0.3.0.tgz checksum: 65229f6de44a2be9f215d11dbff311673fc8ba56 home: https://mariadb.org sources: diff --git a/cmd/helm/downloader/testdata/helmhome/repository/cache/testing-index.yaml b/cmd/helm/downloader/testdata/helmhome/repository/cache/testing-index.yaml new file mode 100644 index 000000000..9cde8e8dd --- /dev/null +++ b/cmd/helm/downloader/testdata/helmhome/repository/cache/testing-index.yaml @@ -0,0 +1 @@ +apiVersion: v1 diff --git a/cmd/helm/downloader/testdata/helmhome/repository/repositories.yaml b/cmd/helm/downloader/testdata/helmhome/repository/repositories.yaml index 10d83457e..c7ddf316a 100644 --- a/cmd/helm/downloader/testdata/helmhome/repository/repositories.yaml +++ b/cmd/helm/downloader/testdata/helmhome/repository/repositories.yaml @@ -2,3 +2,5 @@ apiVersion: v1 repositories: - name: testing url: "http://example.com" + - name: kubernetes-charts + url: "http://example.com/charts"