diff --git a/pkg/kubectl/cmd/apply_test.go b/pkg/kubectl/cmd/apply_test.go index 3b35282312..283266f538 100644 --- a/pkg/kubectl/cmd/apply_test.go +++ b/pkg/kubectl/cmd/apply_test.go @@ -1021,8 +1021,6 @@ func TestUnstructuredIdempotentApply(t *testing.T) { } path := "/namespaces/test/widgets/widget" - verifiedPatch := false - for _, fn := range testingOpenAPISchemaFns { t.Run("test repeated apply operations on an unstructured object", func(t *testing.T) { tf := cmdtesting.NewTestFactory() @@ -1039,41 +1037,15 @@ func TestUnstructuredIdempotentApply(t *testing.T) { Header: defaultHeader(), Body: body}, nil case p == path && m == "PATCH": - // In idempotent updates, kubectl sends a logically empty - // request body with the PATCH request. - // Should look like this: - // Request Body: {"metadata":{"annotations":{}}} + // In idempotent updates, kubectl will resolve to an empty patch and not send anything to the server + // Thus, if we reach this branch, kubectl is unnecessarily sending a patch. patch, err := ioutil.ReadAll(req.Body) if err != nil { t.Fatal(err) } - - contentType := req.Header.Get("Content-Type") - if contentType != "application/merge-patch+json" { - t.Fatalf("Unexpected Content-Type: %s", contentType) - } - - patchMap := map[string]interface{}{} - if err := json.Unmarshal(patch, &patchMap); err != nil { - t.Fatal(err) - } - if len(patchMap) != 1 { - t.Fatalf("Unexpected Patch. Has more than 1 entry. path: %s", patch) - } - - annotationsMap := walkMapPath(t, patchMap, []string{"metadata", "annotations"}) - if len(annotationsMap) != 0 { - t.Fatalf("Unexpected Patch. Found unexpected annotation: %s", patch) - } - - verifiedPatch = true - - body := ioutil.NopCloser(bytes.NewReader(serversideData)) - return &http.Response{ - StatusCode: 200, - Header: defaultHeader(), - Body: body}, nil + t.Fatalf("Unexpected Patch: %s", patch) + return nil, nil default: t.Fatalf("unexpected request: %#v\n%#v", req.URL, req) return nil, nil @@ -1096,9 +1068,6 @@ func TestUnstructuredIdempotentApply(t *testing.T) { if errBuf.String() != "" { t.Fatalf("unexpected error output: %s", errBuf.String()) } - if !verifiedPatch { - t.Fatal("No server-side patch call detected") - } }) } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go b/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go index e81e4f23a8..82e4b4b570 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch.go @@ -116,10 +116,26 @@ func keepOrDeleteNullInObj(m map[string]interface{}, keepNull bool) (map[string] case val != nil: switch typedVal := val.(type) { case map[string]interface{}: - filteredMap[key], err = keepOrDeleteNullInObj(typedVal, keepNull) + // Explicitly-set empty maps are treated as values instead of empty patches + if len(typedVal) == 0 { + if !keepNull { + filteredMap[key] = typedVal + } + continue + } + + var filteredSubMap map[string]interface{} + filteredSubMap, err = keepOrDeleteNullInObj(typedVal, keepNull) if err != nil { return nil, err } + + // If the returned filtered submap was empty, this is an empty patch for the entire subdict, so the key + // should not be set + if len(filteredSubMap) != 0 { + filteredMap[key] = filteredSubMap + } + case []interface{}, string, float64, bool, int, int64, nil: // Lists are always replaced in Json, no need to check each entry in the list. if !keepNull { diff --git a/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch_test.go b/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch_test.go index f462bf915f..9672deaad6 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/jsonmergepatch/patch_test.go @@ -62,12 +62,12 @@ testCases: expectedWithoutNull: {} - description: simple map with all non-nil values originalObj: - nonNilKey: foo - nonNilKey: bar + nonNilKey1: foo + nonNilKey2: bar expectedWithNull: {} expectedWithoutNull: - nonNilKey: foo - nonNilKey: bar + nonNilKey1: foo + nonNilKey2: bar - description: nested map originalObj: mapKey: @@ -88,19 +88,52 @@ testCases: mapKey: nilKey1: null nilKey2: null - expectedWithoutNull: - mapKey: {} + expectedWithoutNull: {} - description: nested map that all subkeys are non-nil originalObj: mapKey: - nonNilKey: foo - nonNilKey: bar - expectedWithNull: - mapKey: {} + nonNilKey1: foo + nonNilKey2: bar + expectedWithNull: {} expectedWithoutNull: mapKey: - nonNilKey: foo - nonNilKey: bar + nonNilKey1: foo + nonNilKey2: bar + - description: explicitly empty map as value + originalObj: + mapKey: {} + expectedWithNull: {} + expectedWithoutNull: + mapKey: {} + - description: explicitly empty nested map + originalObj: + mapKey: + nonNilKey: {} + expectedWithNull: {} + expectedWithoutNull: + mapKey: + nonNilKey: {} + - description: multiple expliclty empty nested maps + originalObj: + mapKey: + nonNilKey1: {} + nonNilKey2: {} + expectedWithNull: {} + expectedWithoutNull: + mapKey: + nonNilKey1: {} + nonNilKey2: {} + - description: nested map with non-null value as empty map + originalObj: + mapKey: + nonNilKey: {} + nilKey: null + expectedWithNull: + mapKey: + nilKey: null + expectedWithoutNull: + mapKey: + nonNilKey: {} - description: empty list originalObj: listKey: []