From 0105b8becb1453d43e33f3ae3aab35c82cd2e824 Mon Sep 17 00:00:00 2001 From: Johannes Aubart Date: Wed, 15 Oct 2025 16:00:52 +0200 Subject: [PATCH] improve the advanced clusteraccess library's abilities to mock fake clients in unit tests --- docs/libraries/clusteraccess.md | 7 +- lib/clusteraccess/advanced/clusteraccess.go | 65 ++++++++++++++----- .../advanced/clusteraccess_test.go | 24 +++++-- 3 files changed, 74 insertions(+), 22 deletions(-) diff --git a/docs/libraries/clusteraccess.md b/docs/libraries/clusteraccess.md index 4290403..77ec280 100644 --- a/docs/libraries/clusteraccess.md +++ b/docs/libraries/clusteraccess.md @@ -206,6 +206,10 @@ 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: @@ -213,7 +217,8 @@ Because most controllers that use the faking callback feature will probably requ - 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:/`. Otherwise, it will be `fake:request:/`. + - 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`. diff --git a/lib/clusteraccess/advanced/clusteraccess.go b/lib/clusteraccess/advanced/clusteraccess.go index b96dcca..c006551 100644 --- a/lib/clusteraccess/advanced/clusteraccess.go +++ b/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:/' if the Cluster could be determined (should be the case most of the time) +// - 'fake:request:/' 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) { diff --git a/lib/clusteraccess/advanced/clusteraccess_test.go b/lib/clusteraccess/advanced/clusteraccess_test.go index 434c7c3..0421edd 100644 --- a/lib/clusteraccess/advanced/clusteraccess_test.go +++ b/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