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

Generate kubelet server cert with cluster CA #74216

Closed
wants to merge 9 commits into from

Conversation

stgleb
Copy link
Contributor

@stgleb stgleb commented Feb 18, 2019

/kind bug

Add kubelet-server phase to join and init commands.
Generate kubelet key and certificate signing request,
create, approve this certificate signing request and download
certificate.

Implements kubeadm design proposal
Fixes kubeadm issue

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 18, 2019
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 18, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @stgleb. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 18, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: stgleb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: neolit123

If they are not already assigned, you can assign the PR to them by writing /assign @neolit123 in a comment when ready.

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

@anarchistHH1983
Copy link

anarchistHH1983 commented Feb 18, 2019 via email

@anarchistHH1983
Copy link

anarchistHH1983 commented Feb 18, 2019 via email

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 18, 2019
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/hold

@stgleb the kubeadm config is beta and we should batch multiple changes per release.
due to the kubeadm team being overbooked it was (mostly) decided to not bump v1beta1 to v1beta2 this cycle(?) this change is not batched with other changes and it might not make it in 1.14.

also:

  • please add a release note instead of NONE
  • and link to the original kubeadm issue - Fixes #1223 seems unrelated.

thanks.
@kubernetes/sig-cluster-lifecycle-pr-reviews
/assign @timothysc @detiber

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 18, 2019
@stgleb
Copy link
Contributor Author

stgleb commented Feb 18, 2019

@neolit123 Thank you, I am not expecting this to be in 1.14

@anarchistHH1983
Copy link

anarchistHH1983 commented Feb 18, 2019 via email

@k8s-ci-robot
Copy link
Contributor

@anarchistHH1983: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

Release

On Mon, Feb 18, 2019, 12:23 PM Lubomir I. Ivanov <notifications@github.com
wrote:

@neolit123 commented on this pull request.

/hold

@stgleb https://github.com/stgleb the kubeadm config is beta and we
should batch multiple changes per release.
due to the kubeadm team being overbooked this cycle it was (mostly)
decided to not bump v1beta1 to v1beta2 this cycle(?) this change is not
batched with other changes and it might not make it in 1.14.

also:

thanks.
@kubernetes/sig-cluster-lifecycle-pr-reviews
https://github.com/orgs/kubernetes/teams/sig-cluster-lifecycle-pr-reviews
/assign @timothysc https://github.com/timothysc @detiber
https://github.com/detiber


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#74216 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/Atif5Xpe3yrqoKFqFUHHol4UhZjKn3vQks5vOuF8gaJpZM4bBSow
.

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.

@stgleb stgleb closed this Mar 27, 2019
@stgleb stgleb reopened this Mar 27, 2019
@mark-casey
Copy link

mark-casey commented Apr 15, 2019

kfox1111
I kind of hit this issue the other day. Is there a reason it isn't rolled up in the bootstrap process? The client bootstrap already creates a signed client cert. why not create a server cert at the same time?

@liggitt just came across this and was recalling your comment from kubernetes/kubeadm#118 (comment). If I recall correctly the issue is during cert renewal, absent a recent bootstrap token, client certs would be self-authn/authz for renewal and serving certs are not?

Coming across this again though, I wonder for non-bootstrap scenarios could a kubelet just approve it's own new serving cert CSR by acting as a node/admin with its client cert? Just had the thought so may be half-baked, but it's all happening in the same process right and if the client cert isn't valid such that it couldn't approve the new serving CSR isn't the node in bad shape anyway?

@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 18, 2019
@stgleb stgleb force-pushed the kubeadm-kubelet-server-cert branch from 7ce28e4 to 74fddc1 Compare April 18, 2019 17:09
@neolit123
Copy link
Member

@stgleb
FYI the design proposal is not approved and also there is ongoing discussion in the kubeadm issue whether we should move away from the kubelet self-signed serving certificate and how.

it might be a good idea to hold on commits, because this PR can end up being rejected.
(also we cannot modify v1beta1 at this point as already mentioned).

@k8s-ci-robot
Copy link
Contributor

@stgleb: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 22, 2019
@stgleb stgleb changed the title [WIP] Generate kubelet server cert with cluster CA Generate kubelet server cert with cluster CA Apr 22, 2019
@k8s-ci-robot 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 Apr 22, 2019
@stgleb
Copy link
Contributor Author

stgleb commented Apr 22, 2019

@neolit123 May I keep it here as a possible solution? I am basically finished.

@neolit123
Copy link
Member

@neolit123 May I keep it here as a possible solution? I am basically finished.

sure.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 21, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 20, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use signed kubelet serving certificates