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.

Sign up for GitHub

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

Jump to bottom

Added option to force apiserver and respective client certificate to … #6403

Merged
merged 2 commits into from
Oct 12, 2020
Merged

Added option to force apiserver and respective client certificate to … #6403

merged 2 commits into from
Oct 12, 2020

Conversation

pestebogdan
Copy link
Contributor

@pestebogdan pestebogdan commented Jul 15, 2020 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
This gives users a way to force regeneration of kube-apiserver certificate (and by the code that's already there , all the relevant client certificates) without the need to upgrade the cluster.
Since certificates are valid for 1 year, some deployments that may not want to upgrade their k8s cluster version might end up broken.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
I added a simple variable named force_certificate_regeneration (default to false) that the users may set to True during a subsequent run of cluster.yml in order to force apiserver certificate regeneration flow.

Does this PR introduce a user-facing change?:

Add force_certificate_regeneration variable

@pestebogdan
Added option to force apiserver and respective client certificate to …
f4f9ee8 
…be regenerated without necessarily needing to bump the K8S cluster version
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 15, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @pestebogdan. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 15, 2020
@k8s-ci-robot k8s-ci-robot requested review from bozzo and holmsten July 15, 2020 14:01
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 15, 2020
@pestebogdan
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

@pestebogdan: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@pestebogdan
Copy link
Contributor Author

/assign @mattymo

@floryut
Copy link
Member

floryut commented Jul 16, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 16, 2020
@pestebogdan
Copy link
Contributor Author

/retest

1 similar comment
@pestebogdan
Copy link
Contributor Author

/retest

@EppO
Copy link
Contributor

EppO commented Jul 31, 2020

This is useful thanks! We don't have a test currently for that, I'll add it to the list in #6400 of CI tests to add
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Aug 1, 2020

I see that the apiserver_sans_check.changed was introduced in #5889.

I haven't tested it in master, but earlier versions would renew the certs at each run of cluster.yml, is that broken or do you have another use case for force_certificate_regeneration?

@pestebogdan
Copy link
Contributor Author

@Miouge1 : From what I tested, re-running cluster.yml does not recreate the certificates (tested latest version and also an old one v2.8.5). apiserver_sans_check only checks and recreates the certificates when a change is done to the SANs IP list, which is not the use case here. The situation this change addresses is when you want to recreate apiserver and the various client certificates due to they becoming expired soon, but you don't want to change the K8s cluster version ( as kubeadm by default will rotate certificates when using it to upgrade the cluster to a newer version, but will not touch them if it's the same version).

@EppO
Copy link
Contributor

EppO commented Sep 1, 2020

a task just to renew the certs (without doing kubeadm upgrade) would be indeed handy. Not sure that's the role of cluster.yml though

@pestebogdan
Copy link
Contributor Author

pestebogdan commented Sep 8, 2020 •
edited
Loading

a task just to renew the certs (without doing kubeadm upgrade) would be indeed handy. Not sure that's the role of cluster.yml though

I understand your point, but the reason it's there is because it leverages the idempotency of cluster.yml (and kubespray in general) , not having to duplicate code that's already proven to work (SAN modification triggers the same flow); that's why the PR only includes just an extra variable and conditional.

@Miouge1
Copy link
Contributor

Miouge1 commented Sep 18, 2020

Kubeadm docs says:

kubeadm renews all the certificates during control plane upgrade.
Source: https://kubernetes.io/docs/tasks/administer-cluster/kubeadm/kubeadm-certs/#automatic-certificate-renewal

Is this a use case for people who have long lived clusters without upgrades or is that feature not enabled in cluster.yml?

@floryut
Copy link
Member

floryut commented Sep 18, 2020

Kubeadm docs says:

kubeadm renews all the certificates during control plane upgrade.
Source: https://kubernetes.io/docs/tasks/administer-cluster/kubeadm/kubeadm-certs/#automatic-certificate-renewal

