kubeadm makes it difficult to override `kube-controller-manager --controllers`

[danwinship]

Trying to override kube-controller-manager --controllers in the obvious way (by setting .controllerManager.extraArgs.controllers in the ClusterConfiguration) doesn't quite work, because it causes kubeadm's default value for that argument ("*,bootstrapsigner,tokencleaner") to be ignored, causing the cluster to then not come up correctly because of the lack of a bootstrap signing controller. (as in https://prow.k8s.io/view/gs/kubernetes-ci-logs/logs/ci-kubernetes-kind-network-deprecate-endpoints/1981104994845200384, fixed by kubernetes/test-infra#35763)

It seems like if kubeadm requires those two controllers to be present, then it ought to override your override to make the right thing happen? Or else provide a way to explicitly set the --controllers arg other than via extraArgs so that it can fix it up afterward. It's awkward for my custom config to have to hardcode knowledge about what controllers kubeadm requires (and of course, if kubeadm starts requiring more non-default controllers in the future, then my config will start failing again).

[k8s-ci-robot]

There are no sig labels on this issue. Please add an appropriate label by using one of the following commands:

• /sig <group-name>
• /wg <group-name>
• /committee <group-name>

Please see the group list for a listing of the SIGs, working groups, and committees available.

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-sigs/prow repository.

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

[neolit123]

/transfer kubeadm

[neolit123]

i can see how that can be awkward but it's a common behavior for all flags and i don't think we should special case --controllers.
a user flag overrides the default, there is no smart merging. if the user wants to change a default's value they'd have to include the defaults again "*,bootstrapsigner,tokencleaner,somecontroller".

about the detauls:

• the tokenclearner is somewhat required for the security reason of expiring BS tokens.
• i've seen cases where the bootstrapsigner is disabled by users, who implement their own logic.

so we cannot have an additive extraArgs + defaults, which means there should be a dedicated knob and that's not a common enough demand to add, IMO.

EDIT: i don't recall if we special case some flags, but this is one example how the override happens even for the authz flag:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/controlplane/manifests.go#L261-L263

[neolit123]

in v1beta4+ extraArgs are a []Arg and that type is a name/value tuple. the problem is that even if we add a field to identify merge method between defaults and an an extraArg we'd have to implement a "way to append", e.g. controllers are separate by , some other list is separated by ;

so it could be:

type Arg struct{
  name string
  value string
  mergeMethod MergeMethodType
}

the the various types could be:

"replace" = just replaces the default with user value (like it does today)
"appendXX" = appends the provided values to the existing defaults with optional separator XX e.g. , or ;

so sounds doable, but it's an API change and we had no demand for it thus far.