add unit tests for the mcp controller (part 3)

Author: Diaphteiros

api/clusters/v1alpha1/cluster_types.go

@@ -130,8 +130,8 @@ func (c *Cluster) GetTenancyCount() int {
 func (c *Cluster) GetRequestUIDs() sets.Set[string] {
 	res := sets.New[string]()
 	for _, fin := range c.Finalizers {
-		if strings.HasPrefix(fin, RequestFinalizerOnClusterPrefix) {
-			res.Insert(strings.TrimPrefix(fin, RequestFinalizerOnClusterPrefix))
+		if uid, ok := strings.CutPrefix(fin, RequestFinalizerOnClusterPrefix); ok {
+			res.Insert(uid)
 		}
 	}
 	return res

api/clusters/v1alpha1/constants/reasons.go

@@ -13,8 +13,14 @@ const (
 	ReasonInternalError = "InternalError"
 	// ReasonWaitingForClusterRequest indicates that something is waiting for a ClusterRequest to become ready.
 	ReasonWaitingForClusterRequest = "WaitingForClusterRequest"
+	// ReasonWaitingForClusterRequestDeletion indicates that something is waiting for a ClusterRequest to be deleted.
+	ReasonWaitingForClusterRequestDeletion = "WaitingForClusterRequestDeletion"
 	// ReasonWaitingForAccessRequest indicates that something is waiting for an AccessRequest to become ready.
 	ReasonWaitingForAccessRequest = "WaitingForAccessRequest"
+	// ReasonWaitingForAccessRequestDeletion indicates that something is waiting for an AccessRequest to be deleted.
+	ReasonWaitingForAccessRequestDeletion = "WaitingForAccessRequestDeletion"
 	// ReasonWaitingForServices indicates that something is waiting for one or more service providers to do something.
 	ReasonWaitingForServices = "WaitingForServices"
+	// ReasonWaitingForServiceDeletion indicates that something is waiting for a service to be deleted.
+	ReasonWaitingForServiceDeletion = "WaitingForServiceDeletion"
 )

api/core/v2alpha1/constants.go

@@ -21,7 +21,7 @@ const (
 	ConditionMeta = "Meta"
 
 	ConditionClusterRequestReady       = "ClusterRequestReady"
-	ConditionPrefixOIDCAccessReady     = "OIDCAccessReady_"
+	ConditionPrefixOIDCAccessReady     = "OIDCAccessReady:"
 	ConditionAllAccessReady            = "AllAccessReady"
 	ConditionAllServicesDeleted        = "AllServicesDeleted"
 	ConditionAllClusterRequestsDeleted = "AllClusterRequestsDeleted"

internal/controllers/managedcontrolplane/access.go

@@ -69,7 +69,11 @@ func (r *ManagedControlPlaneReconciler) manageAccessRequests(ctx context.Context
 	if everythingReady {
 		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionTrue, "", "All accesses are ready")
 	} else {
-		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "Not all accesses are ready")
+		reason := cconst.ReasonWaitingForAccessRequest
+		if allAccessReady {
+			reason = cconst.ReasonWaitingForAccessRequestDeletion
+		}
+		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, reason, "Not all accesses are ready")
 	}
 
 	return everythingReady, removeConditions, nil
@@ -163,7 +167,7 @@ func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessRequests(ctx contex
 		accessRequestsInDeletion.Insert(providerName)
 		if !ar.DeletionTimestamp.IsZero() {
 			log.Debug("Waiting for deletion of AccessRequest that is no longer required", "accessRequestName", ar.Name, "accessRequestNamespace", ar.Namespace, "oidcProviderName", providerName)
-			createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "AccessRequest is being deleted")
+			createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequestDeletion, "AccessRequest is being deleted")
 			continue
 		}
 		log.Debug("Deleting AccessRequest that is no longer needed", "accessRequestName", ar.Name, "accessRequestNamespace", ar.Namespace, "oidcProviderName", providerName)
@@ -172,10 +176,10 @@ func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessRequests(ctx contex
 			errs.Append(rerr)
 			createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, rerr.Reason(), rerr.Error())
 		}
