Merge pull request #40630 from liggitt/apply-null

Automatic merge from submit-queue (batch tested with PRs 40529, 40630)

propagate explicit nulls in apply

Rebase of https://github.com/kubernetes/kubernetes/pull/35496 on top of https://github.com/kubernetes/kubernetes/pull/40260

The client-side propagation of the raw value is no longer needed, since the client is preserving the original object in unstructured form (explicit nulls are preserved).

Kept tests and CreateThreeWayMergePatch changes from https://github.com/kubernetes/kubernetes/pull/35496

```release-note
kubectl apply now supports explicitly clearing values not present in the config by setting them to null
```

- [x] Clean up orphaned objects in test-cmd to preserve pre- and post- conditions
- [x] improve CreateThreeWayMergePatch test to not filter based on string comparison to test name

hack/make-rules/test-cmd-util.sh

@@ -917,6 +917,40 @@ run_kubectl_create_filter_tests() {
   kubectl delete pods selector-test-pod
 }
 
+run_kubectl_apply_deployments_tests() {
+  ## kubectl apply should propagate user defined null values
+  # Pre-Condition: no Deployments, ReplicaSets, Pods exist
+  kube::test::get_object_assert deployments "{{range.items}}{{$id_field}}:{{end}}" ''
+  kube::test::get_object_assert replicasets "{{range.items}}{{$id_field}}:{{end}}" ''
+  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" ''
+  # apply base deployment
+  kubectl apply -f hack/testdata/null-propagation/deployment-l1.yaml "${kube_flags[@]}"
+  # check right deployment exists
+  kube::test::get_object_assert 'deployments my-depl' "{{${id_field}}}" 'my-depl'
+  # check right labels exists
+  kube::test::get_object_assert 'deployments my-depl' "{{.spec.template.metadata.labels.l1}}" 'l1'
+  kube::test::get_object_assert 'deployments my-depl' "{{.spec.selector.matchLabels.l1}}" 'l1'
+  kube::test::get_object_assert 'deployments my-depl' "{{.metadata.labels.l1}}" 'l1'
+
+  # apply new deployment with new template labels
+  kubectl apply -f hack/testdata/null-propagation/deployment-l2.yaml "${kube_flags[@]}"
+  # check right labels exists
+  kube::test::get_object_assert 'deployments my-depl' "{{.spec.template.metadata.labels.l1}}" '<no value>'
+  kube::test::get_object_assert 'deployments my-depl' "{{.spec.selector.matchLabels.l1}}" '<no value>'
+  kube::test::get_object_assert 'deployments my-depl' "{{.metadata.labels.l1}}" '<no value>'
+  kube::test::get_object_assert 'deployments my-depl' "{{.spec.template.metadata.labels.l2}}" 'l2'
+  kube::test::get_object_assert 'deployments my-depl' "{{.spec.selector.matchLabels.l2}}" 'l2'
+  kube::test::get_object_assert 'deployments my-depl' "{{.metadata.labels.l2}}" 'l2'
+
+  # cleanup
+  # need to explicitly remove replicasets and pods because we changed the deployment selector and orphaned things
+  kubectl delete deployments,rs,pods --all --cascade=false --grace-period=0
+  # Post-Condition: no Deployments, ReplicaSets, Pods exist
+  kube::test::get_object_assert deployments "{{range.items}}{{$id_field}}:{{end}}" ''
+  kube::test::get_object_assert replicasets "{{range.items}}{{$id_field}}:{{end}}" ''
+  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" ''
+}
+
 # Runs tests for --save-config tests.
 run_save_config_tests() {
   ## Configuration annotations should be set when --save-config is enabled
@@ -2718,6 +2752,10 @@ runTests() {
     run_kubectl_create_filter_tests
   fi
 
+  if kube::test::if_supports_resource "${deployments}" ; then
+    run_kubectl_apply_deployments_tests
+  fi
+
   ###############
   # Kubectl get #
   ###############

hack/testdata/null-propagation/deployment-l1.yaml

@@ -0,0 +1,13 @@
+apiVersion: extensions/v1beta1
+kind: Deployment
+metadata:
+  name: my-depl
+spec:
+  template:
+    metadata:
+      labels:
+        l1: l1
+    spec:
+      containers:
+      - name: nginx
+        image: gcr.io/google-containers/nginx:1.7.9

hack/testdata/null-propagation/deployment-l2.yaml

@@ -0,0 +1,17 @@
+apiVersion: extensions/v1beta1
+kind: Deployment
+metadata:
+  name: my-depl
+  # We expect this field to be defaulted to the new label l2
+  labels: null
+spec:
+  # We expect this field to be defaulted to the new label l2
+  selector: null
+  template:
+    metadata:
+      labels:
+        l2: l2
+    spec:
+      containers:
+      - name: nginx
+        image: gcr.io/google-containers/nginx:1.7.9

pkg/kubectl/cmd/apply_test.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/annotations"
 	"k8s.io/kubernetes/pkg/api/testapi"
+	"k8s.io/kubernetes/pkg/apis/extensions"
 	cmdtesting "k8s.io/kubernetes/pkg/kubectl/cmd/testing"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 )
@@ -443,3 +444,90 @@ func testApplyMultipleObjects(t *testing.T, asList bool) {
 		t.Fatalf("unexpected output: %s\nexpected: %s OR %s", buf.String(), expectOne, expectTwo)
 	}
 }
