From 4af6e261d09320643253389afb07c87a27aebe76 Mon Sep 17 00:00:00 2001
From: momesgin <mo.mesgin@suse.com>
Date: Thu, 18 Apr 2024 08:25:12 -0700
Subject: [PATCH] Fix removing array type values in machine pool configs
 (#10741)

* fix removing array type values in machine pool configs

* update comment

* lint

* limit custom merge helper to vsphere

---------

Co-authored-by: Mo Mesgin <mmesgin@Mos-M2-MacBook-Pro.local>
---
 .../provisioning.cattle.io.cluster/rke2.vue   | 12 ++++++--
 .../vmwarevsphere-pool-config-merge.test.ts   | 30 +++++++++++++++++++
 .../vmwarevsphere-pool-config-merge.ts        | 25 ++++++++++++++++
 3 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 shell/machine-config/__tests__/vmwarevsphere-pool-config-merge.test.ts
 create mode 100644 shell/machine-config/vmwarevsphere-pool-config-merge.ts

diff --git a/shell/edit/provisioning.cattle.io.cluster/rke2.vue b/shell/edit/provisioning.cattle.io.cluster/rke2.vue
index 5514d817ee..69e900d940 100644
--- a/shell/edit/provisioning.cattle.io.cluster/rke2.vue
+++ b/shell/edit/provisioning.cattle.io.cluster/rke2.vue
@@ -26,6 +26,7 @@ import {
 } from '@shell/utils/object';
 import { allHash } from '@shell/utils/promise';
 import { sortBy } from '@shell/utils/sort';
+import { vspherePoolConfigMerge } from '@shell/machine-config/vmwarevsphere-pool-config-merge';
 
 import { compare, sortable } from '@shell/utils/version';
 import { isHarvesterSatisfiesVersion } from '@shell/utils/cluster';
@@ -66,6 +67,8 @@ const HARVESTER = 'harvester';
 const HARVESTER_CLOUD_PROVIDER = 'harvester-cloud-provider';
 const NETBIOS_TRUNCATION_LENGTH = 15;
 
+const VMWARE_VSPHERE = 'vmwarevsphere';
+
 /**
  * Classes to be adopted by the node badges in Machine pools
  */
@@ -1104,7 +1107,7 @@ export default {
         },
       };
 
-      if (this.provider === 'vmwarevsphere') {
+      if (this.provider === VMWARE_VSPHERE) {
         pool.pool.machineOS = 'linux';
       }
 
@@ -1149,7 +1152,12 @@ export default {
         // We don't allow the user to edit any of the fields in metadata from the UI so it's safe to override it with the
         // metadata defined by the latest backend value. This is primarily used to ensure the resourceVersion is up to date.
         delete clonedCurrentConfig.metadata;
-        machinePool.config = merge(clonedLatestConfig, clonedCurrentConfig);
+
+        if (this.provider === VMWARE_VSPHERE) {
+          machinePool.config = vspherePoolConfigMerge(clonedLatestConfig, clonedCurrentConfig);
+        } else {
+          machinePool.config = merge(clonedLatestConfig, clonedCurrentConfig);
+        }
       }
     },
 
diff --git a/shell/machine-config/__tests__/vmwarevsphere-pool-config-merge.test.ts b/shell/machine-config/__tests__/vmwarevsphere-pool-config-merge.test.ts
new file mode 100644
index 0000000000..1c5990ae83
--- /dev/null
+++ b/shell/machine-config/__tests__/vmwarevsphere-pool-config-merge.test.ts
@@ -0,0 +1,30 @@
+import { vspherePoolConfigMerge } from '@shell/machine-config/vmwarevsphere-pool-config-merge';
+
+describe('vspherePoolConfigMerge', () => {
+  const testCases: Array<[object?, object?, object?]> = [
+    // Some array test cases, an array from the first object should be replaced with the array from the second object
+    [{ a: ['one'] }, { a: [] }, { a: [] }],
+    [{ a: ['one', 'two'] }, { a: ['one', 'two', 'three'] }, { a: ['one', 'two', 'three'] }],
+    [{ a: ['one', 'two'], b: ['three', 'four'] }, { a: ['one'], b: [] }, { a: ['one'], b: [] }],
+    [{
+      a: ['one', 'two'], b: ['three', 'four'], c: 'five'
+    }, { a: ['one'], b: [] }, {
+      a: ['one'], b: [], c: 'five'
+    }],
+    // Some other test cases
+    [{ a: 'one' }, { b: 'two' }, { a: 'one', b: 'two' }],
+    [{ a: 'one' }, { a: '', b: 'two' }, { a: '', b: 'two' }],
+    [{ a: 'one', b: 'two' }, { a: 1, c: { d: null } }, {
+      a: 1, b: 'two', c: { d: null }
+    }],
+    [undefined, undefined, {}],
+    [{}, undefined, {}],
+    [undefined, {}, {}],
+  ];
+
+  it.each(testCases)('should merge properly', (obj1, obj2, expected) => {
+    const result = vspherePoolConfigMerge(obj1, obj2);
+
+    expect(result).toStrictEqual(expected);
+  });
+});
diff --git a/shell/machine-config/vmwarevsphere-pool-config-merge.ts b/shell/machine-config/vmwarevsphere-pool-config-merge.ts
new file mode 100644
index 0000000000..b18689c767
--- /dev/null
+++ b/shell/machine-config/vmwarevsphere-pool-config-merge.ts
@@ -0,0 +1,25 @@
+import mergeWith from 'lodash/mergeWith';
+
+/**
+ * Helper function to alter Lodash merge function default behaviour on merging arrays while updating machine pool configuration.
+ *
+ * In rke2.vue, the syncMachineConfigWithLatest function updates machine pool configuration by
+ * merging the latest configuration received from the backend with the current configuration updated by the user.
+ * However, Lodash's merge function treats arrays like object so index values are merged and not appended to arrays
+ * resulting in undesired outcomes for us, Example:
+ *
+ * const lastSavedConfigFromBE = { a: ["test"] };
+ * const currentConfigByUser = { a: [] };
+ * merge(lastSavedConfigFromBE, currentConfigByUser); // returns { a: ["test"] }; but we expect { a: [] };
+ *
+ * More info: https://github.com/lodash/lodash/issues/1313
+ *
+ * This helper function addresses the issue by always replacing the old array with the new array during the merge process.
+ */
+export function vspherePoolConfigMerge(obj1 = {}, obj2 = {}): Object {
+  return mergeWith(obj1, obj2, (obj1Value, obj2Value) => {
+    if (Array.isArray(obj1Value) && Array.isArray(obj2Value)) {
+      return obj2Value;
+    }
+  });
+}