Merge pull request #2505 from sbueringer/pr-fix-machine-nil

🐛 Fix potential panics when retrieving VSphereCluster from Cluster.spec.infrastructureRef
kubernetes-sigs · Dec 5, 2023 · da206f1 · da206f1
2 parents 2b0393e + 1b211d9
commit da206f1
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 31 deletions.
diff --git a/controllers/vspherecluster_reconciler.go b/controllers/vspherecluster_reconciler.go
@@ -526,7 +526,7 @@ func (r *clusterReconciler) controlPlaneMachineToCluster(ctx context.Context, o
 		log.Error(err, "VSphereMachine is missing cluster label or cluster does not exist")
 		return nil
 	}
-	log = log.WithValues("Cluster", klog.KObj(cluster), "VSphereCluster", klog.KRef(cluster.Namespace, cluster.Spec.InfrastructureRef.Name))
+	log = log.WithValues("Cluster", klog.KObj(cluster))
 	ctx = ctrl.LoggerInto(ctx, log)
 
 	if conditions.IsTrue(cluster, clusterv1.ControlPlaneInitializedCondition) {
@@ -537,6 +537,14 @@ func (r *clusterReconciler) controlPlaneMachineToCluster(ctx context.Context, o
 		return nil
 	}
 
+	if cluster.Spec.InfrastructureRef == nil {
+		log.Info("Failed to get VSphereCluster: Cluster.spec.infrastructureRef is not yet set")
+		return nil
+	}
+
+	log = log.WithValues("VSphereCluster", klog.KRef(cluster.Namespace, cluster.Spec.InfrastructureRef.Name))
+	ctx = ctrl.LoggerInto(ctx, log)
+
 	// Fetch the VSphereCluster
 	vsphereCluster := &infrav1.VSphereCluster{}
 	vsphereClusterKey := client.ObjectKey{

diff --git a/controllers/vspheremachine_controller.go b/controllers/vspheremachine_controller.go
@@ -102,6 +102,13 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 		controllerNameLong = fmt.Sprintf("%s/%s/%s", controllerManagerContext.Namespace, controllerManagerContext.Name, controllerNameShort)
 	}
 
+	r := &machineReconciler{
+		Client:          controllerManagerContext.Client,
+		Recorder:        record.New(mgr.GetEventRecorderFor(controllerNameLong)),
+		VMService:       &services.VimMachineService{Client: controllerManagerContext.Client},
+		supervisorBased: supervisorBased,
+	}
+
 	builder := ctrl.NewControllerManagedBy(mgr).
 		Named(VSphereMachineControllerName).
 		// Watch the controlled, infrastructure resource.
@@ -112,6 +119,13 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 			&clusterv1.Machine{},
 			handler.EnqueueRequestsFromMapFunc(clusterutilv1.MachineToInfrastructureMapFunc(controlledTypeGVK)),
 		).
+		Watches(
+			&clusterv1.Cluster{},
+			handler.EnqueueRequestsFromMapFunc(r.clusterToVSphereMachines),
+			ctrlbldr.WithPredicates(
+				predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx)),
+			),
+		).
 		// Watch a GenericEvent channel for the controlled resource.
 		//
 		// This is useful when there are events outside of Kubernetes that
@@ -123,13 +137,6 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 		).
 		WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), controllerManagerContext.WatchFilterValue))
 
-	r := &machineReconciler{
-		Client:          controllerManagerContext.Client,
-		Recorder:        record.New(mgr.GetEventRecorderFor(controllerNameLong)),
-		VMService:       &services.VimMachineService{Client: controllerManagerContext.Client},
-		supervisorBased: supervisorBased,
-	}
-
 	if supervisorBased {
 		// Watch any VirtualMachine resources owned by this VSphereMachine
 		builder.Owns(&vmoprv1.VirtualMachine{})
@@ -154,16 +161,6 @@ func AddMachineControllerToManager(ctx context.Context, controllerManagerContext
 		)
 	}
 
-	if !supervisorBased {
-		builder.Watches(
-			&clusterv1.Cluster{},
-			handler.EnqueueRequestsFromMapFunc(r.clusterToVSphereMachines),
-			ctrlbldr.WithPredicates(
-				predicates.ClusterUnpausedAndInfrastructureReady(ctrl.LoggerFrom(ctx)),
-			),
-		)
-	}
-
 	return builder.Complete(r)
 }
 
