FastApply: implement and test replacingFields

Signed-off-by: Silvio Moioli <silvio@moioli.net>

pkg/apply/desiredset_compare.go

@@ -100,7 +100,7 @@ func emptyMaps(data map[string]interface{}, keys ...string) bool {
 	return true
 }
 
-func sanitizePatch(patch []byte, removeObjectSetAnnotation bool) ([]byte, error) {
+func sanitizePatch(patch []byte, removeObjectSetAnnotation bool, fastApply bool, replacingFields []string) ([]byte, error) {
 	mod := false
 	data := map[string]interface{}{}
 	err := json.Unmarshal(patch, &data)
@@ -144,6 +144,13 @@ func sanitizePatch(patch []byte, removeObjectSetAnnotation bool) ([]byte, error)
 		}
 	}
 
+	if fastApply {
+		// no patch should ever delete fields unless replacingFields says so
+		// -> remove deletions from patch if they do not match replacingFields
+		removed := removeDeletionsFromPatch("", data, replacingFields)
+		mod = mod || removed
+	}
+
 	if emptyMaps(data, "metadata", "annotations") {
 		return []byte("{}"), nil
 	}
@@ -155,7 +162,75 @@ func sanitizePatch(patch []byte, removeObjectSetAnnotation bool) ([]byte, error)
 	return json.Marshal(data)
 }
 
