From 676e42df9840a9e13ccdb762fe5dce9a360f554f Mon Sep 17 00:00:00 2001 From: nikolayzakharov Date: Wed, 2 Sep 2026 16:12:28 +0200 Subject: [PATCH 1/2] fix: report a missing instance as a final state When the server of an already provisioned machine was gone, the reconcile used the reason InstanceError and returned an error, so the controller retried for ever and wrote an error line for a state that no retry changes. The provider now reports that state with its own error value. The reconcile turns it into the reason InstanceNotFound on the conditions and into a warning event, and it gives no error back to the controller runtime, so the retry stops. It still creates no replacement server. test: cover the error return of the machine reconcile The regression test for the gone server asserted this before. It now asserts the opposite, so a rejected VM creation covers it instead. test: cover the deletion of a machine whose server is gone The spec drives a provisioned control plane machine into the terminal state of a missing instance, then deletes it while the cloud answers the server delete with not found. The load balancer target and the finalizer must go. The spec is the fourth caller of updateMachineControlPlaneLabel, so unparam reports the namespace parameter that always receives "default". The parameter has been refactored away, and the helper writes the namespace like the helper above it. test: cover a failed server delete during machine deletion The delete path removes the finalizer only after the cloud reports the server as deleted or as already gone. The other exit had no spec: a deletion that answers with any other error must keep the object. The new spec injects a transient error into the server delete, then asserts that the reconcile returns that error, that the server is still there, and that the finalizer still holds the object, so the next attempt can find the instance ID. test: drop the excessive comment of the failed delete spec --- controller/controller_test_helpers_test.go | 4 +- controller/stackitmachine_controller_test.go | 110 +++++++++++++++++-- controller/stackitmachine_infrastructure.go | 22 +++- 3 files changed, 125 insertions(+), 11 deletions(-) diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index c861246..d103137 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -130,9 +130,9 @@ func updateMachineBootstrapSecret(ctx context.Context, name, bootstrapSecretName Expect(k8sClient.Update(ctx, machine)).To(Succeed()) } -func updateMachineControlPlaneLabel(ctx context.Context, name, namespace string) { +func updateMachineControlPlaneLabel(ctx context.Context, name string) { machine := &clusterv1.Machine{} - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, machine)).To(Succeed()) + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: "default"}, machine)).To(Succeed()) if machine.Labels == nil { machine.Labels = map[string]string{} } diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 0dfb3e2..3d5babe 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -154,8 +154,9 @@ var _ = Describe("StackitMachine Controller", func() { Expect(fakeCloud.ServerCount()).To(Equal(0)) By("reconciling again") - _, err = reconciler.Reconcile(ctx, request) - Expect(err).To(HaveOccurred(), "reconcile must surface the missing server instead of papering over it") + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) Expect(fakeCloud.CreateServerCalls).To(Equal(1), "a replacement server was created for an already-provisioned machine") @@ -164,7 +165,8 @@ var _ = Describe("StackitMachine Controller", func() { By("reporting a consistent readiness state") degraded := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) - expectCondition(degraded.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") + expectCondition(degraded.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceNotFound") + expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") Expect(degraded.Status.Ready).To(BeFalse(), "legacy status.ready must follow the Ready condition, not contradict it") }) @@ -328,9 +330,25 @@ var _ = Describe("StackitMachine Controller", func() { expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") }) + It("returns an error when VM creation is rejected", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + fakeCloud.FailNextCreateServer = fmt.Errorf("create server rejected: %w", cloud.ErrInvalidInput) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).To(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(fakeCloud.ServerCount()).To(Equal(0)) + + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") + }) + It("registers control plane VMs as API server load balancer targets", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName, namespace) + updateMachineControlPlaneLabel(ctx, machineName) enableStackitClusterLoadBalancer(ctx, clusterName, namespace) reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) createBootstrapSecret(ctx, bootstrapName) @@ -350,7 +368,7 @@ var _ = Describe("StackitMachine Controller", func() { It("requeues when load balancer target registration returns a transient error", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName, namespace) + updateMachineControlPlaneLabel(ctx, machineName) createBootstrapSecret(ctx, bootstrapName) loadBalancerID := createAPIServerLoadBalancer(ctx, fakeCloud) updateStackitClusterLoadBalancer(ctx, clusterName, namespace, loadBalancerID) @@ -367,7 +385,7 @@ var _ = Describe("StackitMachine Controller", func() { It("deletes the VM and removes the finalizer", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName, namespace) + updateMachineControlPlaneLabel(ctx, machineName) enableStackitClusterLoadBalancer(ctx, clusterName, namespace) reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) createBootstrapSecret(ctx, bootstrapName) @@ -416,6 +434,63 @@ var _ = Describe("StackitMachine Controller", func() { "the server was created while the API server had no finalizer to clean it up") }) + It("removes the finalizer when the server is already gone at deletion time", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + updateMachineControlPlaneLabel(ctx, machineName) + enableStackitClusterLoadBalancer(ctx, clusterName, namespace) + reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) + createBootstrapSecret(ctx, bootstrapName) + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + stackitCluster := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) + loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID + Expect(loadBalancerID).NotTo(BeEmpty()) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + + provisioned := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, provisioned)).To(Succeed()) + instanceID := provisioned.Status.InstanceID + Expect(instanceID).NotTo(BeEmpty()) + + By("removing the server behind the provider's back") + Expect(fakeCloud.DeleteServer(ctx, instanceID)).To(Succeed()) + Expect(fakeCloud.ServerCount()).To(Equal(0)) + + By("reconciling into the terminal state") + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + degraded := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) + expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") + Expect(degraded.Status.InstanceID).To(Equal(instanceID)) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1), + "the deletion must still find the load balancer target to remove") + + By("deleting the object while the cloud reports the server as gone") + // The fake deletes an unknown ID without an error, so the not-found answer + // of the cloud must be injected. + fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrNotFound) + + Expect(k8sClient.Delete(ctx, degraded)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + // The reconcile clears FailNextDeleteServer when it calls DeleteServer, + // so a nil field proves the call happened. + Expect(fakeCloud.FailNextDeleteServer).ToNot(HaveOccurred(), + "the machine deletion did not ask the cloud to remove the server") + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) + // A lost status patch leaves a running, tagged server that no field on the // object names, so an empty status.instanceID is not proof that none exists. It("deletes a tagged server whose instance ID was lost from the status", func() { @@ -442,7 +517,6 @@ var _ = Describe("StackitMachine Controller", func() { _, err = reconciler.Reconcile(ctx, request) Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(0), "the tagged server leaked because deletion trusted the empty status") Eventually(func() bool { @@ -451,6 +525,28 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + It("keeps the finalizer when the server deletion fails", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrTransient) + + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, request) + + Expect(err).To(MatchError(cloud.ErrTransient)) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + stillThere := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, stillThere)).To(Succeed()) + Expect(stillThere.Finalizers).To(ContainElement(infrav1.MachineFinalizer)) + }) + It("maps owning Machine events to StackitMachine reconcile requests", func() { machine := &clusterv1.Machine{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index c549df3..114ff9d 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -12,6 +12,7 @@ package controller import ( "context" + "errors" "fmt" "time" @@ -32,6 +33,8 @@ import ( "github.com/stackitcloud/cluster-api-provider-stackit/util" ) +var errInstanceGone = errors.New("instance gone") + func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineScope *scope.MachineScope) (ctrl.Result, error) { log := logf.FromContext(ctx) stackitMachine := machineScope.StackitMachine @@ -83,6 +86,21 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS machineScope.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.MachineCredentialsReadyCondition) server, created, err := r.ensureServer(ctx, cloudClient, machineScope, bootstrapData) + if errors.Is(err, errInstanceGone) { + machineScope.SetNotReady( + "InstanceNotFound", + err.Error(), + infrav1.MachineInstanceReadyCondition, + infrav1.MachineReadyCondition, + ) + if r.Recorder != nil { + r.Recorder.Eventf( + stackitMachine, nil, corev1.EventTypeWarning, "InstanceNotFound", "Reconcile", + "Server %s no longer exists; the Machine must be replaced", stackitMachine.Status.InstanceID, + ) + } + return ctrl.Result{}, nil + } if err != nil { machineScope.SetNotReady( "InstanceError", @@ -285,8 +303,8 @@ func (r *StackitMachineReconciler) ensureServer( // identity, so surface the loss and let Cluster API replace the Machine. if stackitMachine.Status.Initialization.Provisioned { return nil, false, fmt.Errorf( - "%w: server %s for already-provisioned machine no longer exists; the Machine must be replaced", - cloud.ErrNotFound, stackitMachine.Status.InstanceID, + "%w: server %s; the Machine must be replaced", + errInstanceGone, stackitMachine.Status.InstanceID, ) } From 6b2bebd87c6491872489386eaccd13af8874a165 Mon Sep 17 00:00:00 2001 From: Jan Larwig Date: Fri, 18 Sep 2026 14:12:14 +0200 Subject: [PATCH 2/2] test: reduce setup duplication by introduce a new context for test with bootstrap data and tests without Signed-off-by: Jan Larwig --- controller/stackitmachine_controller_test.go | 756 +++++++++---------- 1 file changed, 363 insertions(+), 393 deletions(-) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 3d5babe..452af6a 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -34,17 +34,18 @@ var _ = Describe("StackitMachine Controller", func() { const namespace = "default" var ( - ctx context.Context - clusterName string - machineName string - stackitName string - credentials string - fakeCloud *cloudfake.Client - reconciler *StackitMachineReconciler - request reconcile.Request - stackitKey types.NamespacedName - stackitMach *infrav1.StackitMachine - bootstrapName string + ctx context.Context + clusterName string + machineName string + stackitName string + credentials string + fakeCloud *cloudfake.Client + reconciler *StackitMachineReconciler + request reconcile.Request + stackitKey types.NamespacedName + stackitMach *infrav1.StackitMachine + bootstrapName string + setupControlPlaneLoadBalancer func() ) BeforeEach(func() { @@ -72,6 +73,11 @@ var _ = Describe("StackitMachine Controller", func() { createOwnerMachine(ctx, machineName, clusterName, stackitName) stackitMach = newStackitMachine(stackitName, namespace, machineName) Expect(k8sClient.Create(ctx, stackitMach)).To(Succeed()) + setupControlPlaneLoadBalancer = func() { + updateMachineControlPlaneLabel(ctx, machineName) + enableStackitClusterLoadBalancer(ctx, clusterName, namespace) + reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) + } }) AfterEach(func() { @@ -108,443 +114,407 @@ var _ = Describe("StackitMachine Controller", func() { expectCondition(got.Status.Conditions, infrav1.MachineBootstrapReadyCondition, metav1.ConditionFalse, "BootstrapDataSecretNotFound") }) - It("creates a VM and sets provider status when bootstrap data is available", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - Expect(fakeCloud.ServerCount()).To(Equal(1)) - - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(got.Status.Ready).To(BeTrue()) - Expect(got.Status.InstanceID).NotTo(BeEmpty()) - Expect(got.Status.InstanceState).To(Equal("ACTIVE")) - Expect(got.Status.ProviderID).To(Equal("stackit://" + got.Status.InstanceID)) - Expect(got.Spec.ProviderID).NotTo(BeNil()) - Expect(*got.Spec.ProviderID).To(Equal(got.Status.ProviderID)) - Expect(got.Status.Addresses).To(HaveLen(1)) - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionTrue, "Available") - expectCondition(got.Status.Conditions, infrav1.MachineBootstrapReadyCondition, metav1.ConditionTrue, "Available") - expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionTrue, "Available") - }) - - It("does not silently recreate the server of an already-provisioned machine", func() { - // Recreating it would replay bootstrap data pinned to the previous - // identity: the replacement either never rejoins or rejoins while Machine - // and Node still point at the deleted server. - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) - Expect(fakeCloud.CreateServerCalls).To(Equal(1)) - - provisioned := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, provisioned)).To(Succeed()) - Expect(provisioned.Status.Initialization.Provisioned).To(BeTrue()) - instanceID := provisioned.Status.InstanceID - Expect(instanceID).NotTo(BeEmpty()) - - By("removing the server behind the provider's back") - Expect(fakeCloud.DeleteServer(ctx, instanceID)).To(Succeed()) - Expect(fakeCloud.ServerCount()).To(Equal(0)) - - By("reconciling again") - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - - Expect(fakeCloud.CreateServerCalls).To(Equal(1), - "a replacement server was created for an already-provisioned machine") - Expect(fakeCloud.ServerCount()).To(Equal(0)) - - By("reporting a consistent readiness state") - degraded := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) - expectCondition(degraded.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceNotFound") - expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") - Expect(degraded.Status.Ready).To(BeFalse(), - "legacy status.ready must follow the Ready condition, not contradict it") - }) - - It("attaches provider-managed node SSH access when bastion is enabled", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) - stackitCluster.Spec.Bastion = validBastionSpec() - Expect(k8sClient.Update(ctx, stackitCluster)).To(Succeed()) - stackitCluster.Status.Ready = true - stackitCluster.Status.Bastion.SecurityGroupID = "bastion-sg" - Expect(k8sClient.Status().Update(ctx, stackitCluster)).To(Succeed()) - - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(got.Status.Ready).To(BeTrue()) - Expect(fakeCloud.EnsureNodeSSHCalls).To(Equal(1)) - securityGroups, err := fakeCloud.ListSecurityGroupsByTags(ctx, map[string]string{ - util.LabelClusterName: clusterName, - util.LabelResourceRole: util.ResourceRoleNodeSSH, + Context("with bootstrap data", func() { + BeforeEach(func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) }) - Expect(err).NotTo(HaveOccurred()) - Expect(securityGroups).To(HaveLen(1)) - Expect(fakeCloud.SecurityGroupRemoteSources(securityGroups[0].ID)).To(ConsistOf("bastion-sg")) - Expect(fakeCloud.ServerHasSecurityGroup(got.Status.InstanceID, securityGroups[0].ID)).To(BeTrue()) - }) - - It("does not create a VM when availabilityZone is outside published failure domains", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - got.Spec.AvailabilityZone = "eu01-9" - Expect(k8sClient.Update(ctx, got)).To(Succeed()) - - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - Expect(fakeCloud.ServerCount()).To(Equal(0)) - - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(got.Status.Ready).To(BeFalse()) - expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InvalidFailureDomain") - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InvalidFailureDomain") - }) - - It("requeues on unauthorized credentials and recovers once they are corrected", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { - return nil, fmt.Errorf("authenticate: %w", cloud.ErrUnauthorized) - } - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(Equal(credentialsRetryRequeueAfter)) - Expect(fakeCloud.ServerCount()).To(Equal(0)) + It("creates a VM and sets provider status when bootstrap data is available", func() { - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(fakeCloud.ServerCount()).To(Equal(1)) - By("correcting the credentials") - reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { - return fakeCloud, nil - } - - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) - - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionTrue, "Available") - }) + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeTrue()) + Expect(got.Status.InstanceID).NotTo(BeEmpty()) + Expect(got.Status.InstanceState).To(Equal("ACTIVE")) + Expect(got.Status.ProviderID).To(Equal("stackit://" + got.Status.InstanceID)) + Expect(got.Spec.ProviderID).NotTo(BeNil()) + Expect(*got.Spec.ProviderID).To(Equal(got.Status.ProviderID)) + Expect(got.Status.Addresses).To(HaveLen(1)) + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionTrue, "Available") + expectCondition(got.Status.Conditions, infrav1.MachineBootstrapReadyCondition, metav1.ConditionTrue, "Available") + expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionTrue, "Available") + }) - It("does not call the cloud API when the owning Cluster is paused", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - cluster := &clusterv1.Cluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, cluster)).To(Succeed()) - cluster.Spec.Paused = new(true) - Expect(k8sClient.Update(ctx, cluster)).To(Succeed()) - - cloudClientFactoryCalls := 0 - reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { - cloudClientFactoryCalls++ - return fakeCloud, nil - } + It("does not silently recreate the server of an already-provisioned machine", func() { + // Recreating it would replay bootstrap data pinned to the previous + // identity: the replacement either never rejoins or rejoins while Machine + // and Node still point at the deleted server. + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + Expect(fakeCloud.CreateServerCalls).To(Equal(1)) + + provisioned := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, provisioned)).To(Succeed()) + Expect(provisioned.Status.Initialization.Provisioned).To(BeTrue()) + instanceID := provisioned.Status.InstanceID + Expect(instanceID).NotTo(BeEmpty()) + + By("removing the server behind the provider's back") + Expect(fakeCloud.DeleteServer(ctx, instanceID)).To(Succeed()) + Expect(fakeCloud.ServerCount()).To(Equal(0)) + + By("reconciling again") + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + + Expect(fakeCloud.CreateServerCalls).To(Equal(1), + "a replacement server was created for an already-provisioned machine") + Expect(fakeCloud.ServerCount()).To(Equal(0)) + + By("reporting a consistent readiness state") + degraded := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) + expectCondition(degraded.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceNotFound") + expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") + Expect(degraded.Status.Ready).To(BeFalse(), + "legacy status.ready must follow the Ready condition, not contradict it") + }) - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - Expect(cloudClientFactoryCalls).To(Equal(0)) - Expect(fakeCloud.ServerCount()).To(Equal(0)) + It("attaches provider-managed node SSH access when bastion is enabled", func() { + stackitCluster := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) + stackitCluster.Spec.Bastion = validBastionSpec() + Expect(k8sClient.Update(ctx, stackitCluster)).To(Succeed()) + stackitCluster.Status.Ready = true + stackitCluster.Status.Bastion.SecurityGroupID = "bastion-sg" + Expect(k8sClient.Status().Update(ctx, stackitCluster)).To(Succeed()) - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, clusterv1.PausedCondition, metav1.ConditionTrue, clusterv1.PausedReason) - }) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) - It("does not call the cloud API when the StackitMachine has the paused annotation", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - got.Annotations = map[string]string{clusterv1.PausedAnnotation: ""} - Expect(k8sClient.Update(ctx, got)).To(Succeed()) - - cloudClientFactoryCalls := 0 - reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { - cloudClientFactoryCalls++ - return fakeCloud, nil - } + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeTrue()) + Expect(fakeCloud.EnsureNodeSSHCalls).To(Equal(1)) + securityGroups, err := fakeCloud.ListSecurityGroupsByTags(ctx, map[string]string{ + util.LabelClusterName: clusterName, + util.LabelResourceRole: util.ResourceRoleNodeSSH, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(securityGroups).To(HaveLen(1)) + Expect(fakeCloud.SecurityGroupRemoteSources(securityGroups[0].ID)).To(ConsistOf("bastion-sg")) + Expect(fakeCloud.ServerHasSecurityGroup(got.Status.InstanceID, securityGroups[0].ID)).To(BeTrue()) + }) - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - Expect(cloudClientFactoryCalls).To(Equal(0)) - Expect(fakeCloud.ServerCount()).To(Equal(0)) + It("does not create a VM when availabilityZone is outside published failure domains", func() { + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Spec.AvailabilityZone = "eu01-9" + Expect(k8sClient.Update(ctx, got)).To(Succeed()) - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, clusterv1.PausedCondition, metav1.ConditionTrue, clusterv1.PausedReason) - }) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(fakeCloud.ServerCount()).To(Equal(0)) - It("requeues when server lookup returns a conflict", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - fakeCloud.FailNextFindServer = fmt.Errorf("multiple matching servers: %w", cloud.ErrConflict) + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(got.Status.Ready).To(BeFalse()) + expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InvalidFailureDomain") + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InvalidFailureDomain") + }) - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">", 0)) - Expect(fakeCloud.ServerCount()).To(Equal(0)) + It("requeues on unauthorized credentials and recovers once they are corrected", func() { + reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { + return nil, fmt.Errorf("authenticate: %w", cloud.ErrUnauthorized) + } - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") - }) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(credentialsRetryRequeueAfter)) + Expect(fakeCloud.ServerCount()).To(Equal(0)) - It("requeues when VM creation returns a transient error", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - fakeCloud.FailNextCreateServer = fmt.Errorf("create server timeout: %w", cloud.ErrTransient) + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "CredentialsInvalid") - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">", 0)) - Expect(fakeCloud.ServerCount()).To(Equal(0)) + By("correcting the credentials") + reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { + return fakeCloud, nil + } - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") - }) + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) - It("returns an error when VM creation is rejected", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - fakeCloud.FailNextCreateServer = fmt.Errorf("create server rejected: %w", cloud.ErrInvalidInput) + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineCredentialsReadyCondition, metav1.ConditionTrue, "Available") + }) - result, err := reconciler.Reconcile(ctx, request) - Expect(err).To(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - Expect(fakeCloud.ServerCount()).To(Equal(0)) + It("does not call the cloud API when the owning Cluster is paused", func() { + cluster := &clusterv1.Cluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, cluster)).To(Succeed()) + cluster.Spec.Paused = new(true) + Expect(k8sClient.Update(ctx, cluster)).To(Succeed()) - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") - }) + cloudClientFactoryCalls := 0 + reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { + cloudClientFactoryCalls++ + return fakeCloud, nil + } - It("registers control plane VMs as API server load balancer targets", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName) - enableStackitClusterLoadBalancer(ctx, clusterName, namespace) - reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) - createBootstrapSecret(ctx, bootstrapName) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(cloudClientFactoryCalls).To(Equal(0)) + Expect(fakeCloud.ServerCount()).To(Equal(0)) - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result).To(Equal(reconcile.Result{})) - Expect(fakeCloud.ServerCount()).To(Equal(1)) - Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, clusterv1.PausedCondition, metav1.ConditionTrue, clusterv1.PausedReason) + }) - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) - loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID - Expect(loadBalancerID).NotTo(BeEmpty()) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) - }) + It("does not call the cloud API when the StackitMachine has the paused annotation", func() { + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Annotations = map[string]string{clusterv1.PausedAnnotation: ""} + Expect(k8sClient.Update(ctx, got)).To(Succeed()) - It("requeues when load balancer target registration returns a transient error", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName) - createBootstrapSecret(ctx, bootstrapName) - loadBalancerID := createAPIServerLoadBalancer(ctx, fakeCloud) - updateStackitClusterLoadBalancer(ctx, clusterName, namespace, loadBalancerID) - fakeCloud.FailNextEnsureTarget = fmt.Errorf("update target pool timeout: %w", cloud.ErrTransient) + cloudClientFactoryCalls := 0 + reconciler.CloudClientFactory = func(context.Context, cloud.Credentials) (cloud.Client, error) { + cloudClientFactoryCalls++ + return fakeCloud, nil + } - result, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(cloudClientFactoryCalls).To(Equal(0)) + Expect(fakeCloud.ServerCount()).To(Equal(0)) - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "LoadBalancerTargetError") - }) + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, clusterv1.PausedCondition, metav1.ConditionTrue, clusterv1.PausedReason) + }) - It("deletes the VM and removes the finalizer", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName) - enableStackitClusterLoadBalancer(ctx, clusterName, namespace) - reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) - createBootstrapSecret(ctx, bootstrapName) - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) + It("requeues when server lookup returns a conflict", func() { + fakeCloud.FailNextFindServer = fmt.Errorf("multiple matching servers: %w", cloud.ErrConflict) - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) - loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID - Expect(loadBalancerID).NotTo(BeEmpty()) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + Expect(fakeCloud.ServerCount()).To(Equal(0)) - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") + }) - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(0)) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) - Eventually(func() bool { - err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) - return apierrors.IsNotFound(err) - }).Should(BeTrue()) - }) + It("requeues when VM creation returns a transient error", func() { + fakeCloud.FailNextCreateServer = fmt.Errorf("create server timeout: %w", cloud.ErrTransient) - // The hook observes API server state from inside the cloud call, so it proves - // the ordering rather than only the end result. - It("persists the finalizer before creating the server", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + Expect(fakeCloud.ServerCount()).To(Equal(0)) - var finalizersAtCreate []string - fakeCloud.BeforeCreateServer = func() { got := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - finalizersAtCreate = got.Finalizers - } + expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") + }) - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) + It("returns an error when VM creation is rejected", func() { + fakeCloud.FailNextCreateServer = fmt.Errorf("create server rejected: %w", cloud.ErrInvalidInput) - Expect(finalizersAtCreate).To(ContainElement(infrav1.MachineFinalizer), - "the server was created while the API server had no finalizer to clean it up") - }) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).To(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(fakeCloud.ServerCount()).To(Equal(0)) - It("removes the finalizer when the server is already gone at deletion time", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName) - enableStackitClusterLoadBalancer(ctx, clusterName, namespace) - reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) - createBootstrapSecret(ctx, bootstrapName) + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") + }) - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) + It("registers control plane VMs as API server load balancer targets", func() { + setupControlPlaneLoadBalancer() - stackitCluster := &infrav1.StackitCluster{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) - loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID - Expect(loadBalancerID).NotTo(BeEmpty()) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) - provisioned := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, provisioned)).To(Succeed()) - instanceID := provisioned.Status.InstanceID - Expect(instanceID).NotTo(BeEmpty()) + stackitCluster := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) + loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID + Expect(loadBalancerID).NotTo(BeEmpty()) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + }) - By("removing the server behind the provider's back") - Expect(fakeCloud.DeleteServer(ctx, instanceID)).To(Succeed()) - Expect(fakeCloud.ServerCount()).To(Equal(0)) + It("requeues when load balancer target registration returns a transient error", func() { + updateMachineControlPlaneLabel(ctx, machineName) + loadBalancerID := createAPIServerLoadBalancer(ctx, fakeCloud) + updateStackitClusterLoadBalancer(ctx, clusterName, namespace, loadBalancerID) + fakeCloud.FailNextEnsureTarget = fmt.Errorf("update target pool timeout: %w", cloud.ErrTransient) - By("reconciling into the terminal state") - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) - degraded := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) - expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") - Expect(degraded.Status.InstanceID).To(Equal(instanceID)) - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1), - "the deletion must still find the load balancer target to remove") + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "LoadBalancerTargetError") + }) - By("deleting the object while the cloud reports the server as gone") - // The fake deletes an unknown ID without an error, so the not-found answer - // of the cloud must be injected. - fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrNotFound) + It("deletes the VM and removes the finalizer", func() { + setupControlPlaneLoadBalancer() + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) - Expect(k8sClient.Delete(ctx, degraded)).To(Succeed()) + stackitCluster := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) + loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID + Expect(loadBalancerID).NotTo(BeEmpty()) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - // The reconcile clears FailNextDeleteServer when it calls DeleteServer, - // so a nil field proves the call happened. - Expect(fakeCloud.FailNextDeleteServer).ToNot(HaveOccurred(), - "the machine deletion did not ask the cloud to remove the server") - Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) - Eventually(func() bool { - err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) - return apierrors.IsNotFound(err) - }).Should(BeTrue()) - }) + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(0)) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) - // A lost status patch leaves a running, tagged server that no field on the - // object names, so an empty status.instanceID is not proof that none exists. - It("deletes a tagged server whose instance ID was lost from the status", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) + // The hook observes API server state from inside the cloud call, so it proves + // the ordering rather than only the end result. + It("persists the finalizer before creating the server", func() { + var finalizersAtCreate []string + fakeCloud.BeforeCreateServer = func() { + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + finalizersAtCreate = got.Finalizers + } + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + Expect(finalizersAtCreate).To(ContainElement(infrav1.MachineFinalizer), + "the server was created while the API server had no finalizer to clean it up") + }) - By("losing the persisted instance ID, as if the status patch never landed") - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - got.Status.InstanceID = "" - got.Status.ProviderID = "" - Expect(k8sClient.Status().Update(ctx, got)).To(Succeed()) + It("removes the finalizer when the server is already gone at deletion time", func() { + setupControlPlaneLoadBalancer() + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + stackitCluster := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) + loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID + Expect(loadBalancerID).NotTo(BeEmpty()) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + + provisioned := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, provisioned)).To(Succeed()) + instanceID := provisioned.Status.InstanceID + Expect(instanceID).NotTo(BeEmpty()) + + By("removing the server behind the provider's back") + Expect(fakeCloud.DeleteServer(ctx, instanceID)).To(Succeed()) + Expect(fakeCloud.ServerCount()).To(Equal(0)) + + By("reconciling into the terminal state") + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + degraded := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) + expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") + Expect(degraded.Status.InstanceID).To(Equal(instanceID)) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1), + "the deletion must still find the load balancer target to remove") + + By("deleting the object while the cloud reports the server as gone") + // The fake deletes an unknown ID without an error, so the not-found answer + // of the cloud must be injected. + fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrNotFound) + + Expect(k8sClient.Delete(ctx, degraded)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + // The reconcile clears FailNextDeleteServer when it calls DeleteServer, + // so a nil field proves the call happened. + Expect(fakeCloud.FailNextDeleteServer).ToNot(HaveOccurred(), + "the machine deletion did not ask the cloud to remove the server") + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - got.Spec.ProviderID = nil - Expect(k8sClient.Update(ctx, got)).To(Succeed()) + // A lost status patch leaves a running, tagged server that no field on the + // object names, so an empty status.instanceID is not proof that none exists. + It("deletes a tagged server whose instance ID was lost from the status", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) - By("deleting the StackitMachine") - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + By("losing the persisted instance ID, as if the status patch never landed") + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Status.InstanceID = "" + got.Status.ProviderID = "" + Expect(k8sClient.Status().Update(ctx, got)).To(Succeed()) - _, err = reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(0), - "the tagged server leaked because deletion trusted the empty status") - Eventually(func() bool { - err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) - return apierrors.IsNotFound(err) - }).Should(BeTrue()) - }) + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Spec.ProviderID = nil + Expect(k8sClient.Update(ctx, got)).To(Succeed()) - It("keeps the finalizer when the server deletion fails", func() { - updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - createBootstrapSecret(ctx, bootstrapName) + By("deleting the StackitMachine") + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(0), + "the tagged server leaked because deletion trusted the empty status") + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) - _, err := reconciler.Reconcile(ctx, request) - Expect(err).NotTo(HaveOccurred()) - Expect(fakeCloud.ServerCount()).To(Equal(1)) + It("keeps the finalizer when the server deletion fails", func() { + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) - got := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) - fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrTransient) + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrTransient) - Expect(k8sClient.Delete(ctx, got)).To(Succeed()) - _, err = reconciler.Reconcile(ctx, request) + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, request) - Expect(err).To(MatchError(cloud.ErrTransient)) - Expect(fakeCloud.ServerCount()).To(Equal(1)) - stillThere := &infrav1.StackitMachine{} - Expect(k8sClient.Get(ctx, stackitKey, stillThere)).To(Succeed()) - Expect(stillThere.Finalizers).To(ContainElement(infrav1.MachineFinalizer)) + Expect(err).To(MatchError(cloud.ErrTransient)) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + stillThere := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, stillThere)).To(Succeed()) + Expect(stillThere.Finalizers).To(ContainElement(infrav1.MachineFinalizer)) + }) }) It("maps owning Machine events to StackitMachine reconcile requests", func() {