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,9 +73,9 @@ func createCloudInitSecret(ctx context.Context, name, namespace, key, value stri
 	Expect(k8sClient.Create(ctx, secret)).To(Succeed())
 }
 
-func createOwnerCluster(ctx context.Context, name, namespace string) {
+func createOwnerCluster(ctx context.Context, name string) {
 	cluster := &clusterv1.Cluster{
-		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"},
 		Spec: clusterv1.ClusterSpec{
 			InfrastructureRef: clusterv1.ContractVersionedObjectReference{
 				APIGroup: infrav1.GroupVersion.Group,

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"
@@ -29,87 +30,128 @@ import (
 	"github.com/stackitcloud/cluster-api-provider-stackit/scope"
 )
 
+// bastionDisabledReason is read back to decide whether the cleanup below
+// already ran, so it must not drift.
+const bastionDisabledReason = "Skipped"
+
 func (r *StackitClusterReconciler) reconcileBastion(
 	ctx context.Context,
 	cloudClient cloud.Client,
-	s *scope.ClusterScope,
+	clusterScope *scope.ClusterScope,
 ) (ctrl.Result, bool, error) {
-	sc := s.StackitCluster
-	input := bastionservice.Input(sc, nil)
+	cluster := clusterScope.StackitCluster
+	input := bastionservice.Input(cluster, nil)
 	status := cloud.Bastion{
-		ServerID:        sc.Status.Bastion.ServerID,
-		PublicIPID:      sc.Status.Bastion.PublicIPID,
-		PublicIP:        sc.Status.Bastion.PublicIP,
-		SecurityGroupID: sc.Status.Bastion.SecurityGroupID,
-	}
-
-	if !sc.Spec.Bastion.Enabled {
-		if hasBastionStatus(sc.Status.Bastion) {
-			if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil {
+		ServerID:        cluster.Status.Bastion.ServerID,
+		PublicIPID:      cluster.Status.Bastion.PublicIPID,
+		PublicIP:        cluster.Status.Bastion.PublicIP,
+		SecurityGroupID: cluster.Status.Bastion.SecurityGroupID,
+	}
+
+	if !cluster.Spec.Bastion.Enabled {
+		// The condition carries this reason only after a cleanup has succeeded,
+		// so anything else means we may still own bastion resources — including
+		// the case where EnsureBastion succeeded but its status patch was lost.
+		// Keying on it instead of on the status keeps the tag-based cleanup to
+		// once per cluster rather than once per reconcile, which matters because
+		// this path runs for every cluster without a bastion.
+		condition := meta.FindStatusCondition(cluster.Status.Conditions, infrav1.ClusterBastionReadyCondition)
+		if condition == nil || condition.Reason != bastionDisabledReason {
+			if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil {
 				return ctrl.Result{}, false, err
 			}
 			if err := cloudClient.DeleteBastion(ctx, input, status); err != nil {
 				return ctrl.Result{}, false, err
 			}
-			s.ClearBastionStatus()
+			clusterScope.ClearBastionStatus()
 			if r.Recorder != nil {
-				r.Recorder.Eventf(sc, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion")
+				r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion")
 			}
 		}
-		s.SetConditions(metav1.ConditionTrue, "Skipped", "bastion disabled", infrav1.ClusterBastionReadyCondition)
+		clusterScope.SetConditions(
+			metav1.ConditionTrue,
+			bastionDisabledReason,
+			"bastion disabled",
+			infrav1.ClusterBastionReadyCondition,
+		)
 		return ctrl.Result{}, true, nil
 	}
 
-	if err := validateBastionSpec(sc.Spec.Bastion); err != nil {
-		s.SetNotReady("InvalidBastionSpec", err.Error(), infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
+	if err := validateBastionSpec(cluster.Spec.Bastion); err != nil {
+		clusterScope.SetNotReady(
+			"InvalidBastionSpec",
+			err.Error(),
+			infrav1.ClusterBastionReadyCondition,
+			infrav1.ClusterReadyCondition,
+		)
 		return ctrl.Result{}, false, nil
 	}
 
-	cloudInit, err := r.resolveBastionCloudInit(ctx, sc)
+	cloudInit, err := r.resolveBastionCloudInit(ctx, cluster)
 	if err != nil {
-		s.SetNotReady("CloudInitRefError", err.Error(), infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
+		clusterScope.SetNotReady(
+			"CloudInitRefError",
+			err.Error(),
+			infrav1.ClusterBastionReadyCondition,
+			infrav1.ClusterReadyCondition,
+		)
 		return ctrl.Result{}, false, nil
 	}
 	input.CloudInit = cloudInit
 
-	if bastionNeedsRecreate(sc, cloudInit) {
-		if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil && !cloud.IsNotFound(err) {
+	if bastionNeedsRecreate(cluster, cloudInit) {
+		if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil && !cloud.IsNotFound(err) {
 			return ctrl.Result{}, false, err
 		}
 		if err := cloudClient.DeleteBastion(ctx, input, status); err != nil && !cloud.IsNotFound(err) {
 			return ctrl.Result{}, false, err
 		}
-		s.ClearBastionStatus()
-		s.SetNotReady("Recreating", "recreating bastion because cloudInitRef content changed", infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
+		clusterScope.ClearBastionStatus()
+		clusterScope.SetNotReady("Recreating", "recreating bastion because cloudInitRef content changed", infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
 		if r.Recorder != nil {
 			r.Recorder.Eventf(
-				sc, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate",
+				cluster, nil, corev1.EventTypeNormal, "BastionRecreating", "Recreate",
 				"Recreating bastion because cloudInitRef content changed",
 			)
 		}
 		return ctrl.Result{RequeueAfter: retryableErrorRequeueAfter}, false, nil
 	}
 
-	hadBastionStatus := hasBastionStatus(sc.Status.Bastion)
+	hadBastionStatus := hasBastionStatus(cluster.Status.Bastion)
 	bastion, err := cloudClient.EnsureBastion(ctx, input)
 	if err != nil {
 		return ctrl.Result{}, false, err
 	}
-	s.SetBastionStatus(bastion, bastionCloudInitHash(cloudInit))
+	clusterScope.SetBastionStatus(bastion, bastionCloudInitHash(cloudInit))
 	if !hadBastionStatus && r.Recorder != nil {
 		r.Recorder.Eventf(
-			sc, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID,
+			cluster, nil, corev1.EventTypeNormal, "BastionCreated", "Create", "Created bastion %s", bastion.ServerID,
 		)
 	}
 	if bastion.ServerState != "" && bastion.ServerState != "ACTIVE" {
-		s.SetNotReady("Provisioning", fmt.Sprintf("bastion server state is %s", bastion.ServerState), infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
+		clusterScope.SetNotReady(
+			"Provisioning",
+			fmt.Sprintf("bastion server state is %s", bastion.ServerState),
+			infrav1.ClusterBastionReadyCondition,
+			infrav1.ClusterReadyCondition,
+		)
 		return ctrl.Result{RequeueAfter: 15 * time.Second}, false, nil
 	}
 	if bastion.PublicIP == "" {
-		s.SetNotReady("Provisioning", "waiting for bastion public IP address", infrav1.ClusterBastionReadyCondition, infrav1.ClusterReadyCondition)
+		clusterScope.SetNotReady(
+			"Provisioning",
+			"waiting for bastion public IP address",
+			infrav1.ClusterBastionReadyCondition,
+			infrav1.ClusterReadyCondition,
+		)
 		return ctrl.Result{RequeueAfter: 10 * time.Second}, false, nil
 	}
-	s.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.ClusterBastionReadyCondition)
+	clusterScope.SetConditions(
+		metav1.ConditionTrue,
+		"Available",
+		"",
+		infrav1.ClusterBastionReadyCondition,
+	)
 	return ctrl.Result{}, true, nil
 }