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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃尡 Detect certificate expiry from kube-apiserver serving cert #7355

Merged
merged 1 commit into from
Oct 7, 2022
Merged

馃尡 Detect certificate expiry from kube-apiserver serving cert #7355

merged 1 commit into from
Oct 7, 2022

Conversation

sbueringer
Copy link
Member

Signed-off-by: Stefan B眉ringer buringerst@vmware.com

What this PR does / why we need it:

WIP: waiting for feedback before implementing unit tests

This PR changes the detection of the certificate expiry. Instead of setting it only for new machines
in CABPK now we detect it in KCP from the serving certificate of the kube-apiserver.

This has the big advantage that it allows detecting certificate expiry of pre-existing machines!

For more details see: #7342 (comment)

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Oct 6, 2022
@sbueringer
Copy link
Member Author

/assign @fabriziopandini @vincepri @ykakarap

Please take a look. Quick feedback would be really appreciated!

neolit123
neolit123 reviewed Oct 6, 2022
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.

i think this approach is ok

controlplane/kubeadm/internal/controllers/controller.go Show resolved Hide resolved
@sbueringer
Copy link
Member Author

sbueringer commented Oct 6, 2022 •
edited

/test pull-cluster-api-e2e-full-main

clusterctl upgrade test should be able to prove that existing machines get the expiry annotation after the upgrade

EDIT:
Regular quickstart test:

kind: KubeadmConfig
metadata:
  annotations:
    machine.cluster.x-k8s.io/certificates-expiry: "2023-10-06T06:11:17Z"
  creationTimestamp: "2022-10-06T06:11:04Z"

clusterctl upgrade using ClusterClass (v1.2=>current) [ClusterClass]:

apiVersion: bootstrap.cluster.x-k8s.io/v1beta1
kind: KubeadmConfig
metadata:
  annotations:
    machine.cluster.x-k8s.io/certificates-expiry: "2023-10-06T07:19:20Z"
  creationTimestamp: "2022-10-06T07:19:02Z"

fabriziopandini
fabriziopandini reviewed Oct 6, 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.

The approach looks good to me
We should fix also documentation (see #7268)

controlplane/kubeadm/internal/controllers/controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/workload_cluster.go Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member Author

The approach looks good to me We should fix also documentation (see #7268)

Yes. I put a hold on #7268 so we can adjust it before merge

@sbueringer sbueringer force-pushed the pr-detect-cert-expiry-from-apiserver branch from 5e6695a to 2b04c4e Compare October 6, 2022 14:06
@sbueringer
Copy link
Member Author

sbueringer commented Oct 6, 2022 •
edited

@fabriziopandini All findings should be adressed

/test pull-cluster-api-e2e-full-main

@sbueringer sbueringer force-pushed the pr-detect-cert-expiry-from-apiserver branch from 2b04c4e to f768132 Compare October 6, 2022 14:08
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

fabriziopandini
fabriziopandini reviewed Oct 6, 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.

/lgtm

controlplane/kubeadm/internal/workload_cluster.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2022
@sbueringer
Copy link
Member Author

/hold

Waiting for feedback from Vince and then I want to add some test coverage before merge.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2022
vincepri
vincepri approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

controlplane/kubeadm/internal/controllers/controller.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2022
@vincepri
Copy link
Member

vincepri commented Oct 6, 2022

Ready to unhold/unwip?

@sbueringer
Copy link
Member Author

sbueringer commented Oct 6, 2022 •
edited

I would add a few unit tests tomorrow and Fabrizio can lgtm afterwards

Primarily wanted the non-test code finalized/reviewed

ykakarap
ykakarap reviewed Oct 7, 2022
View reviewed changes
Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

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

The overall approach looks good 馃憤馃徏
This will be a nice improvements for the users.

/lgtm

@sbueringer
Detect certificate expiry from kube-apiserver serving cert
a7a4740 
Signed-off-by: Stefan B眉ringer buringerst@vmware.com
@sbueringer sbueringer force-pushed the pr-detect-cert-expiry-from-apiserver branch from f768132 to a7a4740 Compare October 7, 2022 11:19
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2022
@sbueringer sbueringer changed the title [WIP] 馃尡 Detect certificate expiry from kube-apiserver serving cert 馃尡 Detect certificate expiry from kube-apiserver serving cert Oct 7, 2022
@k8s-ci-robot 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 Oct 7, 2022
@sbueringer
Copy link
Member Author

Added unit test, now ready for merge

fabriziopandini
fabriziopandini reviewed Oct 7, 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.

Great work!
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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:
  • OWNERS [fabriziopandini,vincepri]

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

@sbueringer
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2022
@k8s-ci-robot k8s-ci-robot merged commit adb6819 into kubernetes-sigs:main Oct 7, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Oct 7, 2022
@sbueringer sbueringer deleted the pr-detect-cert-expiry-from-apiserver branch October 7, 2022 12:53
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

@fabriziopandini fabriziopandini fabriziopandini left review comments

@ykakarap ykakarap ykakarap left review comments

@vincepri vincepri vincepri approved these changes

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

Assignees

@vincepri vincepri

@fabriziopandini fabriziopandini

@ykakarap ykakarap

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

KCP should introspect machine certificate expiry information from node to enable automatic certificate renewal
6 participants
@sbueringer @vincepri @k8s-ci-robot @neolit123 @fabriziopandini @ykakarap