Skip to content

✨ Automatically renew control plane machine certificates before expiration through machine repave #6983

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
Merged
merged 1 commit into from
Sep 19, 2022
Merged

✨ Automatically renew control plane machine certificates before expiration through machine repave #6983

merged 1 commit into from
Sep 19, 2022

Conversation

ykakarap
Copy link
Contributor

@ykakarap ykakarap commented Jul 26, 2022 •
edited
Loading

What this PR does / why we need it:

This PR achieves certificate rotation on control plane machine by repaving the machines.

It is achieved by doing the following:

  • Add an annotation (machine.cluster.x-k8s.io/certificates-expiry-date) on KubeadmBootstrapConfig objects that captures the certificates expiry date (1 year from the creation time)
  • Update the machine status with certificate expiry date by either reading that annotation on the machine object or the bootstrap config object.
  • Add a field to KCP called kcp.spec.rolloutBefore.certificatesExpiryDays that can be used to trigger a rollout if the control plane machine's certificates will expire within the specified days.

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 #6529

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 26, 2022
@ykakarap ykakarap changed the title ✨ [WIP][DO_NOT_REVIEW] MHC to mark unhealthy when certificates are about to expire for CP machines ✨ [WIP][DO_NOT_REVIEW] MHC to mark CP machines as unhealthy when certificates are about to expire Jul 26, 2022
@k8s-ci-robot 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 2, 2022
@ykakarap ykakarap changed the title ✨ [WIP][DO_NOT_REVIEW] MHC to mark CP machines as unhealthy when certificates are about to expire ✨ MHC to mark CP machines as unhealthy when certificates are about to expire Aug 3, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2022
sbueringer
sbueringer reviewed Aug 18, 2022
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very nice.

I think mostly nits from my side

Very nice feature and not a lot of changes necessary to make it happen.

Should we change the PR description to Part of. Given that the PR implements the feature but doesn't include other stuff like e.g. the column and the documentation?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2022
@ykakarap ykakarap force-pushed the certificate-rotation branch from d1f39be to 3c3fa0c Compare September 1, 2022 00:19
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2022
@ykakarap ykakarap force-pushed the certificate-rotation branch from 3c3fa0c to d6f18c4 Compare September 2, 2022 02:23
sbueringer
sbueringer reviewed Sep 2, 2022
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

did a high-level review on the delta between the time of my last review and now.

Should we flag the PR as WIP again for now?

@ykakarap
Copy link
Contributor Author

ykakarap commented Sep 2, 2022

/retitle ✨ [WIP] Automatically renew control plane machine certificates before expiration through machine repave

@k8s-ci-robot k8s-ci-robot changed the title ✨ MHC to mark CP machines as unhealthy when certificates are about to expire ✨ [WIP] Automatically renew control plane machine certificates before expiration through machine repave Sep 2, 2022
@sbueringer
Copy link
Member

Reviewed the delta, looks good so far. Once we finalized the API and the PR is otherwise ready I would do some in-depth manual testing and another detailed review

@ykakarap ykakarap changed the title ✨ [WIP] Automatically renew control plane machine certificates before expiration through machine repave ✨ Automatically renew control plane machine certificates before expiration through machine repave Sep 10, 2022
sbueringer
sbueringer reviewed Sep 12, 2022
View reviewed changes
@sbueringer
Copy link
Member

This looks pretty good!

Only nits, except (potentially) #6983 (comment)

fabriziopandini
fabriziopandini reviewed Sep 12, 2022
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

only nits

sbueringer
sbueringer reviewed Sep 13, 2022
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

2 nits otherwise delta looks perfect

This was referenced Sep 14, 2022
Closed
Closed
@sbueringer
Copy link
Member

Great work!

lgtm pending squash

@ykakarap ykakarap force-pushed the certificate-rotation branch from 2d49b40 to 0a93b4f Compare September 14, 2022 15:52
@ykakarap
Copy link
Contributor Author

Squashed.

@sbueringer
Copy link
Member

Thx!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2022
@sbueringer
Copy link
Member

lgtm pending squash

add KCP rollout support based on machine certificate expiry
bac8e27 
A new annotation is added to KubeadmConfig that captures the
certificate expiry information. This information is propagated to
machines. This new information can be used by KCP to perform a rollout
if the machine certificates are about to expire.

Note: The expiry time captured in KubeadmConfig is an approximate time
of when the certificates will expire but it is guaranteed to be before
the actual certificate expiry.
@ykakarap ykakarap force-pushed the certificate-rotation branch from 954be1f to bac8e27 Compare September 16, 2022 23:40
@ykakarap
Copy link
Contributor Author

Squashed.

@fabriziopandini
Copy link
Member

great job! this is a long-awaited feature for many CAPI users
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2022
@sbueringer
Copy link
Member

sbueringer commented Sep 19, 2022 •
edited
Loading

Great work!

/approve

Would be good to surface this feature somewhere in our book

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 Sep 19, 2022
@k8s-ci-robot k8s-ci-robot merged commit fd11302 into kubernetes-sigs:main Sep 19, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Sep 19, 2022
@fabriziopandini
Copy link
Member

Would be good to surface this feature somewhere in our book

@ykakarap do you mind opening an issue for this?

@ykakarap
Copy link
Contributor Author

Would be good to surface this feature somewhere in our book

@ykakarap do you mind opening an issue for this?

Issue: #7247

@ykakarap ykakarap mentioned this pull request Sep 21, 2022
Merged
@sbueringer sbueringer mentioned this pull request Sep 27, 2022
Closed
5 tasks
@ykakarap ykakarap mentioned this pull request Oct 4, 2022
Closed
@furkatgofurov7 furkatgofurov7 mentioned this pull request Dec 12, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@enxebre enxebre Awaiting requested review from enxebre

@killianmuldoon killianmuldoon Awaiting requested review from killianmuldoon

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

Automatically renew machine certificates
4 participants
@ykakarap @sbueringer @fabriziopandini @k8s-ci-robot