-		createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "AccessRequest is being deleted")
+		createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequestDeletion, "AccessRequest is being deleted")
 	}
 	if rerr := errs.Aggregate(); rerr != nil {
-		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "Error deleting AccessRequests that are no longer needed")
+		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequestDeletion, "Error deleting AccessRequests that are no longer needed")
 		return accessRequestsInDeletion, rerr
 	}
 
@@ -214,7 +218,7 @@ func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessSecrets(ctx context
 		accessSecretsInDeletion.Insert(providerName)
 		if !mcpSecret.DeletionTimestamp.IsZero() {
 			log.Debug("Waiting for deletion of access secret that is no longer required", "secretName", mcpSecret.Name, "secretNamespace", mcpSecret.Namespace, "oidcProviderName", providerName)
-			createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "AccessRequest secret is being deleted")
+			createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequestDeletion, "AccessRequest secret is being deleted")
 			continue
 		}
 		log.Debug("Deleting access secret that is no longer required", "secretName", mcpSecret.Name, "secretNamespace", mcpSecret.Namespace, "oidcProviderName", providerName)
@@ -223,10 +227,10 @@ func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessSecrets(ctx context
 			errs.Append(rerr)
 			createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, rerr.Reason(), rerr.Error())
 		}
-		createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "access secret is being deleted")
+		createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequestDeletion, "access secret is being deleted")
 	}
 	if rerr := errs.Aggregate(); rerr != nil {
-		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "Error deleting access secrets that are no longer needed")
+		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequestDeletion, "Error deleting access secrets that are no longer needed")
 		return accessSecretsInDeletion, rerr
 	}
 

internal/controllers/managedcontrolplane/clusters.go

@@ -31,8 +31,8 @@ func (r *ManagedControlPlaneReconciler) deleteRelatedClusterRequests(ctx context
 
 	// identify cluster request finalizers
 	for _, fin := range mcp.Finalizers {
-		if strings.HasPrefix(fin, corev2alpha1.ClusterRequestFinalizerPrefix) {
-			crNames.Insert(strings.TrimPrefix(fin, corev2alpha1.ClusterRequestFinalizerPrefix))
+		if crName, ok := strings.CutPrefix(fin, corev2alpha1.ClusterRequestFinalizerPrefix); ok {
+			crNames.Insert(crName)
 		}
 	}
 

internal/controllers/managedcontrolplane/controller.go

@@ -73,11 +73,6 @@ func (r *ManagedControlPlaneReconciler) Reconcile(ctx context.Context, req recon
 	log.Info("Starting reconcile")
 	rr := r.reconcile(ctx, req)
 
-	if rr.Result.IsZero() && r.Config.ReconcileMCPEveryXDays > 0 {
-		// requeue the MCP for periodic reconciliation
-		rr.Result.RequeueAfter = time.Duration(r.Config.ReconcileMCPEveryXDays) * 24 * time.Hour
-	}
-
 	// status update
 	return ctrlutils.NewOpenMCPStatusUpdaterBuilder[*corev2alpha1.ManagedControlPlane]().
 		WithNestedStruct("Status").
@@ -278,7 +273,7 @@ func (r *ManagedControlPlaneReconciler) handleDelete(ctx context.Context, mcp *c
 				msg.WriteString(fmt.Sprintf("[%s]%s.%s, ", providerName, res.GetKind(), res.GetAPIVersion()))
 			}
 		}
-		createCon(corev2alpha1.ConditionAllServicesDeleted, metav1.ConditionFalse, cconst.ReasonWaitingForServices, strings.TrimSuffix(msg.String(), ", "))
+		createCon(corev2alpha1.ConditionAllServicesDeleted, metav1.ConditionFalse, cconst.ReasonWaitingForServiceDeletion, strings.TrimSuffix(msg.String(), ", "))
 		rr.SmartRequeue = ctrlutils.SR_BACKOFF
 		return rr
 	}
@@ -295,7 +290,7 @@ func (r *ManagedControlPlaneReconciler) handleDelete(ctx context.Context, mcp *c
 	}
 	if !accessReady {
 		log.Info("Waiting for AccessRequests to be deleted")
-		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "Waiting for AccessRequests to be deleted")
+		createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequestDeletion, "Waiting for AccessRequests to be deleted")
 		rr.SmartRequeue = ctrlutils.SR_BACKOFF
 		return rr
 	}
