address review comments

Author: Yuvaraj Kakaraparthi

api/v1beta1/machine_types.go

@@ -52,6 +52,8 @@ const (
 	PreTerminateDeleteHookAnnotationPrefix = "pre-terminate.delete.hook.machine.cluster.x-k8s.io"
 
 	// MachineCertificatesExpiryDateAnnotation annotation specifies the expiry date of the machine certificates.
+	// This annotation can be used my MachineHealthCheck to trigger machine rotation before certificates expire.
+	// This annotation can be set on KubeadmConfig or Machine objects. The value set on the Machine object takes precedence.
 	MachineCertificatesExpiryDateAnnotation = "machine.cluster.x-k8s.io/certificates-expiry-date"
 )
 

api/v1beta1/machinehealthcheck_types.go

@@ -79,6 +79,7 @@ type MachineHealthCheckSpec struct {
 
 // ANCHOR: Certificates
 
+// Certificates contains the information needed to validate the health of machine certificates.
 type Certificates struct {
 	// ExpiresWithinDays is the number of days after which certificates is considered to be expired.
 	// +optional

api/v1beta1/zz_generated.openapi.go

@@ -443,7 +443,7 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex
 	}
 
 	// Update the certificate expiration time in the config.
-	r.reconcileCertificateExpiryTime(ctx, scope)
+	r.addCertificateExpiryAnnotation(scope.Config)
 
 	conditions.MarkTrue(scope.Config, bootstrapv1.CertificatesAvailableCondition)
 
@@ -626,7 +626,7 @@ func (r *KubeadmConfigReconciler) joinControlplane(ctx context.Context, scope *S
 	}
 
 	// Update the certificate expiration time in the config.
-	r.reconcileCertificateExpiryTime(ctx, scope)
+	r.addCertificateExpiryAnnotation(scope.Config)
 
 	conditions.MarkTrue(scope.Config, bootstrapv1.CertificatesAvailableCondition)
 
@@ -1023,16 +1023,16 @@ func (r *KubeadmConfigReconciler) storeBootstrapData(ctx context.Context, scope
 	return nil
 }
 
-// reconcileCertificateExpiryTime reconciles the certificate expiry time on Kubeadmconfig.
+// addCertificateExpiryAnnotation reconciles the certificate expiry time on Kubeadmconfig.
 // Sets the certificate expiration time as on annotation on the config.
 // If the annotation is already present, there is nothing to do.
-func (r *KubeadmConfigReconciler) reconcileCertificateExpiryTime(_ context.Context, scope *Scope) {
-	annotations := scope.Config.GetAnnotations()
+func (r *KubeadmConfigReconciler) addCertificateExpiryAnnotation(config *bootstrapv1.KubeadmConfig) {
+	annotations := config.GetAnnotations()
 	if annotations == nil {
 		annotations = map[string]string{}
 	}
 	if _, ok := annotations[clusterv1.MachineCertificatesExpiryDateAnnotation]; !ok {
 		annotations[clusterv1.MachineCertificatesExpiryDateAnnotation] = now().Add(defaultCertificateExpiryDuration).Format(time.RFC3339)
-		scope.Config.SetAnnotations(annotations)
+		config.SetAnnotations(annotations)
 	}
 }

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -2104,7 +2104,7 @@ func TestKubeadmConfigReconciler_ReconcileCertificateExpiryTime(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			k := &KubeadmConfigReconciler{}
-			k.reconcileCertificateExpiryTime(ctx, &Scope{Config: tt.cfg})
+			k.addCertificateExpiryAnnotation(tt.cfg)
 			annotations := tt.cfg.GetAnnotations()
 			g.Expect(annotations[clusterv1.MachineCertificatesExpiryDateAnnotation]).To(Equal(tt.wantTime))
 		})

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go

@@ -267,6 +267,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 		r.reconcileInfrastructure,
 		r.reconcileNode,
 		r.reconcileInterruptibleNodeLabel,
+		r.reconcileCertificateExpiry,
 	}
 
 	res := ctrl.Result{}

bootstrap/kubeadm/types/upstreamv1beta1/zz_generated.deepcopy.go

@@ -175,7 +175,7 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, cluster *clusterv1.
 	log := ctrl.LoggerFrom(ctx, "cluster", cluster.Name)
 
 	// If the bootstrap data is populated, set ready and return.
