Kubeadm Cert Renewal #67910

liztio · 2018-08-27T20:34:02Z

What this PR does / why we need it:

adds explicit support for renewal of certificates via command

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#206

Special notes for your reviewer:
The targeted documentation is at kubernetes/website#9712

Release note:

Adds the commands `kubeadm alpha phases renew <cert-name>`

timothysc

minor feedback on wip

timothysc · 2018-08-27T21:55:35Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+
+	key, err := certutil.NewPrivateKey()
+	if err != nil {
+		return nil, nil, fmt.Errorf("Couldn't create new private key: %v", err)


b/c these files are net new, definitely use errors.Wrap here.

timothysc · 2018-08-27T22:03:07Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+		if ev.Type != watch.Modified {
+			return nil, nil, fmt.Errorf("unexpected event receieved: %q", ev.Type)
+		}
+	case <-time.After(watchTimeout):


I have to dig-up a different example for you here, but this looks a little weird.

neolit123

@liztio thanks
added some minor comments.

neolit123 · 2018-08-27T21:44:42Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+	select {
+	case ev := <-watcher.ResultChan():
+		if ev.Type != watch.Modified {
+			return nil, nil, fmt.Errorf("unexpected event receieved: %q", ev.Type)


receieved -> received.

optionally %q -> %s

I like %q for this

neolit123 · 2018-08-27T21:46:38Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+
+	// TODO: under what circumstances are there more than one?
+	if status := req.Status.Conditions[0].Type; status != certsapi.CertificateApproved {
+		return nil, nil, fmt.Errorf("Unexpected certificate status %v", status)


Unexpected -> unexpected
status %v -> status: %v

neolit123 · 2018-08-27T21:50:43Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+	client certstype.CertificatesV1beta1Interface
+}
+
+// NewCertsAPIRenawal takes a certificate pair to construct the Interface.


-> takes a client Interface and returns a renewal Interface?

neolit123 · 2018-08-27T21:52:54Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+
+	key, err := certutil.NewPrivateKey()
+	if err != nil {
+		return nil, nil, fmt.Errorf("Couldn't create new private key: %v", err)


Couldn't -> couldn't

neolit123 · 2018-08-27T21:53:05Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+
+	csr, err := x509.CreateCertificateRequest(rand.Reader, reqTmp, key)
+	if err != nil {
+		return nil, nil, fmt.Errorf("Couldn't create csr: %v", err)


Couldn't -> couldn't
csr -> certificate signing request?

neolit123 · 2018-08-27T21:53:54Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+	for i, usage := range cfg.Usages {
+		certsAPIUsage, ok := usageMap[usage]
+		if !ok {
+			return nil, nil, fmt.Errorf("unknown key usage %v", usage)


-> key usage: %v

neolit123 · 2018-08-27T22:04:34Z

cmd/kubeadm/app/phases/certs/renewal/filerenewal.go

+func (r *FileRenewal) Renew(cfg *certutil.Config) (*x509.Certificate, crypto.PrivateKey, error) {
+	caKey, ok := r.caKey.(*rsa.PrivateKey)
+	if !ok {
+		return nil, nil, fmt.Errorf("unsupported private key type %t", r.caKey)


this would print the whole private key which can be quite long.
i would go for something like:
unsupported private key type. Only RSA is supported

kad · 2018-08-29T12:29:59Z

cmd/kubeadm/app/phases/certs/renewal/certsapi.go

+			Organization: cfg.Organization,
+		},
+		DNSNames:    cfg.AltNames.DNSNames,
+		IPAddresses: cfg.AltNames.IPs,


One bad thing is that x509.CertificateRequest doesn't validate what kind of IPs are requested in certificates. I think on our side we should be doing some validation that certificates that we request don't have some weird IPs in it.

The source of these IPs is the existing, on-disk certificates. What IPs are you worried about?

liztio · 2018-08-30T19:07:12Z

/test pull-kubernetes-bazel-test

liztio · 2018-08-30T20:01:54Z

/test pull-kubernetes-e2e-kops-aws

timothysc

/lgtm
/approve

I could needle some things, but we're near freeze, and perfect is the enemy of the good.

k8s-ci-robot · 2018-08-31T19:21:51Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: liztio, timothysc

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:

~~cmd/kubeadm/OWNERS~~ [timothysc]
staging/src/k8s.io/client-go/OWNERS

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

k8s-github-robot · 2018-08-31T23:46:35Z

Automatic merge from submit-queue (batch tested with PRs 64283, 67910, 67803, 68100). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Two implmentations of cert renewal

a53f478

k8s-ci-robot requested review from detiber and kad August 27, 2018 20:36

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Aug 27, 2018

timothysc added the status/approved-for-milestone label Aug 27, 2018

timothysc added this to the v1.12 milestone Aug 27, 2018

k8s-ci-robot added 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 Aug 27, 2018

timothysc reviewed Aug 27, 2018

View reviewed changes

neolit123 reviewed Aug 27, 2018

View reviewed changes

liztio added 2 commits August 28, 2018 17:49

Build artifacts

7e33403

Mechanism for renewing a certificate based on an existing certificate

ab28409

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 Aug 28, 2018

kad reviewed Aug 29, 2018

View reviewed changes

Actually renew certificates (using on-disk CAs)

76be5ca

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 29, 2018

liztio force-pushed the cert-renewal branch from 1ffc1d3 to 948f7a0 Compare August 30, 2018 14:56

liztio changed the title ~~[wip] Kubeadm Cert Renewal~~ Kubeadm Cert Renewal Aug 30, 2018

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 Aug 30, 2018

liztio force-pushed the cert-renewal branch from 948f7a0 to d7bc0ed Compare August 30, 2018 15:26

Add renew all command

d21ed1a

liztio force-pushed the cert-renewal branch from d7bc0ed to d21ed1a Compare August 30, 2018 17:33

timothysc approved these changes Aug 31, 2018

View reviewed changes

k8s-ci-robot assigned timothysc Aug 31, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2018

timothysc added kind/feature Categorizes issue or PR as related to a new feature. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 31, 2018

k8s-github-robot merged commit 17dde46 into kubernetes:master Aug 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubeadm Cert Renewal #67910

Kubeadm Cert Renewal #67910

liztio commented Aug 27, 2018 •

edited

Loading

timothysc left a comment

timothysc Aug 27, 2018

timothysc Aug 27, 2018

neolit123 left a comment

neolit123 Aug 27, 2018

stealthybox Aug 29, 2018

neolit123 Aug 27, 2018

neolit123 Aug 27, 2018

neolit123 Aug 27, 2018

neolit123 Aug 27, 2018

neolit123 Aug 27, 2018

neolit123 Aug 27, 2018

kad Aug 29, 2018

liztio Aug 29, 2018

liztio commented Aug 30, 2018

liztio commented Aug 30, 2018

timothysc left a comment

k8s-ci-robot commented Aug 31, 2018

k8s-github-robot commented Aug 31, 2018

Kubeadm Cert Renewal #67910

Kubeadm Cert Renewal #67910

Conversation

liztio commented Aug 27, 2018 • edited Loading

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liztio commented Aug 30, 2018

liztio commented Aug 30, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 31, 2018

k8s-github-robot commented Aug 31, 2018

liztio commented Aug 27, 2018 •

edited

Loading