fix(plugindefinitions): cleanup orphaned Flux HelmChart resources (#2061)

* fix(plugindefinition): cleanup orphaned Flux HelmChart resources on version bump (#2047)

When the HelmChart version in a PluginDefinition spec is updated, the
controller now lists all HelmChart resources owned (controller-reference)
by the PluginDefinition and deletes any whose name no longer matches the
current FluxHelmChartResourceName (i.e. <pluginDef.Name>-<version>).

This prevents accumulation of orphaned HelmChart resources that were
previously left behind whenever the chart version was bumped.

The cleanup is performed in the EnsureCreated reconcile path after the
new HelmChart has been created/updated, so it only runs when the
PluginDefinition is active.

Adds a controller integration test that:
- creates a PluginDefinition with chart version 1.0.0
- bumps the version to 2.0.0
- asserts the new HelmChart (v2.0.0) is created
- asserts the orphaned HelmChart (v1.0.0) is deleted

Fixes: #2047

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

* fix(plugindefinition): address PR review comments (#2047)

- Ignore NotFound error when deleting orphaned HelmChart (avoids hard
  reconciliation failure if a chart is deleted concurrently)
- Compare owner UIDs directly (types.UID) instead of via string()
  conversions for clarity
- Wire deleteOrphanedHelmCharts into ClusterPluginDefinitionReconciler
  so both PluginDefinition and ClusterPluginDefinition clean up orphans
- Tests now derive expected HelmChart names via FluxHelmChartResourceName()
  instead of hard-coding the name+version concatenation convention
- Added ClusterPluginDefinition orphan cleanup integration test mirroring
  the PluginDefinition test

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

* chore: add sign-off to commits

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

* fix(plugindefinition): address additional review comments on PR #2047

- Add LabelKeyPluginDefinition label to HelmChart during CreateOrUpdate so
  the orphan-cleanup list call can use a label selector instead of scanning
  all HelmCharts in the namespace on every reconcile (scalability).
- Switch deleteOrphanedHelmCharts to use client.MatchingLabels with
  greenhouseapis.LabelKeyPluginDefinition for an efficient server-side filter.
- Replace clientutil.GetOwnerReference + GetObjectKind().GroupVersionKind().Kind
  with metav1.GetControllerOf() which is the idiomatic, reliable way to get
  the controller owner reference (GetObjectKind() can return an empty Kind when
  an object is constructed directly rather than decoded from the API).
- Remove now-unused 'clientutil' import.

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

* fix(plugindefinition): apply review feedback and rebase on upstream main (#2047)

Rebase on top of upstream commit 4d2da4b which was merged by
abhijith-darshan after the PR was opened.

- Incorporate LabelKeyOwnedBy propagation from upstream PR #2062:
  when the PluginDefinition has an 'owned-by' label it is propagated
  to the managed HelmChart.
- Add deleteOrphanedHelmCharts() with:
  - client.MatchingLabels{LabelKeyPluginDefinition: name} for efficient
    server-side filtering (abhijith-darshan request)
  - metav1.GetControllerOf() for reliable owner reference lookup
  - apierrors.IsNotFound guard for concurrent deletion safety
  - lowercase log message (NIT by abhijith-darshan)

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

* fix(plugindefinition): run make fmt to fix lint/formatting errors

Run 'make fmt' (goimports + gofmt) to fix import ordering and formatting
issues flagged by the CI linter.

Signed-off-by: Edrilan Berisha <edi91@gmx.net>

---------

Signed-off-by: Edrilan Berisha <edi91@gmx.net>
Co-authored-by: Abhijith Ravindra <137736216+abhijith-darshan@users.noreply.github.com>

Author: EdrilanBerisha

api/meta/v1alpha1/zz_generated.deepcopy.go

@@ -101,6 +101,11 @@ func (r *ClusterPluginDefinitionReconciler) EnsureCreated(ctx context.Context, o
 	if err != nil {
 		return ctrl.Result{}, lifecycle.Failed, err
 	}
+
+	if err := h.deleteOrphanedHelmCharts(ctx); err != nil {
+		return ctrl.Result{}, lifecycle.Failed, err
+	}
+
 	h.setHelmChartReadyCondition(ctx, helmChart)
 
 	return ctrl.Result{}, lifecycle.Success, nil

api/v1alpha1/zz_generated.deepcopy.go

@@ -104,6 +104,10 @@ func (r *PluginDefinitionReconciler) EnsureCreated(ctx context.Context, obj life
 		return ctrl.Result{}, lifecycle.Failed, err
 	}
 
+	if err := h.deleteOrphanedHelmCharts(ctx); err != nil {
+		return ctrl.Result{}, lifecycle.Failed, err
+	}
+
 	h.setHelmChartReadyCondition(ctx, helmChart)
 
 	return ctrl.Result{}, lifecycle.Success, nil

internal/controller/plugindefinition/cluster_plugindefinition_controller.go

@@ -204,6 +204,109 @@ var _ = Describe("PluginDefinition controller", func() {
 		})
 	})
 
+	Context("When updating a PluginDefinition's HelmChart version", Ordered, func() {
+		It("should delete orphaned HelmChart resources when the chart version is bumped", func() {
+			By("creating a PluginDefinition with chart version 1.0.0")
+			pluginDef := test.NewPluginDefinition(test.Ctx, "orphan-test-plugin", test.TestNamespace,
+				test.WithPluginDefinitionVersion(PluginDefinitionVersion),
+				test.WithPluginDefinitionHelmChart(&greenhousev1alpha1.HelmChartReference{
+					Name:       HelmChart,
+					Repository: HelmRepo,
+					Version:    "1.0.0",
+				}),
+			)
+			err := test.K8sClient.Create(test.Ctx, pluginDef)
+			Expect(err).ToNot(HaveOccurred(), "there should be no error creating the PluginDefinition")
+
+			By("waiting for the initial HelmChart (v1.0.0) to be created")
+			// Use FluxHelmChartResourceName() rather than hard-coding the name+version
+			// convention so that the test stays aligned with the production logic.
+			initialHelmChart := &sourcev1.HelmChart{}
+			initialHelmChart.SetName(pluginDef.FluxHelmChartResourceName())
+			initialHelmChart.SetNamespace(pluginDef.GetNamespace())
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(initialHelmChart), initialHelmChart)
+				g.Expect(err).ToNot(HaveOccurred(), "initial HelmChart v1.0.0 should be created")
+			}).Should(Succeed(), "the initial HelmChart should be created")
+
+			By("updating the PluginDefinition to chart version 2.0.0")
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(pluginDef), pluginDef)
+				g.Expect(err).ToNot(HaveOccurred())
+				pluginDef.Spec.HelmChart.Version = "2.0.0"
+				err = test.K8sClient.Update(test.Ctx, pluginDef)
+				g.Expect(err).ToNot(HaveOccurred(), "should be able to update the PluginDefinition version")
+			}).Should(Succeed())
+
+			By("verifying the new HelmChart (v2.0.0) is created")
+			// Derive the new expected name via FluxHelmChartResourceName() after the version was bumped.
+			newHelmChart := &sourcev1.HelmChart{}
+			newHelmChart.SetName(pluginDef.FluxHelmChartResourceName())
+			newHelmChart.SetNamespace(pluginDef.GetNamespace())
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(newHelmChart), newHelmChart)
+				g.Expect(err).ToNot(HaveOccurred(), "new HelmChart v2.0.0 should be created")
+			}).Should(Succeed(), "the new HelmChart v2.0.0 should be created")
+
+			By("verifying the orphaned HelmChart (v1.0.0) is deleted")
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(initialHelmChart), initialHelmChart)
+				g.Expect(err).To(HaveOccurred(), "orphaned HelmChart v1.0.0 should be deleted")
+				g.Expect(cl.IgnoreNotFound(err)).To(Succeed(), "the error should be a NotFound error")
+			}).Should(Succeed(), "the orphaned HelmChart v1.0.0 should be deleted after the version bump")
+		})
+	})
+
+	Context("When updating a ClusterPluginDefinition's HelmChart version", Ordered, func() {
+		It("should delete orphaned HelmChart resources when the chart version is bumped", func() {
+			By("creating a ClusterPluginDefinition with chart version 1.0.0")
+			clusterDef := test.NewClusterPluginDefinition(test.Ctx, "orphan-cluster-test-plugin",
+				test.WithVersion(PluginDefinitionVersion),
+				test.WithHelmChart(&greenhousev1alpha1.HelmChartReference{
+					Name:       HelmChart,
+					Repository: HelmRepo,
+					Version:    "1.0.0",
+				}),
+			)
+			err := test.K8sClient.Create(test.Ctx, clusterDef)
+			Expect(err).ToNot(HaveOccurred(), "there should be no error creating the ClusterPluginDefinition")
+
+			By("waiting for the initial HelmChart (v1.0.0) to be created")
+			initialHelmChart := &sourcev1.HelmChart{}
+			initialHelmChart.SetName(clusterDef.FluxHelmChartResourceName())
+			initialHelmChart.SetNamespace(flux.HelmRepositoryDefaultNamespace)
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(initialHelmChart), initialHelmChart)
+				g.Expect(err).ToNot(HaveOccurred(), "initial HelmChart v1.0.0 should be created")
+			}).Should(Succeed(), "the initial ClusterPluginDefinition HelmChart should be created")
+
+			By("updating the ClusterPluginDefinition to chart version 2.0.0")
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(clusterDef), clusterDef)
+				g.Expect(err).ToNot(HaveOccurred())
+				clusterDef.Spec.HelmChart.Version = "2.0.0"
+				err = test.K8sClient.Update(test.Ctx, clusterDef)
+				g.Expect(err).ToNot(HaveOccurred(), "should be able to update the ClusterPluginDefinition version")
+			}).Should(Succeed())
+
+			By("verifying the new HelmChart (v2.0.0) is created")
+			newHelmChart := &sourcev1.HelmChart{}
+			newHelmChart.SetName(clusterDef.FluxHelmChartResourceName())
+			newHelmChart.SetNamespace(flux.HelmRepositoryDefaultNamespace)
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(newHelmChart), newHelmChart)
+				g.Expect(err).ToNot(HaveOccurred(), "new HelmChart v2.0.0 should be created")
+			}).Should(Succeed(), "the new ClusterPluginDefinition HelmChart v2.0.0 should be created")
+
+			By("verifying the orphaned HelmChart (v1.0.0) is deleted")
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, cl.ObjectKeyFromObject(initialHelmChart), initialHelmChart)
+				g.Expect(err).To(HaveOccurred(), "orphaned HelmChart v1.0.0 should be deleted")
+				g.Expect(cl.IgnoreNotFound(err)).To(Succeed(), "the error should be a NotFound error")
+			}).Should(Succeed(), "the orphaned ClusterPluginDefinition HelmChart v1.0.0 should be deleted after the version bump")
+		})
+	})
+
 	Context("When creating a ClusterPluginDefinition", Ordered, func() {
 		It("should successfully create a HelmRepository from ClusterPluginDefinition", func() {
 			By("creating a ClusterPluginDefinition")

internal/controller/plugindefinition/plugindefinition_controller.go

@@ -13,6 +13,7 @@ import (
 	fluxmeta "github.com/fluxcd/pkg/apis/meta"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/events"
@@ -188,6 +189,48 @@ func (h *helmer) createUpdateHelmChart(ctx context.Context, helmRepo *sourcev1.H
 	return helmChart, nil
 }
 
+// deleteOrphanedHelmCharts lists HelmChart resources labelled with the owner PluginDefinition
+// name and deletes every one whose name differs from the currently desired FluxHelmChartResourceName.
+// Using a label selector avoids scanning all HelmCharts in the namespace on every reconcile.
+// The controller-reference UID check provides a secondary guard to prevent touching charts that
+// were labelled identically by an unrelated controller.
+func (h *helmer) deleteOrphanedHelmCharts(ctx context.Context) error {
+	currentName := h.pluginDef.FluxHelmChartResourceName()
+	logger := log.FromContext(ctx)
+
+	helmChartList := &sourcev1.HelmChartList{}
+	if err := h.k8sClient.List(ctx, helmChartList,
+		client.InNamespace(h.namespaceName),
+		client.MatchingLabels{greenhouseapis.LabelKeyPluginDefinition: h.pluginDef.GetName()},
+	); err != nil {
+		return fmt.Errorf("failed to list HelmCharts in namespace %s: %w", h.namespaceName, err)
+	}
+
+	for i := range helmChartList.Items {
+		chart := &helmChartList.Items[i]
+		// Use metav1.GetControllerOf to reliably retrieve the controller owner reference
+		// (avoids relying on GetObjectKind().GroupVersionKind().Kind which can be empty).
+		controllerRef := metav1.GetControllerOf(chart)
+		if controllerRef == nil || controllerRef.UID != h.pluginDef.GetUID() {
+			continue
+		}
+		// Keep the current (desired) HelmChart; delete everything else.
+		if chart.Name == currentName {
+			continue
+		}
+		logger.Info("deleting orphaned HelmChart", "namespace", chart.Namespace, "name", chart.Name)
+		if err := h.k8sClient.Delete(ctx, chart); err != nil {
+			// Ignore NotFound — the chart may have been deleted concurrently.
+			if apierrors.IsNotFound(err) {
+				continue
+			}
+			return fmt.Errorf("failed to delete orphaned HelmChart %s/%s: %w", chart.Namespace, chart.Name, err)
+		}
+		h.recorder.Eventf(h.pluginDef, chart, corev1.EventTypeNormal, "Deleted", "reconciling (Cluster-)PluginDefinition", "Deleted orphaned HelmChart %s", chart.Name)
+	}
+	return nil
+}
+
 // ensureChartReplication triggers replication for the Helm chart OCI artifact to the configured mirror registry.
 func (h *helmer) ensureChartReplication(ctx context.Context) error {
 	if !h.ociMirroringEnabled {