Skip to content

Commit 91b5228

Browse files
Karthik-K-NKarthik-K-N
committed
Address reveiw comments
1 parent acbf46e commit 91b5228

File tree

6 files changed

+140
-9
lines changed

6 files changed

+140
-9
lines changed

‎controlplane/kubeadm/internal/controllers/upgrade.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@ func (r *KubeadmControlPlaneReconciler) upgradeControlPlane(
7070
workloadCluster.UpdateFeatureGatesInKubeadmConfigMap(controlPlane.KCP.Spec.KubeadmConfigSpec, parsedVersion),
7171
workloadCluster.UpdateAPIServerInKubeadmConfigMap(controlPlane.KCP.Spec.KubeadmConfigSpec.ClusterConfiguration.APIServer),
7272
workloadCluster.UpdateControllerManagerInKubeadmConfigMap(controlPlane.KCP.Spec.KubeadmConfigSpec.ClusterConfiguration.ControllerManager),
73-
workloadCluster.UpdateSchedulerInKubeadmConfigMap(controlPlane.KCP.Spec.KubeadmConfigSpec.ClusterConfiguration.Scheduler))
73+
workloadCluster.UpdateSchedulerInKubeadmConfigMap(controlPlane.KCP.Spec.KubeadmConfigSpec.ClusterConfiguration.Scheduler),
74+
workloadCluster.UpdateCertificateValidityPeriodDays(controlPlane.KCP.Spec.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays))
7475

