🌱 handle kubeadm 1.24 kubelet ConfigMap name change #6176

fabriziopandini · 2022-02-18T13:21:06Z

What this PR does / why we need it:
This PR makes KCP handle the kubelet ConfigMap as expected by kubeadm >= 1.24

Which issue(s) this PR fixes:
Fixes #5921

controlplane/kubeadm/internal/workload_cluster_rbac.go

controlplane/kubeadm/internal/workload_cluster_test.go

neolit123

LGTM, added only a couple of comments

controlplane/kubeadm/internal/workload_cluster.go

sbueringer · 2022-02-21T18:42:45Z

controlplane/kubeadm/internal/workload_cluster.go

+	if desiredKubeletConfigMapName == previousMinorVersionKubeletConfigMapName {
+		return nil
+	}
+


Q: I know this was like this before but the old ConfigMap is intentionally never deleted, correct?

Yes, in theory the user can create workers with the previous version

controlplane/kubeadm/internal/workload_cluster_rbac.go

sbueringer · 2022-02-21T18:55:32Z

Just for my understanding about how KCP works:

on initial cluster creation the ConfigMap is created based on the ClusterConfiguration we pass into kubeadm init
after that with every upgrade we just preserve the current values +/- fixes like setting the cgroupDriver
we just keep old ConfigMaps / Roles / RoleBindings

Are there some values in the KCP KubeadmConfig that can change and then would be / should be reflected in the kubelet ConfigMap?

sbueringer · 2022-02-22T07:38:01Z

/cherry-pick release-1.1
given that we want to support 1.24 there too

k8s-infra-cherrypick-robot · 2022-02-22T07:38:01Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.1
given that we want to support 1.24 there too

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/test-infra repository.

fabriziopandini · 2022-02-22T22:48:08Z

Just for my understanding about how KCP works:

on initial cluster creation the ConfigMap is created based on the ClusterConfiguration we pass into kubeadm init

after that with every upgrade we just preserve the current values +/- fixes like setting the cgroupDriver

we just keep old ConfigMaps / Roles / RoleBindings

Yes. that's correct

Are there some values in the KCP KubeadmConfig that can change and then would be / should be reflected in the kubelet ConfigMap?

No, given that we allow only to change kubelet flags (via extra args) and they precedence on the content of kubeletConfiguration

sbueringer · 2022-02-23T07:13:20Z

Definitely orthogonal to this PR
/lgtm

Are there some values in the KCP KubeadmConfig that can change and then would be / should be reflected in the kubelet ConfigMap?

No, given that we allow only to change kubelet flags (via extra args) and they precedence on the content of kubeletConfiguration

I looked at the KubeletConfiguration and I found clusterDomain there (no idea if there are others).
If I see correctly:

ClusterConfiguration.Networking.DNSDomain can be set in KCP
in the KubeadmConfig reconciler we default to the Cluster.Spec.ClusterNetwork.ServiceDomain if it is not set (code)
There is code in kubeadm which takes it from ClusterConfiguration and sets it in the KubeletConfiguration (code)

I assume this only happens on kubeadm init and later changes are not reflected in the KubeletConfiguration ConfigMap (?).
I assume in this case that's not really relevant as changing the clusterDomain probably (?) breaks the whole cluster.
Btw ClusterConfiguration.networking is immutable on KCP but the ServiceDomain in the Cluster is mutable, which means the effective domain can be changed by not setting it in KCP and changing it in Cluster.
But maybe the outcome here is that we should make it immutable in the Cluster too.

Just trying to figure out if:

we have a whole class of issues here
there are only a few fields which can't be changed anyway
if there are relevant fields and in those cases the KubeletConfiguration is updated accordingly

I would expect that there are only a few fields (clusterDomain might be the only one) in KubeletConfiguration which are calculated based on fields e.g. from ClusterConfiguration.

Just to confirm another assumption: KubeletConfiguration is uploaded on kubeadm init and then used as config for subsequently joining nodes? And by downloading it and not by Dynamic Kubelet Configuration.

neolit123 · 2022-02-23T10:51:45Z

There is code in kubeadm which takes it from ClusterConfiguration and

