From a383885bcc213f44882302f1731248f348bdf599 Mon Sep 17 00:00:00 2001
From: Spyros Synodinos <138458697+ssyno@users.noreply.github.com>
Date: Thu, 17 Oct 2024 15:26:22 +0300
Subject: [PATCH] old organization handling (#456)

* old organization handling

* test for old-finalizer

* CHANGELOG
---
 CHANGELOG.md                                  |  4 ++
 .../controller/organization_controller.go     | 28 ++++++--
 .../organization_controller_test.go           | 65 +++++++++++++++++++
 3 files changed, 91 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 13edd91..8759e52 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- Add handling on deletion for the old finalizer.
+
 ### Changed
 
 - Disable zap logger development mode to avoid panicking
diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go
index 36172ab..1191a2f 100644
--- a/internal/controller/organization_controller.go
+++ b/internal/controller/organization_controller.go
@@ -35,6 +35,11 @@ import (
 	securityv1alpha1 "github.com/giantswarm/organization-operator/api/v1alpha1"
 )
 
+const (
+	oldFinalizer = "operatorkit.giantswarm.io/organization-operator-organization-controller"
+	newFinalizer = "organization.giantswarm.io/finalizer"
+)
+
 var (
 	organizationsTotal = prometheus.NewGauge(
 		prometheus.GaugeOpts{
@@ -69,9 +74,9 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	}
 
 	// Add finalizer if it doesn't exist
-	if !controllerutil.ContainsFinalizer(organization, "organization.giantswarm.io/finalizer") {
+	if !controllerutil.ContainsFinalizer(organization, newFinalizer) {
 		patch := client.MergeFrom(organization.DeepCopy())
-		controllerutil.AddFinalizer(organization, "organization.giantswarm.io/finalizer")
+		controllerutil.AddFinalizer(organization, newFinalizer)
 		if err := r.Patch(ctx, organization, patch); err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to add finalizer: %w", err)
 		}
@@ -150,11 +155,22 @@ func (r *OrganizationReconciler) reconcileDelete(ctx context.Context, organizati
 		log.Info("Associated namespace not found or already deleted")
 	}
 
-	if controllerutil.ContainsFinalizer(organization, "organization.giantswarm.io/finalizer") {
-		log.Info("Removing finalizer from Organization")
-		controllerutil.RemoveFinalizer(organization, "organization.giantswarm.io/finalizer")
+	// Remove old finalizer if it exists
+	if controllerutil.ContainsFinalizer(organization, oldFinalizer) {
+		log.Info("Removing old finalizer from Organization")
+		controllerutil.RemoveFinalizer(organization, oldFinalizer)
+		if err := r.Update(ctx, organization); err != nil {
+			log.Error(err, "Failed to remove old finalizer")
+			return ctrl.Result{}, err
+		}
+	}
+
+	// Remove new finalizer if it exists
+	if controllerutil.ContainsFinalizer(organization, newFinalizer) {
+		log.Info("Removing new finalizer from Organization")
+		controllerutil.RemoveFinalizer(organization, newFinalizer)
 		if err := r.Update(ctx, organization); err != nil {
-			log.Error(err, "Failed to remove finalizer")
+			log.Error(err, "Failed to remove new finalizer")
 			return ctrl.Result{}, err
 		}
 	}
diff --git a/internal/controller/organization_controller_test.go b/internal/controller/organization_controller_test.go
index 4a4c54b..c8a02ad 100644
--- a/internal/controller/organization_controller_test.go
+++ b/internal/controller/organization_controller_test.go
@@ -186,6 +186,71 @@ var _ = Describe("Organization controller", func() {
 				return fmt.Errorf("namespace still exists")
 			}, timeout, interval).Should(Succeed())
 
+			// Verify that the organization count metric has been updated
+			initialCount := testutil.ToFloat64(organizationsTotal)
+			Consistently(func() bool {
+				currentCount := testutil.ToFloat64(organizationsTotal)
+				return currentCount <= initialCount
+			}, timeout, interval).Should(BeTrue())
+		})
+	})
+	Context("When handling Organizations with old finalizers", func() {
+		It("Should remove the old finalizer when deleting an Organization", func() {
+			ctx := context.Background()
+			oldFinalizerOrg := &securityv1alpha1.Organization{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-old-finalizer",
+					Finalizers: []string{"operatorkit.giantswarm.io/organization-operator-organization-controller"},
+				},
+				Spec: securityv1alpha1.OrganizationSpec{},
+				Status: securityv1alpha1.OrganizationStatus{
+					Namespace: "org-test-old-finalizer",
+				},
+			}
+			Expect(k8sClient.Create(ctx, oldFinalizerOrg)).To(Succeed())
+
+			// Create the associated namespace
+			namespace := &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "org-test-old-finalizer",
+				},
+			}
+			Expect(k8sClient.Create(ctx, namespace)).To(Succeed())
+
+			reconciler := &OrganizationReconciler{
+				Client: k8sClient,
+				Scheme: k8sClient.Scheme(),
+			}
+
+			// Trigger deletion
+			Expect(k8sClient.Delete(ctx, oldFinalizerOrg)).To(Succeed())
+
+			// Wait for the organization to be fully deleted
+			Eventually(func() error {
+				err := k8sClient.Get(ctx, client.ObjectKey{Name: "test-old-finalizer"}, &securityv1alpha1.Organization{})
+				if errors.IsNotFound(err) {
+					return nil
+				}
+				if err != nil {
+					return err
+				}
+				// Trigger reconciliation if the organization still exists
+				_, err = reconciler.Reconcile(ctx, reconcile.Request{
+					NamespacedName: types.NamespacedName{Name: "test-old-finalizer"},
+				})
+				Expect(err).NotTo(HaveOccurred())
+				return fmt.Errorf("organization with old finalizer still exists")
+			}, timeout, interval).Should(Succeed())
+
+			// Verify that the namespace has been deleted
+			Eventually(func() error {
+				err := k8sClient.Get(ctx, client.ObjectKey{Name: "org-test-old-finalizer"}, &corev1.Namespace{})
+				if errors.IsNotFound(err) {
+					return nil
+				}
+				return fmt.Errorf("namespace for organization with old finalizer still exists")
+			}, timeout, interval).Should(Succeed())
+
 			// Verify that the organization count metric has been updated
 			initialCount := testutil.ToFloat64(organizationsTotal)
 			Consistently(func() bool {