accessrequest controller now also adds cluster reference

Author: Diaphteiros

api/clusters/v1alpha1/accessrequest_types.go

@@ -7,14 +7,14 @@ import (
 
 type AccessRequestSpec struct {
 	// ClusterRef is the reference to the Cluster for which access is requested.
-	// Exactly one of clusterRef or requestRef must be set.
+	// If set, requestRef will be ignored.
 	// This value is immutable.
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="clusterRef is immutable"
 	// +optional
 	ClusterRef *NamespacedObjectReference `json:"clusterRef,omitempty"`
 
 	// RequestRef is the reference to the ClusterRequest for whose Cluster access is requested.
-	// Exactly one of clusterRef or requestRef must be set.
+	// Is ignored if clusterRef is set.
 	// This value is immutable.
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="requestRef is immutable"
 	// +optional

api/crds/manifests/clusters.openmcp.cloud_accessrequests.yaml

@@ -50,7 +50,7 @@ spec:
               clusterRef:
                 description: |-
                   ClusterRef is the reference to the Cluster for which access is requested.
-                  Exactly one of clusterRef or requestRef must be set.
+                  If set, requestRef will be ignored.
                   This value is immutable.
                 properties:
                   name:
@@ -135,7 +135,7 @@ spec:
               requestRef:
                 description: |-
                   RequestRef is the reference to the ClusterRequest for whose Cluster access is requested.
-                  Exactly one of clusterRef or requestRef must be set.
+                  Is ignored if clusterRef is set.
                   This value is immutable.
                 properties:
                   name:

docs/controller/accessrequest.md

@@ -1,6 +1,6 @@
 # AccessRequest Controller
 
-The _AccessRequest Controller_ is responsible for labelling `AccessRequest` resources with the name of the ClusterProvider that is responsible for them.
+The _AccessRequest Controller_ is responsible for labelling `AccessRequest` resources with the name of the ClusterProvider that is responsible for them. It also adds a label for the corresponding `ClusterProfile` and adds the cluster reference to the spec, if only the request reference is specified.
 
 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.
 
@@ -16,6 +16,10 @@ ClusterProviders should only reconcile `AccessRequest` resources where both labe
 
 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.
 
+In addition to the labels, the controller also sets `spec.clusterRef`, if only `spec.requestRef` is specified.
+
+After an `AccessRequest` has been prepared this way, the ClusterProviders can easily infer which one is responsible, which `Cluster` resource this request belongs to, and which `ClusterProfile` is used by the `Cluster`, directly from the labels and spec of the `AccessRequest` resource.
+
 ## Configuration
 
 The AccessRequest controller is run as long as `accessrequest` is included in the `--controllers` flag. It is included by default.

internal/controllers/accessrequest/controller.go

@@ -164,8 +164,16 @@ func (r *AccessRequestReconciler) reconcile(ctx context.Context, req reconcile.R
 			return rr
 		}
 	}
+
+	// set cluster reference, if only the request reference is set
+	if ar.Spec.ClusterRef == nil {
+		ar.Spec.ClusterRef = &clustersv1alpha1.NamespacedObjectReference{}
+		ar.Spec.ClusterRef.Name = c.Name
+		ar.Spec.ClusterRef.Namespace = c.Namespace
+	}
+
 	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)
+		rr.ReconcileError = errutils.WithReason(fmt.Errorf("error patching resource '%s': %w", req.String(), err), cconst.ReasonPlatformClusterInteractionProblem)
 		return rr
 	}
 

internal/controllers/accessrequest/controller_test.go

@@ -26,7 +26,7 @@ func arReconciler(c client.Client) reconcile.Reconciler {
 
 var _ = Describe("AccessRequest Controller", func() {
 
-	It("should add the correct provider and profile labels to the AccessRequest if a Cluster is referenced directly", func() {
+	It("should add the correct 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())
@@ -38,16 +38,20 @@ var _ = Describe("AccessRequest Controller", func() {
 		Expect(ar.Labels).To(HaveKeyWithValue(clustersv1alpha1.ProfileLabel, "default"))
 	})
 
-	It("should add the correct provider and profile labels to the AccessRequest if a Cluster is referenced via a ClusterRequest", func() {
+	It("should add the correct labels and cluster reference 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))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 		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"))
+		Expect(ar.Spec.ClusterRef).ToNot(BeNil())
+		Expect(ar.Spec.ClusterRef.Name).To(Equal("my-cluster"))
+		Expect(ar.Spec.ClusterRef.Namespace).To(Equal("foo"))
 	})
 
 	It("should fail if the AccessRequest references a ClusterRequest which is not Granted", func() {
@@ -56,9 +60,13 @@ var _ = Describe("AccessRequest Controller", func() {
 		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))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 		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"))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 	})
 
 	It("should fail if the AccessRequest references an unknown Cluster or ClusterRequest", func() {
@@ -72,15 +80,21 @@ var _ = Describe("AccessRequest Controller", func() {
 		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"))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
 
 		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))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 		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"))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
+		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 	})
 
 	It("should add the respective other label if either provider or profile label is already set", func() {
@@ -109,22 +123,26 @@ var _ = Describe("AccessRequest Controller", 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(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mcr-access-provider", "bar"), ar)).To(Succeed())
 		Expect(ar.Labels).To(HaveKey(clustersv1alpha1.ProviderLabel))
 		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProfileLabel))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 		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))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 
 		ar = &clustersv1alpha1.AccessRequest{}
-		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mc-access-profile", "bar"), ar)).To(Succeed())
+		Expect(env.Client().Get(env.Ctx, ctrlutils.ObjectKey("mcr-access-profile", "bar"), ar)).To(Succeed())
 		Expect(ar.Labels).ToNot(HaveKey(clustersv1alpha1.ProviderLabel))
 		Expect(ar.Labels).To(HaveKey(clustersv1alpha1.ProfileLabel))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 		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"))
+		Expect(ar.Spec.ClusterRef).To(BeNil())
 	})
 
 })

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

@@ -1,12 +1,12 @@
 apiVersion: clusters.openmcp.cloud/v1alpha1
 kind: AccessRequest
 metadata:
-  name: mc-access-profile
+  name: mcr-access-profile
   namespace: bar
   labels:
     profile.clusters.openmcp.cloud: wrong
 spec:
-  clusterRef:
+  requestRef:
     name: my-cluster
     namespace: foo
   permissions:

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

@@ -1,12 +1,12 @@
 apiVersion: clusters.openmcp.cloud/v1alpha1
 kind: AccessRequest
 metadata:
-  name: mc-access-provider
+  name: mcr-access-provider
   namespace: bar
   labels:
     provider.clusters.openmcp.cloud: wrong
 spec:
-  clusterRef:
+  requestRef:
     name: my-cluster
     namespace: foo
   permissions:

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

@@ -0,0 +1,12 @@
+apiVersion: clusters.openmcp.cloud/v1alpha1
+kind: ClusterRequest
+metadata:
+  name: my-cluster
+  namespace: foo
+spec:
+  purpose: test
+status:
+  phase: Granted
+  cluster:
+    name: my-cluster
+    namespace: foo