From 236efef15e5f4ac361f9a82e221ecf86c12a4a24 Mon Sep 17 00:00:00 2001 From: Marc Nuri Date: Mon, 17 Nov 2025 11:22:34 +0100 Subject: [PATCH] fix(go): update NewCfg to return error for better error handling --- native/internal/helm/helm.go | 25 ++++++++++--------- native/internal/helm/helm_test.go | 37 +++++++++++++++++++++++++++- native/internal/helm/install.go | 6 ++++- native/internal/helm/list.go | 5 +++- native/internal/helm/plugins_test.go | 30 ++++++++++++++++++---- native/internal/helm/push.go | 7 +++++- native/internal/helm/registry.go | 12 +++++++-- native/internal/helm/show.go | 6 ++++- native/internal/helm/test.go | 6 ++++- native/internal/helm/uninstall.go | 6 ++++- native/internal/helm/upgrade.go | 5 +++- 11 files changed, 118 insertions(+), 27 deletions(-) diff --git a/native/internal/helm/helm.go b/native/internal/helm/helm.go index 56418f1a..187de270 100644 --- a/native/internal/helm/helm.go +++ b/native/internal/helm/helm.go @@ -56,7 +56,7 @@ type CertOptions struct { Keyring string } -func NewCfg(options *CfgOptions) *action.Configuration { +func NewCfg(options *CfgOptions) (*action.Configuration, error) { settings := cli.New() settings.KubeConfig = options.KubeConfig actionConfig := new(action.Configuration) @@ -73,23 +73,24 @@ func NewCfg(options *CfgOptions) *action.Configuration { effectiveNamespace = "" } restClientGetter := settings.RESTClientGetter() - restClientGetter.(*genericclioptions.ConfigFlags).WrapConfigFn = func(original *rest.Config) *rest.Config { - if options.KubeConfigContents != "" { - // TODO: we could actually merge both kubeconfigs - config, err := clientcmd.RESTConfigFromKubeConfig([]byte(options.KubeConfigContents)) - if err != nil { - panic(err) - } - return config + // Validate KubeConfigContents upfront if provided + if options.KubeConfigContents != "" { + // TODO: we could actually merge both kubeconfigs + parsedConfig, err := clientcmd.RESTConfigFromKubeConfig([]byte(options.KubeConfigContents)) + if err != nil { + return nil, fmt.Errorf("failed to parse kubeconfig contents: %w", err) + } + // Use the validated config in the wrapper + restClientGetter.(*genericclioptions.ConfigFlags).WrapConfigFn = func(original *rest.Config) *rest.Config { + return parsedConfig } - return original } err := actionConfig.Init(restClientGetter, effectiveNamespace, os.Getenv("HELM_DRIVER"), log) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to initialize action configuration: %w", err) } actionConfig.RegistryClient = options.RegistryClient - return actionConfig + return actionConfig, nil } func StatusReport(release *release.Release, showDescription bool, debug bool) string { diff --git a/native/internal/helm/helm_test.go b/native/internal/helm/helm_test.go index a91554ef..7ffe7b1c 100644 --- a/native/internal/helm/helm_test.go +++ b/native/internal/helm/helm_test.go @@ -58,9 +58,12 @@ users: ` func TestNewCfg_KubeConfigContents(t *testing.T) { - cfg := NewCfg(&CfgOptions{ + cfg, err := NewCfg(&CfgOptions{ KubeConfigContents: kubeConfigContentsForTests, }) + if err != nil { + t.Fatalf("Expected NewCfg to succeed, got %s", err) + } restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { t.Fatalf("Expected converting to succeed, got %s", err) @@ -75,3 +78,35 @@ func TestNewCfg_KubeConfigContents(t *testing.T) { t.Fatalf("Expected test-password, got %s", restConfig.Password) } } + +func TestNewCfg_InvalidKubeConfigContents(t *testing.T) { + t.Run("should return error for invalid kubeconfig contents", func(t *testing.T) { + cfg, err := NewCfg(&CfgOptions{ + KubeConfigContents: "invalid yaml content {[}", + }) + if err == nil { + t.Fatal("Expected NewCfg to return error for invalid kubeconfig, got nil") + } + if cfg != nil { + t.Errorf("Expected cfg to be nil when error occurs, got %v", cfg) + } + }) + + t.Run("should return error for malformed kubeconfig", func(t *testing.T) { + malformedConfig := ` +apiVersion: v1 +kind: Config +clusters: + - this is not valid yaml structure +` + cfg, err := NewCfg(&CfgOptions{ + KubeConfigContents: malformedConfig, + }) + if err == nil { + t.Fatal("Expected NewCfg to return error for malformed kubeconfig, got nil") + } + if cfg != nil { + t.Errorf("Expected cfg to be nil when error occurs, got %v", cfg) + } + }) +} diff --git a/native/internal/helm/install.go b/native/internal/helm/install.go index ff9c8744..f5fcd45a 100644 --- a/native/internal/helm/install.go +++ b/native/internal/helm/install.go @@ -106,7 +106,11 @@ func install(options *InstallOptions) (*release.Release, *installOutputs, error) if options.Debug { cfgOptions.KubeOut = outputs.kubeOut } - client := action.NewInstall(NewCfg(cfgOptions)) + cfg, err := NewCfg(cfgOptions) + if err != nil { + return nil, outputs, err + } + client := action.NewInstall(cfg) client.GenerateName = options.GenerateName client.NameTemplate = options.NameTemplate client.Version = options.Version diff --git a/native/internal/helm/list.go b/native/internal/helm/list.go index 0980982f..b8e1cbb7 100644 --- a/native/internal/helm/list.go +++ b/native/internal/helm/list.go @@ -40,12 +40,15 @@ type ListOptions struct { } func List(options *ListOptions) (string, error) { - cfg := NewCfg(&CfgOptions{ + cfg, err := NewCfg(&CfgOptions{ KubeConfig: options.KubeConfig, KubeConfigContents: options.KubeConfigContents, Namespace: options.Namespace, AllNamespaces: options.AllNamespaces, }) + if err != nil { + return "", err + } client := action.NewList(cfg) client.All = options.All client.AllNamespaces = options.AllNamespaces diff --git a/native/internal/helm/plugins_test.go b/native/internal/helm/plugins_test.go index 2ad4c075..570a91fd 100644 --- a/native/internal/helm/plugins_test.go +++ b/native/internal/helm/plugins_test.go @@ -128,9 +128,13 @@ users: func TestAuthPlugins(t *testing.T) { t.Run("should support azure via exec plugin (kubelogin)", func(t *testing.T) { - cfg := NewCfg(&CfgOptions{ + cfg, err := NewCfg(&CfgOptions{ KubeConfigContents: azureExecKubeConfig, }) + if err != nil { + t.Errorf("Expected NewCfg to succeed, got %s", err) + return + } restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { t.Errorf("Expected azure exec kubeconfig to parse successfully, got %s", err) @@ -145,9 +149,13 @@ func TestAuthPlugins(t *testing.T) { } }) t.Run("should support gcp via exec plugin (gke-gcloud-auth-plugin)", func(t *testing.T) { - cfg := NewCfg(&CfgOptions{ + cfg, err := NewCfg(&CfgOptions{ KubeConfigContents: gcpExecKubeConfig, }) + if err != nil { + t.Errorf("Expected NewCfg to succeed, got %s", err) + return + } restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { t.Errorf("Expected gcp exec kubeconfig to parse successfully, got %s", err) @@ -162,9 +170,13 @@ func TestAuthPlugins(t *testing.T) { } }) t.Run("should register oidc auth provider plugin", func(t *testing.T) { - cfg := NewCfg(&CfgOptions{ + cfg, err := NewCfg(&CfgOptions{ KubeConfigContents: oidcKubeConfig, }) + if err != nil { + t.Errorf("Expected NewCfg to succeed, got %s", err) + return + } restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { t.Errorf("Expected oidc kubeconfig to parse successfully, got %s", err) @@ -180,9 +192,13 @@ func TestAuthPlugins(t *testing.T) { } }) t.Run("should support exec auth provider", func(t *testing.T) { - cfg := NewCfg(&CfgOptions{ + cfg, err := NewCfg(&CfgOptions{ KubeConfigContents: execKubeConfig, }) + if err != nil { + t.Errorf("Expected NewCfg to succeed, got %s", err) + return + } restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { t.Errorf("Expected exec kubeconfig to parse successfully, got %s", err) @@ -219,9 +235,13 @@ users: config: some-key: some-value ` - cfg := NewCfg(&CfgOptions{ + cfg, err := NewCfg(&CfgOptions{ KubeConfigContents: unknownAuthConfig, }) + if err != nil { + t.Errorf("Expected NewCfg to succeed, got %s", err) + return + } restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { t.Errorf("Expected parsing to succeed, got %s", err) diff --git a/native/internal/helm/push.go b/native/internal/helm/push.go index 51bb7249..7238cfa4 100644 --- a/native/internal/helm/push.go +++ b/native/internal/helm/push.go @@ -40,8 +40,13 @@ func Push(options *PushOptions) (string, error) { return "", err } + cfg, err := NewCfg(&CfgOptions{RegistryClient: registryClient}) + if err != nil { + return "", err + } + pushOptions := []action.PushOpt{ - action.WithPushConfig(NewCfg(&CfgOptions{RegistryClient: registryClient})), + action.WithPushConfig(cfg), action.WithTLSClientConfig(options.CertFile, options.KeyFile, options.CaFile), action.WithInsecureSkipTLSVerify(options.InsecureSkipTLSverify), action.WithPlainHTTP(options.PlainHttp), diff --git a/native/internal/helm/registry.go b/native/internal/helm/registry.go index b5bd741d..e6518b20 100644 --- a/native/internal/helm/registry.go +++ b/native/internal/helm/registry.go @@ -50,7 +50,11 @@ func RegistryLogin(options *RegistryOptions) (string, error) { // Manually set the logrus (which is used) output to out logrus.SetOutput(debugBuffer) } - err = action.NewRegistryLogin(NewCfg(&CfgOptions{RegistryClient: registryClient})).Run( + cfg, err := NewCfg(&CfgOptions{RegistryClient: registryClient}) + if err != nil { + return "", err + } + err = action.NewRegistryLogin(cfg).Run( debugBuffer /* ignored */, options.Hostname, options.Username, options.Password, action.WithCertFile(options.CertFile), action.WithKeyFile(options.KeyFile), @@ -78,7 +82,11 @@ func RegistryLogout(options *RegistryOptions) (string, error) { // Manually set the logrus (which is used) output to out logrus.SetOutput(debugBuffer) } - err = action.NewRegistryLogout(NewCfg(&CfgOptions{RegistryClient: registryClient})).Run( + cfg, err := NewCfg(&CfgOptions{RegistryClient: registryClient}) + if err != nil { + return "", err + } + err = action.NewRegistryLogout(cfg).Run( debugBuffer /* ignored */, options.Hostname) return appendToOutOrErr(debugBuffer, getRegistryClientOut().String(), err) } diff --git a/native/internal/helm/show.go b/native/internal/helm/show.go index a2e32b15..1391dfde 100644 --- a/native/internal/helm/show.go +++ b/native/internal/helm/show.go @@ -64,7 +64,11 @@ func Show(options *ShowOptions) (string, error) { if err != nil { return "", err } - client := action.NewShowWithConfig(format, NewCfg(&CfgOptions{})) + cfg, err := NewCfg(&CfgOptions{}) + if err != nil { + return "", err + } + client := action.NewShowWithConfig(format, cfg) client.SetRegistryClient(registryClient) client.Version = options.Version cp, err := client.ChartPathOptions.LocateChart(options.Path, cli.New()) diff --git a/native/internal/helm/test.go b/native/internal/helm/test.go index 582dc0bf..7a436b56 100644 --- a/native/internal/helm/test.go +++ b/native/internal/helm/test.go @@ -36,7 +36,11 @@ func Test(options *TestOptions) (string, error) { KubeConfigContents: options.KubeConfigContents, Namespace: options.Namespace, } - client := action.NewReleaseTesting(NewCfg(cfgOptions)) + cfg, err := NewCfg(cfgOptions) + if err != nil { + return "", err + } + client := action.NewReleaseTesting(cfg) client.Namespace = options.Namespace client.Timeout = options.Timeout release, err := client.Run(options.ReleaseName) diff --git a/native/internal/helm/uninstall.go b/native/internal/helm/uninstall.go index 3194a667..74ef9b0c 100644 --- a/native/internal/helm/uninstall.go +++ b/native/internal/helm/uninstall.go @@ -45,7 +45,11 @@ func Uninstall(options *UninstallOptions) (string, error) { if options.Debug { cfgOptions.KubeOut = kubeOut } - client := action.NewUninstall(NewCfg(cfgOptions)) + cfg, err := NewCfg(cfgOptions) + if err != nil { + return "", err + } + client := action.NewUninstall(cfg) client.DryRun = options.DryRun client.DisableHooks = options.NoHooks client.IgnoreNotFound = options.IgnoreNotFound diff --git a/native/internal/helm/upgrade.go b/native/internal/helm/upgrade.go index ea5a592e..a918ab97 100644 --- a/native/internal/helm/upgrade.go +++ b/native/internal/helm/upgrade.go @@ -78,7 +78,10 @@ func Upgrade(options *UpgradeOptions) (string, error) { if options.Debug { cfgOptions.KubeOut = kubeOut } - cfg := NewCfg(cfgOptions) + cfg, err := NewCfg(cfgOptions) + if err != nil { + return "", err + } // Install if release doesn't exist if options.Install {