@@ -239,25 +236,29 @@ func (r *machineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 
 	// Checking whether cluster is nil here as we still want to allow delete even if cluster is not found.
 	if cluster == nil {
+		log.Info("Cluster could not be fetched")
+		return reconcile.Result{}, nil
+	}
+
+	if cluster.Spec.InfrastructureRef == nil {
+		log.Info("Cluster.spec.infrastructureRef is not yet set")
 		return reconcile.Result{}, nil
 	}
 
 	// If the VSphereCluster has been previously set as an ownerReference remove it. This may have been set in an older
 	// version of CAPV to prevent VSphereMachines from being orphaned, but is no longer needed.
 	// For more info see: https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/issues/2054
 	// TODO: This should be removed in a future release of CAPV.
-	if cluster.Spec.InfrastructureRef != nil {
-		machineContext.GetVSphereMachine().SetOwnerReferences(
-			clusterutilv1.RemoveOwnerRef(
-				machineContext.GetVSphereMachine().GetOwnerReferences(),
-				metav1.OwnerReference{
-					Name:       cluster.Spec.InfrastructureRef.Name,
-					APIVersion: cluster.Spec.InfrastructureRef.APIVersion,
-					Kind:       cluster.Spec.InfrastructureRef.Kind,
-				},
-			),
-		)
-	}
+	machineContext.GetVSphereMachine().SetOwnerReferences(
+		clusterutilv1.RemoveOwnerRef(
+			machineContext.GetVSphereMachine().GetOwnerReferences(),
+			metav1.OwnerReference{
+				Name:       cluster.Spec.InfrastructureRef.Name,
+				APIVersion: cluster.Spec.InfrastructureRef.APIVersion,
+				Kind:       cluster.Spec.InfrastructureRef.Kind,
+			},
+		),
+	)
 
 	// Fetch the VSphereCluster and update the machine context
 	machineContext, err = r.VMService.FetchVSphereCluster(ctx, cluster, machineContext)

diff --git a/controllers/vspherevm_controller.go b/controllers/vspherevm_controller.go
@@ -576,7 +576,7 @@ func (r vmReconciler) retrieveVcenterSession(ctx context.Context, vsphereVM *inf
 	}
 
 	if cluster.Spec.InfrastructureRef == nil {
-		return nil, errors.Errorf("cannot retrieve vCenter session for cluster %s: .spec.infrastructureRef is nil", klog.KObj(cluster))
+		return nil, errors.Errorf("cannot retrieve vCenter session for cluster %s: Cluster.spec.infrastructureRef is nil", klog.KObj(cluster))
 	}
 	key := ctrlclient.ObjectKey{
 		Namespace: cluster.Namespace,

diff --git a/pkg/util/cluster.go b/pkg/util/cluster.go
@@ -45,6 +45,11 @@ func GetVSphereClusterFromVMwareMachine(ctx context.Context, c client.Client, ma
 		return nil, err
 	}
 
+	if cluster.Spec.InfrastructureRef == nil {
+		return nil, errors.Errorf("error getting VSphereCluster name from VSphereMachine %s/%s: Cluster.spec.infrastructureRef not yet set",
+			machine.Namespace, machine.Name)
+	}
+
 	vsphereClusterKey := apitypes.NamespacedName{
 		Namespace: machine.Namespace,
 		Name:      cluster.Spec.InfrastructureRef.Name,
@@ -70,6 +75,10 @@ func GetVSphereClusterFromVSphereMachine(ctx context.Context, c client.Client, m
 		return nil, err
 	}
 
+	if cluster.Spec.InfrastructureRef == nil {
+		return nil, errors.Errorf("error getting VSphereCluster name from VSphereMachine %s/%s: Cluster.spec.infrastructureRef not yet set",
+			machine.Namespace, machine.Name)
+	}
 	vsphereClusterKey := apitypes.NamespacedName{
 		Namespace: machine.Namespace,
 		Name:      cluster.Spec.InfrastructureRef.Name,