Move registry client creation into one place

Signed-off-by: Chen Winston <winston.chen@nokia-sbell.com>

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(),
+		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),

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")
+	registry.AddRegistryCmdFlags(f)
-	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")
 
 	return cmd
 }

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")
+	registry.AddRegistryCmdFlags(f)
-	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")
 	return cmd
 }

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 {

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 {

cmd/helm/registry.go

@@ -33,7 +33,17 @@ func newRegistryCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 		Short:  "login to or logout from a registry",
 		Long:   registryHelp,
 		Hidden: !FeatureGateOCI.IsEnabled(),
-		PersistentPreRunE: checkOCIFeatureGate(),
+		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),

cmd/helm/root.go

@@ -153,16 +153,27 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string
 	flags.ParseErrorsWhitelist.UnknownFlags = true
 	flags.Parse(args)
 
+	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 nil, err
+			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
+	}
+
 	// Add subcommands
 	cmd.AddCommand(
 		// chart commands

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)

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"

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)

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{

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 {

pkg/getter/ocigetter.go

@@ -49,7 +49,7 @@ func (g *OCIGetter) get(href string) (*bytes.Buffer, error) {
 		return nil, err
 	}
 
-	// In case any TLS setting is set, recreate the registry client
+	if client == nil {
 		opts := []registry.ClientOption{}
 
 		if g.opts.caFile != "" {
@@ -63,8 +63,6 @@ func (g *OCIGetter) get(href string) (*bytes.Buffer, error) {
 		if g.opts.insecureSkipVerifyTLS {
 			opts = append(opts, registry.ClientOptInsecureSkipVerifyTLS(g.opts.insecureSkipVerifyTLS))
 		}
-
-	if len(opts) > 0 {
 		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()
+	client := OCIGetter{}
-	if err != nil {
-		return nil, err
-	}
-
-	client := OCIGetter{
-		opts: options{
-			registryClient: registryClient,
-		},
-	}
 
 	for _, opt := range ops {
 		opt(&client.opts)

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) {
+	_, err := NewOCIGetter()
-		if ops.registryClient == nil {
+	if err != nil {
-			t.Fatalf("the OCIGetter's registryClient should not be null")
+		t.Fatal(err)
 	}
+
+	// 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)
 	}
 
-	NewOCIGetter(testfn)
+	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)
+	}
 }