+
+const (
+	filenameDeployObjServerside = "../../../test/fixtures/pkg/kubectl/cmd/apply/deploy-serverside.yaml"
+	filenameDeployObjClientside = "../../../test/fixtures/pkg/kubectl/cmd/apply/deploy-clientside.yaml"
+)
+
+func readDeploymentFromFile(t *testing.T, file string) []byte {
+	raw := readBytesFromFile(t, file)
+	obj := &extensions.Deployment{}
+	if err := runtime.DecodeInto(testapi.Extensions.Codec(), raw, obj); err != nil {
+		t.Fatal(err)
+	}
+	objJSON, err := runtime.Encode(testapi.Extensions.Codec(), obj)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return objJSON
+}
+
+func TestApplyNULLPreservation(t *testing.T) {
+	initTestErrorHandler(t)
+	deploymentName := "nginx-deployment"
+	deploymentPath := "/namespaces/test/deployments/" + deploymentName
+
+	verifiedPatch := false
+	deploymentBytes := readDeploymentFromFile(t, filenameDeployObjServerside)
+
+	f, tf, _, _ := cmdtesting.NewTestFactory()
+	tf.UnstructuredClient = &fake.RESTClient{
+		APIRegistry:          api.Registry,
+		NegotiatedSerializer: unstructuredSerializer,
+		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+			switch p, m := req.URL.Path, req.Method; {
+			case p == deploymentPath && m == "GET":
+				body := ioutil.NopCloser(bytes.NewReader(deploymentBytes))
+				return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: body}, nil
+			case p == deploymentPath && m == "PATCH":
+				patch, err := ioutil.ReadAll(req.Body)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				patchMap := map[string]interface{}{}
+				if err := json.Unmarshal(patch, &patchMap); err != nil {
+					t.Fatal(err)
+				}
+				annotationMap := walkMapPath(t, patchMap, []string{"metadata", "annotations"})
+				if _, ok := annotationMap[annotations.LastAppliedConfigAnnotation]; !ok {
+					t.Fatalf("patch does not contain annotation:\n%s\n", patch)
+				}
+				strategy := walkMapPath(t, patchMap, []string{"spec", "strategy"})
+				if value, ok := strategy["rollingUpdate"]; !ok || value != nil {
+					t.Fatalf("patch did not retain null value in key: rollingUpdate:\n%s\n", patch)
+				}
+				verifiedPatch = true
+
+				// The real API server would had returned the patched object but Kubectl
+				// is ignoring the actual return object.
+				// TODO: Make this match actual server behavior by returning the patched object.
+				body := ioutil.NopCloser(bytes.NewReader(deploymentBytes))
+				return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: body}, nil
+			default:
+				t.Fatalf("unexpected request: %#v\n%#v", req.URL, req)
+				return nil, nil
+			}
+		}),
+	}
+	tf.Namespace = "test"
+	tf.ClientConfig = defaultClientConfig()
+	buf := bytes.NewBuffer([]byte{})
+	errBuf := bytes.NewBuffer([]byte{})
+
+	cmd := NewCmdApply(f, buf, errBuf)
+	cmd.Flags().Set("filename", filenameDeployObjClientside)
+	cmd.Flags().Set("output", "name")
+
+	cmd.Run(cmd, []string{})
+
+	expected := "deployment/" + deploymentName + "\n"
+	if buf.String() != expected {
+		t.Fatalf("unexpected output: %s\nexpected: %s", buf.String(), expected)
+	}
+
+	if !verifiedPatch {
+		t.Fatal("No server-side patch call detected")
+	}
+}

