fixup apply null tests

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

@@ -919,8 +919,10 @@ run_kubectl_create_filter_tests() {
 
 run_kubectl_apply_deployments_tests() {
   ## kubectl apply should propagate user defined null values
-  # Pre-Condition: no Deployments exists
+  # 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
@@ -941,7 +943,12 @@ run_kubectl_apply_deployments_tests() {
   kube::test::get_object_assert 'deployments my-depl' "{{.metadata.labels.l2}}" 'l2'
 
   # cleanup
-  kubectl delete deployments my-depl
+  # 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.

pkg/kubectl/cmd/apply_test.go

@@ -20,7 +20,6 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net/http"
 	"os"
@@ -37,7 +36,6 @@ import (
 	"k8s.io/kubernetes/pkg/api/annotations"
 	"k8s.io/kubernetes/pkg/api/testapi"
 	"k8s.io/kubernetes/pkg/apis/extensions"
-	"k8s.io/kubernetes/pkg/client/restclient/fake"
 	cmdtesting "k8s.io/kubernetes/pkg/kubectl/cmd/testing"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 )
@@ -452,33 +450,8 @@ const (
 	filenameDeployObjClientside = "../../../test/fixtures/pkg/kubectl/cmd/apply/deploy-clientside.yaml"
 )
 
-// validateNULLPatchApplication retrieves the StrategicMergePatch created by kubectl apply and checks that this patch
+func readDeploymentFromFile(t *testing.T, file string) []byte {
-// contains the null value defined by the user.
+	raw := readBytesFromFile(t, file)
-func validateNULLPatchApplication(t *testing.T, req *http.Request) {
-	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)
-	}
-
-	annotationsMap := walkMapPath(t, patchMap, []string{"metadata", "annotations"})
-	if _, ok := annotationsMap[annotations.LastAppliedConfigAnnotation]; !ok {
-		t.Fatalf("patch does not contain annotation:\n%s\n", patch)
-	}
-
-	labelMap := walkMapPath(t, patchMap, []string{"spec", "strategy"})
-	// Checks if `rollingUpdate = null` exists in the patch
-	if deleteMe, ok := labelMap["rollingUpdate"]; !ok || deleteMe != nil {
-		t.Fatalf("patch did not retain null value in key: rollingUpdate:\n%s\n", patch)
-	}
-}
-
-func getServersideObject(t *testing.T) io.ReadCloser {
-	raw := readBytesFromFile(t, filenameDeployObjServerside)
 	obj := &extensions.Deployment{}
 	if err := runtime.DecodeInto(testapi.Extensions.Codec(), raw, obj); err != nil {
 		t.Fatal(err)
@@ -487,27 +460,51 @@ func getServersideObject(t *testing.T) io.ReadCloser {
 	if err != nil {
 		t.Fatal(err)
 	}
-	return ioutil.NopCloser(bytes.NewReader(objJSON))
+	return objJSON
 }
 
 func TestApplyNULLPreservation(t *testing.T) {
+	initTestErrorHandler(t)
 	deploymentName := "nginx-deployment"
 	deploymentPath := "/namespaces/test/deployments/" + deploymentName
 
-	f, tf, _, ns := cmdtesting.NewExtensionsAPIFactory()
+	verifiedPatch := false
-	tf.Client = &fake.RESTClient{
+	deploymentBytes := readDeploymentFromFile(t, filenameDeployObjServerside)
-		NegotiatedSerializer: ns,
+
-		GroupName:            "extensions",
+	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":
-				return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: getServersideObject(t)}, nil
+				body := ioutil.NopCloser(bytes.NewReader(deploymentBytes))
+				return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: body}, nil
 			case p == deploymentPath && m == "PATCH":
-				validateNULLPatchApplication(t, req)
+				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.
-				return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: getServersideObject(t)}, nil
+				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
@@ -529,4 +526,8 @@ func TestApplyNULLPreservation(t *testing.T) {
 	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

@@ -646,27 +646,6 @@ func NewAPIFactory() (cmdutil.Factory, *TestFactory, runtime.Codec, runtime.Nego
 	}, t, testapi.Default.Codec(), testapi.Default.NegotiatedSerializer()
 }
 
-type fakeExtensionsAPIFactory struct {
-	fakeAPIFactory
-}
-
-func (f *fakeExtensionsAPIFactory) JSONEncoder() runtime.Encoder {
-	return testapi.Extensions.Codec()
-}
-
-func NewExtensionsAPIFactory() (cmdutil.Factory, *TestFactory, runtime.Codec, runtime.NegotiatedSerializer) {
-	t := &TestFactory{
-		Validator: validation.NullSchema{},
-	}
-	rf := cmdutil.NewFactory(nil)
-	return &fakeExtensionsAPIFactory{
-		fakeAPIFactory: fakeAPIFactory{
-			Factory: rf,
-			tf:      t,
-		},
-	}, t, testapi.Default.Codec(), testapi.Default.NegotiatedSerializer()
-}
-
 func testDynamicResources() []*discovery.APIGroupResources {
 	return []*discovery.APIGroupResources{
 		{
@@ -690,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_test.go

@@ -64,17 +64,21 @@ 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.
 // The difference is that all the fields in StrategicMergePatchRawTestCaseData are json-encoded data.
 type StrategicMergePatchRawTestCaseData struct {
-	Original []byte
+	Original     []byte
-	Modified []byte
+	Modified     []byte
-	Current  []byte
+	Current      []byte
-	TwoWay   []byte
+	TwoWay       []byte
-	ThreeWay []byte
+	ThreeWay     []byte
-	Result   []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)
+		original, expectedTwoWayPatch, _, expectedResult := twoWayTestCaseToJSONOrFail(t, c)
-		testPatchApplication(t, original, twoWay, modified, c.Description)
+		testPatchApplication(t, original, expectedTwoWayPatch, expectedResult, c.Description)
 	}
 }
 
@@ -1817,13 +1821,16 @@ testCases:
               other: b
         - name: 2
           other: b
-  - description: defined null values should propagate overwrite current fields (with conflict) (ignore two-way application)
+  - 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
@@ -1838,6 +1845,28 @@ testCases:
     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{
@@ -2003,45 +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)
+	testPatchCreation(t, expectedPatch, actualPatch, c.Description)
-	if !strings.Contains(c.Description, "ignore two-way application") {
+	testPatchApplication(t, original, actualPatch, expectedResult, c.Description)
-		testPatchApplication(t, original, actual, modified, 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)
+	testPatchCreation(t, expectedPatch, actualPatch, c.Description)
-	testPatchApplication(t, original, actual, modified, 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) {