Merge pull request #7969 from smarterclayton/rest_fixes

Fixes to apiserver for subresources and naming

pkg/apiserver/api_installer.go

@@ -128,6 +128,24 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 		return err
 	}
 
+	// subresources must have parent resources, and follow the namespacing rules of their parent
+	if hasSubresource {
+		parentStorage, ok := a.group.Storage[resource]
+		if !ok {
+			return fmt.Errorf("subresources can only be declared when the parent is also registered: %s needs %s", path, resource)
+		}
+		parentObject := parentStorage.New()
+		_, parentKind, err := a.group.Typer.ObjectVersionAndKind(parentObject)
+		if err != nil {
+			return err
+		}
+		parentMapping, err := a.group.Mapper.RESTMapping(parentKind, a.group.Version)
+		if err != nil {
+			return err
+		}
+		mapping.Scope = parentMapping.Scope
+	}
+
 	// what verbs are supported by the storage, used to know what verbs we support per path
 	creater, isCreater := storage.(rest.Creater)
 	namedCreater, isNamedCreater := storage.(rest.NamedCreater)
@@ -302,9 +320,11 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 			actions = appendIf(actions, action{"CONNECT", itemPath, nameParams, namer}, isConnecter)
 			actions = appendIf(actions, action{"CONNECT", itemPath + "/{path:*}", nameParams, namer}, isConnecter && connectSubpath)
 
-			// list across namespace.
+			// list or post across namespace.
+			// TODO: more strongly type whether a resource allows these actions on "all namespaces" (bulk delete)
 			namer = scopeNaming{scope, a.group.Linker, gpath.Join(a.prefix, itemPath), true}
 			actions = appendIf(actions, action{"LIST", resource, params, namer}, isLister)
