✨ Add CertificateValidityPeriod and CACertificateValidityPeriod to KubeadmConfig #12335

Karthik-K-N · 2025-06-09T17:00:54Z

What this PR does / why we need it:

Adds CertificateValidityPeriod and CACertificateValidityPeriod to KubeadmConfig

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

/area provider/bootstrap-kubeadm

k8s-ci-robot · 2025-06-09T17:00:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

Karthik-K-N · 2025-06-09T17:05:20Z

/hold

Added CertificateValidityPeriod to KubeadmConfig if the changes (especially conversions) are in right direction will do the similar changes for CACertificateValidityPeriod as well.

@sbueringer Please let me know your opinion. Thanks.

Karthik-K-N · 2025-06-09T17:09:10Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

@@ -207,6 +207,11 @@ type ClusterConfiguration struct {
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=63
 	ClusterName string `json:"clusterName,omitempty"`
+
+	// CertificateValidityPeriod specifies the validity period for a non-CA certificate generated by kubeadm.
+	// Default value: 8760h (365 days * 24 hours = 1 year)


Just copied the Default value from kubeadm, Should we add a logic to add default value or is it implicitly taken care by kubeadm and remove it from here?

Good question. I definitely want to document the default value here and I don't want to default directly on the API via OpenAPI defaulting or mutating webhook. So that we still know what the user set (if they set it)

I think maybe we just let kubeadm default it, but no strong opinion

@fabriziopandini WDYT?

Talked to Fabrizio, I'll suggest the full godoc comments after the next review

Karthik-K-N · 2025-06-11T14:51:41Z

Added both the fields, Need to make changes to honor those fields while generating CA.

sbueringer · 2025-06-11T18:32:03Z

@Karthik-K-N Sorry can you rebase again? I think now we got all PRs merged for now. I'll look into the PR afterwards

Karthik-K-N · 2025-06-12T06:06:16Z

@Karthik-K-N Sorry can you rebase again? I think now we got all PRs merged for now. I'll look into the PR afterwards

No issues, Thanks, I just rebased the PR, I have added both the fields with their conversions, I will just try to work on how to use these fields while generating certificate

Karthik-K-N · 2025-06-12T09:25:05Z

/hold cancel

Ready for initial review, UT is pending

sbueringer · 2025-06-12T11:44:39Z

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

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

sbueringer

Thank you for working on this

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

api/bootstrap/kubeadm/v1beta1/conversion_test.go

bootstrap/kubeadm/types/upstreamv1beta3/conversion_test.go

bootstrap/kubeadm/types/upstreamv1beta4/conversion.go

sbueringer · 2025-06-12T12:11:50Z

bootstrap/kubeadm/types/upstreamv1beta4/conversion_test.go

-	obj.CertificateValidityPeriod = nil
+
+	if obj.CertificateValidityPeriod != nil {
+		obj.CertificateValidityPeriod = ptr.To[metav1.Duration](metav1.Duration{Duration: time.Duration(c.Int31()) * time.Second})


This can be changed to time.Days when we change the fields on the v1beta2 api to *Days

Please check if max of c.Int31() * time.Days is already more than max of Duration. Then we don't have to do anything here.

We only had to add this for other cases because it was possible to have a Duration which was to high to be converted to Seconds int32

controlplane/kubeadm/internal/webhooks/kubeadm_control_plane_test.go

util/secret/certificates.go

Karthik-K-N · 2025-06-13T07:23:50Z

bootstrap/kubeadm/types/upstreamv1beta4/conversion_test.go

-	obj.CertificateValidityPeriod = nil
+
+	if obj.CertificateValidityPeriod != nil {
+		obj.CertificateValidityPeriod = ptr.To[metav1.Duration](metav1.Duration{Duration: time.Duration(c.Int31()%24) * time.Hour * 24})


Since there is no built in methods in time package to use time.Days(), I was facing round trip errors while converting hours to Days back and forth due to round off of minute field.
So added this way to always get whole hours rather than in minutes.

Please suggest if there are any better ways to handle this.

Karthik-K-N · 2025-06-13T07:26:22Z

Address all the review comments, The PR is ready for next review. Thanks.

k8s-ci-robot · 2025-06-13T15:27:28Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2025

k8s-ci-robot requested review from enxebre and richardcase June 9, 2025 17:00

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 9, 2025

Karthik-K-N commented Jun 9, 2025

View reviewed changes

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2025

Karthik-K-N commented Jun 9, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2025

Karthik-K-N force-pushed the kubeadm-task branch from 86bbf38 to 58d06f0 Compare June 11, 2025 07:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2025

Karthik-K-N force-pushed the kubeadm-task branch from 58d06f0 to ea0faae Compare June 11, 2025 08:54

sbueringer mentioned this pull request Jun 11, 2025

🐛 Fix continuous reconciles because of apiVersion differences in Cluster topology controller #12341

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2025

Karthik-K-N added 2 commits June 12, 2025 10:54

Add CertificateValidityPeriod type to KubeadmConfig

7f713bb

Add CACertificateValidityPeriod to Kubeadmconfig

29ee524

Karthik-K-N force-pushed the kubeadm-task branch from 75aca66 to 29ee524 Compare June 12, 2025 05:33

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2025

Use CACertificateValidityPeriod while generating certificates

1680d8a

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2025

sbueringer added area/provider/control-plane-kubeadm Issues or PRs related to KCP area/provider/bootstrap-kubeadm Issues or PRs related to CAPBK labels Jun 12, 2025

k8s-ci-robot removed do-not-merge/needs-area PR is missing an area label labels Jun 12, 2025

Karthik-K-N commented Jun 12, 2025

View reviewed changes

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go Outdated Show resolved Hide resolved

sbueringer reviewed Jun 12, 2025

View reviewed changes

Address reveiw comments

2834127

Karthik-K-N commented Jun 13, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2025

✨ Add CertificateValidityPeriod and CACertificateValidityPeriod to KubeadmConfig #12335

Are you sure you want to change the base?

✨ Add CertificateValidityPeriod and CACertificateValidityPeriod to KubeadmConfig #12335

Conversation

Karthik-K-N commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 9, 2025

Uh oh!

Uh oh!

Karthik-K-N commented Jun 9, 2025

Uh oh!

Karthik-K-N Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N commented Jun 11, 2025

Uh oh!

sbueringer commented Jun 11, 2025

Uh oh!

Karthik-K-N commented Jun 12, 2025

Uh oh!

Karthik-K-N commented Jun 12, 2025

Uh oh!

sbueringer commented Jun 12, 2025

Uh oh!

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbueringer Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Karthik-K-N Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N commented Jun 13, 2025

Uh oh!

k8s-ci-robot commented Jun 13, 2025

Uh oh!

Uh oh!

Karthik-K-N commented Jun 9, 2025 •

edited

Loading

sbueringer Jun 12, 2025 •

edited

Loading