Merge pull request kubernetes#3066 from derekwaynecarr/client_update

Do not use namespace in url paths pre v1beta3 from client
duda · Dec 20, 2014 · 36cfc02 · 36cfc02
2 parents 3672f93 + abb6632
commit 36cfc02
Show file tree

Hide file tree

Showing 10 changed files with 109 additions and 42 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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.