ref(downloader): pass in options to ChartDownloader

This restores the ability to pass in parameters at runtime to the
ChartDownloader, enabling users to pass in parameters like the --username
and --password flags.

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>

pkg/action/install.go

@@ -630,8 +630,9 @@ func (c *ChartPathOptions) LocateChart(name string, settings cli.EnvSettings) (s
 		Out:      os.Stdout,
 		Keyring:  c.Keyring,
 		Getters:  getter.All(settings),
-		Username: c.Username,
-		Password: c.Password,
+		Options: []getter.Option{
+			getter.WithBasicAuth(c.Username, c.Password),
+		},
 	}
 	if c.Verify {
 		dl.Verify = downloader.VerifyAlways

pkg/action/pull.go

@@ -62,8 +62,9 @@ func (p *Pull) Run(chartRef string) (string, error) {
 		Keyring:  p.Keyring,
 		Verify:   downloader.VerifyNever,
 		Getters:  getter.All(p.Settings),
-		Username: p.Username,
-		Password: p.Password,
+		Options: []getter.Option{
+			getter.WithBasicAuth(p.Username, p.Password),
+		},
 	}
 
 	if p.Verify {

pkg/downloader/chart_downloader.go

@@ -68,10 +68,8 @@ type ChartDownloader struct {
 	HelmHome helmpath.Home
 	// Getter collection for the operation
 	Getters getter.Providers
-	// Chart repository username
-	Username string
-	// Chart repository password
-	Password string
+	// Options provide parameters to be passed along to the Getter being initialized.
+	Options []getter.Option
 }
 
 // DownloadTo retrieves a chart. Depending on the settings, it may also download a provenance file.
@@ -86,7 +84,17 @@ type ChartDownloader struct {
 // Returns a string path to the location where the file was downloaded and a verification
 // (if provenance was verified), or an error if something bad happened.
 func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *provenance.Verification, error) {
-	u, g, err := c.ResolveChartVersion(ref, version)
+	u, err := c.ResolveChartVersion(ref, version)
+	if err != nil {
+		return "", nil, err
+	}
+
+	constructor, err := c.Getters.ByScheme(u.Scheme)
+	if err != nil {
+		return "", nil, err
+	}
+
+	g, err := constructor(c.Options...)
 	if err != nil {
 		return "", nil, err
 	}
@@ -132,8 +140,8 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
 
 // ResolveChartVersion resolves a chart reference to a URL.
 //
-// It returns the URL as well as a preconfigured repo.Getter that can fetch
-// the URL.
+// It returns the URL and sets the ChartDownloader's Options that can fetch
+// the URL using the appropriate Getter.
 //
 // A reference may be an HTTP URL, a 'reponame/chartname' reference, or a local path.
 //
@@ -144,21 +152,16 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
 //		* If version is non-empty, this will return the URL for that version
 //		* If version is empty, this will return the URL for the latest version
 //		* If no version can be found, an error is returned
-func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, getter.Getter, error) {
+func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, error) {
 	u, err := url.Parse(ref)
 	if err != nil {
-		return nil, nil, errors.Errorf("invalid chart URL format: %s", ref)
+		return nil, errors.Errorf("invalid chart URL format: %s", ref)
 	}
+	c.Options = append(c.Options, getter.WithURL(ref))
 
 	rf, err := repo.LoadFile(c.HelmHome.RepositoryFile())
 	if err != nil {
-		return u, nil, err
-	}
-
-	// TODO add user-agent
-	g, err := getter.NewHTTPGetter(getter.WithURL(ref))
-	if err != nil {
-		return u, nil, err
+		return u, err
 	}
 
 	if u.IsAbs() && len(u.Host) > 0 && len(u.Path) > 0 {
@@ -173,24 +176,31 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, ge
 			// If there is no special config, return the default HTTP client and
 			// swallow the error.
 			if err == ErrNoOwnerRepo {
-				r := &repo.ChartRepository{}
-				r.Client = g
-				g.SetBasicAuth(c.getRepoCredentials(r))
-				return u, g, nil
+				return u, nil
 			}
-			return u, nil, err
+			return u, err
 		}
 
-		r, err := repo.NewChartRepository(rc, c.Getters)
 		// If we get here, we don't need to go through the next phase of looking
-		// up the URL. We have it already. So we just return.
-		return u, r.Client, err
+		// up the URL. We have it already. So we just set the parameters and return.
+		c.Options = append(
+			c.Options,
+			getter.WithURL(rc.URL),
+			getter.WithTLSClientConfig(rc.CertFile, rc.KeyFile, rc.CAFile),
+		)
+		if rc.Username != "" && rc.Password != "" {
+			c.Options = append(
+				c.Options,
+				getter.WithBasicAuth(rc.Username, rc.Password),
+			)
+		}
+		return u, nil
 	}
 
 	// See if it's of the form: repo/path_to_chart
 	p := strings.SplitN(u.Path, "/", 2)
 	if len(p) < 2 {
-		return u, nil, errors.Errorf("non-absolute URLs should be in form of repo_name/path_to_chart, got: %s", u)
+		return u, errors.Errorf("non-absolute URLs should be in form of repo_name/path_to_chart, got: %s", u)
 	}
 
 	repoName := p[0]
@@ -198,41 +208,43 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, ge
 	rc, err := pickChartRepositoryConfigByName(repoName, rf.Repositories)
 
 	if err != nil {
-		return u, nil, err
+		return u, err
 	}
 
 	r, err := repo.NewChartRepository(rc, c.Getters)
 	if err != nil {
-		return u, nil, err
+		return u, err
+	}
+	if r != nil && r.Config != nil && r.Config.Username != "" && r.Config.Password != "" {
+		c.Options = append(c.Options, getter.WithBasicAuth(r.Config.Username, r.Config.Password))
 	}
-	g.SetBasicAuth(c.getRepoCredentials(r))
 
 	// Next, we need to load the index, and actually look up the chart.
 	i, err := repo.LoadIndexFile(c.HelmHome.CacheIndex(r.Config.Name))
 	if err != nil {
-		return u, g, errors.Wrap(err, "no cached repo found. (try 'helm repo update')")
+		return u, errors.Wrap(err, "no cached repo found. (try 'helm repo update')")
 	}
 
 	cv, err := i.Get(chartName, version)
 	if err != nil {
-		return u, g, errors.Wrapf(err, "chart %q matching %s not found in %s index. (try 'helm repo update')", chartName, version, r.Config.Name)
+		return u, errors.Wrapf(err, "chart %q matching %s not found in %s index. (try 'helm repo update')", chartName, version, r.Config.Name)
 	}
 
 	if len(cv.URLs) == 0 {
-		return u, g, errors.Errorf("chart %q has no downloadable URLs", ref)
+		return u, errors.Errorf("chart %q has no downloadable URLs", ref)
 	}
 
 	// TODO: Seems that picking first URL is not fully correct
 	u, err = url.Parse(cv.URLs[0])
 	if err != nil {
-		return u, g, errors.Errorf("invalid chart URL format: %s", ref)
+		return u, errors.Errorf("invalid chart URL format: %s", ref)
 	}
 
 	// If the URL is relative (no scheme), prepend the chart repo's base URL
 	if !u.IsAbs() {
 		repoURL, err := url.Parse(rc.URL)
 		if err != nil {
-			return repoURL, nil, err
+			return repoURL, err
 		}
 		q := repoURL.Query()
 		// We need a trailing slash for ResolveReference to work, but make sure there isn't already one
@@ -240,32 +252,17 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, ge
 		u = repoURL.ResolveReference(u)
 		u.RawQuery = q.Encode()
 		// TODO add user-agent
-		g, err := getter.NewHTTPGetter(getter.WithURL(rc.URL))
-		if err != nil {
-			return repoURL, nil, err
+		if _, err := getter.NewHTTPGetter(getter.WithURL(rc.URL)); err != nil {
+			return repoURL, err
 		}
-		g.SetBasicAuth(c.getRepoCredentials(r))
-		return u, g, err
-	}
-
-	return u, g, nil
-}
-
-// If this ChartDownloader is not configured to use credentials, and the chart repository sent as an argument is,
-// then the repository's configured credentials are returned.
-// Else, this ChartDownloader's credentials are returned.
-func (c *ChartDownloader) getRepoCredentials(r *repo.ChartRepository) (username, password string) {
-	username = c.Username
-	password = c.Password
-	if r != nil && r.Config != nil {
-		if username == "" {
-			username = r.Config.Username
-		}
-		if password == "" {
-			password = r.Config.Password
+		if r != nil && r.Config != nil && r.Config.Username != "" && r.Config.Password != "" {
+			c.Options = append(c.Options, getter.WithBasicAuth(r.Config.Username, r.Config.Password))
 		}
+		return u, err
 	}
-	return
+
+	// TODO add user-agent
+	return u, nil
 }
 
 // VerifyChart takes a path to a chart archive and a keyring, and verifies the chart.

pkg/downloader/chart_downloader_test.go

@@ -16,11 +16,8 @@ limitations under the License.
 package downloader
 
 import (
-	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/http/httptest"
-	"net/url"
 	"os"
 	"path/filepath"
 	"testing"
@@ -61,7 +58,7 @@ func TestResolveChartRef(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		u, _, err := c.ResolveChartVersion(tt.ref, tt.version)
+		u, err := c.ResolveChartVersion(tt.ref, tt.version)
 		if err != nil {
 			if tt.fail {
 				continue
@@ -87,78 +84,88 @@ func TestVerifyChart(t *testing.T) {
 	}
 }
 
-func TestDownload(t *testing.T) {
-	expect := "Call me Ishmael"
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprint(w, expect)
-	}))
-	defer srv.Close()
-
-	provider, err := getter.ByScheme("http", cli.EnvSettings{})
-	if err != nil {
-		t.Fatal("No http provider found")
+func TestIsTar(t *testing.T) {
+	tests := map[string]bool{
+		"foo.tgz":           true,
+		"foo/bar/baz.tgz":   true,
+		"foo-1.2.3.4.5.tgz": true,
+		"foo.tar.gz":        false, // for our purposes
+		"foo.tgz.1":         false,
+		"footgz":            false,
 	}
 
-	g, err := provider.New(getter.WithURL(srv.URL))
-	if err != nil {
-		t.Fatal(err)
+	for src, expect := range tests {
+		if isTar(src) != expect {
+			t.Errorf("%q should be %t", src, expect)
+		}
 	}
-	got, err := g.Get(srv.URL)
+}
+
+func TestDownloadTo(t *testing.T) {
+	tmp, err := ioutil.TempDir("", "helm-downloadto-")
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer os.RemoveAll(tmp)
 
-	if got.String() != expect {
-		t.Errorf("Expected %q, got %q", expect, got.String())
+	hh := helmpath.Home(tmp)
+	dest := filepath.Join(hh.String(), "dest")
+	configDirectories := []string{
+		hh.String(),
+		hh.Repository(),
+		hh.Cache(),
+		dest,
 	}
-
-	// test with server backed by basic auth
-	basicAuthSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		username, password, ok := r.BasicAuth()
-		if !ok || username != "username" || password != "password" {
-			t.Errorf("Expected request to use basic auth and for username == 'username' and password == 'password', got '%v', '%s', '%s'", ok, username, password)
+	for _, p := range configDirectories {
+		if fi, err := os.Stat(p); err != nil {
+			if err := os.MkdirAll(p, 0755); err != nil {
+				t.Fatalf("Could not create %s: %s", p, err)
+			}
+		} else if !fi.IsDir() {
+			t.Fatalf("%s must be a directory", p)
 		}
-		fmt.Fprint(w, expect)
-	}))
-
-	defer basicAuthSrv.Close()
+	}
 
-	u, _ := url.ParseRequestURI(basicAuthSrv.URL)
-	httpgetter, err := getter.NewHTTPGetter(
-		getter.WithURL(u.String()),
-		getter.WithBasicAuth("username", "password"),
-	)
-	if err != nil {
+	// Set up a fake repo
+	srv := repotest.NewServer(tmp)
+	defer srv.Stop()
+	if _, err := srv.CopyCharts("testdata/*.tgz*"); err != nil {
+		t.Error(err)
+		return
+	}
+	if err := srv.LinkIndices(); err != nil {
 		t.Fatal(err)
 	}
-	got, err = httpgetter.Get(u.String())
+
+	c := ChartDownloader{
+		HelmHome: hh,
+		Out:      os.Stderr,
+		Verify:   VerifyAlways,
+		Keyring:  "testdata/helm-test-key.pub",
+		Getters:  getter.All(cli.EnvSettings{}),
+	}
+	cname := "/signtest-0.1.0.tgz"
+	where, v, err := c.DownloadTo(srv.URL()+cname, "", dest)
 	if err != nil {
-		t.Fatal(err)
+		t.Error(err)
+		return
 	}
 
-	if got.String() != expect {
-		t.Errorf("Expected %q, got %q", expect, got.String())
+	if expect := filepath.Join(dest, cname); where != expect {
+		t.Errorf("Expected download to %s, got %s", expect, where)
 	}
-}
 
-func TestIsTar(t *testing.T) {
-	tests := map[string]bool{
-		"foo.tgz":           true,
-		"foo/bar/baz.tgz":   true,
-		"foo-1.2.3.4.5.tgz": true,
-		"foo.tar.gz":        false, // for our purposes
-		"foo.tgz.1":         false,
-		"footgz":            false,
+	if v.FileHash == "" {
+		t.Error("File hash was empty, but verification is required.")
 	}
 
-	for src, expect := range tests {
-		if isTar(src) != expect {
-			t.Errorf("%q should be %t", src, expect)
-		}
+	if _, err := os.Stat(filepath.Join(dest, cname)); err != nil {
+		t.Error(err)
+		return
 	}
 }
 
-func TestDownloadTo(t *testing.T) {
+func TestDownloadTo_WithOptions(t *testing.T) {
 	tmp, err := ioutil.TempDir("", "helm-downloadto-")
 	if err != nil {
 		t.Fatal(err)
@@ -183,8 +190,16 @@ func TestDownloadTo(t *testing.T) {
 		}
 	}
 
-	// Set up a fake repo
+	// Set up a fake repo with basic auth enabled
 	srv := repotest.NewServer(tmp)
+	srv.Stop()
+	srv.WithMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		username, password, ok := r.BasicAuth()
+		if !ok || username != "username" || password != "password" {
+			t.Errorf("Expected request to use basic auth and for username == 'username' and password == 'password', got '%v', '%s', '%s'", ok, username, password)
+		}
+	}))
+	srv.Start()
 	defer srv.Stop()
 	if _, err := srv.CopyCharts("testdata/*.tgz*"); err != nil {
 		t.Error(err)
@@ -200,6 +215,9 @@ func TestDownloadTo(t *testing.T) {
 		Verify:   VerifyAlways,
 		Keyring:  "testdata/helm-test-key.pub",
 		Getters:  getter.All(cli.EnvSettings{}),
+		Options: []getter.Option{
+			getter.WithBasicAuth("username", "password"),
+		},
 	}
 	cname := "/signtest-0.1.0.tgz"
 	where, v, err := c.DownloadTo(srv.URL()+cname, "", dest)

pkg/downloader/manager.go

@@ -236,8 +236,9 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 			Keyring:  m.Keyring,
 			HelmHome: m.HelmHome,
 			Getters:  m.Getters,
-			Username: username,
-			Password: password,
+			Options: []getter.Option{
+				getter.WithBasicAuth(username, password),
+			},
 		}
 
 		if _, _, err := dl.DownloadTo(churl, "", destPath); err != nil {

pkg/getter/httpgetter_test.go

@@ -16,11 +16,15 @@ limitations under the License.
 package getter
 
 import (
+	"fmt"
 	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"path/filepath"
 	"testing"
 
 	"helm.sh/helm/internal/test"
+	"helm.sh/helm/pkg/cli"
 )
 
 func TestHTTPGetter(t *testing.T) {
@@ -80,3 +84,57 @@ func TestHTTPGetter(t *testing.T) {
 		t.Errorf("Expected NewHTTPGetter to contain %q as the user agent, got %q", "Groot", hg.opts.userAgent)
 	}
 }
+
+func TestDownload(t *testing.T) {
+	expect := "Call me Ishmael"
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, expect)
+	}))
+	defer srv.Close()
+
+	provider, err := ByScheme("http", cli.EnvSettings{})
+	if err != nil {
+		t.Fatal("No http provider found")
+	}
+
+	g, err := provider.New(WithURL(srv.URL))
+	if err != nil {
+		t.Fatal(err)
+	}
+	got, err := g.Get(srv.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if got.String() != expect {
+		t.Errorf("Expected %q, got %q", expect, got.String())
+	}
+
+	// test with server backed by basic auth
+	basicAuthSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		username, password, ok := r.BasicAuth()
+		if !ok || username != "username" || password != "password" {
+			t.Errorf("Expected request to use basic auth and for username == 'username' and password == 'password', got '%v', '%s', '%s'", ok, username, password)
+		}
+		fmt.Fprint(w, expect)
+	}))
+
+	defer basicAuthSrv.Close()
+
+	u, _ := url.ParseRequestURI(basicAuthSrv.URL)
+	httpgetter, err := NewHTTPGetter(
+		WithURL(u.String()),
+		WithBasicAuth("username", "password"),
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	got, err = httpgetter.Get(u.String())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if got.String() != expect {
+		t.Errorf("Expected %q, got %q", expect, got.String())
+	}
+}

pkg/repo/repotest/server.go

@@ -69,7 +69,7 @@ func NewServer(docroot string) *Server {
 	srv := &Server{
 		docroot: root,
 	}
-	srv.start()
+	srv.Start()
 	// Add the testing repository as the only repo.
 	if err := setTestingRepository(helmpath.Home(docroot), "test", srv.URL()); err != nil {
 		panic(err)
@@ -79,8 +79,15 @@ func NewServer(docroot string) *Server {
 
 // Server is an implementation of a repository server for testing.
 type Server struct {
-	docroot string
-	srv     *httptest.Server
+	docroot    string
+	srv        *httptest.Server
+	middleware http.HandlerFunc
+}
+
+// WithMiddleware injects middleware in front of the server. This can be used to inject
+// additional functionality like layering in an authentication frontend.
+func (s *Server) WithMiddleware(middleware http.HandlerFunc) {
+	s.middleware = middleware
 }
 
 // Root gets the docroot for the server.
@@ -129,8 +136,13 @@ func (s *Server) CreateIndex() error {
 	return ioutil.WriteFile(ifile, d, 0755)
 }
 
-func (s *Server) start() {
-	s.srv = httptest.NewServer(http.FileServer(http.Dir(s.docroot)))
+func (s *Server) Start() {
+	s.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if s.middleware != nil {
+			s.middleware.ServeHTTP(w, r)
+		}
+		http.FileServer(http.Dir(s.docroot)).ServeHTTP(w, r)
+	}))
 }
 
 // Stop stops the server and closes all connections.