add unit tests for the mcp controller (part 1)

Author: Diaphteiros

internal/config/config_accessrequest.go

@@ -6,6 +6,9 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
+var _ Validatable = &AccessRequestConfig{}
+var _ Completable = &AccessRequestConfig{}
+
 type AccessRequestConfig struct {
 	// If set, only AccessRequests that match the selector will be reconciled.
 	Selector *Selector `json:"selector,omitempty"`

internal/config/config_managedcontrolplane.go

@@ -1,11 +1,17 @@
 package config
 
 import (
+	"fmt"
+
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	commonapi "github.com/openmcp-project/openmcp-operator/api/common"
+	corev2alpha1 "github.com/openmcp-project/openmcp-operator/api/core/v2alpha1"
 )
 
+var _ Defaultable = &ManagedControlPlaneConfig{}
+var _ Validatable = &ManagedControlPlaneConfig{}
+
 type ManagedControlPlaneConfig struct {
 	// MCPClusterPurpose is the purpose that is used for ClusterRequests created for ManagedControlPlane resources.
 	MCPClusterPurpose string `json:"mcpClusterPurpose"`
@@ -22,6 +28,10 @@ type ManagedControlPlaneConfig struct {
 }
 
 func (c *ManagedControlPlaneConfig) Default(_ *field.Path) error {
+	c.StandardOIDCProvider.Default()
+	if c.StandardOIDCProvider.Name == "" {
+		c.StandardOIDCProvider.Name = corev2alpha1.DefaultOIDCProviderName
+	}
 	return nil
 }
 
@@ -31,6 +41,18 @@ func (c *ManagedControlPlaneConfig) Validate(fldPath *field.Path) error {
 	if c.MCPClusterPurpose == "" {
 		errs = append(errs, field.Required(fldPath.Child("mcpClusterPurpose"), "MCP cluster purpose must be set"))
 	}
+	if c.ReconcileMCPEveryXDays < 0 {
+		errs = append(errs, field.Invalid(fldPath.Child("reconcileMCPEveryXDays"), c.ReconcileMCPEveryXDays, "reconcile interval must be 0 or greater"))
+	}
+	if c.StandardOIDCProvider == nil {
+		oidcFldPath := fldPath.Child("standardOIDCProvider")
+		if len(c.StandardOIDCProvider.RoleBindings) > 0 {
+			errs = append(errs, field.Forbidden(oidcFldPath.Child("roleBindings"), "role bindings are specified in the MCP spec and may not be set in the config"))
+		}
+		if c.StandardOIDCProvider.Name != "" && c.StandardOIDCProvider.Name != corev2alpha1.DefaultOIDCProviderName {
+			errs = append(errs, field.Invalid(oidcFldPath.Child("name"), c.StandardOIDCProvider.Name, fmt.Sprintf("standard OIDC provider name must be '%s' or left empty (in which case it will be defaulted)", corev2alpha1.DefaultOIDCProviderName)))
+		}
+	}
 
 	return errs.ToAggregate()
 }

internal/config/config_scheduler.go

@@ -11,6 +11,10 @@ import (
 	clustersv1alpha1 "github.com/openmcp-project/openmcp-operator/api/clusters/v1alpha1"
 )
 
+var _ Defaultable = &SchedulerConfig{}
+var _ Validatable = &SchedulerConfig{}
+var _ Completable = &SchedulerConfig{}
+
 type SchedulerConfig struct {
 	// Scope determines whether the scheduler considers all clusters or only the ones in the same namespace as the ClusterRequest.
 	// Defaults to "Namespaced".

internal/controllers/managedcontrolplane/access.go

@@ -11,6 +11,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	"github.com/openmcp-project/controller-utils/pkg/collections"
 	ctrlutils "github.com/openmcp-project/controller-utils/pkg/controller"
 	errutils "github.com/openmcp-project/controller-utils/pkg/errors"
 	"github.com/openmcp-project/controller-utils/pkg/logging"
@@ -48,16 +49,21 @@ func (r *ManagedControlPlaneReconciler) manageAccessRequests(ctx context.Context
 	}
 
 	// remove conditions for AccessRequests that are neither required nor in deletion (= have been deleted already)
-	removeConditions := sets.New[string]()
-	for _, con := range mcp.Status.Conditions {
-		if !strings.HasPrefix(con.Type, corev2alpha1.ConditionPrefixOIDCAccessReady) {
-			continue
-		}
-		providerName := strings.TrimPrefix(con.Type, corev2alpha1.ConditionPrefixOIDCAccessReady)
-		if _, ok := updatedAccessRequests[providerName]; !ok && !accessRequestsInDeletion.Has(providerName) {
-			removeConditions.Insert(con.Type)
+	// first, build a set of OIDC provider names that have a condition in the MCP status
+	removeConditions := collections.AggregateSlice(mcp.Status.Conditions, func(con metav1.Condition, cur sets.Set[string]) sets.Set[string] {
+		if providerName, found := strings.CutPrefix(con.Type, corev2alpha1.ConditionPrefixOIDCAccessReady); found {
+			cur.Insert(providerName)
 		}
-	}
+		return cur
+	}, sets.New[string]())
+	// then, remove all conditions from it which belong to updated AccessRequests
+	removeConditions = removeConditions.Difference(sets.KeySet(updatedAccessRequests))
+	// and all conditions that are in deletion
+	removeConditions = removeConditions.Difference(accessRequestsInDeletion)
+	// now, add the condition prefix again
+	removeConditions = collections.ProjectMapToMap(removeConditions, func(providerName string, _ sets.Empty) (string, sets.Empty) {
+		return corev2alpha1.ConditionPrefixOIDCAccessReady + providerName, sets.Empty{}
+	})
 
 	everythingReady := accessRequestsInDeletion.Len() == 0 && accessSecretsInDeletion.Len() == 0 && allAccessReady
 	if everythingReady {
@@ -111,7 +117,7 @@ func (r *ManagedControlPlaneReconciler) createOrUpdateDesiredAccessRequests(ctx
 			}
 			ar.Labels[corev2alpha1.MCPLabel] = mcp.Name
 			ar.Labels[apiconst.ManagedByLabel] = ControllerName
-			ar.Labels[corev2alpha1.OIDCProviderLabel] = corev2alpha1.DefaultOIDCProviderName
+			ar.Labels[corev2alpha1.OIDCProviderLabel] = oidc.Name
 
 			return nil
 		}); err != nil {
@@ -120,7 +126,7 @@ func (r *ManagedControlPlaneReconciler) createOrUpdateDesiredAccessRequests(ctx
 			createCon(corev2alpha1.ConditionAllAccessReady, metav1.ConditionFalse, cconst.ReasonWaitingForAccessRequest, "Error creating/updating AccessRequest for OIDC provider "+oidc.Name)
 			return nil, rerr
 		}
-		updatedAccessRequests[corev2alpha1.DefaultOIDCProviderName] = ar
+		updatedAccessRequests[oidc.Name] = ar
 	}
 
 	return updatedAccessRequests, nil
@@ -154,7 +160,7 @@ func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessRequests(ctx contex
 		if ar.Spec.OIDCProvider != nil {
 			providerName = ar.Spec.OIDCProvider.Name
 		}
-		accessRequestsInDeletion.Insert(ar.Name)
+		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")
@@ -177,6 +183,7 @@ func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessRequests(ctx contex
 }
 
 // deleteUndesiredAccessSecrets deletes all access secrets belonging to the ManagedControlPlane that are not copied from an up-to-date AccessRequest.
+// It also deletes all mappings for which no secret exists from the ManagedControlPlane status.
 // It returns a set of OIDC provider names for which the AccessRequest secrets are still in deletion.
 func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessSecrets(ctx context.Context, mcp *corev2alpha1.ManagedControlPlane, updatedAccessRequests map[string]*clustersv1alpha1.AccessRequest, createCon func(conType string, status metav1.ConditionStatus, reason, message string)) (sets.Set[string], errutils.ReasonableError) {
 	log := logging.FromContextOrPanic(ctx)
@@ -223,6 +230,11 @@ func (r *ManagedControlPlaneReconciler) deleteUndesiredAccessSecrets(ctx context
 		return accessSecretsInDeletion, rerr
 	}
 
+	// delete all references to access secrets that are being deleted from the ManagedControlPlane status
+	for providerName := range accessSecretsInDeletion {
+		delete(mcp.Status.Access, providerName)
+	}
+
 	return accessSecretsInDeletion, nil
 }
 
@@ -244,7 +256,7 @@ func (r *ManagedControlPlaneReconciler) syncAccessSecrets(ctx context.Context, m
 			// copy access request secret and reference it in the ManagedControlPlane status
 			arSecret := &corev1.Secret{}
 			arSecret.Name = ar.Status.SecretRef.Name
-			arSecret.Namespace = ar.Status.SecretRef.Namespace
+			arSecret.Namespace = ar.Namespace
 			if err := r.PlatformCluster.Client().Get(ctx, client.ObjectKeyFromObject(arSecret), arSecret); err != nil {
 				rerr := errutils.WithReason(fmt.Errorf("error getting AccessRequest secret '%s/%s': %w", arSecret.Namespace, arSecret.Name, err), cconst.ReasonPlatformClusterInteractionProblem)
 				createCon(corev2alpha1.ConditionPrefixOIDCAccessReady+providerName, metav1.ConditionFalse, rerr.Reason(), rerr.Error())

internal/controllers/managedcontrolplane/controller.go

@@ -204,7 +204,8 @@ func (r *ManagedControlPlaneReconciler) handleCreateOrUpdate(ctx context.Context
 
 		log.Info("ClusterRequest not found, creating it", "clusterRequestName", cr.Name, "clusterRequestNamespace", cr.Namespace, "purpose", r.Config.MCPClusterPurpose)
 		cr.Spec = clustersv1alpha1.ClusterRequestSpec{
-			Purpose: r.Config.MCPClusterPurpose,
+			Purpose:                r.Config.MCPClusterPurpose,
+			WaitForClusterDeletion: ptr.To(true),
 		}
 		if err := r.PlatformCluster.Client().Create(ctx, cr); err != nil {
 			rr.ReconcileError = errutils.WithReason(fmt.Errorf("error creating ClusterRequest '%s/%s': %w", cr.Namespace, cr.Name, err), cconst.ReasonPlatformClusterInteractionProblem)