7576
// Etcd local and external are mutually exclusive and they cannot be switched, once set.
7677
if controlPlane.KCP.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil {

‎controlplane/kubeadm/internal/webhooks/kubeadm_control_plane.go

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,10 @@ const (
152152
timeouts = "timeouts"
153153
)
154154

155-
const minimumCertificatesExpiryDays = 7
155+
const (
156+
minimumCertificatesExpiryDays = 7
157+
defaultCACertificatesExpiryDays = 3650
158+
)
156159

157160
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
158161
func (webhook *KubeadmControlPlane) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
@@ -329,6 +332,27 @@ func validateKubeadmControlPlaneSpec(s controlplanev1.KubeadmControlPlaneSpec, p
329332
if s.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil {
330333
externalEtcd = true
331334
}
335+
path := field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration")
336+
if s.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays > s.KubeadmConfigSpec.ClusterConfiguration.CACertificateValidityPeriodDays {
337+
allErrs = append(
338+
allErrs,
339+
field.Invalid(
340+
path.Child("certificateValidityPeriodDays"),
341+
s.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays,
342+
fmt.Sprintf("must be less than or equal to caCertificateValidityPeriodDays %v", s.KubeadmConfigSpec.ClusterConfiguration.CACertificateValidityPeriodDays),
343+
),
344+
)
345+
}
346+
if s.KubeadmConfigSpec.ClusterConfiguration.CACertificateValidityPeriodDays == 0 && s.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays > defaultCACertificatesExpiryDays {
347+
allErrs = append(
348+
allErrs,
349+
field.Invalid(
350+
path.Child("certificateValidityPeriodDays"),
351+
s.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays,
352+
fmt.Sprintf("must be less than or equal to default value of caCertificateValidityPeriodDays %v", defaultCACertificatesExpiryDays),
353+
),
354+
)
355+
}
332356
}
333357

334358
if !externalEtcd {
@@ -381,16 +405,17 @@ func validateKubeadmControlPlaneSpec(s controlplanev1.KubeadmControlPlaneSpec, p
381405
allErrs = append(allErrs, field.Invalid(pathPrefix.Child("version"), s.Version, "must be a valid semantic version"))
382406
}
383407

384-
allErrs = append(allErrs, validateRolloutBefore(s.RolloutBefore, pathPrefix.Child("rolloutBefore"))...)
408+
allErrs = append(allErrs, validateRolloutBefore(s.RolloutBefore, s.KubeadmConfigSpec.ClusterConfiguration, pathPrefix.Child("rolloutBefore"))...)
385409
allErrs = append(allErrs, validateRolloutStrategy(s.RolloutStrategy, s.Replicas, pathPrefix.Child("rolloutStrategy"))...)
386410

387411
if s.MachineNamingStrategy != nil {
388412
allErrs = append(allErrs, validateNamingStrategy(s.MachineNamingStrategy, pathPrefix.Child("machineNamingStrategy"))...)
389413
}
414+
390415
return allErrs
391416
}
392417

393-
func validateRolloutBefore(rolloutBefore *controlplanev1.RolloutBefore, pathPrefix *field.Path) field.ErrorList {
418+
func validateRolloutBefore(rolloutBefore *controlplanev1.RolloutBefore, clusterConfiguration *bootstrapv1.ClusterConfiguration, pathPrefix *field.Path) field.ErrorList {
394419
allErrs := field.ErrorList{}
395420

396421
if rolloutBefore == nil {
@@ -401,6 +426,16 @@ func validateRolloutBefore(rolloutBefore *controlplanev1.RolloutBefore, pathPref
401426
if *rolloutBefore.CertificatesExpiryDays < minimumCertificatesExpiryDays {
402427
allErrs = append(allErrs, field.Invalid(pathPrefix.Child("certificatesExpiryDays"), *rolloutBefore.CertificatesExpiryDays, fmt.Sprintf("must be greater than or equal to %v", minimumCertificatesExpiryDays)))
403428
}
429+
430+
if clusterConfiguration == nil {
431+
return allErrs
432+
}
433+
434+
if clusterConfiguration.CertificateValidityPeriodDays != 0 {
435+
if *rolloutBefore.CertificatesExpiryDays < clusterConfiguration.CertificateValidityPeriodDays {
436+
allErrs = append(allErrs, field.Invalid(pathPrefix.Child("certificatesExpiryDays"), *rolloutBefore.CertificatesExpiryDays, fmt.Sprintf("must be greater than or equal to certificateValidityPeriodDays %v", clusterConfiguration.CertificateValidityPeriodDays)))
437+
}
438+
}
404439
}
405440

406441
return allErrs

‎controlplane/kubeadm/internal/webhooks/kubeadm_control_plane_test.go

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -337,8 +337,8 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
337337
ImageTag: "1.6.5",
338338
},
339339
},
340-
CertificateValidityPeriodDays: 365,
341-
CACertificateValidityPeriodDays: 365,
340+
CertificateValidityPeriodDays: 6,
341+
CACertificateValidityPeriodDays: 6,
342342
},
343343
JoinConfiguration: &bootstrapv1.JoinConfiguration{
344344
NodeRegistration: bootstrapv1.NodeRegistrationOptions{
@@ -743,13 +743,23 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
743743
unsetTimeouts.Spec.KubeadmConfigSpec.JoinConfiguration.Timeouts = nil
744744

745745
certificateValidityPeriod := before.DeepCopy()
746-
certificateValidityPeriod.Spec.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays = 23456
746+
certificateValidityPeriod.Spec.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays = 5
747747

748748
invalidUpdateCACertificateValidityPeriodDays := before.DeepCopy()
749749
invalidUpdateCACertificateValidityPeriodDays.Spec.KubeadmConfigSpec.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{
750-
CACertificateValidityPeriodDays: 23456,
750+
CACertificateValidityPeriodDays: 4,
751751
}
752752

753+
invalidRolloutBeforeCertificatesExpiryDays := before.DeepCopy()
754+
invalidRolloutBeforeCertificatesExpiryDays.Spec.RolloutBefore.CertificatesExpiryDays = ptr.To[int32](2)
755+
756+
invalidCertificateValidityPeriodDays := before.DeepCopy()
757+
invalidCertificateValidityPeriodDays.Spec.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays = 300
758+
759+
invalidCertificateValidityPeriod := before.DeepCopy()
760+
invalidCertificateValidityPeriod.Spec.KubeadmConfigSpec.ClusterConfiguration.CACertificateValidityPeriodDays = 0
761+
invalidCertificateValidityPeriod.Spec.KubeadmConfigSpec.ClusterConfiguration.CertificateValidityPeriodDays = 3651
762+
753763
tests := []struct {
754764
name string
755765
enableIgnitionFeature bool
@@ -1119,6 +1129,24 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
11191129
before: before,
11201130
kcp: invalidUpdateCACertificateValidityPeriodDays,
11211131
},
1132+
{
1133+
name: "should return error when rolloutBefore CertificatesExpiryDays less than cluster CertificateValidityPeriodDays",
1134+
expectErr: true,
1135+
before: before,
1136+
kcp: invalidRolloutBeforeCertificatesExpiryDays,
1137+
},
1138+
{
1139+
name: "should return error when CertificateValidityPeriodDays greater than CACertificateValidityPeriodDays",
1140+
expectErr: true,
1141+
before: before,
1142+
kcp: invalidCertificateValidityPeriodDays,
1143+
},
1144+
{
1145+
name: "should return error when CertificateValidityPeriodDays greater than CACertificateValidityPeriodDays",
1146+
expectErr: true,
1147+
before: before,
1148+
kcp: invalidCertificateValidityPeriod,
1149+
},
11221150
}
11231151

11241152
for _, tt := range tests {

‎controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func (webhook *KubeadmControlPlaneTemplate) ValidateDelete(_ context.Context, _
139139
func validateKubeadmControlPlaneTemplateResourceSpec(s controlplanev1.KubeadmControlPlaneTemplateResourceSpec, pathPrefix *field.Path) field.ErrorList {
140140
allErrs := field.ErrorList{}
141141

142-
allErrs = append(allErrs, validateRolloutBefore(s.RolloutBefore, pathPrefix.Child("rolloutBefore"))...)
142+
allErrs = append(allErrs, validateRolloutBefore(s.RolloutBefore, s.KubeadmConfigSpec.ClusterConfiguration, pathPrefix.Child("rolloutBefore"))...)
143143
allErrs = append(allErrs, validateRolloutStrategy(s.RolloutStrategy, nil, pathPrefix.Child("rolloutStrategy"))...)
144144
if s.MachineNamingStrategy != nil {
145145
allErrs = append(allErrs, validateNamingStrategy(s.MachineNamingStrategy, pathPrefix.Child("machineNamingStrategy"))...)

‎controlplane/kubeadm/internal/workload_cluster.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ type WorkloadCluster interface {
9494
UpdateAPIServerInKubeadmConfigMap(apiServer bootstrapv1.APIServer) func(*bootstrapv1.ClusterConfiguration)
9595
UpdateControllerManagerInKubeadmConfigMap(controllerManager bootstrapv1.ControllerManager) func(*bootstrapv1.ClusterConfiguration)
9696
UpdateSchedulerInKubeadmConfigMap(scheduler bootstrapv1.Scheduler) func(*bootstrapv1.ClusterConfiguration)
97+
UpdateCertificateValidityPeriodDays(certificateValidityPeriodDays int32) func(*bootstrapv1.ClusterConfiguration)
9798
UpdateKubeProxyImageInfo(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane) error
9899
UpdateCoreDNS(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane) error
99100
RemoveEtcdMemberForMachine(ctx context.Context, machine *clusterv1.Machine) error
@@ -219,6 +220,13 @@ func (w *Workload) UpdateSchedulerInKubeadmConfigMap(scheduler bootstrapv1.Sched
219220
}
220221
}
221222

223+
// UpdateCertificateValidityPeriodDays updates CertificateValidityPeriodDays in kubeadm config map.
224+
func (w *Workload) UpdateCertificateValidityPeriodDays(certificateValidityPeriodDays int32) func(*bootstrapv1.ClusterConfiguration) {
225+
return func(c *bootstrapv1.ClusterConfiguration) {
226+
c.CertificateValidityPeriodDays = certificateValidityPeriodDays
227+
}
228+
}
229+
222230
// UpdateClusterConfiguration gets the ClusterConfiguration kubeadm-config ConfigMap, converts it to the
223231
// Cluster API representation, and then applies a mutation func; if changes are detected, the
224232
// data are converted back into the Kubeadm API version in use for the target Kubernetes version and the

‎controlplane/kubeadm/internal/workload_cluster_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1025,6 +1025,65 @@ func TestUpdateFeatureGatesInKubeadmConfigMap(t *testing.T) {
10251025
}
10261026
}
10271027

1028+
func TestUpdateCertificateValidityPeriodDaysInKubeadmConfigMap(t *testing.T) {
1029+
tests := []struct {
1030+
name string
1031+
clusterConfigurationData string
1032+
certificateValidityDays int32
1033+
wantClusterConfiguration string
1034+
}{
1035+
{
1036+
name: "it should set the certificateValidityDays in config",
1037+
certificateValidityDays: int32(5),
1038+
clusterConfigurationData: utilyaml.Raw(`
1039+
apiVersion: kubeadm.k8s.io/v1beta3
1040+
kind: ClusterConfiguration
1041+
`),
1042+
wantClusterConfiguration: utilyaml.Raw(`
1043+
apiServer: {}
1044+
apiVersion: kubeadm.k8s.io/v1beta4
1045+
certificateValidityPeriod: 120h0m0s
1046+
controllerManager: {}
1047+
dns: {}
1048+
etcd: {}
1049+
kind: ClusterConfiguration
1050+
kubernetesVersion: v1.33.1
1051+
networking: {}
1052+
proxy: {}
1053+
scheduler: {}
1054+
`),
1055+
},
1056+
}
1057+
for _, tt := range tests {
1058+
t.Run(tt.name, func(t *testing.T) {
1059+
g := NewWithT(t)
1060+
fakeClient := fake.NewClientBuilder().WithObjects(&corev1.ConfigMap{
1061+
ObjectMeta: metav1.ObjectMeta{
1062+
Name: kubeadmConfigKey,
1063+
Namespace: metav1.NamespaceSystem,
1064+
},
1065+
Data: map[string]string{
1066+
clusterConfigurationKey: tt.clusterConfigurationData,
1067+
},
1068+
}).Build()
1069+
1070+
w := &Workload{
1071+
Client: fakeClient,
1072+
}
1073+
err := w.UpdateClusterConfiguration(ctx, semver.MustParse("1.33.1"), w.UpdateCertificateValidityPeriodDays(tt.certificateValidityDays))
1074+
g.Expect(err).ToNot(HaveOccurred())
1075+
1076+
var actualConfig corev1.ConfigMap
1077+
g.Expect(w.Client.Get(
1078+
ctx,
1079+
client.ObjectKey{Name: kubeadmConfigKey, Namespace: metav1.NamespaceSystem},
1080+
&actualConfig,
1081+
)).To(Succeed())
1082+
g.Expect(actualConfig.Data[clusterConfigurationKey]).Should(Equal(tt.wantClusterConfiguration), cmp.Diff(tt.wantClusterConfiguration, actualConfig.Data[clusterConfigurationKey]))
1083+
})
1084+
}
1085+
}
1086+
10281087
func TestDefaultFeatureGates(t *testing.T) {
10291088
tests := []struct {
10301089
name string

0 commit comments

Comments
 (0)