diff --git a/exp/internal/controllers/machinepool_controller_phases.go b/exp/internal/controllers/machinepool_controller_phases.go index cb60b4429569..064ec4cb574a 100644 --- a/exp/internal/controllers/machinepool_controller_phases.go +++ b/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. diff --git a/exp/internal/controllers/machinepool_controller_phases_test.go b/exp/internal/controllers/machinepool_controller_phases_test.go index dee57c28ed9e..45cf96d03ed4 100644 --- a/exp/internal/controllers/machinepool_controller_phases_test.go +++ b/exp/internal/controllers/machinepool_controller_phases_test.go @@ -472,6 +472,173 @@ func TestReconcileMachinePoolPhases(t *testing.T) { r.reconcilePhase(machinepool) g.Expect(machinepool.Status.GetTypedPhase()).To(Equal(expv1.MachinePoolPhaseDeleting)) }) + + t.Run("Should keep `Running` when MachinePool bootstrap config is changed to another ready one", func(t *testing.T) { + g := NewWithT(t) + + defaultKubeconfigSecret = kubeconfig.GenerateSecret(defaultCluster, kubeconfig.FromEnvTestConfig(env.Config, defaultCluster)) + machinePool := defaultMachinePool.DeepCopy() + bootstrapConfig := defaultBootstrap.DeepCopy() + infraConfig := defaultInfra.DeepCopy() + + // Set bootstrap ready. + err := unstructured.SetNestedField(bootstrapConfig.Object, true, "status", "ready") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(bootstrapConfig.Object, "secret-data", "status", "dataSecretName") + g.Expect(err).ToNot(HaveOccurred()) + + // Set infra ready. + err = unstructured.SetNestedStringSlice(infraConfig.Object, []string{"test://id-1"}, "spec", "providerIDList") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(infraConfig.Object, true, "status", "ready") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(infraConfig.Object, []interface{}{ + map[string]interface{}{ + "type": "InternalIP", + "address": "10.0.0.1", + }, + map[string]interface{}{ + "type": "InternalIP", + "address": "10.0.0.2", + }, + }, "addresses") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(infraConfig.Object, int64(1), "status", "replicas") + g.Expect(err).ToNot(HaveOccurred()) + + // Set NodeRef. + machinePool.Status.NodeRefs = []corev1.ObjectReference{{Kind: "Node", Name: "machinepool-test-node"}} + + // Set replicas to fully reconciled + machinePool.Spec.ProviderIDList = []string{"test://id-1"} + machinePool.Status.ReadyReplicas = 1 + machinePool.Status.Replicas = 1 + + r := &MachinePoolReconciler{ + Client: fake.NewClientBuilder().WithObjects(defaultCluster, defaultKubeconfigSecret, machinePool, bootstrapConfig, infraConfig, builder.TestBootstrapConfigCRD, builder.TestInfrastructureMachineTemplateCRD).Build(), + } + + res, err := r.reconcile(ctx, defaultCluster, machinePool) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(res.Requeue).To(BeFalse()) + + r.reconcilePhase(machinePool) + g.Expect(machinePool.Status.GetTypedPhase()).To(Equal(expv1.MachinePoolPhaseRunning)) + g.Expect(*machinePool.Spec.Template.Spec.Bootstrap.DataSecretName).To(Equal("secret-data")) + + // Change bootstrap reference. + newBootstrapConfig := defaultBootstrap.DeepCopy() + newBootstrapConfig.SetName("bootstrap-config2") + err = unstructured.SetNestedField(newBootstrapConfig.Object, true, "status", "ready") + g.Expect(err).ToNot(HaveOccurred()) + err = unstructured.SetNestedField(newBootstrapConfig.Object, "secret-data-new", "status", "dataSecretName") + g.Expect(err).ToNot(HaveOccurred()) + err = r.Client.Create(ctx, newBootstrapConfig) + g.Expect(err).ToNot(HaveOccurred()) + machinePool.Spec.Template.Spec.Bootstrap.ConfigRef.Name = newBootstrapConfig.GetName() + + // Reconcile again. The new bootstrap config should be used. + res, err = r.reconcile(ctx, defaultCluster, machinePool) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(res.Requeue).To(BeFalse()) + + r.reconcilePhase(machinePool) + g.Expect(*machinePool.Spec.Template.Spec.Bootstrap.DataSecretName).To(Equal("secret-data-new")) + g.Expect(machinePool.Status.BootstrapReady).To(BeTrue()) + g.Expect(machinePool.Status.GetTypedPhase()).To(Equal(expv1.MachinePoolPhaseRunning)) + }) + + t.Run("Should keep `Running` when MachinePool bootstrap config is changed to a non-ready one", func(t *testing.T) { + g := NewWithT(t) + + defaultKubeconfigSecret = kubeconfig.GenerateSecret(defaultCluster, kubeconfig.FromEnvTestConfig(env.Config, defaultCluster)) + machinePool := defaultMachinePool.DeepCopy() + bootstrapConfig := defaultBootstrap.DeepCopy() + infraConfig := defaultInfra.DeepCopy() + + // Set bootstrap ready + err := unstructured.SetNestedField(bootstrapConfig.Object, true, "status", "ready") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(bootstrapConfig.Object, "secret-data", "status", "dataSecretName") + g.Expect(err).ToNot(HaveOccurred()) + + // Set infra ready + err = unstructured.SetNestedStringSlice(infraConfig.Object, []string{"test://id-1"}, "spec", "providerIDList") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(infraConfig.Object, true, "status", "ready") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(infraConfig.Object, []interface{}{ + map[string]interface{}{ + "type": "InternalIP", + "address": "10.0.0.1", + }, + map[string]interface{}{ + "type": "InternalIP", + "address": "10.0.0.2", + }, + }, "addresses") + g.Expect(err).ToNot(HaveOccurred()) + + err = unstructured.SetNestedField(infraConfig.Object, int64(1), "status", "replicas") + g.Expect(err).ToNot(HaveOccurred()) + + // Set NodeRef + machinePool.Status.NodeRefs = []corev1.ObjectReference{{Kind: "Node", Name: "machinepool-test-node"}} + + // Set replicas to fully reconciled + machinePool.Spec.ProviderIDList = []string{"test://id-1"} + machinePool.Status.ReadyReplicas = 1 + machinePool.Status.Replicas = 1 + + r := &MachinePoolReconciler{ + Client: fake.NewClientBuilder().WithObjects(defaultCluster, defaultKubeconfigSecret, machinePool, bootstrapConfig, infraConfig, builder.TestBootstrapConfigCRD, builder.TestInfrastructureMachineTemplateCRD).Build(), + } + + res, err := r.reconcile(ctx, defaultCluster, machinePool) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(res.Requeue).To(BeFalse()) + + r.reconcilePhase(machinePool) + g.Expect(machinePool.Status.GetTypedPhase()).To(Equal(expv1.MachinePoolPhaseRunning)) + g.Expect(*machinePool.Spec.Template.Spec.Bootstrap.DataSecretName).To(Equal("secret-data")) + + // Change bootstrap reference + newBootstrapConfig := defaultBootstrap.DeepCopy() + newBootstrapConfig.SetName("bootstrap-config2") + err = unstructured.SetNestedField(newBootstrapConfig.Object, false, "status", "ready") + g.Expect(err).ToNot(HaveOccurred()) + // Fill the `dataSecretName` so we can check if the machine pool uses the non-ready secret immediately or, + // as it should, not yet + err = unstructured.SetNestedField(newBootstrapConfig.Object, "secret-data-new", "status", "dataSecretName") + g.Expect(err).ToNot(HaveOccurred()) + err = r.Client.Create(ctx, newBootstrapConfig) + g.Expect(err).ToNot(HaveOccurred()) + machinePool.Spec.Template.Spec.Bootstrap.ConfigRef.Name = newBootstrapConfig.GetName() + + // Reconcile again. The new bootstrap config should be used + res, err = r.reconcile(ctx, defaultCluster, machinePool) + g.Expect(err).ToNot(HaveOccurred()) + + // Controller should wait until bootstrap provider reports ready bootstrap config + g.Expect(res.Requeue).To(BeFalse()) + + r.reconcilePhase(machinePool) + + // The old secret should still be used, as the new bootstrap config is not marked ready + g.Expect(*machinePool.Spec.Template.Spec.Bootstrap.DataSecretName).To(Equal("secret-data")) + g.Expect(machinePool.Status.BootstrapReady).To(BeFalse()) + + // There is no phase defined for "changing to new bootstrap config", so it should still be `Running` the + // old configuration + g.Expect(machinePool.Status.GetTypedPhase()).To(Equal(expv1.MachinePoolPhaseRunning)) + }) } func TestReconcileMachinePoolBootstrap(t *testing.T) { @@ -605,7 +772,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 +810,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 +889,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()) }, }, }