Always set ?namespace in query if specified

Revise our code to only call Request.Namespace() if a namespace *should* be present. For root scoped resources, namespace should be ignored. For namespaced resources, it is an error to have Namespace=="".
satnam6502 · Feb 16, 2015 · 3e2e471 · 3e2e471
1 parent 9175082
commit 3e2e471
Show file tree

Hide file tree

Showing 10 changed files with 100 additions and 63 deletions.
diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go
@@ -279,18 +279,19 @@ func runReplicationControllerTest(c *client.Client) {
 	}
 
 	glog.Infof("Creating replication controllers")
-	if _, err := c.ReplicationControllers(api.NamespaceDefault).Create(&controller); err != nil {
+	updated, err := c.ReplicationControllers(api.NamespaceDefault).Create(&controller)
+	if err != nil {
 		glog.Fatalf("Unexpected error: %v", err)
 	}
 	glog.Infof("Done creating replication controllers")
 
 	// Give the controllers some time to actually create the pods
-	if err := wait.Poll(time.Second, time.Second*30, client.ControllerHasDesiredReplicas(c, &controller)); err != nil {
+	if err := wait.Poll(time.Second, time.Second*30, client.ControllerHasDesiredReplicas(c, updated)); err != nil {
 		glog.Fatalf("FAILED: pods never created %v", err)
 	}
 
 	// wait for minions to indicate they have info about the desired pods
-	pods, err := c.Pods(api.NamespaceDefault).List(labels.Set(controller.Spec.Selector).AsSelector())
+	pods, err := c.Pods(api.NamespaceDefault).List(labels.Set(updated.Spec.Selector).AsSelector())
 	if err != nil {
 		glog.Fatalf("FAILED: unable to get pods to list: %v", err)
 	}
@@ -523,7 +524,7 @@ func runMasterServiceTest(client *client.Client) {
 }
 
 func runServiceTest(client *client.Client) {
-	pod := api.Pod{
+	pod := &api.Pod{
 		ObjectMeta: api.ObjectMeta{
 			Name: "foo",
 			Labels: map[string]string{
@@ -548,14 +549,14 @@ func runServiceTest(client *client.Client) {
 			PodIP: "1.2.3.4",
 		},
 	}
-	_, err := client.Pods(api.NamespaceDefault).Create(&pod)
+	pod, err := client.Pods(api.NamespaceDefault).Create(pod)
 	if err != nil {
 		glog.Fatalf("Failed to create pod: %v, %v", pod, err)
 	}
 	if err := wait.Poll(time.Second, time.Second*20, podExists(client, pod.Namespace, pod.Name)); err != nil {
 		glog.Fatalf("FAILED: pod never started running %v", err)
 	}
-	svc1 := api.Service{
+	svc1 := &api.Service{
 		ObjectMeta: api.ObjectMeta{Name: "service1"},
 		Spec: api.ServiceSpec{
 			Selector: map[string]string{
@@ -566,15 +567,15 @@ func runServiceTest(client *client.Client) {
 			SessionAffinity: "None",
 		},
 	}
-	_, err = client.Services(api.NamespaceDefault).Create(&svc1)
+	svc1, err = client.Services(api.NamespaceDefault).Create(svc1)
 	if err != nil {
 		glog.Fatalf("Failed to create service: %v, %v", svc1, err)
 	}
 	if err := wait.Poll(time.Second, time.Second*20, endpointsSet(client, svc1.Namespace, svc1.Name, 1)); err != nil {
 		glog.Fatalf("FAILED: unexpected endpoints: %v", err)
 	}
 	// A second service with the same port.
-	svc2 := api.Service{
+	svc2 := &api.Service{
 		ObjectMeta: api.ObjectMeta{Name: "service2"},
 		Spec: api.ServiceSpec{
 			Selector: map[string]string{
@@ -585,7 +586,7 @@ func runServiceTest(client *client.Client) {
 			SessionAffinity: "None",
 		},
 	}
-	_, err = client.Services(api.NamespaceDefault).Create(&svc2)
+	svc2, err = client.Services(api.NamespaceDefault).Create(svc2)
 	if err != nil {
 		glog.Fatalf("Failed to create service: %v, %v", svc2, err)
 	}

diff --git a/pkg/client/cache/listwatch_test.go b/pkg/client/cache/listwatch_test.go
@@ -42,7 +42,7 @@ func buildResourcePath(prefix, namespace, resource string) string {
 	base := path.Join("/api", testapi.Version(), prefix)
 	if len(namespace) > 0 {
 		if !(testapi.Version() == "v1beta1" || testapi.Version() == "v1beta2") {
-			base = path.Join(base, "ns", namespace)
+			base = path.Join(base, "namespaces", namespace)
 		}
 	}
 	return path.Join(base, resource)
@@ -58,10 +58,8 @@ func buildQueryValues(namespace string, query url.Values) url.Values {
 			}
 		}
 	}
-	if len(namespace) > 0 {
-		if testapi.Version() == "v1beta1" || testapi.Version() == "v1beta2" {
-			v.Set("namespace", namespace)
-		}
+	if testapi.Version() == "v1beta1" || testapi.Version() == "v1beta2" {
+		v.Set("namespace", namespace)
 	}
 	return v
 }

diff --git a/pkg/client/events.go b/pkg/client/events.go
@@ -66,7 +66,7 @@ func (e *events) Create(event *api.Event) (*api.Event, error) {
 	}
 	result := &api.Event{}
 	err := e.client.Post().
-		Namespace(event.Namespace).
+		NamespaceIfScoped(event.Namespace, len(event.Namespace) > 0).
 		Resource("events").
 		Body(event).
 		Do().
@@ -85,7 +85,7 @@ func (e *events) Update(event *api.Event) (*api.Event, error) {
 	}
 	result := &api.Event{}
 	err := e.client.Put().
-		Namespace(event.Namespace).
+		NamespaceIfScoped(event.Namespace, len(event.Namespace) > 0).
 		Resource("events").
 		Name(event.Name).
 		Body(event).
@@ -98,7 +98,7 @@ func (e *events) Update(event *api.Event) (*api.Event, error) {
 func (e *events) List(label, field labels.Selector) (*api.EventList, error) {
 	result := &api.EventList{}
 	err := e.client.Get().
-		Namespace(e.namespace).
+		NamespaceIfScoped(e.namespace, len(e.namespace) > 0).
 		Resource("events").
 		SelectorParam("labels", label).
 		SelectorParam("fields", field).
@@ -115,7 +115,7 @@ func (e *events) Get(name string) (*api.Event, error) {
 
 	result := &api.Event{}
 	err := e.client.Get().
-		Namespace(e.namespace).
+		NamespaceIfScoped(e.namespace, len(e.namespace) > 0).
 		Resource("events").
 		Name(name).
 		Do().
@@ -127,7 +127,7 @@ func (e *events) Get(name string) (*api.Event, error) {
 func (e *events) Watch(label, field labels.Selector, resourceVersion string) (watch.Interface, error) {
 	return e.client.Get().
 		Prefix("watch").
-		Namespace(e.namespace).
+		NamespaceIfScoped(e.namespace, len(e.namespace) > 0).
 		Resource("events").
 		Param("resourceVersion", resourceVersion).
 		SelectorParam("labels", label).

diff --git a/pkg/client/events_test.go b/pkg/client/events_test.go
@@ -62,7 +62,9 @@ func TestEventCreate(t *testing.T) {
 	}
 	timeStamp := util.Now()
 	event := &api.Event{
-		//namespace: namespace{"default"},
+		ObjectMeta: api.ObjectMeta{
+			Namespace: api.NamespaceDefault,
+		},
 		InvolvedObject: *objReference,
 		FirstTimestamp: timeStamp,
 		LastTimestamp:  timeStamp,
@@ -80,7 +82,7 @@ func TestEventCreate(t *testing.T) {
 	response, err := c.Setup().Events("").Create(event)
 
 	if err != nil {
-		t.Errorf("%#v should be nil.", err)
+		t.Fatalf("%v should be nil.", err)
 	}
 
 	if e, a := *objReference, response.InvolvedObject; !reflect.DeepEqual(e, a) {
@@ -99,6 +101,9 @@ func TestEventGet(t *testing.T) {
 	}
 	timeStamp := util.Now()
 	event := &api.Event{
+		ObjectMeta: api.ObjectMeta{
+			Namespace: "other",
+		},
 		InvolvedObject: *objReference,
 		FirstTimestamp: timeStamp,
 		LastTimestamp:  timeStamp,
@@ -116,7 +121,7 @@ func TestEventGet(t *testing.T) {
 	response, err := c.Setup().Events("").Get("1")
 
 	if err != nil {
-		t.Errorf("%#v should be nil.", err)
+		t.Fatalf("%v should be nil.", err)
 	}
 
 	if e, r := event.InvolvedObject, response.InvolvedObject; !reflect.DeepEqual(e, r) {

diff --git a/pkg/client/request.go b/pkg/client/request.go
@@ -181,6 +181,14 @@ func (r *Request) Namespace(namespace string) *Request {
 	return r
 }
 
+// NamespaceIfScoped is a convenience function to set a namespace if scoped is true
+func (r *Request) NamespaceIfScoped(namespace string, scoped bool) *Request {
+	if scoped {
+		return r.Namespace(namespace)
+	}
+	return r
+}
+
 // AbsPath overwrites an existing path with the segments provided. Trailing slashes are preserved
 // when a single segment is passed.
 func (r *Request) AbsPath(segments ...string) *Request {
@@ -320,7 +328,7 @@ func (r *Request) finalURL() string {
 		query.Add(key, value)
 	}
 
-	if r.namespaceSet && r.namespaceInQuery && len(r.namespace) > 0 {
+	if r.namespaceSet && r.namespaceInQuery {
 		query.Add("namespace", r.namespace)
 	}
 
@@ -427,6 +435,14 @@ func (r *Request) Do() Result {
 			return Result{err: &RequestConstructionError{r.err}}
 		}
 
+		// TODO: added to catch programmer errors (invoking operations with an object with an empty namespace)
+		if (r.verb == "GET" || r.verb == "PUT" || r.verb == "DELETE") && r.namespaceSet && len(r.resourceName) > 0 && len(r.namespace) == 0 {
+			return Result{err: &RequestConstructionError{fmt.Errorf("an empty namespace may not be set when a resource name is provided")}}
+		}
+		if (r.verb == "POST") && r.namespaceSet && len(r.namespace) == 0 {
+			return Result{err: &RequestConstructionError{fmt.Errorf("an empty namespace may not be set during creation")}}
+		}
+
 		req, err := http.NewRequest(r.verb, r.finalURL(), r.body)
 		if err != nil {
 			return Result{err: &RequestConstructionError{err}}

diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
@@ -56,17 +56,15 @@ func TestCreateObjects(t *testing.T) {
 	items := []runtime.Object{}
 
 	items = append(items, &api.Pod{
-		TypeMeta:   api.TypeMeta{APIVersion: "v1beta1", Kind: "Pod"},
-		ObjectMeta: api.ObjectMeta{Name: "test-pod"},
+		ObjectMeta: api.ObjectMeta{Name: "test-pod", Namespace: "default"},
 	})
 
 	items = append(items, &api.Service{
-		TypeMeta:   api.TypeMeta{APIVersion: "v1beta1", Kind: "Service"},
-		ObjectMeta: api.ObjectMeta{Name: "test-service"},
+		ObjectMeta: api.ObjectMeta{Name: "test-service", Namespace: "default"},
 	})
 
 	typer, mapper := getTyperAndMapper()
-	client, s := getFakeClient(t, []string{"/api/v1beta1/pods", "/api/v1beta1/services"})
+	client, s := getFakeClient(t, []string{"/api/v1beta1/pods?namespace=default", "/api/v1beta1/services?namespace=default"})
 
 	errs := CreateObjects(typer, mapper, client, items)
 	s.Close()

diff --git a/pkg/kubectl/resource/helper.go b/pkg/kubectl/resource/helper.go
@@ -35,6 +35,8 @@ type Helper struct {
 	// An interface for reading or writing the resource version of this
 	// type.
 	Versioner runtime.ResourceVersioner
+	// True if the resource type is scoped to namespaces
+	NamespaceScoped bool
 }
 
 // NewHelper creates a Helper from a ResourceMapping
@@ -44,12 +46,14 @@ func NewHelper(client RESTClient, mapping *meta.RESTMapping) *Helper {
 		Resource:   mapping.Resource,
 		Codec:      mapping.Codec,
 		Versioner:  mapping.MetadataAccessor,
+
+		NamespaceScoped: mapping.Scope.Name() == meta.RESTScopeNameNamespace,
 	}
 }
 
 func (m *Helper) Get(namespace, name string) (runtime.Object, error) {
 	return m.RESTClient.Get().
-		Namespace(namespace).
+		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
 		Name(name).
 		Do().
@@ -58,7 +62,7 @@ func (m *Helper) Get(namespace, name string) (runtime.Object, error) {
 
 func (m *Helper) List(namespace string, selector labels.Selector) (runtime.Object, error) {
 	return m.RESTClient.Get().
-		Namespace(namespace).
+		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
 		SelectorParam("labels", selector).
 		Do().
@@ -68,7 +72,7 @@ func (m *Helper) List(namespace string, selector labels.Selector) (runtime.Objec
 func (m *Helper) Watch(namespace, resourceVersion string, labelSelector, fieldSelector labels.Selector) (watch.Interface, error) {
 	return m.RESTClient.Get().
 		Prefix("watch").
-		Namespace(namespace).
+		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
 		Param("resourceVersion", resourceVersion).
 		SelectorParam("labels", labelSelector).
@@ -79,7 +83,7 @@ func (m *Helper) Watch(namespace, resourceVersion string, labelSelector, fieldSe
 func (m *Helper) WatchSingle(namespace, name, resourceVersion string) (watch.Interface, error) {
 	return m.RESTClient.Get().
 		Prefix("watch").
-		Namespace(namespace).
+		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
 		Name(name).
 		Param("resourceVersion", resourceVersion).
@@ -88,7 +92,7 @@ func (m *Helper) WatchSingle(namespace, name, resourceVersion string) (watch.Int
 
 func (m *Helper) Delete(namespace, name string) error {
 	return m.RESTClient.Delete().
-		Namespace(namespace).
+		NamespaceIfScoped(namespace, m.NamespaceScoped).
 		Resource(m.Resource).
 		Name(name).
 		Do().
@@ -100,14 +104,14 @@ func (m *Helper) Create(namespace string, modify bool, data []byte) (runtime.Obj
 		obj, err := m.Codec.Decode(data)
 		if err != nil {
 			// We don't know how to check a version on this object, but create it anyway
-			return createResource(m.RESTClient, m.Resource, namespace, data)
+			return m.createResource(m.RESTClient, m.Resource, namespace, data)
 		}
 
 		// Attempt to version the object based on client logic.
 		version, err := m.Versioner.ResourceVersion(obj)
 		if err != nil {
 			// We don't know how to clear the version on this object, so send it to the server as is
-			return createResource(m.RESTClient, m.Resource, namespace, data)
+			return m.createResource(m.RESTClient, m.Resource, namespace, data)
 		}
 		if version != "" {
 			if err := m.Versioner.SetResourceVersion(obj, ""); err != nil {
@@ -121,11 +125,11 @@ func (m *Helper) Create(namespace string, modify bool, data []byte) (runtime.Obj
 		}
 	}
 
-	return createResource(m.RESTClient, m.Resource, namespace, data)
+	return m.createResource(m.RESTClient, m.Resource, namespace, data)
 }
 
-func createResource(c RESTClient, resource, namespace string, data []byte) (runtime.Object, error) {
-	return c.Post().Namespace(namespace).Resource(resource).Body(data).Do().Get()
+func (m *Helper) createResource(c RESTClient, resource, namespace string, data []byte) (runtime.Object, error) {
+	return c.Post().NamespaceIfScoped(namespace, m.NamespaceScoped).Resource(resource).Body(data).Do().Get()
 }
 
 func (m *Helper) Update(namespace, name string, overwrite bool, data []byte) (runtime.Object, error) {
@@ -134,21 +138,21 @@ func (m *Helper) Update(namespace, name string, overwrite bool, data []byte) (ru
 	obj, err := m.Codec.Decode(data)
 	if err != nil {
 		// We don't know how to handle this object, but update it anyway
-		return updateResource(c, m.Resource, namespace, name, data)
+		return m.updateResource(c, m.Resource, namespace, name, data)
 	}
 
 	// Attempt to version the object based on client logic.
 	version, err := m.Versioner.ResourceVersion(obj)
 	if err != nil {
 		// We don't know how to version this object, so send it to the server as is
-		return updateResource(c, m.Resource, namespace, name, data)
+		return m.updateResource(c, m.Resource, namespace, name, data)
 	}
 	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()
 		if err != nil {
 			// The object does not exist, but we want it to be created
-			return updateResource(c, m.Resource, namespace, name, data)
+			return m.updateResource(c, m.Resource, namespace, name, data)
 		}
 		serverVersion, err := m.Versioner.ResourceVersion(serverObj)
 		if err != nil {
@@ -164,9 +168,9 @@ func (m *Helper) Update(namespace, name string, overwrite bool, data []byte) (ru
 		data = newData
 	}
 
-	return updateResource(c, m.Resource, namespace, name, data)
+	return m.updateResource(c, m.Resource, namespace, name, data)
 }
 
-func updateResource(c RESTClient, resource, namespace, name string, data []byte) (runtime.Object, error) {
-	return c.Put().Namespace(namespace).Resource(resource).Name(name).Body(data).Do().Get()
+func (m *Helper) updateResource(c RESTClient, resource, namespace, name string, data []byte) (runtime.Object, error) {
+	return c.Put().NamespaceIfScoped(namespace, m.NamespaceScoped).Resource(resource).Name(name).Body(data).Do().Get()
 }