+			actions = appendIf(actions, action{"POST", resource, params, namer}, isCreater)
 			actions = appendIf(actions, action{"WATCHLIST", "watch/" + resource, params, namer}, allowWatchList)
 
 		} else {
@@ -691,6 +711,9 @@ var _ ScopeNamer = legacyScopeNaming{}
 
 // Namespace returns the namespace from the query or the default.
 func (n legacyScopeNaming) Namespace(req *restful.Request) (namespace string, err error) {
+	if n.scope.Name() == meta.RESTScopeNameRoot {
+		return api.NamespaceNone, nil
+	}
 	values, ok := req.Request.URL.Query()[n.scope.ParamName()]
 	if !ok || len(values) == 0 {
 		// legacy behavior

pkg/apiserver/apiserver_test.go

@@ -114,13 +114,13 @@ func init() {
 	// api.Status is returned in errors
 
 	// "internal" version
-	api.Scheme.AddKnownTypes("", &Simple{}, &SimpleList{}, &api.Status{}, &api.ListOptions{}, &SimpleGetOptions{})
+	api.Scheme.AddKnownTypes("", &Simple{}, &SimpleList{}, &api.Status{}, &api.ListOptions{}, &SimpleGetOptions{}, &SimpleRoot{})
 	// "version" version
 	// TODO: Use versioned api objects?
-	api.Scheme.AddKnownTypes(testVersion, &Simple{}, &SimpleList{}, &v1beta1.Status{}, &SimpleGetOptions{})
+	api.Scheme.AddKnownTypes(testVersion, &Simple{}, &SimpleList{}, &v1beta1.Status{}, &SimpleGetOptions{}, &SimpleRoot{})
 	// "version2" version
 	// TODO: Use versioned api objects?
-	api.Scheme.AddKnownTypes(testVersion2, &Simple{}, &SimpleList{}, &v1beta3.Status{}, &SimpleGetOptions{})
+	api.Scheme.AddKnownTypes(testVersion2, &Simple{}, &SimpleList{}, &v1beta3.Status{}, &SimpleGetOptions{}, &SimpleRoot{})
 
 	// Register SimpleGetOptions with the server versions to convert query params to it
 	api.Scheme.AddKnownTypes("v1beta1", &SimpleGetOptions{})
@@ -132,8 +132,14 @@ func init() {
 	for _, version := range versions {
 		for kind := range api.Scheme.KnownTypes(version) {
 			mixedCase := true
-			legacyNsMapper.Add(meta.RESTScopeNamespaceLegacy, kind, version, mixedCase)
-			nsMapper.Add(meta.RESTScopeNamespace, kind, version, mixedCase)
+			root := kind == "SimpleRoot"
+			if root {
+				legacyNsMapper.Add(meta.RESTScopeRoot, kind, version, mixedCase)
+				nsMapper.Add(meta.RESTScopeRoot, kind, version, mixedCase)
+			} else {
+				legacyNsMapper.Add(meta.RESTScopeNamespaceLegacy, kind, version, mixedCase)
+				nsMapper.Add(meta.RESTScopeNamespace, kind, version, mixedCase)
+			}
 		}
 	}
 
@@ -235,6 +241,15 @@ type Simple struct {
 
 func (*Simple) IsAnAPIObject() {}
 
+type SimpleRoot struct {
+	api.TypeMeta   `json:",inline"`
+	api.ObjectMeta `json:"metadata"`
+	Other          string            `json:"other,omitempty"`
+	Labels         map[string]string `json:"labels,omitempty"`
+}
+
+func (*SimpleRoot) IsAnAPIObject() {}
+
 type SimpleGetOptions struct {
 	api.TypeMeta `json:",inline"`
 	Param1       string `json:"param1"`
@@ -550,6 +565,28 @@ func (storage *NamedCreaterRESTStorage) Create(ctx api.Context, name string, obj
 	return obj, err
 }
 
+type SimpleTypedStorage struct {
+	errors   map[string]error
+	item     runtime.Object
+	baseType runtime.Object
+
+	actualNamespace  string
+	namespacePresent bool
+}
+
+func (storage *SimpleTypedStorage) New() runtime.Object {
+	return storage.baseType
+}
+
+func (storage *SimpleTypedStorage) Get(ctx api.Context, id string) (runtime.Object, error) {
+	storage.checkContext(ctx)
+	return api.Scheme.CopyOrDie(storage.item), storage.errors["get"]
+}
+
+func (storage *SimpleTypedStorage) checkContext(ctx api.Context) {
+	storage.actualNamespace, storage.namespacePresent = api.NamespaceFrom(ctx)
+}
+
 func extractBody(response *http.Response, object runtime.Object) (string, error) {
 	defer response.Body.Close()
 	body, err := ioutil.ReadAll(response.Body)
@@ -1182,6 +1219,7 @@ func TestConnect(t *testing.T) {
 		},
 	}
 	storage := map[string]rest.Storage{
+		"simple":         &SimpleRESTStorage{},
 		"simple/connect": connectStorage,
 	}
 	handler := handle(storage)
@@ -1219,6 +1257,7 @@ func TestConnectWithOptions(t *testing.T) {
 		emptyConnectOptions: &SimpleGetOptions{},
 	}
 	storage := map[string]rest.Storage{
+		"simple":         &SimpleRESTStorage{},
 		"simple/connect": connectStorage,
 	}
 	handler := handle(storage)
@@ -1265,6 +1304,7 @@ func TestConnectWithOptionsAndPath(t *testing.T) {
 		takesPath:           "atAPath",
 	}
 	storage := map[string]rest.Storage{
+		"simple":         &SimpleRESTStorage{},
 		"simple/connect": connectStorage,
 	}
 	handler := handle(storage)
@@ -1829,10 +1869,87 @@ func TestCreateChecksDecode(t *testing.T) {
 	}
 }
 
+func TestParentResourceIsRequired(t *testing.T) {
+	storage := &SimpleTypedStorage{
+		baseType: &SimpleRoot{}, // a root scoped type
+		item:     &SimpleRoot{},
+	}
+	group := &APIGroupVersion{
+		Storage: map[string]rest.Storage{
+			"simple/sub": storage,
+		},
+		Root:      "/api",
+		Creater:   api.Scheme,
+		Convertor: api.Scheme,
+		Typer:     api.Scheme,
+		Linker:    selfLinker,
+
+		Admit:   admissionControl,
+		Context: requestContextMapper,
+		Mapper:  namespaceMapper,
+
+		Version:       testVersion2,
+		ServerVersion: "v1beta3",
+		Codec:         codec,
+	}
+	container := restful.NewContainer()
+	if err := group.InstallREST(container); err == nil {
+		t.Fatal("expected error")
+	}
+
+	storage = &SimpleTypedStorage{
+		baseType: &SimpleRoot{}, // a root scoped type
+		item:     &SimpleRoot{},
+	}
+	group = &APIGroupVersion{
+		Storage: map[string]rest.Storage{
+			"simple":     &SimpleRESTStorage{},
+			"simple/sub": storage,
+		},
+		Root:      "/api",
+		Creater:   api.Scheme,
+		Convertor: api.Scheme,
+		Typer:     api.Scheme,
+		Linker:    selfLinker,
+
+		Admit:   admissionControl,
+		Context: requestContextMapper,
+		Mapper:  namespaceMapper,
+
+		Version:       testVersion2,
+		ServerVersion: "v1beta3",
+		Codec:         codec,
+	}
+	container = restful.NewContainer()
+	if err := group.InstallREST(container); err != nil {
+		t.Fatal(err)
+	}
+
+	// resource is NOT registered in the root scope
+	w := httptest.NewRecorder()
+	container.ServeHTTP(w, &http.Request{Method: "GET", URL: &url.URL{Path: "/api/simple/test/sub"}})
+	if w.Code != http.StatusNotFound {
+		t.Errorf("expected not found: %#v", w)
+	}
+
+	// resource is registered in the namespace scope
+	w = httptest.NewRecorder()
+	container.ServeHTTP(w, &http.Request{Method: "GET", URL: &url.URL{Path: "/api/version2/namespaces/test/simple/test/sub"}})
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected OK: %#v", w)
+	}
+	if storage.actualNamespace != "test" {
+		t.Errorf("namespace should be set %#v", storage)
+	}
+}
+
 func TestCreateWithName(t *testing.T) {
 	pathName := "helloworld"
 	storage := &NamedCreaterRESTStorage{SimpleRESTStorage: &SimpleRESTStorage{}}
-	handler := handle(map[string]rest.Storage{"simple/sub": storage})
+	handler := handle(map[string]rest.Storage{
+		"simple":     &SimpleRESTStorage{},
+		"simple/sub": storage,
+	})
 	server := httptest.NewServer(handler)
 	defer server.Close()
 	client := http.Client{}

pkg/kubectl/resource/builder_test.go

@@ -147,6 +147,15 @@ func streamYAMLTestData() (io.Reader, *api.PodList, *api.ServiceList) {
 	return r, pods, svc
 }
 
+func streamTestObject(obj runtime.Object) io.Reader {
+	r, w := io.Pipe()
+	go func() {
+		defer w.Close()
+		w.Write([]byte(runtime.EncodeOrDie(latest.Codec, obj)))
+	}()
+	return r
+}
+
 type testVisitor struct {
 	InjectErr error
 	Infos     []*Info
@@ -617,6 +626,31 @@ func TestSingularObject(t *testing.T) {
 	}
 }
 
+func TestSingularRootScopedObject(t *testing.T) {
+	node := &api.Node{ObjectMeta: api.ObjectMeta{Name: "test"}, Spec: api.NodeSpec{ExternalID: "test"}}
+	r := streamTestObject(node)
+	infos, err := NewBuilder(latest.RESTMapper, api.Scheme, fakeClient()).
+		NamespaceParam("test").DefaultNamespace().
+		Stream(r, "STDIN").
+		Flatten().
+		Do().Infos()
+
+	if err != nil || len(infos) != 1 {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if infos[0].Namespace != "" {
+		t.Errorf("namespace should be empty: %#v", infos[0])
+	}
+	n, ok := infos[0].Object.(*api.Node)
+	if !ok {
+		t.Fatalf("unexpected object: %#v", infos[0].Object)
+	}
+	if n.Name != "test" || n.Namespace != "" {
+		t.Errorf("unexpected object: %#v", n)
+	}
+}
+
 func TestListObject(t *testing.T) {
 	pods, _ := testData()
 	labelKey := api.LabelSelectorQueryParam(testapi.Version())

pkg/kubectl/resource/visitor.go

@@ -474,6 +474,9 @@ func FilterNamespace(info *Info) error {
 // set. If info.Object is set, it will be mutated as well.
 func SetNamespace(namespace string) VisitorFunc {
 	return func(info *Info) error {
+		if !info.Namespaced() {
+			return nil
+		}
 		if len(info.Namespace) == 0 {
 			info.Namespace = namespace
 			UpdateObjectNamespace(info)

pkg/master/master.go

@@ -719,7 +719,7 @@ func (m *Master) api_v1beta2() *apiserver.APIGroupVersion {
 func (m *Master) api_v1beta3() *apiserver.APIGroupVersion {
 	storage := make(map[string]rest.Storage)
 	for k, v := range m.storage {
-		if k == "minions" {
+		if k == "minions" || k == "minions/status" {
 			continue
 		}
 		storage[strings.ToLower(k)] = v
@@ -735,7 +735,7 @@ func (m *Master) api_v1beta3() *apiserver.APIGroupVersion {
 func (m *Master) api_v1() *apiserver.APIGroupVersion {
 	storage := make(map[string]rest.Storage)
 	for k, v := range m.storage {
-		if k == "minions" {
+		if k == "minions" || k == "minions/status" {
 			continue
 		}
 		storage[strings.ToLower(k)] = v