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) {