From abb6632d756957dbfb970dd79fd3e005e3ffbecf Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Fri, 19 Dec 2014 16:32:42 -0500 Subject: [PATCH] Do not use namespace in url paths pre v1beta3 from client --- pkg/client/client_test.go | 63 ++++++++++++------- pkg/client/fake.go | 8 +-- pkg/client/helper.go | 8 ++- pkg/client/request.go | 25 +++++--- pkg/client/request_test.go | 2 +- pkg/client/restclient.go | 10 ++- pkg/config/config_test.go | 2 +- pkg/controller/replication_controller_test.go | 9 ++- plugin/pkg/scheduler/factory/factory.go | 2 +- plugin/pkg/scheduler/factory/factory_test.go | 22 ++++++- 10 files changed, 109 insertions(+), 42 deletions(-) diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index d7ada22541a..e4a4b9c6f39 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -130,6 +130,7 @@ func (c *testClient) ValidateCommon(t *testing.T, err error) { requestBody := body(c.Request.Body, c.Request.RawBody) actualQuery := c.handler.RequestReceived.URL.Query() t.Logf("got query: %v", actualQuery) + t.Logf("path: %v", c.Request.Path) // We check the query manually, so blank it out so that FakeHandler.ValidateRequest // won't check it. c.handler.RequestReceived.URL.RawQuery = "" @@ -161,18 +162,38 @@ func (c *testClient) ValidateCommon(t *testing.T, err error) { } } -// convenience function to build paths +// buildResourcePath is a convenience function for knowing if a namespace should in a path param or not func buildResourcePath(namespace, resource string) string { if len(namespace) > 0 { - return path.Join("ns", namespace, resource) + if NamespaceInPathFor(testapi.Version()) { + return path.Join("ns", namespace, resource) + } } return resource } +// buildQueryValues is a convenience function for knowing if a namespace should go in a query param or not +func buildQueryValues(namespace string, query url.Values) url.Values { + v := url.Values{} + if query != nil { + for key, values := range query { + for _, value := range values { + v.Add(key, value) + } + } + } + if len(namespace) > 0 { + if !NamespaceInPathFor(testapi.Version()) { + v.Set("namespace", namespace) + } + } + return v +} + func TestListEmptyPods(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: &api.PodList{}}, } podList, err := c.Setup().Pods(ns).List(labels.Everything()) @@ -182,7 +203,7 @@ func TestListEmptyPods(t *testing.T) { func TestListPods(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: &api.PodList{ Items: []api.Pod{ @@ -214,7 +235,7 @@ func validateLabels(a, b string) bool { func TestListPodsLabels(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods"), Query: url.Values{"labels": []string{"foo=bar,name=baz"}}}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods"), Query: buildQueryValues(ns, url.Values{"labels": []string{"foo=bar,name=baz"}})}, Response: Response{ StatusCode: 200, Body: &api.PodList{ @@ -244,7 +265,7 @@ func TestListPodsLabels(t *testing.T) { func TestGetPod(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods/foo")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/pods/foo"), Query: buildQueryValues(ns, nil)}, Response: Response{ StatusCode: 200, Body: &api.Pod{ @@ -278,7 +299,7 @@ func TestGetPodWithNoName(t *testing.T) { func TestDeletePod(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "DELETE", Path: buildResourcePath(ns, "/pods/foo")}, + Request: testRequest{Method: "DELETE", Path: buildResourcePath(ns, "/pods/foo"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200}, } err := c.Setup().Pods(ns).Delete("foo") @@ -299,7 +320,7 @@ func TestCreatePod(t *testing.T) { }, } c := &testClient{ - Request: testRequest{Method: "POST", Path: buildResourcePath(ns, "/pods"), Body: requestPod}, + Request: testRequest{Method: "POST", Path: buildResourcePath(ns, "/pods"), Query: buildQueryValues(ns, nil), Body: requestPod}, Response: Response{ StatusCode: 200, Body: requestPod, @@ -325,7 +346,7 @@ func TestUpdatePod(t *testing.T) { }, } c := &testClient{ - Request: testRequest{Method: "PUT", Path: buildResourcePath(ns, "/pods/foo")}, + Request: testRequest{Method: "PUT", Path: buildResourcePath(ns, "/pods/foo"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: requestPod}, } receivedPod, err := c.Setup().Pods(ns).Update(requestPod) @@ -363,7 +384,7 @@ func TestListControllers(t *testing.T) { func TestGetController(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/replicationControllers/foo")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/replicationControllers/foo"), Query: buildQueryValues(ns, nil)}, Response: Response{ StatusCode: 200, Body: &api.ReplicationController{ @@ -402,7 +423,7 @@ func TestUpdateController(t *testing.T) { ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"}, } c := &testClient{ - Request: testRequest{Method: "PUT", Path: buildResourcePath(ns, "/replicationControllers/foo")}, + Request: testRequest{Method: "PUT", Path: buildResourcePath(ns, "/replicationControllers/foo"), Query: buildQueryValues(ns, nil)}, Response: Response{ StatusCode: 200, Body: &api.ReplicationController{ @@ -427,7 +448,7 @@ func TestUpdateController(t *testing.T) { func TestDeleteController(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "DELETE", Path: buildResourcePath(ns, "/replicationControllers/foo")}, + Request: testRequest{Method: "DELETE", Path: buildResourcePath(ns, "/replicationControllers/foo"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200}, } err := c.Setup().ReplicationControllers(ns).Delete("foo") @@ -440,7 +461,7 @@ func TestCreateController(t *testing.T) { ObjectMeta: api.ObjectMeta{Name: "foo"}, } c := &testClient{ - Request: testRequest{Method: "POST", Path: buildResourcePath(ns, "/replicationControllers"), Body: requestController}, + Request: testRequest{Method: "POST", Path: buildResourcePath(ns, "/replicationControllers"), Body: requestController, Query: buildQueryValues(ns, nil)}, Response: Response{ StatusCode: 200, Body: &api.ReplicationController{ @@ -474,7 +495,7 @@ func body(obj runtime.Object, raw *string) *string { func TestListServices(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/services")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/services"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: &api.ServiceList{ Items: []api.Service{ @@ -504,7 +525,7 @@ func TestListServices(t *testing.T) { func TestListServicesLabels(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/services"), Query: url.Values{"labels": []string{"foo=bar,name=baz"}}}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/services"), Query: buildQueryValues(ns, url.Values{"labels": []string{"foo=bar,name=baz"}})}, Response: Response{StatusCode: 200, Body: &api.ServiceList{ Items: []api.Service{ @@ -536,7 +557,7 @@ func TestListServicesLabels(t *testing.T) { func TestGetService(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/services/1")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/services/1"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: &api.Service{ObjectMeta: api.ObjectMeta{Name: "service-1"}}}, } response, err := c.Setup().Services(ns).Get("1") @@ -557,7 +578,7 @@ func TestGetServiceWithNoName(t *testing.T) { func TestCreateService(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "POST", Path: buildResourcePath(ns, "/services"), Body: &api.Service{ObjectMeta: api.ObjectMeta{Name: "service-1"}}}, + Request: testRequest{Method: "POST", Path: buildResourcePath(ns, "/services"), Body: &api.Service{ObjectMeta: api.ObjectMeta{Name: "service-1"}}, Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: &api.Service{ObjectMeta: api.ObjectMeta{Name: "service-1"}}}, } response, err := c.Setup().Services(ns).Create(&api.Service{ObjectMeta: api.ObjectMeta{Name: "service-1"}}) @@ -568,7 +589,7 @@ func TestUpdateService(t *testing.T) { ns := api.NamespaceDefault svc := &api.Service{ObjectMeta: api.ObjectMeta{Name: "service-1", ResourceVersion: "1"}} c := &testClient{ - Request: testRequest{Method: "PUT", Path: buildResourcePath(ns, "/services/service-1"), Body: svc}, + Request: testRequest{Method: "PUT", Path: buildResourcePath(ns, "/services/service-1"), Body: svc, Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: svc}, } response, err := c.Setup().Services(ns).Update(svc) @@ -578,7 +599,7 @@ func TestUpdateService(t *testing.T) { func TestDeleteService(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "DELETE", Path: buildResourcePath(ns, "/services/1")}, + Request: testRequest{Method: "DELETE", Path: buildResourcePath(ns, "/services/1"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200}, } err := c.Setup().Services(ns).Delete("1") @@ -588,7 +609,7 @@ func TestDeleteService(t *testing.T) { func TestListEndpooints(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/endpoints")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/endpoints"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: &api.EndpointsList{ Items: []api.Endpoints{ @@ -607,7 +628,7 @@ func TestListEndpooints(t *testing.T) { func TestGetEndpoints(t *testing.T) { ns := api.NamespaceDefault c := &testClient{ - Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/endpoints/endpoint-1")}, + Request: testRequest{Method: "GET", Path: buildResourcePath(ns, "/endpoints/endpoint-1"), Query: buildQueryValues(ns, nil)}, Response: Response{StatusCode: 200, Body: &api.Endpoints{ObjectMeta: api.ObjectMeta{Name: "endpoint-1"}}}, } response, err := c.Setup().Endpoints(ns).Get("endpoint-1") diff --git a/pkg/client/fake.go b/pkg/client/fake.go index e0ca630e796..64975530def 100644 --- a/pkg/client/fake.go +++ b/pkg/client/fake.go @@ -96,16 +96,16 @@ type FakeRESTClient struct { } func (c *FakeRESTClient) Get() *Request { - return NewRequest(c, "GET", &url.URL{Host: "localhost"}, c.Codec) + return NewRequest(c, "GET", &url.URL{Host: "localhost"}, c.Codec, true) } func (c *FakeRESTClient) Put() *Request { - return NewRequest(c, "PUT", &url.URL{Host: "localhost"}, c.Codec) + return NewRequest(c, "PUT", &url.URL{Host: "localhost"}, c.Codec, true) } func (c *FakeRESTClient) Post() *Request { - return NewRequest(c, "POST", &url.URL{Host: "localhost"}, c.Codec) + return NewRequest(c, "POST", &url.URL{Host: "localhost"}, c.Codec, true) } func (c *FakeRESTClient) Delete() *Request { - return NewRequest(c, "DELETE", &url.URL{Host: "localhost"}, c.Codec) + return NewRequest(c, "DELETE", &url.URL{Host: "localhost"}, c.Codec, true) } func (c *FakeRESTClient) Do(req *http.Request) (*http.Response, error) { c.Req = req diff --git a/pkg/client/helper.go b/pkg/client/helper.go index 0f7b2e34a90..ac8c66780a2 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -117,7 +117,7 @@ func RESTClientFor(config *Config) (*RESTClient, error) { return nil, err } - client := NewRESTClient(baseURL, versionInterfaces.Codec) + client := NewRESTClient(baseURL, versionInterfaces.Codec, NamespaceInPathFor(version)) transport, err := TransportFor(config) if err != nil { @@ -252,3 +252,9 @@ func defaultVersionFor(config *Config) string { } return version } + +// namespaceInPathFor is used to control what api version should use namespace in url paths +func NamespaceInPathFor(version string) bool { + // we use query param for v1beta1/v1beta2, v1beta3+ will use path param + return (version != "v1beta1" && version != "v1beta2") +} diff --git a/pkg/client/request.go b/pkg/client/request.go index eb6cad9109c..040ddfc81fe 100644 --- a/pkg/client/request.go +++ b/pkg/client/request.go @@ -96,20 +96,23 @@ type Request struct { sync bool timeout time.Duration + // flag to control how to use namespace in urls + namespaceAsPath bool + // output err error body io.Reader } // NewRequest creates a new request with the core attributes. -func NewRequest(client HTTPClient, verb string, baseURL *url.URL, codec runtime.Codec) *Request { +func NewRequest(client HTTPClient, verb string, baseURL *url.URL, codec runtime.Codec, namespaceAsPath bool) *Request { return &Request{ - client: client, - verb: verb, - baseURL: baseURL, - codec: codec, - - path: baseURL.Path, + client: client, + verb: verb, + baseURL: baseURL, + codec: codec, + namespaceAsPath: namespaceAsPath, + path: baseURL.Path, } } @@ -136,8 +139,14 @@ func (r *Request) Namespace(namespace string) *Request { if r.err != nil { return r } + if len(namespace) > 0 { - return r.Path("ns").Path(namespace) + if r.namespaceAsPath { + return r.Path("ns").Path(namespace) + } else { + return r.setParam("namespace", namespace) + } + } return r } diff --git a/pkg/client/request_test.go b/pkg/client/request_test.go index bf89182411c..ef04e9362d8 100644 --- a/pkg/client/request_test.go +++ b/pkg/client/request_test.go @@ -137,7 +137,7 @@ func TestTransformResponse(t *testing.T) { {Response: &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(invalid))}, Data: invalid}, } for i, test := range testCases { - r := NewRequest(nil, "", uri, testapi.Codec()) + r := NewRequest(nil, "", uri, testapi.Codec(), true) if test.Response.Body == nil { test.Response.Body = ioutil.NopCloser(bytes.NewReader([]byte{})) } diff --git a/pkg/client/restclient.go b/pkg/client/restclient.go index 9b8d4b542a7..926e43b1d64 100644 --- a/pkg/client/restclient.go +++ b/pkg/client/restclient.go @@ -36,6 +36,10 @@ import ( type RESTClient struct { baseURL *url.URL + // namespaceInPath controls if URLs should encode the namespace as path param instead of query param + // needed for backward compatibility + namespaceInPath bool + // Codec is the encoding and decoding scheme that applies to a particular set of // REST resources. Codec runtime.Codec @@ -56,7 +60,7 @@ type RESTClient struct { // NewRESTClient creates a new RESTClient. This client performs generic REST functions // such as Get, Put, Post, and Delete on specified paths. Codec controls encoding and // decoding of responses from the server. -func NewRESTClient(baseURL *url.URL, c runtime.Codec) *RESTClient { +func NewRESTClient(baseURL *url.URL, c runtime.Codec, namespaceInPath bool) *RESTClient { base := *baseURL if !strings.HasSuffix(base.Path, "/") { base.Path += "/" @@ -68,6 +72,8 @@ func NewRESTClient(baseURL *url.URL, c runtime.Codec) *RESTClient { baseURL: &base, Codec: c, + namespaceInPath: namespaceInPath, + // Make asynchronous requests by default Sync: false, @@ -98,7 +104,7 @@ func (c *RESTClient) Verb(verb string) *Request { if poller == nil { poller = c.DefaultPoll } - return NewRequest(c.Client, verb, c.baseURL, c.Codec).Poller(poller).Sync(c.Sync).Timeout(c.Timeout) + return NewRequest(c.Client, verb, c.baseURL, c.Codec, c.namespaceInPath).Poller(poller).Sync(c.Sync).Timeout(c.Timeout) } // Post begins a POST request. Short for c.Verb("POST"). diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 5150c46de87..e5bac102ceb 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -48,7 +48,7 @@ func getFakeClient(t *testing.T, validURLs []string) (ClientPosterFunc, *httptes return func(mapping *meta.RESTMapping) (RESTClientPoster, error) { fakeCodec := runtime.CodecFor(api.Scheme, "v1beta1") fakeUri, _ := url.Parse(server.URL + "/api/v1beta1") - return client.NewRESTClient(fakeUri, fakeCodec), nil + return client.NewRESTClient(fakeUri, fakeCodec, false), nil }, server } diff --git a/pkg/controller/replication_controller_test.go b/pkg/controller/replication_controller_test.go index 6f224b86463..21f17d768d5 100644 --- a/pkg/controller/replication_controller_test.go +++ b/pkg/controller/replication_controller_test.go @@ -37,6 +37,13 @@ import ( "github.com/coreos/go-etcd/etcd" ) +func makeNamespaceURL(namespace, suffix string) string { + if client.NamespaceInPathFor(testapi.Version()) { + return makeURL("/ns/" + namespace + suffix) + } + return makeURL(suffix + "?namespace=" + namespace) +} + func makeURL(suffix string) string { return path.Join("/api", testapi.Version(), suffix) } @@ -221,7 +228,7 @@ func TestCreateReplica(t *testing.T) { }, Spec: controllerSpec.Spec.Template.Spec, } - fakeHandler.ValidateRequest(t, makeURL("/ns/default/pods"), "POST", nil) + fakeHandler.ValidateRequest(t, makeNamespaceURL("default", "/pods"), "POST", nil) actualPod, err := client.Codec.Decode([]byte(fakeHandler.RequestBody)) if err != nil { t.Errorf("Unexpected error: %#v", err) diff --git a/plugin/pkg/scheduler/factory/factory.go b/plugin/pkg/scheduler/factory/factory.go index e2b44f17fc4..d87a596e9cf 100644 --- a/plugin/pkg/scheduler/factory/factory.go +++ b/plugin/pkg/scheduler/factory/factory.go @@ -204,7 +204,7 @@ func (factory *ConfigFactory) pollMinions() (cache.Enumerator, error) { func (factory *ConfigFactory) makeDefaultErrorFunc(backoff *podBackoff, podQueue *cache.FIFO) func(pod *api.Pod, err error) { return func(pod *api.Pod, err error) { - glog.Errorf("Error scheduling %v: %v; retrying", pod.Name, err) + glog.Errorf("Error scheduling %v %v: %v; retrying", pod.Namespace, pod.Name, err) backoff.gc() // Retry asynchronously. // Note that this is extremely rudimentary and we need a more real error handling path. diff --git a/plugin/pkg/scheduler/factory/factory_test.go b/plugin/pkg/scheduler/factory/factory_test.go index 36ad254a685..3075775716a 100644 --- a/plugin/pkg/scheduler/factory/factory_test.go +++ b/plugin/pkg/scheduler/factory/factory_test.go @@ -19,6 +19,7 @@ package factory import ( "net/http" "net/http/httptest" + "path" "reflect" "testing" "time" @@ -183,6 +184,22 @@ func TestPollMinions(t *testing.T) { } } +func makeNamespaceURL(namespace, suffix string, isClient bool) string { + if client.NamespaceInPathFor(testapi.Version()) { + return makeURL("/ns/" + namespace + suffix) + } + // if this is a url the client should call, encode the url + if isClient { + return makeURL(suffix + "?namespace=" + namespace) + } + // its not a client url, so its what the server needs to listen on + return makeURL(suffix) +} + +func makeURL(suffix string) string { + return path.Join("/api", testapi.Version(), suffix) +} + func TestDefaultErrorFunc(t *testing.T) { testPod := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "bar"}} handler := util.FakeHandler{ @@ -191,8 +208,9 @@ func TestDefaultErrorFunc(t *testing.T) { T: t, } mux := http.NewServeMux() + // FakeHandler musn't be sent requests other than the one you want to test. - mux.Handle("/api/"+testapi.Version()+"/ns/bar/pods/foo", &handler) + mux.Handle(makeNamespaceURL("bar", "/pods/foo", false), &handler) server := httptest.NewServer(mux) defer server.Close() factory := NewConfigFactory(client.NewOrDie(&client.Config{Host: server.URL, Version: testapi.Version()})) @@ -213,7 +231,7 @@ func TestDefaultErrorFunc(t *testing.T) { if !exists { continue } - handler.ValidateRequest(t, "/api/"+testapi.Version()+"/ns/bar/pods/foo", "GET", nil) + handler.ValidateRequest(t, makeNamespaceURL("bar", "/pods/foo", true), "GET", nil) if e, a := testPod, got; !reflect.DeepEqual(e, a) { t.Errorf("Expected %v, got %v", e, a) }