Fix cross-namespace LIST and WATCH #4395

smarterclayton · 2015-02-12T20:02:22Z

Extracted from #4263 comments @roberthbailey

Realized after this went in that cross-namespace queries were broken.

roberthbailey · 2015-02-12T21:11:00Z

pkg/apiserver/api_installer.go

+func (n rootScopeNaming) Name(req *restful.Request) (namespace, name string, err error) {
+	name = req.PathParameter("name")
+	if len(name) == 0 {
+		err = errEmptyName


I'm still not sure I'm thrilled with the naked returns. How is assigning err here better than just having this line be return "", errEmptyName?

Naked returns bug me, also.

I do, however, like having names to the two return types rather than just returning (string, string) so that it's obvious to callers what is being returned.

Agreed, will fix that today.

On Feb 13, 2015, at 1:03 AM, roberthbailey notifications@github.com wrote:

In pkg/apiserver/api_installer.go:

@@ -401,6 +321,140 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage
return nil
}

+// rootScopeNaming reads only names from a request and ignores namespaces
+type rootScopeNaming struct {

scope meta.RESTScope

runtime.SelfLinker

itemPath string
+}

+func (n rootScopeNaming) Namespace(req *restful.Request) (namespace string, err error) {

return
+}

+func (n rootScopeNaming) Name(req *restful.Request) (namespace, name string, err error) {

name = req.PathParameter("name")

if len(name) == 0 {

err = errEmptyName
I do, however, like having names to the two return types rather than just returning (string, string) so that it's obvious to callers what is being returned.

—
Reply to this email directly or view it on GitHub.

Made that change.

----- Original Message -----

@@ -401,6 +321,140 @@ func (a *APIInstaller) registerResourceHandlers(path
string, storage RESTStorage
return nil
}

+// rootScopeNaming reads only names from a request and ignores namespaces
+type rootScopeNaming struct {

scope meta.RESTScope

runtime.SelfLinker

itemPath string
+}

+func (n rootScopeNaming) Namespace(req *restful.Request) (namespace
string, err error) {

return
+}

+func (n rootScopeNaming) Name(req *restful.Request) (namespace, name
string, err error) {

name = req.PathParameter("name")

if len(name) == 0 {

err = errEmptyName

I do, however, like having names to the two return types rather than just
returning (string, string) so that it's obvious to callers what is being
returned.

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/4395/files#r24645956

roberthbailey · 2015-02-12T21:13:31Z

I left a few initial comments but I'm going to be out of the office until Tuesday. Unassigning in the hopes that someone else will look at this before then, but if not feel free to reassign back to me at that point.

bgrant0607 · 2015-02-13T19:02:02Z

Please rebase. There's a merge conflict.

Otherwise, LGTM.

@lavalamp Suggested defining a struct that contains both namespace and name, which sounds good to me, but that can be a separate PR.

bgrant0607 · 2015-02-13T19:51:40Z

@nikhiljindal said he made some comments directly on a commit that were lost in a rebase. @smarterclayton did you see them in email?

smarterclayton · 2015-02-13T20:05:25Z

I think so, but will double check when I rebase

On Feb 13, 2015, at 2:51 PM, Brian Grant notifications@github.com wrote:

@nikhiljindal said he made some comments directly on a commit that were lost in a rebase. @smarterclayton did you see them in email?

—
Reply to this email directly or view it on GitHub.

googlebot · 2015-02-16T02:13:45Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

smarterclayton · 2015-02-16T02:14:56Z

This has been updated with nikhil's comment. It resolves a fairly serious bug introduced with the first version of this pull - cross namespace LIST and WATCH (which we apparently had no test cases for) has been corrected (the scope namer passed to the cross namespace methods does not set a default namespace). Added more test cases.

smarterclayton · 2015-02-16T03:18:05Z

Actually... picking the bandaid a bit more, and a lot more cross namespace query arterial blood comes pouring out... Now it seems like the client was not properly namespacing requests in a lot of cases, which means that a few api.NamespaceAll queries were only working on the default namespace.

smarterclayton · 2015-02-16T04:43:25Z

@derekwaynecarr did we change NamespaceAll recently? I'm seeing changes that are possibly related to my earlier api_installer changes from #4263, but also errors that look like we weren't handling namespace scoping consistently from the client (i.e. clients would not be able to properly set NamespaceAll scope).

After digging into this more:

The "query all" code in endpoints controller sends NamespaceAll == ""
client/request.go would only set namespace param if it was length > 0
server treated missing namespace param as "default namespace"
endpoints were not being set in different namespaces

smarterclayton · 2015-02-16T04:52:10Z

My second commit here properly enforces namespace != "" on namespace scoped resources, and forces our client code to distinguish between namespace not present and namespace empty. Given the larger scope of the change it needs deeper review. This now properly distinguishes in client code between namespace and non-namespaced scoped resources.

The legacy behavior of handling empty namespace like "default" on POST is preserved in the first commit.

Fix bug with cross namespace queries not being possible. Ensure selflink is set on lists correctly.

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=="".

derekwaynecarr · 2015-02-16T06:25:20Z

I am not aware of a change. I will review this PR more in morning

derekwaynecarr · 2015-02-16T06:38:04Z

At this point, handlers_test had cases for ensuring namespace all behaviors since that was the prior choke point. but I am guessing that the changes were at a level above that test.

Sent from my iPhone

On Feb 15, 2015, at 11:43 PM, Clayton Coleman notifications@github.com wrote:

@derekwaynecarr did we change NamespaceAll recently? I'm seeing changes that are possibly related to my earlier api_installer changes from #4263, but also errors that look like we weren't handling namespace scoping consistently from the client (i.e. clients would not be able to properly set NamespaceAll scope).

After digging into this more:

The "query all" code in endpoints controller sends NamespaceAll == ""
client/request.go would only set namespace param if it was length > 0
server treated missing namespace param as "default namespace"
endpoints were not being set in different namespaces
—
Reply to this email directly or view it on GitHub.

smarterclayton · 2015-02-16T14:31:22Z

They were - I think this implies to me that there needs to be a higher level set of tests around paths, methods, and RESTStorage objects that permutes all endpoints (like auth_test, but specifically to highlight the modes).

----- Original Message -----

At this point, handlers_test had cases for ensuring namespace all behaviors
since that was the prior choke point. but I am guessing that the changes
were at a level above that test.

Sent from my iPhone

On Feb 15, 2015, at 11:43 PM, Clayton Coleman notifications@github.com
wrote:

@derekwaynecarr did we change NamespaceAll recently? I'm seeing changes
that are possibly related to my earlier api_installer changes from #4263,
but also errors that look like we weren't handling namespace scoping
consistently from the client (i.e. clients would not be able to properly
set NamespaceAll scope).

After digging into this more:

The "query all" code in endpoints controller sends NamespaceAll == ""
client/request.go would only set namespace param if it was length > 0
server treated missing namespace param as "default namespace"
endpoints were not being set in different namespaces
—
Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub:
#4395 (comment)

derekwaynecarr · 2015-02-16T16:34:45Z

LGTM - the client side change will help stop subtle errors. the server side change once I found it is right ;-)

Allows self links to be directly passed

smarterclayton · 2015-02-16T21:58:14Z

Added a better integration test for overlapping endpoint namespaces (to explicitly validate NamespaceAll) and made SelfLink more correct for default namespaces.

bgrant0607 · 2015-02-17T16:00:32Z

pkg/apiserver/api_installer.go

-			actions = appendIf(actions, action{"PROXY", "/proxy/" + itemPath, nameParams}, storageVerbs["Redirector"])
+			namer := legacyScopeNaming{scope, a.group.linker, gpath.Join(a.prefix, itemPath)}
+
+			actions = appendIf(actions, action{"LIST", path, namespaceParams, namer}, storageVerbs["RESTLister"])


Doesn't have to be done for this PR, but it would be useful to factor out the common lines of code between the two cases. Or maybe we should just eliminate v1beta1. :-)

bgrant0607 · 2015-02-17T16:02:54Z

LGTM. Feel free to merge.

Fix cross-namespace LIST and WATCH

brendandburns · 2015-02-18T03:48:32Z

This broke e2e on update.sh, because it gets tripped up in the check for an empty namespace.

I'm going to try to patch forward rather than roll back, but pretty please let's be careful about running e2e.

smarterclayton · 2015-02-18T04:14:07Z

I saw passing update, but obviously this wasn't true. Can you cc me on the fix?

On Feb 17, 2015, at 10:48 PM, Brendan Burns notifications@github.com wrote:

This broke e2e on update.sh, because it gets tripped up in the check for an empty namespace.

I'm going to try to patch forward rather than roll back, but pretty please let's be careful about running e2e.

—
Reply to this email directly or view it on GitHub.

saad-ali assigned saad-ali and roberthbailey and unassigned saad-ali Feb 12, 2015

roberthbailey reviewed Feb 12, 2015
View reviewed changes

roberthbailey removed their assignment Feb 12, 2015

bgrant0607 self-assigned this Feb 13, 2015

smarterclayton force-pushed the split_naming branch 2 times, most recently from 06de1d6 to a99e9b1 Compare February 13, 2015 18:46

smarterclayton force-pushed the split_naming branch from a99e9b1 to dc48aa0 Compare February 16, 2015 02:13

smarterclayton added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Feb 16, 2015

smarterclayton changed the title ~~Split naming behavior out into objects that are derived from the request~~ Fix cross-namespace LIST and WATCH Feb 16, 2015

smarterclayton added the kind/bug Categorizes issue or PR as related to a bug. label Feb 16, 2015

smarterclayton force-pushed the split_naming branch from dc48aa0 to fcfe32a Compare February 16, 2015 04:45

smarterclayton force-pushed the split_naming branch from fcfe32a to 2b68f9f Compare February 16, 2015 05:18

smarterclayton added 2 commits February 16, 2015 00:23

Split naming behavior out into objects that are derived from the request

9175082

Fix bug with cross namespace queries not being possible. Ensure selflink is set on lists correctly.

Always set ?namespace in query if specified

3e2e471

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=="".

smarterclayton force-pushed the split_naming branch from 2b68f9f to 3e2e471 Compare February 16, 2015 05:24

smarterclayton added 2 commits February 16, 2015 16:31

Provide a helper on client/request.go for full URI

7e07d71

Allows self links to be directly passed

Test namespace overlap in test-integration.sh

eebe00c

bgrant0607 reviewed Feb 17, 2015
View reviewed changes

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2015

bgrant0607 added a commit that referenced this pull request Feb 17, 2015

Merge pull request #4395 from smarterclayton/split_naming

876d651

Fix cross-namespace LIST and WATCH

bgrant0607 merged commit 876d651 into kubernetes:master Feb 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cross-namespace LIST and WATCH #4395

Fix cross-namespace LIST and WATCH #4395

smarterclayton commented Feb 12, 2015

roberthbailey Feb 12, 2015

bgrant0607 Feb 13, 2015

roberthbailey Feb 13, 2015

smarterclayton Feb 13, 2015

smarterclayton Feb 13, 2015

roberthbailey commented Feb 12, 2015

bgrant0607 commented Feb 13, 2015

bgrant0607 commented Feb 13, 2015

smarterclayton commented Feb 13, 2015

googlebot commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

derekwaynecarr commented Feb 16, 2015

derekwaynecarr commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

derekwaynecarr commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

bgrant0607 Feb 17, 2015

bgrant0607 commented Feb 17, 2015

brendandburns commented Feb 18, 2015

smarterclayton commented Feb 18, 2015

Fix cross-namespace LIST and WATCH #4395

Fix cross-namespace LIST and WATCH #4395

Conversation

smarterclayton commented Feb 12, 2015

roberthbailey Feb 12, 2015

Choose a reason for hiding this comment

bgrant0607 Feb 13, 2015

Choose a reason for hiding this comment

roberthbailey Feb 13, 2015

Choose a reason for hiding this comment

smarterclayton Feb 13, 2015

Choose a reason for hiding this comment

smarterclayton Feb 13, 2015

Choose a reason for hiding this comment

roberthbailey commented Feb 12, 2015

bgrant0607 commented Feb 13, 2015

bgrant0607 commented Feb 13, 2015

smarterclayton commented Feb 13, 2015

googlebot commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

derekwaynecarr commented Feb 16, 2015

derekwaynecarr commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

derekwaynecarr commented Feb 16, 2015

smarterclayton commented Feb 16, 2015

bgrant0607 Feb 17, 2015

Choose a reason for hiding this comment

bgrant0607 commented Feb 17, 2015

brendandburns commented Feb 18, 2015

smarterclayton commented Feb 18, 2015