There is no atomic update guarantee right? Apiserver can crash after updating the configmap but before updating AdmissionControlConfiguration.
We should be fine if we always update the configmap first.

Yeah, apiserver should always update the configmap first.

No, apiserver would check for changes and rebuild the configmap on startup if necessary.

should be an enum, not a string?

a subresource does not make sense without a resource? how does this work?

should retry be in at all? if yes, should it be in our first implementation?

the aggregator uses a service instead of directly listing an address.

This cannot include a client cert. Not as a file path, that doesn't make sense in a dynamic config object (won't work in envs where users don't have access to apiserver disk, which is half of the point of the dynamic config).

Instead, we have to make available a CA that the webhook trusts.

Keep this. Take a look at the aggregation registration api types.

a per-resource per-initializer timeout seems extreme. Better to have a global timeout?

s/predicate/predict/

No, apiserver would check for changes and rebuild the configmap on startup if necessary.

Thanks for the clear specification of the options. IMO 2 is an optimization of 1, we should see if 1 is really problematic before doing the extra work. I would guess that an extra trip to etcd is going to be an insignificant amount of latency compared to the initializers/webhooks themselves. @smarterclayton agree?

@wojtek-t This is as good a place as any to ask about how this is going to impact our SLO, whether the 1 second return SLO will still make sense for create requests.

@lavalamp - thanks for looping me in.

So I added a question about #2: https://github.com/kubernetes/community/pull/611/files#r116245850 which I think I don't understand.

The additional read from etcd for every create request sounds problematic, but we need to do some math to compute how many QPS it will generate.
I think that the only thing we are afraid about it latency of the call to etcd. We probably should care mostly about larger clusters. So let's say we are targeting 100pods/s throughput. So there will be O(100) create requests then.
In 5000-node clusters, we have 500 writes/s of just Node updates, so it's not that in such clusters we will have 10x more requests - it will be more like 10-20%. Still a lot though.

@smarterclayton i changed the comment. Does it match what you have in mind?

How is API group specified?

Updated to include the group name.

OK, I still think this needs to be the same as the api here https://github.com/mbohlool/kubernetes/blob/54fb3a6e7a3dc88bfb4f9cd1e9ec6d7290d016d6/staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1/types.go

I had assumed that we would want to allow components to register initializers independently. I.e. the component that provides "foo" initializer might be completely decoupled from initializer "bar", and therefore want to register in a decoupled way. That however assumes that two components can lazily define "before" and "after".

I think in general, most initializers don't care about before or after, and simply want a chance to fire. For those that do... what should we do?

I thought we would never trust initializers to register themselves. I thought it would be a human admin who wrote ACC. A single careless initializer could destroy the carefully ordered list of initializers, right?

I'm just thinking that most initializers are going to be like 3rd party controllers. They're independently delivered. If we have one resource, the install steps for those controllers are going to be ugly patch statements. If we have multiple resources, we'll have to declare dependencies.

Sure. In the future, will we want to enforce order among webhooks? If not, we can make the webhook part of ACC multiple instances.

I think webhook "phases" or "before or after" are inevitable.

I think a single ACC is much easier if both intilaizers and webhooks will have order.

If the initializer and webhook wish to do self registration, they can specify partial order with the latest strategic merge patch syntax.

What would that look like?

-1 for using PATCH for installation.

I think we probably eventually want the user to POST all the initializers/webhooks, and then a controller assembles the ordered list. However I think the thing Chao has here will be a lot easier to implement in the remaining time. Having this list here also lets us push off modeling the dependency graph into the future. So I propose to keep it as is for 1.7, even though it means installing these things will be a bit harder.

According to https://github.com/kubernetes/community/pull/537/files, it would look like:

resourceInitializers:
  - resource
    $setElementOrder/initializers:
    - existing_initialier_a
    - existing_initialier_b
    - new_initializer
    - existing_initialier_c
    initilizers:
    - name: new_initiailizer
      failurePolicy: "ignore"

I'm ok with harder for now.

Fairly expensive today.

Thinking about this... watch from within the same etcd server is effectively free. We need to guarantee that the clock time for any given ACC is within some bound (because an admission controller can be arbitrarily delayed between when the data is read from etcd and when it applies the rules, so a hard boundary is probably too hard).

