✨ 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

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.

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

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.

JoelSpeed · 2025-06-30T17:52:02Z

/lgtm

k8s-ci-robot · 2025-06-30T17:52:10Z

LGTM label has been added.

Git tree hash: e5fdec0cee28873da6d595d3c1e08f0db9640aba

sbueringer · 2025-06-30T17:54:07Z

Will try to take another look soon (I currently assume worst case next week)

sbueringer · 2025-07-08T08:57:02Z

@Karthik-K-N Can you rebase the PR please? :) I'll review directly afterwards

k8s-ci-robot · 2025-07-08T14:34:36Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2025-07-08T14:34:40Z

[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 ask for approval from joelspeed. 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

Karthik-K-N · 2025-07-08T14:36:17Z

@Karthik-K-N Can you rebase the PR please? :) I'll review directly afterwards

Done. Thanks

sbueringer · 2025-07-09T09:27:43Z

/test pull-cluster-api-e2e-main

sbueringer

Thank you very much!

sbueringer · 2025-07-09T09:32:02Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

@@ -180,6 +180,20 @@ type ClusterConfiguration struct {
 	// featureGates enabled by the user.
 	// +optional
 	FeatureGates map[string]bool `json:"featureGates,omitempty"`
+
+	// certificateValidityPeriodDays specifies the validity period for a non-CA certificate generated by kubeadm.
+	// If not specified, kubeadm will use a default of 3650 days (10 years).


Suggested change

// If not specified, kubeadm will use a default of 3650 days (10 years).

// If not specified, kubeadm will use a default of 365 days (1 year).

sbueringer · 2025-07-09T09:35:55Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

+	// If not specified, kubeadm will use a default of 3650 days (10 years).
+	// +optional
+	// +kubebuilder:validation:Minimum=1
+	// +kubebuilder:validation:Maximum=10950


Suggested change

// +kubebuilder:validation:Maximum=10950

// +kubebuilder:validation:Maximum=1095

APIserver won't survive more than 3 years + 1 day => See kubernetes/kubernetes#130047 (comment)

sbueringer · 2025-07-09T09:37:31Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

+	// +kubebuilder:validation:Maximum=10950
+	CertificateValidityPeriodDays int32 `json:"certificateValidityPeriodDays,omitempty"`
+
+	// caCertificateValidityPeriodDays specifies the validity period for a CA certificate generated by kubeadm.


Suggested change

// caCertificateValidityPeriodDays specifies the validity period for a CA certificate generated by kubeadm.

// caCertificateValidityPeriodDays specifies the validity period for CA certificates generated by kubeadm.

sbueringer · 2025-07-09T09:37:45Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

@@ -180,6 +180,20 @@ type ClusterConfiguration struct {
 	// featureGates enabled by the user.
 	// +optional
 	FeatureGates map[string]bool `json:"featureGates,omitempty"`
+
+	// certificateValidityPeriodDays specifies the validity period for a non-CA certificate generated by kubeadm.


Suggested change

// certificateValidityPeriodDays specifies the validity period for a non-CA certificate generated by kubeadm.

// certificateValidityPeriodDays specifies the validity period for non-CA certificates generated by kubeadm.

sbueringer · 2025-07-09T09:38:56Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

+	// If not specified, kubeadm will use a default of 3650 days (10 years).
+	// +optional
+	// +kubebuilder:validation:Minimum=1
+	// +kubebuilder:validation:Maximum=10950


Suggested change

// +kubebuilder:validation:Maximum=10950

// +kubebuilder:validation:Maximum=36500

Let's use 100 years instead of 30 years. Rotating CA certificates is very painful today.

sbueringer · 2025-07-09T09:48:07Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

+
+	// certificateValidityPeriodDays specifies the validity period for a non-CA certificate generated by kubeadm.
+	// If not specified, kubeadm will use a default of 3650 days (10 years).
+	// +optional


Suggested change

// +optional

// This field is only supported with Kubernetes v1.31 or above.

// +optional

(This only applies to this field, as the other field is implemented by CAPI controllers)

sbueringer · 2025-07-09T10:01:51Z

bootstrap/kubeadm/types/upstreamv1beta4/conversion_test.go

+	obj.CertificateValidityPeriodDays %= 24
+	obj.CACertificateValidityPeriodDays %= 24


I assume this avoids an overflow in metav1.Duration. Can we use this instead?

obj.CertificateValidityPeriodDays = c.Int31n(3 * 365 + 1) obj.CACertificateValidityPeriodDays = c.Int31n(100 * 365 + 1)

This limits the days to our supported min/max range

sbueringer · 2025-07-09T10:06:32Z

bootstrap/kubeadm/types/upstreamv1beta4/conversion_test.go

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


Similar here. Should we use this?

obj.CertificateValidityPeriod = ptr.To[metav1.Duration](metav1.Duration{Duration: time.Duration(c.Int31n(3*365)+1) * time.Hour * 24}) obj.CACertificateValidityPeriod = ptr.To[metav1.Duration](metav1.Duration{Duration: time.Duration(c.Int31n(100*365)+1) * time.Hour * 24})

Currently we only use 1-24 days

sbueringer · 2025-07-09T10:10:00Z

controlplane/kubeadm/internal/webhooks/kubeadm_control_plane_test.go

@@ -740,6 +740,14 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 	unsetTimeouts.Spec.KubeadmConfigSpec.InitConfiguration.Timeouts = nil
 	unsetTimeouts.Spec.KubeadmConfigSpec.JoinConfiguration.Timeouts = nil

+	certificateValidityPeriod := before.DeepCopy()


Let's also set this and CACertificateValidityPeriodDays in before please

sbueringer · 2025-07-09T10:21:50Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

+	CertificateValidityPeriodDays int32 `json:"certificateValidityPeriodDays,omitempty"`
+
+	// caCertificateValidityPeriodDays specifies the validity period for a CA certificate generated by kubeadm.
+	// If not specified, kubeadm will use a default of 3650 days (10 years).


Suggested change

// If not specified, kubeadm will use a default of 3650 days (10 years).

// If not specified, Cluster API will use a default of 3650 days (10 years).

// This field cannot be modified.

(It is us in this case, not kubeadm)

sbueringer · 2025-07-09T11:21:36Z

controlplane/kubeadm/internal/webhooks/kubeadm_control_plane.go

@@ -179,6 +179,7 @@ func (webhook *KubeadmControlPlane) ValidateUpdate(_ context.Context, oldObj, ne
 		{spec, kubeadmConfigSpec, clusterConfiguration, controllerManager, "*"},
 		{spec, kubeadmConfigSpec, clusterConfiguration, scheduler},


We need additional validation (+ corresponding test coverage)

validateRolloutBefore:

If rolloutBefore.CertificatesExpiryDays is set we have to ensure certificateValidityPeriodDays > rolloutBefore.CertificatesExpiryDays.

Otherwise control plane Machines are continuously rolled out because the cert expires too soon

Example: CertificatesExpiryDays is 14 and certificateValidityPeriodDays is 10 => certs will only be valid for 10 days, rolloutBefore triggers because certs are valid <= 14 days

validateKubeadmControlPlaneSpec

CertificateValidityPeriodDays <= CACertificateValidityPeriodDays

if CACertificateValidityPeriodDays is not set (==0) CertificateValidityPeriodDays <= the default value for CACertificateValidityPeriodDays

sbueringer · 2025-07-09T11:29:16Z

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

@@ -180,6 +180,20 @@ type ClusterConfiguration struct {
 	// featureGates enabled by the user.
 	// +optional


[had to put the comment somewhere]

I tested changes to certificateValidityPeriodDays, unfortunately it doesn't work as expected

A change to certificateValidityPeriodDays triggers a rollout but new Machines are not using the new value of certificateValidityPeriodDays.

I think we have to update the kubeadm-config ConfigMap in the wl cluster so kubeadm picks up the change.

We should add a new mutator somewhere around here:

cluster-api/controlplane/kubeadm/internal/controllers/upgrade.go

Line 69 in 686d66b

workloadCluster.UpdateImageRepositoryInKubeadmConfigMap(imageRepository),

This can be implemented similar to UpdateImageRepositoryInKubeadmConfigMap (please also add test coverage). I think we can simply set whatever is set on controlPlane.KCP.Spec.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays.

Let me know if you have any questions :)

k8s-ci-robot · 2025-07-09T11:44:29Z

@Karthik-K-N: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-e2e-main	`9582a44`	link	true	`/test pull-cluster-api-e2e-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

sbueringer · 2025-07-09T12:30:21Z

Re: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api/12335/pull-cluster-api-e2e-main/1942878337315639296

e2e tests are fine, the one failure is currently expected

Karthik-K-N · 2025-07-14T15:37:24Z

Addressed all the review comments, Please take a look. Please let me know if I need to add any more test cases. thanks

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

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

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

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

Karthik-K-N commented Jun 13, 2025

View reviewed changes

k8s-ci-robot assigned JoelSpeed Jun 30, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2025

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

Karthik-K-N added 7 commits July 8, 2025 19:26

Add CertificateValidityPeriod type to KubeadmConfig

e47d59d

Add CACertificateValidityPeriod to Kubeadmconfig

ad0b245

Use CACertificateValidityPeriod while generating certificates

9da25c6

Address reveiw comments

905f389

Address review comments

0954d21

Change data type from *int32 to int32

b4a1ef0

Rebase with main

9582a44

Karthik-K-N force-pushed the kubeadm-task branch from 7169ebe to 9582a44 Compare July 8, 2025 14:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2025

k8s-ci-robot requested a review from JoelSpeed July 8, 2025 14:34

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

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 9, 2025

sbueringer reviewed Jul 9, 2025

View reviewed changes

Karthik-K-N added 2 commits July 14, 2025 13:54

Address review comments

acbf46e

Address reveiw comments

8819693

	// If not specified, kubeadm will use a default of 3650 days (10 years).
	// If not specified, kubeadm will use a default of 365 days (1 year).

	// +kubebuilder:validation:Maximum=10950
	// +kubebuilder:validation:Maximum=1095

	// caCertificateValidityPeriodDays specifies the validity period for a CA certificate generated by kubeadm.
	// caCertificateValidityPeriodDays specifies the validity period for CA certificates generated by kubeadm.

	// certificateValidityPeriodDays specifies the validity period for a non-CA certificate generated by kubeadm.
	// certificateValidityPeriodDays specifies the validity period for non-CA certificates generated by kubeadm.

	// +kubebuilder:validation:Maximum=10950
	// +kubebuilder:validation:Maximum=36500

	// +optional
	// This field is only supported with Kubernetes v1.31 or above.
	// +optional

		obj.CertificateValidityPeriodDays %= 24
		obj.CACertificateValidityPeriodDays %= 24

	// If not specified, kubeadm will use a default of 3650 days (10 years).
	// If not specified, Cluster API will use a default of 3650 days (10 years).
	// This field cannot be modified.

		@@ -179,6 +179,7 @@ func (webhook *KubeadmControlPlane) ValidateUpdate(_ context.Context, oldObj, ne
		{spec, kubeadmConfigSpec, clusterConfiguration, controllerManager, "*"},
		{spec, kubeadmConfigSpec, clusterConfiguration, scheduler},

		@@ -180,6 +180,20 @@ type ClusterConfiguration struct {
		// featureGates enabled by the user.
		// +optional

✨ 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!

Uh oh!

Karthik-K-N commented Jun 9, 2025

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Jun 30, 2025

Uh oh!

k8s-ci-robot commented Jun 30, 2025

Uh oh!

sbueringer commented Jun 30, 2025

Uh oh!

sbueringer commented Jul 8, 2025

Uh oh!

k8s-ci-robot commented Jul 8, 2025

Uh oh!

k8s-ci-robot commented Jul 8, 2025

Uh oh!

Karthik-K-N commented Jul 8, 2025

Uh oh!

sbueringer commented Jul 9, 2025

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Karthik-K-N commented Jun 9, 2025 •

edited

Loading

sbueringer commented Jul 9, 2025 •

edited

Loading