From c8105d59b77cf923d7000ff89c0f578b6fcada48 Mon Sep 17 00:00:00 2001 From: Reinaldo de Souza Jr Date: Mon, 28 Sep 2020 19:01:18 +0200 Subject: [PATCH] Add tests Signed-off-by: Reinaldo de Souza Jr --- pkg/downloader/chart_downloader.go | 23 ++-- pkg/downloader/chart_downloader_test.go | 144 ++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 12 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 3705f27c1..989264448 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -187,12 +187,11 @@ func (c *ChartDownloader) downloadChartFiles(ref, version string) (string, error } resourceURL := u.String() - cacheKey, err := c.cacheKey(resourceURL) + dstPath, err := c.cachePathFor(repo, resourceURL) if err != nil { return "", err } - dstPath := c.cachePathFor(repo, cacheKey) if err := c.cachedDownload(g, resourceURL, dstPath); err != nil { return dstPath, err } @@ -241,27 +240,27 @@ func (c *ChartDownloader) cachedDownload(g getter.Getter, resourceURL, dstPath s return fileutil.AtomicWriteFile(dstPath, data, 0644) } -// Returns the chart cache path for a given repo +// Returns the chart cache path for a given chart of a repo. // TODO: might be vulnerable to path traversals but I don't see any safe implementation in helm codebase // https://github.com/golang/go/issues/20126 -func (c *ChartDownloader) cachePathFor(repo *repo.Entry, cacheKey string) string { +func (c *ChartDownloader) cachePathFor(repo *repo.Entry, chartURL string) (string, error) { repoName := "-" // represents an 'unknown' repo if repo != nil && repo.Name != "" { repoName = repo.Name } - return filepath.Join(c.ChartCache, repoName, cacheKey) + cacheKey, err := c.cacheKey(chartURL) + if err != nil { + return "", err + } + + return filepath.Join(c.ChartCache, repoName, cacheKey), nil } -// cacheKey returns a cache key that uniquely identifies a chart resource and is human readable +// cacheKey returns a cache key that uniquely identifies a chart resource and is human readable. +// It also preserves the filename to make copy operations easier. func (c *ChartDownloader) cacheKey(href string) (string, error) { baseURI, fileName := path.Split(href) - - // Given - // https://storage.cloud.google.com/kubernetes-charts/airflow-7.9.0.tgz - // https://storage.cloud.google.com/kubernetes-charts/ = e8e54542a371d321238a5969734af6f1d8ab04db (sha1) - // Returns - // e8e54542a371d321238a5969734af6f1d8ab04db-airflow-7.9.0.tgz digest, err := c.digest(baseURI) if err != nil { return "", err diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 85f5aa332..5c5600fda 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -18,8 +18,10 @@ package downloader import ( "net/http" "os" + "path" "path/filepath" "reflect" + "sort" "testing" "helm.sh/helm/v3/internal/test/ensure" @@ -34,6 +36,39 @@ const ( repoCache = "testdata/repository" ) +func TestCacheKey(t *testing.T) { + for _, tt := range []struct { + chartURL string + repo *repo.Entry + expected string + }{ + { + repo: nil, chartURL: "https://storage.cloud.google.com/kubernetes-charts/airflow-7.9.0.tgz", + expected: "-/e8e54542a371d321238a5969734af6f1d8ab04db/airflow-7.9.0.tgz", + }, + { + repo: &repo.Entry{}, chartURL: "https://storage.cloud.google.com/kubernetes-charts/airflow-7.9.0.tgz", + expected: "-/e8e54542a371d321238a5969734af6f1d8ab04db/airflow-7.9.0.tgz", + }, + { + repo: &repo.Entry{Name: "unstable"}, chartURL: "https://storage.cloud.google.com/kubernetes-charts/airflow-7.9.0.tgz", + expected: "unstable/e8e54542a371d321238a5969734af6f1d8ab04db/airflow-7.9.0.tgz", + }, + } { + c := &ChartDownloader{} + got, err := c.cachePathFor(tt.repo, tt.chartURL) + if err != nil { + t.Errorf("failed to compute cacheKey: %s", err) + continue + } + + if got != tt.expected { + t.Errorf("expected %s, got %s", tt.expected, got) + continue + } + } +} + func TestResolveChartRef(t *testing.T) { tests := []struct { name, ref, expect, version string @@ -203,6 +238,115 @@ func TestIsTar(t *testing.T) { } } +func TestFetch_provenanceVerification(t *testing.T) { + // Setup a temp chart cache path + chartCachePath := ensure.TempDir(t) + defer os.RemoveAll(chartCachePath) + + // Set up a fake repo + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") + if err != nil { + t.Fatal(err) + } + + if err := srv.CreateIndex(); err != nil { + t.Fatal(err) + } + + if err := srv.LinkIndices(); err != nil { + t.Fatal(err) + } + + c := ChartDownloader{ + Out: os.Stderr, + Keyring: "testdata/helm-test-key.pub", + RepositoryConfig: repoConfig, + RepositoryCache: repoCache, + ChartCache: chartCachePath, + Getters: getter.All(&cli.EnvSettings{ + RepositoryConfig: repoConfig, + RepositoryCache: repoCache, + }), + } + + // Fetch a chart without provenance + for _, tt := range []struct { + chart string + verify VerificationStrategy + expectedCache []string + expectVerified bool + }{ + { + chart: "local-subchart-0.1.0.tgz", + verify: VerifyNever, + expectedCache: []string{"local-subchart-0.1.0.tgz"}, + }, + { + chart: "local-subchart-0.1.0.tgz", + verify: VerifyLater, + expectedCache: []string{"local-subchart-0.1.0.tgz"}, + }, + { + chart: "local-subchart-0.1.0.tgz", + verify: VerifyIfPossible, + expectedCache: []string{"local-subchart-0.1.0.tgz"}, // no provenance available + }, + { + chart: "signtest-0.1.0.tgz", + verify: VerifyNever, + expectedCache: []string{"signtest-0.1.0.tgz"}, + }, + { + chart: "signtest-0.1.0.tgz", + verify: VerifyLater, + expectedCache: []string{"signtest-0.1.0.tgz", "signtest-0.1.0.tgz.prov"}, + }, + { + chart: "signtest-0.1.0.tgz", + verify: VerifyIfPossible, + expectedCache: []string{"signtest-0.1.0.tgz", "signtest-0.1.0.tgz.prov"}, + expectVerified: true, + }, + { + chart: "signtest-0.1.0.tgz", + verify: VerifyAlways, + expectedCache: []string{"signtest-0.1.0.tgz", "signtest-0.1.0.tgz.prov"}, + expectVerified: true, + }, + } { + os.RemoveAll(chartCachePath) // Starts with an empty cache + + c.Verify = tt.verify + where, ver, err := c.Fetch(srv.URL()+"/"+tt.chart, "") + if err != nil { + t.Error(err) + continue + } + + if got := path.Base(where); got != tt.chart { + t.Errorf("chart is not cached with the same filename. expected: %s, got: %s\n", tt.chart, got) + continue + } + + cachedCharts, _ := filepath.Glob(filepath.Join(chartCachePath, "*/*/*.tgz*")) + cachedFilenames := make([]string, 0, len(cachedCharts)) + for _, inCache := range cachedCharts { + cachedFilenames = append(cachedFilenames, filepath.Base(inCache)) // Remove the tmpdir and cache key + } + + sort.Strings(cachedFilenames) + sort.Strings(tt.expectedCache) + if !reflect.DeepEqual(cachedFilenames, tt.expectedCache) { + t.Errorf("unexpected cached files: expected %v, got %v\n", tt.expectedCache, cachedFilenames) + continue + } + + if tt.expectVerified && ver.FileHash == "" { + t.Error("File hash was empty, but verification is required.") + } + } +} + func TestDownloadTo(t *testing.T) { dest := ensure.TempDir(t) defer os.RemoveAll(dest)