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 check certificate expiration command #77863

Merged
merged 2 commits into from May 19, 2019

Conversation

@fabriziopandini
Copy link
Member

commented May 14, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds a new command kubeadm alpha certs check-expiration

Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#1563

Does this PR introduce a user-facing change?:

Kubeadm: a new command `kubeadm alpha certs check-expiration` was created in order to help users in managing expiration for local PKI certificates
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

/sig cluster-lifecycle
/area kubeadm
/priority important-soon
@kubernetes/sig-cluster-lifecycle-pr-reviews

/assign @rosti
/assign @timothysc

@neolit123
Copy link
Member

left a comment

thanks for this work @fabriziopandini
this would be a very useful feature.

during the office hours we expressed that we might not want to add the extra info for upgrade plan, but i think it's fine and it's definitely nice to have...
.but we cannot add guarantees for the output format of this, as we don't have machine readable output there yet.

i've added a couple of extra comments.

cmd/kubeadm/app/cmd/upgrade/plan.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/certs/renewal/expiration.go Outdated Show resolved Hide resolved
// RenewalPeriodDays defines the leng of a period before the certificate expiration when renewal should be
// planned and executed. This period consider that the default duration for kubeadm certificates is 365d,
// the fact that kubeadm handles automatic certificate renewal during upgrades, and the release frequence of K8s.
const RenewalPeriodDays = 180

This comment has been minimized.

Copy link
@neolit123

neolit123 May 14, 2019

Member

ideally the constant for total time (365d) and this constant should be in the same file.

but i have a bit of concern here, as we are imposing a recommendation that not all users might agree with - 180d.

i think that if the user call check-expiration we should always print all certs that we manage and next to them in how much time they will expire. depending on a threshold of say 1 month we can tag some of the certs as recommended for renewal.

for upgrade plan we can only print the certs that are are about to expire in 1 month remaining.

this would be the UX i would like to see from kubeadm if i manage my certs with it.
WDYT @fabriziopandini

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini May 14, 2019

Author Member

1 Month

I have no strong opinion and the threshold is a constant and it can be changed easily

However, based on issues and slack request of help, IMO 1 month is too short, especially considering users that are going to rely on kubeadm upgrade for automatic certs renewal.

In that case, we should consider the K8s release cycle of 90d, and according to the kubeadm survey, the fact that not all the user are upgrading frequently (hence the initial proposal of 180d).

Said that I will change to whatever value there is agreement on.

I think that if the user calls check-expiration we should always print all certs that we manage and next to them in how much time they will expire. depending on a threshold of say X month we can tag some of the certs as recommended for renewal.

for upgrade plan we can only print the certs that are are about to expire in X month remaining.

This is exactly how it works; see example above from the check-expiration command; upgrade output is the same but it contains only certificates set for expiration

This comment has been minimized.

Copy link
@neolit123

neolit123 May 14, 2019

Member

Said that I will change to whatever value there is agreement on.

ok, let's discuss tomorrow (today):

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini May 15, 2019

Author Member

In the meantime

ideally the constant for total time (365d) and this constant should be in the same file.

was fixed

This comment has been minimized.

Copy link
@mauilion

mauilion May 16, 2019

Contributor

I agree that 180 days is pretty far out.
Remember that the old rotated out cert is still valid.
so the old cert is a liability for 180 days if we go this route. 30 or 60 days would be better.

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini May 16, 2019

Author Member

@mauilion this was discussed again in kubeadm office hours, and it was decided to drop the idea and always renew during upgrades

@rosti
Copy link
Member

left a comment

Thanks @fabriziopandini !
Looks OK on first pass.

cmd/kubeadm/app/phases/certs/renewal/expiration.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/cmd/upgrade/plan.go Outdated Show resolved Hide resolved

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:certs-expiration branch from 4aaf2d1 to c34d468 May 15, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@rosti @neolit123
Added/Fixed all the tests and addressed all the comments. Main points still open are:
a) 180d (current) vs 1m vs w whatever value we choose as renewalPeriod
b) show expiration in upgrade plan (current) or not

@neolit123

This comment has been minimized.

Copy link
Member

commented May 15, 2019

a) 180d (current) vs 1m vs w whatever value we choose as renewalPeriod

let's get feedback on that from the office hours today.

b) show expiration in upgrade plan (current) or not

it's ok to print on plan.

@stealthybox

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Feedback from office hours is to always renew certs on upgrade.
We'll should publish an Action Required release-note to warn kubeadm users who are copying these certs around that kubeadm is now automatically changing them on upgrade.

Also, we might consider an option to opt-out.

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:certs-expiration branch from c34d468 to d40ed8c May 16, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XXL labels May 16, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@stealthybox @rosti @neolit123
PR is now updated as discussed in the kubeadm office hours; an example of the command output is:

root@kind-control-plane:/# kubeadm alpha certs check-expiration
CERTIFICATE                EXPIRES                  RESIDUAL TIME   EXTERNALLY MANAGED
admin.conf                 May 15, 2020 13:03 UTC   364d            true
apiserver                  May 15, 2020 13:00 UTC   364d            true
apiserver-etcd-client      May 15, 2020 13:00 UTC   364d            false
apiserver-kubelet-client   May 15, 2020 13:00 UTC   364d            true
controller-manager.conf    May 15, 2020 13:03 UTC   364d            true
etcd-healthcheck-client    May 15, 2020 13:00 UTC   364d            false
etcd-peer                  May 15, 2020 13:00 UTC   364d            false
etcd-server                May 15, 2020 13:00 UTC   364d            false
front-proxy-client         May 15, 2020 13:00 UTC   364d            false
scheduler.conf             May 15, 2020 13:03 UTC   364d            true

Since we are always renewing certificates during an upgrade, I dropped the changes to kubeadm upgrade plan too, thus simplifying the PR.

We'll should publish an Action Required release-note to warn kubeadm users who are copying these certs around that kubeadm is now automatically changing them on upgrade.

The place for this "Action Required" is #76862; I will add this there

Also, we might consider an option to opt-out.

Already done in #76862

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

/test pull-kubernetes-e2e-gce

@fabriziopandini fabriziopandini changed the title [WIP] Kubeadm: Add check certificate expiration command Kubeadm: Add check certificate expiration command May 16, 2019

@rosti
Copy link
Member

left a comment

Thanks @fabriziopandini !
Looks great, only a tiny nit and I think we can merge it.

cmd/kubeadm/app/cmd/alpha/certs.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/cmd/alpha/certs.go Outdated Show resolved Hide resolved

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:certs-expiration branch from d40ed8c to e4d87b0 May 18, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

@rosti comments addressed!

@dixudx
dixudx approved these changes May 19, 2019
Copy link
Member

left a comment

/lgtm

Thanks @fabianofranz

@fejta-bot

This comment has been minimized.

Copy link

commented May 19, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 81a61ae into kubernetes:master May 19, 2019

20 checks passed

cla/linuxfoundation fabriziopandini authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
globervinodhn pushed a commit to globervinodhn/kubernetes that referenced this pull request May 20, 2019
Merge pull request kubernetes#77863 from fabriziopandini/certs-expira…
…tion

Kubeadm: Add check certificate expiration command

@fabriziopandini fabriziopandini deleted the fabriziopandini:certs-expiration branch May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.