Merge pull request #12655 from Karthik-K-N/add-nil-check

🌱 Add input validations for desired state generator function

Author: k8s-ci-robot

controllers/clustercache/cluster_cache_fake.go

@@ -44,3 +44,8 @@ func NewFakeClusterCache(workloadClient client.Client, clusterKey client.ObjectK
 	}
 	return testCacheTracker
 }
+
+// NewFakeEmptyClusterCache creates a new empty ClusterCache that can be used by unit tests.
+func NewFakeEmptyClusterCache() ClusterCache {
+	return &clusterCache{}
+}

exp/topology/desiredstate/desired_state.go

@@ -63,13 +63,21 @@ type Generator interface {
 }
 
 // NewGenerator creates a new generator to generate desired state.
-func NewGenerator(client client.Client, clusterCache clustercache.ClusterCache, runtimeClient runtimeclient.Client) Generator {
+func NewGenerator(client client.Client, clusterCache clustercache.ClusterCache, runtimeClient runtimeclient.Client) (Generator, error) {
+	if client == nil || clusterCache == nil {
+		return nil, errors.New("Client and ClusterCache must not be nil")
+	}
+
+	if feature.Gates.Enabled(feature.RuntimeSDK) && runtimeClient == nil {
+		return nil, errors.New("RuntimeClient must not be nil")
+	}
+
 	return &generator{
 		Client:        client,
 		ClusterCache:  clusterCache,
 		RuntimeClient: runtimeClient,
 		patchEngine:   patches.NewEngine(runtimeClient),
-	}
+	}, nil
 }
 
 // generator is a generator to generate desired state.

internal/controllers/topology/cluster/cluster_controller.go

@@ -156,7 +156,11 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
 		Scheme:          mgr.GetScheme(),
 		PredicateLogger: &predicateLog,
 	}
-	r.desiredStateGenerator = desiredstate.NewGenerator(r.Client, r.ClusterCache, r.RuntimeClient)
+	r.desiredStateGenerator, err = desiredstate.NewGenerator(r.Client, r.ClusterCache, r.RuntimeClient)
+	if err != nil {
+		return errors.Wrap(err, "failed creating desired state generator")
+	}
+
 	r.recorder = mgr.GetEventRecorderFor("topology/cluster-controller")
 	r.ssaCache = ssa.NewCache("topology/cluster")
 	return nil

internal/controllers/topology/cluster/reconcile_state_test.go

@@ -47,6 +47,7 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1"
 	runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2"
+	"sigs.k8s.io/cluster-api/controllers/clustercache"
 	"sigs.k8s.io/cluster-api/controllers/external"
 	runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
 	"sigs.k8s.io/cluster-api/exp/topology/desiredstate"
@@ -1167,14 +1168,21 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 
 			fakeClient := fake.NewClientBuilder().WithObjects(tt.s.Current.Cluster).Build()
 
+			desiredStateGenerator, err := desiredstate.NewGenerator(
+				fakeClient,
+				clustercache.NewFakeEmptyClusterCache(),
+				fakeRuntimeClient,
+			)
+			g.Expect(err).ToNot(HaveOccurred())
+
 			r := &Reconciler{
 				Client:                fakeClient,
 				APIReader:             fakeClient,
 				RuntimeClient:         fakeRuntimeClient,
-				desiredStateGenerator: desiredstate.NewGenerator(fakeClient, nil, fakeRuntimeClient),
+				desiredStateGenerator: desiredStateGenerator,
 			}
 
-			err := r.callAfterClusterUpgrade(ctx, tt.s)
+			err = r.callAfterClusterUpgrade(ctx, tt.s)
 			if tt.wantError {
 				g.Expect(err).To(HaveOccurred())
 			} else {

test/extension/handlers/topologymutation/handler_integration_test.go

@@ -50,6 +50,7 @@ import (
 	runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1"
 	runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2"
 	"sigs.k8s.io/cluster-api/controllers"
+	"sigs.k8s.io/cluster-api/controllers/clustercache"
 	runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
 	runtimeclient "sigs.k8s.io/cluster-api/exp/runtime/client"
 	"sigs.k8s.io/cluster-api/exp/topology/desiredstate"
@@ -114,7 +115,12 @@ func TestHandler(t *testing.T) {
 	clientWithV1Beta2ContractCRD := fake.NewClientBuilder().WithScheme(scheme).WithObjects(crd).Build()
 
 	// Create a desired state generator.
-	desiredStateGenerator := desiredstate.NewGenerator(clientWithV1Beta2ContractCRD, nil, runtimeClient)
+	desiredStateGenerator, err := desiredstate.NewGenerator(
+		clientWithV1Beta2ContractCRD,
+		clustercache.NewFakeEmptyClusterCache(),
+		runtimeClient,
+	)
+	g.Expect(err).ToNot(HaveOccurred())
 
 	// Note: as of today we don't have to set any fields and also don't have to call
 	// SetupWebhookWithManager because DefaultAndValidateVariables doesn't need any of that.