KCP: Handle CoreDNS upgrades

[vincepri]

This issue is a follow-up to #2226.

1. relax the validating webhook to allow mutations to the CoreDNS image details in KCP spec
2. add code to KCP controller that syncs the CoreDNS image details from the KCP to the kubeadm-config ConfigMap in the workload cluster
3. add code to the KCP controller that performs a Corefile migration, if necessary
4. add code to the KCP controller that updates the container image in the CoreDNS Deployment

- make all of this opt in (or out), either via a feature gate (all or nothing), or per KCP (spec field) -- this has been removed from the plan given that the feature is only applicable when a user changes the image

/kind feature
/milestone v0.3.0
/priority critical-urgent
/area control-plane

[vincepri]

/assign

[ncdc]

I think my preference re using a feature gate vs spec field to enable/disable CoreDNS upgrades would be a spec field that defaults to false, so you have to deliberately opt in. WDYT?

[detiber]

I'm a +1 on the use of a feature gate here

[ncdc]

And not controlling it per cluster?

[detiber]

And not controlling it per cluster?

per-cluster may be nice, but without having some type of real requirement around per-cluster granularity for use of the feature and that this should eventually fall under the purview of the addons manager, I think avoiding adding a field would help with type stability as we try to move towards beta types.

[vincepri]

Chatting with @sethp-nr, and brainstorming a little more, we might want to have this configurable by cluster / KCP instance, so I was thinking we can add a new field and be opt-out.

My reasoning to have it be opt-out was mostly because I'd like to maintain feature parity with what kubeadm upgrade would do today, but open to suggestions.

[vincepri]

thinking about the data model, I was going to propose we can add in KCP:

spec:
  managedAddons:
     coreDNS: true

naming tbd of course, but something that potentially can be expanded to other addons as well

[detiber]

Would we consider it an attribute of the KubeadmControlPlane or rather an attribute of the Cluster?

In the short term, implementation wise, I would agree that it is the responsibility of KCP, but if we move to a model where an addon manager is used, it might make more sense for it to be an attribute of the Cluster.

I'm also trying to think of the case where we could eventually be talking about addons such as CNI, which might have more complicated upgrade handling than coreDNS would, where you may not want to perform the upgrade until after all Nodes in the cluster have been upgraded (or possibly before).

[vincepri]

it might make more sense for it to be an attribute of the Cluster

with this I agree! I was thinking we could revisit in v1alpha4 or when addon management has better user stories, for today I guess quickest path might be KCP and then we can strip it out?

[ncdc]

We are probably going to get this wrong the first time we do it and we'll probably move this around in the future 😄 but I don't see any immediate, specific harm if we have this on the KCP spec, and make it off by default. That sound ok?

[ncdc]

Chatted with Vince on Slack. He clarified that most users will expect a CoreDNS upgrade if they are manually running kubeadm upgrade. I'm fine going either opt-in or opt-out - whatever the consensus is, as long as we can enable/disable the behavior per cluster.

[vincepri]

Thinking about this more, we can skip the bool entirely given that users are still required to set the CoreDNS tag to trigger an upgrade.

In the future, if we provide an automated mapping we'll need to make sure to provide ways to opt-out.

Does that sound good?

[ncdc]

Yes! Also here is more backstory:

The original thinking was to have a ConfigMap or some other metadata in the management cluster that could map from Kubernetes version to CoreDNS version, and upgrade CoreDNS as necessary based on that information. We've since dropped that idea and the only way to force a CoreDNS upgrade would be to change the image tag in the KCP spec. So no need for a separate bool or feature gate.

[detiber]

+1 to avoiding the bool/feature gate if we are limiting to the case where the user is explicitly setting the coredns image registry/tag in the config.

[wfernandes]

/assign
/lifecycle active