From 4bdcc03c0b0f61f84fdb927af98463896e4992aa Mon Sep 17 00:00:00 2001 From: Anastasis Andronidis Date: Mon, 24 Oct 2016 21:34:28 -0700 Subject: [PATCH 1/3] test for explicit null value propagation in apply --- hack/make-rules/test-cmd-util.sh | 31 +++++++ .../null-propagation/deployment-l1.yaml | 13 +++ .../null-propagation/deployment-l2.yaml | 17 ++++ pkg/kubectl/cmd/apply_test.go | 87 +++++++++++++++++++ pkg/kubectl/cmd/testing/fake.go | 21 +++++ .../kubectl/cmd/apply/deploy-clientside.yaml | 16 ++++ .../kubectl/cmd/apply/deploy-serverside.yaml | 46 ++++++++++ 7 files changed, 231 insertions(+) create mode 100644 hack/testdata/null-propagation/deployment-l1.yaml create mode 100644 hack/testdata/null-propagation/deployment-l2.yaml create mode 100644 test/fixtures/pkg/kubectl/cmd/apply/deploy-clientside.yaml create mode 100644 test/fixtures/pkg/kubectl/cmd/apply/deploy-serverside.yaml diff --git a/hack/make-rules/test-cmd-util.sh b/hack/make-rules/test-cmd-util.sh index 708eedaac83..8fc72ae5333 100644 --- a/hack/make-rules/test-cmd-util.sh +++ b/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}}" '' + kube::test::get_object_assert 'deployments my-depl' "{{.spec.selector.matchLabels.l1}}" '' + kube::test::get_object_assert 'deployments my-depl' "{{.metadata.labels.l1}}" '' + 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 # ############### diff --git a/hack/testdata/null-propagation/deployment-l1.yaml b/hack/testdata/null-propagation/deployment-l1.yaml new file mode 100644 index 00000000000..c5123abc5ee --- /dev/null +++ b/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 diff --git a/hack/testdata/null-propagation/deployment-l2.yaml b/hack/testdata/null-propagation/deployment-l2.yaml new file mode 100644 index 00000000000..6bef22f8815 --- /dev/null +++ b/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 diff --git a/pkg/kubectl/cmd/apply_test.go b/pkg/kubectl/cmd/apply_test.go index c41de7aa158..62564c74789 100644 --- a/pkg/kubectl/cmd/apply_test.go +++ b/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) + } +} diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index 320a5d5e4ff..a9929b96df7 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/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{ { diff --git a/test/fixtures/pkg/kubectl/cmd/apply/deploy-clientside.yaml b/test/fixtures/pkg/kubectl/cmd/apply/deploy-clientside.yaml new file mode 100644 index 00000000000..b3aa9071c4b --- /dev/null +++ b/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 diff --git a/test/fixtures/pkg/kubectl/cmd/apply/deploy-serverside.yaml b/test/fixtures/pkg/kubectl/cmd/apply/deploy-serverside.yaml new file mode 100644 index 00000000000..ffc11aa9df4 --- /dev/null +++ b/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 From 31c6f49846866f4a20acc52a7d41c39f666bacb1 Mon Sep 17 00:00:00 2001 From: Anastasis Andronidis Date: Mon, 17 Oct 2016 21:28:16 -0700 Subject: [PATCH 2/3] CreateThreeWayMergePatch should propagate explicit null fields --- .../pkg/util/strategicpatch/patch.go | 28 +++++++----- .../pkg/util/strategicpatch/patch_test.go | 45 ++++++++++++++----- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go index b9124b322e6..1694aea3376 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go +++ b/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,13 +622,17 @@ 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) } - continue + if ignoreUnmatchedNulls { + continue + } } _, ok := original[k] @@ -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 } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go index 5af6d6ba6b4..0f07342b498 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go @@ -532,7 +532,7 @@ testCases: threeWay: name: null value: null - result: + result: other: a - description: delete all fields from map with conflict original: @@ -549,7 +549,7 @@ testCases: threeWay: name: null value: null - result: + result: other: a - description: add field and delete all fields from map original: @@ -677,12 +677,12 @@ testCases: mergingList: - name: 3 value: 3 - - name: 4 - value: 4 + - name: 4 + value: 4 modified: mergingList: - - name: 4 - value: 4 + - name: 4 + value: 4 - name: 1 - name: 2 value: 2 @@ -699,8 +699,8 @@ testCases: mergingList: - name: 3 value: 3 - - name: 4 - value: 4 + - name: 4 + value: 4 result: mergingList: - name: 1 @@ -710,8 +710,8 @@ testCases: other: b - name: 3 value: 3 - - name: 4 - value: 4 + - name: 4 + value: 4 - description: merge lists of maps with conflict original: mergingList: @@ -1817,6 +1817,27 @@ testCases: other: b - name: 2 other: b + - description: defined null values should propagate overwrite current fields (with conflict) (ignore two-way application) + original: + name: 2 + twoWay: + name: 1 + value: 1 + other: null + modified: + name: 1 + value: 1 + other: null + current: + name: a + other: a + threeWay: + name: 1 + value: 1 + other: null + result: + name: 1 + value: 1 `) var strategicMergePatchRawTestCases = []StrategicMergePatchRawTestCase{ @@ -1992,7 +2013,9 @@ func testTwoWayPatch(t *testing.T, c StrategicMergePatchTestCase) { } testPatchCreation(t, expected, actual, c.Description) - testPatchApplication(t, original, actual, modified, c.Description) + if !strings.Contains(c.Description, "ignore two-way application") { + testPatchApplication(t, original, actual, modified, c.Description) + } } func testTwoWayPatchForRawTestCase(t *testing.T, c StrategicMergePatchRawTestCase) { From cf74abd892fd61fbfc6967b1814bc911df8fd413 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Mon, 23 Jan 2017 17:17:15 -0500 Subject: [PATCH 3/3] fixup apply null tests --- hack/make-rules/test-cmd-util.sh | 11 ++- pkg/kubectl/cmd/apply_test.go | 75 ++++++++--------- pkg/kubectl/cmd/testing/fake.go | 35 ++++---- .../pkg/util/strategicpatch/patch_test.go | 83 ++++++++++++++----- 4 files changed, 121 insertions(+), 83 deletions(-) diff --git a/hack/make-rules/test-cmd-util.sh b/hack/make-rules/test-cmd-util.sh index 8fc72ae5333..3c19baf772a 100644 --- a/hack/make-rules/test-cmd-util.sh +++ b/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. diff --git a/pkg/kubectl/cmd/apply_test.go b/pkg/kubectl/cmd/apply_test.go index 62564c74789..509d06f2413 100644 --- a/pkg/kubectl/cmd/apply_test.go +++ b/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 -// 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) +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) @@ -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() - tf.Client = &fake.RESTClient{ - NegotiatedSerializer: ns, - GroupName: "extensions", + 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": - 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") + } } diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index a9929b96df7..8a42326cffa 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/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"}, + }, + }, + }, } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go b/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go index 0f07342b498..ad5bd235a61 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go +++ b/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 - Modified []byte - Current []byte - TwoWay []byte - ThreeWay []byte - Result []byte + Original []byte + Modified []byte + Current []byte + 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,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) - if !strings.Contains(c.Description, "ignore two-way application") { - 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) {