@@ -314,7 +309,8 @@ func (r *ManagedControlPlaneReconciler) handleDelete(ctx context.Context, mcp *c
 		if !ok {
 			return false
 		}
-		return strings.HasPrefix(fin, corev2alpha1.ClusterRequestFinalizerPrefix) && !remainingCRs.Has(strings.TrimPrefix(fin, corev2alpha1.ClusterRequestFinalizerPrefix))
+		crName, ok := strings.CutPrefix(fin, corev2alpha1.ClusterRequestFinalizerPrefix)
+		return ok && !remainingCRs.Has(crName)
 	})...)
 	if len(finalizersToRemove) > 0 {
 		log.Debug("Removing ClusterRequest finalizers for deleted ClusterRequests from MCP", "finalizers", strings.Join(sets.List(finalizersToRemove), ", "))
@@ -336,7 +332,7 @@ func (r *ManagedControlPlaneReconciler) handleDelete(ctx context.Context, mcp *c
 	if remainingCRs.Len() > 0 {
 		tmp := strings.Join(sets.List(remainingCRs), ", ")
 		log.Info("Waiting for ClusterRequests to be deleted", "remainingClusterRequests", tmp)
-		createCon(corev2alpha1.ConditionAllClusterRequestsDeleted, metav1.ConditionFalse, cconst.ReasonWaitingForClusterRequest, fmt.Sprintf("Waiting for the following ClusterRequests to be deleted: %s", tmp))
+		createCon(corev2alpha1.ConditionAllClusterRequestsDeleted, metav1.ConditionFalse, cconst.ReasonWaitingForClusterRequestDeletion, fmt.Sprintf("Waiting for the following ClusterRequests to be deleted: %s", tmp))
 		rr.SmartRequeue = ctrlutils.SR_BACKOFF
 		return rr
 	}
@@ -353,6 +349,7 @@ func (r *ManagedControlPlaneReconciler) handleDelete(ctx context.Context, mcp *c
 		}
 	}
 	createCon(corev2alpha1.ConditionMeta, metav1.ConditionTrue, "", "MCP finalizer removed")
+	rr.Result.RequeueAfter = 0
 	if len(mcp.Finalizers) == 0 {
 		// if we just removed the last finalizer on the MCP
 		// (which should usually be the case, unless something external added one)

internal/controllers/managedcontrolplane/controller_test.go

@@ -3,6 +3,7 @@ package managedcontrolplane_test
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -104,7 +105,7 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 		Expect(env.Client(onboarding).Get(env.Ctx, client.ObjectKeyFromObject(mcp), mcp)).To(Succeed())
 		Expect(res.RequeueAfter).To(BeNumerically(">", 0))
 		Expect(res.RequeueAfter).To(BeNumerically("<", 1*time.Minute))
-		Expect(mcp.Finalizers).To(ConsistOf(
+		Expect(mcp.Finalizers).To(ContainElements(
 			corev2alpha1.MCPFinalizer,
 			corev2alpha1.ClusterRequestFinalizerPrefix+mcp.Name,
 		))
@@ -265,7 +266,7 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 			MatchCondition(TestCondition().
 				WithType(corev2alpha1.ConditionAllAccessReady).
 				WithStatus(metav1.ConditionFalse).
-				WithReason(cconst.ReasonWaitingForAccessRequest)),
+				WithReason(cconst.ReasonWaitingForAccessRequestDeletion)),
 		))
 		removedOIDCIdx := -1
 		for i, oidc := range oidcProviders {
@@ -276,7 +277,7 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 					MatchCondition(TestCondition().
 						WithType(corev2alpha1.ConditionPrefixOIDCAccessReady + oidc.Name).
 						WithStatus(metav1.ConditionFalse).
-						WithReason(cconst.ReasonWaitingForAccessRequest),
+						WithReason(cconst.ReasonWaitingForAccessRequestDeletion),
 					)))
 				Expect(mcp.Status.Access).ToNot(HaveKey(oidc.Name))
 			} else {
@@ -374,11 +375,53 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 		Expect(env.Client(onboarding).Get(env.Ctx, client.ObjectKeyFromObject(mcp), mcp)).To(Succeed())
 		Expect(res.RequeueAfter).To(BeNumerically(">", 0))
 		Expect(res.RequeueAfter).To(BeNumerically("<", 1*time.Minute))
-		// TODO
+		serviceResources := []client.Object{
+			&corev1.ConfigMap{},
+			&corev1.ServiceAccount{},
+			&corev1.Secret{},
+		}
+		for _, obj := range serviceResources {
+			obj.SetName(mcp.Name)
+			obj.SetNamespace(mcp.Namespace)
+			Expect(env.Client(onboarding).Get(env.Ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed())
+			Expect(obj.GetDeletionTimestamp().IsZero()).To(BeFalse())
+		}
+		Expect(mcp.Status.Conditions).To(ContainElements(
+			MatchCondition(TestCondition().
+				WithType(corev2alpha1.ConditionAllServicesDeleted).
+				WithStatus(metav1.ConditionFalse).
+				WithReason(cconst.ReasonWaitingForServiceDeletion)),
+		))
+		for _, oidc := range oidcProviders {
+			By("verifying AccessRequest does not have a deletion timestamp for oidc provider: " + oidc.Name)
+			ar := &clustersv1alpha1.AccessRequest{}
+			ar.SetName(ctrlutils.K8sNameHash(mcp.Name, oidc.Name))
+			ar.SetNamespace(platformNamespace)
+			Expect(env.Client(platform).Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
+			Expect(ar.DeletionTimestamp.IsZero()).To(BeTrue())
+		}
+		for _, obj := range []client.Object{cr, cr2, cr3} {
+			By("verifying ClusterRequest does not have a deletion timestamp: " + obj.GetName())
+			Expect(env.Client(platform).Get(env.Ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed())
+			Expect(obj.GetDeletionTimestamp().IsZero()).To(BeTrue())
+		}
 
 		// remove service finalizers
 		By("fake: removing service finalizers")
-		// TODO
+		for _, obj := range serviceResources {
+			By("fake: removing finalizer from service resource: " + obj.GetObjectKind().GroupVersionKind().Kind)
+			Expect(env.Client(onboarding).Get(env.Ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed())
+			controllerutil.RemoveFinalizer(obj, "dummy")
+			Expect(env.Client(onboarding).Update(env.Ctx, obj)).To(Succeed())
+		}
+		newFins := []string{}
+		for _, fin := range mcp.Finalizers {
+			if !strings.HasPrefix(fin, corev2alpha1.ServiceDependencyFinalizerPrefix) {
+				newFins = append(newFins, fin)
+			}
+		}
+		mcp.Finalizers = newFins
+		Expect(env.Client(onboarding).Update(env.Ctx, mcp)).To(Succeed())
 
 		// reconcile the MCP again
 		// expected outcome:
@@ -399,7 +442,7 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 			MatchCondition(TestCondition().
 				WithType(corev2alpha1.ConditionAllAccessReady).
 				WithStatus(metav1.ConditionFalse).
-				WithReason(cconst.ReasonWaitingForAccessRequest)),
+				WithReason(cconst.ReasonWaitingForAccessRequestDeletion)),
 		))
 		for _, oidc := range oidcProviders {
 			By("verifying AccessRequest and access secret deletion status for oidc provider: " + oidc.Name)
@@ -412,7 +455,7 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 				MatchCondition(TestCondition().
 					WithType(corev2alpha1.ConditionPrefixOIDCAccessReady + oidc.Name).
 					WithStatus(metav1.ConditionFalse).
-					WithReason(cconst.ReasonWaitingForAccessRequest),
+					WithReason(cconst.ReasonWaitingForAccessRequestDeletion),
 				),
 			))
 		}