-func applyPatch(gvk schema.GroupVersionKind, reconciler Reconciler, patcher Patcher, debugID string, ignoreOriginal, fastApply bool, diffPatches [][]byte, oldObject, newObject runtime.Object) (bool, error) {
+// hasPrefixIn returns true if any prefixes match s
+func hasPrefixIn(s string, prefixes []string) bool {
+	for _, prefix := range prefixes {
+		if strings.HasPrefix(s, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
+// removeDeletionsFromPatch removes deletions from JSON Merge Patch or a Strategic Merge Patch, excluding exclusions
+// returns true if the patch was modified
+func removeDeletionsFromPatch(prefix string, data map[string]any, exclusions []string) bool {
+	modified := false
+
+	for field, value := range data {
+		excludedFromDeletion := hasPrefixIn(prefix+field, exclusions)
+
+		// delete JSON Merge Patch style deletions that should not be retained
+		// see https://datatracker.ietf.org/doc/html/rfc7386
+		if value == nil {
+			if !excludedFromDeletion {
+				delete(data, field)
+				modified = true
+				continue
+			}
+		}
+
+		// delete Strategic Merge Patch style deletions that should not be retained
+		// https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md#delete-directive
+		// if the field is not in the retaining map, remove it
+		if mapValue, ok := value.(map[string]any); ok {
+			if patchValue, ok := mapValue["$patch"]; ok {
+				if stringValue, ok := patchValue.(string); ok && stringValue == "delete" {
+					if !excludedFromDeletion {
+						delete(data, field)
+						modified = true
+						continue
+					}
+				}
+			}
+		}
+
+		// sub-fields of excluded fields are also excluded, skip to the next
+		if excludedFromDeletion {
+			continue
+		}
+
+		// else call recursively depending on value type
+		if mapValue, ok := value.(map[string]interface{}); ok {
+			// item is a map, recurse
+			mapModified := removeDeletionsFromPatch(prefix+field+".", mapValue, exclusions)
+			modified = modified || mapModified
+		} else if listValue, ok := value.([]interface{}); ok {
+			for _, item := range listValue {
+				// list item is an object, recurse
+				if mapValue, ok := item.(map[string]interface{}); ok {
+					mapModified := removeDeletionsFromPatch(prefix+field+".", mapValue, exclusions)
+					modified = modified || mapModified
+					continue
+				}
+			}
+		}
+	}
+
+	return modified
+}
+
+func applyPatch(gvk schema.GroupVersionKind, reconciler Reconciler, patcher Patcher, debugID string, ignoreOriginal, fastApply bool, replacingFields []string, diffPatches [][]byte, oldObject, newObject runtime.Object) (bool, error) {
 	oldMetadata, err := meta.Accessor(oldObject)
 	if err != nil {
 		return false, err
@@ -189,7 +264,7 @@ func applyPatch(gvk schema.GroupVersionKind, reconciler Reconciler, patcher Patc
 		return false, nil
 	}
 
-	patch, err = sanitizePatch(patch, false)
+	patch, err = sanitizePatch(patch, false, fastApply, replacingFields)
 	if err != nil {
 		return false, err
 	}
@@ -250,7 +325,7 @@ func (o *desiredSet) compareObjects(gvk schema.GroupVersionKind, reconciler Reco
 		GroupVersionKind: gvk,
 	}]...)
 
-	if ran, err := applyPatch(gvk, reconciler, patcher, debugID, o.ignorePreviousApplied, o.fastApply, diffPatches, oldObject, newObject); err != nil {
+	if ran, err := applyPatch(gvk, reconciler, patcher, debugID, o.ignorePreviousApplied, o.fastApply, o.replacingFields, diffPatches, oldObject, newObject); err != nil {
 		return err
 	} else if !ran {
 		logrus.Debugf("DesiredSet - No change(2) %s %s/%s for %s", gvk, oldMetadata.GetNamespace(), oldMetadata.GetName(), debugID)

pkg/apply/desiredset_compare_test.go

@@ -552,6 +552,8 @@ func Test_sanitizePatch(t *testing.T) {
 	type args struct {
 		patch                     []byte
 		removeObjectSetAnnotation bool
+		fastApply                 bool
+		replacingFields           []string
 	}
 	tests := []struct {
 		name    string
@@ -564,6 +566,7 @@ func Test_sanitizePatch(t *testing.T) {
 			args: args{
 				patch:                     []byte(`{}`),
 				removeObjectSetAnnotation: false,
+				replacingFields:           []string{},
 			},
 			want:    []byte(`{}`),
 			wantErr: assert.NoError,
@@ -573,6 +576,7 @@ func Test_sanitizePatch(t *testing.T) {
 			args: args{
 				patch:                     []byte(`{1: "one"}`),
 				removeObjectSetAnnotation: false,
+				replacingFields:           []string{},
 			},
 			want:    nil,
 			wantErr: assert.Error,
@@ -582,6 +586,7 @@ func Test_sanitizePatch(t *testing.T) {
 			args: args{
 				patch:                     []byte(`{"kind": "patched", "apiVersion": "patched", "status": "patched", "metadata": {"creationTimestamp": "patched", "preserve": "this"}, "preserve": "this too"}`),
 				removeObjectSetAnnotation: false,
+				replacingFields:           []string{},
 			},
 			want:    []byte(`{"metadata":{"preserve":"this"},"preserve":"this too"}`),
 			wantErr: assert.NoError,
@@ -591,6 +596,7 @@ func Test_sanitizePatch(t *testing.T) {
 			args: args{
 				patch:                     []byte(`{"metadata": {"annotations": {"objectset.rio.cattle.io/test": "delete me"}}}`),
 				removeObjectSetAnnotation: true,
+				replacingFields:           []string{},
 			},
 			want:    []byte(`{}`),
 			wantErr: assert.NoError,
@@ -600,14 +606,37 @@ func Test_sanitizePatch(t *testing.T) {
 			args: args{
 				patch:                     []byte(`{"metadata": {"annotations": {"objectset.rio.cattle.io/test": "do not delete me"}}}`),
 				removeObjectSetAnnotation: false,
+				replacingFields:           []string{},
 			},
 			want:    []byte(`{"metadata": {"annotations": {"objectset.rio.cattle.io/test": "do not delete me"}}}`),
 			wantErr: assert.NoError,
 		},
+		{
+			name: "RemoveJSONPatchDeletions",
+			args: args{
+				patch:                     []byte(`{"a":{"b":[{"c":[{"d":null},{"e":null},{"f":"leave f alone"}]}]},"z":"leave z alone"}`),
+				removeObjectSetAnnotation: true,
+				fastApply:                 true,
+				replacingFields:           []string{"a.b.c.e", "a.b.c.f"},
+			},
+			want:    []byte(`{"a":{"b":[{"c":[{},{"e":null},{"f":"leave f alone"}]}]},"z":"leave z alone"}`),
+			wantErr: assert.NoError,
+		},
+		{
+			name: "RemoveStrategicPatchDeletions",
+			args: args{
+				patch:                     []byte(`{"a":{"b":[{"c":[{"d":{"$patch": "delete"}},{"e":{"$patch": "delete"}},{"f":"leave f alone"}]}]},"z":"leave z alone"}`),
+				removeObjectSetAnnotation: true,
+				fastApply:                 true,
+				replacingFields:           []string{"a.b.c.e", "a.b.c.f"},
+			},
+			want:    []byte(`{"a":{"b":[{"c":[{},{"e":{"$patch":"delete"}},{"f":"leave f alone"}]}]},"z":"leave z alone"}`),
+			wantErr: assert.NoError,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := sanitizePatch(tt.args.patch, tt.args.removeObjectSetAnnotation)
+			got, err := sanitizePatch(tt.args.patch, tt.args.removeObjectSetAnnotation, tt.args.fastApply, tt.args.replacingFields)
 			if !tt.wantErr(t, err, fmt.Sprintf("sanitizePatch(%v, %v)", tt.args.patch, tt.args.removeObjectSetAnnotation)) {
 				return
 			}
@@ -616,6 +645,145 @@ func Test_sanitizePatch(t *testing.T) {
 	}
 }
 
+func Test_removeDeletionsFromPatch(t *testing.T) {
+	type args struct {
+		data      string
+		retaining []string
+	}
+	tests := []struct {
+		name         string
+		args         args
+		modified     bool
+		modifiedData string
+	}{
+		{
+			name: "JSONMergePatchNonExistingField",
+			args: args{
+				data:      `{"a":"z","c":"d"}`,
+				retaining: []string{"containers"},
+			},
+			modified:     false,
+			modifiedData: `{"a":"z","c":"d"}`,
+		},
+		{
+			name: "JSONMergePatchNonExistingFieldWithDeletion",
+			args: args{
+				data:      `{"a":"z","c": {"f": null}}`,
+				retaining: []string{"containers"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c":{}}`,
+		},
+		{
+			name: "JSONMergePatchRetainDeletion",
+			args: args{
+				data:      `{"a":"z","c":{"f": null}}`,
+				retaining: []string{"c.f"},
+			},
+			modified:     false,
+			modifiedData: `{"a":"z","c":{"f":null}}`,
+		},
+		{
+			name: "JSONMergePatchRetainDeletionRecursively",
+			args: args{
+				data:      `{"a":"z","c":{"f": null}}`,
+				retaining: []string{"c"},
+			},
+			modified:     false,
+			modifiedData: `{"a":"z","c":{"f":null}}`,
+		},
+		{
+			name: "JSONMergePatchDeleteDeletion",
+			args: args{
+				data:      `{"a":"z","c":{"f":null}}`,
+				retaining: []string{"a"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c":{}}`,
+		},
+		{
+			name: "JSONMergePatchDeleteDeletionWithOverspecificRetaining",
+			args: args{
+				data:      `{"a":"z","c":{"f":null}}`,
+				retaining: []string{"a", "c.f.blabla"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c":{}}`,
+		},
+		{
+			name: "JSONMergePatchDeleteFromArrayField",
+			args: args{
+				data:      `{"a":"z","c": [{"f": null}, {"g": null}]}`,
+				retaining: []string{"a", "c.g", "c.f.h"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c": [{},{"g": null}]}`,
+		},
+
+		{
+			name: "JSONStrategicMergePatchNonExistingFieldWithDeletion",
+			args: args{
+				data:      `{"a":"z","c":{"f":{"$patch": "delete"}}}`,
+				retaining: []string{"containers"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c":{}}`,
+		},
+		{
+			name: "JSONMergePatchRetainDeletion",
+			args: args{
+				data:      `{"a":"z","c":{"f":{"$patch":"delete"}}}`,
+				retaining: []string{"c.f"},
+			},
+			modifiedData: `{"a":"z","c":{"f":{"$patch":"delete"}}}`,
+		},
+		{
+			name: "JSONMergePatchRetainDeletionRecursively",
+			args: args{
+				data:      `{"a":"z","c":{"f":{"$patch":"delete"}}}`,
+				retaining: []string{"c"},
+			},
+			modifiedData: `{"a":"z","c":{"f":{"$patch":"delete"}}}`,
+		},
+		{
+			name: "JSONStrategicMergePatchDeleteDeletion",
+			args: args{
+				data:      `{"a":"z","c": {"f": {"$patch": "delete"}}}`,
+				retaining: []string{"a"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c":{}}`,
+		},
+		{
+			name: "JSONStrategicMergePatchDeleteDeletionWithOverspecificRetaining",
+			args: args{
+				data:      `{"a":"z","c":{"f":{"$patch":"delete"}}}`,
+				retaining: []string{"a", "c.f.blabla"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c":{}}`,
+		},
+		{
+			name: "JSONStrategicMergePatchDeleteFromArrayField",
+			args: args{
+				data:      `{"a":"z","c": [{"f": {"$patch": "delete"}}, {"g": {"$patch": "delete"}}]}`,
+				retaining: []string{"a", "c.g", "c.f.h"},
+			},
+			modified:     true,
+			modifiedData: `{"a":"z","c": [{},{"g": {"$patch": "delete"}}]}`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			data := toMap(tt.args.data, t)
+			assert.Equalf(t, tt.modified, removeDeletionsFromPatch("", data, tt.args.retaining), "removeDeletionsFromPatch(%v, %v)", data, tt.args.retaining)
+
+			modifiedData := toMap(tt.modifiedData, t)
+			assert.Equalf(t, modifiedData, data, "removeDeletionsFromPatch(%v, %v)", data, tt.args.retaining)
+		})
+	}
+}
+
 // Utilities
 
 // testCRDGVK is the GVK of a CustomResourceDefinition, which uses MergePatchType (because it is not registered)
@@ -644,6 +812,17 @@ func toBytes(obj any, t *testing.T) []byte {
 	return res
 }
 
+// toMap converts a JSON string to a map
+func toMap(data string, t *testing.T) map[string]any {
+	t.Helper()
+	var obj map[string]any
+	err := json.Unmarshal([]byte(data), &obj)
+	if err != nil {
+		t.Fatalf("failed to unmarshal %v: %v", data, err)
+	}
+	return obj
+}
+
 // configMapGVK is the GVK of a ConfigMap, which uses StrategicMergePatchType
 var configMapGVK = schema.GroupVersionKind{
 	Group:   "",

pkg/apply/desiredset_process.go

@@ -268,7 +268,7 @@ func (o *desiredSet) process(debugID string, set labels.Selector, gvk schema.Gro
 
 		reconciler = nil
 		patcher = func(namespace, name string, pt types2.PatchType, data []byte) (runtime.Object, error) {
-			data, err := sanitizePatch(data, true)
+			data, err := sanitizePatch(data, true, o.fastApply, o.replacingFields)
 			if err != nil {
 				return nil, err
 			}