[v0.13] Align Helm deployer client construction (#5047)

* 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.

* Avoids returning an error when deleting OCI artifacts (#4221)

* Avoids returning an error when deleting OCI artifacts

Returning an error when deleting an OCI artifact could lead to infinite requeing if there is a permanent
error accessing the registry.

The registry is an external resource out of Fleet and we don't have full control.
We already log an event when there is an error deleting the OCI artifact.

This was leading to errors in e2e tests, because one of the tests was checking if there was an error
when trying to use credentials with no delete permissions for the OCI registry.

If the test was the last one, it was logging the expected event and CI was successful, otherwise the next
test was failing randomly depending on which test was (if the test was reseting the OCI storage env variable then
the controller was restarted and unblocked the situation).


Orders the tests and now the only test that needs the OCI_STORAGE=false env variable is executed the last one.




---------

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

---------

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>
Co-authored-by: Xavi Garcia <xavi.garcia@suse.com>

Author: thardeck

e2e/single-cluster/oci_registry_test.go

@@ -232,7 +232,7 @@ func getOCIRegistryExternalIP(k kubectl.Command) string {
 	return externalIP
 }
 
-var _ = Describe("Single Cluster Deployments using OCI registry", Label("oci-registry", "infra-setup"), func() {
+var _ = Describe("Single Cluster Deployments using OCI registry", Label("oci-registry", "infra-setup"), Ordered, func() {
 	const asset = "single-cluster/test-oci.yaml"
 	var (
 		k                   kubectl.Command
@@ -372,10 +372,12 @@ var _ = Describe("Single Cluster Deployments using OCI registry", Label("oci-reg
 		if contentIDToPurge != "" {
 			k8sclient.ObjectShouldNotExist(clientUpstream, contentIDToPurge, "", &fleet.Content{}, true)
 		} else if contentsID != "" {
-			// check that the oci artifact was deleted
-			_, err = ocistorage.NewOCIWrapper().PullManifest(context.TODO(), ociOpts, contentsID)
-			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("not found"))
+			Eventually(func(g Gomega) {
+				// check that the oci artifact was deleted
+				_, err = ocistorage.NewOCIWrapper().PullManifest(context.TODO(), ociOpts, contentsID)
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring("not found"))
+			}).Should(Succeed())
 		}
 
 		_, err = k.Delete("events", "-n", env.Namespace, "--all")
@@ -483,57 +485,6 @@ var _ = Describe("Single Cluster Deployments using OCI registry", Label("oci-reg
 		})
 	})
 
-	When("applying a gitrepo with the default ociSecret also deployed with incorrect reference and OCI env var is not set", func() {
-		Context("containing a public oci based helm chart", func() {
-			BeforeEach(func() {
-				envVarValue = "false"
-				insecureSkipTLS = true
-				deployDefaultSecret = true
-				deploySpecificSecretName = ""
-				forceDefaultReference = "not-valid-oci-registry.com"
-				forceUserDefinedReference = ""
-				useReaderAsWriter = false
-				useNoDeleterUser = false
-			})
-
-			It("creates the bundle with no OCI storage", func() {
-				var bundle fleet.Bundle
-				By("creating the bundle", func() {
-					k8sclient.GetObjectShouldSucceed(clientUpstream, "sample-simple-chart-oci", env.Namespace, &bundle)
-				})
-				By("not setting the ContentsID field in the bundle", func() {
-					Expect(bundle.Spec.ContentsID).To(BeEmpty())
-				})
-				By("creating a bundle deployment", func() {
-					var bd fleet.BundleDeployment
-					k8sclient.GetObjectShouldSucceed(clientUpstream, "sample-simple-chart-oci", downstreamNamespace, &bd)
-					Expect(bd.Spec.DeploymentID).ToNot(BeEmpty())
-					tokens := strings.Split(bd.Spec.DeploymentID, ":")
-					Expect(tokens).To(HaveLen(2))
-					contentsID = tokens[0]
-					// save the contents id to purge after the test
-					// this will prevent interference with the previous test
-					// as the resources sha256 is the same
-					contentIDToPurge = contentsID
-				})
-				By("creating a content resource with the expected ID", func() {
-					var content fleet.Content
-					k8sclient.GetObjectShouldSucceed(clientUpstream, contentsID, "", &content)
-				})
-				By("not creating a bundle secret", func() {
-					k8sclient.ObjectShouldNotExist(clientUpstream, contentsID, env.Namespace, &corev1.Secret{}, false)
-				})
-				By("not creating a bundle deployment secret", func() {
-					k8sclient.ObjectShouldNotExist(clientUpstream, contentsID, downstreamNamespace, &corev1.Secret{}, false)
-				})
-				By("deploying the helm chart", func() {
-					var cm corev1.ConfigMap
-					k8sclient.GetObjectShouldSucceed(clientUpstream, "test-simple-chart-config", "default", &cm)
-				})
-			})
-		})
-	})
-
 	When("creating a gitrepo resource with invalid ociRegistry info", func() {
 		Context("containing a public oci based helm chart", func() {
 			BeforeEach(func() {
@@ -951,4 +902,55 @@ var _ = Describe("Single Cluster Deployments using OCI registry", Label("oci-reg
 			})
 		})
 	})
+
+	When("applying a gitrepo with the default ociSecret also deployed with incorrect reference and OCI env var is not set", func() {
+		Context("containing a public oci based helm chart", func() {
+			BeforeEach(func() {
+				envVarValue = "false"
+				insecureSkipTLS = true
+				deployDefaultSecret = true
+				deploySpecificSecretName = ""
+				forceDefaultReference = "not-valid-oci-registry.com"
+				forceUserDefinedReference = ""
+				useReaderAsWriter = false
+				useNoDeleterUser = false
+			})
+
+			It("creates the bundle with no OCI storage", func() {
+				var bundle fleet.Bundle
+				By("creating the bundle", func() {
+					k8sclient.GetObjectShouldSucceed(clientUpstream, "sample-simple-chart-oci", env.Namespace, &bundle)
+				})
+				By("not setting the ContentsID field in the bundle", func() {
+					Expect(bundle.Spec.ContentsID).To(BeEmpty())
+				})
+				By("creating a bundle deployment", func() {
+					var bd fleet.BundleDeployment
+					k8sclient.GetObjectShouldSucceed(clientUpstream, "sample-simple-chart-oci", downstreamNamespace, &bd)
+					Expect(bd.Spec.DeploymentID).ToNot(BeEmpty())
+					tokens := strings.Split(bd.Spec.DeploymentID, ":")
+					Expect(tokens).To(HaveLen(2))
+					contentsID = tokens[0]
+					// save the contents id to purge after the test
+					// this will prevent interference with the previous test
+					// as the resources sha256 is the same
+					contentIDToPurge = contentsID
+				})
+				By("creating a content resource with the expected ID", func() {
+					var content fleet.Content
+					k8sclient.GetObjectShouldSucceed(clientUpstream, contentsID, "", &content)
+				})
+				By("not creating a bundle secret", func() {
+					k8sclient.ObjectShouldNotExist(clientUpstream, contentsID, env.Namespace, &corev1.Secret{}, false)
+				})
+				By("not creating a bundle deployment secret", func() {
+					k8sclient.ObjectShouldNotExist(clientUpstream, contentsID, downstreamNamespace, &corev1.Secret{}, false)
+				})
+				By("deploying the helm chart", func() {
+					var cm corev1.ConfigMap
+					k8sclient.GetObjectShouldSucceed(clientUpstream, "test-simple-chart-config", "default", &cm)
+				})
+			})
+		})
+	})
 })

internal/cmd/controller/reconciler/bundle_controller.go

@@ -481,6 +481,10 @@ func (r *BundleReconciler) handleDelete(ctx context.Context, logger logr.Logger,
 		return ctrl.Result{RequeueAfter: requeueAfterBundleDeploymentCleanup}, batchDeleteBundleDeployments(ctx, r.Client, bds)
 	}
 
+	if err := r.maybeDeleteOCIArtifact(ctx, bundle); err != nil {
+		return ctrl.Result{}, err
+	}
+
 	metrics.BundleCollector.Delete(req.Name, req.Namespace)
 	controllerutil.RemoveFinalizer(bundle, finalize.BundleFinalizer)
 	if err := r.Update(ctx, bundle); err != nil {
@@ -492,7 +496,7 @@ func (r *BundleReconciler) handleDelete(ctx context.Context, logger logr.Logger,
 		logger.V(1).Info("Cannot delete bundle's values secret, owner garbage collection will remove it")
 	}
 
-	return ctrl.Result{}, r.maybeDeleteOCIArtifact(ctx, bundle)
+	return ctrl.Result{}, err
 }
 
 // ensureFinalizer adds a finalizer to a recently created bundle.
@@ -799,7 +803,11 @@ func (r *BundleReconciler) maybeDeleteOCIArtifact(ctx context.Context, bundle *f
 		r.Recorder.Event(bundle, fleetevent.Warning, "FailedToDeleteOCIArtifact", fmt.Sprintf("deleting OCI artifact %q: %v", bundle.Spec.ContentsID, err.Error()))
 	}
 
-	return err
+	// In case there's an error deleting from the OCI registry,
+	// we return nil because otherwise the controller would retry the operation,
+	// and since the registry is not a component of Fleet,
+	// we don't have full control over it.
+	return nil
 }
 
 func batchDeleteBundleDeployments(ctx context.Context, c client.Client, list []fleet.BundleDeployment) error {

internal/helmdeployer/deployer.go

@@ -83,7 +83,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
 	}
@@ -147,7 +147,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
 
@@ -156,12 +156,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 {
@@ -173,7 +173,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

@@ -20,8 +20,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"
 )
 
@@ -70,12 +72,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", 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
 	}
@@ -311,7 +323,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
 	}
@@ -325,8 +337,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 {
@@ -339,8 +351,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
 				}
@@ -365,8 +376,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
 				}
@@ -384,6 +394,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 {