test for explicit null value propagation in apply

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

@@ -917,6 +917,33 @@ 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 exists
+  kube::test::get_object_assert deployments "{{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
+  kubectl delete deployments my-depl
+}
+
 # Runs tests for --save-config tests.
 run_save_config_tests() {
   ## Configuration annotations should be set when --save-config is enabled
@@ -2718,6 +2745,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

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"os"
@@ -35,6 +36,8 @@ import (
 	"k8s.io/kubernetes/pkg/api"
 	"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"
 )
@@ -443,3 +446,87 @@ 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"
+)
+
+// validateNULLPatchApplication retrieves the StrategicMergePatch created by kubectl apply and checks that this patch
+// contains the null value defined by the user.
+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)
+	}
+	objJSON, err := runtime.Encode(testapi.Extensions.Codec(), obj)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return ioutil.NopCloser(bytes.NewReader(objJSON))
+}
+
+func TestApplyNULLPreservation(t *testing.T) {
+	deploymentName := "nginx-deployment"
+	deploymentPath := "/namespaces/test/deployments/" + deploymentName
+
+	f, tf, _, ns := cmdtesting.NewExtensionsAPIFactory()
+	tf.Client = &fake.RESTClient{
+		NegotiatedSerializer: ns,
+		GroupName:            "extensions",
+		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
+			case p == deploymentPath && m == "PATCH":
+				validateNULLPatchApplication(t, req)
+				// 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
+			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)
+	}
+}

pkg/kubectl/cmd/testing/fake.go

@@ -646,6 +646,27 @@ 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{
 		{

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