@@ -464,6 +507,12 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 			ar.SetNamespace(platformNamespace)
 			Expect(env.Client(platform).Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(MatchError(apierrors.IsNotFound, "IsNotFound"))
 		}
+		Expect(mcp.Status.Conditions).To(ContainElements(
+			MatchCondition(TestCondition().
+				WithType(corev2alpha1.ConditionAllClusterRequestsDeleted).
+				WithStatus(metav1.ConditionFalse).
+				WithReason(cconst.ReasonWaitingForClusterRequestDeletion)),
+		))
 
 		// remove finalizers from cr2 and cr3
 		By("fake: removing finalizers from additional ClusterRequests")
@@ -486,6 +535,12 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 		By("verifying ClusterRequest deletion status")
 		Expect(env.Client(platform).Get(env.Ctx, client.ObjectKeyFromObject(cr), cr)).To(Succeed())
 		Expect(cr.GetDeletionTimestamp().IsZero()).To(BeFalse(), "ClusterRequest should be marked for deletion")
+		Expect(mcp.Status.Conditions).To(ContainElements(
+			MatchCondition(TestCondition().
+				WithType(corev2alpha1.ConditionAllClusterRequestsDeleted).
+				WithStatus(metav1.ConditionFalse).
+				WithReason(cconst.ReasonWaitingForClusterRequestDeletion)),
+		))
 
 		// remove finalizer from cr
 		By("fake: removing finalizer from primary ClusterRequest")
