improve the advanced clusteraccess library's abilities to mock fake clients in unit tests

docs/libraries/clusteraccess.md

@@ -206,14 +206,19 @@ For all of these keys, the package offers constants that are prefixed with `Faki
 
 While the signature of a callback function is always the same, any argument except for `ctx`, `platformClusterClient`, and `key` may be nil if not known at the point of execution.
 
+In addition to the callbacks, the ClusterAccess Reconciler also takes a `FakeClientGenerator` via its `WithFakeClientGenerator` method. If set to something other than `nil`, the reconciler's `Access` method will pass the raw kubeconfig bytes retrieved from the `AccessRequest`'s secret into this function, instead of creating a regular client from it.
+
+The combination of `WithFakingCallback` and `WithFakeClientGenerator` can enable unit tests for a controller which uses the advanced ClusterAccess library that do not require any test-specific logic in the controller's logic itself.
+
 ##### Convenience Implementations
 
 Because most controllers that use the faking callback feature will probably require a very similar logic for the aforementioned callback keys, the package provides a convenience implementation for each key:
 - `FakeClusterRequestReadiness` generates a callback function for the `WaitingForClusterRequestReadiness` key. It creates a `Cluster` next to the `ClusterRequest`, sets the reference to it in the request's `status` and sets the request to `Granted`.
   - This mocks cluster scheduler behavior.
 - `FakeAccessRequestReadiness` generates a callback function for the `WaitingForAccessRequestReadiness` key. It creates a `Secret` containing a `kubeconfig` key, references the secret in the request's status and sets the `AccessRequest` to `Granted`.
   - This mocks ClusterProvider behavior.
-  - Note that the `Access` getter method currently cannot handle the default kubeconfig written into the secret (which is just `fake`) and will always return an error, unless the method has been provided with a more realistic kubeconfig.
+  - If name and namespace of the `Cluster` the `AccessRequest` is for can be identified (which should usually be the case), the fake kubeconfig's content will be `fake:cluster:<cluster-namespace>/<cluster-name>`. Otherwise, it will be `fake:request:<request-namespace>/<request-name>`.
+    - This information can be used by a `FakeClientGenerator` to return a fitting fake client implementation.
 - `FakeClusterRequestDeletion` generates a callback function for the `WaitingForClusterRequestDeletion` key. Depending on its arguments, the generated function can remove specific or all finalizers on `Cluster` and/or `ClusterRequest`, and potentially also delete the `Cluster` resource.
   - This mocks cluster scheduler behavior.
 - `FakeAccessRequestDeletion` generates a callback function for the `WaitingForAccessRequestDeletion` key. It deletes the `Secret`, potentially removing the specified finalizers from it before, and then removes the configured finalizers from the `AccessRequest`.

lib/clusteraccess/advanced/clusteraccess.go

@@ -92,6 +92,11 @@ type ClusterAccessReconciler interface {
 	// The key determines when the callback function is executed.
 	// This feature is meant for unit testing, where usually no ClusterProvider, which could answer ClusterRequests and AccessRequests, is running.
 	WithFakingCallback(key string, callback FakingCallback) ClusterAccessReconciler
+	// WithFakeClientGenerator sets a fake client generator function.
+	// If non-nil, the function will be called during the Access method with the kubeconfig data retrieved from the AccessRequest secret and the scheme configured for the corresponding cluster registration.
+	// This allows injecting custom behavior for generating the client from the kubeconfig data, e.g. returning a standard fake client if the kubeconfig data is just 'fake' or something similar.
+	// Note that this is unset by default and must be set explicitly.
+	WithFakeClientGenerator(f FakeClientGenerator) ClusterAccessReconciler
 }
 
 // FakingCallback is a function that allows to mock a desired state for unit testing.
@@ -122,6 +127,10 @@ func DefaultManagedLabelGenerator(controllerName string, req reconcile.Request,
 	return controllerName, fmt.Sprintf("%s.%s.%s", req.Namespace, req.Name, reg.ID()), nil
 }
 
+// FakeClientGenerator is a function that generates a fake client.Client.
+// It is used as an argument for the ClusterAccessReconciler's WithFakeClientGenerator method.
+type FakeClientGenerator func(ctx context.Context, kcfgData []byte, scheme *runtime.Scheme, additionalData ...any) (client.Client, error)
+
 type ClusterRegistration interface {
 	// ID is the unique identifier for the cluster.
 	ID() string
@@ -436,7 +445,8 @@ type reconcilerImpl struct {
 	registrations map[string]ClusterRegistration
 	managedBy     ManagedLabelGenerator
 
-	fakingCallbacks map[string]FakingCallback
+	fakingCallbacks    map[string]FakingCallback
+	generateFakeClient FakeClientGenerator
 }
 
 // NewClusterAccessReconciler creates a new Cluster Access Reconciler.
@@ -467,7 +477,7 @@ func (r *reconcilerImpl) Access(ctx context.Context, request reconcile.Request,
 	if !ar.Status.IsGranted() {
 		return nil, fmt.Errorf("AccessRequest '%s/%s' for request '%s' with id '%s' is not granted", ar.Namespace, ar.Name, request.String(), id)
 	}
-	access, err := AccessFromAccessRequest(ctx, r.platformClusterClient, id, reg.Scheme(), ar)
+	access, err := accessFromAccessRequest(ctx, r.platformClusterClient, id, reg.Scheme(), ar, r.generateFakeClient, additionalData...)
 	if err != nil {
 		return nil, fmt.Errorf("unable to get access for request '%s' with id '%s': %w", request.String(), id, err)
 	}
@@ -933,6 +943,12 @@ func (r *reconcilerImpl) WithFakingCallback(key string, callback FakingCallback)
 	return r
 }
 
+// WithFakeClientGenerator implements Reconciler.
+func (r *reconcilerImpl) WithFakeClientGenerator(gen FakeClientGenerator) ClusterAccessReconciler {
+	r.generateFakeClient = gen
+	return r
+}
+
 const (
 	// FakingCallback_WaitingForAccessRequestReadiness is a key for a faking callback that is called when the reconciler is waiting for the AccessRequest to be granted.
 	// Note that the execution happens directly before the return of the reconcile function (with a requeue). This means that the reconciliation needs to run a second time to pick up the changes made in the callback.
@@ -975,6 +991,10 @@ func StableRequestNameFromLocalName(controllerName, localName, suffix string) st
 
 // AccessFromAccessRequest provides access to a k8s cluster based on the given AccessRequest.
 func AccessFromAccessRequest(ctx context.Context, platformClusterClient client.Client, id string, scheme *runtime.Scheme, ar *clustersv1alpha1.AccessRequest) (*clusters.Cluster, error) {
+	return accessFromAccessRequest(ctx, platformClusterClient, id, scheme, ar, nil)
+}
+
+func accessFromAccessRequest(ctx context.Context, platformClusterClient client.Client, id string, scheme *runtime.Scheme, ar *clustersv1alpha1.AccessRequest, generateFakeClient FakeClientGenerator, additionalData ...any) (*clusters.Cluster, error) {
 	if ar.Status.SecretRef == nil {
 		return nil, fmt.Errorf("AccessRequest '%s/%s' has no secret reference in status", ar.Namespace, ar.Name)
 	}
@@ -992,15 +1012,24 @@ func AccessFromAccessRequest(ctx context.Context, platformClusterClient client.C
 		return nil, fmt.Errorf("kubeconfig key '%s' not found in AccessRequest secret '%s/%s'", clustersv1alpha1.SecretKeyKubeconfig, s.Namespace, s.Name)
 	}
 
-	config, err := clientcmd.RESTConfigFromKubeConfig(kubeconfigBytes)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create rest config from kubeconfig bytes: %w", err)
-	}
+	var c *clusters.Cluster
+	if generateFakeClient != nil {
+		fc, err := generateFakeClient(ctx, kubeconfigBytes, scheme, additionalData...)
+		if err != nil {
+			return nil, fmt.Errorf("error creating fake client: %w", err)
+		}
+		c = clusters.NewTestClusterFromClient(id, fc)
+	} else {
+		config, err := clientcmd.RESTConfigFromKubeConfig(kubeconfigBytes)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create rest config from kubeconfig bytes: %w", err)
+		}
 
-	c := clusters.New(id).WithRESTConfig(config)
+		c = clusters.New(id).WithRESTConfig(config)
 
-	if err = c.InitializeClient(scheme); err != nil {
-		return nil, fmt.Errorf("failed to initialize client: %w", err)
+		if err = c.InitializeClient(scheme); err != nil {
+			return nil, fmt.Errorf("failed to initialize client: %w", err)
+		}
 	}
 
 	return c, nil
@@ -1115,13 +1144,14 @@ func FakeClusterRequestReadiness(clusterSpec *clustersv1alpha1.ClusterSpec) Faki
 }
 
 // FakeAccessRequestReadiness returns a faking callback that sets the AccessRequest to 'Granted'.
-// If kcfgData is not nil or empty, it will be used as 'kubeconfig' data in the secret referenced by the AccessRequest.
-// Otherwise, the content of the kubeconfig key will just be 'fake'.
+// The content of the secret's 'kubeconfig' key will one of the following:
+// - 'fake:cluster:<cluster-namespace>/<cluster-name>' if the Cluster could be determined (should be the case most of the time)
+// - 'fake:request:<request-namespace>/<request-name>' if no Cluster could be determined
 // The callback is a no-op if the AccessRequest is already granted (Secret reference and existence are not checked in this case).
 // It returns an error if the AccessRequest is nil.
 //
 // The returned callback is meant to be used with the key stored in FakingCallback_WaitingForAccessRequestReadiness.
-func FakeAccessRequestReadiness(kcfgData []byte) FakingCallback {
+func FakeAccessRequestReadiness() FakingCallback {
 	return func(ctx context.Context, platformClusterClient client.Client, key string, req *reconcile.Request, cr *clustersv1alpha1.ClusterRequest, ar *clustersv1alpha1.AccessRequest, c *clustersv1alpha1.Cluster, access *clusters.Cluster) error {
 		if ar == nil {
 			return fmt.Errorf("AccessRequest is nil")
@@ -1164,14 +1194,17 @@ func FakeAccessRequestReadiness(kcfgData []byte) FakingCallback {
 		}
 
 		// create secret
-		if len(kcfgData) == 0 {
-			kcfgData = []byte("fake")
-		}
 		s := &corev1.Secret{}
 		s.Name = ar.Name
 		s.Namespace = ar.Namespace
+		var kcfgBytes []byte
+		if ar.Spec.ClusterRef != nil {
+			kcfgBytes = fmt.Appendf(nil, "fake:cluster:%s/%s", ar.Spec.ClusterRef.Namespace, ar.Spec.ClusterRef.Name)
+		} else {
+			kcfgBytes = fmt.Appendf(nil, "fake:request:%s/%s", req.Namespace, req.Name)
+		}
 		s.Data = map[string][]byte{
-			clustersv1alpha1.SecretKeyKubeconfig: kcfgData,
+			clustersv1alpha1.SecretKeyKubeconfig: kcfgBytes,
 		}
 		if err := platformClusterClient.Get(ctx, client.ObjectKeyFromObject(s), s); err != nil {
 			if !apierrors.IsNotFound(err) {

lib/clusteraccess/advanced/clusteraccess_test.go

@@ -17,6 +17,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	testutils "github.com/openmcp-project/controller-utils/pkg/testing"
@@ -56,7 +57,10 @@ func defaultClusterAccessReconciler(env *testutils.Environment, controllerName s
 			Purposes: []string{"test"},
 			Tenancy:  clustersv1alpha1.TENANCY_EXCLUSIVE,
 		})).
-		WithFakingCallback(advanced.FakingCallback_WaitingForAccessRequestReadiness, advanced.FakeAccessRequestReadiness(nil))
+		WithFakingCallback(advanced.FakingCallback_WaitingForAccessRequestReadiness, advanced.FakeAccessRequestReadiness()).
+		WithFakeClientGenerator(func(ctx context.Context, kcfgData []byte, scheme *runtime.Scheme, additionalData ...any) (client.Client, error) {
+			return fake.NewClientBuilder().WithScheme(scheme).Build(), nil
+		})
 }
 
 var _ = Describe("Advanced Cluster Access", func() {
@@ -115,7 +119,8 @@ var _ = Describe("Advanced Cluster Access", func() {
 		Expect(c).To(Equal(cCopy)) // the method should have returned the up-to-date object
 
 		// check cluster access
-		// cannot be tested at the moment, because the corresponding library expects 'real' kubeconfigs
+		_, err = rec.Access(env.Ctx, req, "foobar")
+		Expect(err).To(HaveOccurred()) // no access was requested, so this should fail
 
 		// EXAMPLE 2
 		// check namespace
@@ -159,7 +164,10 @@ var _ = Describe("Advanced Cluster Access", func() {
 		Expect(c2).To(Equal(c2Copy)) // the method should have returned the up-to-date object
 
 		// check cluster access
-		// cannot be tested at the moment, because the corresponding library expects 'real' kubeconfigs
+		access, err := rec.Access(env.Ctx, req, "foobaz")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(access.ID()).To(Equal("foobaz"))
+		Expect(access.Client()).ToNot(BeNil())
 
 		// delete everything again, except for namespaces
 		Eventually(expectNoRequeue(env.Ctx, rec, req, true)).Should(Succeed())
@@ -242,7 +250,10 @@ var _ = Describe("Advanced Cluster Access", func() {
 		Expect(c).To(Equal(cCopy)) // the method should have returned the up-to-date object
 
 		// check cluster access
-		// cannot be tested at the moment, because the corresponding library expects 'real' kubeconfigs
+		access, err := rec.Access(env.Ctx, req, "foobar")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(access.ID()).To(Equal("foobar"))
+		Expect(access.Client()).ToNot(BeNil())
 
 		// delete everything again
 		// ClusterRequest and Cluster were not created by this library, so they should not be deleted
@@ -333,7 +344,10 @@ var _ = Describe("Advanced Cluster Access", func() {
 		Expect(c).To(Equal(cCopy)) // the method should have returned the up-to-date object
 
 		// check cluster access
-		// cannot be tested at the moment, because the corresponding library expects 'real' kubeconfigs
+		access, err := rec.Access(env.Ctx, req, "foobar")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(access.ID()).To(Equal("foobar"))
+		Expect(access.Client()).ToNot(BeNil())
 
 		// delete everything again
 		// Cluster was not created by this library, so it should not be deleted