From 7ea1d1df66c56956f9b473035df0ada43bbc2502 Mon Sep 17 00:00:00 2001 From: George Jenkins Date: Fri, 27 Dec 2024 22:07:27 -0800 Subject: [PATCH 1/2] refactor: Remove duplicate `FindChartIn*RepoURL` funcs Signed-off-by: George Jenkins --- pkg/action/install.go | 14 +++++++-- pkg/action/pull.go | 13 +++++++- pkg/repo/chartrepo.go | 62 +++++++++++++++++++++++--------------- pkg/repo/chartrepo_test.go | 13 ++++++-- 4 files changed, 72 insertions(+), 30 deletions(-) diff --git a/pkg/action/install.go b/pkg/action/install.go index ec074a8d2..c90746c13 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -789,8 +789,18 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( dl.Verify = downloader.VerifyAlways } if c.RepoURL != "" { - chartURL, err := repo.FindChartInAuthAndTLSAndPassRepoURL(c.RepoURL, c.Username, c.Password, name, version, - c.CertFile, c.KeyFile, c.CaFile, c.InsecureSkipTLSverify, c.PassCredentialsAll, getter.All(settings)) + chartURL, err := repo.FindChartInRepoURL( + c.RepoURL, + name, + version, + c.CertFile, + c.KeyFile, + c.CaFile, + getter.All(settings), + repo.WithUsernamePassword(c.Username, c.Password), + repo.WithInsecureSkipTLSverify(c.InsecureSkipTLSverify), + repo.WithPassCredentialsAll(c.PassCredentialsAll), + ) if err != nil { return "", err } diff --git a/pkg/action/pull.go b/pkg/action/pull.go index 63bc11d3b..a09cbe368 100644 --- a/pkg/action/pull.go +++ b/pkg/action/pull.go @@ -117,7 +117,18 @@ func (p *Pull) Run(chartRef string) (string, error) { } if p.RepoURL != "" { - chartURL, err := repo.FindChartInAuthAndTLSAndPassRepoURL(p.RepoURL, p.Username, p.Password, chartRef, p.Version, p.CertFile, p.KeyFile, p.CaFile, p.InsecureSkipTLSverify, p.PassCredentialsAll, getter.All(p.Settings)) + chartURL, err := repo.FindChartInRepoURL( + p.RepoURL, + chartRef, + p.Version, + p.CertFile, + p.KeyFile, + p.CaFile, + getter.All(p.Settings), + repo.WithUsernamePassword(p.Username, p.Password), + repo.WithInsecureSkipTLSverify(p.InsecureSkipTLSverify), + repo.WithPassCredentialsAll(p.PassCredentialsAll), + ) if err != nil { return out.String(), err } diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index e20c7e20f..33f790601 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -196,33 +196,45 @@ func (r *ChartRepository) generateIndex() error { return nil } -// FindChartInRepoURL finds chart in chart repository pointed by repoURL -// without adding repo to repositories -func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) { - return FindChartInAuthRepoURL(repoURL, "", "", chartName, chartVersion, certFile, keyFile, caFile, getters) +type findChartInRepoURLOptions struct { + Username string + Password string + PassCredentialsAll bool + InsecureSkipTLSverify bool +} + +type FindChartInRepoURLOption func(*findChartInRepoURLOptions) + +// WithUsernamePassword specifies the username/password credntials for the repository +func WithUsernamePassword(username, password string) FindChartInRepoURLOption { + return func(options *findChartInRepoURLOptions) { + options.Username = username + options.Password = password + } } -// FindChartInAuthRepoURL finds chart in chart repository pointed by repoURL -// without adding repo to repositories, like FindChartInRepoURL, -// but it also receives credentials for the chart repository. -func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) { - return FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile, false, getters) +// WithPassCredentialsAll flags whether credentials should be passed on to other domains +func WithPassCredentialsAll(passCredentialsAll bool) FindChartInRepoURLOption { + return func(options *findChartInRepoURLOptions) { + options.PassCredentialsAll = passCredentialsAll + } } -// FindChartInAuthAndTLSRepoURL finds chart in chart repository pointed by repoURL -// without adding repo to repositories, like FindChartInRepoURL, -// but it also receives credentials and TLS verify flag for the chart repository. -// TODO Helm 4, FindChartInAuthAndTLSRepoURL should be integrated into FindChartInAuthRepoURL. -func FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, insecureSkipTLSverify bool, getters getter.Providers) (string, error) { - return FindChartInAuthAndTLSAndPassRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile, insecureSkipTLSverify, false, getters) +// WithInsecureSkipTLSverify skips TLS verification for repostory communication +func WithInsecureSkipTLSverify(insecureSkipTLSverify bool) FindChartInRepoURLOption { + return func(options *findChartInRepoURLOptions) { + options.InsecureSkipTLSverify = insecureSkipTLSverify + } } -// FindChartInAuthAndTLSAndPassRepoURL finds chart in chart repository pointed by repoURL -// without adding repo to repositories, like FindChartInRepoURL, -// but it also receives credentials, TLS verify flag, and if credentials should -// be passed on to other domains. -// TODO Helm 4, FindChartInAuthAndTLSAndPassRepoURL should be integrated into FindChartInAuthRepoURL. -func FindChartInAuthAndTLSAndPassRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, insecureSkipTLSverify, passCredentialsAll bool, getters getter.Providers) (string, error) { +// FindChartInRepoURL finds chart in chart repository pointed by repoURL +// without adding repo to repositories +func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, error) { + + opts := findChartInRepoURLOptions{} + for _, option := range options { + option(&opts) + } // Download and write the index file to a temporary location buf := make([]byte, 20) @@ -231,14 +243,14 @@ func FindChartInAuthAndTLSAndPassRepoURL(repoURL, username, password, chartName, c := Entry{ URL: repoURL, - Username: username, - Password: password, - PassCredentialsAll: passCredentialsAll, + Username: opts.Username, + Password: opts.Password, + PassCredentialsAll: opts.PassCredentialsAll, CertFile: certFile, KeyFile: keyFile, CAFile: caFile, Name: name, - InsecureSkipTLSverify: insecureSkipTLSverify, + InsecureSkipTLSverify: opts.InsecureSkipTLSverify, } r, err := NewChartRepository(&c, getters) if err != nil { diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go index 341bafadc..836a2d66f 100644 --- a/pkg/repo/chartrepo_test.go +++ b/pkg/repo/chartrepo_test.go @@ -297,7 +297,16 @@ func TestFindChartInAuthAndTLSAndPassRepoURL(t *testing.T) { } defer srv.Close() - chartURL, err := FindChartInAuthAndTLSAndPassRepoURL(srv.URL, "", "", "nginx", "", "", "", "", true, false, getter.All(&cli.EnvSettings{})) + chartURL, err := FindChartInRepoURL( + srv.URL, + "nginx", + "", + "", + "", + "", + getter.All(&cli.EnvSettings{}), + WithInsecureSkipTLSverify(true), + ) if err != nil { t.Fatalf("%v", err) } @@ -306,7 +315,7 @@ func TestFindChartInAuthAndTLSAndPassRepoURL(t *testing.T) { } // If the insecureSkipTLSVerify is false, it will return an error that contains "x509: certificate signed by unknown authority". - _, err = FindChartInAuthAndTLSAndPassRepoURL(srv.URL, "", "", "nginx", "0.1.0", "", "", "", false, false, getter.All(&cli.EnvSettings{})) + _, err = FindChartInRepoURL(srv.URL, "nginx", "0.1.0", "", "", "", getter.All(&cli.EnvSettings{})) // Go communicates with the platform and different platforms return different messages. Go itself tests darwin // differently for its message. On newer versions of Darwin the message includes the "Acme Co" portion while older // versions of Darwin do not. As there are people developing Helm using both old and new versions of Darwin we test From 0ce267d907dd776b0cf5ca3f2406f5b5ecce0832 Mon Sep 17 00:00:00 2001 From: George Jenkins Date: Fri, 24 Jan 2025 20:38:12 -0800 Subject: [PATCH 2/2] more options Signed-off-by: George Jenkins --- pkg/action/install.go | 6 ++---- pkg/action/pull.go | 6 ++---- pkg/downloader/manager.go | 2 +- pkg/repo/chartrepo.go | 34 +++++++++++++++++++++++++++------- pkg/repo/chartrepo_test.go | 18 +++++++----------- 5 files changed, 39 insertions(+), 27 deletions(-) diff --git a/pkg/action/install.go b/pkg/action/install.go index c90746c13..ef3f0fdc7 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -792,11 +792,9 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( chartURL, err := repo.FindChartInRepoURL( c.RepoURL, name, - version, - c.CertFile, - c.KeyFile, - c.CaFile, getter.All(settings), + repo.WithChartVersion(version), + repo.WithClientTLS(c.CertFile, c.KeyFile, c.CaFile), repo.WithUsernamePassword(c.Username, c.Password), repo.WithInsecureSkipTLSverify(c.InsecureSkipTLSverify), repo.WithPassCredentialsAll(c.PassCredentialsAll), diff --git a/pkg/action/pull.go b/pkg/action/pull.go index a09cbe368..7fc1a9fdb 100644 --- a/pkg/action/pull.go +++ b/pkg/action/pull.go @@ -120,11 +120,9 @@ func (p *Pull) Run(chartRef string) (string, error) { chartURL, err := repo.FindChartInRepoURL( p.RepoURL, chartRef, - p.Version, - p.CertFile, - p.KeyFile, - p.CaFile, getter.All(p.Settings), + repo.WithChartVersion(p.Version), + repo.WithClientTLS(p.CertFile, p.KeyFile, p.CaFile), repo.WithUsernamePassword(p.Username, p.Password), repo.WithInsecureSkipTLSverify(p.InsecureSkipTLSverify), repo.WithPassCredentialsAll(p.PassCredentialsAll), diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 5a509646d..f10cbfb8d 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -742,7 +742,7 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* return } } - url, err = repo.FindChartInRepoURL(repoURL, name, version, certFile, keyFile, caFile, m.Getters) + url, err = repo.FindChartInRepoURL(repoURL, name, m.Getters, repo.WithChartVersion(version), repo.WithClientTLS(certFile, keyFile, caFile)) if err == nil { return url, username, password, false, false, "", "", "", err } diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index 33f790601..3629bd24b 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -201,10 +201,21 @@ type findChartInRepoURLOptions struct { Password string PassCredentialsAll bool InsecureSkipTLSverify bool + CertFile string + KeyFile string + CAFile string + ChartVersion string } type FindChartInRepoURLOption func(*findChartInRepoURLOptions) +// WithChartVersion specifies the chart version to find +func WithChartVersion(chartVersion string) FindChartInRepoURLOption { + return func(options *findChartInRepoURLOptions) { + options.ChartVersion = chartVersion + } +} + // WithUsernamePassword specifies the username/password credntials for the repository func WithUsernamePassword(username, password string) FindChartInRepoURLOption { return func(options *findChartInRepoURLOptions) { @@ -220,6 +231,15 @@ func WithPassCredentialsAll(passCredentialsAll bool) FindChartInRepoURLOption { } } +// WithClientTLS species the cert, key, and CA files for client mTLS +func WithClientTLS(certFile, keyFile, caFile string) FindChartInRepoURLOption { + return func(options *findChartInRepoURLOptions) { + options.CertFile = certFile + options.KeyFile = keyFile + options.CAFile = caFile + } +} + // WithInsecureSkipTLSverify skips TLS verification for repostory communication func WithInsecureSkipTLSverify(insecureSkipTLSverify bool) FindChartInRepoURLOption { return func(options *findChartInRepoURLOptions) { @@ -229,7 +249,7 @@ func WithInsecureSkipTLSverify(insecureSkipTLSverify bool) FindChartInRepoURLOpt // FindChartInRepoURL finds chart in chart repository pointed by repoURL // without adding repo to repositories -func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, error) { +func FindChartInRepoURL(repoURL string, chartName string, getters getter.Providers, options ...FindChartInRepoURLOption) (string, error) { opts := findChartInRepoURLOptions{} for _, option := range options { @@ -246,9 +266,9 @@ func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caF Username: opts.Username, Password: opts.Password, PassCredentialsAll: opts.PassCredentialsAll, - CertFile: certFile, - KeyFile: keyFile, - CAFile: caFile, + CertFile: opts.CertFile, + KeyFile: opts.KeyFile, + CAFile: opts.CAFile, Name: name, InsecureSkipTLSverify: opts.InsecureSkipTLSverify, } @@ -272,10 +292,10 @@ func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caF } errMsg := fmt.Sprintf("chart %q", chartName) - if chartVersion != "" { - errMsg = fmt.Sprintf("%s version %q", errMsg, chartVersion) + if opts.ChartVersion != "" { + errMsg = fmt.Sprintf("%s version %q", errMsg, opts.ChartVersion) } - cv, err := repoIndex.Get(chartName, chartVersion) + cv, err := repoIndex.Get(chartName, opts.ChartVersion) if err != nil { return "", errors.Errorf("%s not found in %s repository", errMsg, repoURL) } diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go index 836a2d66f..e3330b8eb 100644 --- a/pkg/repo/chartrepo_test.go +++ b/pkg/repo/chartrepo_test.go @@ -300,10 +300,6 @@ func TestFindChartInAuthAndTLSAndPassRepoURL(t *testing.T) { chartURL, err := FindChartInRepoURL( srv.URL, "nginx", - "", - "", - "", - "", getter.All(&cli.EnvSettings{}), WithInsecureSkipTLSverify(true), ) @@ -315,7 +311,7 @@ func TestFindChartInAuthAndTLSAndPassRepoURL(t *testing.T) { } // If the insecureSkipTLSVerify is false, it will return an error that contains "x509: certificate signed by unknown authority". - _, err = FindChartInRepoURL(srv.URL, "nginx", "0.1.0", "", "", "", getter.All(&cli.EnvSettings{})) + _, err = FindChartInRepoURL(srv.URL, "nginx", getter.All(&cli.EnvSettings{}), WithChartVersion("0.1.0")) // Go communicates with the platform and different platforms return different messages. Go itself tests darwin // differently for its message. On newer versions of Darwin the message includes the "Acme Co" portion while older // versions of Darwin do not. As there are people developing Helm using both old and new versions of Darwin we test @@ -336,7 +332,7 @@ func TestFindChartInRepoURL(t *testing.T) { } defer srv.Close() - chartURL, err := FindChartInRepoURL(srv.URL, "nginx", "", "", "", "", getter.All(&cli.EnvSettings{})) + chartURL, err := FindChartInRepoURL(srv.URL, "nginx", getter.All(&cli.EnvSettings{})) if err != nil { t.Fatalf("%v", err) } @@ -344,7 +340,7 @@ func TestFindChartInRepoURL(t *testing.T) { t.Errorf("%s is not the valid URL", chartURL) } - chartURL, err = FindChartInRepoURL(srv.URL, "nginx", "0.1.0", "", "", "", getter.All(&cli.EnvSettings{})) + chartURL, err = FindChartInRepoURL(srv.URL, "nginx", getter.All(&cli.EnvSettings{}), WithChartVersion("0.1.0")) if err != nil { t.Errorf("%s", err) } @@ -359,7 +355,7 @@ func TestErrorFindChartInRepoURL(t *testing.T) { RepositoryCache: t.TempDir(), }) - if _, err := FindChartInRepoURL("http://someserver/something", "nginx", "", "", "", "", g); err == nil { + if _, err := FindChartInRepoURL("http://someserver/something", "nginx", g); err == nil { t.Errorf("Expected error for bad chart URL, but did not get any errors") } else if !strings.Contains(err.Error(), `looks like "http://someserver/something" is not a valid chart repository or cannot be reached`) { t.Errorf("Expected error for bad chart URL, but got a different error (%v)", err) @@ -371,19 +367,19 @@ func TestErrorFindChartInRepoURL(t *testing.T) { } defer srv.Close() - if _, err = FindChartInRepoURL(srv.URL, "nginx1", "", "", "", "", g); err == nil { + if _, err = FindChartInRepoURL(srv.URL, "nginx1", g); err == nil { t.Errorf("Expected error for chart not found, but did not get any errors") } else if err.Error() != `chart "nginx1" not found in `+srv.URL+` repository` { t.Errorf("Expected error for chart not found, but got a different error (%v)", err) } - if _, err = FindChartInRepoURL(srv.URL, "nginx1", "0.1.0", "", "", "", g); err == nil { + if _, err = FindChartInRepoURL(srv.URL, "nginx1", g, WithChartVersion("0.1.0")); err == nil { t.Errorf("Expected error for chart not found, but did not get any errors") } else if err.Error() != `chart "nginx1" version "0.1.0" not found in `+srv.URL+` repository` { t.Errorf("Expected error for chart not found, but got a different error (%v)", err) } - if _, err = FindChartInRepoURL(srv.URL, "chartWithNoURL", "", "", "", "", g); err == nil { + if _, err = FindChartInRepoURL(srv.URL, "chartWithNoURL", g); err == nil { t.Errorf("Expected error for no chart URLs available, but did not get any errors") } else if err.Error() != `chart "chartWithNoURL" has no downloadable URLs` { t.Errorf("Expected error for chart not found, but got a different error (%v)", err)