pkg/kubectl/cmd/testing/fake.go

@@ -669,5 +669,19 @@ func testDynamicResources() []*discovery.APIGroupResources {
 				},
 			},
 		},
+		{
+			Group: metav1.APIGroup{
+				Name: "extensions",
+				Versions: []metav1.GroupVersionForDiscovery{
+					{Version: "v1beta1"},
+				},
+				PreferredVersion: metav1.GroupVersionForDiscovery{Version: "v1beta1"},
+			},
+			VersionedResources: map[string][]metav1.APIResource{
+				"v1beta1": {
+					{Name: "deployments", Namespaced: true, Kind: "Deployment"},
+				},
+			},
+		},
 	}
 }

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

@@ -548,7 +548,7 @@ func StrategicMergeMapPatch(original, patch JSONMap, dataStruct interface{}) (JS
 	if err != nil {
 		return nil, err
 	}
-	return mergeMap(original, patch, t, true)
+	return mergeMap(original, patch, t, true, true)
 }
 
 func getTagStructType(dataStruct interface{}) (reflect.Type, error) {
@@ -574,7 +574,10 @@ var errBadPatchTypeFmt = "unknown patch type: %s in map: %v"
 // both the original map and the patch because getting a deep copy of a map in
 // golang is highly non-trivial.
 // flag mergeDeleteList controls if using the parallel list to delete or keeping the list.
-func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDeleteList bool) (map[string]interface{}, error) {
+// 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
+// ignoreUnmatchedNulls == false.
+func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDeleteList, ignoreUnmatchedNulls bool) (map[string]interface{}, error) {
 	if v, ok := patch[directiveMarker]; ok {
 		if v == replaceDirective {
 			// If the patch contains "$patch: replace", don't merge it, just use the
@@ -619,14 +622,18 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet
 		}
 
 		// If the value of this key is null, delete the key if it exists in the
-		// original. Otherwise, skip it.
+		// original. Otherwise, check if we want to preserve it or skip it.
+		// Preserving the null value is useful when we want to send an explicit
+		// delete to the API server.
 		if patchV == nil {
 			if _, ok := original[k]; ok {
 				delete(original, k)
 			}
 
+			if ignoreUnmatchedNulls {
 				continue
 			}
+		}
 
 		_, ok := original[k]
 		if !ok {
@@ -654,7 +661,7 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet
 				typedOriginal := original[k].(map[string]interface{})
 				typedPatch := patchV.(map[string]interface{})
 				var err error
-				original[k], err = mergeMap(typedOriginal, typedPatch, fieldType, mergeDeleteList)
+				original[k], err = mergeMap(typedOriginal, typedPatch, fieldType, mergeDeleteList, ignoreUnmatchedNulls)
 				if err != nil {
 					return nil, err
 				}
@@ -667,7 +674,7 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet
 				typedOriginal := original[k].([]interface{})
 				typedPatch := patchV.([]interface{})
 				var err error
-				original[k], err = mergeSlice(typedOriginal, typedPatch, elemType, fieldPatchMergeKey, mergeDeleteList, isDeleteList)
+				original[k], err = mergeSlice(typedOriginal, typedPatch, elemType, fieldPatchMergeKey, mergeDeleteList, isDeleteList, ignoreUnmatchedNulls)
 				if err != nil {
 					return nil, err
 				}
@@ -688,7 +695,7 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeDelet
 // Merge two slices together. Note: This may modify both the original slice and
 // the patch because getting a deep copy of a slice in golang is highly
 // non-trivial.
-func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey string, mergeDeleteList, isDeleteList bool) ([]interface{}, error) {
+func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey string, mergeDeleteList, isDeleteList, ignoreUnmatchedNulls bool) ([]interface{}, error) {
 	if len(original) == 0 && len(patch) == 0 {
 		return original, nil
 	}
@@ -779,7 +786,7 @@ func mergeSlice(original, patch []interface{}, elemType reflect.Type, mergeKey s
 			var mergedMaps interface{}
 			var err error
 			// Merge into original.
-			mergedMaps, err = mergeMap(originalMap, typedV, elemType, mergeDeleteList)
+			mergedMaps, err = mergeMap(originalMap, typedV, elemType, mergeDeleteList, ignoreUnmatchedNulls)
 			if err != nil {
 				return nil, err
 			}
@@ -1282,7 +1289,8 @@ func mapsOfMapsHaveConflicts(typedLeft, typedRight map[string]interface{}, struc
 // configurations. Conflicts are defined as keys changed differently from original to modified
 // than from original to current. In other words, a conflict occurs if modified changes any key
 // in a way that is different from how it is changed in current (e.g., deleting it, changing its
-// value).
+// value). We also propagate values fields that do not exist in original but are explicitly
+// defined in modified.
 func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct interface{}, overwrite bool, fns ...PreconditionFunc) ([]byte, error) {
 	originalMap := map[string]interface{}{}
 	if len(original) > 0 {
@@ -1324,7 +1332,7 @@ func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct int
 		return nil, err
 	}
 
-	patchMap, err := mergeMap(deletionsMap, deltaMap, t, false)
+	patchMap, err := mergeMap(deletionsMap, deltaMap, t, false, false)
 	if err != nil {
 		return nil, err
 	}

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

@@ -64,6 +64,9 @@ type StrategicMergePatchTestCaseData struct {
 	ThreeWay map[string]interface{}
 	// Result is the expected object after applying the three-way patch on current object.
 	Result map[string]interface{}
+	// TwoWayResult is the expected object after applying the two-way patch on current object.
+	// If nil, Modified is used.
+	TwoWayResult map[string]interface{}
 }
 
 // The meaning of each field is the same as StrategicMergePatchTestCaseData's.
@@ -75,6 +78,7 @@ type StrategicMergePatchRawTestCaseData struct {
 	TwoWay       []byte
 	ThreeWay     []byte
 	Result       []byte
+	TwoWayResult []byte
 }
 
 type MergeItem struct {
@@ -360,8 +364,8 @@ func TestCustomStrategicMergePatch(t *testing.T) {
 	}
 
 	for _, c := range tc.TestCases {
-		original, twoWay, modified := twoWayTestCaseToJSONOrFail(t, c)
-		testPatchApplication(t, original, twoWay, modified, c.Description)
+		original, expectedTwoWayPatch, _, expectedResult := twoWayTestCaseToJSONOrFail(t, c)
+		testPatchApplication(t, original, expectedTwoWayPatch, expectedResult, c.Description)
 	}
 }
 
@@ -1817,6 +1821,52 @@ testCases:
               other: b
         - name: 2
           other: b
+  - description: defined null values should propagate overwrite current fields (with conflict)
+    original:
+      name: 2
+    twoWay:
+      name: 1
+      value: 1
+      other: null
+    twoWayResult:
+      name: 1
+      value: 1
+    modified:
+      name: 1
+      value: 1
+      other: null
+    current:
+      name: a
+      other: a
+    threeWay:
+      name: 1
+      value: 1
+      other: null
+    result:
+      name: 1
+      value: 1
+  - description: defined null values should propagate removing original fields
+    original:
+      name: original-name
+      value: original-value
+    current:
+      name: original-name
+      value: original-value
+      other: current-other
+    modified:
+      name: modified-name
+      value: null
+    twoWay:
+      name: modified-name
+      value: null
+    twoWayResult:
+      name: modified-name
+    threeWay:
+      name: modified-name
+      value: null
+    result:
+      name: modified-name
+      other: current-other
 `)
 
 var strategicMergePatchRawTestCases = []StrategicMergePatchRawTestCase{
@@ -1982,43 +2032,53 @@ func testStrategicMergePatchWithCustomArguments(t *testing.T, description, origi
 }
 
 func testTwoWayPatch(t *testing.T, c StrategicMergePatchTestCase) {
-	original, expected, modified := twoWayTestCaseToJSONOrFail(t, c)
+	original, expectedPatch, modified, expectedResult := twoWayTestCaseToJSONOrFail(t, c)
 
-	actual, err := CreateTwoWayMergePatch(original, modified, mergeItem)
+	actualPatch, err := CreateTwoWayMergePatch(original, modified, mergeItem)
 	if err != nil {
 		t.Errorf("error: %s\nin test case: %s\ncannot create two way patch: %s:\n%s\n",
 			err, c.Description, original, toYAMLOrError(c.StrategicMergePatchTestCaseData))
 		return
 	}
 
-	testPatchCreation(t, expected, actual, c.Description)
-	testPatchApplication(t, original, actual, modified, c.Description)
+	testPatchCreation(t, expectedPatch, actualPatch, c.Description)
+	testPatchApplication(t, original, actualPatch, expectedResult, c.Description)
 }
 
 func testTwoWayPatchForRawTestCase(t *testing.T, c StrategicMergePatchRawTestCase) {
-	original, expected, modified := twoWayRawTestCaseToJSONOrFail(t, c)
+	original, expectedPatch, modified, expectedResult := twoWayRawTestCaseToJSONOrFail(t, c)
 
-	actual, err := CreateTwoWayMergePatch(original, modified, mergeItem)
+	actualPatch, err := CreateTwoWayMergePatch(original, modified, mergeItem)
 	if err != nil {
 		t.Errorf("error: %s\nin test case: %s\ncannot create two way patch:\noriginal:%s\ntwoWay:%s\nmodified:%s\ncurrent:%s\nthreeWay:%s\nresult:%s\n",
 			err, c.Description, c.Original, c.TwoWay, c.Modified, c.Current, c.ThreeWay, c.Result)
 		return
 	}
 
-	testPatchCreation(t, expected, actual, c.Description)
-	testPatchApplication(t, original, actual, modified, c.Description)
+	testPatchCreation(t, expectedPatch, actualPatch, c.Description)
+	testPatchApplication(t, original, actualPatch, expectedResult, c.Description)
 }
 
-func twoWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]byte, []byte, []byte) {
+func twoWayTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchTestCase) ([]byte, []byte, []byte, []byte) {
+	expectedResult := c.TwoWayResult
+	if expectedResult == nil {
+		expectedResult = c.Modified
+	}
 	return testObjectToJSONOrFail(t, c.Original, c.Description),
 		testObjectToJSONOrFail(t, c.TwoWay, c.Description),
-		testObjectToJSONOrFail(t, c.Modified, c.Description)
+		testObjectToJSONOrFail(t, c.Modified, c.Description),
+		testObjectToJSONOrFail(t, expectedResult, c.Description)
 }
 
-func twoWayRawTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchRawTestCase) ([]byte, []byte, []byte) {
+func twoWayRawTestCaseToJSONOrFail(t *testing.T, c StrategicMergePatchRawTestCase) ([]byte, []byte, []byte, []byte) {
+	expectedResult := c.TwoWayResult
+	if expectedResult == nil {
+		expectedResult = c.Modified
+	}
 	return yamlToJSONOrError(t, c.Original),
 		yamlToJSONOrError(t, c.TwoWay),
-		yamlToJSONOrError(t, c.Modified)
+		yamlToJSONOrError(t, c.Modified),
+		yamlToJSONOrError(t, expectedResult)
 }
 
 func testThreeWayPatch(t *testing.T, c StrategicMergePatchTestCase) {

test/fixtures/pkg/kubectl/cmd/apply/deploy-clientside.yaml

@@ -0,0 +1,16 @@
+apiVersion: extensions/v1beta1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+spec:
+  strategy:
+    type: Recreate
+    rollingUpdate: null
+  template:
+    metadata:
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx

test/fixtures/pkg/kubectl/cmd/apply/deploy-serverside.yaml

@@ -0,0 +1,46 @@
+apiVersion: extensions/v1beta1
+kind: Deployment
+metadata:
+  annotations:
+    deployment.kubernetes.io/revision: "1"
+    kubectl.kubernetes.io/last-applied-configuration: '{"kind":"Deployment","apiVersion":"extensions/v1beta1","metadata":{"name":"nginx-deployment","creationTimestamp":null},"spec":{"template":{"metadata":{"creationTimestamp":null,"labels":{"name":"nginx"}},"spec":{"containers":[{"name":"nginx","image":"nginx","resources":{}}]}},"strategy":{}},"status":{}}'
+  creationTimestamp: 2016-10-24T22:15:06Z
+  generation: 6
+  labels:
+    name: nginx
+  name: nginx-deployment
+  namespace: test
+  resourceVersion: "355959"
+  selfLink: /apis/extensions/v1beta1/namespaces/test/deployments/nginx-deployment
+  uid: 51ac266e-9a37-11e6-8738-0800270c4edc
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      name: nginx
+  strategy:
+    rollingUpdate:
+      maxSurge: 1
+      maxUnavailable: 1
+    type: RollingUpdate
+  template:
+    metadata:
+      creationTimestamp: null
+      labels:
+        name: nginx
+    spec:
+      containers:
+      - image: nginx
+        imagePullPolicy: Always
+        name: nginx
+        resources: {}
+        terminationMessagePath: /dev/termination-log
+      dnsPolicy: ClusterFirst
+      restartPolicy: Always
+      securityContext: {}
+      terminationGracePeriodSeconds: 30
+status:
+  availableReplicas: 1
+  observedGeneration: 6
+  replicas: 1
+  updatedReplicas: 1