KEP: Namespace Initializer

[easeway]

Migrated from kubernetes/community#2543:

I submit this proposal because the generic Initializer is planned to be deprecated, however the mechanism is very useful for container resources like namespace, and I hope we can keep this mechanism, at least specialize it and move it towards beta/GA. If the generic Initializer will move on to beta/GA, I will abandon this proposal.

Note: this proposal requires changes in core Kubernetes, and adding a new field in stable API object.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: easeway
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: calebamiles

If they are not already assigned, you can assign the PR to them by writing /assign @calebamiles in a comment when ready.

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:

• keps/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[thockin]

I think this suffers the same problem that initializers do - it's incredibly complicated.

I much prefer the pattern that is being used in a couple other places - readiness gates. Now, Namespace doesn't have a "ready" field but it does have a Phase, so we could use that. Specifically:

User creates namespace
Admission controller(s) add values to .spec.readinessGates[]
Namespace is not "phase: Active" until .status.Conditions[] contains one condition for each readinessGate with value True.

This is imperfect for a few reasons, but I hope we can work around them:

1. Not sure anyone checks Phase today
2. If a gate becomes False, can the Phase regress? That's weird
3. Nothing stops all WATCH notifiers for resources created inside the namespace, so controllers will process them before NS is Active (probably OK)

[easeway]

I think .spec.readinessGates[] describes better as a desired state and .status.Conditions[] as the actual state, compared to .spec.Initializers which serves something mixed with desired state and actual state.

Extending .status.Phase is simple, however I don't think anyone is checking this because for namespaces, the only values are "Active" and "Terminating", so the clients will assume "Active" when namespace is created.

[easeway]

Another problem this proposal trying to solve is to reduce the complexity and work for implementing a controller initializing the namespaces. That's why NamespaceInitializerConfiguration is inherited from InitializerConfiguration. Without that, the developers have to build an external AdmissionWebHook in addition to the controller, regardless of either updating ".spec.Initializers" or ".spec.ReadinessGates". It becomes much easier for developers if we have a built-in NamespaceInitializers admission webhook to take care of this common task.

[thockin]

@easeway

I don't think anyone is checking this because for namespaces, the only values are "Active" and "Terminating"

This is probably the biggest argument I see, honestly. and I am not sure how to overcome it, but I think it's worth trying.

developers have to build an external AdmissionWebHook in addition to the controller,

I don't buy that building a webhook is a significant burden, especially as compared to the rest of the complexity here.

Does this proposal include the ?includeUninitialized=true part from initializers?

What does it mean for a Foo inside a namespace to be created and "ready" when the namespace is not? Does the Foo appear in WATCH streams? Is the uninitializedness inherited?

[easeway]

We can have a try to define "Pending" (or "Initializing") as a third value for Namespace.Status.Phase.

Does this proposal include the ?includeUninitialized=true part from initializers?

What does it mean for a Foo inside a namespace to be created and "ready" when the namespace is not? Does the Foo appear in WATCH streams? Is the uninitializedness inherited?

The hiding mechanism is not inherited, so there's no includeUninitialized. The detailed discussion thread is here: kubernetes/community#2543 (comment)

I would still like to argue about the burden building a webhook. Because I did that before. There are two major concerns from me if we require the development of a webhook every time the developer wants to build a controller to initialize a namespace:

• The effort developing a webhook purely for namespace initialization is duplicated, and non-trivial, because developing a external admission webhook with good quality requires the following expertise from a developer:

  • Knowledge of TLS and certificate management, as TLS is required from API server
  • Well managed certificate lifecycle. A valid certification will expire maximally in two years, and in a more secure case, the expiry will be shorter, so the developer should implement some mechanism to rotate/refresh certificate properly
  • Security knowledge, the private key of the TLS certificate should be properly managed
  • Scalability issue related to TLS certificate: as described above, the webhook actually has a "state" (the cert/private key) which requires addition effort to make the implementation scalable (changing the "replicas" in ReplicaSet), at least, a new replica is created, the new Pod will have it's own private key, a new cert will be generated (self-sign will be simpler, all other mechanisms will be significantly more complicated), then at least the cert bundle in AdmissionWebhookConfiguration should be updated. Then comes another issue, if a Pod is destroy/recreated, the private key will be regenerated, and the old cert should be removed from cert bundle in AdmissionWebhookConfiguration, and actually this task is very difficult and complicated.

