Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubeadm: add support for separate super-admin.conf kubeconfig file #121305

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Oct 17, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

TL;DR, add a new "super-admin.conf" file that has super powers.
make the file "admin.conf" to be a regular admin user, instead of having the super powers.
see release note and commit text for the included commits.

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#2414

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

ACTION REQUIRED: kubeadm: deploy a separate "super-admin.conf" file. The User in "admin.conf" is now bound to a new RBAC Group "kubeadm:cluster-admins" that have "cluster-admin" ClusterRole access. The User in "super-admin.conf" is bound to the "system:masters" built-in super-powers / break-glass Group that can bypass RBAC. Before this change the default "admin.conf" was bound to "system:masters" Group which was undesired. Executing "kubeadm init phase kubeconfig all" or just "kubeadm init" will now generate the new "super-admin.conf" file. The cluster admin can then decide to keep the file present on a node host or move it to a safe location. "kubadm certs renew" will renew the certificate in "super-admin.conf" to one year if the file exists. If it does not exist a "MISSING" note will be printed. "kubeadm upgrade apply" for this release will migrate this particular node to the two file setup. Subsequent kubeadm releases will continue to optionally renew the certificate in "super-admin.conf" if the file exists on disk and if renew on upgrade is not disabled. "kubeadm join --control-plane" will now generate only an "admin.conf" file that has the less privileged User.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://git.k8s.io/enhancements/keps/sig-cluster-lifecycle/kubeadm/4214-separate-super-user-kubeconfig

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 17, 2023
@neolit123 neolit123 force-pushed the 1.29-super-admin-conf branch 3 times, most recently from 63e9a8d to 8c413c8 Compare October 18, 2023 16:31
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2023
@neolit123 neolit123 force-pushed the 1.29-super-admin-conf branch 2 times, most recently from c4eb331 to 55fd7a6 Compare October 19, 2023 17:41
@neolit123
Copy link
Member Author

/cc @pacoxu
(@SataQiu and @chendave are already on CC as reviewers from the bot)

this is ready for review. PTAL

i will add the v1.29 milestone, but we can skip for 1.29 if there are concerns.

@neolit123
Copy link
Member Author

/milestone v1.29

@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 19, 2023
@neolit123 neolit123 marked this pull request as ready for review October 19, 2023 17:43
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 19, 2023
@chendave
Copy link
Member

Can you pls squash the last commit or just rename the subject? it's now "kubeconf".

Copy link
Member

@SataQiu SataQiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b8bbb7f264d386b202fd9c1dbe5447c0047a841c

- Register the new file in /certs/renewal, so that the
file is renewed if present. If not present the common message "MISSING"
is shown. Same for other certs/kubeconfig files.
- In /kubeconfig, update the spec for admin.conf to use
the "kubeadm:cluster-admins" Group. A new spec is added for
the "super-admin.conf" file that uses the "system:masters" Group.
- Add a new function EnsureAdminClusterRoleBinding() that includes
logic to ensure that admin.conf contains a User that is properly
bound on the "cluster-admin" built-in ClusterRole. This requires
bootstrapping using the "system:masters" containing "super-admin.conf".
Add detailed unit tests for this new logic.
- In /upgrade#PerformPostUpgradeTasks() add logic to create the
"admin.conf" and "super-admin.conf" with the new, updated specs.
Add detailed unit tests for this new logic.
- In /upgrade#StaticPodControlPlane() ensure that renewal of
"super-admin.conf" is performed if the file exists.
Update unit tests.
- Update unit tests in certs_test.go related to the "renew" CLI command.
- In /init, (d *initData) Client(), make sure that the new logic
for bootstrapping an "admin.conf" user is performed, by calling
EnsureAdminClusterRoleBinding() from the phases backend. Add a
"adminKubeConfigBootstrapped" flag that helps call this logic only
once per "kubeadm init" binary execution.
- In /phases/init include a new subphase for generating
the "super-admin.conf" file.
- In /phases/reset make sure the file "super-admin.conf" is
cleaned if present. Update unit tests.
@neolit123
Copy link
Member Author

Can you pls squash the last commit or just rename the subject? it's now "kubeconf".

updated.

Copy link
Member

@chendave chendave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/unhold
unhold as there is 2* LGTM on this already

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, neolit123

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:

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

@k8s-ci-robot k8s-ci-robot merged commit c8125c4 into kubernetes:master Oct 27, 2023
14 checks passed
@Vyom-Yadav
Copy link
Member

@chendave @SataQiu @neolit123 We have a failure on master-informing board, looks like it is related to this PR, #121587

@neolit123
Copy link
Member Author

seems related, i will comment there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants