fix: bastion and machine reconcile defects

cloud/sdk_client.go

@@ -260,11 +260,15 @@ func (c *SDKClient) EnsureBastion(ctx context.Context, input BastionInput) (*Bas
 	if err != nil {
 		return nil, err
 	}
-	// The security group is already part of the CreateServer payload above.
-	// Attaching it again here used to fail with 404 while the server had no
-	// port yet, and with 400 "Duplicate items in the list" once it had one —
-	// and because the error aborted EnsureBastion, the public IP below was
-	// only assigned a reconcile later.
+	// The group is already in the CreateServer payload, but CreateServer
+	// short-circuits on an existing server found by tags — this is then the only
+	// path that re-attaches a group that was detached out of band (for example
+	// by an interrupted DeleteBastion). Kept deliberately, and idempotent: the
+	// duplicate/no-port-yet responses are tolerated so they can no longer abort
+	// the reconcile before the public IP below is assigned.
+	if err := c.addSecurityGroupToServer(ctx, server.ID, securityGroup.ID); err != nil {
+		return nil, err
+	}
 
 	publicIP, err := c.ensurePublicIP(ctx, input.Tags)
 	if err != nil {
@@ -705,6 +709,13 @@ func (c *SDKClient) ensureBastionSecurityGroupRules(ctx context.Context, securit
 		if cidr == "" {
 			return fmt.Errorf("%w: empty bastion allowed CIDR", ErrInvalidInput)
 		}
+		if _, seen := desired[cidr]; seen {
+			// A CIDR listed twice would otherwise be created twice:
+			// existingRules is a snapshot from before this loop, so the second
+			// pass does not see the rule the first pass just created, and the
+			// duplicate create fails the whole bastion reconcile.
+			continue
+		}
 		desired[cidr] = struct{}{}
 		if hasSSHRule(existingRules, cidr) {
 			continue
@@ -827,12 +838,18 @@ func (c *SDKClient) findSecurityGroupByTags(ctx context.Context, tags map[string
 	return matched[0], nil
 }
 
+// addSecurityGroupToServer attaches the group idempotently. Three outcomes are
+// expected and must not fail the caller:
+//   - conflict: already attached
+//   - invalid input: the API rejects the duplicate ("Duplicate items in the list")
+//   - not found: the server has no network port yet, so there is nothing to
+//     attach to — the next reconcile retries once it does
 func (c *SDKClient) addSecurityGroupToServer(ctx context.Context, serverID, securityGroupID string) error {
 	if err := c.iaasClient.DefaultAPI.
 		AddSecurityGroupToServer(ctx, c.projectID, c.region, serverID, securityGroupID).
 		Execute(); err != nil {
 		err := classifySDKError("add security group to server", err)
-		if !IsConflict(err) {
+		if !IsConflict(err) && !IsInvalidInput(err) && !IsNotFound(err) {
 			return err
 		}
 	}

cloud/sdk_client_test.go

@@ -433,13 +433,14 @@ func lookup(m map[string]any, key string) any {
 	return nil
 }
 
-// TestSDKClientEnsureBastionAttachesSecurityGroupOnlyOnce guards against the
-// redundant security-group attach that used to follow CreateServer. The group
-// is already part of the create payload; attaching it again failed with 404
-// while the server had no port yet and with 400 "Duplicate items in the list"
-// once it had one — and the error aborted EnsureBastion before the public IP
-// was assigned.
-func TestSDKClientEnsureBastionAttachesSecurityGroupOnlyOnce(t *testing.T) {
+// TestSDKClientEnsureBastionToleratesDuplicateSecurityGroupAttach guards the
+// idempotency of the security-group attach. The group is already in the create
+// payload, so the attach usually hits a duplicate ("400 Duplicate items in the
+// list") or a server without a port yet ("404 ... as device id on any ports").
+// Neither may abort EnsureBastion — that used to leave the public IP unassigned
+// for a full reconcile cycle. The attach itself is kept because CreateServer
+// short-circuits on an existing server, making this the only re-attach path.
+func TestSDKClientEnsureBastionToleratesDuplicateSecurityGroupAttach(t *testing.T) {
 	var (
 		createPayload    map[string]any
 		attachCallCount  int
@@ -467,10 +468,13 @@ func TestSDKClientEnsureBastionAttachesSecurityGroupOnlyOnce(t *testing.T) {
 				"id": testSDKServerID, "name": "bastion", "status": "ACTIVE", "machineType": "c2i.1",
 			})
 		case strings.Contains(path, "/security-groups/") && strings.Contains(path, "/servers/"):
-			// PUT /servers/{id}/security-groups/{id} or the inverse ordering:
-			// any call here is the redundant attach this test guards against.
+			// Answer the way the real API does for an already-attached group.
 			attachCallCount++
-			w.WriteHeader(http.StatusNoContent)
+			w.WriteHeader(http.StatusBadRequest)
+			writeJSON(t, w, map[string]any{
+				"code": 400,
+				"msg":  "request invalid: Invalid input for security_groups. Reason: Duplicate items in the list.",
+			})
 		case r.Method == http.MethodGet && strings.HasSuffix(path, "/public-ips"):
 			writeJSON(t, w, map[string]any{"items": []any{}})
 		case r.Method == http.MethodPost && strings.HasSuffix(path, "/public-ips"):
@@ -503,8 +507,8 @@ func TestSDKClientEnsureBastionAttachesSecurityGroupOnlyOnce(t *testing.T) {
 		t.Fatalf("EnsureBastion() error = %v", err)
 	}
 
-	if attachCallCount != 0 {
-		t.Fatalf("security group attached %d extra time(s) after CreateServer, want 0", attachCallCount)
+	if attachCallCount != 1 {
+		t.Fatalf("attach attempted %d time(s), want exactly 1 (the idempotent re-attach)", attachCallCount)
 	}
 	groups, _ := createPayload["securityGroups"].([]any)
 	if len(groups) != 1 || groups[0] != testSDKSecurityGroup {
@@ -554,6 +558,8 @@ func TestSDKClientEnsureBastionRevokesRemovedCIDR(t *testing.T) {
 				createdRuleCIDRs = append(createdRuleCIDRs, cidr)
 			}
 			writeJSON(t, w, map[string]any{"id": "77777777-7777-4777-8777-777777777777", "direction": "ingress"})
+		case strings.Contains(path, "/security-groups/") && strings.Contains(path, "/servers/"):
+			w.WriteHeader(http.StatusNoContent)
 		case r.Method == http.MethodDelete && strings.Contains(path, "/rules/"):
 			parts := strings.Split(strings.TrimSuffix(path, "/"), "/")
 			deletedRuleIDs = append(deletedRuleIDs, parts[len(parts)-1])
@@ -603,3 +609,74 @@ func TestSDKClientEnsureBastionRevokesRemovedCIDR(t *testing.T) {
 		t.Fatalf("deleted rules %v, want [%s] — the revoked CIDR keeps its SSH access", deletedRuleIDs, staleRuleID)
 	}
 }
+
+// TestSDKClientEnsureBastionDeduplicatesRepeatedCIDRs guards against a CIDR
+// listed twice producing two identical rules: existingRules is a snapshot taken
+// before the loop, so the second pass would not see the rule the first pass
+// created and the duplicate create fails the whole reconcile.
+func TestSDKClientEnsureBastionDeduplicatesRepeatedCIDRs(t *testing.T) {
+	var createdRuleCIDRs []string
+	server := newSDKTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		path := r.URL.Path
+		switch {
+		case r.Method == http.MethodGet && strings.HasSuffix(path, "/security-groups"):
+			writeJSON(t, w, map[string]any{"items": []any{
+				map[string]any{
+					"id": testSDKSecurityGroup, "name": "bastion-ssh",
+					"labels": map[string]any{"cluster": "test"},
+				},
+			}})
+		case r.Method == http.MethodGet && strings.HasSuffix(path, "/rules"):
+			writeJSON(t, w, map[string]any{"items": []any{}})
+		case r.Method == http.MethodPost && strings.HasSuffix(path, "/rules"):
+			payload := readJSON(t, r)
+			if cidr, ok := payload["ipRange"].(string); ok {
+				createdRuleCIDRs = append(createdRuleCIDRs, cidr)
+			}
+			writeJSON(t, w, map[string]any{"id": "77777777-7777-4777-8777-777777777777", "direction": "ingress"})
+		case r.Method == http.MethodGet && strings.HasSuffix(path, "/servers"):
+			writeJSON(t, w, map[string]any{"items": []any{
+				map[string]any{
+					"id": testSDKServerID, "name": "bastion", "status": "ACTIVE", "machineType": "c2i.1",
+					"labels": map[string]any{"cluster": "test"},
+				},
+			}})
+		case strings.Contains(path, "/security-groups/") && strings.Contains(path, "/servers/"):
+			w.WriteHeader(http.StatusNoContent)
+		case r.Method == http.MethodGet && strings.HasSuffix(path, "/public-ips"):
+			writeJSON(t, w, map[string]any{"items": []any{
+				map[string]any{
+					"id": "66666666-6666-4666-8666-666666666666", "ip": "203.0.113.10", "networkInterface": "nic-1",
+					"labels": map[string]any{"cluster": "test"},
+				},
+			}})
+		case r.Method == http.MethodGet && strings.Contains(path, "/servers/"):
+			writeJSON(t, w, map[string]any{
+				"id": testSDKServerID, "name": "bastion", "status": "ACTIVE", "machineType": "c2i.1",
+			})
+		default:
+			t.Fatalf("unexpected request: %s %s", r.Method, r.URL.String())
+		}
+	}))
+
+	client := newTestSDKClient(t, server.URL)
+	if _, err := client.EnsureBastion(context.Background(), BastionInput{
+		Name:        "bastion",
+		ProjectID:   testSDKProjectID,
+		Region:      testSDKRegion,
+		NetworkID:   testSDKNetworkID,
+		ImageID:     testSDKImageID,
+		MachineType: "c2i.1",
+		SSHKeyName:  "default",
+		// The same CIDR twice, plus a distinct one.
+		AllowedCIDRs: []string{"203.0.113.0/24", "203.0.113.0/24", "198.51.100.0/24"},
+		Tags:         map[string]string{"cluster": "test"},
+	}); err != nil {
+		t.Fatalf("EnsureBastion() error = %v", err)
+	}
+
+	if len(createdRuleCIDRs) != 2 {
+		t.Fatalf("created %d rules (%v), want 2 — the repeated CIDR was created twice",
+			len(createdRuleCIDRs), createdRuleCIDRs)
+	}
+}

controller/controller_test_helpers_test.go

@@ -73,6 +73,9 @@ func createCloudInitSecret(ctx context.Context, name, namespace, key, value stri
 	Expect(k8sClient.Create(ctx, secret)).To(Succeed())
 }
 
+// Every caller passes the same namespace. This is fine for testing.
+//
+//nolint:unparam
 func createOwnerCluster(ctx context.Context, name, namespace string) {
 	cluster := &clusterv1.Cluster{
 		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},

controller/stackitcluster_bastion.go

@@ -19,6 +19,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -44,7 +45,17 @@ func (r *StackitClusterReconciler) reconcileBastion(
 	}
 
 	if !sc.Spec.Bastion.Enabled {
-		if hasBastionStatus(sc.Status.Bastion) {
+		// Status alone is not proof that no bastion exists: EnsureBastion can
+		// succeed and the status patch can be lost, after which disabling the
+		// bastion would silently leave it running with port 22 open — while the
+		// condition below claims it is disabled.
+		//
+		// The BastionReady condition lives in the same status subresource, so it
+		// is missing in exactly that case. Using it as the trigger keeps the
+		// tag-based cleanup to once per cluster instead of once per reconcile,
+		// which matters because this path runs for every cluster without a
+		// bastion.
+		if hasBastionStatus(sc.Status.Bastion) || meta.FindStatusCondition(sc.Status.Conditions, infrav1.ClusterBastionReadyCondition) == nil {
 			if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil {
 				return ctrl.Result{}, false, err
 			}

controller/stackitcluster_controller_test.go

@@ -202,6 +202,118 @@ var _ = Describe("StackitCluster Controller", func() {
 		}).Should(BeTrue())
 	})
 
+	It("cleans up bastion resources during deletion even when bastion status was never persisted", func() {
+		// Regression test for debug/deletion-bug.md: the cloud-cleanup block used
+		// to be gated on persisted status for the bastion, while the load
+		// balancer was gated on its spec flag. A bastion created without its
+		// status patch landing (process restart, conflict) therefore skipped
+		// cleanup entirely and leaked server, public IP and security group.
+		createOwnerCluster(ctx, clusterName+"-nolb", namespace)
+		defer deleteIfExists(ctx, &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nolb", Namespace: namespace},
+		})
+		lbDisabled := newStackitCluster(clusterName+"-nolb", namespace, false)
+		lbDisabled.Spec.CredentialsSecretRef.Name = credentials
+		lbDisabled.Spec.Bastion = validBastionSpec()
+		Expect(k8sClient.Create(ctx, lbDisabled)).To(Succeed())
+		defer deleteIfExists(ctx, lbDisabled)
+
+		key := types.NamespacedName{Namespace: namespace, Name: lbDisabled.Name}
+		req := reconcile.Request{NamespacedName: key}
+		_, err := reconciler.Reconcile(ctx, req)
+		Expect(err).NotTo(HaveOccurred())
+
+		got := &infrav1.StackitCluster{}
+		Expect(k8sClient.Get(ctx, key, got)).To(Succeed())
+		Expect(got.Status.Bastion.ServerID).NotTo(BeEmpty())
+		Expect(fakeCloud.ServerCount()).To(Equal(1))
+
+		By("losing the persisted bastion status, as if the patch never landed")
+		got.Status.Bastion = infrav1.StackitBastionStatus{}
+		Expect(k8sClient.Status().Update(ctx, got)).To(Succeed())
+		Expect(k8sClient.Get(ctx, key, got)).To(Succeed())
+		Expect(hasBastionStatus(got.Status.Bastion)).To(BeFalse())
+		Expect(got.Status.APIServerLoadBalancerID).To(BeEmpty())
+
+		By("deleting the cluster")
+		Expect(k8sClient.Delete(ctx, got)).To(Succeed())
+		_, err = reconciler.Reconcile(ctx, req)
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(fakeCloud.ServerCount()).To(Equal(0),
+			"bastion server leaked because cleanup was gated on status alone")
+		Expect(fakeCloud.PublicIPCount()).To(Equal(0))
+		Expect(fakeCloud.SecurityGroupCount()).To(Equal(0))
+	})
+
+	It("finalizes deletion when the credentials Secret is already gone", func() {
+		// A missing credentials Secret cannot be recovered from, and it commonly
+		// disappears first during namespace teardown. Broadening the delete gate
+		// to spec.Bastion.Enabled made a working cloud client mandatory for every
+		// bastion cluster, which would strand such a cluster in Terminating.
+		createOwnerCluster(ctx, clusterName+"-nocreds", namespace)
+		defer deleteIfExists(ctx, &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nocreds", Namespace: namespace},
+		})
+		orphaned := newStackitCluster(clusterName+"-nocreds", namespace, false)
+		orphaned.Spec.CredentialsSecretRef.Name = credentials
+		orphaned.Spec.Bastion = validBastionSpec()
+		Expect(k8sClient.Create(ctx, orphaned)).To(Succeed())
+		defer deleteIfExists(ctx, orphaned)
+
+		key := types.NamespacedName{Namespace: namespace, Name: orphaned.Name}
+		req := reconcile.Request{NamespacedName: key}
+		_, err := reconciler.Reconcile(ctx, req)
+		Expect(err).NotTo(HaveOccurred())
+
+		By("removing the credentials Secret, as namespace teardown would")
+		Expect(k8sClient.Delete(ctx, &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{Name: credentials, Namespace: namespace},
+		})).To(Succeed())
+
+		got := &infrav1.StackitCluster{}
+		Expect(k8sClient.Get(ctx, key, got)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, got)).To(Succeed())
+
+		_, err = reconciler.Reconcile(ctx, req)
+		Expect(err).NotTo(HaveOccurred(), "deletion must not block on a Secret that can never come back")
+
+		Eventually(func() bool {
+			return apierrors.IsNotFound(k8sClient.Get(ctx, key, &infrav1.StackitCluster{}))
+		}).Should(BeTrue(), "cluster stayed in Terminating because the finalizer was never removed")
+	})
+
+	It("tears the bastion down when disabled even if its status was never persisted", func() {
+		// Counterpart to the deletion path: disabling the bastion used to be
+		// gated on hasBastionStatus alone. With the status lost, nothing was torn
+		// down while the condition reported "bastion disabled" — leaving port 22
+		// open for the rest of the cluster's life.
+		got := &infrav1.StackitCluster{}
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		got.Spec.Bastion = validBastionSpec()
+		Expect(k8sClient.Update(ctx, got)).To(Succeed())
+		_, err := reconciler.Reconcile(ctx, request)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(fakeCloud.ServerCount()).To(Equal(1))
+
+		By("losing the persisted bastion status and its condition")
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		got.Status.Bastion = infrav1.StackitBastionStatus{}
+		got.Status.Conditions = nil
+		Expect(k8sClient.Status().Update(ctx, got)).To(Succeed())
+
+		By("disabling the bastion")
+		Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed())
+		got.Spec.Bastion.Enabled = false
+		Expect(k8sClient.Update(ctx, got)).To(Succeed())
+		_, err = reconciler.Reconcile(ctx, request)
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(fakeCloud.ServerCount()).To(Equal(0),
+			"bastion kept running with port 22 open while reporting itself disabled")
+		Expect(fakeCloud.PublicIPCount()).To(Equal(0))
+	})
+
 	It("validates bastion specs", func() {
 		spec := validBastionSpec()
 		Expect(validateBastionSpec(spec)).To(Succeed())

controller/stackitcluster_infrastructure.go

@@ -16,6 +16,7 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -193,9 +194,30 @@ func bootstrapTargetIP(network *cloud.Network) string {
 
 func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope.ClusterScope) error {
 	sc := s.StackitCluster
-	if sc.Status.APIServerLoadBalancerID != "" || hasBastionStatus(sc.Status.Bastion) || sc.Spec.APIServerLoadBalancer.Enabled {
+	// Both the load balancer and the bastion are gated by their spec flag, not
+	// only by persisted status: a resource can be created and the reconcile can
+	// stop before the status patch lands. Relying on status alone would skip
+	// cleanup entirely and leak the bastion server, its public IP and its
+	// security groups. The tag-based lookups in DeleteBastion tolerate an empty
+	// status, so running the block without one is safe.
+	if sc.Status.APIServerLoadBalancerID != "" || hasBastionStatus(sc.Status.Bastion) ||
+		sc.Spec.APIServerLoadBalancer.Enabled || sc.Spec.Bastion.Enabled {
 		cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, sc)
 		if err != nil {
+			// A missing credentials Secret can never be recovered from — it
+			// commonly disappears first during namespace teardown. Blocking here
+			// would strand the cluster in Terminating forever, so finalize and
+			// make the possible leak loud instead. Any other credentials problem
+			// is fixable, so keep retrying for those.
+			if apierrors.IsNotFound(err) {
+				if r.Recorder != nil {
+					r.Recorder.Eventf(sc, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete",
+						"Credentials Secret is gone; finalizing without cloud cleanup. "+
+							"Any remaining STACKIT resources for this cluster must be removed manually: %v", err)
+				}
+				controllerutil.RemoveFinalizer(sc, infrav1.ClusterFinalizer)
+				return nil
+			}
 			util.SetConditions(
 				&sc.Status.Conditions,
 				sc.Generation,
@@ -222,7 +244,11 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope
 				)
 			}
 		}
-		if hasBastionStatus(sc.Status.Bastion) {
+		// Driven by intent as well as by status: DeleteBastion and
+		// DeleteNodeSSHAccess resolve their resources by tag when the status
+		// fields are empty, so this also cleans up a bastion whose status patch
+		// never landed.
+		if hasBastionStatus(sc.Status.Bastion) || sc.Spec.Bastion.Enabled {
 			if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil && !cloud.IsNotFound(err) {
 				return err
 			}