-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KEP: Namespace Population #2177
Conversation
14c552f
to
d3fb413
Compare
/cc @bgrant0607 |
|
||
Using external admission webhook increases the complexity, and also latency. | ||
As the webhook must be reliable all the time, it must be deployed in HA. | ||
It's recommended to create a built-in admission controller for namespace initialization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this statement is true. I certainly don't think we should put this admission controller in kube/kube if it can be built with an external admission controller, which I believe it can.
I'm on the fence about whether this feels like a part of Kube or not, but if this is a controller and a CRD and is plugging in to extension, I don't see anything that requires built in web hook admission yet, certainly not until it graduates from beta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with building an external webhook. I will remove this statement.
individually. | ||
No dynamic objects (generating the object definition during population, | ||
e.g. string substitution) is supported - all objects are defined statically, | ||
except `$(CREATOR)` and `$(NAMESPACE)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reference prior proposals here on templating. This is still somewhat problematic - you'll need to define what the rules are here, what fields it can be applied to, what happens when it is applied incorrectly (like to an array or to a string key).
labels: {} | ||
# annotations to be injected into all objects defined in templates | ||
annotations: {} | ||
templates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This object has some scaling problems - include at minimum a discussion on what would happen at the max limit for templates. Also you'll need to define ordering (is ordering in this significant) in case you didn't already and I just missed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bgrant0607 had suggested that we could put the namespace templates in kube-system
namespace. In that case we could use ResourceQuota to protect against creating too many. (His suggestion wasn't related to quota, but I think it would allow quota.) AIUI intention is that only cluster admin can create namespace templates, since they are part of the security profile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crazy idea: what if the template was a namespace? I.e. have some way of indicating that a namespace is a template and shouldn't actually be instantiated, and then clone all its objects into new namespaces when the template applies.
EDIT: oops, I see this was already proposed below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details in Apply the template section. Will update.
# excludes lists names of namespaces which must be excluded from being applied | ||
# with current namespace template. | ||
excludes: [] | ||
common: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not do this - it's too clever. Users can easily duplicate labels and be precise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was for other components to specify labels they are interested in when integrating namespace population mechanism, not for users. I will remove it, as it's not necessary for now.
# labelSelector selects namespaces by labels to populate defined objects | ||
labelSelector: {} | ||
# excludes lists names of namespaces which must be excluded from being applied | ||
# with current namespace template. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't scale at all. I don't know that this is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should also be a label selector, so the semantics are "take all namespaces that match labelSelector
and then remove any than match excludes
"? We have complicated label selection rules in features like node affinity (in, not in, gt, lt, present, absent) and this would be even simpler than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the excludes
. This is a general object selecting problem which should not belong to here. I will call out in Non-Goals.
namespace from `NamespaceTemplate`, it has no idea who created the namespace. | ||
To be able to substitute `$(CREATOR)`, a `MutatingAdmissionWebhook` is involved in namespace | ||
creation request. | ||
It adds an annotation `policy/namespace-creator=name` to the namespace being created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a proper kubernetes namespace, probably "kubernetes.io". I don't see why namespace creation is special here. And I don't know that this is the correct naming "creator" - it probably needs to match the syntax and semantics of the user object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton Would calling this something like authentication.alpha.kubernetes.io/creator
and putting the username, uid, groups, and extra (assuming all of those are available an admission controller) as the value, satisfy what you're asking for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resource types in the proposal was originally targeted under policy
(which doesn't have k8s.io
suffix). Anyway, I can change it to authorization.k8s.io/creator
. This can be changed anyway when people agree on something.
To be able to substitute `$(CREATOR)`, a `MutatingAdmissionWebhook` is involved in namespace | ||
creation request. | ||
It adds an annotation `policy/namespace-creator=name` to the namespace being created. | ||
This annotation is immutable from further update/patch requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty big deal, it means that any naive code that changes the annotation on namespaces will break when this is introduced. I'm not sure we should be doing a one off "this annotation is special" approach without a bit more discussion on next steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protection is necessary. I will update the details with compatibility into consideration.
A newly created namespace is not ready before all objects in `NamespaceTemplate` | ||
are fully populated. | ||
To determine the readiness of a namespace, simply inspect `phase` in `status` | ||
subresource of the namespace. It's ready when the `phase` is `Active`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me whether you are proposing adding a new phase.
A newly created namespace is not ready before all objects in `NamespaceTemplate` | ||
are fully populated. | ||
To determine the readiness of a namespace, simply inspect `phase` in `status` | ||
subresource of the namespace. It's ready when the `phase` is `Active`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are only two phases: active and terminating. Are you saying there will be a third phase?
To determine the readiness of a namespace, simply inspect `phase` in `status` | ||
subresource of the namespace. It's ready when the `phase` is `Active`. | ||
Alternatively, the `NamespaceTemplate` controller is registered as an _Initialization Controller_ | ||
and uses `Initializer` admission control to hide the namespace before it's fully populated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don’t do this (hide ns)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Mentioning this as an alternative. We will not go this path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't change phase (it's an enum, so clients won't expect a new phase). See my comments at the bottom - I think we want to adapt the spirit of initializers (which were designed around this use case) without requiring the existing initializer concept (of which this is the primary use case we have, and we can do it as first class on namespace rather than implicit).
|
||
To avoid the situation, a few mechanisms can be used: | ||
|
||
##### Creating namespace through service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a similar concept in openshift via a feature called projectrequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking with jordan I'm almost convinced we can kill project request off in favor of this, with the security caveats.
users/groups the privilege to create namespaces. | ||
Beyond that, it is the simplest and recommended approach. | ||
|
||
##### Use of Initializers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like just creating namespace through delegated service avoids the need for this complexity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With help of a delegated service, it makes the problem simpler. However that requires the delegated service, with concerns in some use cases:
- The user experience is inconsistent using delegated service, as in different environment/teams/corps, the delegated services are different
- The cluster admin will either spend effort developing their own delegated service or use an existing solution, and they have to educate the users of the cluster about how to create a namespace.
In a self-service namespace creation use case, the users of the cluster want to simply use "kubectl" to create namespaces.
|
||
#### Opt-out | ||
|
||
Cluster administrator is able to opt-out some namespaces from being populated by the controller. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the mechanism described here is going to fit well in existing clusters.
Since this mechanism does nothing to rate limit or control namespace creation under quota, your proposed solution doesn't really solve "defend against malicious creators", so I don't think namespace template should default to covering all namespaces or attempt to solve that problem. Instead, this proposal is about "automated defaulting", and so it needs to be opt in by the user specifying a label. If the administrator sets a namespace template label that intersects with a system namespace, that's the administrator's fault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with opt-in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be misunderstanding, but I believe that making this opt-in runs counter to the goal here, which is to allow the cluster admin to force policies on new namespaces.
Of course obviously it's opt-in in the sense that if the cluster admin decides not to use the feature, then nothing happens, i.e. no change from today.
It seems that your main objection to this proposal is that it does not address quota for namespace creation. Am I understanding correctly?
The mechanism is agnostic to object types as long as they are namespace scoped. | ||
The mechanism works in a single cluster. | ||
|
||
### Non-Goals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to add as a non-goal (because this solution can't provide it)
- Defend against namespace exhaustion or malicious namespace creators.
and any elements of the proposed solution that overlap should be dropped. Instead it is a defaulting solution that could be composed with a future change that defends namespace creation itself via some pattern or hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-service namespace creation is a much harder problem than this proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton IIUC when you say "Self-service namespace creation is a much harder problem than this proposal" you are meaning "Self-service namespace creation with certain properties that this proposal does not provide is a much harder problem than this proposal."
Assuming that's what you meant, could you be specific what other properties you had in mind, other than namespace exhaustion? What did you mean by "malicious namespace creators" -- are those malicious namespace creators trying to do something other than namespace exhaustion (for example do you see a way they would be able to create namespaces that don't get populated with the requisite policy objects, or that would allow them to later remove/modify those policy objects)?
|
||
#### Self-serviced namespace creation | ||
|
||
Self-serviced namespace creation allows certain users/groups to create namespaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this proposal as written can correctly protect against this, so it either needs to be dropped, split, or a lot more work put into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton What is the "this" you're referring to where you say "protect against this"? I think you are referring to something that is not in this sentence, but I wasn't sure exactly what.
Certainly people are doing self-service namespace creation today on stock upstream Kubernetes clusters, so I assume you are referring to doing that with some additional guarantees. Can you specify which guarantees you think this proposal does not cover? The one you have mentioned elsewhere is quota on namespace creation. Are there others?
|
||
## Graduation Criteria | ||
|
||
Namespaces can be securely populated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something this proposal doesn't provide, so this needs to be rethought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton Is there an aspect of security that is missing other than quota on namespace creation?
@smarterclayton could you please clarify:
If yes, then the core API change is required because of adding Initializers field in NamespaceSpec. |
For clarity, we have generally agreed to remove alpha generic initializers
because they don’t offer enough value over webhooks. This is a more
targeted namespace-specific lifecycle which is consistent with the special
nature of namespaces as a “ACL bucket” that doesn’t apply to other objects.
I suppose we could preserve the generic field, but I might argue that each
object has its own lifecycle (pods vs ns vs nodes) and so init containers
vs namespace initialization are both domain specific. Also, the things
that use objects are inextricably linked to how they are initialized - a
Kube proxy behaves one way with services as opposed to how an ingress
behaves with secrets.
|
I'll open a PR to the initializers proposal to describe the current state and that we desire to deprecate and remove the feature as not having enough value relative to web hooks to justify the additional complexity. I think this proposal covers the core use case we do care about (namespace initialization). In the short term, we could use the alpha initializers field in metadata to test this, and then if we reach beta on it we can move the field out of metadata and into namespace ( Or we could leave the initializers field present, and remove the infrastructure that supports hiding the objects from end users. We would still have a namespace finalizers field, but others could use |
In any case, I think it makes a ton of sense to make this KEP explicit in what the changes to "core" kubernetes are and what is being built outside. I'd suggest perhaps even splitting it into 2 KEPs to ensure that the "core" features can stand on their own. |
/ok-to-test |
aa13f64
to
dc7e429
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Updated to explicitly call out this proposal doesn't include anything related to changes in core Kubernetes which should be discussed in a separate proposal. |
/wg policy |
Can we be explicit about:
I am especially unclear about no. 2 and 4. It's difficult to evaluate this from a policy management perspective without a better understanding of the exact problem. re @easeway |
Requires FeatureGate Initializers to be on: |
dc7e429
to
4b8eab5
Compare
@ericavonb Updated motivation section |
@smarterclayton @jbeda Separate KEP filed containing core Kubernetes changes: #2543 and this KEP doesn't contain anything requiring changes in core Kubernetes. |
/kind kep |
Now that #2543 is in flight, what does that mean for this KEP? |
@mattfarina #2543 is a dependency of this KEP. #2543 is for core Kubernetes changes required by this feature and this KEP focuses on non-core Kubernetes changes. This can't land without #2543. |
REMINDER: KEPs are moving to k/enhancements on November 30. Please attempt to merge this KEP before then to signal consensus. Any questions regarding this move should be directed to that thread and not asked on GitHub. |
KEPs have moved to k/enhancements. Any questions regarding this move should be directed to that thread and not asked on GitHub. |
@justaugustus: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Summary
Namespace Population is an automated mechanism to make sure the predefined policy objects
(e.g. NetworkPolicy, Role, RoleBinding) are present in selected namespaces.
Motivation
Kubernetes users create namespaces (self-service namespace creation) and want to
ensure security/isolation policies are enforced between them and other users.
Just like addon manager creates cluster-scope policy objects like PodSecurityPolicy,
ClusterRole, ClusterRoleBinding at cluster creation time.
It creates namespace-scope policy objects during namespace creation time.
This is part of Security Profile proposal
/sig auth
/sig api-machinery
/sig cluster-lifecycle
/cc @davidopp
/cc @tallclair
/cc @ericchiang
/cc @liggitt
/cc @bgrant0607
/cc @roberthbailey
/cc @lavalamp