🐛 Fix node deletion under managed control planes

controllers/machine_controller.go

@@ -376,24 +376,38 @@ func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, cluster *cl
 		return errNilNodeRef
 	}
 
-	// Get all of the machines that belong to this cluster.
-	machines, err := getActiveMachinesInCluster(ctx, r.Client, machine.Namespace, machine.Labels[clusterv1.ClusterLabelName])
+	// Get the control plane object associated with the cluster
+	controlPlane, err := getControlPlaneFromRef(ctx, r.Client, cluster.Spec.ControlPlaneRef)
 	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
+	// 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) {
+
+		// 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
+		}
 	}
+	// In other control plane types, it is OK to delete the NodeRef
+	return nil
 }
 
 func (r *MachineReconciler) drainNode(ctx context.Context, cluster *clusterv1.Cluster, nodeName string, machineName string) error {

controllers/machine_controller_test.go

@@ -783,7 +783,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 			expectedError: nil,
 		},
 		{
-			name: "has nodeRef and control plane is healthy",
+			name: "has nodeRef and control plane is being deleted",
 			cluster: &clusterv1.Cluster{
 				ObjectMeta: metav1.ObjectMeta{
 					DeletionTimestamp: &deletionts,
@@ -792,6 +792,40 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 			machine:       &clusterv1.Machine{},
 			expectedError: errClusterIsBeingDeleted,
 		},
+		{
+			name: "has nodeRef and control plane is healthy and externally managed",
+			cluster: &clusterv1.Cluster{
+				Spec: clusterv1.ClusterSpec{
+					ControlPlaneRef: &corev1.ObjectReference{
+						APIVersion: "controlplane.cluster.x-k8s.io/v1alpha3",
+						Kind:       "AWSManagedControlPlane",
+						Name:       "test-cluster",
+						Namespace:  "test-cluster",
+					},
+				},
+			},
+			machine: &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "created",
+					Namespace: "default",
+					Labels: map[string]string{
+						clusterv1.ClusterLabelName: "test",
+					},
+					Finalizers: []string{clusterv1.MachineFinalizer},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName:       "test-cluster",
+					InfrastructureRef: corev1.ObjectReference{},
+					Bootstrap:         clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
+				},
+				Status: clusterv1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: "test",
+					},
+				},
+			},
+			expectedError: nil,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -844,13 +878,26 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
 				m2.Labels[clusterv1.MachineControlPlaneLabelName] = ""
 			}
 
+			emp := &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"status": map[string]interface{}{
+						"externalManagedControlPlane": true,
+					},
+				},
+			}
+			emp.SetAPIVersion("controlplane.cluster.x-k8s.io/v1alpha3")
+			emp.SetKind("AWSManagedControlPlane")
+			emp.SetName("test-cluster")
+			emp.SetNamespace("test-cluster")
+
 			mr := &MachineReconciler{
 				Client: helpers.NewFakeClientWithScheme(
 					scheme.Scheme,
 					tc.cluster,
 					tc.machine,
 					m1,
 					m2,
+					emp,
 				),
 				Log:    log.Log,
 				scheme: scheme.Scheme,

controllers/machine_helpers.go

@@ -20,9 +20,12 @@ 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"
 )
 
@@ -66,3 +69,16 @@ 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,11 +22,14 @@ 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) {
@@ -199,3 +202,46 @@ 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

@@ -197,6 +197,20 @@ func GetControlPlaneMachinesFromList(machineList *clusterv1.MachineList) (res []
 	return
 }
 
+// IsExternalManagedControlPlane returns a bool indicating whether the control plane referenced
+// in the passed ObjectReference 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
+	}
+	return managed
+}
+
 // GetMachineIfExists gets a machine from the API server if it exists.
 func GetMachineIfExists(c client.Client, namespace, name string) (*clusterv1.Machine, error) {
 	if c == nil {

util/util_test.go

@@ -25,6 +25,7 @@ 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/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
@@ -598,6 +599,46 @@ func TestGetMachinesForCluster(t *testing.T) {
 	g.Expect(machines.Items[0].Labels[clusterv1.ClusterLabelName]).To(Equal(cluster.Name))
 }
 
+func TestIsExternalManagedControlPlane(t *testing.T) {
+	g := NewWithT(t)
+
+	t.Run("should return true if control plane status externalManagedControlPlane is true", func(t *testing.T) {
+		controlPlaneRef := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"status": map[string]interface{}{
+					"externalManagedControlPlane": true,
+				},
+			},
+		}
+		result := IsExternalManagedControlPlane(controlPlaneRef)
+		g.Expect(result).Should(Equal(true))
+	})
+
+	t.Run("should return false if control plane status externalManagedControlPlane is false", func(t *testing.T) {
+		controlPlaneRef := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"status": map[string]interface{}{
+					"externalManagedControlPlane": false,
+				},
+			},
+		}
+		result := IsExternalManagedControlPlane(controlPlaneRef)
+		g.Expect(result).Should(Equal(false))
+	})
+
+	t.Run("should return false if control plane status externalManagedControlPlane is not set", func(t *testing.T) {
+		controlPlaneRef := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"status": map[string]interface{}{
+					"someOtherStatusField": "someValue",
+				},
+			},
+		}
+		result := IsExternalManagedControlPlane(controlPlaneRef)
+		g.Expect(result).Should(Equal(false))
+	})
+}
+
 func TestEnsureOwnerRef(t *testing.T) {
 	g := NewWithT(t)