fix(cordonhelper): Avoid mutating local node before API call

Signed-off-by: cz21 <cz21ok@gmail.com>

staging/src/k8s.io/kubectl/pkg/drain/cordon.go

@@ -86,9 +86,10 @@ func (c *CordonHelper) PatchOrReplaceWithContext(clientCtx context.Context, clie
 		return err, nil
 	}
 
-	c.node.Spec.Unschedulable = c.desired
+	desiredNode := c.node.DeepCopy()
+	desiredNode.Spec.Unschedulable = c.desired
 
-	newData, err := json.Marshal(c.node)
+	newData, err := json.Marshal(desiredNode)
 	if err != nil {
 		return err, nil
 	}
@@ -99,13 +100,21 @@ func (c *CordonHelper) PatchOrReplaceWithContext(clientCtx context.Context, clie
 		if serverDryRun {
 			patchOptions.DryRun = []string{metav1.DryRunAll}
 		}
-		_, err = client.Patch(clientCtx, c.node.Name, types.StrategicMergePatchType, patchBytes, patchOptions)
+		var updatedNode *corev1.Node
+		updatedNode, err = client.Patch(clientCtx, c.node.Name, types.StrategicMergePatchType, patchBytes, patchOptions)
+		if err == nil {
+			c.node.Spec.Unschedulable = updatedNode.Spec.Unschedulable
+		}
 	} else {
 		updateOptions := metav1.UpdateOptions{}
 		if serverDryRun {
 			updateOptions.DryRun = []string{metav1.DryRunAll}
 		}
-		_, err = client.Update(clientCtx, c.node, updateOptions)
+		var updatedNode *corev1.Node
+		updatedNode, err = client.Update(clientCtx, desiredNode, updateOptions)
+		if err == nil {
+			c.node.Spec.Unschedulable = updatedNode.Spec.Unschedulable
+		}
 	}
 	return err, patchErr
 }

staging/src/k8s.io/kubectl/pkg/drain/cordon_test.go

@@ -17,6 +17,8 @@ limitations under the License.
 package drain
 
 import (
+	"context"
+	"errors"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -24,6 +26,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
 )
 
 type newCordonHelperFromRuntimeObjectTestCase struct {
@@ -145,3 +149,95 @@ func TestUpdateIfRequired(t *testing.T) {
 		})
 	}
 }
+
+func TestCordonHelper_PatchOrReplaceWithContext(t *testing.T) {
+	tests := []struct {
+		name         string
+		node         *corev1.Node
+		clientset    *fake.Clientset
+		serverDryRun bool
+		shouldFail   bool
+	}{
+		{
+			name: "update success: local object should be updated",
+			node: &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "fake-node",
+				},
+				Spec: corev1.NodeSpec{
+					Unschedulable: false,
+				},
+			},
+			clientset: fake.NewClientset(&corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "fake-node",
+				},
+				Spec: corev1.NodeSpec{
+					Unschedulable: false,
+				},
+			}),
+			serverDryRun: false,
+			shouldFail:   false,
+		},
+		{
+			name: "update faild: local object should not be updated",
+			node: &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "fake-node",
+				},
+				Spec: corev1.NodeSpec{
+					Unschedulable: false,
+				},
+			},
+			clientset: fake.NewClientset(&corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "fake-node",
+				},
+				Spec: corev1.NodeSpec{
+					Unschedulable: false,
+				},
+			}),
+			serverDryRun: false,
+			shouldFail:   true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := NewCordonHelper(tt.node)
+			c.desired = true
+			if tt.shouldFail {
+				tt.clientset.Fake.PrependReactor("update", "nodes", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, errors.New("fake update error")
+				})
+				tt.clientset.Fake.PrependReactor("patch", "nodes", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, errors.New("fake update error")
+				})
+			}
+
+			err, _ := c.PatchOrReplaceWithContext(context.Background(), tt.clientset, tt.serverDryRun)
+			if tt.shouldFail {
+				if err == nil {
+					t.Errorf("PatchOrReplaceWithContext() failed: update should failed but no error returned")
+					return
+				}
+			} else {
+				if err != nil {
+					t.Errorf("PatchOrReplaceWithContext() failed: update should succeed but got error: %v", err)
+					return
+				}
+			}
+
+			if err != nil {
+				t.Logf("patch/update failed, error: %v, c.node.Spec.Unschedulable shouldn't be updated", err)
+				if c.node.Spec.Unschedulable == c.desired {
+					t.Errorf("PatchOrReplaceWithContext() failed: update failed but c.node's unschedulable was updated")
+				}
+			} else {
+				t.Log("patch/update successed, c.node.Spec.Unschedulable should be updated")
+				if c.node.Spec.Unschedulable != c.desired {
+					t.Errorf("PatchOrReplaceWithContext() failed: update succeeded but c.node's unschedulable was not updated")
+				}
+			}
+		})
+	}
+}