From 13e5bc961b3eb6ef22bd3f8987d3c7d7ba15c786 Mon Sep 17 00:00:00 2001 From: Reinaldo de Souza Jr Date: Fri, 25 Sep 2020 22:00:39 +0200 Subject: [PATCH] Add characterization tests to understand getter.WithURL() Signed-off-by: Reinaldo de Souza Jr --- pkg/downloader/chart_downloader_test.go | 41 +++++++++++++++-- pkg/getter/httpgetter_test.go | 59 ++++++++++++++----------- 2 files changed, 69 insertions(+), 31 deletions(-) diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index affd30612..76136b80e 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -19,6 +19,7 @@ import ( "net/http" "os" "path/filepath" + "reflect" "testing" "helm.sh/helm/v3/internal/test/ensure" @@ -42,6 +43,7 @@ func TestResolveChartRef(t *testing.T) { {name: "full URL", ref: "http://example.com/foo-1.2.3.tgz", expect: "http://example.com/foo-1.2.3.tgz"}, {name: "full URL, HTTPS", ref: "https://example.com/foo-1.2.3.tgz", expect: "https://example.com/foo-1.2.3.tgz"}, {name: "full URL, with authentication", ref: "http://username:password@example.com/foo-1.2.3.tgz", expect: "http://username:password@example.com/foo-1.2.3.tgz"}, + {name: "full URL, no repo", ref: "https://unknown-example.com/foo-1.2.3.tgz", expect: "https://unknown-example.com/foo-1.2.3.tgz"}, {name: "reference, testing repo", ref: "testing/alpine", expect: "http://example.com/alpine-1.2.3.tgz"}, {name: "reference, version, testing repo", ref: "testing/alpine", version: "0.2.0", expect: "http://example.com/alpine-0.2.0.tgz"}, {name: "reference, version, malformed repo", ref: "malformed/alpine", version: "1.2.3", expect: "http://dl.example.com/alpine-1.2.3.tgz"}, @@ -85,15 +87,41 @@ func TestResolveChartOpts(t *testing.T) { tests := []struct { name, ref, version string expect []getter.Option + expectChartURL string }{ { - name: "repo with CA-file", - ref: "testing-ca-file/foo", + name: "reference", + ref: "testing/alpine", + expectChartURL: "http://example.com/alpine-1.2.3.tgz", + expect: []getter.Option{ + getter.WithURL("http://example.com/alpine-1.2.3.tgz"), + }, + }, + { + name: "reference, repo with CA-file", + ref: "testing-ca-file/foo", + expectChartURL: "https://example.com/foo-1.2.3.tgz", expect: []getter.Option{ getter.WithURL("https://example.com/foo-1.2.3.tgz"), getter.WithTLSClientConfig("cert", "key", "ca"), }, }, + { + name: "full URL", + ref: "http://example.com/foo-1.2.3.tgz", + expectChartURL: "http://example.com/foo-1.2.3.tgz", + expect: []getter.Option{ + getter.WithURL("http://example.com/foo-1.2.3.tgz"), + }, + }, + { + name: "full URL, no repo", + ref: "http://unknown-example.com/foo-1.2.3.tgz", + expectChartURL: "http://unknown-example.com/foo-1.2.3.tgz", + expect: []getter.Option{ + getter.WithURL("http://unknown-example.com/foo-1.2.3.tgz"), + }, + }, } c := ChartDownloader{ @@ -136,8 +164,13 @@ func TestResolveChartOpts(t *testing.T) { continue } - if *(got.(*getter.HTTPGetter)) != *(expect.(*getter.HTTPGetter)) { - t.Errorf("%s: expected %s, got %s", tt.name, expect, got) + if got := u.String(); tt.expectChartURL != u.String() { + t.Errorf("%s: expected %#v, got %#v", tt.name, tt.expectChartURL, got) + continue + } + + if !reflect.DeepEqual(got.(*getter.HTTPGetter), expect.(*getter.HTTPGetter)) { + t.Errorf("%s: expected %#v, got %#v", tt.name, expect, got) } } } diff --git a/pkg/getter/httpgetter_test.go b/pkg/getter/httpgetter_test.go index 90578f7b7..804b1dad1 100644 --- a/pkg/getter/httpgetter_test.go +++ b/pkg/getter/httpgetter_test.go @@ -180,6 +180,7 @@ func TestDownload(t *testing.T) { func TestDownloadTLS(t *testing.T) { cd := "../../testdata" + // Certificate's server name is "helm.sh" ca, pub, priv := filepath.Join(cd, "rootca.crt"), filepath.Join(cd, "crt.pem"), filepath.Join(cd, "key.pem") tlsSrv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) @@ -194,36 +195,40 @@ func TestDownloadTLS(t *testing.T) { defer tlsSrv.Close() u, _ := url.ParseRequestURI(tlsSrv.URL) - g, err := NewHTTPGetter( - WithURL(u.String()), - WithTLSClientConfig(pub, priv, ca), - ) - if err != nil { - t.Fatal(err) - } - - if _, err := g.Get(u.String()); err != nil { - t.Error(err) - } - - // now test with TLS config being passed along in .Get (see #6635) - g, err = NewHTTPGetter() - if err != nil { - t.Fatal(err) - } + ipAddress := u.String() + + u.Host = tlsConf.ServerName + hostnameAddress := u.String() + + // tlsSrv.URL has an IP as host (127.0.0.1) + // for this reason, need to also test with a hostname address + for _, aURL := range []string{ipAddress, hostnameAddress} { + g, _ := NewHTTPGetter( + WithURL(aURL), + WithTLSClientConfig(pub, priv, ca), + ) + if _, err := g.Get(ipAddress); err != nil { + t.Errorf("%s: %s", aURL, err) + } - if _, err := g.Get(u.String(), WithURL(u.String()), WithTLSClientConfig(pub, priv, ca)); err != nil { - t.Error(err) - } + // now test with TLS config being passed along in .Get (see #6635) + g, _ = NewHTTPGetter() + if _, err := g.Get(ipAddress, WithURL(aURL), WithTLSClientConfig(pub, priv, ca)); err != nil { + t.Errorf("%s: %s", aURL, err) + } - // test with only the CA file (see also #6635) - g, err = NewHTTPGetter() - if err != nil { - t.Fatal(err) - } + // test with only the CA file (see also #6635) + g, _ = NewHTTPGetter() + if _, err := g.Get(ipAddress, WithURL(aURL), WithTLSClientConfig("", "", ca)); err != nil { + t.Errorf("%s: %s", aURL, err) + } - if _, err := g.Get(u.String(), WithURL(u.String()), WithTLSClientConfig("", "", ca)); err != nil { - t.Error(err) + // test TLS validation actually validates + g, _ = NewHTTPGetter() + u.Host = "totally-legit-helm.sh" + if _, err := g.Get(ipAddress, WithURL(u.String()), WithTLSClientConfig(pub, priv, ca)); err == nil { + t.Errorf("%s: expected to fail with: 'certificate is valid for helm.sh, not totally-legit-helm.sh'", aURL) + } } }