The scenario of greatest concern is when an apiservers watch client is delayed relative to the mutation operations being executed, perhaps due to a network hang. If we had a single etcd watch on a set of key ranges, our minimum bound is:

The most recent admission control configuration delivered on the same watch channel as any returned event on that watch channel must be no older than X seconds after the last update operation on this server was performed, otherwise a consistent read must be performed.

That can degrade to just performing a consistent read from each api server every X seconds and delaying / rejecting requests until the read returns.

Said another way, our etcd client simply has to ensure that the open watch channel from the server that also watches ACC has delivered ANY event within the time window we care about.

I don't know if the etcd client and server preserve the guarantee that would be necessary for the optimization there though. The client has to construct a watch, and the etcd server has to guarantee a "delivered before" behavior over the connection (event on ACC key has to be delivered before another key with higher index). gRPC substreams (one for each watch) might not support that if two watches are in different sub streams because the packets for the one stream can be delayed.

Or the api server write to the endpoints object has to track the member observed high etcd index and delay writes if other members of the cluster have higher observed indices (if the watch cache value is higher than the read value of the GET on endpoints)

I kind of like the upper bound and the degrading thing. Will make e2e tests slow though.

So from first principles:

1. admin must be able to know when config is applied
2. admin can always restart an apiserver and guarantee it sees the latest config (fallback)
3. admin needs to be able to ensure config eventually applies within some bound

Those are good goals for an alpha api. To just do 3, could we:

1. do a consistent read every 30s
2. if the consistent read fails, block the admission plugin (fail) until it succeeds

Tests would have to get clever about verifying the config is working. If we're concerned, we can simply add a watch on the config objects and always use the latest, but the consistent read must succeed every 30s. That would allow tests to work most of the time.

Why is 2 an explicit goal? That sounds very easy to achieve.

Sure. I'll add this option to the doc, and let's see if we are on the same page.

I just mean as - it's the thing that they have to do today, and it's worked so far. So it's ok to say "you must restart your masters to guarantee the latest config applied", which is probably reasonable.

Got it.

@smarterclayton - got it. Thanks a lot for explanation. That makes perfect sense to me.
I added a potential improvement for it in #611 (comment)

Sorry, I think I don't understand this option. Where is the "resourceVersion of the ACC" coming from? Do we still get it from etcd?
If not (just from local cache), I don't understand how it would work e.g. in HA setup.

The apiserver that handles the ACC update will persist the updated ACC's rv in a configmap. When updating the rv, the apiserver needs to ensure its increasing.

@wojtek-t how do you like this approach suggested by @smarterclayton ? It should be much lighter.

Let's start out with this option. Seems way easier. It is not super important that these things get applied instantly.

@caesarxuchao - I'm completely fine with this option.
I would even go step further, and do:

• let's do consistent read every 1s
• if there wasn't any successful read over last 5s (or 10s) then block everythin

1qps will not be visible and this will give us some tolerance on unsuccessful reads (which may happen from time to time).

WDYT?

@wojtek-t it sounds good. I'll incorporate it to the doc.

I would say-- if multiple ports are exposed, it will use 443 if available and be an error condition otherwise. If only a single port is exposed it can just use that.

Let's start out with this option. Seems way easier. It is not super important that these things get applied instantly.

s/bug/but/

this is unclear to me-- is the first step going to create things fail-open or fail-closed?

(I think we should do fail-open for 1.7 and add fail-closed behavior later when this feature grows up)

As long as it is explicitly specified that it is fail open

@smarterclayton do you agree with adding FailurePolicy to initializers? It's not in your original proposal #132.

Use case here is "the system doesn't grind to a halt"?

Who in this scenario reaps these? Without this, initializers don't need a controller (or we can avoid it). With it, we must have one.

Use case here is "the system doesn't grind to a halt"?

Yes.

Yes, we'll need a controller. I thought we could let apiserver do the reaping, but lavalamp pointed out apiserver could crash.

Or we can say initializers always fail closed for 1.7.

Section for future work here?

Does a per-resource timeout make sense?

Does the timeout need to be customizable at all? What bad thing happens if we hard-code 30s?

Will we ever have an initializer that takes longer than 30s?

We can start with a global default of 30s. In Clayton's proposal i don't see any existing mutating admission controller that's going to take longer than 30s.