Merge pull request #59284 from Addepar/fix-empty-null-patch

Automatic merge from submit-queue (batch tested with PRs 59284, 63602). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Exclude keys containing empty patches in the final patch

**What this PR does / why we need it**: 
This minimizes the 3-way JSON merge patch generated when calculating the patch necessary to send to the server. It does this by removing empty maps created from deleting keys in the keepOrDeleteNullInObj method.

This is not only a slight performance improvement (less PATCH requests) but also necessary when working with custom resources that have RBAC restrictions.

**Which issue(s) this PR fixes**: N/A

**Special notes for your reviewer**: N/A

**Release note**:

```release-note
NONE
```

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
+						// 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
@@ -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")
-			}
 		})
 	}
 }

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: []