[v0.14] Align Helm deployer client construction (#5046)

* 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

@@ -84,7 +84,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
 	}
@@ -141,7 +141,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)
 	cfg.Releases.MaxHistory = MaxHelmHistory
 	cfg.KubeClient = kClient
 
@@ -150,12 +150,12 @@ func (h *Helm) getCfg(ctx context.Context, namespace, serviceAccountName string)
 	return cfg, err
 }
 
-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) {
 	logger := log.FromContext(ctx).WithName("helmSDK")
 	info := func(format string, v ...interface{}) {
 		logger.V(1).Info(fmt.Sprintf(format, v...))
 	}
-	kc := kube.New(h.getter)
+	kc := kube.New(getter)
 	kc.Log = info
 	clientSet, err := kc.Factory.KubernetesClientSet()
 	if err != nil {
@@ -167,7 +167,7 @@ func (h *Helm) createCfg(ctx context.Context, namespace string) (action.Configur
 	store.MaxHistory = MaxHelmHistory
 
 	return action.Configuration{
-		RESTClientGetter: h.getter,
+		RESTClientGetter: getter,
 		Releases:         store,
 		KubeClient:       kc,
 		Log:              info,

internal/helmdeployer/install.go

@@ -21,8 +21,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"
 )
 
@@ -76,12 +78,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
 	}
@@ -319,7 +331,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
 	}
@@ -333,8 +345,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 {
@@ -349,8 +361,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
 				}
@@ -377,8 +388,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
 				}
@@ -396,6 +406,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

@@ -16,10 +16,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) {
@@ -171,9 +168,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"
 
@@ -189,9 +183,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{
@@ -208,7 +201,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
@@ -220,9 +213,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"
@@ -233,8 +223,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{
@@ -248,7 +238,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"])
 }
@@ -257,11 +247,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{
@@ -276,7 +263,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")