Merge pull request #1490 from devigned/nil-drain

fix nil panic in AzureMachinePoolMachine scope when no node is found for the providerID

Author: k8s-ci-robot

azure/scope/machinepoolmachine.go

@@ -288,10 +288,16 @@ func (s *MachinePoolMachineScope) CordonAndDrain(ctx context.Context) error {
 		node, err = s.workloadNodeGetter.GetNodeByObjectReference(ctx, *nodeRef)
 	}
 
-	if err != nil && apierrors.IsNotFound(err) {
-		return nil // node was already gone, so no need to cordon and drain
-	} else if err != nil {
+	switch {
+	case err != nil && !apierrors.IsNotFound(err):
+		// failed due to an unexpected error
 		return errors.Wrap(err, "failed to find node")
+	case err != nil && apierrors.IsNotFound(err):
+		// node was not found due to 404 when finding by ObjectReference
+		return nil
+	case node == nil:
+		// node was not found due to not finding a nodes with the ProviderID
+		return nil
 	}
 
 	// Drain node before deletion and issue a patch in order to make this operation visible to the users.
@@ -301,7 +307,7 @@ func (s *MachinePoolMachineScope) CordonAndDrain(ctx context.Context) error {
 			return errors.Wrap(err, "failed to build a patchHelper when draining node")
 		}
 
-		s.V(4).Info("Draining node", "node", s.AzureMachinePoolMachine.Status.NodeRef.Name)
+		s.V(4).Info("Draining node", "node", node.Name)
 		// The DrainingSucceededCondition never exists before the node is drained for the first time,
 		// so its transition time can be used to record the first time draining.
 		// This `if` condition prevents the transition time to be changed more than once.

azure/scope/machinepoolmachine_test.go

@@ -27,6 +27,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/klog/v2/klogr"
 	"sigs.k8s.io/cluster-api-provider-azure/api/v1alpha4"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
 	mock_scope "sigs.k8s.io/cluster-api-provider-azure/azure/scope/mocks"
@@ -316,6 +317,100 @@ func TestMachineScope_UpdateStatus(t *testing.T) {
 	}
 }
 
+func TestMachinePoolMachineScope_CordonAndDrain(t *testing.T) {
+	scheme := runtime.NewScheme()
+	_ = capiv1exp.AddToScheme(scheme)
+	_ = infrav1.AddToScheme(scheme)
+
+	var (
+		clusterScope = ClusterScope{
+			Cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "cluster-foo",
+				},
+			},
+		}
+	)
+
+	cases := []struct {
+		Name  string
+		Setup func(mockNodeGetter *mock_scope.MocknodeGetter, ampm *infrav1.AzureMachinePoolMachine) *infrav1.AzureMachinePoolMachine
+		Err   string
+	}{
+		{
+			Name: "should skip cordon and drain if the node does not exist with provider ID",
+			Setup: func(mockNodeGetter *mock_scope.MocknodeGetter, ampm *infrav1.AzureMachinePoolMachine) *infrav1.AzureMachinePoolMachine {
+				mockNodeGetter.EXPECT().GetNodeByProviderID(gomock2.AContext(), FakeProviderID).Return(nil, nil)
+				return ampm
+			},
+		},
+		{
+			Name: "should skip cordon and drain if the node does not exist with node reference",
+			Setup: func(mockNodeGetter *mock_scope.MocknodeGetter, ampm *infrav1.AzureMachinePoolMachine) *infrav1.AzureMachinePoolMachine {
+				nodeRef := corev1.ObjectReference{
+					Name: "node1",
+				}
+				ampm.Status.NodeRef = &nodeRef
+				mockNodeGetter.EXPECT().GetNodeByObjectReference(gomock2.AContext(), nodeRef).Return(nil, nil)
+				return ampm
+			},
+		},
+		{
+			Name: "if GetNodeByProviderID fails with an error, an error will be returned",
+			Setup: func(mockNodeGetter *mock_scope.MocknodeGetter, ampm *infrav1.AzureMachinePoolMachine) *infrav1.AzureMachinePoolMachine {
+				mockNodeGetter.EXPECT().GetNodeByProviderID(gomock2.AContext(), FakeProviderID).Return(nil, errors.New("boom"))
+				return ampm
+			},
+			Err: "failed to find node: boom",
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.Name, func(t *testing.T) {
+			var (
+				controller = gomock.NewController(t)
+				mockClient = mock_scope.NewMocknodeGetter(controller)
+				g          = NewWithT(t)
+				params     = MachinePoolMachineScopeParams{
+					Client:       fake.NewClientBuilder().WithScheme(scheme).Build(),
+					ClusterScope: &clusterScope,
+					MachinePool: &capiv1exp.MachinePool{
+						Spec: capiv1exp.MachinePoolSpec{
+							Template: clusterv1.MachineTemplateSpec{
+								Spec: clusterv1.MachineSpec{
+									Version: to.StringPtr("v1.19.11"),
+								},
+							},
+						},
+					},
+					AzureMachinePool: new(infrav1.AzureMachinePool),
+				}
+			)
+
+			defer controller.Finish()
+
+			ampm := c.Setup(mockClient, &infrav1.AzureMachinePoolMachine{
+				Spec: infrav1.AzureMachinePoolMachineSpec{
+					ProviderID: FakeProviderID,
+				},
+			})
+			params.AzureMachinePoolMachine = ampm
+			s, err := NewMachinePoolMachineScope(params)
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(s).ToNot(BeNil())
+			s.Logger = klogr.New()
+			s.workloadNodeGetter = mockClient
+
+			err = s.CordonAndDrain(context.TODO())
+			if c.Err == "" {
+				g.Expect(err).To(Succeed())
+			} else {
+				g.Expect(err).To(MatchError(c.Err))
+			}
+		})
+	}
+}
+
 func getReadyNode() *corev1.Node {
 	return &corev1.Node{
 		ObjectMeta: metav1.ObjectMeta{