[v0.15] Align Helm deployer client construction (#5044)

* Use consistent getter in Helm configuration

createCfg used the base getter for both RESTClientGetter and KubeClient
independently of the getter produced in getCfg. Accept a getter
parameter in createCfg so callers control which getter is used for all
client construction.

* Use Helm configuration getter for valuesFrom client

Construct the kubernetes.Interface used to resolve valuesFrom
references from the Helm configuration's RESTClientGetter.

Author: thardeck

internal/helmdeployer/deployer.go

@@ -85,7 +85,7 @@ func (h *Helm) Setup(ctx context.Context, client client.Client, getter genericcl
 	h.client = client
 	h.getter = getter
 
-	cfg, err := h.createCfg(ctx, "")
+	cfg, err := h.createCfg(ctx, "", h.getter)
 	if err != nil {
 		return err
 	}
@@ -139,7 +139,7 @@ func (h *Helm) getCfg(ctx context.Context, namespace, serviceAccountName string)
 	kClient := kube.New(getter)
 	kClient.Namespace = namespace
 
-	cfg, err := h.createCfg(ctx, namespace)
+	cfg, err := h.createCfg(ctx, namespace, getter)
 	if err != nil {
 		return nil, err
 	}
@@ -151,7 +151,7 @@ func (h *Helm) getCfg(ctx context.Context, namespace, serviceAccountName string)
 	return cfg, nil
 }
 
-func (h *Helm) createCfg(ctx context.Context, namespace string) (*action.Configuration, error) {
+func (h *Helm) createCfg(ctx context.Context, namespace string, getter genericclioptions.RESTClientGetter) (*action.Configuration, error) {
 	// Create a logger handler for Helm SDK components.
 	// This uses Fleet's controller-runtime logger (which uses logr/zapr) and adapts it to slog.
 	// The logger level is set to V(1) to match the verbosity level used in Helm v3.
@@ -160,7 +160,7 @@ func (h *Helm) createCfg(ctx context.Context, namespace string) (*action.Configu
 		Level: slog.LevelDebug,
 	})
 
-	kc := kube.New(h.getter)
+	kc := kube.New(getter)
 	kc.SetLogger(handler)
 	clientSet, err := kc.Factory.KubernetesClientSet()
 	if err != nil {
@@ -172,7 +172,7 @@ func (h *Helm) createCfg(ctx context.Context, namespace string) (*action.Configu
 	store.MaxHistory = MaxHelmHistory
 
 	cfg := &action.Configuration{
-		RESTClientGetter: h.getter,
+		RESTClientGetter: getter,
 		Releases:         store,
 		KubeClient:       kc,
 	}

internal/helmdeployer/install.go

@@ -23,8 +23,10 @@ import (
 	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/types"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/yaml"
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -78,12 +80,22 @@ func (h *Helm) install(ctx context.Context, bundleID string, manifest *manifest.
 	logger := log.FromContext(ctx).WithName("helm-deployer").WithName("install").WithValues("commit", manifest.Commit, "dryRun", dryRunCfg.DryRun)
 	timeout, defaultNamespace, releaseName := h.getOpts(bundleID, options)
 
-	values, err := h.getValues(ctx, options, defaultNamespace)
+	cfg, err := h.getCfg(ctx, defaultNamespace, options.ServiceAccount)
 	if err != nil {
 		return nil, err
 	}
 
-	cfg, err := h.getCfg(ctx, defaultNamespace, options.ServiceAccount)
+	// kubeClient is nil in template mode; getValues treats a nil client as
+	// "skip ValuesFrom lookup" and returns only the statically defined values.
+	var kubeClient kubernetes.Interface
+	if !h.template {
+		kubeClient, err = kubeClientFromGetter(cfg.RESTClientGetter)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	values, err := h.getValues(ctx, options, defaultNamespace, kubeClient)
 	if err != nil {
 		return nil, err
 	}
@@ -466,7 +478,7 @@ func handleOrphanedRelease(ctx context.Context, cfg *action.Configuration, lastR
 	return false, nil
 }
 
-func (h *Helm) getValues(ctx context.Context, options fleet.BundleDeploymentOptions, defaultNamespace string) (map[string]interface{}, error) {
+func (h *Helm) getValues(ctx context.Context, options fleet.BundleDeploymentOptions, defaultNamespace string, kubeClient kubernetes.Interface) (map[string]interface{}, error) {
 	if options.Helm == nil {
 		return nil, nil
 	}
@@ -480,8 +492,8 @@ func (h *Helm) getValues(ctx context.Context, options fleet.BundleDeploymentOpti
 	if values == nil {
 		values = map[string]interface{}{}
 	}
-	// do not run this when using template
-	if !h.template {
+	// kubeClient is nil in template mode; skip the cluster lookups in that case.
+	if kubeClient != nil {
 		for _, valuesFrom := range options.Helm.ValuesFrom {
 			var tempValues map[string]interface{}
 			if valuesFrom.ConfigMapKeyRef != nil {
@@ -496,8 +508,7 @@ func (h *Helm) getValues(ctx context.Context, options fleet.BundleDeploymentOpti
 				if key == "" {
 					key = DefaultKey
 				}
-				configMap := &corev1.ConfigMap{}
-				err := h.client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, configMap)
+				configMap, err := kubeClient.CoreV1().ConfigMaps(namespace).Get(ctx, name, metav1.GetOptions{})
 				if err != nil {
 					return nil, err
 				}
@@ -524,8 +535,7 @@ func (h *Helm) getValues(ctx context.Context, options fleet.BundleDeploymentOpti
 				if key == "" {
 					key = DefaultKey
 				}
-				secret := &corev1.Secret{}
-				err := h.client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, secret)
+				secret, err := kubeClient.CoreV1().Secrets(namespace).Get(ctx, name, metav1.GetOptions{})
 				if err != nil {
 					return nil, err
 				}
@@ -543,6 +553,21 @@ func (h *Helm) getValues(ctx context.Context, options fleet.BundleDeploymentOpti
 	return values, nil
 }
 
+// restConfigGetter produces a *rest.Config. Both
+// genericclioptions.RESTClientGetter and action.RESTClientGetter satisfy it.
+type restConfigGetter interface {
+	ToRESTConfig() (*rest.Config, error)
+}
+
+func kubeClientFromGetter(getter restConfigGetter) (kubernetes.Interface, error) {
+	restConfig, err := getter.ToRESTConfig()
+	if err != nil {
+		return nil, err
+	}
+
+	return kubernetes.NewForConfig(restConfig)
+}
+
 func valuesFromSecret(name, namespace, key string, secret *corev1.Secret) (map[string]interface{}, error) {
 	var m map[string]interface{}
 	if secret == nil {

internal/helmdeployer/install_test.go

@@ -19,10 +19,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	kruntime "k8s.io/apimachinery/pkg/runtime"
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 )
 
 func TestValuesFrom(t *testing.T) {
@@ -560,9 +557,6 @@ func TestValuesFromUsesDefaultNamespaceWhenResourceCopiedDownstream(t *testing.T
 	a := assert.New(t)
 	r := require.New(t)
 
-	scheme := kruntime.NewScheme()
-	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
-
 	// default namespace where the Helm release lives
 	defaultNS := "helm-default"
 
@@ -578,9 +572,8 @@ func TestValuesFromUsesDefaultNamespaceWhenResourceCopiedDownstream(t *testing.T
 		Data:       map[string][]byte{DefaultKey: []byte("secVal: secDefault")},
 	}
 
-	cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cm, &sec).Build()
-
-	h := &Helm{client: cl, template: false}
+	kubeClient := kubernetesfake.NewSimpleClientset(&cm, &sec)
+	h := &Helm{template: false}
 
 	opts := fleet.BundleDeploymentOptions{
 		Helm: &fleet.HelmOptions{
@@ -597,7 +590,7 @@ func TestValuesFromUsesDefaultNamespaceWhenResourceCopiedDownstream(t *testing.T
 	os.Setenv(experimental.CopyResourcesDownstreamFlag, "true")
 	defer os.Unsetenv(experimental.CopyResourcesDownstreamFlag)
 
-	vals, err := h.getValues(context.TODO(), opts, defaultNS)
+	vals, err := h.getValues(context.TODO(), opts, defaultNS, kubeClient)
 	r.NoError(err)
 
 	// configmap and secret data should have been read from defaultNS
@@ -609,9 +602,6 @@ func TestValuesFromUsesProvidedNamespaceWhenNotCopiedDownstream(t *testing.T) {
 	a := assert.New(t)
 	r := require.New(t)
 
-	scheme := kruntime.NewScheme()
-	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
-
 	// namespaces
 	defaultNS := "helm-default"
 	providedNS := "explicit-ns"
@@ -622,8 +612,8 @@ func TestValuesFromUsesProvidedNamespaceWhenNotCopiedDownstream(t *testing.T) {
 		Data:       map[string]string{DefaultKey: "cmVal: cmProvided"},
 	}
 
-	cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cmProvided).Build()
-	h := &Helm{client: cl, template: false}
+	kubeClient := kubernetesfake.NewSimpleClientset(&cmProvided)
+	h := &Helm{template: false}
 
 	opts := fleet.BundleDeploymentOptions{
 		Helm: &fleet.HelmOptions{
@@ -637,7 +627,7 @@ func TestValuesFromUsesProvidedNamespaceWhenNotCopiedDownstream(t *testing.T) {
 	os.Setenv(experimental.CopyResourcesDownstreamFlag, "true")
 	defer os.Unsetenv(experimental.CopyResourcesDownstreamFlag)
 
-	vals, err := h.getValues(context.TODO(), opts, defaultNS)
+	vals, err := h.getValues(context.TODO(), opts, defaultNS, kubeClient)
 	r.NoError(err)
 	a.Equal("cmProvided", vals["cmVal"])
 }
@@ -646,11 +636,8 @@ func TestValuesFromErrorWhenCopiedDownstreamButExperimentalDisabled(t *testing.T
 	a := assert.New(t)
 	r := require.New(t)
 
-	scheme := kruntime.NewScheme()
-	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
-
-	cl := fake.NewClientBuilder().WithScheme(scheme).Build()
-	h := &Helm{client: cl, template: false}
+	kubeClient := kubernetesfake.NewSimpleClientset()
+	h := &Helm{template: false}
 
 	opts := fleet.BundleDeploymentOptions{
 		Helm: &fleet.HelmOptions{
@@ -665,7 +652,7 @@ func TestValuesFromErrorWhenCopiedDownstreamButExperimentalDisabled(t *testing.T
 	// ensure experimental feature is disabled
 	os.Unsetenv(experimental.CopyResourcesDownstreamFlag)
 
-	_, err := h.getValues(context.TODO(), opts, "default-ns")
+	_, err := h.getValues(context.TODO(), opts, "default-ns", kubeClient)
 	r.Error(err)
 	// get will fail trying to read from provided-ns and should report not found
 	a.True(apierrors.IsNotFound(err), "expected a NotFound error when valuesFrom references resources and experimental feature is disabled")