Update MachinePool bootstrap dataSecretName when bootstrap config changes

Author: Jont828

exp/internal/controllers/machinepool_controller_phases.go

@@ -197,48 +197,52 @@ func (r *MachinePoolReconciler) reconcileBootstrap(ctx context.Context, cluster
 			return ctrl.Result{}, nil
 		}
 		bootstrapConfig = bootstrapReconcileResult.Result
-	}
 
-	// If the bootstrap data secret is populated, set ready and return.
-	if m.Spec.Template.Spec.Bootstrap.DataSecretName != nil {
-		m.Status.BootstrapReady = true
-		conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition)
-		return ctrl.Result{}, nil
-	}
+		// If the bootstrap config is being deleted, return early.
+		if !bootstrapConfig.GetDeletionTimestamp().IsZero() {
+			return ctrl.Result{}, nil
+		}
 
-	// If the bootstrap config is being deleted, return early.
-	if !bootstrapConfig.GetDeletionTimestamp().IsZero() {
-		return ctrl.Result{}, nil
-	}
+		// Determine if the bootstrap provider is ready.
+		ready, err := external.IsReady(bootstrapConfig)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
 
-	// Determine if the bootstrap provider is ready.
-	ready, err := external.IsReady(bootstrapConfig)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
+		// Report a summary of current status of the bootstrap object defined for this machine pool.
+		conditions.SetMirror(m, clusterv1.BootstrapReadyCondition,
+			conditions.UnstructuredGetter(bootstrapConfig),
+			conditions.WithFallbackValue(ready, clusterv1.WaitingForDataSecretFallbackReason, clusterv1.ConditionSeverityInfo, ""),
+		)
 
-	// Report a summary of current status of the bootstrap object defined for this machine pool.
-	conditions.SetMirror(m, clusterv1.BootstrapReadyCondition,
-		conditions.UnstructuredGetter(bootstrapConfig),
-		conditions.WithFallbackValue(ready, clusterv1.WaitingForDataSecretFallbackReason, clusterv1.ConditionSeverityInfo, ""),
-	)
+		if !ready {
+			log.V(2).Info("Bootstrap provider is not ready, requeuing")
+			m.Status.BootstrapReady = ready
+			return ctrl.Result{RequeueAfter: externalReadyWait}, nil
+		}
 
-	if !ready {
-		log.V(2).Info("Bootstrap provider is not ready, requeuing")
-		return ctrl.Result{RequeueAfter: externalReadyWait}, nil
+		// Get and set the name of the secret containing the bootstrap data.
+		secretName, _, err := unstructured.NestedString(bootstrapConfig.Object, "status", "dataSecretName")
+		if err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve dataSecretName from bootstrap provider for MachinePool %q in namespace %q", m.Name, m.Namespace)
+		} else if secretName == "" {
+			return ctrl.Result{}, errors.Errorf("retrieved empty dataSecretName from bootstrap provider for MachinePool %q in namespace %q", m.Name, m.Namespace)
+		}
+
+		m.Spec.Template.Spec.Bootstrap.DataSecretName = pointer.String(secretName)
+		m.Status.BootstrapReady = true
+		return ctrl.Result{}, nil
 	}
 
-	// Get and set the name of the secret containing the bootstrap data.
-	secretName, _, err := unstructured.NestedString(bootstrapConfig.Object, "status", "dataSecretName")
-	if err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve dataSecretName from bootstrap provider for MachinePool %q in namespace %q", m.Name, m.Namespace)
-	} else if secretName == "" {
-		return ctrl.Result{}, errors.Errorf("retrieved empty dataSecretName from bootstrap provider for MachinePool %q in namespace %q", m.Name, m.Namespace)
+	// If dataSecretName is set without a ConfigRef, this means the user brought their own bootstrap data.
+	if m.Spec.Template.Spec.Bootstrap.DataSecretName != nil {
+		m.Status.BootstrapReady = true
+		conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition)
+		return ctrl.Result{}, nil
 	}
 
-	m.Spec.Template.Spec.Bootstrap.DataSecretName = pointer.String(secretName)
-	m.Status.BootstrapReady = true
-	return ctrl.Result{}, nil
+	// This should never happen because the MachinePool webhook would not allow neither ConfigRef nor DataSecretName to be set.]
+	return ctrl.Result{}, errors.Errorf("neither .spec.bootstrap.configRef nor .spec.bootstrap.dataSecretName are set for MachinePool %q in namespace %q", m.Name, m.Namespace)
 }
 
 // reconcileInfrastructure reconciles the Spec.InfrastructureRef object on a MachinePool.

exp/internal/controllers/machinepool_controller_phases_test.go

@@ -605,7 +605,7 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
 			expectError: true,
 		},
 		{
-			name: "existing machinepool, bootstrap data should not change",
+			name: "existing machinepool with config ref, update data secret name",
 			bootstrapConfig: map[string]interface{}{
 				"kind":       builder.TestBootstrapConfigKind,
 				"apiVersion": builder.BootstrapGroupVersion.String(),
@@ -643,13 +643,52 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
 				},
 			},
 			expectError: false,
+			expected: func(g *WithT, m *expv1.MachinePool) {
+				g.Expect(m.Status.BootstrapReady).To(BeTrue())
+				g.Expect(*m.Spec.Template.Spec.Bootstrap.DataSecretName).To(Equal("secret-data"))
+			},
+		},
+		{
+			name: "existing machinepool without config ref, do not update data secret name",
+			bootstrapConfig: map[string]interface{}{
+				"kind":       builder.TestBootstrapConfigKind,
+				"apiVersion": builder.BootstrapGroupVersion.String(),
+				"metadata": map[string]interface{}{
+					"name":      "bootstrap-config1",
+					"namespace": metav1.NamespaceDefault,
+				},
+				"spec": map[string]interface{}{},
+				"status": map[string]interface{}{
+					"ready":          true,
+					"dataSecretName": "secret-data",
+				},
+			},
+			machinepool: &expv1.MachinePool{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "bootstrap-test-existing",
+					Namespace: metav1.NamespaceDefault,
+				},
+				Spec: expv1.MachinePoolSpec{
+					Template: clusterv1.MachineTemplateSpec{
+						Spec: clusterv1.MachineSpec{
+							Bootstrap: clusterv1.Bootstrap{
+								DataSecretName: pointer.String("data"),
+							},
+						},
+					},
+				},
+				Status: expv1.MachinePoolStatus{
+					BootstrapReady: true,
+				},
+			},
+			expectError: false,
 			expected: func(g *WithT, m *expv1.MachinePool) {
 				g.Expect(m.Status.BootstrapReady).To(BeTrue())
 				g.Expect(*m.Spec.Template.Spec.Bootstrap.DataSecretName).To(Equal("data"))
 			},
 		},
 		{
-			name: "existing machinepool, bootstrap provider is to not ready",
+			name: "existing machinepool, bootstrap provider is not ready",
 			bootstrapConfig: map[string]interface{}{
 				"kind":       builder.TestBootstrapConfigKind,
 				"apiVersion": builder.BootstrapGroupVersion.String(),
@@ -683,12 +722,13 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
 					},
 				},
 				Status: expv1.MachinePoolStatus{
-					BootstrapReady: true,
+					BootstrapReady: false,
 				},
 			},
-			expectError: false,
+			expectError:  false,
+			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
 			expected: func(g *WithT, m *expv1.MachinePool) {
-				g.Expect(m.Status.BootstrapReady).To(BeTrue())
+				g.Expect(m.Status.BootstrapReady).To(BeFalse())
 			},
 		},
 	}