fix: Always copy ResourceVersion before update

- Fix FakeDynamicClient to return an object from deletes
- Fix FakeDynamicClient to return DeepCopy to avoid mutation
- Fix InventoryClient to return a DeepCopy of the input when
  dry-run is enabled.

pkg/apply/common_test.go

@@ -295,24 +295,24 @@ func (ir *inventoryReactor) updateFakeDynamicClient(fdc *dynamicfake.FakeDynamic
 	fdc.PrependReactor("create", "configmaps", func(action clienttesting.Action) (bool, runtime.Object, error) {
 		obj := *action.(clienttesting.CreateAction).GetObject().(*unstructured.Unstructured)
 		ir.inventoryObj = &obj
-		return true, nil, nil
+		return true, ir.inventoryObj.DeepCopy(), nil
 	})
 	fdc.PrependReactor("list", "configmaps", func(action clienttesting.Action) (bool, runtime.Object, error) {
 		uList := &unstructured.UnstructuredList{
 			Items: []unstructured.Unstructured{},
 		}
 		if ir.inventoryObj != nil {
-			uList.Items = append(uList.Items, *ir.inventoryObj)
+			uList.Items = append(uList.Items, *ir.inventoryObj.DeepCopy())
 		}
 		return true, uList, nil
 	})
 	fdc.PrependReactor("get", "configmaps", func(action clienttesting.Action) (bool, runtime.Object, error) {
-		return true, ir.inventoryObj, nil
+		return true, ir.inventoryObj.DeepCopy(), nil
 	})
 	fdc.PrependReactor("update", "configmaps", func(action clienttesting.Action) (bool, runtime.Object, error) {
 		obj := *action.(clienttesting.UpdateAction).GetObject().(*unstructured.Unstructured)
 		ir.inventoryObj = &obj
-		return true, nil, nil
+		return true, ir.inventoryObj.DeepCopy(), nil
 	})
 }
 

pkg/inventory/inventory-client.go

@@ -110,15 +110,12 @@ func (cic *ClusterInventoryClient) Merge(localInv InventoryInfo, objs object.Obj
 			return nil, err
 		}
 		klog.V(4).Infof("creating initial inventory object with %d objects", len(objs))
-		var createdObj *unstructured.Unstructured
+		createdObj, err := cic.createInventoryObj(invInfo, dryRun)
-		if createdObj, err = cic.createInventoryObj(invInfo, dryRun); err != nil {
+		if err != nil {
 			return nil, err
 		}
-		// TODO(Liujingfang1): Remove the nil check statement after
+		// Status update requires the latest ResourceVersion
-		// fixing the client mock in the unit test.
+		invInfo.SetResourceVersion(createdObj.GetResourceVersion())
-		if createdObj != nil {
-			invInfo.SetResourceVersion(createdObj.GetResourceVersion())
-		}
 		if err := cic.updateStatus(invInfo, dryRun); err != nil {
 			return nil, err
 		}
@@ -144,20 +141,13 @@ func (cic *ClusterInventoryClient) Merge(localInv InventoryInfo, objs object.Obj
 			return pruneIds, nil
 		}
 		if !objs.Equal(clusterObjs) {
-			clusterInv, err = wrappedInv.GetObject()
+			klog.V(4).Infof("update cluster inventory: %s/%s", clusterInv.GetNamespace(), clusterInv.GetName())
+			appliedObj, err := cic.applyInventoryObj(clusterInv, dryRun)
 			if err != nil {
 				return pruneIds, err
 			}
-			klog.V(4).Infof("update cluster inventory: %s/%s", clusterInv.GetNamespace(), clusterInv.GetName())
+			// Status update requires the latest ResourceVersion
-			var appliedObj *unstructured.Unstructured
+			clusterInv.SetResourceVersion(appliedObj.GetResourceVersion())
-			if appliedObj, err = cic.applyInventoryObj(clusterInv, dryRun); err != nil {
-				return pruneIds, err
-			}
-			// TODO(Liujingfang1): Remove the nil check statement after
-			// fixing the client mock in the unit test.
-			if appliedObj != nil {
-				clusterInv.SetResourceVersion(appliedObj.GetResourceVersion())
-			}
 		}
 		if err := cic.updateStatus(clusterInv, dryRun); err != nil {
 			return pruneIds, err
@@ -190,15 +180,12 @@ func (cic *ClusterInventoryClient) Replace(localInv InventoryInfo, objs object.O
 	if !objs.Equal(clusterObjs) {
 		klog.V(4).Infof("replace cluster inventory: %s/%s", clusterInv.GetNamespace(), clusterInv.GetName())
 		klog.V(4).Infof("replace cluster inventory %d objects", len(objs))
-		var appliedObj *unstructured.Unstructured
+		appliedObj, err := cic.applyInventoryObj(clusterInv, dryRun)
-		if appliedObj, err = cic.applyInventoryObj(clusterInv, dryRun); err != nil {
+		if err != nil {
 			return fmt.Errorf("failed to write updated inventory to cluster: %w", err)
 		}
-		// TODO(Liujingfang1): Remove the nil check statement after
+		// Status update requires the latest ResourceVersion
-		// fixing the client mock in the unit test.
+		clusterInv.SetResourceVersion(appliedObj.GetResourceVersion())
-		if appliedObj != nil {
-			clusterInv.SetResourceVersion(appliedObj.GetResourceVersion())
-		}
 	}
 	if err := cic.updateStatus(clusterInv, dryRun); err != nil {
 		return err
@@ -364,7 +351,7 @@ func (cic *ClusterInventoryClient) GetClusterInventoryObjs(inv InventoryInfo) (o
 func (cic *ClusterInventoryClient) applyInventoryObj(obj *unstructured.Unstructured, dryRun common.DryRunStrategy) (*unstructured.Unstructured, error) {
 	if dryRun.ClientOrServerDryRun() {
 		klog.V(4).Infof("dry-run apply inventory object: not applied")
-		return nil, nil
+		return obj.DeepCopy(), nil
 	}
 	if obj == nil {
 		return nil, fmt.Errorf("attempting apply a nil inventory object")
@@ -384,7 +371,7 @@ func (cic *ClusterInventoryClient) applyInventoryObj(obj *unstructured.Unstructu
 func (cic *ClusterInventoryClient) createInventoryObj(obj *unstructured.Unstructured, dryRun common.DryRunStrategy) (*unstructured.Unstructured, error) {
 	if dryRun.ClientOrServerDryRun() {
 		klog.V(4).Infof("dry-run create inventory object: not created")
-		return nil, nil
+		return obj.DeepCopy(), nil
 	}
 	if obj == nil {
 		return nil, fmt.Errorf("attempting create a nil inventory object")