From d23c5318691729bcec72405829fb338ab5541f09 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 12 Feb 2016 14:46:23 -0500 Subject: [PATCH] Can't replace a generic resource that is cluster scoped It should be allowed to invoke kubectl replace with a JSON file that has no resource version set. Namespaced resources were working correctly, but cluster resources were silently failing to lookup the current state of the object to get the resource version because we weren't using NamespaceIfScoped(). Added a failing test. --- hack/test-cmd.sh | 25 +++++++++ pkg/kubectl/resource/helper.go | 2 +- pkg/kubectl/resource/helper_test.go | 83 +++++++++++++++++++---------- 3 files changed, 82 insertions(+), 28 deletions(-) diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh index 64acf9cf944..1bd88603493 100755 --- a/hack/test-cmd.sh +++ b/hack/test-cmd.sh @@ -493,6 +493,31 @@ runTests() { kube::test::get_object_assert 'pod valid-pod' "{{(index .spec.containers 0).name}}" 'replaced-k8s-serve-hostname' #cleaning rm /tmp/tmp-valid-pod.json + + ## replace of a cluster scoped resource can succeed + # Pre-condition: a node exists + kubectl create -f - "${kube_flags[@]}" << __EOF__ +{ + "kind": "Node", + "apiVersion": "v1", + "metadata": { + "name": "node-${version}-test" + } +} +__EOF__ + kubectl replace -f - "${kube_flags[@]}" << __EOF__ +{ + "kind": "Node", + "apiVersion": "v1", + "metadata": { + "name": "node-${version}-test", + "annotations": {"a":"b"} + } +} +__EOF__ + # Post-condition: the node command succeeds + kube::test::get_object_assert "node node-${version}-test" "{{.metadata.annotations.a}}" 'b' + kubectl delete node node-${version}-test ## kubectl edit can update the image field of a POD. tmp-editor.sh is a fake editor echo -e '#!/bin/bash\nsed -i "s/nginx/gcr.io\/google_containers\/serve_hostname/g" $1' > /tmp/tmp-editor.sh diff --git a/pkg/kubectl/resource/helper.go b/pkg/kubectl/resource/helper.go index fcce5b0cae6..849a6c0406f 100644 --- a/pkg/kubectl/resource/helper.go +++ b/pkg/kubectl/resource/helper.go @@ -144,7 +144,7 @@ func (m *Helper) Replace(namespace, name string, overwrite bool, obj runtime.Obj } if version == "" && overwrite { // Retrieve the current version of the object to overwrite the server object - serverObj, err := c.Get().Namespace(namespace).Resource(m.Resource).Name(name).Do().Get() + serverObj, err := c.Get().NamespaceIfScoped(namespace, m.NamespaceScoped).Resource(m.Resource).Name(name).Do().Get() if err != nil { // The object does not exist, but we want it to be created return m.replaceResource(c, m.Resource, namespace, name, obj) diff --git a/pkg/kubectl/resource/helper_test.go b/pkg/kubectl/resource/helper_test.go index 541daf880f4..bfb05f7f747 100644 --- a/pkg/kubectl/resource/helper_test.go +++ b/pkg/kubectl/resource/helper_test.go @@ -357,40 +357,42 @@ func TestHelperList(t *testing.T) { } func TestHelperReplace(t *testing.T) { - expectPut := func(req *http.Request) bool { + expectPut := func(path string, req *http.Request) bool { if req.Method != "PUT" { t.Errorf("unexpected method: %#v", req) return false } - parts := splitPath(req.URL.Path) - if parts[1] != "bar" { - t.Errorf("url doesn't contain namespace: %#v", req.URL) - return false - } - if parts[2] != "foo" { - t.Errorf("url doesn't contain name: %#v", req) + if req.URL.Path != path { + t.Errorf("unexpected url: %v", req.URL) return false } return true } tests := []struct { - Resp *http.Response - HTTPClient *http.Client - HttpErr error - Overwrite bool - Object runtime.Object + Resp *http.Response + HTTPClient *http.Client + HttpErr error + Overwrite bool + Object runtime.Object + Namespace string + NamespaceScoped bool + ExpectPath string ExpectObject runtime.Object Err bool - Req func(*http.Request) bool + Req func(string, *http.Request) bool }{ { - HttpErr: errors.New("failure"), - Err: true, + Namespace: "bar", + NamespaceScoped: true, + HttpErr: errors.New("failure"), + Err: true, }, { - Object: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, + Namespace: "bar", + NamespaceScoped: true, + Object: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, Resp: &http.Response{ StatusCode: http.StatusNotFound, Body: objBody(&unversioned.Status{Status: unversioned.StatusFailure}), @@ -398,19 +400,26 @@ func TestHelperReplace(t *testing.T) { Err: true, }, { - Object: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, - ExpectObject: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, + Namespace: "bar", + NamespaceScoped: true, + Object: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, + ExpectPath: "/namespaces/bar/foo", + ExpectObject: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, Resp: &http.Response{ StatusCode: http.StatusOK, Body: objBody(&unversioned.Status{Status: unversioned.StatusSuccess}), }, Req: expectPut, }, + // namespace scoped resource { + Namespace: "bar", + NamespaceScoped: true, Object: &api.Pod{ ObjectMeta: api.ObjectMeta{Name: "foo"}, Spec: apitesting.DeepEqualSafePodSpec(), }, + ExpectPath: "/namespaces/bar/foo", ExpectObject: &api.Pod{ ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}, Spec: apitesting.DeepEqualSafePodSpec(), @@ -424,11 +433,32 @@ func TestHelperReplace(t *testing.T) { }), Req: expectPut, }, + // cluster scoped resource { - Object: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}}, - ExpectObject: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}}, - Resp: &http.Response{StatusCode: http.StatusOK, Body: objBody(&unversioned.Status{Status: unversioned.StatusSuccess})}, - Req: expectPut, + Object: &api.Node{ + ObjectMeta: api.ObjectMeta{Name: "foo"}, + }, + ExpectObject: &api.Node{ + ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}, + }, + Overwrite: true, + ExpectPath: "/foo", + HTTPClient: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + if req.Method == "PUT" { + return &http.Response{StatusCode: http.StatusOK, Body: objBody(&unversioned.Status{Status: unversioned.StatusSuccess})}, nil + } + return &http.Response{StatusCode: http.StatusOK, Body: objBody(&api.Node{ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}})}, nil + }), + Req: expectPut, + }, + { + Namespace: "bar", + NamespaceScoped: true, + Object: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}}, + ExpectPath: "/namespaces/bar/foo", + ExpectObject: &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"}}, + Resp: &http.Response{StatusCode: http.StatusOK, Body: objBody(&unversioned.Status{Status: unversioned.StatusSuccess})}, + Req: expectPut, }, } for i, test := range tests { @@ -441,23 +471,22 @@ func TestHelperReplace(t *testing.T) { modifier := &Helper{ RESTClient: client, Versioner: testapi.Default.MetadataAccessor(), - NamespaceScoped: true, + NamespaceScoped: test.NamespaceScoped, } - _, err := modifier.Replace("bar", "foo", test.Overwrite, test.Object) + _, err := modifier.Replace(test.Namespace, "foo", test.Overwrite, test.Object) if (err != nil) != test.Err { t.Errorf("%d: unexpected error: %t %v", i, test.Err, err) } if err != nil { continue } - if test.Req != nil && !test.Req(client.Req) { + if test.Req != nil && !test.Req(test.ExpectPath, client.Req) { t.Errorf("%d: unexpected request: %#v", i, client.Req) } body, err := ioutil.ReadAll(client.Req.Body) if err != nil { t.Fatalf("%d: unexpected error: %#v", i, err) } - t.Logf("got body: %s", string(body)) expect := []byte{} if test.ExpectObject != nil { expect = []byte(runtime.EncodeOrDie(testapi.Default.Codec(), test.ExpectObject))