Merge pull request #44597 from mengqiy/replacekeys

Automatic merge from submit-queue (batch tested with PRs 46302, 44597, 44742, 46554)

support replaceKeys patch strategy

Implementing according to [this proposal](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md).
The revision is in kubernetes/community#620.

```release-note
support replaceKeys patch strategy and directive for strategic merge patch
```

staging/src/k8s.io/apimachinery/pkg/util/mergepatch/errors.go

@@ -23,9 +23,11 @@ import (
 )
 
 var (
-	ErrBadJSONDoc                     = errors.New("Invalid JSON document")
+	ErrBadJSONDoc                     = errors.New("invalid JSON document")
-	ErrNoListOfLists                  = errors.New("Lists of lists are not supported")
+	ErrNoListOfLists                  = errors.New("lists of lists are not supported")
-	ErrBadPatchFormatForPrimitiveList = errors.New("Invalid patch format of primitive list")
+	ErrBadPatchFormatForPrimitiveList = errors.New("invalid patch format of primitive list")
+	ErrBadPatchFormatForRetainKeys    = errors.New("invalid patch format of retainKeys")
+	ErrPatchContentNotMatchRetainKeys = errors.New("patch content doesn't match retainKeys list")
 )
 
 func ErrNoMergeKey(m map[string]interface{}, k string) error {

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go

@@ -43,7 +43,10 @@ const (
 	replaceDirective = "replace"
 	mergeDirective   = "merge"
 
+	retainKeysStrategy = "retainKeys"
+
 	deleteFromPrimitiveListDirectivePrefix = "$deleteFromPrimitiveList"
+	retainKeysDirective                    = "$" + retainKeysStrategy
 )
 
 // JSONMap is a representations of JSON object encoded as map[string]interface{}
@@ -58,11 +61,21 @@ type DiffOptions struct {
 	IgnoreChangesAndAdditions bool
 	// IgnoreDeletions indicates if we keep the deletions in the patch.
 	IgnoreDeletions bool
+	// We introduce a new value retainKeys for patchStrategy.
+	// It indicates that all fields needing to be preserved must be
+	// present in the `retainKeys` list.
+	// And the fields that are present will be merged with live object.
+	// All the missing fields will be cleared when patching.
+	BuildRetainKeysDirective bool
 }
 
 type MergeOptions struct {
-	// MergeDeleteList indicates if we are merging the delete parallel list.
+	// MergeParallelList indicates if we are merging the parallel list.
-	MergeDeleteList bool
+	// We don't merge parallel list when calling mergeMap() in CreateThreeWayMergePatch()
+	// which is called client-side.
+	// We merge parallel list iff when calling mergeMap() in StrategicMergeMapPatch()
+	// which is called server-side
+	MergeParallelList bool
 	// IgnoreUnmatchedNulls indicates if we should process the unmatched nulls.
 	IgnoreUnmatchedNulls bool
 }
@@ -107,10 +120,7 @@ func CreateTwoWayMergeMapPatch(original, modified JSONMap, dataStruct interface{
 		return nil, err
 	}
 
-	diffOptions := DiffOptions{
+	diffOptions := DiffOptions{}
-		IgnoreChangesAndAdditions: false,
-		IgnoreDeletions:           false,
-	}
 	patchMap, err := diffMaps(original, modified, t, diffOptions)
 	if err != nil {
 		return nil, err
@@ -127,13 +137,31 @@ func CreateTwoWayMergeMapPatch(original, modified JSONMap, dataStruct interface{
 }
 
 // Returns a (recursive) strategic merge patch that yields modified when applied to original.
+// Including:
+// - Adding fields to the patch present in modified, missing from original
+// - Setting fields to the patch present in modified and original with different values
+// - Delete fields present in original, missing from modified through
+// - IFF map field - set to nil in patch
+// - IFF list of maps && merge strategy - use deleteDirective for the elements
+// - IFF list of primitives && merge strategy - use parallel deletion list
+// - IFF list of maps or primitives with replace strategy (default) - set patch value to the value in modified
+// - Build $retainKeys directive for fields with retainKeys patch strategy
 func diffMaps(original, modified map[string]interface{}, t reflect.Type, diffOptions DiffOptions) (map[string]interface{}, error) {
 	patch := map[string]interface{}{}
+	// Get the underlying type for pointers
 	if t.Kind() == reflect.Ptr {
 		t = t.Elem()
 	}
+	// This will be used to build the $retainKeys directive sent in the patch
+	retainKeysList := make([]interface{}, 0, len(modified))
 
+	// Compare each value in the modified map against the value in the original map
 	for key, modifiedValue := range modified {
+		// Get the underlying type for pointers
+		if diffOptions.BuildRetainKeysDirective && modifiedValue != nil {
+			retainKeysList = append(retainKeysList, key)
+		}
+
 		originalValue, ok := original[key]
 		if !ok {
 			// Key was added, so add to patch
@@ -144,6 +172,7 @@ func diffMaps(original, modified map[string]interface{}, t reflect.Type, diffOpt
 		}
 
 		// The patch may have a patch directive
+		// TODO: figure out if we need this. This shouldn't be needed by apply. When would the original map have patch directives in it?
 		foundDirectiveMarker, err := handleDirectiveMarker(key, originalValue, modifiedValue, patch)
 		if err != nil {
 			return nil, err
@@ -177,6 +206,14 @@ func diffMaps(original, modified map[string]interface{}, t reflect.Type, diffOpt
 	}
 
 	updatePatchIfMissing(original, modified, patch, diffOptions)
+	// Insert the retainKeysList iff there are values present in the retainKeysList and
+	// either of the following is true:
+	// - the patch is not empty
+	// - there are additional field in original that need to be cleared
+	if len(retainKeysList) > 0 &&
+		(len(patch) > 0 || hasAdditionalNewField(original, modified)) {
+		patch[retainKeysDirective] = sortScalars(retainKeysList)
+	}
 	return patch, nil
 }
 
@@ -207,7 +244,7 @@ func handleDirectiveMarker(key string, originalValue, modifiedValue interface{},
 // diffOptions contains multiple options to control how we do the diff.
 func handleMapDiff(key string, originalValue, modifiedValue, patch map[string]interface{},
 	t reflect.Type, diffOptions DiffOptions) error {
-	fieldType, fieldPatchStrategy, _, err := forkedjson.LookupPatchMetadata(t, key)
+	fieldType, fieldPatchStrategies, _, err := forkedjson.LookupPatchMetadata(t, key)
 	if err != nil {
 		// We couldn't look up metadata for the field
 		// If the values are identical, this doesn't matter, no patch is needed
@@ -217,7 +254,12 @@ func handleMapDiff(key string, originalValue, modifiedValue, patch map[string]in
 		// Otherwise, return the error
 		return err
 	}
-	switch fieldPatchStrategy {
+	retainKeys, patchStrategy, err := extractRetainKeysPatchStrategy(fieldPatchStrategies)
+	if err != nil {
+		return err
+	}
+	diffOptions.BuildRetainKeysDirective = retainKeys
+	switch patchStrategy {
 	// The patch strategic from metadata tells us to replace the entire object instead of diffing it
 	case replaceDirective:
 		if !diffOptions.IgnoreChangesAndAdditions {
@@ -244,7 +286,7 @@ func handleMapDiff(key string, originalValue, modifiedValue, patch map[string]in
 // diffOptions contains multiple options to control how we do the diff.
 func handleSliceDiff(key string, originalValue, modifiedValue []interface{}, patch map[string]interface{},
 	t reflect.Type, diffOptions DiffOptions) error {
-	fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, key)
+	fieldType, fieldPatchStrategies, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, key)
 	if err != nil {
 		// We couldn't look up metadata for the field
 		// If the values are identical, this doesn't matter, no patch is needed
@@ -254,10 +296,14 @@ func handleSliceDiff(key string, originalValue, modifiedValue []interface{}, pat
 		// Otherwise, return the error
 		return err
 	}
-
+	retainKeys, patchStrategy, err := extractRetainKeysPatchStrategy(fieldPatchStrategies)
-	switch fieldPatchStrategy {
+	if err != nil {
+		return err
+	}
+	switch patchStrategy {
 	// Merge the 2 slices using mergePatchKey
 	case mergeDirective:
+		diffOptions.BuildRetainKeysDirective = retainKeys
 		addList, deletionList, err := diffLists(originalValue, modifiedValue, fieldType.Elem(), fieldPatchMergeKey, diffOptions)
 		if err != nil {
 			return err
@@ -559,7 +605,7 @@ func StrategicMergeMapPatch(original, patch JSONMap, dataStruct interface{}) (JS
 		return nil, err
 	}
 	mergeOptions := MergeOptions{
-		MergeDeleteList:      true,
+		MergeParallelList:    true,
 		IgnoreUnmatchedNulls: true,
 	}
 	return mergeMap(original, patch, t, mergeOptions)
@@ -571,6 +617,7 @@ func getTagStructType(dataStruct interface{}) (reflect.Type, error) {
 	}
 
 	t := reflect.TypeOf(dataStruct)
+	// Get the underlying type for pointers
 	if t.Kind() == reflect.Ptr {
 		t = t.Elem()
 	}
@@ -622,10 +669,69 @@ func preprocessDeletionListForMerging(key string, original map[string]interface{
 	return false, false, "", nil
 }
 
+// applyRetainKeysDirective looks for a retainKeys directive and applies to original
+// - if no directive exists do nothing
+// - if directive is found, clear keys in original missing from the directive list
+// - validate that all keys present in the patch are present in the retainKeys directive
+// note: original may be another patch request, e.g. applying the add+modified patch to the deletions patch. In this case it may have directives
+func applyRetainKeysDirective(original, patch map[string]interface{}, options MergeOptions) error {
+	retainKeysInPatch, foundInPatch := patch[retainKeysDirective]
+	if !foundInPatch {
+		return nil
+	}
+	// cleanup the directive
+	delete(patch, retainKeysDirective)
+
+	if !options.MergeParallelList {
+		// If original is actually a patch, make sure the retainKeys directives are the same in both patches if present in both.
+		// If not present in the original patch, copy from the modified patch.
+		retainKeysInOriginal, foundInOriginal := original[retainKeysDirective]
+		if foundInOriginal {
+			if !reflect.DeepEqual(retainKeysInOriginal, retainKeysInPatch) {
+				// This error actually should never happen.
+				return fmt.Errorf("%v and %v are not deep equal: this may happen when calculating the 3-way diff patch", retainKeysInOriginal, retainKeysInPatch)
+			}
+		} else {
+			original[retainKeysDirective] = retainKeysInPatch
+		}
+		return nil
+	}
+
+	retainKeysList, ok := retainKeysInPatch.([]interface{})
+	if !ok {
+		return mergepatch.ErrBadPatchFormatForRetainKeys
+	}
+
+	// validate patch to make sure all fields in the patch are present in the retainKeysList.
+	// The map is used only as a set, the value is never referenced
+	m := map[interface{}]struct{}{}
+	for _, v := range retainKeysList {
+		m[v] = struct{}{}
+	}
+	for k, v := range patch {
+		if v == nil || strings.HasPrefix(k, deleteFromPrimitiveListDirectivePrefix) {
+			continue
+		}
+		// If there is an item present in the patch but not in the retainKeys list,
+		// the patch is invalid.
+		if _, found := m[k]; !found {
+			return mergepatch.ErrBadPatchFormatForRetainKeys
+		}
+	}
+
+	// clear not present fields
+	for k := range original {
+		if _, found := m[k]; !found {
+			delete(original, k)
+		}
+	}
+	return nil
+}
+
 // Merge fields from a patch map into the original map. Note: This may modify
 // both the original map and the patch because getting a deep copy of a map in
 // golang is highly non-trivial.
-// flag mergeOptions.MergeDeleteList controls if using the parallel list to delete or keeping the list.
+// flag mergeOptions.MergeParallelList controls if using the parallel list to delete or keeping the list.
 // If patch contains any null field (e.g. field_1: null) that is not
 // present in original, then to propagate it to the end result use
 // mergeOptions.IgnoreUnmatchedNulls == false.
@@ -640,9 +746,14 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeOptio
 		original = map[string]interface{}{}
 	}
 
+	err := applyRetainKeysDirective(original, patch, mergeOptions)
+	if err != nil {
+		return nil, err
+	}
+
 	// Start merging the patch into the original.
 	for k, patchV := range patch {
-		skipProcessing, isDeleteList, noPrefixKey, err := preprocessDeletionListForMerging(k, original, patchV, mergeOptions.MergeDeleteList)
+		skipProcessing, isDeleteList, noPrefixKey, err := preprocessDeletionListForMerging(k, original, patchV, mergeOptions.MergeParallelList)
 		if err != nil {
 			return nil, err
 		}
@@ -686,16 +797,21 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeOptio
 		}
 		// If they're both maps or lists, recurse into the value.
 		// First find the fieldPatchStrategy and fieldPatchMergeKey.
-		fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
+		fieldType, fieldPatchStrategies, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
 		if err != nil {
 			return nil, err
 		}
+		_, patchStrategy, err := extractRetainKeysPatchStrategy(fieldPatchStrategies)
+		if err != nil {
+			return nil, err
+		}
+
 		switch originalType.Kind() {
 		case reflect.Map:
 
-			original[k], err = mergeMapHandler(original[k], patchV, fieldType, fieldPatchStrategy, mergeOptions)
+			original[k], err = mergeMapHandler(original[k], patchV, fieldType, patchStrategy, mergeOptions)
 		case reflect.Slice:
-			original[k], err = mergeSliceHandler(original[k], patchV, fieldType, fieldPatchStrategy, fieldPatchMergeKey, isDeleteList, mergeOptions)
+			original[k], err = mergeSliceHandler(original[k], patchV, fieldType, patchStrategy, fieldPatchMergeKey, isDeleteList, mergeOptions)
 		default:
 			original[k] = patchV
 		}
@@ -755,7 +871,7 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 
 	// If the elements are not maps, merge the slices of scalars.
 	if t.Kind() != reflect.Map {
-		if mergeOptions.MergeDeleteList && isDeleteList {
+		if mergeOptions.MergeParallelList && isDeleteList {
 			return deleteFromSlice(original, patch), nil
 		}
 		// Maybe in the future add a "concat" mode that doesn't
@@ -935,14 +1051,24 @@ func sortMergeListsByName(mapJSON []byte, dataStruct interface{}) ([]byte, error
 func sortMergeListsByNameMap(s map[string]interface{}, t reflect.Type) (map[string]interface{}, error) {
 	newS := map[string]interface{}{}
 	for k, v := range s {
-		if strings.HasPrefix(k, deleteFromPrimitiveListDirectivePrefix) {
+		if k == retainKeysDirective {
+			typedV, ok := v.([]interface{})
+			if !ok {
+				return nil, mergepatch.ErrBadPatchFormatForRetainKeys
+			}
+			v = sortScalars(typedV)
+		} else if strings.HasPrefix(k, deleteFromPrimitiveListDirectivePrefix) {
 			typedV, ok := v.([]interface{})
 			if !ok {
 				return nil, mergepatch.ErrBadPatchFormatForPrimitiveList
 			}
 			v = uniqifyAndSortScalars(typedV)
 		} else if k != directiveMarker {
-			fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
+			fieldType, fieldPatchStrategies, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
+			if err != nil {
+				return nil, err
+			}
+			_, patchStrategy, err := extractRetainKeysPatchStrategy(fieldPatchStrategies)
 			if err != nil {
 				return nil, err
 			}
@@ -955,7 +1081,7 @@ func sortMergeListsByNameMap(s map[string]interface{}, t reflect.Type) (map[stri
 					return nil, err
 				}
 			} else if typedV, ok := v.([]interface{}); ok {
-				if fieldPatchStrategy == mergeDirective {
+				if patchStrategy == mergeDirective {
 					var err error
 					v, err = sortMergeListsByNameArray(typedV, fieldType.Elem(), fieldPatchMergeKey, true)
 					if err != nil {
@@ -1201,15 +1327,19 @@ func mergingMapFieldsHaveConflicts(
 
 func mapsHaveConflicts(typedLeft, typedRight map[string]interface{}, structType reflect.Type) (bool, error) {
 	for key, leftValue := range typedLeft {
-		if key != directiveMarker {
+		if key != directiveMarker && key != retainKeysDirective {
 			if rightValue, ok := typedRight[key]; ok {
-				fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(structType, key)
+				fieldType, fieldPatchStrategies, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(structType, key)
+				if err != nil {
+					return true, err
+				}
+				_, patchStrategy, err := extractRetainKeysPatchStrategy(fieldPatchStrategies)
 				if err != nil {
 					return true, err
 				}
 
 				if hasConflicts, err := mergingMapFieldsHaveConflicts(leftValue, rightValue,
-					fieldType, fieldPatchStrategy, fieldPatchMergeKey); hasConflicts {
+					fieldType, patchStrategy, fieldPatchMergeKey); hasConflicts {
 					return true, err
 				}
 			}
@@ -1348,7 +1478,6 @@ func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct int
 	// original to modified, and delta, which is the difference from current to modified without
 	// deletions, and then apply delta to deletions as a patch, which should be strictly additive.
 	deltaMapDiffOptions := DiffOptions{
-		IgnoreChangesAndAdditions: false,
 		IgnoreDeletions: true,
 	}
 	deltaMap, err := diffMaps(currentMap, modifiedMap, t, deltaMapDiffOptions)
@@ -1357,17 +1486,13 @@ func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct int
 	}
 	deletionsMapDiffOptions := DiffOptions{
 		IgnoreChangesAndAdditions: true,
-		IgnoreDeletions:           false,
 	}
 	deletionsMap, err := diffMaps(originalMap, modifiedMap, t, deletionsMapDiffOptions)
 	if err != nil {
 		return nil, err
 	}
 
-	mergeOptions := MergeOptions{
+	mergeOptions := MergeOptions{}
-		MergeDeleteList:      false,
-		IgnoreUnmatchedNulls: false,
-	}
 	patchMap, err := mergeMap(deletionsMap, deltaMap, t, mergeOptions)
 	if err != nil {
 		return nil, err
@@ -1383,10 +1508,7 @@ func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct int
 	// If overwrite is false, and the patch contains any keys that were changed differently,
 	// then return a conflict error.
 	if !overwrite {
-		changeMapDiffOptions := DiffOptions{
+		changeMapDiffOptions := DiffOptions{}
-			IgnoreChangesAndAdditions: false,
-			IgnoreDeletions:           false,
-		}
 		changedMap, err := diffMaps(originalMap, currentMap, t, changeMapDiffOptions)
 		if err != nil {
 			return nil, err
@@ -1438,3 +1560,45 @@ func sliceTypeAssertion(original, patch interface{}) ([]interface{}, []interface
 	}
 	return typedOriginal, typedPatch, nil
 }
+
+// extractRetainKeysPatchStrategy process patch strategy, which is a string may contains multiple
+// patch strategies seperated by ",". It returns a boolean var indicating if it has
+// retainKeys strategies and a string for the other strategy.
+func extractRetainKeysPatchStrategy(strategies []string) (bool, string, error) {
+	switch len(strategies) {
+	case 0:
+		return false, "", nil
+	case 1:
+		singleStrategy := strategies[0]
+		switch singleStrategy {
+		case retainKeysStrategy:
+			return true, "", nil
+		default:
+			return false, singleStrategy, nil
+		}
+	case 2:
+		switch {
+		case strategies[0] == retainKeysStrategy:
+			return true, strategies[1], nil
+		case strategies[1] == retainKeysStrategy:
+			return true, strategies[0], nil
+		default:
+			return false, "", fmt.Errorf("unexpected patch strategy: %v", strategies)
+		}
+	default:
+		return false, "", fmt.Errorf("unexpected patch strategy: %v", strategies)
+	}
+}
+
+// hasAdditionalNewField returns if original map has additional key with non-nil value than modified.
+func hasAdditionalNewField(original, modified map[string]interface{}) bool {
+	for k, v := range original {
+		if v == nil {
+			continue
+		}
+		if _, found := modified[k]; !found {
+			return true
+		}
+	}
+	return false
+}

staging/src/k8s.io/apimachinery/third_party/forked/golang/json/fields.go

@@ -17,16 +17,25 @@ import (
 	"unicode/utf8"
 )
 
+const (
+	patchStrategyTagKey = "patchStrategy"
+	patchMergeKeyTagKey = "patchMergeKey"
+)
+
 // Finds the patchStrategy and patchMergeKey struct tag fields on a given
 // struct field given the struct type and the JSON name of the field.
+// It returns field type, a slice of patch strategies, merge key and error.
 // TODO: fix the returned errors to be introspectable.
-func LookupPatchMetadata(t reflect.Type, jsonField string) (reflect.Type, string, string, error) {
+func LookupPatchMetadata(t reflect.Type, jsonField string) (
+	elemType reflect.Type, patchStrategies []string, patchMergeKey string, e error) {
 	if t.Kind() == reflect.Map {
-		return t.Elem(), "", "", nil
+		elemType = t.Elem()
+		return
 	}
 	if t.Kind() != reflect.Struct {
-		return nil, "", "", fmt.Errorf("merging an object in json but data type is not map or struct, instead is: %s",
+		e = fmt.Errorf("merging an object in json but data type is not map or struct, instead is: %s",
 			t.Kind().String())
+		return
 	}
 	jf := []byte(jsonField)
 	// Find the field that the JSON library would use.
@@ -50,11 +59,14 @@ func LookupPatchMetadata(t reflect.Type, jsonField string) (reflect.Type, string
 		for i := 1; i < len(f.index); i++ {
 			tjf = tjf.Type.Field(f.index[i])
 		}
-		patchStrategy := tjf.Tag.Get("patchStrategy")
+		patchStrategy := tjf.Tag.Get(patchStrategyTagKey)
-		patchMergeKey := tjf.Tag.Get("patchMergeKey")
+		patchMergeKey = tjf.Tag.Get(patchMergeKeyTagKey)
-		return tjf.Type, patchStrategy, patchMergeKey, nil
+		patchStrategies = strings.Split(patchStrategy, ",")
+		elemType = tjf.Type
+		return
 	}
-	return nil, "", "", fmt.Errorf("unable to find api field in struct %s for the json field %q", t.Name(), jsonField)
+	e = fmt.Errorf("unable to find api field in struct %s for the json field %q", t.Name(), jsonField)
+	return
 }
 
 // A field represents a single field found in a struct.