From 29b4d8c2b272dac26343ffa317e6b5752992db86 Mon Sep 17 00:00:00 2001 From: Chen Winston Date: Tue, 13 Apr 2021 15:06:41 +0800 Subject: [PATCH] Move registry client creation into one place Signed-off-by: Chen Winston --- cmd/helm/chart.go | 31 ++++++++++++++--- cmd/helm/chart_pull.go | 22 +----------- cmd/helm/chart_push.go | 23 ++----------- cmd/helm/install.go | 17 +++++++++ cmd/helm/pull.go | 3 ++ cmd/helm/registry.go | 20 ++++++++--- cmd/helm/root.go | 27 ++++++++++----- cmd/helm/show.go | 3 ++ cmd/helm/template.go | 3 ++ cmd/helm/upgrade.go | 3 ++ internal/experimental/registry/client.go | 27 +++++++++++++++ pkg/action/install.go | 5 ++- pkg/getter/ocigetter.go | 35 +++++++------------ pkg/getter/ocigetter_test.go | 44 +++++++++++++++++++++--- 14 files changed, 174 insertions(+), 89 deletions(-) diff --git a/cmd/helm/chart.go b/cmd/helm/chart.go index adc874cfe..c9d800290 100644 --- a/cmd/helm/chart.go +++ b/cmd/helm/chart.go @@ -29,13 +29,34 @@ This command consists of multiple subcommands to work with the chart cache. The subcommands can be used to push, pull, tag, list, or remove Helm charts. ` +// https://github.com/spf13/cobra/issues/216#issuecomment-703846787 +func callPersistentPreRunE(cmd *cobra.Command, args []string) error { + if parent := cmd.Parent(); parent != nil { + if parent.PersistentPreRunE != nil { + return parent.PersistentPreRunE(parent, args) + } + } + + return nil +} + func newChartCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { cmd := &cobra.Command{ - Use: "chart", - Short: "push, pull, tag, or remove Helm charts", - Long: chartHelp, - Hidden: !FeatureGateOCI.IsEnabled(), - PersistentPreRunE: checkOCIFeatureGate(), + Use: "chart", + Short: "push, pull, tag, or remove Helm charts", + Long: chartHelp, + Hidden: !FeatureGateOCI.IsEnabled(), + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + if err := checkOCIFeatureGate()(cmd, args); err != nil { + return err + } + + if err := callPersistentPreRunE(cmd, args); err != nil { + return err + } + + return nil + }, } cmd.AddCommand( newChartListCmd(cfg, out), diff --git a/cmd/helm/chart_pull.go b/cmd/helm/chart_pull.go index d923a9921..198852981 100644 --- a/cmd/helm/chart_pull.go +++ b/cmd/helm/chart_pull.go @@ -33,8 +33,6 @@ This will store the chart in the local registry cache to be used later. ` func newChartPullCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { - var insecureOpt, plainHTTPOpt bool - var caFile, certFile, keyFile string cmd := &cobra.Command{ Use: "pull [ref]", Short: "pull a chart from remote", @@ -43,30 +41,12 @@ func newChartPullCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { Hidden: !FeatureGateOCI.IsEnabled(), RunE: func(cmd *cobra.Command, args []string) error { ref := args[0] - registryClient, err := registry.NewClient( - registry.ClientOptDebug(settings.Debug), - registry.ClientOptWriter(out), - registry.ClientOptCredentialsFile(settings.RegistryConfig), - registry.ClientOptPlainHTTP(plainHTTPOpt), - registry.ClientOptInsecureSkipVerifyTLS(insecureOpt), - registry.ClientOptCAFile(caFile), - registry.ClientOptCertKeyFiles(certFile, keyFile), - ) - if err != nil { - return err - } - - cfg.RegistryClient = registryClient return action.NewChartPull(cfg).Run(out, ref) }, } f := cmd.Flags() - f.BoolVarP(&insecureOpt, "insecure-skip-tls-verify", "", false, "skip registry tls certificate checks") - f.BoolVarP(&plainHTTPOpt, "plain-http", "", false, "use plain http to connect to the registry instead of https") - f.StringVar(&certFile, "cert-file", "", "identify HTTPS client using this SSL certificate file") - f.StringVar(&keyFile, "key-file", "", "identify HTTPS client using this SSL key file") - f.StringVar(&caFile, "ca-file", "", "verify certificates of HTTPS-enabled registry using this CA bundle") + registry.AddRegistryCmdFlags(f) return cmd } diff --git a/cmd/helm/chart_push.go b/cmd/helm/chart_push.go index 925c7b1c7..d8de6b878 100644 --- a/cmd/helm/chart_push.go +++ b/cmd/helm/chart_push.go @@ -35,8 +35,6 @@ Must first run "helm chart save" or "helm chart pull". ` func newChartPushCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { - var insecureOpt, plainHTTPOpt bool - var caFile, certFile, keyFile string cmd := &cobra.Command{ Use: "push [ref]", Short: "push a chart to remote", @@ -45,29 +43,12 @@ func newChartPushCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { Hidden: !FeatureGateOCI.IsEnabled(), RunE: func(cmd *cobra.Command, args []string) error { ref := args[0] - registryClient, err := registry.NewClient( - registry.ClientOptDebug(settings.Debug), - registry.ClientOptWriter(out), - registry.ClientOptCredentialsFile(settings.RegistryConfig), - registry.ClientOptPlainHTTP(plainHTTPOpt), - registry.ClientOptInsecureSkipVerifyTLS(insecureOpt), - registry.ClientOptCAFile(caFile), - registry.ClientOptCertKeyFiles(certFile, keyFile), - ) - if err != nil { - return err - } - - cfg.RegistryClient = registryClient return action.NewChartPush(cfg).Run(out, ref) }, } f := cmd.Flags() - f.BoolVarP(&insecureOpt, "insecure-skip-tls-verify", "", false, "skip registry tls certificate checks") - f.BoolVarP(&plainHTTPOpt, "plain-http", "", false, "use plain http to connect to the registry instead of https") - f.StringVar(&certFile, "cert-file", "", "identify HTTPS client using this SSL certificate file") - f.StringVar(&keyFile, "key-file", "", "identify HTTPS client using this SSL key file") - f.StringVar(&caFile, "ca-file", "", "verify certificates of HTTPS-enabled registry using this CA bundle") + registry.AddRegistryCmdFlags(f) + return cmd } diff --git a/cmd/helm/install.go b/cmd/helm/install.go index fac2131c1..50bb3831e 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -26,6 +26,7 @@ import ( "github.com/spf13/pflag" "helm.sh/helm/v3/cmd/helm/require" + "helm.sh/helm/v3/internal/experimental/registry" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" @@ -103,6 +104,19 @@ To see the list of chart repositories, use 'helm repo list'. To search for charts in a repository, use 'helm search'. ` +func preRunEWithChartPathOptions(cmd *cobra.Command, args []string, cp *action.ChartPathOptions) error { + registry.InsecureOpt = cp.InsecureSkipTLSverify + registry.CAFileOpt = cp.CaFile + registry.CertFileOpt = cp.CertFile + registry.KeyFile = cp.KeyFile + + if err := callPersistentPreRunE(cmd, args); err != nil { + return err + } + + return nil +} + func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { client := action.NewInstall(cfg) valueOpts := &values.Options{} @@ -116,6 +130,9 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return compInstall(args, toComplete, client) }, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return preRunEWithChartPathOptions(cmd, args, &client.ChartPathOptions) + }, RunE: func(_ *cobra.Command, args []string) error { rel, err := runInstall(args, client, valueOpts, out) if err != nil { diff --git a/cmd/helm/pull.go b/cmd/helm/pull.go index 7711320f1..6ccb5c069 100644 --- a/cmd/helm/pull.go +++ b/cmd/helm/pull.go @@ -58,6 +58,9 @@ func newPullCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { } return compListCharts(toComplete, false) }, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return preRunEWithChartPathOptions(cmd, args, &client.ChartPathOptions) + }, RunE: func(cmd *cobra.Command, args []string) error { client.Settings = settings if client.Version == "" && client.Devel { diff --git a/cmd/helm/registry.go b/cmd/helm/registry.go index d13c308b2..6fc7126d3 100644 --- a/cmd/helm/registry.go +++ b/cmd/helm/registry.go @@ -29,11 +29,21 @@ This command consists of multiple subcommands to interact with registries. func newRegistryCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { cmd := &cobra.Command{ - Use: "registry", - Short: "login to or logout from a registry", - Long: registryHelp, - Hidden: !FeatureGateOCI.IsEnabled(), - PersistentPreRunE: checkOCIFeatureGate(), + Use: "registry", + Short: "login to or logout from a registry", + Long: registryHelp, + Hidden: !FeatureGateOCI.IsEnabled(), + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + if err := checkOCIFeatureGate()(cmd, args); err != nil { + return err + } + + if err := callPersistentPreRunE(cmd, args); err != nil { + return err + } + + return nil + }, } cmd.AddCommand( newRegistryLoginCmd(cfg, out), diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 285c80021..8a00df985 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -153,15 +153,26 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string flags.ParseErrorsWhitelist.UnknownFlags = true flags.Parse(args) - registryClient, err := registry.NewClient( - registry.ClientOptDebug(settings.Debug), - registry.ClientOptWriter(out), - registry.ClientOptCredentialsFile(settings.RegistryConfig), - ) - if err != nil { - return nil, err + cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { + registryClient, err := registry.NewClient( + registry.ClientOptDebug(settings.Debug), + registry.ClientOptWriter(out), + registry.ClientOptCredentialsFile(settings.RegistryConfig), + registry.ClientOptPlainHTTP(registry.PlainHTTPOpt), + registry.ClientOptInsecureSkipVerifyTLS(registry.InsecureOpt), + registry.ClientOptCAFile(registry.CAFileOpt), + registry.ClientOptCertKeyFiles(registry.CertFileOpt, registry.KeyFile), + ) + if err != nil { + return err + } + // Used by helm install/show/upgrade/template in LocateChart + registry.DefaultClient = registryClient + // Used by helm pull/registry/chart/... + actionConfig.RegistryClient = registryClient + + return nil } - actionConfig.RegistryClient = registryClient // Add subcommands cmd.AddCommand( diff --git a/cmd/helm/show.go b/cmd/helm/show.go index 888d2d3f3..625014b0a 100644 --- a/cmd/helm/show.go +++ b/cmd/helm/show.go @@ -111,6 +111,9 @@ func newShowCmd(out io.Writer) *cobra.Command { Long: showChartDesc, Args: require.ExactArgs(1), ValidArgsFunction: validArgsFunc, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return preRunEWithChartPathOptions(cmd, args, &client.ChartPathOptions) + }, RunE: func(cmd *cobra.Command, args []string) error { client.OutputFormat = action.ShowChart output, err := runShow(args, client) diff --git a/cmd/helm/template.go b/cmd/helm/template.go index 1110771f0..803da479e 100644 --- a/cmd/helm/template.go +++ b/cmd/helm/template.go @@ -63,6 +63,9 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return compInstall(args, toComplete, client) }, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return preRunEWithChartPathOptions(cmd, args, &client.ChartPathOptions) + }, RunE: func(_ *cobra.Command, args []string) error { client.DryRun = true client.ReleaseName = "RELEASE-NAME" diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index 1952b8421..2692fef35 100644 --- a/cmd/helm/upgrade.go +++ b/cmd/helm/upgrade.go @@ -72,6 +72,9 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { Short: "upgrade a release", Long: upgradeDesc, Args: require.ExactArgs(2), + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return preRunEWithChartPathOptions(cmd, args, &client.ChartPathOptions) + }, ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { if len(args) == 0 { return compListReleases(toComplete, args, cfg) diff --git a/internal/experimental/registry/client.go b/internal/experimental/registry/client.go index 95d1d55e8..7f6b4e47b 100644 --- a/internal/experimental/registry/client.go +++ b/internal/experimental/registry/client.go @@ -34,6 +34,7 @@ import ( "github.com/gosuri/uitable" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" + "github.com/spf13/pflag" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/helmpath" @@ -44,6 +45,24 @@ const ( CredentialsFileBasename = "config.json" ) +// DefaultClient defines the default registry client +var DefaultClient *Client + +// InsecureOpt defines access registry witout TLS verification +var InsecureOpt bool + +// PlainHTTPOpt indicates access registry in plain HTTP +var PlainHTTPOpt bool + +// CAFileOpt defines the CA cert file for registry access +var CAFileOpt string + +// CertFileOpt defines the client cert file for registry access +var CertFileOpt string + +// KeyFile defines the client key file for registry access +var KeyFile string + type ( // Client works with OCI-compliant registries and local Helm chart cache Client struct { @@ -64,6 +83,14 @@ type ( } ) +func AddRegistryCmdFlags(f *pflag.FlagSet) { + f.BoolVarP(&InsecureOpt, "insecure-skip-tls-verify", "", false, "skip registry tls certificate checks") + f.BoolVarP(&PlainHTTPOpt, "plain-http", "", false, "use plain http to connect to the registry instead of https") + f.StringVar(&CertFileOpt, "cert-file", "", "identify HTTPS client using this SSL certificate file") + f.StringVar(&KeyFile, "key-file", "", "identify HTTPS client using this SSL key file") + f.StringVar(&CAFileOpt, "ca-file", "", "verify certificates of HTTPS-enabled registry using this CA bundle") +} + // NewClient returns a new registry client with config func NewClient(opts ...ClientOption) (*Client, error) { client := &Client{ diff --git a/pkg/action/install.go b/pkg/action/install.go index c55e72adc..0ad8b1aed 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -35,6 +35,7 @@ import ( "k8s.io/cli-runtime/pkg/resource" "sigs.k8s.io/yaml" + "helm.sh/helm/v3/internal/experimental/registry" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/cli" @@ -662,7 +663,9 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( return "", errors.Errorf("--version flag is explicitly required for OCI registries") } - dl.Options = append(dl.Options, getter.WithTagName(version)) + dl.Options = append(dl.Options, + getter.WithRegistryClient(registry.DefaultClient), + getter.WithTagName(version)) } if c.Verify { diff --git a/pkg/getter/ocigetter.go b/pkg/getter/ocigetter.go index 4501ce380..565267f6b 100644 --- a/pkg/getter/ocigetter.go +++ b/pkg/getter/ocigetter.go @@ -49,22 +49,20 @@ func (g *OCIGetter) get(href string) (*bytes.Buffer, error) { return nil, err } - // In case any TLS setting is set, recreate the registry client - opts := []registry.ClientOption{} + if client == nil { + opts := []registry.ClientOption{} - if g.opts.caFile != "" { - opts = append(opts, registry.ClientOptCAFile(g.opts.caFile)) - } - - if g.opts.certFile != "" && g.opts.keyFile != "" { - opts = append(opts, registry.ClientOptCertKeyFiles(g.opts.certFile, g.opts.keyFile)) - } + if g.opts.caFile != "" { + opts = append(opts, registry.ClientOptCAFile(g.opts.caFile)) + } - if g.opts.insecureSkipVerifyTLS { - opts = append(opts, registry.ClientOptInsecureSkipVerifyTLS(g.opts.insecureSkipVerifyTLS)) - } + if g.opts.certFile != "" && g.opts.keyFile != "" { + opts = append(opts, registry.ClientOptCertKeyFiles(g.opts.certFile, g.opts.keyFile)) + } - if len(opts) > 0 { + if g.opts.insecureSkipVerifyTLS { + opts = append(opts, registry.ClientOptInsecureSkipVerifyTLS(g.opts.insecureSkipVerifyTLS)) + } client, err = registry.NewClient(opts...) if err != nil { return nil, err @@ -81,16 +79,7 @@ func (g *OCIGetter) get(href string) (*bytes.Buffer, error) { // NewOCIGetter constructs a valid http/https client as a Getter func NewOCIGetter(ops ...Option) (Getter, error) { - registryClient, err := registry.NewClient() - if err != nil { - return nil, err - } - - client := OCIGetter{ - opts: options{ - registryClient: registryClient, - }, - } + client := OCIGetter{} for _, opt := range ops { opt(&client.opts) diff --git a/pkg/getter/ocigetter_test.go b/pkg/getter/ocigetter_test.go index fc548b7a6..ab1ca25bf 100644 --- a/pkg/getter/ocigetter_test.go +++ b/pkg/getter/ocigetter_test.go @@ -16,15 +16,49 @@ limitations under the License. package getter import ( + "path/filepath" "testing" ) func TestNewOCIGetter(t *testing.T) { - testfn := func(ops *options) { - if ops.registryClient == nil { - t.Fatalf("the OCIGetter's registryClient should not be null") - } + _, err := NewOCIGetter() + if err != nil { + t.Fatal(err) } - NewOCIGetter(testfn) + // Test with options + cd := "../../testdata" + join := filepath.Join + ca, pub, priv := join(cd, "rootca.crt"), join(cd, "crt.pem"), join(cd, "key.pem") + insecure := true + + g, err := NewOCIGetter( + WithInsecureSkipVerifyTLS(insecure), + WithTLSClientConfig(pub, priv, ca), + ) + + if err != nil { + t.Fatal(err) + } + + og, ok := g.(*OCIGetter) + if !ok { + t.Fatal("expected NewOCIGetter to produce an *OCIGetter") + } + + if og.opts.certFile != pub { + t.Errorf("Expected NewOCIGetter to contain %q as the public key file, got %q", pub, og.opts.certFile) + } + + if og.opts.keyFile != priv { + t.Errorf("Expected NewOCIGetter to contain %q as the private key file, got %q", priv, og.opts.keyFile) + } + + if og.opts.caFile != ca { + t.Errorf("Expected NewOCIGetter to contain %q as the CA file, got %q", ca, og.opts.caFile) + } + + if og.opts.insecureSkipVerifyTLS != insecure { + t.Errorf("Expected NewOCIGetter to contain %t as InsecureSkipVerifyTLs flag, got %t", false, og.opts.insecureSkipVerifyTLS) + } }