From 964bc8afdaaef2183c70a132cc61d34b82b53228 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 7 May 2015 23:51:36 -0400 Subject: [PATCH 1/4] Verify namespace is not set on root objects --- pkg/kubectl/resource/builder_test.go | 34 ++++++++++++++++++++++++++++ pkg/kubectl/resource/visitor.go | 3 +++ 2 files changed, 37 insertions(+) diff --git a/pkg/kubectl/resource/builder_test.go b/pkg/kubectl/resource/builder_test.go index 3ca445f5cfe..e74f8fae509 100644 --- a/pkg/kubectl/resource/builder_test.go +++ b/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()) diff --git a/pkg/kubectl/resource/visitor.go b/pkg/kubectl/resource/visitor.go index dce54c8d156..ceeb9882497 100644 --- a/pkg/kubectl/resource/visitor.go +++ b/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) From b2a2ce0bb3983d1bc7ceb956af9400d0e54466ff Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 8 May 2015 10:13:42 -0400 Subject: [PATCH 2/4] Legacy scope naming should NEVER set namespace for root ... resources. --- pkg/apiserver/api_installer.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go index 7b27c94f794..59a83bbf6e8 100644 --- a/pkg/apiserver/api_installer.go +++ b/pkg/apiserver/api_installer.go @@ -691,6 +691,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 From 84d1f1901601db5fa1e05ad5f0e64e7996ba07e5 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 8 May 2015 00:31:53 -0400 Subject: [PATCH 3/4] Subresources should be in their parent rest scope A subresource like "Binding" does not necessarily have to have a namespace. The RESTScope of a subresource should always be its parent resource. --- pkg/apiserver/api_installer.go | 18 +++++ pkg/apiserver/apiserver_test.go | 129 ++++++++++++++++++++++++++++++-- pkg/master/master.go | 4 +- 3 files changed, 143 insertions(+), 8 deletions(-) diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go index 59a83bbf6e8..711d01df718 100644 --- a/pkg/apiserver/api_installer.go +++ b/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) diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index 1dd95c3f2cf..18766e540fe 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/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{} diff --git a/pkg/master/master.go b/pkg/master/master.go index 5fa1734ce7d..4543df11d02 100644 --- a/pkg/master/master.go +++ b/pkg/master/master.go @@ -711,7 +711,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 @@ -727,7 +727,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 From ecbca9eb171b9a1f978b9676595db1dac181fa90 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 11 May 2015 10:07:34 -0400 Subject: [PATCH 4/4] Allow v1beta3 to POST events to all namespaces A namespaced resource that supports ALL may allow creation on the root (all namespaces) collection, thus adding POST here. We need to better formalize the definition of calls on namespaced resources at the root scope, so Storage objects that do not support that call pattern can do so at definition time and reject those calls. --- pkg/apiserver/api_installer.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/apiserver/api_installer.go b/pkg/apiserver/api_installer.go index 711d01df718..7aa68b871c0 100644 --- a/pkg/apiserver/api_installer.go +++ b/pkg/apiserver/api_installer.go @@ -320,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 {