sets it in the KubeletConfiguration (code <https://github.com/kubernetes/kubernetes/blob/c2a8cd359f1f70d364808d800dd995c23a7773ab/cmd/kubeadm/app/componentconfigs/kubelet.go#L147> ) This kubeadm feature is old. Probably someone requested it as propagation from the cluster config to the kubelet config, so that they don't have to explicitly pass a kublet config. kubletconfig on init is also the base config / global one so this seems a bit redundant to me.

I assume this only happens on kubeadm init and later changes are not

reflected in the KubeletConfiguration ConfigMap (?). kubeadm upgrade can mutate the kubelet config stored in the cluster if e.g. secure defaults change (rare). But subsequent joins after init would not change it.

I assume in this case that's not really relevant as changing the

clusterDomain probably (?) breaks the whole cluster. Probably breaks it, but who knows what happens? The component maintainers likely don't know and don't have tests for that. This renders the change unsupported. If users change it from my understanding they need to restart all pods on a node, where a kublet is configured with that kubeletconfig. The kubelet config has a design flaw where it holds both global and instance specific options. As per the field description the cluster domain sounds like a global and immutable field and changing it is untested and unsupported in node local and cluster scenarios.

fabriziopandini · 2022-02-23T12:04:23Z

What about moving the orthogonal discussion to an issue? this could quickly cross the line of redesigning the bootstrap API and providing a better abstraction layer on top of kubeadm/bootstrap providers.

sbueringer · 2022-02-23T13:00:29Z

What about moving the orthogonal discussion to an issue? this could quickly cross the line of redesigning the bootstrap API and providing a better abstraction layer on top of kubeadm/bootstrap providers.

You're right. Just wanted to confirm my impression of the current state is correct.

Thx @neolit123 for the additional context.

Should this be a part of: #4464?

controlplane/kubeadm/internal/workload_cluster.go

sbueringer · 2022-02-24T15:23:19Z

/lgtm

vincepri

/approve

k8s-ci-robot · 2022-02-24T16:07:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

~~OWNERS~~ [vincepri]

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

k8s-infra-cherrypick-robot · 2022-02-24T16:09:10Z

@sbueringer: new pull request created: #6206

In response to this:

/cherry-pick release-1.1
given that we want to support 1.24 there too

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/test-infra repository.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 18, 2022

k8s-ci-robot requested review from JoelSpeed and stmcginnis February 18, 2022 13:21

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 18, 2022

neolit123 reviewed Feb 18, 2022

View reviewed changes

controlplane/kubeadm/internal/workload_cluster_rbac.go Outdated Show resolved Hide resolved

neolit123 reviewed Feb 18, 2022

View reviewed changes

controlplane/kubeadm/internal/workload_cluster_test.go Outdated Show resolved Hide resolved

neolit123 reviewed Feb 18, 2022

View reviewed changes

sbueringer reviewed Feb 21, 2022

View reviewed changes

fabriziopandini force-pushed the handle-kubeadm1.24-kubelet-ConfigMap-name-change branch from 2ea191f to e594009 Compare February 22, 2022 22:06

k8s-ci-robot assigned sbueringer Feb 23, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2022

vincepri reviewed Feb 23, 2022

View reviewed changes

controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved

handle kubeadm 1.24 kubelet ConfigMap name change

9705e43

fabriziopandini force-pushed the handle-kubeadm1.24-kubelet-ConfigMap-name-change branch from e594009 to 9705e43 Compare February 24, 2022 14:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2022

vincepri approved these changes Feb 24, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2022

k8s-ci-robot merged commit 38467d2 into kubernetes-sigs:main Feb 24, 2022

k8s-ci-robot added this to the v1.2 milestone Feb 24, 2022

k8s-infra-cherrypick-robot mentioned this pull request Feb 24, 2022

🌱 handle kubeadm 1.24 kubelet ConfigMap name change #6206

Merged

fabriziopandini deleted the handle-kubeadm1.24-kubelet-ConfigMap-name-change branch March 1, 2022 09:50

sbueringer mentioned this pull request May 10, 2022

Tasks for Kubernetes v1.24 #5968

Closed

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 handle kubeadm 1.24 kubelet ConfigMap name change #6176

🌱 handle kubeadm 1.24 kubelet ConfigMap name change #6176

fabriziopandini commented Feb 18, 2022

neolit123 left a comment

sbueringer Feb 21, 2022

fabriziopandini Feb 22, 2022

sbueringer commented Feb 21, 2022 •

edited

Loading

sbueringer commented Feb 22, 2022

k8s-infra-cherrypick-robot commented Feb 22, 2022

fabriziopandini commented Feb 22, 2022

sbueringer commented Feb 23, 2022 •

edited

Loading

neolit123 commented Feb 23, 2022 via email

fabriziopandini commented Feb 23, 2022

sbueringer commented Feb 23, 2022 •

edited

Loading

sbueringer commented Feb 24, 2022

vincepri left a comment

k8s-ci-robot commented Feb 24, 2022

k8s-infra-cherrypick-robot commented Feb 24, 2022

🌱 handle kubeadm 1.24 kubelet ConfigMap name change #6176

🌱 handle kubeadm 1.24 kubelet ConfigMap name change #6176

Conversation

fabriziopandini commented Feb 18, 2022

neolit123 left a comment

Choose a reason for hiding this comment

sbueringer Feb 21, 2022

Choose a reason for hiding this comment

fabriziopandini Feb 22, 2022

Choose a reason for hiding this comment

sbueringer commented Feb 21, 2022 • edited Loading

sbueringer commented Feb 22, 2022

k8s-infra-cherrypick-robot commented Feb 22, 2022

fabriziopandini commented Feb 22, 2022

sbueringer commented Feb 23, 2022 • edited Loading

neolit123 commented Feb 23, 2022 via email

fabriziopandini commented Feb 23, 2022

sbueringer commented Feb 23, 2022 • edited Loading

sbueringer commented Feb 24, 2022

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 24, 2022

k8s-infra-cherrypick-robot commented Feb 24, 2022

sbueringer commented Feb 21, 2022 •

edited

Loading

sbueringer commented Feb 23, 2022 •

edited

Loading

sbueringer commented Feb 23, 2022 •

edited

Loading