accessrequest controller now also adds profile label

Author: Diaphteiros

api/clusters/v1alpha1/constants.go

@@ -81,6 +81,8 @@ const (
 	DeleteWithoutRequestsLabel = GroupName + "/delete-without-requests"
 	// ProviderLabel is used to indicate the provider that is responsible for an AccessRequest.
 	ProviderLabel = "provider." + GroupName
+	// ProfileLabel is used to make the profile information easily accessible for the ClusterProviders.
+	ProfileLabel = "profile." + GroupName
 )
 
 const (

docs/controller/accessrequest.md

@@ -4,9 +4,17 @@ The _AccessRequest Controller_ is responsible for labelling `AccessRequest` reso
 
 This is needed because the information, which ClusterProvider is responsible for answering the `AccessRequest` is contained in the referenced `ClusterProfile`. Depending on `AccessRequest`'s spec, a `Cluster` and potentially also a `ClusterRequest` must be fetched before the `ClusterProfile` is known, which then has to be fetched too. If multiple ClusterProviders are running in the cluster, all of them would need to fetch these resources, only for all but one of them to notice that they are not responsible and don't have to do anything.
 
-To increase performance and simplify reconciliation logic in the individual ClusterProviders, this central AccessRequest controller takes over the task of figuring out the responsible ClusterProvider and adds a `provider.clusters.openmcp.cloud` label with its name to the `AccessRequest` resource. It reacts only on resources which do not yet have this label, so it should reconcile each `AccessRequest` only once (excluding repeated reconciliations due to errors).
+To increase performance and simplify reconciliation logic in the individual ClusterProviders, this central AccessRequest controller takes over the task of figuring out the ClusterProfile and the responsible ClusterProvider and it adds these as labels to the `AccessRequest` resource. It reacts only on resources which do not yet have both of these labels set, so it should reconcile each `AccessRequest` only once (excluding repeated reconciliations due to errors).
 
-ClusterProviders should only reconcile `AccessRequest` resources where the value of the `provider.clusters.openmcp.cloud` label matches their own provider name and ignore resources with other values or if the label is missing completely.
+The added labels are:
+```yaml
+provider.clusters.openmcp.cloud: <provider-name>
+profile.clusters.openmcp.cloud: <profile-name>
+```
+
+ClusterProviders should only reconcile `AccessRequest` resources where both labels are set and the value of the provider label matches their own provider name. Resources where either label is missing or the value of the provider label does not match the own provider name must be ignored.
+
+Note that if a reconciled `AccessRequest` already has one of the labels set, but its value differs from the expected one, the controller will log an error, but not update the resource in any way, to not accidentally move the responsibility from one provider to another. This also means that `AccessRequest` resources that have only one of the labels set, and that one to a wrong value, will not be handled - this controller won't update the resource and the ClusterProvider should not pick it up because one of the labels is missing. It is therefore strongly recommended to not set the labels when creating a new `AccessRequest` resource.
 
 ## Configuration
 

internal/controllers/accessrequest/controller.go

@@ -140,7 +140,7 @@ func (r *AccessRequestReconciler) reconcile(ctx context.Context, req reconcile.R
 	// fetch ClusterProfile resource
 	cp := &clustersv1alpha1.ClusterProfile{}
 	cp.SetName(c.Spec.Profile)
-	log.Debug("Fetching ClusterProfile", "clusterProfileName", cp.Name)
+	log.Debug("Fetching ClusterProfile", "profileName", cp.Name)
 	if err := r.PlatformCluster.Client().Get(ctx, client.ObjectKeyFromObject(cp), cp); err != nil {
 		if apierrors.IsNotFound(err) {
 			rr.ReconcileError = errutils.WithReason(fmt.Errorf("ClusterProfile '%s' not found", cp.Name), cconst.ReasonInvalidReference)
@@ -150,13 +150,22 @@ func (r *AccessRequestReconciler) reconcile(ctx context.Context, req reconcile.R
 		return rr
 	}
 
-	log.Info("Identified responsible ClusterProvider", "providerName", cp.Spec.ProviderRef.Name)
-	if err := ctrlutils.EnsureLabel(ctx, r.PlatformCluster.Client(), ar, clustersv1alpha1.ProviderLabel, cp.Spec.ProviderRef.Name, true); err != nil {
-		if ctrlutils.IsMetadataEntryAlreadyExistsError(err) {
-			log.Error(err, "label '%s' already set on resource '%s'", clustersv1alpha1.ProviderLabel, req.String())
+	log.Info("Identified responsible ClusterProvider", "providerName", cp.Spec.ProviderRef.Name, "profileName", cp.Name)
+	arOld := ar.DeepCopy()
+	if err := ctrlutils.EnsureLabel(ctx, nil, ar, clustersv1alpha1.ProviderLabel, cp.Spec.ProviderRef.Name, false); err != nil {
+		if e, ok := err.(*ctrlutils.MetadataEntryAlreadyExistsError); ok {
+			log.Error(err, "label '%s' already set on resource '%s', but with value '%s' instead of the desired value '%s'", e.Key, req.String(), e.ActualValue, e.DesiredValue)
 			return rr
 		}
-		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error setting label '%s' with value '%s' on resource '%s': %w", clustersv1alpha1.ProviderLabel, cp.Spec.ProviderRef.Name, req.String(), err), cconst.ReasonPlatformClusterInteractionProblem)
+	}
+	if err := ctrlutils.EnsureLabel(ctx, nil, ar, clustersv1alpha1.ProfileLabel, cp.Name, false); err != nil {
+		if e, ok := err.(*ctrlutils.MetadataEntryAlreadyExistsError); ok {
+			log.Error(err, "label '%s' already set on resource '%s', but with value '%s' instead of the desired value '%s'", e.Key, req.String(), e.ActualValue, e.DesiredValue)
+			return rr
+		}
+	}
+	if err := r.PlatformCluster.Client().Patch(ctx, ar, client.MergeFrom(arOld)); err != nil {
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching labels on resource '%s': %w", req.String(), err), cconst.ReasonPlatformClusterInteractionProblem)
 		return rr
 	}
 
@@ -169,8 +178,11 @@ func (r *AccessRequestReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		// watch AccessRequest resources
 		For(&clustersv1alpha1.AccessRequest{}).
 		WithEventFilter(predicate.And(
-			// ignore resources that already have the provider label set
-			predicate.Not(ctrlutils.HasLabelPredicate(clustersv1alpha1.ProviderLabel, "")),
+			// ignore resources that already have the provider AND profile label set
+			predicate.Not(predicate.And(
+				ctrlutils.HasLabelPredicate(clustersv1alpha1.ProviderLabel, ""),
+				ctrlutils.HasLabelPredicate(clustersv1alpha1.ProfileLabel, ""),
+			)),
 			ctrlutils.LabelSelectorPredicate(r.Config.Selector.Completed()),
 			predicate.Or(
 				ctrlutils.GotAnnotationPredicate(clustersv1alpha1.OperationAnnotation, clustersv1alpha1.OperationAnnotationValueReconcile),

internal/controllers/accessrequest/controller_test.go

@@ -26,41 +26,48 @@ func arReconciler(c client.Client) reconcile.Reconciler {
 
 var _ = Describe("AccessRequest Controller", func() {
 
-	It("should add the correct provider label to the AccessRequest if a Cluster is referenced directly", func() {
+	It("should add the correct provider and profile labels to the AccessRequest if a Cluster is referenced directly", func() {
 		env := testutils.NewEnvironmentBuilder().WithFakeClient(scheme).WithInitObjectPath("testdata", "test-01").WithReconcilerConstructor(arReconciler).Build()
 		ar := &clustersv1alpha1.AccessRequest{}
 		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mc-access", "bar"), ar)).To(Succeed())
 		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
 		env.ShouldReconcile(testutils.RequestFromObject(ar))
 		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
 		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProviderLabel, "asdf"))
+		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProfileLabel, "default"))
 	})
 
-	It("should add the correct provider label to the AccessRequest if a Cluster is referenced via a ClusterRequest", func() {
+	It("should add the correct provider and profile labels to the AccessRequest if a Cluster is referenced via a ClusterRequest", func() {
 		env := testutils.NewEnvironmentBuilder().WithFakeClient(scheme).WithInitObjectPath("testdata", "test-01").WithReconcilerConstructor(arReconciler).Build()
 		ar := &clustersv1alpha1.AccessRequest{}
 		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mcr-access", "bar"), ar)).To(Succeed())
 		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
 		env.ShouldReconcile(testutils.RequestFromObject(ar))
 		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
 		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProviderLabel, "asdf"))
+		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProfileLabel, "default"))
 	})
 
 	It("should fail if the AccessRequest references a ClusterRequest which is not Granted", func() {
 		env := testutils.NewEnvironmentBuilder().WithFakeClient(scheme).WithInitObjectPath("testdata", "test-02").WithReconcilerConstructor(arReconciler).Build()
 		ar := &clustersv1alpha1.AccessRequest{}
 		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mcr-access", "bar"), ar)).To(Succeed())
 		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
 		env.ShouldNotReconcile(testutils.RequestFromObject(ar))
 		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
 		Expect(ar.Status.Message).To(ContainSubstring("not granted"))
 	})
 
 	It("should fail if the AccessRequest references an unknown Cluster or ClusterRequest", func() {
 		env := testutils.NewEnvironmentBuilder().WithFakeClient(scheme).WithInitObjectPath("testdata", "test-03").WithReconcilerConstructor(arReconciler).Build()
+
 		ar := &clustersv1alpha1.AccessRequest{}
 		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mc-access", "bar"), ar)).To(Succeed())
 		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
 		env.ShouldNotReconcile(testutils.RequestFromObject(ar))
 		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
 		Expect(ar.Status.Reason).To(Equal(cconst.ReasonInvalidReference))
@@ -69,10 +76,55 @@ var _ = Describe("AccessRequest Controller", func() {
 		ar = &clustersv1alpha1.AccessRequest{}
 		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mcr-access", "bar"), ar)).To(Succeed())
 		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
 		env.ShouldNotReconcile(testutils.RequestFromObject(ar))
 		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
 		Expect(ar.Status.Reason).To(Equal(cconst.ReasonInvalidReference))
 		Expect(ar.Status.Message).To(ContainSubstring("not found"))
 	})
 
+	It("should add the respective other label if either provider or profile label is already set", func() {
+		env := testutils.NewEnvironmentBuilder().WithFakeClient(scheme).WithInitObjectPath("testdata", "test-04").WithReconcilerConstructor(arReconciler).Build()
+
+		ar := &clustersv1alpha1.AccessRequest{}
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mc-access-provider", "bar"), ar)).To(Succeed())
+		Expect(ar.Labels).To(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
+		env.ShouldReconcile(testutils.RequestFromObject(ar))
+		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
+		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProviderLabel, "asdf"))
+		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProfileLabel, "default"))
+
+		ar = &clustersv1alpha1.AccessRequest{}
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mc-access-profile", "bar"), ar)).To(Succeed())
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).To(HaveKey(clustersv1alpha1.ProfileLabel))
+		env.ShouldReconcile(testutils.RequestFromObject(ar))
+		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
+		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProviderLabel, "asdf"))
+		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProfileLabel, "default"))
+	})
+
+	It("should not overwrite either label if already set to a different value", func() {
+		env := testutils.NewEnvironmentBuilder().WithFakeClient(scheme).WithInitObjectPath("testdata", "test-05").WithReconcilerConstructor(arReconciler).Build()
+
+		ar := &clustersv1alpha1.AccessRequest{}
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mc-access-provider", "bar"), ar)).To(Succeed())
+		Expect(ar.Labels).To(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
+		env.ShouldReconcile(testutils.RequestFromObject(ar))
+		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
+		Expect(ar.Labels).ToNot(HaveKeyWithValue(clustersv1alpha1.ProviderLabel, "asdf"))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
+
+		ar = &clustersv1alpha1.AccessRequest{}
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mc-access-profile", "bar"), ar)).To(Succeed())
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).To(HaveKey(clustersv1alpha1.ProfileLabel))
+		env.ShouldReconcile(testutils.RequestFromObject(ar))
+		Expect(env.Client().Get(env.Ctx, client.ObjectKeyFromObject(ar), ar)).To(Succeed())
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKeyWithValue(clustersv1alpha1.ProfileLabel, "default"))
+	})
+
 })

