🌱 Support admin config for Kubeadm v1.29 #9682

killianmuldoon · 2023-11-07T11:23:20Z

Add support to the Kubeadm Control Plane provider for the new Kubeadm ClusterRoles to use the super-admin config introduced in the v1.29 cycle.

Part of #9578
Fixes #9633

killianmuldoon

/area provider/controlplane-kubeadm

k8s-ci-robot · 2023-11-07T11:23:45Z

@killianmuldoon: The label(s) area/provider/controlplane-kubeadm cannot be applied, because the repository doesn't have them.

In response to this:

/area provider/controlplane-kubeadm

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.

killianmuldoon · 2023-11-07T11:23:57Z

/area provider/control-plane-kubeadm

killianmuldoon

/test

killianmuldoon · 2023-11-07T11:24:34Z

controlplane/kubeadm/internal/workload_cluster_rbac.go

@@ -35,6 +36,14 @@ const (
 	// GetNodesClusterRoleName defines the name of the ClusterRole and ClusterRoleBinding to get nodes.
 	GetNodesClusterRoleName = "kubeadm:get-nodes"

+	// SuperAdminKubeConfigFileName defines name for the kubeconfig aimed to be used by the super-admin of the cluster.
+	SuperAdminKubeConfigFileName = "super-admin.conf"


We should be able to import these constants once 1.29 is released, but for now we copy them like these other constants. Not sure if it's worth the import.

Not sure if we can import from this part of k/k without getting a dependency on k/k itself

This specific constant seems to be unused, let's delete it if we don't use it

k8s-ci-robot · 2023-11-07T11:24:56Z

@killianmuldoon: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-full-dualstack-and-ipv6-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-scale-main-experimental

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

/test

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.

killianmuldoon · 2023-11-07T11:25:05Z

/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main

killianmuldoon · 2023-11-07T11:40:14Z

/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main

killianmuldoon · 2023-11-07T13:08:57Z

/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main

killianmuldoon · 2023-11-07T13:09:04Z

/test pull-cluster-api-e2e-full-main

killianmuldoon · 2023-11-07T13:09:27Z

/hold

Let's keep this back until after we create the release branch for v1.6.0

killianmuldoon · 2023-11-07T13:12:47Z

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main

neolit123 · 2023-11-07T13:49:35Z

controlplane/kubeadm/internal/workload_cluster_rbac.go

+// AddClusterAdminRoleBinding creates ClusterRoleBinding rules to use the kubeadm:cluster-admins Cluster Role created in Kubeadm v1.29.
+func (w *Workload) AddClusterAdminRoleBinding(ctx context.Context, version semver.Version) error {
+	// If the upgrade is not to a Cluster of version v1.29.0 or higher this is a no-op
+	if !version.GTE(semver.MustParse("1.29.0")) {


if you are running this code in CI against kubeadm CI artifacts, the exact version where this feature was somehow useable and the RBAC was needed is:
v1.29.0-alpha.2.188+05076de57fc49f

Updated to use the CAPI util that ignores preRelease versions for checks like these. Thanks for catching this!

controlplane/kubeadm/internal/workload_cluster.go

killianmuldoon · 2023-11-07T14:35:12Z

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main

sbueringer

Just some minor findings

sbueringer · 2023-11-07T14:43:43Z

controlplane/kubeadm/internal/workload_cluster_rbac.go

@@ -35,6 +36,14 @@ const (
 	// GetNodesClusterRoleName defines the name of the ClusterRole and ClusterRoleBinding to get nodes.
 	GetNodesClusterRoleName = "kubeadm:get-nodes"

+	// SuperAdminKubeConfigFileName defines name for the kubeconfig aimed to be used by the super-admin of the cluster.
+	SuperAdminKubeConfigFileName = "super-admin.conf"


Not sure if we can import from this part of k/k without getting a dependency on k/k itself

sbueringer · 2023-11-07T14:44:29Z

controlplane/kubeadm/internal/workload_cluster_rbac.go

@@ -35,6 +36,14 @@ const (
 	// GetNodesClusterRoleName defines the name of the ClusterRole and ClusterRoleBinding to get nodes.
 	GetNodesClusterRoleName = "kubeadm:get-nodes"

+	// SuperAdminKubeConfigFileName defines name for the kubeconfig aimed to be used by the super-admin of the cluster.
+	SuperAdminKubeConfigFileName = "super-admin.conf"


This specific constant seems to be unused, let's delete it if we don't use it

sbueringer · 2023-11-07T14:45:26Z

controlplane/kubeadm/internal/controllers/upgrade.go

@@ -68,6 +68,11 @@ func (r *KubeadmControlPlaneReconciler) upgradeControlPlane(
 		return ctrl.Result{}, errors.Wrap(err, "failed to set role and role binding for kubeadm")
 	}

+	// Ensure kubeadm clusterRole & Bindings for v1.29+ as per https://github.com/kubernetes/kubernetes/pull/121305
+	if err := workloadCluster.AllowClusterAdminPermissions(ctx, parsedVersion); err != nil {
+		return ctrl.Result{}, errors.Wrap(err, "failed to set role and role binding for kubeadm")


the error text seems to be misleading / a copy&paste error

error message is more specific now

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

killianmuldoon · 2023-11-07T15:40:55Z

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main

sbueringer · 2023-11-07T15:51:24Z

Thx

/lgtm
/approve

Feel free to hold cancel when you want to merge

k8s-ci-robot · 2023-11-07T15:51:34Z

LGTM label has been added.

Git tree hash: 9ab2c962607bef49b2d277425df1fa3bb2ade6dc

k8s-ci-robot · 2023-11-07T15:51:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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

killianmuldoon · 2023-11-07T15:54:13Z

Feel free to hold cancel when you want to merge

IMO let's leave this open until after the 1.6.0 RC is cut next Tuesday. Would prefer to get as clean a test signal as possible, and 1.29 support won't make the 1.6.0 release in any case. But up to you 😅

g-gaston

/lgtm

sbueringer · 2023-11-07T19:05:08Z

I think merging on main shouldn't affect 1.6.

I also think a cherry pick would be good to already unblock other providers running tests against Kubernetes 1.29. I think a cherry pick is okay as the change is very straightforward

killianmuldoon · 2023-11-07T19:33:33Z

Fair enough - I'm just a little nervous with the state of the CI signal this close to release.

/hold cancel

/cherry-pick release-1.5

k8s-infra-cherrypick-robot · 2023-11-07T19:33:35Z

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

In response to this:

Fair enough - I'm just a little nervous with the state of the CI signal this close to release.

/hold cancel

/cherry-pick release-1.5

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.

killianmuldoon · 2023-11-07T19:33:42Z

/cherry-pick release-1.5

k8s-infra-cherrypick-robot · 2023-11-07T19:33:44Z

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

In response to this:

/cherry-pick release-1.5

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.

killianmuldoon · 2023-11-07T19:33:45Z

/cherry-pick release-1.4

k8s-infra-cherrypick-robot · 2023-11-07T19:33:47Z

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

In response to this:

/cherry-pick release-1.4

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.

killianmuldoon · 2023-11-07T19:36:21Z

I think merging on main shouldn't affect 1.6.

Just to be clear - main is 1.6 right now, but let's merge. I don't think this is going to have a negative impact really. I was just mentally starting the code freeze early 😬

k8s-infra-cherrypick-robot · 2023-11-07T19:40:56Z

@killianmuldoon: new pull request created: #9684

In response to this:

/cherry-pick release-1.5

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-infra-cherrypick-robot · 2023-11-07T19:41:33Z

@killianmuldoon: new pull request created: #9685

In response to this:

/cherry-pick release-1.4

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.

sbueringer · 2023-11-08T00:02:26Z

Thank you

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 7, 2023

killianmuldoon changed the title ~~🌱 Support super-admin config for Kubeadm v1.29~~ [WIP] 🌱 Support super-admin config for Kubeadm v1.29 Nov 7, 2023

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 7, 2023

k8s-ci-robot requested a review from g-gaston November 7, 2023 11:23

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 7, 2023

k8s-ci-robot requested a review from stmcginnis November 7, 2023 11:23

killianmuldoon commented Nov 7, 2023

View reviewed changes

k8s-ci-robot added area/provider/control-plane-kubeadm Issues or PRs related to KCP and removed do-not-merge/needs-area PR is missing an area label labels Nov 7, 2023

killianmuldoon commented Nov 7, 2023

View reviewed changes

killianmuldoon force-pushed the pr-kubeadm-rbac branch from caa2e90 to 4f0b400 Compare November 7, 2023 11:39

killianmuldoon mentioned this pull request Nov 7, 2023

Tasks to bump to Kubernetes v1.29 #9578

Closed

14 tasks

killianmuldoon changed the title ~~[WIP] 🌱 Support super-admin config for Kubeadm v1.29~~ 🌱 Support super-admin config for Kubeadm v1.29 Nov 7, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2023

killianmuldoon force-pushed the pr-kubeadm-rbac branch from 4f0b400 to 57ba9d3 Compare November 7, 2023 13:08

killianmuldoon changed the title ~~🌱 Support super-admin config for Kubeadm v1.29~~ 🌱 Support admin config for Kubeadm v1.29 Nov 7, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2023

killianmuldoon force-pushed the pr-kubeadm-rbac branch from 57ba9d3 to 57dd6a3 Compare November 7, 2023 13:11

neolit123 reviewed Nov 7, 2023

View reviewed changes

killianmuldoon force-pushed the pr-kubeadm-rbac branch from 57dd6a3 to 623bc7a Compare November 7, 2023 14:29

sbueringer reviewed Nov 7, 2023

View reviewed changes

Support super-admin config for Kubeadm v1.29

67c222e

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

killianmuldoon force-pushed the pr-kubeadm-rbac branch from f48b33e to 67c222e Compare November 7, 2023 14:54

k8s-ci-robot assigned sbueringer Nov 7, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2023

g-gaston reviewed Nov 7, 2023

View reviewed changes

k8s-ci-robot assigned g-gaston Nov 7, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2023

k8s-ci-robot merged commit 6384395 into kubernetes-sigs:main Nov 7, 2023
22 checks passed

k8s-ci-robot added this to the v1.6 milestone Nov 7, 2023

This was referenced Nov 7, 2023

[release-1.5] 🌱 Support admin config for Kubeadm v1.29 #9684

Merged

[release-1.4] 🌱 Support admin config for Kubeadm v1.29 #9685

Merged

🌱 Support admin config for Kubeadm v1.29 #9682

🌱 Support admin config for Kubeadm v1.29 #9682

Conversation

killianmuldoon commented Nov 7, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killianmuldoon commented Nov 7, 2023

sbueringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killianmuldoon commented Nov 7, 2023

sbueringer commented Nov 7, 2023

k8s-ci-robot commented Nov 7, 2023

k8s-ci-robot commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

g-gaston left a comment

Choose a reason for hiding this comment

sbueringer commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

k8s-infra-cherrypick-robot commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

k8s-infra-cherrypick-robot commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

k8s-infra-cherrypick-robot commented Nov 7, 2023

killianmuldoon commented Nov 7, 2023

k8s-infra-cherrypick-robot commented Nov 7, 2023

k8s-infra-cherrypick-robot commented Nov 7, 2023

sbueringer commented Nov 8, 2023