Enforce namespace unique constraints #1564

derekwaynecarr · 2014-10-03T15:52:19Z

This is the update to actually enable namespace support.

At the moment, I pass a namespace as a query parameter in order to support namespace scoping on the v1beta urls. Once v1beta3 lands, I will modify to pass namespace in the path. Note the client libraries were modified to make this easy to do, so should be a trivial change once ready.

I broke the commits into logical sections to improve readability.

derekwaynecarr · 2014-10-03T16:35:17Z

Not sure why Go 1.2.2 is showing an error, but Go 1.3 is building and passing Travis without issue.

smarterclayton · 2014-10-03T16:45:37Z

Probably using a Go 1.3 specific feature

derekwaynecarr · 2014-10-03T19:44:13Z

Travis flaked, all things pass now.

erictune · 2014-10-06T17:55:27Z

Have a test that sets and gets a non-default namespace? Is hack/test-cmd.sh the right place to add that?

erictune · 2014-10-06T18:03:23Z

I can see three possible "features" that require some notion of a "default namespace":

the one you use for your pods, etc if you don't bother to explicitly create and reference another namespace.
the value of the Namespace property you use when you want to operate on cluster-wide resource (one which is not naturally associated with a single namespace) such as minions or (not nested) namespaces.
the Namespace of authorization policies that affect all namespaces, or are about namespaces (not really defaults, since they probably have precedence over policies that are set in specific namespaces.

Do you see "default" being used for all of those? Not something that has to be determined in this PR, but something to think about.

erictune · 2014-10-06T18:11:04Z

pkg/apiserver/resthandler.go

 	sync := req.URL.Query().Get("sync") == "true"
 	timeout := parseTimeout(req.URL.Query().Get("timeout"))
+	// if a namespace if specified, it's always used.
+	// for list/watch operations, a namespace is not required if omitted.


I think it will seem odd for the "average" user that normal GET and POST operate only on one namespace, but list and watch can operate on all namespaces. The things that need to list/watch all objects are special cases like controllers. I wonder if we should put the burden on those special cases to explicitly say they want to see all namespaces.

To that end, what if namespace is required to be specified for list and watch, but you can optionally specify "__ALL" in the namespace parameter to match all namespaces? What do you think?

Also thinking about the three different types of "default" namespaces and whether those should have their own special identifiers.

My take on this is its less of a concern when we move away from the query parameter and instead namespace is in the resource path.

erictune · 2014-10-06T18:39:11Z

one minor addition to a test is requested. LGTM after that.
Can wait for future PR for changes to "default" values, if any.

derekwaynecarr · 2014-10-06T18:49:52Z

Made following updates:

Cleaned up makeEtcdKey per @smarterclayton request to makeEtcdListKey and makeEtcdItemKey
Added unit test for kubecfg LoadNamespaceInfo per @erictune
Improved code style for resthandler.go and default namespace per @smarterclayton request
Improved etcd_test.go TestEtcdCreatePodFailsWithoutNamespace per @erictune

Thanks for feedback.

erictune · 2014-10-07T00:06:38Z

LGTM. One tiny suggestion. And please squash for merge.

derekwaynecarr · 2014-10-07T15:10:32Z

@erictune - squashed commits at appropriate boundaries. made last requested change.

smarterclayton · 2014-10-07T15:13:20Z

Hold on merge until we close the discussion about this requiring downtime and recreation of all resources.

derekwaynecarr · 2014-10-07T15:25:01Z

My preference is to not migrate existing resources into the new etcd key-paths, but I do think we need to start the discussion on how we can do both on and offline migration of etcd content in the future in case we move resources again.

In reviewing the following from @thockin, it appears that services v2 is taking a similar statement here: https://groups.google.com/forum/#!topic/kubernetes-announce/oO7Bs_c16lI

In a future PR, I think its imperative we can ask a registry object what schema version it is running [ which is different than api version ], and that we can ask a registry object to go from schema version N to N+1.

thockin · 2014-10-07T18:15:31Z

If a live update is doable without crippling the feature (which services v2
is not) and not too hard to do (say O(2 days) of eng time), we should
consider it.

But at this point it sounds like we have a few such things which together
add up to more than O(2 days) work to live migrate AND we're early enough
that losing momentum is worse than asking customers to nuke.

On Tue, Oct 7, 2014 at 8:25 AM, Derek Carr notifications@github.com wrote:

My preference is to not migrate existing resources into the new etcd
key-paths, but I do think we need to start the discussion on how we can do
both on and offline migration of etcd content in the future in case we move
resources again.

In reviewing the following from @thockin https://github.com/thockin, it
appears that services v2 is taking a similar statement here:
https://groups.google.com/forum/#!topic/kubernetes-announce/oO7Bs_c16lI

In a future PR, I think its imperative we can ask a registry object what
schema version it is running [ which is different than api version ], and
that we can ask a registry object to go from schema version N to N+1.

Reply to this email directly or view it on GitHub
#1564 (comment)
.

derekwaynecarr · 2014-10-07T18:59:20Z

The migration process for this would require the following:

Resources Affected:

Pods
ReplicationControllers
Services
ServiceEndpoints

To perform a migration, we would need to do the following:

Stop services on kubernetes-master (apiserver, controller-manager, scheduler)
Run some undefined script from somewhere that modifies etcd as follows:

Move resources
a. /registry/pods to /registry/pods/default
b. /registry/controllers to /registry/controllers/default
c. /registry/services/specs to /registry/services/specs/default
d. /registry/services/endpoints to /registry/services/endpoints/default

Update each resource in above and ensure:
resource.Namespace = "default"

Update kubernetes-master to latest source
Start apiserver, controller-manager, scheduler

derekwaynecarr · 2014-10-08T14:29:01Z

Rebase completed to update changes to api.JSONBase becoming api.TypeMeta

smarterclayton · 2014-10-08T15:34:15Z

pkg/tools/etcd_tools.go

-		if err != nil {
-			return err
+		if node.Dir {
+			h.decodeNodeList(node.Nodes, slicePtr)


Can you return here to avoid the deep else?

@smarterclayton No. It's possible in the future that we will have a node structure in etcd where a directory and a node exist at the same depth in the hierarchy, so we need to process all non-directory sibling nodes.

For example:

/key/registry/foo/bar <- Node.Dir=false
/key/registry/foo/abc <- Node.Dir=true
/key/registry/foo/abc/def <- Node.Dir = false

calling List nodes recursively on /key/registry/foo would need to return {bar,def}

I meant continue

Sure thing - update complete.

----- Original Message -----
From: "Clayton Coleman" notifications@github.com
To: "GoogleCloudPlatform/kubernetes" kubernetes@noreply.github.com
Cc: "Derek Carr" decarr@redhat.com
Sent: Wednesday, October 8, 2014 1:36:47 PM
Subject: Re: [kubernetes] Enforce namespace unique constraints (#1564)

@@ -139,16 +145,20 @@ func (h *EtcdHelper) ExtractList(key string, slicePtr interface{}, resourceVersi
}
v := pv.Elem()
for _, node := range nodes {

obj := reflect.New(v.Type().Elem())

err = h.Codec.DecodeInto([]byte(node.Value), obj.Interface().(runtime.Object))

if h.ResourceVersioner != nil {

_ = h.ResourceVersioner.SetResourceVersion(obj.Interface().(runtime.Object), node.ModifiedIndex)

// being unable to set the version does not prevent the object from being extracted

}

if err != nil {

return err

if node.Dir {

h.decodeNodeList(node.Nodes, slicePtr)

I meant continue

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

smarterclayton · 2014-10-08T15:34:44Z

One last comment, then LGTM

derekwaynecarr · 2014-10-09T21:06:33Z

rebased again.

derekwaynecarr · 2014-10-10T15:58:23Z

rebased again.

lavalamp · 2014-10-10T18:13:22Z

I would like to look at this change, please hold for a bit. ETA 4 hours

lavalamp · 2014-10-10T18:16:48Z

pkg/client/request.go

@@ -74,6 +74,15 @@ func (r *Request) Sync(sync bool) *Request {
 	return r
 }

+// Namespace sets a namespace parameter on a request based on the value in the context
+func (r *Request) Namespace(ctx api.Context) *Request {


Is there a reason why this function takes an api.Context? That is for tracking client requests server side. There is not much sense forcing clients to use it, no? I think this should just take a string. I also think it's something you should set on the client object, not the request object-- if you're doing namespaced operations, you probably want many in a row with the same namespace.

Very sorry for not noticing this earlier, I was out all last week and have just been catching up this week.

I had the same argument earlier - derek had something that half convinced me (convenience for client code writers), although that may be solvable. Derek, sway me back??? :)

On Oct 10, 2014, at 2:17 PM, Daniel Smith notifications@github.com wrote:

In pkg/client/request.go:

@@ -74,6 +74,15 @@ func (r *Request) Sync(sync bool) *Request {
return r
}

+// Namespace sets a namespace parameter on a request based on the value in the context
+func (r *Request) Namespace(ctx api.Context) *Request {
Is there a reason why this function takes an api.Context? That is for tracking client requests server side. There is not much sense forcing clients to use it, no? I think this should just take a string. I also think it's something you should set on the client object, not the request object-- if you're doing namespaced operations, you probably want many in a row with the same namespace.

Very sorry for not noticing this earlier, I was out all last week and have just been catching up this week.

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

Is there a reason why this function takes an api.Context? That is for tracking client requests server side. There is not much sense forcing clients to use it, no?

If you just think about the kubecfg client or the end-user of kubernetes, then sure, you tend to work in a single namespace, and this argument holds. In fact, its how I first wrote the code, but if you take a step back, you realize the majority of our existing clients are server-side and need to operate across namespace boundaries to perform their operation. It then becomes a matter of wanting to make it easier for those clients to operate.

The replication-controller and scheduler both follow a similar pattern:

initialize the daemon with an interface X that is subset of kubeclient

run a sync loop

List/Watch over all name-spaces using X to resource type Y

In response, do some series of CRUD operations in Y.Namespace

In OpenShift Origin, we have a similar pattern for build-controllers and deployment-controllers. I suspect others may choose to write plug-ins that follow the same convention to meet a particular need.

If we kept context on Client as a means of passing namespace around, you are left with the following when looking at the above style-use case.

A. Create new client objects when you change namespace boundary
B. Require the ability to mutate a context on a client interface

(A) seems like an anti-pattern when its an obvious control loop in our internal usage of Kubernetes to require so many throw-away clients when we have callers that work in multiple namespace boundaries.

(B) has issues with concurrency because we share the same client, and so we would need locking, and things get ugly quick.

That left me with (C) pass context as argument - which lets me reuse the client object, and keep clean enough reading code after actually writing all the other options for callers that work on resources across namespace boundaries.

As for this particular function taking a api.Context object instead of a string, it actually keeps the LIST/WATCH operations clean when working in the no namespace-boundary versus passing the empty string as input around.

It should be possible to override the namespace on a particular request, if necessary. Just like we have defaults for other things, the client has a namespace setting which requests get by default, but if you need to change the namespace for a particular request, you can.

The common client functions should be able to get the namespace out of the object, anyway, you shouldn't have to pass it again in a context. I can't think of a reason why server components should even need to reference namespaces, other than perhaps copying it from a {replicationController,pod} to a {pod,binding}.

As long as we agree with override on a particular request, then we get what we both want, and then its a manner of finding the best way to provide that override into the operation. Can we evolve this in a subsequent PR and not hold up this support? I know there are many things waiting on this feature making it in from our end, and it seems like this type of thing can get further discussion after the fact.

The thing I want is for this to not take an api.Context. There's no reason not to take a string, is there?

(also, all other client methods shouldn't take api.Context, but I won't block this PR on that.)

Ok, I can update Namespace to take a string, and callers will do things like the following for now:

err = c.Put().Namespace(api.Namespace(ctx)).Path("services").Path(svc.ID).Body(svc).Do().Into(result)

Will open a separate PR to discuss alternate approaches to providing an override to client context in a thread-safe manner.

I can live with that for the moment, thanks.

Actually we can use #1708 to discuss possible alternative solutions now that you understand the current requirements I worked against.

derekwaynecarr · 2014-10-10T21:24:15Z

Updates requested by @lavalamp completed.

smarterclayton · 2014-10-12T03:59:46Z

LGTM

erictune · 2014-10-14T17:58:53Z

Still LGTM. Not merging because my understanding is that this is to be merged on the "big breakage day", which is not chosen yet. Assigning to @thockin who I think will do the merges on that day.

lavalamp · 2014-10-14T18:14:24Z

pkg/api/context.go

@@ -71,3 +77,12 @@ func ValidNamespace(ctx Context, resource *TypeMeta) bool {
 	}
 	return ns == resource.Namespace && ok
 }
+
+// WithNamespaceDefaultIfNone returns a context whose namespace is the default if and only if the parent context has no namespace value


What is the difference between the default and not being set?

The default Namespace is literally, Namespace="default".

It is the assumed namespace context when calling the legacy API patterns where namespace was not in the resource path.

A namespace not being set means the operation context has no namespace scope at all. It is useful for LIST and WATCH operations when working across namespace boundaries.

I find this really confusing. Is it too late to ask for a system where "" == "default", and if you want to span namespaces, you have to explicitly say namespace "*"?

From a REST perspective, GET /pods returning all pods seems more correct than GET /pods/ns/* returning all pods.

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

@@ -71,3 +77,12 @@ func ValidNamespace(ctx Context, resource *TypeMeta)
bool {
}
return ns == resource.Namespace && ok
}
+
+// WithNamespaceDefaultIfNone returns a context whose namespace is the
default if and only if the parent context has no namespace value

I find this really confusing. Is it too late to ask for a system where "" ==
"default", and if you want to span namespaces, you have to explicitly say
namespace "*"?

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

Yeah, I'm not arguing about the REST path, I'm arguing about not using the go type's default value of "" to mean the default. Perhaps we need to make a custom type to represent namespaces.

Fine with me, we can merge and fix up later.

Final answer: is this PR done enough to commit tomorrow?

@lavalamp @derekwaynecarr @smarterclayton

On Wed, Oct 15, 2014 at 10:54 AM, Daniel Smith notifications@github.com
wrote:

In pkg/api/context.go:

@@ -71,3 +77,12 @@ func ValidNamespace(ctx Context, resource *TypeMeta) bool {
}
return ns == resource.Namespace && ok
}
+
+// WithNamespaceDefaultIfNone returns a context whose namespace is the default if and only if the parent context has no namespace value

Fine with me, we can merge and fix up later.

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

I vote yes

Sent from my iPhone

On Oct 15, 2014, at 4:29 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/context.go:

@@ -71,3 +77,12 @@ func ValidNamespace(ctx Context, resource *TypeMeta) bool {
}
return ns == resource.Namespace && ok
}
+
+// WithNamespaceDefaultIfNone returns a context whose namespace is the default if and only if the parent context has no namespace value
Final answer: is this PR done enough to commit tomorrow? @lavalamp @derekwaynecarr @smarterclayton
…
—
Reply to this email directly or view it on GitHub.

yes

On Wed, Oct 15, 2014 at 1:34 PM, Derek Carr notifications@github.com
wrote:

In pkg/api/context.go:

@@ -71,3 +77,12 @@ func ValidNamespace(ctx Context, resource *TypeMeta) bool {
}
return ns == resource.Namespace && ok
}
+
+// WithNamespaceDefaultIfNone returns a context whose namespace is the default if and only if the parent context has no namespace value

I vote yes
… <#149158515c230ada_>
Sent from my iPhone
On Oct 15, 2014, at 4:29 PM, Tim Hockin notifications@github.com
wrote: In pkg/api/context.go: > @@ -71,3 +77,12 @@ func ValidNamespace(ctx
Context, resource *TypeMeta) bool { > } > return ns == resource.Namespace
&& ok > } > + > +// WithNamespaceDefaultIfNone returns a context whose
namespace is the default if and only if the parent context has no namespace
value Final answer: is this PR done enough to commit tomorrow? @lavalamp
https://github.com/lavalamp @derekwaynecarr
https://github.com/derekwaynecarr @smarterclayton
https://github.com/smarterclayton … — Reply to this email directly or
view it on GitHub.

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

derekwaynecarr · 2014-10-16T15:20:58Z

Rebased branch to latest.

derekwaynecarr · 2014-10-16T15:45:50Z

Forced a push to try to fix Travis flake on go 1.2, where go 1.3 worked. Go 1.2 was reporting an error reaching etcd in integration tests.

thockin · 2014-10-16T15:46:59Z

I will kick travis until i get a green or get tired.

On Thu, Oct 16, 2014 at 8:46 AM, Derek Carr notifications@github.com
wrote:

Forced a push to try to fix Travis flake on go 1.2, where go 1.3 worked.
Go 1.2 was reporting an error reaching etcd in integration tests.

Reply to this email directly or view it on GitHub
#1564 (comment)
.

…and server namespace aware

thockin · 2014-10-16T17:13:49Z

Thar she blows!

…_constraints Enforce namespace unique constraints

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch 2 times, most recently from 0994cbf to 197c7ac Compare October 3, 2014 16:12

bgrant0607 assigned erictune Oct 3, 2014

bgrant0607 added the area/security label Oct 3, 2014

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 197c7ac to f9c3e19 Compare October 6, 2014 16:51

erictune reviewed Oct 6, 2014
View reviewed changes

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from f4330d0 to 998a549 Compare October 6, 2014 18:47

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 998a549 to 7fdc31f Compare October 6, 2014 20:23

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 7fdc31f to 1764a03 Compare October 7, 2014 15:09

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 1764a03 to e7ceb34 Compare October 7, 2014 15:26

pmorie mentioned this pull request Oct 8, 2014

Add api.Context to client openshift/origin#186

Closed

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from e7ceb34 to f2ca39a Compare October 8, 2014 14:28

smarterclayton reviewed Oct 8, 2014
View reviewed changes

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from f2ca39a to e72bf13 Compare October 8, 2014 17:42

derekwaynecarr mentioned this pull request Oct 9, 2014

Kubelet can't serve pod info for non default namespace hosts #1688

Closed

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 4147679 to 567bb1f Compare October 9, 2014 21:06

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 567bb1f to 2132c11 Compare October 10, 2014 15:58

derekwaynecarr mentioned this pull request Oct 10, 2014

Why do we have context in client's methods? #1708

Closed

lavalamp reviewed Oct 10, 2014
View reviewed changes

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 2132c11 to 5e8d0bc Compare October 10, 2014 21:24

erictune assigned thockin and unassigned erictune Oct 14, 2014

lavalamp reviewed Oct 14, 2014
View reviewed changes

erictune mentioned this pull request Oct 15, 2014

Add event creation library and implement in scheduler. #1789

Merged

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from 5e8d0bc to bd85baa Compare October 16, 2014 15:20

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch 2 times, most recently from a28f129 to f4bf740 Compare October 16, 2014 15:45

thockin mentioned this pull request Oct 16, 2014

Rackspace - Switch to CoreOS for standard cluster #1832

Merged

derekwaynecarr added 2 commits October 16, 2014 13:02

Extract list must flatten nodes across directories

b63974b

Enforce unique constraint at namespace boundary in etcd, make client …

085ca40

…and server namespace aware

derekwaynecarr force-pushed the enforce_namespace_unique_constraints branch from f4bf740 to 085ca40 Compare October 16, 2014 17:06

thockin added a commit that referenced this pull request Oct 16, 2014

Merge pull request #1564 from derekwaynecarr/enforce_namespace_unique…

8a9c40c

…_constraints Enforce namespace unique constraints

thockin merged commit 8a9c40c into kubernetes:master Oct 16, 2014

derekwaynecarr deleted the enforce_namespace_unique_constraints branch April 17, 2015 17:56

Enforce namespace unique constraints #1564

Enforce namespace unique constraints #1564

Conversation

derekwaynecarr commented Oct 3, 2014

derekwaynecarr commented Oct 3, 2014

smarterclayton commented Oct 3, 2014

derekwaynecarr commented Oct 3, 2014

erictune commented Oct 6, 2014

erictune commented Oct 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erictune commented Oct 6, 2014

derekwaynecarr commented Oct 6, 2014

erictune commented Oct 7, 2014

derekwaynecarr commented Oct 7, 2014

smarterclayton commented Oct 7, 2014

derekwaynecarr commented Oct 7, 2014

thockin commented Oct 7, 2014

derekwaynecarr commented Oct 7, 2014

derekwaynecarr commented Oct 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Oct 8, 2014

derekwaynecarr commented Oct 9, 2014

derekwaynecarr commented Oct 10, 2014

lavalamp commented Oct 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Oct 10, 2014

smarterclayton commented Oct 12, 2014

erictune commented Oct 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Oct 16, 2014

derekwaynecarr commented Oct 16, 2014

thockin commented Oct 16, 2014

thockin commented Oct 16, 2014