internal/controllers/accessrequest/testdata/test-04/accessrequest-profile.yaml

@@ -0,0 +1,19 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: AccessRequest
+metadata:
+  name: mc-access-profile
+  namespace: bar
+  labels:
+    profile.clusters.openmcp.cloud: default
+spec:
+  clusterRef:
+    name: my-cluster
+    namespace: foo
+  permissions:
+  - rules:
+    - apiGroups:
+      - "*"
+      resources:
+      - "*"
+      verbs:
+      - "*"

internal/controllers/accessrequest/testdata/test-04/accessrequest-provider.yaml

@@ -0,0 +1,19 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: AccessRequest
+metadata:
+  name: mc-access-provider
+  namespace: bar
+  labels:
+    provider.clusters.openmcp.cloud: asdf
+spec:
+  clusterRef:
+    name: my-cluster
+    namespace: foo
+  permissions:
+  - rules:
+    - apiGroups:
+      - "*"
+      resources:
+      - "*"
+      verbs:
+      - "*"

internal/controllers/accessrequest/testdata/test-04/cluster.yaml

@@ -0,0 +1,13 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: Cluster
+metadata:
+  name: my-cluster
+  namespace: foo
+spec:
+  profile: default
+  kubernetes:
+    version: "1.32"
+  purposes:
+  - test
+  tenancy: Exclusive
+  

