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

Kubeadm: Refactor renewal package #77780

Merged
merged 2 commits into from
May 16, 2019
Merged

Kubeadm: Refactor renewal package #77780

merged 2 commits into from
May 16, 2019

Conversation

fabriziopandini
Copy link
Member

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 k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 11, 2019
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/kubeadm priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 11, 2019
@k8s-ci-robot
Copy link
Contributor

[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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2019
neolit123
neolit123 reviewed May 11, 2019
View reviewed changes
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.

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/apirenewer_test.go Outdated 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
cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go Show resolved Hide resolved
cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go Outdated Show resolved Hide resolved
@fabriziopandini
Copy link
Member Author

@neolit123 thanks for the review!
All comment are addressed

@fabriziopandini fabriziopandini mentioned this pull request May 14, 2019
Merged
rosti
rosti reviewed May 14, 2019
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

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.

cmd/kubeadm/app/cmd/alpha/certs.go Outdated
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{}, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing error check here?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@neolit123
Copy link
Member

/lgtm

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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2019
@fabriziopandini
Copy link
Member Author

Uhmm.. test seems stuck
/retest

@k8s-ci-robot k8s-ci-robot merged commit e1770e6 into kubernetes:master May 16, 2019
@fabriziopandini fabriziopandini deleted the refactor-renewal-package branch May 28, 2019 10:26
@neolit123 neolit123 mentioned this pull request Jun 10, 2019
Closed
@SataQiu SataQiu mentioned this pull request Dec 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@rosti rosti rosti left review comments

@detiber detiber Awaiting requested review from detiber

@ereslibre ereslibre Awaiting requested review from ereslibre

Assignees

@timothysc timothysc

@neolit123 neolit123

@rosti rosti

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fabriziopandini @k8s-ci-robot @neolit123 @rosti @timothysc