From e410a03c741c4529bb87b62f6b1b29fcd7149ae0 Mon Sep 17 00:00:00 2001 From: Matthew Fisher Date: Wed, 19 Jun 2019 14:17:37 -0700 Subject: [PATCH] ref(getter): introduce Options for passing in getter parameters instead of hard-coding the parameters being passed in the constructor, we should pass in an Options struct that can be used to pass in those parameters. Signed-off-by: Matthew Fisher --- pkg/action/install.go | 2 +- pkg/downloader/chart_downloader.go | 4 +- pkg/downloader/chart_downloader_test.go | 8 ++- pkg/getter/getter.go | 55 ++++++++++++++++++- pkg/getter/getter_test.go | 6 +- pkg/getter/httpgetter.go | 42 +++++++------- pkg/getter/httpgetter_test.go | 40 +++++++++++++- pkg/getter/plugingetter.go | 20 +++---- pkg/getter/plugingetter_test.go | 2 +- .../testdata/output/httpgetter-servername.txt | 1 + pkg/plugin/installer/http_installer.go | 2 +- pkg/repo/chartrepo.go | 12 +++- 12 files changed, 145 insertions(+), 49 deletions(-) create mode 100644 pkg/getter/testdata/output/httpgetter-servername.txt diff --git a/pkg/action/install.go b/pkg/action/install.go index 1a463e64e..22cd5e5af 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -751,7 +751,7 @@ func readFile(filePath string, settings cli.EnvSettings) ([]byte, error) { return ioutil.ReadFile(filePath) } - getter, err := getterConstructor(filePath, "", "", "") + getter, err := getterConstructor(getter.WithURL(filePath)) if err != nil { return []byte{}, err } diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index be82e5da0..918a0fce9 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -156,7 +156,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, ge } // TODO add user-agent - g, err := getter.NewHTTPGetter(ref, "", "", "") + g, err := getter.NewHTTPGetter(getter.WithURL(ref)) if err != nil { return u, nil, err } @@ -240,7 +240,7 @@ 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(rc.URL, "", "", "") + g, err := getter.NewHTTPGetter(getter.WithURL(rc.URL)) if err != nil { return repoURL, nil, err } diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 615eb4357..1128ac728 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -99,7 +99,7 @@ func TestDownload(t *testing.T) { t.Fatal("No http provider found") } - g, err := provider.New(srv.URL, "", "", "") + g, err := provider.New(getter.WithURL(srv.URL)) if err != nil { t.Fatal(err) } @@ -124,11 +124,13 @@ func TestDownload(t *testing.T) { defer basicAuthSrv.Close() u, _ := url.ParseRequestURI(basicAuthSrv.URL) - httpgetter, err := getter.NewHTTPGetter(u.String(), "", "", "") + httpgetter, err := getter.NewHTTPGetter( + getter.WithURL(u.String()), + getter.WithBasicAuth("username", "password"), + ) if err != nil { t.Fatal(err) } - httpgetter.SetBasicAuth("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 d305395ee..7fba441c1 100644 --- a/pkg/getter/getter.go +++ b/pkg/getter/getter.go @@ -24,6 +24,55 @@ import ( "helm.sh/helm/pkg/cli" ) +// options are generic parameters to be provided to the getter during instantiation. +// +// Getters may or may not ignore these parameters as they are passed in. +type options struct { + url string + certFile string + keyFile string + caFile string + username string + password string + userAgent string +} + +// Option allows specifying various settings configurable by the user for overriding the defaults +// used when performing Get operations with the Getter. +type Option func(*options) + +// WithURL informs the getter the server name that will be used when fetching objects. Used in conjunction with +// WithTLSClientConfig to set the TLSClientConfig's server name. +func WithURL(url string) Option { + return func(opts *options) { + opts.url = url + } +} + +// WithBasicAuth sets the request's Authorization header to use the provided credentials +func WithBasicAuth(username, password string) Option { + return func(opts *options) { + opts.username = username + opts.password = password + } +} + +// WithUserAgent sets the request's User-Agent header to use the provided agent name. +func WithUserAgent(userAgent string) Option { + return func(opts *options) { + opts.userAgent = userAgent + } +} + +// WithTLSClientConfig sets the client client auth with the provided credentials. +func WithTLSClientConfig(certFile, keyFile, caFile string) Option { + return func(opts *options) { + opts.certFile = certFile + opts.keyFile = keyFile + opts.caFile = caFile + } +} + // Getter is an interface to support GET to the specified URL. type Getter interface { //Get file content by url string @@ -32,7 +81,7 @@ type Getter interface { // Constructor is the function for every getter which creates a specific instance // according to the configuration -type Constructor func(URL, CertFile, KeyFile, CAFile string) (Getter, error) +type Constructor func(options ...Option) (Getter, error) // Provider represents any getter and the schemes that it supports. // @@ -69,8 +118,8 @@ func (p Providers) ByScheme(scheme string) (Constructor, error) { } // All finds all of the registered getters as a list of Provider instances. -// Currently the build-in http/https getter and the discovered -// plugins with downloader notations are collected. +// Currently, the built-in getters and the discovered plugins with downloader +// notations are collected. func All(settings cli.EnvSettings) Providers { result := Providers{ { diff --git a/pkg/getter/getter_test.go b/pkg/getter/getter_test.go index d03c82686..41ed1079c 100644 --- a/pkg/getter/getter_test.go +++ b/pkg/getter/getter_test.go @@ -23,7 +23,7 @@ import ( func TestProvider(t *testing.T) { p := Provider{ []string{"one", "three"}, - func(h, e, l, m string) (Getter, error) { return nil, nil }, + func(_ ...Option) (Getter, error) { return nil, nil }, } if !p.Provides("three") { @@ -33,8 +33,8 @@ func TestProvider(t *testing.T) { func TestProviders(t *testing.T) { ps := Providers{ - {[]string{"one", "three"}, func(h, e, l, m string) (Getter, error) { return nil, nil }}, - {[]string{"two", "four"}, func(h, e, l, m string) (Getter, error) { return nil, nil }}, + {[]string{"one", "three"}, func(_ ...Option) (Getter, error) { return nil, nil }}, + {[]string{"two", "four"}, func(_ ...Option) (Getter, error) { return nil, nil }}, } if _, err := ps.ByScheme("one"); err != nil { diff --git a/pkg/getter/httpgetter.go b/pkg/getter/httpgetter.go index b221e42ad..94e182b5b 100644 --- a/pkg/getter/httpgetter.go +++ b/pkg/getter/httpgetter.go @@ -28,21 +28,19 @@ import ( // HTTPGetter is the efault HTTP(/S) backend handler type HTTPGetter struct { - client *http.Client - username string - password string - userAgent string + client *http.Client + opts options } -// SetBasicAuth sets the credentials for the getter +// SetBasicAuth sets the request's Authorization header to use the provided credentials. func (g *HTTPGetter) SetBasicAuth(username, password string) { - g.username = username - g.password = password + g.opts.username = username + g.opts.password = password } -// SetUserAgent sets the HTTP User-Agent for the getter +// SetUserAgent sets the request's User-Agent header to use the provided agent name. func (g *HTTPGetter) SetUserAgent(userAgent string) { - g.userAgent = userAgent + g.opts.userAgent = userAgent } //Get performs a Get from repo.Getter and returns the body. @@ -60,12 +58,12 @@ func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { return buf, err } // req.Header.Set("User-Agent", "Helm/"+strings.TrimPrefix(version.GetVersion(), "v")) - if g.userAgent != "" { - req.Header.Set("User-Agent", g.userAgent) + if g.opts.userAgent != "" { + req.Header.Set("User-Agent", g.opts.userAgent) } - if g.username != "" && g.password != "" { - req.SetBasicAuth(g.username, g.password) + if g.opts.username != "" && g.opts.password != "" { + req.SetBasicAuth(g.opts.username, g.opts.password) } resp, err := g.client.Do(req) @@ -82,21 +80,26 @@ func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { } // newHTTPGetter constructs a valid http/https client as Getter -func newHTTPGetter(url, certFile, keyFile, caFile string) (Getter, error) { - return NewHTTPGetter(url, certFile, keyFile, caFile) +func newHTTPGetter(options ...Option) (Getter, error) { + return NewHTTPGetter(options...) } // NewHTTPGetter constructs a valid http/https client as HTTPGetter -func NewHTTPGetter(url, certFile, keyFile, caFile string) (*HTTPGetter, error) { +func NewHTTPGetter(options ...Option) (*HTTPGetter, error) { var client HTTPGetter - if certFile != "" && keyFile != "" { - tlsConf, err := tlsutil.NewClientTLS(certFile, keyFile, caFile) + + for _, opt := range options { + opt(&client.opts) + } + + if client.opts.certFile != "" && client.opts.keyFile != "" { + tlsConf, err := tlsutil.NewClientTLS(client.opts.certFile, client.opts.keyFile, client.opts.caFile) if err != nil { return &client, errors.Wrap(err, "can't create TLS config for client") } tlsConf.BuildNameToCertificate() - sni, err := urlutil.ExtractHostname(url) + sni, err := urlutil.ExtractHostname(client.opts.url) if err != nil { return &client, err } @@ -111,5 +114,6 @@ func NewHTTPGetter(url, certFile, keyFile, caFile string) (*HTTPGetter, error) { } else { client.client = http.DefaultClient } + return &client, nil } diff --git a/pkg/getter/httpgetter_test.go b/pkg/getter/httpgetter_test.go index 90bd0612b..5f964582d 100644 --- a/pkg/getter/httpgetter_test.go +++ b/pkg/getter/httpgetter_test.go @@ -19,10 +19,12 @@ import ( "net/http" "path/filepath" "testing" + + "helm.sh/helm/internal/test" ) func TestHTTPGetter(t *testing.T) { - g, err := newHTTPGetter("http://example.com", "", "", "") + g, err := newHTTPGetter(WithURL("http://example.com")) if err != nil { t.Fatal(err) } @@ -37,12 +39,44 @@ func TestHTTPGetter(t *testing.T) { cd := "../../testdata" join := filepath.Join ca, pub, priv := join(cd, "ca.pem"), join(cd, "crt.pem"), join(cd, "key.pem") - g, err = newHTTPGetter("http://example.com/", pub, priv, ca) + g, err = newHTTPGetter( + WithURL("http://example.com"), + WithTLSClientConfig(pub, priv, ca), + ) if err != nil { t.Fatal(err) } - if _, ok := g.(*HTTPGetter); !ok { + hg, ok := g.(*HTTPGetter) + if !ok { t.Fatal("Expected newHTTPGetter to produce an httpGetter") } + + transport, ok := hg.client.Transport.(*http.Transport) + if !ok { + t.Errorf("Expected newHTTPGetter to set up an HTTP transport") + } + + test.AssertGoldenString(t, transport.TLSClientConfig.ServerName, "output/httpgetter-servername.txt") + + // Test other options + hg, err = NewHTTPGetter( + WithBasicAuth("I", "Am"), + WithUserAgent("Groot"), + ) + if err != nil { + t.Fatal(err) + } + + if hg.opts.username != "I" { + t.Errorf("Expected NewHTTPGetter to contain %q as the username, got %q", "I", hg.opts.username) + } + + if hg.opts.password != "Am" { + t.Errorf("Expected NewHTTPGetter to contain %q as the password, got %q", "Am", hg.opts.password) + } + + if hg.opts.userAgent != "Groot" { + t.Errorf("Expected NewHTTPGetter to contain %q as the user agent, got %q", "Groot", hg.opts.userAgent) + } } diff --git a/pkg/getter/plugingetter.go b/pkg/getter/plugingetter.go index c3a88297b..fff5193b2 100644 --- a/pkg/getter/plugingetter.go +++ b/pkg/getter/plugingetter.go @@ -54,16 +54,16 @@ func collectPlugins(settings cli.EnvSettings) (Providers, error) { // pluginGetter is a generic type to invoke custom downloaders, // implemented in plugins. type pluginGetter struct { - command string - certFile, keyFile, cAFile string - settings cli.EnvSettings - name string - base string + command string + settings cli.EnvSettings + name string + base string + opts options } // Get runs downloader plugin command func (p *pluginGetter) Get(href string) (*bytes.Buffer, error) { - argv := []string{p.certFile, p.keyFile, p.cAFile, href} + argv := []string{p.opts.certFile, p.opts.keyFile, p.opts.caFile, href} prog := exec.Command(filepath.Join(p.base, p.command), argv...) plugin.SetupPluginEnv(p.settings, p.name, p.base) prog.Env = os.Environ() @@ -82,16 +82,16 @@ func (p *pluginGetter) Get(href string) (*bytes.Buffer, error) { // newPluginGetter constructs a valid plugin getter func newPluginGetter(command string, settings cli.EnvSettings, name, base string) Constructor { - return func(URL, CertFile, KeyFile, CAFile string) (Getter, error) { + return func(options ...Option) (Getter, error) { result := &pluginGetter{ command: command, - certFile: CertFile, - keyFile: KeyFile, - cAFile: CAFile, settings: settings, name: name, base: base, } + for _, opt := range options { + opt(&result.opts) + } return result, nil } } diff --git a/pkg/getter/plugingetter_test.go b/pkg/getter/plugingetter_test.go index 470515dae..8c3ec6e51 100644 --- a/pkg/getter/plugingetter_test.go +++ b/pkg/getter/plugingetter_test.go @@ -78,7 +78,7 @@ func TestPluginGetter(t *testing.T) { env := hh(false) pg := newPluginGetter("echo", env, "test", ".") - g, err := pg("test://foo/bar", "", "", "") + g, err := pg() if err != nil { t.Fatal(err) } diff --git a/pkg/getter/testdata/output/httpgetter-servername.txt b/pkg/getter/testdata/output/httpgetter-servername.txt new file mode 100644 index 000000000..caa12a8fb --- /dev/null +++ b/pkg/getter/testdata/output/httpgetter-servername.txt @@ -0,0 +1 @@ +example.com \ No newline at end of file diff --git a/pkg/plugin/installer/http_installer.go b/pkg/plugin/installer/http_installer.go index e3789bb37..80ade67e0 100644 --- a/pkg/plugin/installer/http_installer.go +++ b/pkg/plugin/installer/http_installer.go @@ -84,7 +84,7 @@ func NewHTTPInstaller(source string, home helmpath.Home) (*HTTPInstaller, error) return nil, err } - get, err := getConstructor.New(source, "", "", "") + get, err := getConstructor.New(getter.WithURL(source)) if err != nil { return nil, err } diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index 745a76489..0cf5851cb 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -63,7 +63,10 @@ func NewChartRepository(cfg *Entry, getters getter.Providers) (*ChartRepository, if err != nil { return nil, errors.Errorf("could not find protocol handler for: %s", u.Scheme) } - client, err := getterConstructor(cfg.URL, cfg.CertFile, cfg.KeyFile, cfg.CAFile) + client, err := getterConstructor( + getter.WithURL(cfg.URL), + getter.WithTLSClientConfig(cfg.CertFile, cfg.KeyFile, cfg.CAFile), + ) if err != nil { return nil, errors.Wrapf(err, "could not construct protocol handler for: %s", u.Scheme) } @@ -121,11 +124,14 @@ func (r *ChartRepository) DownloadIndexFile(cachePath string) error { indexURL = parsedURL.String() // TODO add user-agent - g, err := getter.NewHTTPGetter(indexURL, r.Config.CertFile, r.Config.KeyFile, r.Config.CAFile) + g, err := getter.NewHTTPGetter( + getter.WithURL(indexURL), + getter.WithTLSClientConfig(r.Config.CertFile, r.Config.KeyFile, r.Config.CAFile), + getter.WithBasicAuth(r.Config.Username, r.Config.Password), + ) if err != nil { return err } - g.SetBasicAuth(r.Config.Username, r.Config.Password) resp, err := g.Get(indexURL) if err != nil { return err