@@ -500,7 +555,7 @@ var _ = Describe("ManagedControlPlane Controller", func() {
 		By("fifth MCP reconciliation after delete")
 		res = env.ShouldReconcile(mcpRec, testutils.RequestFromObject(mcp))
 		Expect(env.Client(onboarding).Get(env.Ctx, client.ObjectKeyFromObject(mcp), mcp)).To(MatchError(apierrors.IsNotFound, "IsNotFound"))
-		Expect(res.RequeueAfter).To(BeNumerically("~", int64(rec.Config.ReconcileMCPEveryXDays)*24*int64(time.Hour), int64(time.Second)))
+		Expect(res.IsZero()).To(BeTrue())
 		Expect(env.Client(platform).Get(env.Ctx, client.ObjectKeyFromObject(cr), cr)).To(MatchError(apierrors.IsNotFound, "IsNotFound"))
 	})
 

internal/controllers/managedcontrolplane/services.go

@@ -34,8 +34,8 @@ func (r *ManagedControlPlaneReconciler) deleteDependingServices(ctx context.Cont
 
 	// identify service finalizers
 	for _, fin := range mcp.Finalizers {
-		if strings.HasPrefix(fin, corev2alpha1.ServiceDependencyFinalizerPrefix) {
-			serviceProviderNames.Insert(strings.TrimPrefix(fin, corev2alpha1.ServiceDependencyFinalizerPrefix))
+		if service, ok := strings.CutPrefix(fin, corev2alpha1.ServiceDependencyFinalizerPrefix); ok {
+			serviceProviderNames.Insert(service)
 		}
 	}
 

internal/controllers/managedcontrolplane/testdata/test-01/onboarding/mcp-01.yaml

@@ -3,6 +3,9 @@ kind: ManagedControlPlane
 metadata:
   name: mcp-01
   namespace: test
+  finalizers:
+  - services.openmcp.cloud/sp-01
+  - services.openmcp.cloud/sp-02
 spec:
   iam:
     roleBindings:

internal/controllers/managedcontrolplane/testdata/test-01/onboarding/sr-01-01.yaml

@@ -0,0 +1,9 @@
+# this is a fake service resource
+# we use standard k8s resources for testing purposes, so we don't have to add a custom resource definition or schema
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: mcp-01
+  namespace: test
+  finalizers:
+  - dummy