Is this a use case for people who have long lived clusters without upgrades or is that feature not enabled in cluster.yml?

First one

This gives users a way to force regeneration of kube-apiserver certificate (and by the code that's already there , all the relevant client certificates) without the need to upgrade the cluster.
Since certificates are valid for 1 year, some deployments that may not want to upgrade their k8s cluster version might end up broken.

@Miouge1
Copy link
Contributor

Miouge1 commented Sep 24, 2020

Considering that certificate (expiry and renewal) is a common question, it would be good to add some documentation about the expected behavior and the options available. Would that be something you would be able to do @pestebogdan before we merge this?

@pestebogdan
Copy link
Contributor Author

Considering that certificate (expiry and renewal) is a common question, it would be good to add some documentation about the expected behavior and the options available. Would that be something you would be able to do @pestebogdan before we merge this?

Sure thing. I did put a comment line above the new variable declaration, but i could give more context. I'm new to contributing here, where exactly would that extra documentation need to reside ?

@floryut
Copy link
Member

floryut commented Oct 7, 2020

Considering that certificate (expiry and renewal) is a common question, it would be good to add some documentation about the expected behavior and the options available. Would that be something you would be able to do @pestebogdan before we merge this?

Sure thing. I did put a comment line above the new variable declaration, but i could give more context. I'm new to contributing here, where exactly would that extra documentation need to reside ?

@pestebogdan I guess you could add something in docs, maybe a new md about certificate regeneration during upgrade and this flag ?
I'm fine with merging this and doing the doc in another PR if you don't have time now

@floryut
Copy link
Member

floryut commented Oct 12, 2020 •
edited
Loading

Agreeing with myself, adding this to the todo list.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, pestebogdan

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 Oct 12, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5e32655 into kubernetes-sigs:master Oct 12, 2020
erulabs added a commit to kubesail/kubespray that referenced this pull request Oct 13, 2020
@erulabs
Merge branch 'master' of https://github.com/kubernetes-sigs/kubespray
49b541f 
* 'master' of https://github.com/kubernetes-sigs/kubespray:
  change owner to root for bin_dir directory (kubernetes-sigs#6814)
  Modify imagepullpolicy (kubernetes-sigs#6816)
  fix: add tags for set facts nodelocaldns (kubernetes-sigs#6813)
  Make reset work for crio (kubernetes-sigs#6812)
  Added option to force apiserver and respective client certificate to … (kubernetes-sigs#6403)
  cleanup kubelet_deployment_type (kubernetes-sigs#6815)
  allow pre-existing floating IPs to be specified with k8s_master_fips (kubernetes-sigs#6755)
  Fix line-spacing in no_proxy.yml (kubernetes-sigs#6810)
  Fix handler naming issue for Kubeadm | kubelet (kubernetes-sigs#6803)
  Disable dashboard by default (kubernetes-sigs#6804)
  Chmod kubeconfig to avoid group-readable (kubernetes-sigs#6800)
  Update bunch of dependencies (kubernetes-sigs#6801)
  If no_proxy_exclude_workers is true, workers will be excluded from the no_proxy variable.  This prevents docker engine restarting when scaling workers. (kubernetes-sigs#6520)
@pestebogdan pestebogdan mentioned this pull request Nov 9, 2020
Closed
@floryut floryut mentioned this pull request Dec 19, 2020
Closed
@floryut floryut mentioned this pull request Jan 6, 2021
Closed
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jan 16, 2021
@pestebogdan @LuckySB
Added option to force apiserver and respective client certificate to … (
04dcca6 
kubernetes-sigs#6403)

* Added option to force apiserver and respective client certificate to be regenerated without necessarily needing to bump the K8S cluster version

* Removed extra blank line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bozzo bozzo Awaiting requested review from bozzo

@holmsten holmsten Awaiting requested review from holmsten

Assignees

@EppO EppO

@mattymo mattymo

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. 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pestebogdan @k8s-ci-robot @floryut @EppO @Miouge1 @mattymo