inlining some code for readability

controllers/machine_controller.go

@@ -376,38 +376,43 @@ func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, cluster *cl
 		return errNilNodeRef
 	}
 
-	// Get the control plane object associated with the cluster
-	controlPlane, err := getControlPlaneFromRef(ctx, r.Client, cluster.Spec.ControlPlaneRef)
-	if err != nil {
-		return err
-	}
-
-	// Check if the ControlPlane is externally managed (AKS, EKS, GKE, etc)
-	// and skip the following section if control plane is externally managed
-	// because there will be no control plane nodes registered
-	if !util.IsExternalManagedControlPlane(controlPlane) {
+	var controlPlane *unstructured.Unstructured
 
-		// Get all of the machines that belong to this cluster.
-		machines, err := getActiveMachinesInCluster(ctx, r.Client, machine.Namespace, machine.Labels[clusterv1.ClusterLabelName])
+	// controlPlaneRef is an optional field in the Cluster so skip the external
+	// managed control plane check if it is nil
+	if cluster.Spec.ControlPlaneRef != nil {
+		var err error
+		controlPlane, err = external.Get(ctx, r.Client, cluster.Spec.ControlPlaneRef, cluster.Spec.ControlPlaneRef.Namespace)
 		if err != nil {
 			return err
 		}
 
-		// Whether or not it is okay to delete the NodeRef depends on the
-		// number of remaining control plane members and whether or not this
-		// machine is one of them.
-		switch numControlPlaneMachines := len(util.GetControlPlaneMachines(machines)); {
-		case numControlPlaneMachines == 0:
-			// Do not delete the NodeRef if there are no remaining members of
-			// the control plane.
-			return errNoControlPlaneNodes
-		default:
-			// Otherwise it is okay to delete the NodeRef.
+		// Check if the ControlPlane is externally managed (AKS, EKS, GKE, etc)
+		// and skip the following section if control plane is externally managed
+		// because there will be no control plane nodes registered
+		if util.IsExternalManagedControlPlane(controlPlane) {
 			return nil
 		}
 	}
-	// In other control plane types, it is OK to delete the NodeRef
-	return nil
+
+	// Get all of the machines that belong to this cluster.
+	machines, err := getActiveMachinesInCluster(ctx, r.Client, machine.Namespace, machine.Labels[clusterv1.ClusterLabelName])
+	if err != nil {
+		return err
+	}
+
+	// Whether or not it is okay to delete the NodeRef depends on the
+	// number of remaining control plane members and whether or not this
+	// machine is one of them.
+	switch numControlPlaneMachines := len(util.GetControlPlaneMachines(machines)); {
+	case numControlPlaneMachines == 0:
+		// Do not delete the NodeRef if there are no remaining members of
+		// the control plane.
+		return errNoControlPlaneNodes
+	default:
+		// Otherwise it is okay to delete the NodeRef.
+		return nil
+	}
 }
 
 func (r *MachineReconciler) drainNode(ctx context.Context, cluster *clusterv1.Cluster, nodeName string, machineName string) error {

controllers/machine_helpers.go

@@ -20,12 +20,9 @@ import (
 	"context"
 
 	"github.com/pkg/errors"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
-	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -69,16 +66,3 @@ func hasMatchingLabels(matchSelector metav1.LabelSelector, matchLabels map[strin
 	}
 	return true
 }
-
-// getControlPlaneFromRef returns the control plane object from the provided ObjectReference
-func getControlPlaneFromRef(ctx context.Context, c client.Client, controlPlaneRef *corev1.ObjectReference) (*unstructured.Unstructured, error) {
-	// controlPlaneRef is an optional field in the Cluster so return nil if it isn't set
-	if controlPlaneRef == nil {
-		return nil, nil
-	}
-	controlPlane, err := external.Get(ctx, c, controlPlaneRef, controlPlaneRef.Namespace)
-	if err != nil {
-		return nil, err
-	}
-	return controlPlane, nil
-}

controllers/machine_helpers_test.go

@@ -22,14 +22,11 @@ import (
 
 	. "github.com/onsi/gomega"
 
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
-	kcpv1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3"
 )
 
 func Test_getActiveMachinesInCluster(t *testing.T) {
@@ -202,46 +199,3 @@ func TestMachineHealthCheckHasMatchingLabels(t *testing.T) {
 		})
 	}
 }
-
-func TestGetControlPlaneFromRef(t *testing.T) {
-	g := NewWithT(t)
-
-	kcp1 := kcpv1.KubeadmControlPlane{
-		TypeMeta: metav1.TypeMeta{
-			Kind: "KubeadmControlPlane",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "kcp1",
-			Namespace: "kcp1",
-			Labels: map[string]string{
-				clusterv1.ClusterLabelName: "kcp1",
-			},
-		},
-		Status: kcpv1.KubeadmControlPlaneStatus{
-			Ready:       true,
-			Initialized: true,
-		},
-	}
-
-	controlPlaneRef := &corev1.ObjectReference{
-		APIVersion: "controlplane.cluster.x-k8s.io/v1alpha3",
-		Kind:       "KubeadmControlPlane",
-		Name:       "kcp1",
-		Namespace:  "kcp1",
-	}
-
-	ctx := context.Background()
-	c := fake.NewFakeClientWithScheme(scheme.Scheme, &kcp1)
-	controlPlane, err := getControlPlaneFromRef(ctx, c, controlPlaneRef)
-	g.Expect(err).To(BeNil())
-
-	ready, found, err := unstructured.NestedBool(controlPlane.Object, "status", "ready")
-	g.Expect(ready).To(Equal(true))
-	g.Expect(found).To(Equal(true))
-	g.Expect(err).To(BeNil())
-
-	managed, found, err := unstructured.NestedBool(controlPlane.Object, "status", "externalManagedControlPlane")
-	g.Expect(managed).To(Equal(false))
-	g.Expect(found).To(Equal(false))
-	g.Expect(err).To(BeNil())
-}

util/util.go

@@ -200,10 +200,6 @@ func GetControlPlaneMachinesFromList(machineList *clusterv1.MachineList) (res []
 // IsExternalManagedControlPlane returns a bool indicating whether the control plane referenced
 // in the passed Unstructured resource is an externally managed control plane such as AKS, EKS, GKE, etc.
 func IsExternalManagedControlPlane(controlPlane *unstructured.Unstructured) bool {
-	// The controlPlaneRef field in the Cluster is optional, so return false if the control plane is not set
-	if controlPlane == nil {
-		return false
-	}
 	managed, found, err := unstructured.NestedBool(controlPlane.Object, "status", "externalManagedControlPlane")
 	if err != nil || !found {
 		return false

util/util_test.go

@@ -602,12 +602,6 @@ func TestGetMachinesForCluster(t *testing.T) {
 func TestIsExternalManagedControlPlane(t *testing.T) {
 	g := NewWithT(t)
 
-	t.Run("should return false if the controlPlane resource is nil", func(t *testing.T) {
-		var controlPlane *unstructured.Unstructured
-		result := IsExternalManagedControlPlane(controlPlane)
-		g.Expect(result).Should(Equal(false))
-	})
-
 	t.Run("should return true if control plane status externalManagedControlPlane is true", func(t *testing.T) {
 		controlPlane := &unstructured.Unstructured{
 			Object: map[string]interface{}{