From 9ded6179dc444f983095ac33d93e648218129d0b Mon Sep 17 00:00:00 2001
From: killianmuldoon <kmuldoon@vmware.com>
Date: Wed, 30 Aug 2023 10:16:06 +0100
Subject: [PATCH] Fix nil pointer error in retrieveVcenterSession

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
---
 controllers/vspherevm_controller.go      |  4 +
 controllers/vspherevm_controller_test.go | 97 ++++++++++++++++--------
 2 files changed, 68 insertions(+), 33 deletions(-)

diff --git a/controllers/vspherevm_controller.go b/controllers/vspherevm_controller.go
index ce1d0b4bff..3fb789c244 100644
--- a/controllers/vspherevm_controller.go
+++ b/controllers/vspherevm_controller.go
@@ -28,6 +28,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	apitypes "k8s.io/apimachinery/pkg/types"
+	"k8s.io/klog/v2"
 	"k8s.io/utils/pointer"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/remote"
@@ -569,6 +570,9 @@ func (r vmReconciler) retrieveVcenterSession(ctx goctx.Context, vsphereVM *infra
 			params)
 	}
 
+	if cluster.Spec.InfrastructureRef == nil {
+		return nil, errors.Errorf("cannot retrieve vCenter session for cluster %s: .spec.infrastructureRef is nil", klog.KObj(cluster))
+	}
 	key := ctrlclient.ObjectKey{
 		Namespace: cluster.Namespace,
 		Name:      cluster.Spec.InfrastructureRef.Name,
diff --git a/controllers/vspherevm_controller_test.go b/controllers/vspherevm_controller_test.go
index 44d6ffe777..3dbcfae453 100644
--- a/controllers/vspherevm_controller_test.go
+++ b/controllers/vspherevm_controller_test.go
@@ -420,18 +420,6 @@ func TestRetrievingVCenterCredentialsFromCluster(t *testing.T) {
 		},
 	}
 
-	cluster := &clusterv1.Cluster{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "valid-cluster",
-			Namespace: "test",
-		},
-		Spec: clusterv1.ClusterSpec{
-			InfrastructureRef: &corev1.ObjectReference{
-				Name: vsphereCluster.Name,
-			},
-		},
-	}
-
 	machine := &clusterv1.Machine{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "foo",
@@ -442,8 +430,6 @@ func TestRetrievingVCenterCredentialsFromCluster(t *testing.T) {
 		},
 	}
 
-	initObjs := createMachineOwnerHierarchy(machine)
-
 	vsphereMachine := &infrav1.VSphereMachine{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "foo-vm",
@@ -480,26 +466,71 @@ func TestRetrievingVCenterCredentialsFromCluster(t *testing.T) {
 		Status: infrav1.VSphereVMStatus{},
 	}
 
-	initObjs = append(initObjs, secret, vsphereVM, vsphereMachine, machine, cluster, vsphereCluster)
-	controllerMgrContext := fake.NewControllerManagerContext(initObjs...)
+	t.Run("Retrieve credentials from cluster", func(t *testing.T) {
+		cluster := &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "valid-cluster",
+				Namespace: "test",
+			},
+			Spec: clusterv1.ClusterSpec{
+				InfrastructureRef: &corev1.ObjectReference{
+					Name: vsphereCluster.Name,
+				},
+			},
+		}
 
-	controllerContext := &context.ControllerContext{
-		ControllerManagerContext: controllerMgrContext,
-		Recorder:                 record.New(apirecord.NewFakeRecorder(100)),
-		Logger:                   log.Log,
-	}
-	r := vmReconciler{ControllerContext: controllerContext}
-
-	_, err = r.Reconcile(goctx.Background(), ctrl.Request{NamespacedName: util.ObjectKey(vsphereVM)})
-	g := NewWithT(t)
-	g.Expect(err).NotTo(HaveOccurred())
-
-	vm := &infrav1.VSphereVM{}
-	vmKey := util.ObjectKey(vsphereVM)
-	g.Expect(r.Client.Get(goctx.Background(), vmKey, vm)).NotTo(HaveOccurred())
-	g.Expect(conditions.Has(vm, infrav1.VCenterAvailableCondition)).To(BeTrue())
-	vCenterCondition := conditions.Get(vm, infrav1.VCenterAvailableCondition)
-	g.Expect(vCenterCondition.Status).To(Equal(corev1.ConditionTrue))
+		initObjs := createMachineOwnerHierarchy(machine)
+		initObjs = append(initObjs, secret, vsphereVM, vsphereMachine, machine, cluster, vsphereCluster)
+		controllerMgrContext := fake.NewControllerManagerContext(initObjs...)
+
+		controllerContext := &context.ControllerContext{
+			ControllerManagerContext: controllerMgrContext,
+			Recorder:                 record.New(apirecord.NewFakeRecorder(100)),
+			Logger:                   log.Log,
+		}
+		r := vmReconciler{ControllerContext: controllerContext}
+
+		_, err = r.Reconcile(goctx.Background(), ctrl.Request{NamespacedName: util.ObjectKey(vsphereVM)})
+		g := NewWithT(t)
+		g.Expect(err).NotTo(HaveOccurred())
+
+		vm := &infrav1.VSphereVM{}
+		vmKey := util.ObjectKey(vsphereVM)
+		g.Expect(r.Client.Get(goctx.Background(), vmKey, vm)).NotTo(HaveOccurred())
+		g.Expect(conditions.Has(vm, infrav1.VCenterAvailableCondition)).To(BeTrue())
+		vCenterCondition := conditions.Get(vm, infrav1.VCenterAvailableCondition)
+		g.Expect(vCenterCondition.Status).To(Equal(corev1.ConditionTrue))
+	},
+	)
+
+	t.Run("Error if cluster infrastructureRef is nil", func(t *testing.T) {
+		cluster := &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "valid-cluster",
+				Namespace: "test",
+			},
+
+			// InfrastructureRef is nil so we should get an error.
+			Spec: clusterv1.ClusterSpec{
+				InfrastructureRef: nil,
+			},
+		}
+		initObjs := createMachineOwnerHierarchy(machine)
+		initObjs = append(initObjs, secret, vsphereVM, vsphereMachine, machine, cluster, vsphereCluster)
+		controllerMgrContext := fake.NewControllerManagerContext(initObjs...)
+
+		controllerContext := &context.ControllerContext{
+			ControllerManagerContext: controllerMgrContext,
+			Recorder:                 record.New(apirecord.NewFakeRecorder(100)),
+			Logger:                   log.Log,
+		}
+		r := vmReconciler{ControllerContext: controllerContext}
+
+		_, err = r.Reconcile(goctx.Background(), ctrl.Request{NamespacedName: util.ObjectKey(vsphereVM)})
+		g := NewWithT(t)
+		g.Expect(err).To(HaveOccurred())
+	},
+	)
 }
 
 func Test_reconcile(t *testing.T) {