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 guide for kubelet serving certs and metrics-server #27071

Merged

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Mar 15, 2021

note: targeting the master branch is accurate for this change.

Include a new section of the kubeadm certificate management page
to talk about kubelet serving certificates and how to make
them signed. Also include a note about using secure connection
with the metrics-server.

fixes kubernetes/kubeadm#1602

/sig cluster-lifecycle

@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 15, 2021
@neolit123
Copy link
Member Author

/cc @fabriziopandini @randomvariable

@netlify
Copy link

netlify bot commented Mar 15, 2021

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 30c6e77

https://deploy-preview-27071--kubernetes-io-master-staging.netlify.app

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks!

Some feedback. I'm not sure about the heading “Cannot use the metrics-server securely in a kubeadm cluster” and also I'd like to include a 3rd party content warning for kubelet-rubber-stamp.

Plus a few less important details.

@neolit123 neolit123 force-pushed the 1.21-add-kubelet-serving-cert-guide branch 2 times, most recently from f6a9a3a to 3de80b5 Compare March 23, 2021 23:03
@neolit123
Copy link
Member Author

updated to address the review comments.

Comment on lines +298 to +277
Third party custom controllers can be used:
- [kubelet-rubber-stamp](https://github.com/kontena/kubelet-rubber-stamp)

Such a controller is not a secure mechanism unless it not only verifies the CommonName
in the CSR but also verifies the requested IPs and domain names. This would prevent
a malicious actor that has access to a kubelet client certificate to create
CSRs requesting serving certificates for any IP or domain name.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should express options on external project; TBH I would avoid the link entirely but I don't want to block on this

Copy link
Member Author

@neolit123 neolit123 Mar 24, 2021

Choose a reason for hiding this comment

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

@sftim and randomvariable seem to be fine with mentioning this third-party option.

for cloud provider scenarios we already tell users to consult their provider.
users that don't trust this would have to either use a different rubber-stamp or write their own controller / DS / scripts.

@alvaroaleman
Copy link
Member

@neolit123: PR needs rebase.

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 k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2021
@sftim
Copy link
Contributor

sftim commented Apr 3, 2021

Needs a rebase, but assuming the obvious approach to rebasing: LGTM.

@neolit123 neolit123 force-pushed the 1.21-add-kubelet-serving-cert-guide branch from 3de80b5 to 1434983 Compare April 3, 2021 18:01
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2021
@neolit123
Copy link
Member Author

rebased.

/cc @fabriziopandini
for tech review / LGTM.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Some feedback. I don't think it need block merging these changes.

LGTM!

Comment on lines 231 to 232
- Find and edit the `kubelet-config-x.yy` ConfigMap under `kube-system` to include
the field `serverTLSBootstrap: true` under the `config` key
Copy link
Contributor

@sftim sftim Apr 3, 2021

Choose a reason for hiding this comment

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

We could also write:

Suggested change
- Find and edit the `kubelet-config-x.yy` ConfigMap under `kube-system` to include
the field `serverTLSBootstrap: true` under the `config` key
- Find and edit the `kubelet-config-{{< skew latestVersion >}}` ConfigMap in the `kube-system` namespace.
In that ConfigMap, the `config` key has a
[kubelet configuration](/docs/reference/config-api/kubelet-config.v1beta1/#kubelet-config-k8s-io-v1beta1-KubeletConfiguration) document as
its value. Edit the kubelet configuration document to set `serverTLSBootstrap: true`.

(untested!)

  • maybe in a follow-up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://kubernetes.io/docs/tasks/administer-cluster/reconfigure-kubelet/ recommends changing the ConfigMap that kubelets use (a different name); is that advice related to what's recommended here?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe in a follow-up PR?

updated in this PR.

https://kubernetes.io/docs/tasks/administer-cluster/reconfigure-kubelet/ recommends changing the ConfigMap that kubelets use (a different name); is that advice related to what's recommended here?

kubeadm uses a single ConfigMap, so need to introduce a new ConfigMap with a different name.

Copy link
Member

@fabriziopandini fabriziopandini 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 Apr 6, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7026c4a74d5a963e5d7e1975c03ed0b156e6f363

Include a new section of the kubeadm certificate management page
to talk about kubelet serving certificates and how to make
them signed. Also include a note about using secure connection
with the metrics-server.
@neolit123 neolit123 force-pushed the 1.21-add-kubelet-serving-cert-guide branch from 1434983 to 30c6e77 Compare April 6, 2021 22:19
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2021
@neolit123
Copy link
Member Author

@sftim
updated with your two suggestions above.
tech LGTM already passed by @fabriziopandini

@kbhawkey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2021
@kbhawkey
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: 59c4229345bb7edcf59b4501db23c61b8575a2f1

@k8s-ci-robot k8s-ci-robot merged commit 9b300a4 into kubernetes:master Apr 21, 2021
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provide workarounds for the kubelet self-signed serving certificate
7 participants