-	if m.Spec.Bootstrap.DataSecretName != nil && m.Spec.Bootstrap.ConfigRef == nil {
+	if m.Spec.Bootstrap.DataSecretName != nil {
 		m.Status.BootstrapReady = true
 		conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition)
 		return ctrl.Result{}, nil
@@ -222,37 +222,6 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, cluster *clusterv1.
 		return ctrl.Result{RequeueAfter: externalReadyWait}, nil
 	}
 
-	// If the machine is a control plane machine, check for certificate expiry information in the following places:
-	// - As an annotation on the Machine
-	// - As an annotation on the bootstrap config
-	// If the certificate expiry information is found, set the Machine's status accordingly.
-	// Note: If both values are defined the value in the machine annotation should take precedence.
-	if util.IsControlPlaneMachine(m) {
-		var annotations map[string]string
-
-		// Check for certificate expiry information in the bootstrap config.
-		annotations = bootstrapConfig.GetAnnotations()
-		if expiry, ok := annotations[clusterv1.MachineCertificatesExpiryDateAnnotation]; ok {
-			expiryTime, err := time.Parse(time.RFC3339, expiry)
-			if err != nil {
-				return ctrl.Result{}, errors.Wrap(err, "failed to parse expiry date")
-			}
-			expTime := metav1.NewTime(expiryTime)
-			m.Status.CertificatesExpiryDate = &expTime
-		}
-
-		// Check for certificate expiry information in the machine annotation.
-		annotations = m.GetAnnotations()
-		if expiry, ok := annotations[clusterv1.MachineCertificatesExpiryDateAnnotation]; ok {
-			expiryTime, err := time.Parse(time.RFC3339, expiry)
-			if err != nil {
-				return ctrl.Result{}, errors.Wrap(err, "failed to parse expiry date")
-			}
-			expTime := metav1.NewTime(expiryTime)
-			m.Status.CertificatesExpiryDate = &expTime
-		}
-	}
-
 	// Get and set the name of the secret containing the bootstrap data.
 	secretName, _, err := unstructured.NestedString(bootstrapConfig.Object, "status", "dataSecretName")
 	if err != nil {
@@ -261,11 +230,8 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, cluster *clusterv1.
 		return ctrl.Result{}, errors.Errorf("retrieved empty dataSecretName from bootstrap provider for Machine %q in namespace %q", m.Name, m.Namespace)
 	}
 
-	if m.Spec.Bootstrap.DataSecretName == nil {
-		m.Spec.Bootstrap.DataSecretName = pointer.StringPtr(secretName)
-	}
+	m.Spec.Bootstrap.DataSecretName = pointer.StringPtr(secretName)
 	m.Status.BootstrapReady = true
-	conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition)
 	return ctrl.Result{}, nil
 }
 
@@ -346,3 +312,44 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, cluster *clust
 	m.Spec.ProviderID = pointer.StringPtr(providerID)
 	return ctrl.Result{}, nil
 }
+
+func (r *Reconciler) reconcileCertificateExpiry(ctx context.Context, _ *clusterv1.Cluster, m *clusterv1.Machine) (ctrl.Result, error) {
+	var annotations map[string]string
+
+	if !util.IsControlPlaneMachine(m) {
+		// If the machine is not a control plane machine, return early.
+		return ctrl.Result{}, nil
+	}
+
+	if m.Spec.Bootstrap.ConfigRef != nil {
+		bootstrapConfig, err := external.Get(ctx, r.Client, m.Spec.Bootstrap.ConfigRef, m.Namespace)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+
+		// Check for certificate expiry information in the bootstrap config.
+		annotations = bootstrapConfig.GetAnnotations()
+		if expiry, ok := annotations[clusterv1.MachineCertificatesExpiryDateAnnotation]; ok {
+			expiryTime, err := time.Parse(time.RFC3339, expiry)
+			if err != nil {
+				return ctrl.Result{}, errors.Wrap(err, "failed to parse expiry date")
+			}
+			expTime := metav1.NewTime(expiryTime)
+			m.Status.CertificatesExpiryDate = &expTime
+		}
+	}
+
+	// Check for certificate expiry information in the machine annotation.
+	// This should take precedence over other information.
+	annotations = m.GetAnnotations()
+	if expiry, ok := annotations[clusterv1.MachineCertificatesExpiryDateAnnotation]; ok {
+		expiryTime, err := time.Parse(time.RFC3339, expiry)
+		if err != nil {
+			return ctrl.Result{}, errors.Wrap(err, "failed to parse expiry date")
+		}
+		expTime := metav1.NewTime(expiryTime)
+		m.Status.CertificatesExpiryDate = &expTime
+	}
+
+	return ctrl.Result{}, nil
+}