A few thoughts on the structure of this:

• can we separate the dynamic config setup into a separate function/file to make the inputs (config and client) and outputs (controller and resulting config) clearer?
• mutating the config in the KubeletServer object makes this hard to reason about. can you hold the dynamically fetched config locally and propagate it to where it needs to go instead?
• I would expect all the dynamic config lookup to happen after the BootstrapKubeconfig handling... otherwise, bootstrapped kubelets won't have a client yet.

I can split that into another function in server.go.

KubeletServer already gets propagated to where the config is needed, and this concept is deeply threaded through the server.go code. I'm open to discussing whether this is the right flow to have, but I think it's out of scope for this PR - which is really more focused on the "obtain a valid config" part vs. the "pass that config around" part.

I'll look into moving the setup to after the BootstrapKubeconfig handling.

this will fail in TLS bootstrap cases where the kubelet doesn't have a client yet

I'll look into moving this after the TLS bootstrap.

I was under the impression that TLS bootstrap shouldn't depend on the KubeletConfiguration, but who was I kidding, everything depends on the KubeletConfiguration ;). It needs ContentType, KubeAPIQPS, and KubeAPIBurst. This creates a mild chicken-egg problem.

Present "solution" (imperfect) is to do the bare minimum to get a client based on the config supplied at startup and initialize the config controller with this client, then allow the client to be regenerated once we have a validated config.

This still leaves the config controller using the old client for its sync-loop, which means the QPS, etc. parameters might not be uniform between the clients. I'll look into splitting starting the conifg sync-loop out from running the Run() function on the controller, so that any new client can be injected there when it is available.

I was under the impression that TLS bootstrap shouldn't depend on the KubeletConfiguration, but who was I kidding, everything depends on the KubeletConfiguration ;). It needs ContentType, KubeAPIQPS, and KubeAPIBurst. This creates a mild chicken-egg problem.

I don't see those settings being required. From what I can see, the kubelet TLS bootstrap requires this information:

• bootstrap kubeconfig file
• path to write resulting kubeconfig to
• dir to write resulting client certificate to
• name of the node (which requires the following info to determine)
  • cloud provider
  • cloud provider config file
  • hostname override

Those files and paths don't seems like something you'd need to be dynamic, and the name of the node is needed to look up the dynamic config in the first place.

Yeah, you're right. I was erroneously thinking of "get a client" as somehow involved because I saw that path reading the kubeconfig file. It's the client getting that needs the API call config parameters I mentioned above.

The cert dir, cloud provider, and cloud provider config file path, however, are still on the KubeletConfiguration. I wasn't planning on touching the KubeletConfiguration type itself in this PR, since I had another PR for refactoring that... but I suppose it doesn't hurt to remove a few parameters from it that clearly shouldn't be dynamic - especially given it means we won't have to worry about regenerating certs.

Ok those fields are yanked, no cert regen.
Clients are regenerated from latest config so that the QPS, etc. parameters are up-to-date, and then the latest client is injected when starting the sync loop.

I'd still expect an error returned, not an internal panic

If the Kubelet cannot reliably determine a configuration to use, it should refuse to start.

I don't disagree, but that should happen via a bubbling error, not a panic deep in a config library

how do we determine whether a node is permitted to access a particular configmap? nodes create their API objects on startup, if needed. Should they be permitted to create a Node API object that references a configmap config source? That's letting a node escalate their view permissions.

A Node can use the configuration from a particular ConfigMap if its Kubelet can read that ConfigMap. IIUC, the Kubelet registers the Node object. Either the Kubelet has permission to read the config it sets on the Node at that time, or it doesn't, and I don't see how this permission changes when the Node object is created.

I think we should probably allow Nodes to reference config at creation time, so an autoscaler can spin up Nodes that immediately refer to the desired config.

