Improve logic for cleaning up of service resources

Leaked service instances and bindings have caused some problems to CI
clusters, since they have finalizers and prevent the namespaces in which
they are created to go away. Resolving this problem requres manual
intervention. This commit makes the logic retry until no binding or
instances are left for each service plan of the related broker

tests/e2e/e2e_suite_test.go

@@ -16,7 +16,6 @@ import (
 	"time"
 
 	"code.cloudfoundry.org/korifi/tests/helpers"
-	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 	"code.cloudfoundry.org/korifi/tests/helpers/fail_handler"
 
 	"code.cloudfoundry.org/go-loggregator/v8/rpc/loggregator_v2"
@@ -1229,12 +1228,6 @@ func createBroker(brokerURL string) string {
 	return brokerGUID
 }
 
-func cleanupBroker(brokerGUID string) {
-	GinkgoHelper()
-
-	broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
-}
-
 func expectJobCompletes(resp *resty.Response) {
 	GinkgoHelper()
 

tests/e2e/service_bindings_test.go

@@ -3,6 +3,7 @@ package e2e_test
 import (
 	"net/http"
 
+	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 	"github.com/go-resty/resty/v2"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -54,15 +55,17 @@ var _ = Describe("Service Bindings", func() {
 		})
 
 		When("binding to a managed service instance", func() {
-			BeforeEach(func() {
-				brokerGUID := createBroker(serviceBrokerURL)
-				DeferCleanup(func() {
-					cleanupBroker(brokerGUID)
-				})
+			var brokerGUID string
 
+			BeforeEach(func() {
+				brokerGUID = createBroker(serviceBrokerURL)
 				instanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID)
 			})
 
+			AfterEach(func() {
+				broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
+			})
+
 			It("succeeds with a job redirect", func() {
 				Expect(httpError).NotTo(HaveOccurred())
 				Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted))
@@ -112,16 +115,19 @@ var _ = Describe("Service Bindings", func() {
 		})
 
 		When("bound to a managed service instance", func() {
+			var brokerGUID string
+
 			BeforeEach(func() {
-				brokerGUID := createBroker(serviceBrokerURL)
-				DeferCleanup(func() {
-					cleanupBroker(brokerGUID)
-				})
+				brokerGUID = createBroker(serviceBrokerURL)
 
 				instanceGUID := createManagedServiceInstance(brokerGUID, spaceGUID)
 				bindingGUID = createManagedServiceBinding(appGUID, instanceGUID, "")
 			})
 
+			AfterEach(func() {
+				broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
+			})
+
 			It("succeeds with a job redirect", func() {
 				Expect(httpError).NotTo(HaveOccurred())
 				Expect(httpResp).To(SatisfyAll(

tests/e2e/service_brokers_test.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
+	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 	"github.com/go-resty/resty/v2"
 	"github.com/google/uuid"
 	. "github.com/onsi/ginkgo/v2"
@@ -50,7 +51,7 @@ var _ = Describe("Service Brokers", func() {
 			jobURLSplit := strings.Split(jobURL, "~")
 			Expect(jobURLSplit).To(HaveLen(2))
 			DeferCleanup(func() {
-				cleanupBroker(jobURLSplit[1])
+				broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(jobURLSplit[1]).Delete()
 			})
 		})
 	})
@@ -63,10 +64,10 @@ var _ = Describe("Service Brokers", func() {
 
 		BeforeEach(func() {
 			brokerGUID = createBroker(serviceBrokerURL)
+		})
 
-			DeferCleanup(func() {
-				cleanupBroker(brokerGUID)
-			})
+		AfterEach(func() {
+			broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
 		})
 
 		JustBeforeEach(func() {
@@ -97,7 +98,7 @@ var _ = Describe("Service Brokers", func() {
 		})
 
 		AfterEach(func() {
-			cleanupBroker(brokerGUID)
+			broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
 		})
 
 		JustBeforeEach(func() {
@@ -140,7 +141,7 @@ var _ = Describe("Service Brokers", func() {
 		})
 
 		AfterEach(func() {
-			cleanupBroker(brokerGUID)
+			broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
 		})
 
 		JustBeforeEach(func() {

tests/e2e/service_instances_test.go

@@ -4,6 +4,7 @@ import (
 	"net/http"
 	"strings"
 
+	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 	"github.com/go-resty/resty/v2"
 	"github.com/google/uuid"
 	. "github.com/onsi/ginkgo/v2"
@@ -78,11 +79,10 @@ var _ = Describe("Service Instances", func() {
 		})
 
 		When("creating a managed service instance", func() {
+			var brokerGUID string
+
 			BeforeEach(func() {
-				brokerGUID := createBroker(serviceBrokerURL)
-				DeferCleanup(func() {
-					cleanupBroker(brokerGUID)
-				})
+				brokerGUID = createBroker(serviceBrokerURL)
 
 				var plansResp resourceList[resource]
 				catalogResp, err := adminClient.R().SetResult(&plansResp).Get("/v3/service_plans?service_broker_guids=" + brokerGUID)
@@ -110,6 +110,10 @@ var _ = Describe("Service Instances", func() {
 				}
 			})
 
+			AfterEach(func() {
+				broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
+			})
+
 			It("succeeds with a job redirect", func() {
 				Expect(httpError).NotTo(HaveOccurred())
 				Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted))
@@ -181,15 +185,18 @@ var _ = Describe("Service Instances", func() {
 		})
 
 		When("deleting a managed service instance", func() {
+			var brokerGUID string
+
 			BeforeEach(func() {
-				brokerGUID := createBroker(serviceBrokerURL)
-				DeferCleanup(func() {
-					cleanupBroker(brokerGUID)
-				})
+				brokerGUID = createBroker(serviceBrokerURL)
 
 				serviceInstanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID)
 			})
 
+			AfterEach(func() {
+				broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
+			})
+
 			It("succeeds with a job redirect", func() {
 				Expect(httpError).NotTo(HaveOccurred())
 				Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted))

tests/e2e/service_offerings_test.go

@@ -3,6 +3,7 @@ package e2e_test
 import (
 	"net/http"
 
+	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 	"github.com/go-resty/resty/v2"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -20,7 +21,7 @@ var _ = Describe("Service Offerings", func() {
 	})
 
 	AfterEach(func() {
-		cleanupBroker(brokerGUID)
+		broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
 	})
 
 	Describe("List", func() {

tests/e2e/service_plans_test.go

@@ -5,6 +5,7 @@ import (
 	"net/http"
 
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
+	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 	"github.com/BooleanCat/go-functional/v2/it/itx"
 	"github.com/go-resty/resty/v2"
 	. "github.com/onsi/ginkgo/v2"
@@ -23,7 +24,7 @@ var _ = Describe("Service Plans", func() {
 	})
 
 	AfterEach(func() {
-		cleanupBroker(brokerGUID)
+		broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete()
 	})
 
 	Describe("List", func() {

tests/helpers/broker/deleter.go

@@ -2,9 +2,12 @@ package broker
 
 import (
 	"context"
+	"fmt"
+	"time"
 
 	korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
 	"code.cloudfoundry.org/korifi/tools/k8s"
+	"github.com/BooleanCat/go-functional/v2/it/itx"
 	. "github.com/onsi/ginkgo/v2" //lint:ignore ST1001 this is a test file
 	. "github.com/onsi/gomega"    //lint:ignore ST1001 this is a test file
 
@@ -15,12 +18,28 @@ import (
 )
 
 type CatalogDeleter struct {
+	ctx                  context.Context
+	k8sClient            client.Client
 	rootNamespace        string
 	catalogLabelSelector client.MatchingLabels
+	maxRetries           int
 }
 
 func NewCatalogDeleter(rootNamespace string) *CatalogDeleter {
-	return &CatalogDeleter{rootNamespace: rootNamespace}
+	ctx := context.Background()
+
+	config, err := controllerruntime.GetConfig()
+	Expect(err).NotTo(HaveOccurred())
+
+	k8sClient, err := client.New(config, client.Options{Scheme: scheme.Scheme})
+	Expect(err).NotTo(HaveOccurred())
+
+	return &CatalogDeleter{
+		ctx:           ctx,
+		k8sClient:     k8sClient,
+		rootNamespace: rootNamespace,
+		maxRetries:    50,
+	}
 }
 
 func (d *CatalogDeleter) ForBrokerGUID(brokerGUID string) *CatalogDeleter {
@@ -42,60 +61,83 @@ func (d *CatalogDeleter) ForBrokerName(brokerName string) *CatalogDeleter {
 func (d *CatalogDeleter) Delete() {
 	GinkgoHelper()
 
-	ctx := context.Background()
-
-	config, err := controllerruntime.GetConfig()
-	Expect(err).NotTo(HaveOccurred())
-
-	k8sClient, err := client.New(config, client.Options{Scheme: scheme.Scheme})
-	Expect(err).NotTo(HaveOccurred())
-
 	servicePlans := &korifiv1alpha1.CFServicePlanList{}
-	Expect(k8sClient.List(ctx, servicePlans, client.InNamespace(d.rootNamespace), d.catalogLabelSelector)).To(Succeed())
+	Expect(d.k8sClient.List(d.ctx, servicePlans, client.InNamespace(d.rootNamespace), d.catalogLabelSelector)).To(Succeed())
 	for _, plan := range servicePlans.Items {
-		serviceBindings := &korifiv1alpha1.CFServiceBindingList{}
-		Expect(k8sClient.List(ctx, serviceBindings, client.MatchingLabels{
-			korifiv1alpha1.PlanGUIDLabelKey: plan.Name,
-		})).To(Succeed())
-		for _, binding := range serviceBindings.Items {
-			Expect(k8s.PatchResource(ctx, k8sClient, &binding, func() {
-				binding.Finalizers = nil
-			})).To(Succeed())
-
-			Expect(k8sClient.Delete(ctx, &binding)).To(Succeed())
-		}
-
-		serviceInstances := &korifiv1alpha1.CFServiceInstanceList{}
-		Expect(k8sClient.List(ctx, serviceInstances)).To(Succeed())
-		for _, instance := range serviceInstances.Items {
-			if instance.Spec.PlanGUID != plan.Name {
-				continue
-			}
-			Expect(k8s.Patch(ctx, k8sClient, &instance, func() {
-				instance.Finalizers = nil
-			})).To(Succeed())
-			Expect(k8sClient.Delete(ctx, &instance)).To(Succeed())
-		}
-
-		Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, &korifiv1alpha1.CFServiceBroker{
+		Expect(d.cleanupBindings(plan.Name)).To(Succeed())
+		Expect(d.cleanupInsances(plan.Name)).To(Succeed())
+		Expect(client.IgnoreNotFound(d.k8sClient.Delete(d.ctx, &korifiv1alpha1.CFServiceBroker{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace: d.rootNamespace,
 				Name:      plan.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel],
 			},
 		}))).To(Succeed())
 	}
 
-	Expect(k8sClient.DeleteAllOf(
-		ctx,
+	Expect(d.k8sClient.DeleteAllOf(
+		d.ctx,
 		&korifiv1alpha1.CFServiceOffering{},
 		client.InNamespace(d.rootNamespace),
 		d.catalogLabelSelector,
 	)).To(Succeed())
 
-	Expect(k8sClient.DeleteAllOf(
-		ctx,
+	Expect(d.k8sClient.DeleteAllOf(
+		d.ctx,
 		&korifiv1alpha1.CFServicePlan{},
 		client.InNamespace(d.rootNamespace),
 		d.catalogLabelSelector,
 	)).To(Succeed())
 }
+
+func (d *CatalogDeleter) cleanupBindings(planName string) error {
+	for retries := 0; retries < d.maxRetries; retries++ {
+		serviceBindings := &korifiv1alpha1.CFServiceBindingList{}
+		Expect(d.k8sClient.List(d.ctx, serviceBindings, client.MatchingLabels{
+			korifiv1alpha1.PlanGUIDLabelKey: planName,
+		})).To(Succeed())
+
+		if len(serviceBindings.Items) == 0 {
+			return nil
+		}
+
+		for _, binding := range serviceBindings.Items {
+			forceDelete(d.ctx, d.k8sClient, &binding)
+		}
+
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	return fmt.Errorf("failed to clean up service bindings for plan %q", planName)
+}
+
+func (d *CatalogDeleter) cleanupInsances(planName string) error {
+	for retries := 0; retries < d.maxRetries; retries++ {
+		serviceInstances := &korifiv1alpha1.CFServiceInstanceList{}
+		Expect(d.k8sClient.List(d.ctx, serviceInstances)).To(Succeed())
+
+		instancesForPlan := itx.FromSlice(serviceInstances.Items).
+			Filter(func(instance korifiv1alpha1.CFServiceInstance) bool {
+				return instance.Spec.PlanGUID == planName
+			}).Collect()
+
+		if len(instancesForPlan) == 0 {
+			return nil
+		}
+
+		for _, instance := range instancesForPlan {
+			forceDelete(d.ctx, d.k8sClient, &instance)
+		}
+
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	return fmt.Errorf("failed to clean up service bindings for plan %q", planName)
+}
+
+func forceDelete[T any, PT k8s.ObjectWithDeepCopy[T]](ctx context.Context, k8sClient client.Client, obj PT) {
+	Expect(k8s.PatchResource(ctx, k8sClient, obj, func() {
+		obj.SetFinalizers(nil)
+	})).To(Succeed())
+
+	Expect(k8sClient.Delete(ctx, obj)).To(Succeed())
+}

tests/smoke/catalog_test.go

@@ -2,6 +2,7 @@ package smoke_test
 
 import (
 	"code.cloudfoundry.org/korifi/tests/helpers"
+	"code.cloudfoundry.org/korifi/tests/helpers/broker"
 
 	"github.com/BooleanCat/go-functional/v2/it"
 	"github.com/google/uuid"
@@ -25,7 +26,7 @@ var _ = Describe("Service Catalog", func() {
 	})
 
 	AfterEach(func() {
-		cleanupBroker(brokerName)
+		broker.NewCatalogDeleter(sharedData.RootNamespace).ForBrokerName(brokerName).Delete()
 	})
 
 	Describe("cf service-brokers", func() {