From 4a4abc96439f9e51216f7fc8751266dd186821bc Mon Sep 17 00:00:00 2001 From: eyalbe4 Date: Thu, 1 Mar 2018 16:37:24 +0200 Subject: [PATCH] Removed GetWithCredentials API. --- pkg/downloader/chart_downloader.go | 87 ++++++++++++--------- pkg/downloader/chart_downloader_test.go | 11 ++- pkg/getter/getter.go | 2 - pkg/getter/httpgetter.go | 35 +++++---- pkg/getter/httpgetter_test.go | 4 +- pkg/getter/plugingetter.go | 4 - pkg/plugin/installer/http_installer_test.go | 4 - pkg/repo/chartrepo.go | 8 +- 8 files changed, 87 insertions(+), 68 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 87c80db0c..9fe89820e 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -85,25 +85,12 @@ 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, r, err := c.ResolveChartVersionAndGetRepo(ref, version) + u, r, g, err := c.ResolveChartVersionAndGetRepo(ref, version) if err != nil { return "", nil, err } - // If this ChartDownloader is not configured to use credentials, and the chart repository is, - // then we should use the repository's configured credentials. - username := c.Username - password := c.Password - if r.Config != nil { - if username == "" { - username = r.Config.Username - } - if password == "" { - password = r.Config.Password - } - } - - data, err := r.Client.GetWithCredentials(u.String(), username, password) + data, err := g.Get(u.String()) if err != nil { return "", nil, err } @@ -157,7 +144,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven // * 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) { - u, r, err := c.ResolveChartVersionAndGetRepo(ref, version) + u, r, _, err := c.ResolveChartVersionAndGetRepo(ref, version) if r != nil { return u, r.Client, err } @@ -165,16 +152,22 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, ge } // Same as the ResolveChartVersion method, but returns the chart repositoryy. -func (c *ChartDownloader) ResolveChartVersionAndGetRepo(ref, version string) (*url.URL, *repo.ChartRepository, error) { +func (c *ChartDownloader) ResolveChartVersionAndGetRepo(ref, version string) (*url.URL, *repo.ChartRepository, *getter.HttpGetter, error) { u, err := url.Parse(ref) if err != nil { - return nil, nil, fmt.Errorf("invalid chart URL format: %s", ref) + return nil, nil, nil, fmt.Errorf("invalid chart URL format: %s", ref) } rf, err := repo.LoadRepositoriesFile(c.HelmHome.RepositoryFile()) if err != nil { - return u, nil, err + return u, nil, nil, err + } + + g, err := getter.NewHTTPGetter(ref, "", "", "") + if err != nil { + return u, nil, nil, err } + g.SetCredentials(c.getRepoCredentials(nil)) if u.IsAbs() && len(u.Host) > 0 && len(u.Path) > 0 { // In this case, we have to find the parent repo that contains this chart @@ -182,81 +175,101 @@ func (c *ChartDownloader) ResolveChartVersionAndGetRepo(ref, version string) (*u // through each repo cache file and finding a matching URL. But basically // we want to find the repo in case we have special SSL cert config // for that repo. + rc, err := c.scanReposForURL(ref, rf) if err != nil { // If there is no special config, return the default HTTP client and // swallow the error. if err == ErrNoOwnerRepo { - getterConstructor, err := c.Getters.ByScheme(u.Scheme) - if err != nil { - return u, nil, err - } r := &repo.ChartRepository{} - r.Client, err = getterConstructor(ref, "", "", "") - return u, r, err + r.Client = g + g.SetCredentials(c.getRepoCredentials(r)) + return u, r, g, err } - return u, nil, err + return u, nil, nil, 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, err + return u, r, g, err } // See if it's of the form: repo/path_to_chart p := strings.SplitN(u.Path, "/", 2) if len(p) < 2 { - return u, nil, fmt.Errorf("Non-absolute URLs should be in form of repo_name/path_to_chart, got: %s", u) + return u, nil, nil, fmt.Errorf("Non-absolute URLs should be in form of repo_name/path_to_chart, got: %s", u) } repoName := p[0] chartName := p[1] rc, err := pickChartRepositoryConfigByName(repoName, rf.Repositories) if err != nil { - return u, nil, err + return u, nil, nil, err } r, err := repo.NewChartRepository(rc, c.Getters) if err != nil { - return u, nil, err + return u, nil, nil, err } // 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, r, fmt.Errorf("no cached repo found. (try 'helm repo update'). %s", err) + return u, r, g, fmt.Errorf("no cached repo found. (try 'helm repo update'). %s", err) } cv, err := i.Get(chartName, version) if err != nil { - return u, r, fmt.Errorf("chart %q matching %s not found in %s index. (try 'helm repo update'). %s", chartName, version, r.Config.Name, err) + return u, r, g, fmt.Errorf("chart %q matching %s not found in %s index. (try 'helm repo update'). %s", chartName, version, r.Config.Name, err) } if len(cv.URLs) == 0 { - return u, r, fmt.Errorf("chart %q has no downloadable URLs", ref) + return u, r, g, fmt.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, r, fmt.Errorf("invalid chart URL format: %s", ref) + return u, r, g, fmt.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, r, err + return repoURL, r, nil, err } q := repoURL.Query() // We need a trailing slash for ResolveReference to work, but make sure there isn't already one repoURL.Path = strings.TrimSuffix(repoURL.Path, "/") + "/" u = repoURL.ResolveReference(u) u.RawQuery = q.Encode() - return u, r, err + g, err := getter.NewHTTPGetter(rc.URL, "", "", "") + if err != nil { + return repoURL, r, nil, err + } + g.SetCredentials(c.getRepoCredentials(r)) + return u, r, g, err } - return u, r, nil + return u, r, 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 + } + } + return } // VerifyChart takes a path to a chart archive and a keyring, and verifies the chart. diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index ca691cd66..80efa77e8 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -99,11 +99,11 @@ func TestDownload(t *testing.T) { t.Fatal("No http provider found") } - getter, err := provider.New(srv.URL, "", "", "") + g, err := provider.New(srv.URL, "", "", "") if err != nil { t.Fatal(err) } - got, err := getter.Get(srv.URL) + got, err := g.Get(srv.URL) if err != nil { t.Fatal(err) } @@ -124,7 +124,12 @@ func TestDownload(t *testing.T) { defer basicAuthSrv.Close() u, _ := url.ParseRequestURI(basicAuthSrv.URL) - got, err = getter.GetWithCredentials(u.String(), "username", "password") + httpgetter, err := getter.NewHTTPGetter(u.String(), "", "", "") + if err != nil { + t.Fatal(err) + } + httpgetter.SetCredentials("username", "password") + got, err = httpgetter.Get(u.String()) if err != nil { t.Fatal(err) } diff --git a/pkg/getter/getter.go b/pkg/getter/getter.go index de9d59578..ca018884a 100644 --- a/pkg/getter/getter.go +++ b/pkg/getter/getter.go @@ -27,8 +27,6 @@ import ( type Getter interface { //Get file content by url string Get(url string) (*bytes.Buffer, error) - //Get file content by url, username and password strings - GetWithCredentials(href, username, password string) (*bytes.Buffer, error) } // Constructor is the function for every getter which creates a specific instance diff --git a/pkg/getter/httpgetter.go b/pkg/getter/httpgetter.go index 5748ae9f7..dd462ce5f 100644 --- a/pkg/getter/httpgetter.go +++ b/pkg/getter/httpgetter.go @@ -28,21 +28,23 @@ import ( ) //httpGetter is the efault HTTP(/S) backend handler -type httpGetter struct { - client *http.Client +type HttpGetter struct { + client *http.Client + username string + password string } -//Get performs a Get from repo.Getter and returns the body. -func (g *httpGetter) Get(href string) (*bytes.Buffer, error) { - return g.get(href, "", "") +func (g *HttpGetter) SetCredentials(username, password string) { + g.username = username + g.password = password } -//Get performs a Get from repo.Getter using credentials and returns the body. -func (g *httpGetter) GetWithCredentials(href, username, password string) (*bytes.Buffer, error) { - return g.get(href, username, password) +//Get performs a Get from repo.Getter and returns the body. +func (g *HttpGetter) Get(href string) (*bytes.Buffer, error) { + return g.get(href) } -func (g *httpGetter) get(href, username, password string) (*bytes.Buffer, error) { +func (g *HttpGetter) get(href string) (*bytes.Buffer, error) { buf := bytes.NewBuffer(nil) // Set a helm specific user agent so that a repo server and metrics can @@ -53,8 +55,8 @@ func (g *httpGetter) get(href, username, password string) (*bytes.Buffer, error) } req.Header.Set("User-Agent", "Helm/"+strings.TrimPrefix(version.GetVersion(), "v")) - if username != "" && password != "" { - req.SetBasicAuth(username, password) + if g.username != "" && g.password != "" { + req.SetBasicAuth(g.username, g.password) } resp, err := g.client.Do(req) @@ -72,17 +74,22 @@ func (g *httpGetter) get(href, username, password string) (*bytes.Buffer, error) // newHTTPGetter constructs a valid http/https client as Getter func newHTTPGetter(URL, CertFile, KeyFile, CAFile string) (Getter, error) { - var client httpGetter + return NewHTTPGetter(URL, CertFile, KeyFile, CAFile) +} + +// NewHTTPGetter constructs a valid http/https client as HttpGetter +func NewHTTPGetter(URL, CertFile, KeyFile, CAFile string) (*HttpGetter, error) { + var client HttpGetter if CertFile != "" && KeyFile != "" { tlsConf, err := tlsutil.NewClientTLS(CertFile, KeyFile, CAFile) if err != nil { - return nil, fmt.Errorf("can't create TLS config for client: %s", err.Error()) + return &client, fmt.Errorf("can't create TLS config for client: %s", err.Error()) } tlsConf.BuildNameToCertificate() sni, err := urlutil.ExtractHostname(URL) if err != nil { - return nil, err + return &client, err } tlsConf.ServerName = sni diff --git a/pkg/getter/httpgetter_test.go b/pkg/getter/httpgetter_test.go index 477e0bc4f..fe3fde22a 100644 --- a/pkg/getter/httpgetter_test.go +++ b/pkg/getter/httpgetter_test.go @@ -27,7 +27,7 @@ func TestHTTPGetter(t *testing.T) { t.Fatal(err) } - if hg, ok := g.(*httpGetter); !ok { + if hg, ok := g.(*HttpGetter); !ok { t.Fatal("Expected newHTTPGetter to produce an httpGetter") } else if hg.client != http.DefaultClient { t.Fatal("Expected newHTTPGetter to return a default HTTP client.") @@ -42,7 +42,7 @@ func TestHTTPGetter(t *testing.T) { t.Fatal(err) } - if _, ok := g.(*httpGetter); !ok { + if _, ok := g.(*HttpGetter); !ok { t.Fatal("Expected newHTTPGetter to produce an httpGetter") } } diff --git a/pkg/getter/plugingetter.go b/pkg/getter/plugingetter.go index edbce131b..c747eef7f 100644 --- a/pkg/getter/plugingetter.go +++ b/pkg/getter/plugingetter.go @@ -79,10 +79,6 @@ func (p *pluginGetter) Get(href string) (*bytes.Buffer, error) { return buf, nil } -func (p *pluginGetter) GetWithCredentials(href, username, password string) (*bytes.Buffer, error) { - return p.Get(href) -} - // newPluginGetter constructs a valid plugin getter func newPluginGetter(command string, settings environment.EnvSettings, name, base string) Constructor { return func(URL, CertFile, KeyFile, CAFile string) (Getter, error) { diff --git a/pkg/plugin/installer/http_installer_test.go b/pkg/plugin/installer/http_installer_test.go index 648b9b0c0..ca1a71e3e 100644 --- a/pkg/plugin/installer/http_installer_test.go +++ b/pkg/plugin/installer/http_installer_test.go @@ -35,10 +35,6 @@ type TestHTTPGetter struct { func (t *TestHTTPGetter) Get(href string) (*bytes.Buffer, error) { return t.MockResponse, t.MockError } -func (t *TestHTTPGetter) GetWithCredentials(href, username, password string) (*bytes.Buffer, error) { - return t.MockResponse, t.MockError -} - // Fake plugin tarball data var fakePluginB64 = "H4sIAKRj51kAA+3UX0vCUBgGcC9jn+Iwuk3Peza3GeyiUlJQkcogCOzgli7dJm4TvYk+a5+k479UqquUCJ/fLs549sLO2TnvWnJa9aXnjwujYdYLovxMhsPcfnHOLdNkOXthM/IVQQYjg2yyLLJ4kXGhLp5j0z3P41tZksqxmspL3B/O+j/XtZu1y8rdYzkOZRCxduKPk53ny6Wwz/GfIIf1As8lxzGJSmoHNLJZphKHG4YpTCE0wVk3DULfpSJ3DMMqkj3P5JfMYLdX1Vr9Ie/5E5cstcdC8K04iGLX5HaJuKpWL17F0TCIBi5pf/0pjtLhun5j3f9v6r7wfnI/H0eNp9d1/5P6Gez0vzo7wsoxfrAZbTny/o9k6J8z/VkO/LPlWdC1iVpbEEcq5nmeJ13LEtmbV0k2r2PrOs9PuuNglC5rL1Y5S/syXRQmutaNw1BGnnp8Wq3UG51WvX1da3bKtZtCN/R09DwAAAAAAAAAAAAAAAAAAADAb30AoMczDwAoAAA=" diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index b73dda52f..c76cc7913 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -119,8 +119,12 @@ func (r *ChartRepository) DownloadIndexFile(cachePath string) error { parsedURL.Path = strings.TrimSuffix(parsedURL.Path, "/") + "/index.yaml" indexURL = parsedURL.String() - resp, err := r.Client.GetWithCredentials(indexURL, r.Config.Username, r.Config.Password) - + g, err := getter.NewHTTPGetter(indexURL, r.Config.CertFile, r.Config.KeyFile, r.Config.CAFile) + if err != nil { + return err + } + g.SetCredentials(r.Config.Username, r.Config.Password) + resp, err := g.Get(indexURL) if err != nil { return err }