I re-read your scoped Kubelet access proposal (kubernetes/community#585) just now. If the node authorizer takes "related to the requesting node" to include "node -> configmap", then the permissions I just noted are too broad to keep things scoped to just that Kubelet's node. Since that uses specs as sources of scope information, you're correct that there is an escalation path. The node authorizer would have to either disallow Kubelets from creating nodes that initially reference configuration, or have some other source of permissions information for this configuration.

I think this is fixed with the additions to plugin/pkg/admission/noderestriction/admission.go in this PR.

minimum of 2 is odd... why are we required to let the kubelet crash twice?

also, it is currently defaulting to 10

Ah, thanks for that catch on the default vs. the comment.

The Kubelet has to restart at least once to take up the new configuration, and since the curSymlink would have been updated when the new configuration was downloaded, the startups file will contain one restart since that update. So we can't count the restart as a "crash" until we see 2 restarts.

so the kubelet can't tell when it crashed vs when it shut down in an orderly way to pick up a new config?

I don't think there's an existing way to do that.

I sketched one idea out just now:
Have a file that contains a bool - True by default (so the initial Kubelet startup isn't treated as a crash). If the Kubelet is about to exit to refresh its config, it writes True to the file. On restart, only startups where the file is False are recorded, and the Kubelet always resets the file to False after checking whether it should record a startup.

But there are edge cases - if you reboot a node for a legitimate reason (like an upgrade), it will be counted as a Kubelet crash unless you write True to the file as part of your reboot sequence.

I could also just make the threshold non-inclusive (> rather than >=), so the minimum would be 1, which might be less confusing. Just relying on the threshold is an imperfect solution, I agree, but I think it gives users decent wiggle-room without the implementation having to track additional state.

I made it so the minimum can be 1 0, meaning no crashes allowed. I'd like to postpone trying to differentiate crashes from config-exits, as I have a feeling any hasty solution I dream up now will be riddled with edge-cases.

process exiting inside a config library is unexpected

Not in this case. We want the Kubelet to complain mightily if something is messing with its configuration filesystem by refusing to start if it can't reliably determine a configuration.

I wouldn't think of this as a library. It's a core part of the Kubelet startup workflow.

returning an error is more expected and makes testing easier. os.Exit inside a config package is unexpected.

+1, please don't exit here

Ok, I'll see if I can find a better way to plumb errors around. It was much nicer to implement this under the assumption that the Kubelet should just stop when it hits these classes of error, but the point about testing is enough reason for me to change this.

There are a lot of very low-level, very-bad errors that should prevent the controller from continuing, so I'm going to see if I can find a way return these fatal-class errors from the high-level Run() without plumbing everything through return values under the hood - which makes error handling much uglier, and makes it harder to differentiate between "fatal" and "non-fatal" error classes. Maybe something along these lines: https://blog.golang.org/errors-are-values.

In the interest of immediately stopping the controller when things are really really wrong without inserting a bunch of error-handling boilerplate, how do you feel about panics in the very low-level functions that are recovered in Run() and in the sync-loop? These are testable, unlike the fatal errors, allow us to return an error from Run(), and only require a very small change (fatalf calls glog.ErrorDepth and panics instead of calling glog.FatalDepth).

There is precedent in the Go standard library for using panic like this, see https://golang.org/src/encoding/json/decode.go (func (d *decodeState) error(err error), which calls panic, and the recover call in unmarshal, for example).

Changes to the "panic" model are in the second commit, for reference.

I think the initial version of this should define the key we load config from. If we want to decompose into multiple keys later, we'll be able to, but "take the first arbitrary key" limits how we can evolve this in the future.

How does this limit us? I don't see a problem evolving from "take the first arbitrary key" to "take all arbitrary keys". Besides, as long as it's in alpha we can make any breaking change we want.

This evolution will happen alongside #44252, which should be done for 1.8, prior to dynamic configuration exiting alpha.

It means the only direction we can evolve is "take all arbitrary keys", which I'm not a fan of.

Why not? The information we care about is the set of config objects (presently just one) in the data associated with those keys. As long as we get the right set of objects, why should we care about how they're distributed across the keys? We can evolve by adding new object types.

We will always have the schematic requirement that all the correct objects exist in the configuration payload. I'm wary of layering arbitrary schematic requirements on top of this, like requiring specific key names, because it just adds unnecessary complexity and more opportunities for user error.

As long as we get the right set of objects, why should we care about how they're distributed across the keys?

We should start with a well-specified key. It is far easier to relax later than it is to tighten.

We can evolve by adding new object types.

In the simplest example, consider a kubelet built with this PR. Pointed at a configmap containing two keys, one of which is the current config object type and the other is a new config object type, it will fail if it encounters the other unknown object first.

Instead, if it looked for a specific key, then a single configmap could provide a compatible object to kubelets with this PR, and a new config object type to kubelets that knew to look for the appropriate key.

I'm wary of layering arbitrary schematic requirements on top of this, like requiring specific key names, because it just adds unnecessary complexity and more opportunities for user error.

Given the naming requirements around the config map hash/algorithm, I assume a tool will be needed to generate these. It should set the key names correctly.

In general, it wouldn't be safe to roll out newly-typed configuration until all clients are updated to understand it. If we were planning on adding a new config object, we'd have to make it optional, upgrade the Kubelets to understand it, and then roll out the new configurations that use that object.

That said, I see some usefulness to being able to express a transition of configuration across Kubelet upgrades in the ConfigMap. What would you think of requiring the key name match the minimum version of the Kubelet that should be allowed to use a given configuration, e.g. 1.7.0, or a key expressing the version of the API group that the config came from, e.g. kubelet.v1alpha1? A tool could definitely be created to understand these relationships.

Alternatively, we could just glue each config object to a key name, and Kubelets could just ignore keys they don't understand.

For this PR, I'll just require the config live in a key called kubelet.v1alpha1, and we can keep discussing this as the feature develops.

Alternatively, we could just glue each config object to a key name, and Kubelets could just ignore keys they don't understand.

That is what I expected, and seems more in line with how most APIs behave

Ok. I'll actually reduce it all the way back to the kubelet key that was originally in the proposal, so we don't commit to any scheme beyond that.

Can we bump this up to 10 for now until we resolve #50216? Otherwise, this could be very dangerous for anyone attempt to use this feature end up to no good kubelet config.

Done