• It unnecessary to add a webhook in API path in a ad-hoc way. Without a built-in implementation, every controller initializing a namespace will be shipped with its own webhook. Thinking about different use cases for namespace initialization, these webhooks are added ad-hoc and introduce multiple hops and delays in API path.

[thockin]

Without hiding, how is this different than simply having controllers configure it async?

We already established that we don't think most code will be paying attention to Phase, so (for example) the user could start running Deployments before initialization is done. How is this different?

[easeway]

Without hiding, how is this different than simply having controllers configure it async?

It's acceptable to have controllers configure it async, based on the clients are responsible for retrying. The problem today, is that there's no way to effectively tell whether the namespace is ready or not.

We already established that we don't think most code will be paying attention to Phase, so (for example) the user could start running Deployments before initialization is done. How is this different?

Here's a link to very early discussion about Phase: kubernetes/community#2177 (comment) and .status.Conditions is the recommended way (as here). If there's a way to tell the readiness of a namespace, it helps a lot for all the clients, especially automation scripts to be simplified and work more effectively, for reasons of:

• The clients can worry less types of errors
  Without telling whether namespace is ready or not, the client has to handle a lot of complex error cases and determine which are worth retry, which should fail fast. And some times it's impossible because the client never knows the details of namespace initialization. An error can be permission denied because RBAC has not been setup, or Pod creation failure if PodSecurityPolicy is turned on, but the rolebinding hasn't been created yet, or others (I feel this can't be determined, based on the system is highly pluggable)
• The wait time of retry can be based on watch, instead of periodically (blindly) polling
  Without a mechanism to tell the readiness of a namespace, the client has to periodically retry the operation. Watch doesn't work, because namespace will never change after being created.

[thockin]

If it is OK to have controllers configure a Namespace async, then we don't need hiding. If we don't need hiding, we should do the simplest possible thing. If we don't want to change Phase (and I buy the argument, but it is arguable), then we should move towards consistency across APIs. The readinessGates pattern is finding favor in a couple places -- I think that is net simpler. We need to make it easy to produce webhooks anyway.

…

On Wed, Dec 5, 2018 at 1:19 PM EaseWay ***@***.***> wrote: Without hiding, how is this different than simply having controllers configure it async? It's acceptable to have controllers configure it async, based on the clients are responsible for retrying. The problem today, is that there's no way to effectively tell whether the namespace is ready or not. We already established that we don't think most code will be paying attention to Phase, so (for example) the user could start running Deployments before initialization is done. How is this different? Here's a link to very early discussion about Phase: kubernetes/community#2177 (comment) <kubernetes/community#2177 (comment)> and .status.Conditions is the recommended way. If there's a way to tell the readiness of a namespace, it helps a lot for all the clients, especially automation scripts to be simplified and work more effectively, for reasons of: - The clients can worry less types of errors Without telling whether namespace is ready or not, the client has to handle a lot of complex error cases and determine which are worth retry, which should fail fast. And some times it's impossible because the client never knows the details of namespace initialization. An error can be permission denied because RBAC has not been setup, or others (I feel this can't be determined, based on the system is highly pluggable) - The wait time of retry can be based on watch, instead of periodically (blindly) polling Without a mechanism to tell the readiness of a namespace, the client has to periodically retry the operation. Watch doesn't work, because namespace will never change after being created. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#645 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVObgPu1S7oyZGWircuKbhot1qsugks5u2DhogaJpZM4Y_rPq> .

[easeway]

If it is OK to have controllers configure a Namespace async, then we don't need hiding. If we don't need hiding, we should do the simplest possible thing. If we don't want to change Phase (and I buy the argument, but it is arguable), then we should move towards consistency across APIs.

Can we agree on the problem: currently there's no way to tell if namespace is ready or not?
And can we agree we do need a solution to effectively tell if namespace is ready or not?

If the answers are YES and YES, we can move on with the details. If either one is NO, we should circle back to discuss about the problem and whether we need a solution.

The readinessGates pattern is finding favor in a couple places -- I think that is net simpler. We need to make it easy to produce webhooks anyway.

IMO readinessGates is a good pattern. @smarterclayton may have more thoughts on this. As this is the discussion from original PR: https://github.com/kubernetes/community/pull/2543/files#r234880870

[thockin]

Can we agree on the problem: currently there's no way to tell if

namespace is ready or not? No. There IS a way. The `phase` exists for that. Whether we like that API or not -- it exists. It MIGHT be in use by some (pedantic) consumers of the system. We can't arbitrarily change that signal. If `phase: Active` doesn't mean that the Namespace is ready to use, we have a breaking change. `readinessGates` was actually designed in the face of a very similar constraint. Pod's "Ready" condition existed and meant that workload controllers could proceed in an update. We absolutely could not break that API, so we changed the inputs to it to include extensible conditions. Phases is harder because it is more enum-like, but honestly I think the risk is lower to changing that API. I think we could add an "Initializing" phase and, in parallel, include a proper design that deprecates `phase` (e.g. add a `ready` status field).

…

On Wed, Dec 5, 2018 at 1:39 PM EaseWay ***@***.***> wrote: If it is OK to have controllers configure a Namespace async, then we don't need hiding. If we don't need hiding, we should do the simplest possible thing. If we don't want to change Phase (and I buy the argument, but it is arguable), then we should move towards consistency across APIs. Can we agree on the problem: currently there's no way to tell if namespace is ready or not? And can we agree we do need a solution to effectively tell if namespace is ready or not? If the answers are YES and YES, we can move on with the details. If either one is NO, we should circle back to discuss about the problem and whether we need a solution. The readinessGates pattern is finding favor in a couple places -- I think that is net simpler. We need to make it easy to produce webhooks anyway. IMO readinessGates is a good pattern. @smarterclayton <https://github.com/smarterclayton> may have more thoughts on this. As this is the discussion from original PR: https://github.com/kubernetes/community/pull/2543/files#r234880870 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#645 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVPIjZwLUqJoT3udqodqnw06x5klDks5u2D0WgaJpZM4Y_rPq> .

[easeway]

Thanks @thockin you made it clear and you are suggesting to add "Initializing" to Phase. I'm OK with that. Leave to other reviewers to comment on this: @smarterclayton, @deads2k, @liggitt, @lavalamp.

Once we agree on this, the rest of the proposal is about how to determine the "Phase".

[lavalamp]

I still haven't looked at this proposal again but I think adding a new value to the "phase" enum is not a backwards compatible change. (contra @thockin) Maybe it's not a big breakage but it definitely would make current documentation about the field flat-out wrong ("A namespace can be in one of two phases").

Is status.phase blank until the namespace lifecycle controller sets it? If so, then current integrations must already handle this case, and I suggest we leave it blank and use a status.initializing boolean to indicate the initialization stage.

If not, I think that "Active" is closer to initializing than "Terminating", so an initializing namespace should be in phase "active" with, again, a status.initializing boolean set to true.

I think I don't need to mention that the current API object violates the general principle of not using enums in cases where there's a chance a new value might get added in the future :)

[justaugustus]

Please remove any references to NEXT_KEP_NUMBER and rename the KEP to just be the draft date and KEP title.
KEP numbers will be obsolete once #703 merges.