Exclude keys containing empty patches in the final patch

pkg/kubectl/cmd/apply_test.go

@@ -977,8 +977,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()
@@ -995,41 +993,15 @@ func TestUnstructuredIdempotentApply(t *testing.T) {
 							Header:     defaultHeader(),
 							Body:       body}, nil
 					case p == path && m == "PATCH":
-						// In idempotent updates, kubectl sends a logically empty
+						// In idempotent updates, kubectl will resolve to an empty patch and not send anything to the server
-						// request body with the PATCH request.
+						// Thus, if we reach this branch, kubectl is unnecessarily sending a patch.
-						// Should look like this:
-						// Request Body: {"metadata":{"annotations":{}}}
 
 						patch, err := ioutil.ReadAll(req.Body)
 						if err != nil {
 							t.Fatal(err)
 						}
-
+						t.Fatalf("Unexpected Patch: %s", patch)
-						contentType := req.Header.Get("Content-Type")
+						return nil, nil
-						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
 					default:
 						t.Fatalf("unexpected request: %#v\n%#v", req.URL, req)
 						return nil, nil
@@ -1053,9 +1025,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")
-			}
 		})
 	}
 }

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 {

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
+      nonNilKey1: foo
-      nonNilKey: bar
+      nonNilKey2: bar
     expectedWithNull: {}
     expectedWithoutNull:
-      nonNilKey: foo
+      nonNilKey1: foo
-      nonNilKey: bar
+      nonNilKey2: bar
   - description: nested map
     originalObj:
       mapKey:
@@ -88,19 +88,52 @@ testCases:
       mapKey:
         nilKey1: null
         nilKey2: null
-    expectedWithoutNull:
+    expectedWithoutNull: {}
-      mapKey: {}
   - description: nested map that all subkeys are non-nil
     originalObj:
       mapKey:
-        nonNilKey: foo
+        nonNilKey1: foo
-        nonNilKey: bar
+        nonNilKey2: bar
-    expectedWithNull:
+    expectedWithNull: {}
-      mapKey: {}
     expectedWithoutNull:
       mapKey:
-        nonNilKey: foo
+        nonNilKey1: foo
-        nonNilKey: bar
+        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: []