internal/controllers/accessrequest/testdata/test-04/clusterprofile.yaml

@@ -0,0 +1,18 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: ClusterProfile
+metadata:
+  generation: 1
+  name: default
+spec:
+  environment: default
+  providerConfigRef:
+    name: foobar
+  providerRef:
+    name: asdf
+  supportedVersions:
+  - version: 1.32.2
+  - version: 1.31.7
+  - deprecated: true
+    version: 1.31.6
+  - deprecated: true
+    version: 1.31.5

internal/controllers/accessrequest/testdata/test-05/accessrequest-profile.yaml

@@ -0,0 +1,19 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: AccessRequest
+metadata:
+  name: mc-access-profile
+  namespace: bar
+  labels:
+    profile.clusters.openmcp.cloud: wrong
+spec:
+  clusterRef:
+    name: my-cluster
+    namespace: foo
+  permissions:
+  - rules:
+    - apiGroups:
+      - "*"
+      resources:
+      - "*"
+      verbs:
+      - "*"

internal/controllers/accessrequest/testdata/test-05/accessrequest-provider.yaml

@@ -0,0 +1,19 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: AccessRequest
+metadata:
+  name: mc-access-provider
+  namespace: bar
+  labels:
+    provider.clusters.openmcp.cloud: wrong
+spec:
+  clusterRef:
+    name: my-cluster
+    namespace: foo
+  permissions:
+  - rules:
+    - apiGroups:
+      - "*"
+      resources:
+      - "*"
+      verbs:
+      - "*"