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: Refactor renewal package #77780

Merged

Conversation

@fabriziopandini
Copy link
Member

commented May 11, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
the code implemented for managing renewal is getting more and more complex after detecting external CA and renewing kubeConfig. This PR refactors this code concentrating all the complexity into one package and making more transparent all the renewal internals to its consumers.

This refactor fixes also an inconsistency between kubeadm alpha certs renew --csr-only and kubeadm alpha certs renew and sets the ground for implementing certs expiration checks in a cleaner and less invasive way

Which issue(s) this PR fixes:
Rif kubernetes/kubeadm#1361
Rif kubernetes/kubeadm#1540

Does this PR introduce a user-facing change?:

kubeadm: kubeadm alpha certs renew  --csr-only now reads the current certificates as the authoritative source for certificates attributes (same as kubeadm alpha certs renew)

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

/assign @rosti
/assign @timothysc

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 11, 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

@k8s-ci-robot k8s-ci-robot requested review from detiber and ereslibre May 11, 2019

@neolit123
Copy link
Member

left a comment

thanks @fabriziopandini
that's a pretty big refactor and the new manager logic seems valid to me.

i think we should start allowing separate commits per PR, to make the review easier.

cmd/kubeadm/app/cmd/alpha/certs.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/cmd/alpha/certs.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/cmd/alpha/certs.go Show resolved Hide resolved
cmd/kubeadm/app/phases/certs/renewal/manager.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/certs/renewal/readwriter.go Outdated Show resolved Hide resolved

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:refactor-renewal-package branch from 35af1f3 to de02424 May 12, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

@neolit123 thanks for the review!
All comment are addressed

@rosti
Copy link
Member

left a comment

Thanks @fabriziopandini !
Looks way better than before (especially for renewCertsByComponent)!
Found a tiny nit at the first pass, but other than that, I'll try to do another pass tomorrow as the change is large.

kubeadmutil.CheckErr(err)
// Get a renewal manager for a generic Cluster configuration, that is used only for getting
// the list of certificates for building subcommands
rm, _ := renewal.NewManager(&kubeadmapi.ClusterConfiguration{}, "")

This comment has been minimized.

Copy link
@rosti

rosti May 14, 2019

Member

Missing error check here?

This comment has been minimized.

Copy link
@rosti

rosti May 14, 2019

Member

Actually, the error that is returned by NewManager is not looked at anywhere.

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini May 15, 2019

Author Member

@rosti, good catch!
I somehow forgot to fix them
Now it is addressed

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:refactor-renewal-package branch from de02424 to 1c89787 May 15, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented May 15, 2019

/lgtm

LGTM-ing here to unblock the work.
retrospective reviews are fine, but we need to move forward.

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Uhmm.. test seems stuck
/retest

@k8s-ci-robot k8s-ci-robot merged commit e1770e6 into kubernetes:master May 16, 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

@fabriziopandini fabriziopandini deleted the fabriziopandini:refactor-renewal-package 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.