From 288542f76df85d5f3afb591e73309958248587a3 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 06:28:47 +0000 Subject: [PATCH 01/14] Address review findings: clear status.ready on machine cloud failures and deduplicate repeated allowedCIDRs, each with a regression test --- cloud/sdk_client.go | 7 ++ cloud/sdk_client_test.go | 69 ++++++++++++++++++++ controller/stackitmachine_controller_test.go | 7 ++ controller/stackitmachine_infrastructure.go | 5 ++ 4 files changed, 88 insertions(+) diff --git a/cloud/sdk_client.go b/cloud/sdk_client.go index 01c35c8..89d372c 100644 --- a/cloud/sdk_client.go +++ b/cloud/sdk_client.go @@ -705,6 +705,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 diff --git a/cloud/sdk_client_test.go b/cloud/sdk_client_test.go index b22ee46..f795504 100644 --- a/cloud/sdk_client_test.go +++ b/cloud/sdk_client_test.go @@ -603,3 +603,72 @@ 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 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) + } +} diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 3cee372..194530b 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -163,6 +163,13 @@ var _ = Describe("StackitMachine Controller", func() { 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, "InstanceError") + 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() { diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 901a376..2bfa838 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -73,6 +73,9 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope server, created, err := r.ensureServer(ctx, cloudClient, s, bootstrapData) if err != nil { + // Keep the legacy boolean in step with the conditions: a machine whose + // server could not be ensured is not ready, even if it was before. + sm.Status.Ready = false return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, @@ -100,6 +103,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, s, server); err != nil { + sm.Status.Ready = false return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, @@ -112,6 +116,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, s, server); err != nil { + sm.Status.Ready = false return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, From 01aead2ff32fe681b66c41ce86545e8d1ab93556 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 06:55:50 +0000 Subject: [PATCH 02/14] fix hardcoded variable in template --- templates/cluster-template-bastion.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/cluster-template-bastion.yaml b/templates/cluster-template-bastion.yaml index 3c25b01..50488f5 100644 --- a/templates/cluster-template-bastion.yaml +++ b/templates/cluster-template-bastion.yaml @@ -157,7 +157,7 @@ metadata: namespace: ${NAMESPACE} spec: clusterName: ${CLUSTER_NAME} - replicas: 3 + replicas: ${WORKER_MACHINE_COUNT} selector: matchLabels: cluster.x-k8s.io/cluster-name: ${CLUSTER_NAME} From ff4747882ff05607a7375349682ec814d36f5bf3 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 06:58:55 +0000 Subject: [PATCH 03/14] Clean up bastion resources by intent rather than persisted status, with an envtest regression test --- controller/stackitcluster_controller_test.go | 44 ++++++++++++++++++++ controller/stackitcluster_infrastructure.go | 15 ++++++- 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index a853bcc..3cecde2 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -202,6 +202,50 @@ 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("validates bastion specs", func() { spec := validBastionSpec() Expect(validateBastionSpec(spec)).To(Succeed()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 25cf061..4e78216 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -193,7 +193,14 @@ 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 { util.SetConditions( @@ -222,7 +229,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 } From f32ae5d8e06960be6ebca1b08deb0fee2bd8c552 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 07:24:23 +0000 Subject: [PATCH 04/14] Restore an idempotent security-group re-attach so a detached group is repaired instead of aborting bastion reconciliation --- cloud/sdk_client.go | 22 ++++++++++++++++------ cloud/sdk_client_test.go | 29 +++++++++++++++++------------ 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/cloud/sdk_client.go b/cloud/sdk_client.go index 89d372c..fc02cb2 100644 --- a/cloud/sdk_client.go +++ b/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 { @@ -834,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 } } diff --git a/cloud/sdk_client_test.go b/cloud/sdk_client_test.go index f795504..d28269d 100644 --- a/cloud/sdk_client_test.go +++ b/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,10 @@ 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) + _, _ = w.Write([]byte(`{"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 +504,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 +555,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]) @@ -635,6 +638,8 @@ func TestSDKClientEnsureBastionDeduplicatesRepeatedCIDRs(t *testing.T) { "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{ From 8a737670eca39b8f3a9a947c9689e3f98968226e Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 07:27:14 +0000 Subject: [PATCH 05/14] Finalize cluster deletion when the credentials Secret is already gone instead of hanging in Terminating --- controller/stackitcluster_controller_test.go | 37 ++++++++++++++++++++ controller/stackitcluster_infrastructure.go | 15 ++++++++ 2 files changed, 52 insertions(+) diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 3cecde2..f8c3a98 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -246,6 +246,43 @@ var _ = Describe("StackitCluster Controller", func() { 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("validates bastion specs", func() { spec := validBastionSpec() Expect(validateBastionSpec(spec)).To(Succeed()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 4e78216..70a90dd 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/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" @@ -203,6 +204,20 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope 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, From 4af4cda0b71257ff6cbbf1d2f99fb9a1f4586a5f Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 07:32:28 +0000 Subject: [PATCH 06/14] Tear down a disabled bastion by intent so a lost status patch cannot leave it running while reporting itself disabled --- controller/stackitcluster_bastion.go | 15 +++++++++- controller/stackitcluster_controller_test.go | 31 ++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index 508f4d6..d89dab7 100644 --- a/controller/stackitcluster_bastion.go +++ b/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,19 @@ 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 sweep to once per cluster instead of once per reconcile, + // which matters because this path runs for every cluster without a + // bastion. + sweep := hasBastionStatus(sc.Status.Bastion) || + meta.FindStatusCondition(sc.Status.Conditions, infrav1.ClusterBastionReadyCondition) == nil + if sweep { if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil { return ctrl.Result{}, false, err } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index f8c3a98..cdda629 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -283,6 +283,37 @@ var _ = Describe("StackitCluster Controller", func() { }).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()) From 57fb9f3bfca1924fa79ab116ce94d0e4ef979d6f Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 14:42:44 +0000 Subject: [PATCH 07/14] fix lint --- controller/controller_test_helpers_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index c495ee8..26faa55 100644 --- a/controller/controller_test_helpers_test.go +++ b/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}, From c950d3a2f7787f9a1cdfafeca7774a84882ca345 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 14:45:54 +0000 Subject: [PATCH 08/14] Stub the duplicate-attach rejection with writeJSON instead of a hand-written JSON body --- cloud/sdk_client_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cloud/sdk_client_test.go b/cloud/sdk_client_test.go index d28269d..11b0888 100644 --- a/cloud/sdk_client_test.go +++ b/cloud/sdk_client_test.go @@ -471,7 +471,10 @@ func TestSDKClientEnsureBastionToleratesDuplicateSecurityGroupAttach(t *testing. // Answer the way the real API does for an already-attached group. attachCallCount++ w.WriteHeader(http.StatusBadRequest) - _, _ = w.Write([]byte(`{"code":400,"msg":"request invalid: Invalid input for security_groups. Reason: Duplicate items in the list."}`)) + 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"): From 651c6cb6360b4cf4d8532f74cf6c60a94a74abc3 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 14:46:15 +0000 Subject: [PATCH 09/14] Inline the bastion cleanup condition instead of naming it in a single-use variable --- controller/stackitcluster_bastion.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index d89dab7..0d30bf2 100644 --- a/controller/stackitcluster_bastion.go +++ b/controller/stackitcluster_bastion.go @@ -52,12 +52,10 @@ func (r *StackitClusterReconciler) reconcileBastion( // // 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 sweep to once per cluster instead of once per reconcile, + // tag-based cleanup to once per cluster instead of once per reconcile, // which matters because this path runs for every cluster without a // bastion. - sweep := hasBastionStatus(sc.Status.Bastion) || - meta.FindStatusCondition(sc.Status.Conditions, infrav1.ClusterBastionReadyCondition) == nil - if sweep { + 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 } From d0e714fbce83b42d9a6f63eeaa59fec1f773d003 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Fri, 14 Aug 2026 14:46:50 +0000 Subject: [PATCH 10/14] Flip machine readiness through MachineScope.SetNotReady instead of assigning Status.Ready directly --- controller/stackitmachine_infrastructure.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 2bfa838..863b4e2 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -73,9 +73,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope server, created, err := r.ensureServer(ctx, cloudClient, s, bootstrapData) if err != nil { - // Keep the legacy boolean in step with the conditions: a machine whose - // server could not be ensured is not ready, even if it was before. - sm.Status.Ready = false + s.SetNotReady("InstanceError", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, @@ -103,7 +101,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, s, server); err != nil { - sm.Status.Ready = false + s.SetNotReady("BastionSSHAccessError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, @@ -116,7 +114,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, s, server); err != nil { - sm.Status.Ready = false + s.SetNotReady("LoadBalancerTargetError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, From 2b0b2d6919f2dadafaeb93b9a16ecc9911ef00fa Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Tue, 18 Aug 2026 10:22:27 +0000 Subject: [PATCH 11/14] Drop the parameter from createOwnerCluster instead of suppressing unparam --- controller/controller_test_helpers_test.go | 7 ++----- controller/stackitcluster_controller_test.go | 6 +++--- controller/stackitmachine_controller_test.go | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index 26faa55..fd95fb3 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -73,12 +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) { +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, diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index cdda629..5c7797e 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -60,7 +60,7 @@ var _ = Describe("StackitCluster Controller", func() { } createCredentialsSecret(ctx, credentials, namespace, testProjectID) - createOwnerCluster(ctx, clusterName, namespace) + createOwnerCluster(ctx, clusterName) stackitClust = newStackitCluster(clusterName, namespace, true) stackitClust.Spec.CredentialsSecretRef.Name = credentials Expect(k8sClient.Create(ctx, stackitClust)).To(Succeed()) @@ -208,7 +208,7 @@ var _ = Describe("StackitCluster Controller", func() { // 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) + createOwnerCluster(ctx, clusterName+"-nolb") defer deleteIfExists(ctx, &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nolb", Namespace: namespace}, }) @@ -251,7 +251,7 @@ var _ = Describe("StackitCluster Controller", func() { // 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) + createOwnerCluster(ctx, clusterName+"-nocreds") defer deleteIfExists(ctx, &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: clusterName + "-nocreds", Namespace: namespace}, }) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 194530b..ed72106 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -67,7 +67,7 @@ var _ = Describe("StackitMachine Controller", func() { } createCredentialsSecret(ctx, credentials, namespace, testProjectID) - createOwnerCluster(ctx, clusterName, namespace) + createOwnerCluster(ctx, clusterName) createReadyStackitCluster(ctx, clusterName, namespace, credentials) createOwnerMachine(ctx, machineName, namespace, clusterName, stackitName, nil) stackitMach = newStackitMachine(stackitName, namespace, machineName) From 90cf3d94146f52619bf10bdec7c4af9feacf963d Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Tue, 18 Aug 2026 12:13:30 +0000 Subject: [PATCH 12/14] Tear the bastion down when disabled even if its status was lost --- controller/stackitcluster_bastion.go | 25 ++++++++-------- controller/stackitcluster_controller_test.go | 30 ++++++++++++++++++++ 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index 0d30bf2..f24563d 100644 --- a/controller/stackitcluster_bastion.go +++ b/controller/stackitcluster_bastion.go @@ -30,6 +30,10 @@ 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, @@ -45,17 +49,14 @@ func (r *StackitClusterReconciler) reconcileBastion( } if !sc.Spec.Bastion.Enabled { - // 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 { + // 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(sc.Status.Conditions, infrav1.ClusterBastionReadyCondition) + if condition == nil || condition.Reason != bastionDisabledReason { if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil { return ctrl.Result{}, false, err } @@ -67,7 +68,7 @@ func (r *StackitClusterReconciler) reconcileBastion( r.Recorder.Eventf(sc, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") } } - s.SetConditions(metav1.ConditionTrue, "Skipped", "bastion disabled", infrav1.ClusterBastionReadyCondition) + s.SetConditions(metav1.ConditionTrue, bastionDisabledReason, "bastion disabled", infrav1.ClusterBastionReadyCondition) return ctrl.Result{}, true, nil } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 5c7797e..454b9f0 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -314,6 +314,36 @@ var _ = Describe("StackitCluster Controller", func() { Expect(fakeCloud.PublicIPCount()).To(Equal(0)) }) + It("tears the bastion down when disabled even if only its status was lost", func() { + // Narrower than the case above: the condition survives and still reports + // the bastion as available, only the bastion status fields are gone. + // Gating the cleanup on hasBastionStatus left the server running here. + 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 while keeping the conditions") + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Status.Bastion = infrav1.StackitBastionStatus{} + Expect(k8sClient.Status().Update(ctx, got)).To(Succeed()) + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.ClusterBastionReadyCondition, metav1.ConditionTrue, "Available") + + By("disabling the bastion") + 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 because cleanup was gated on the bastion status") + Expect(fakeCloud.PublicIPCount()).To(Equal(0)) + }) + It("validates bastion specs", func() { spec := validBastionSpec() Expect(validateBastionSpec(spec)).To(Succeed()) From d8eefd7e46d5b96bdc277adf4e0f094f02190bc1 Mon Sep 17 00:00:00 2001 From: Alexander Mai Date: Tue, 18 Aug 2026 12:27:09 +0000 Subject: [PATCH 13/14] Clean up cloud resources unconditionally on cluster deletion so no spec or status combination can leak them --- controller/stackitcluster_controller_test.go | 29 +++++ controller/stackitcluster_infrastructure.go | 124 +++++++++---------- 2 files changed, 88 insertions(+), 65 deletions(-) diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index 454b9f0..ef24ac0 100644 --- a/controller/stackitcluster_controller_test.go +++ b/controller/stackitcluster_controller_test.go @@ -344,6 +344,35 @@ var _ = Describe("StackitCluster Controller", func() { Expect(fakeCloud.PublicIPCount()).To(Equal(0)) }) + It("cleans up the load balancer during deletion when it was disabled and its status was lost", func() { + // Counterpart to the bastion case: flipping apiServerLoadBalancer.enabled + // off neither deletes the load balancer nor clears its ID, so with the + // status patch lost the deletion gate matched nothing and the load + // balancer stayed behind. + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.LoadBalancerCount()).To(Equal(1)) + + By("losing the persisted load balancer ID") + got := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Status.APIServerLoadBalancerID = "" + Expect(k8sClient.Status().Update(ctx, got)).To(Succeed()) + + By("disabling the load balancer") + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + got.Spec.APIServerLoadBalancer.Enabled = false + Expect(k8sClient.Update(ctx, got)).To(Succeed()) + + By("deleting the cluster") + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + Expect(fakeCloud.LoadBalancerCount()).To(Equal(0), + "load balancer leaked because cleanup was gated on spec and status") + }) + It("validates bastion specs", func() { spec := validBastionSpec() Expect(validateBastionSpec(spec)).To(Succeed()) diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index 70a90dd..f4c28ef 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -194,76 +194,70 @@ func bootstrapTargetIP(network *cloud.Network) string { func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope.ClusterScope) error { sc := s.StackitCluster - // 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 + // Cleanup runs unconditionally. Neither spec nor status is a trustworthy + // record of what exists in the cloud: a resource can be created before its + // status patch lands, and disabling the load balancer or the bastion leaves + // the running resource behind. ResolveID, DeleteBastion and + // DeleteNodeSSHAccess all fall back to tag lookups and tolerate NotFound, so + // asking for everything costs a handful of list calls once per cluster and + // removes every combination in which a resource could be missed. + 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) } - util.SetConditions( - &sc.Status.Conditions, - sc.Generation, - metav1.ConditionFalse, - "CredentialsInvalid", - err.Error(), - infrav1.ClusterCredentialsReadyCondition, - ) - return err + controllerutil.RemoveFinalizer(sc, infrav1.ClusterFinalizer) + return nil } - loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, sc) - if err != nil { + util.SetConditions( + &sc.Status.Conditions, + sc.Generation, + metav1.ConditionFalse, + "CredentialsInvalid", + err.Error(), + infrav1.ClusterCredentialsReadyCondition, + ) + return err + } + loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, sc) + if err != nil { + return err + } + if loadBalancerID != "" { + if err := cloudClient.DeleteAPIServerLoadBalancer(ctx, loadBalancerID); err != nil && !cloud.IsNotFound(err) { return err } - if loadBalancerID != "" { - if err := cloudClient.DeleteAPIServerLoadBalancer(ctx, loadBalancerID); err != nil && !cloud.IsNotFound(err) { - return err - } - sc.Status.APIServerLoadBalancerID = "" - if r.Recorder != nil { - r.Recorder.Eventf( - sc, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", - "Deleted API server load balancer %s", loadBalancerID, - ) - } + sc.Status.APIServerLoadBalancerID = "" + if r.Recorder != nil { + r.Recorder.Eventf( + sc, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", + "Deleted API server load balancer %s", loadBalancerID, + ) } - // 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 - } - if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(sc, nil), cloud.Bastion{ - ServerID: sc.Status.Bastion.ServerID, - PublicIPID: sc.Status.Bastion.PublicIPID, - PublicIP: sc.Status.Bastion.PublicIP, - SecurityGroupID: sc.Status.Bastion.SecurityGroupID, - }); err != nil && !cloud.IsNotFound(err) { - return err - } - s.ClearBastionStatus() - if r.Recorder != nil { - r.Recorder.Eventf(sc, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") - } + } + if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil && !cloud.IsNotFound(err) { + return err + } + if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(sc, nil), cloud.Bastion{ + ServerID: sc.Status.Bastion.ServerID, + PublicIPID: sc.Status.Bastion.PublicIPID, + PublicIP: sc.Status.Bastion.PublicIP, + SecurityGroupID: sc.Status.Bastion.SecurityGroupID, + }); err != nil && !cloud.IsNotFound(err) { + return err + } + if hasBastionStatus(sc.Status.Bastion) { + s.ClearBastionStatus() + if r.Recorder != nil { + r.Recorder.Eventf(sc, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") } } controllerutil.RemoveFinalizer(sc, infrav1.ClusterFinalizer) From b77942006315e9c320ea8aff09246233d48db230 Mon Sep 17 00:00:00 2001 From: Jan Larwig Date: Tue, 18 Aug 2026 15:55:17 +0200 Subject: [PATCH 14/14] chore: speaking variable names and little formatting for better readability Signed-off-by: Jan Larwig --- controller/stackitcluster_bastion.go | 86 ++++++++---- controller/stackitcluster_infrastructure.go | 145 ++++++++++++-------- 2 files changed, 146 insertions(+), 85 deletions(-) diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index f24563d..5e07cb4 100644 --- a/controller/stackitcluster_bastion.go +++ b/controller/stackitcluster_bastion.go @@ -37,91 +37,121 @@ 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, + ServerID: cluster.Status.Bastion.ServerID, + PublicIPID: cluster.Status.Bastion.PublicIPID, + PublicIP: cluster.Status.Bastion.PublicIP, + SecurityGroupID: cluster.Status.Bastion.SecurityGroupID, } - if !sc.Spec.Bastion.Enabled { + 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(sc.Status.Conditions, infrav1.ClusterBastionReadyCondition) + condition := meta.FindStatusCondition(cluster.Status.Conditions, infrav1.ClusterBastionReadyCondition) if condition == nil || condition.Reason != bastionDisabledReason { - if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil { + 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, bastionDisabledReason, "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 } diff --git a/controller/stackitcluster_infrastructure.go b/controller/stackitcluster_infrastructure.go index f4c28ef..1380c0c 100644 --- a/controller/stackitcluster_infrastructure.go +++ b/controller/stackitcluster_infrastructure.go @@ -31,34 +31,39 @@ import ( "github.com/stackitcloud/cluster-api-provider-stackit/util" ) -func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, s *scope.ClusterScope) (ctrl.Result, error) { +func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, clusterScope *scope.ClusterScope) (ctrl.Result, error) { log := logf.FromContext(ctx) - sc := s.StackitCluster + cluster := clusterScope.StackitCluster - if !controllerutil.ContainsFinalizer(sc, infrav1.ClusterFinalizer) { - controllerutil.AddFinalizer(sc, infrav1.ClusterFinalizer) + if !controllerutil.ContainsFinalizer(cluster, infrav1.ClusterFinalizer) { + controllerutil.AddFinalizer(cluster, infrav1.ClusterFinalizer) } - sc.Status.FailureDomains = stackitFailureDomains(sc.Spec.Region) + cluster.Status.FailureDomains = stackitFailureDomains(cluster.Spec.Region) - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, sc) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, cluster) if err != nil { - sc.Status.Ready = false + cluster.Status.Ready = false return util.CredentialFailureResult( - &sc.Status.Conditions, - sc.Generation, + &cluster.Status.Conditions, + cluster.Generation, err, infrav1.ClusterCredentialsReadyCondition, infrav1.ClusterReadyCondition, ) } - s.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.ClusterCredentialsReadyCondition) + clusterScope.SetConditions( + metav1.ConditionTrue, + "Available", + "", + infrav1.ClusterCredentialsReadyCondition, + ) - network, err := cloudClient.GetNetwork(ctx, sc.Spec.Network.ID) + network, err := cloudClient.GetNetwork(ctx, cluster.Spec.Network.ID) if err != nil { - sc.Status.Ready = false + cluster.Status.Ready = false return util.CloudFailureResult( - &sc.Status.Conditions, - sc.Generation, + &cluster.Status.Conditions, + cluster.Generation, "NetworkNotFound", err, retryableErrorRequeueAfter, @@ -67,21 +72,26 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, s *scope infrav1.ClusterReadyCondition, ) } - s.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.ClusterNetworkReadyCondition) + clusterScope.SetConditions( + metav1.ConditionTrue, + "Available", + "", + infrav1.ClusterNetworkReadyCondition, + ) - if sc.Spec.APIServerLoadBalancer.Enabled { + if cluster.Spec.APIServerLoadBalancer.Enabled { lb, err := cloudClient.EnsureAPIServerLoadBalancer( ctx, loadbalancerservice.APIServerInput( - sc, + cluster, []cloud.LoadBalancerTargetInput{loadbalancerservice.BootstrapTarget(bootstrapTargetIP(network))}, ), ) if err != nil { - sc.Status.Ready = false + cluster.Status.Ready = false return util.CloudFailureResult( - &sc.Status.Conditions, - sc.Generation, + &cluster.Status.Conditions, + cluster.Generation, "LoadBalancerError", err, retryableErrorRequeueAfter, @@ -90,45 +100,65 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, s *scope infrav1.ClusterReadyCondition, ) } - hadLoadBalancerID := sc.Status.APIServerLoadBalancerID != "" + hadLoadBalancerID := cluster.Status.APIServerLoadBalancerID != "" if lb != nil { - sc.Status.APIServerLoadBalancerID = lb.ID + cluster.Status.APIServerLoadBalancerID = lb.ID if !hadLoadBalancerID && lb.ID != "" && r.Recorder != nil { r.Recorder.Eventf( - sc, nil, corev1.EventTypeNormal, "LoadBalancerCreated", "Create", + cluster, nil, corev1.EventTypeNormal, "LoadBalancerCreated", "Create", "Created API server load balancer %s", lb.ID, ) } } if lb == nil || lb.IP == "" { - s.SetNotReady("Provisioning", "waiting for API server load balancer IP address", infrav1.ClusterLoadBalancerReadyCondition, infrav1.ClusterReadyCondition) + clusterScope.SetNotReady( + "Provisioning", + "waiting for API server load balancer IP address", + infrav1.ClusterLoadBalancerReadyCondition, + infrav1.ClusterReadyCondition, + ) return ctrl.Result{RequeueAfter: 10 * time.Second}, nil } endpoint := clusterv1.APIEndpoint{ Host: lb.IP, Port: defaultAPIServerPort, } - s.SetAPIServerEndpoint(endpoint) - s.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.ClusterLoadBalancerReadyCondition) + clusterScope.SetAPIServerEndpoint(endpoint) + clusterScope.SetConditions( + metav1.ConditionTrue, + "Available", + "", + infrav1.ClusterLoadBalancerReadyCondition, + ) if r.Recorder != nil { r.Recorder.Eventf( - sc, nil, corev1.EventTypeNormal, "LoadBalancerReady", "SetReady", + cluster, nil, corev1.EventTypeNormal, "LoadBalancerReady", "SetReady", "API server load balancer is ready at %s", lb.IP, ) } - } else if sc.Spec.ControlPlaneEndpoint.Host != "" { - sc.Status.APIServerEndpoint = sc.Spec.ControlPlaneEndpoint - s.SetConditions(metav1.ConditionTrue, "Skipped", "external endpoint provided", infrav1.ClusterLoadBalancerReadyCondition) + } else if cluster.Spec.ControlPlaneEndpoint.Host != "" { + cluster.Status.APIServerEndpoint = cluster.Spec.ControlPlaneEndpoint + clusterScope.SetConditions( + metav1.ConditionTrue, + "Skipped", + "external endpoint provided", + infrav1.ClusterLoadBalancerReadyCondition, + ) } else { - s.SetNotReady("EndpointMissing", "apiServerLoadBalancer.enabled is false and controlPlaneEndpoint is empty", infrav1.ClusterLoadBalancerReadyCondition, infrav1.ClusterReadyCondition) + clusterScope.SetNotReady( + "EndpointMissing", + "apiServerLoadBalancer.enabled is false and controlPlaneEndpoint is empty", + infrav1.ClusterLoadBalancerReadyCondition, + infrav1.ClusterReadyCondition, + ) return ctrl.Result{}, nil } - if result, ready, err := r.reconcileBastion(ctx, cloudClient, s); err != nil { - sc.Status.Ready = false + if result, ready, err := r.reconcileBastion(ctx, cloudClient, clusterScope); err != nil { + cluster.Status.Ready = false return util.CloudFailureResult( - &sc.Status.Conditions, - sc.Generation, + &cluster.Status.Conditions, + cluster.Generation, "BastionError", err, retryableErrorRequeueAfter, @@ -140,8 +170,8 @@ func (r *StackitClusterReconciler) reconcileNormal(ctx context.Context, s *scope return result, nil } - s.SetReady() - log.V(1).Info("StackitCluster ready", "endpoint", sc.Status.APIServerEndpoint) + clusterScope.SetReady() + log.V(1).Info("StackitCluster ready", "endpoint", cluster.Status.APIServerEndpoint) return ctrl.Result{}, nil } @@ -192,8 +222,9 @@ func bootstrapTargetIP(network *cloud.Network) string { return "10.0.0.1" } -func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope.ClusterScope) error { - sc := s.StackitCluster +func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, clusterScope *scope.ClusterScope) error { + cluster := clusterScope.StackitCluster + // Cleanup runs unconditionally. Neither spec nor status is a trustworthy // record of what exists in the cloud: a resource can be created before its // status patch lands, and disabling the load balancer or the bastion leaves @@ -201,7 +232,7 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope // DeleteNodeSSHAccess all fall back to tag lookups and tolerate NotFound, so // asking for everything costs a handful of list calls once per cluster and // removes every combination in which a resource could be missed. - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, sc) + cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, cluster) if err != nil { // A missing credentials Secret can never be recovered from — it commonly // disappears first during namespace teardown. Blocking here would strand @@ -210,16 +241,16 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope // retrying for those. if apierrors.IsNotFound(err) { if r.Recorder != nil { - r.Recorder.Eventf(sc, nil, corev1.EventTypeWarning, "CleanupSkipped", "Delete", + r.Recorder.Eventf(cluster, 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) + controllerutil.RemoveFinalizer(cluster, infrav1.ClusterFinalizer) return nil } util.SetConditions( - &sc.Status.Conditions, - sc.Generation, + &cluster.Status.Conditions, + cluster.Generation, metav1.ConditionFalse, "CredentialsInvalid", err.Error(), @@ -227,7 +258,7 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope ) return err } - loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, sc) + loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, cluster) if err != nil { return err } @@ -235,31 +266,31 @@ func (r *StackitClusterReconciler) reconcileDelete(ctx context.Context, s *scope if err := cloudClient.DeleteAPIServerLoadBalancer(ctx, loadBalancerID); err != nil && !cloud.IsNotFound(err) { return err } - sc.Status.APIServerLoadBalancerID = "" + cluster.Status.APIServerLoadBalancerID = "" if r.Recorder != nil { r.Recorder.Eventf( - sc, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", + cluster, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", "Deleted API server load balancer %s", loadBalancerID, ) } } - if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(sc)); err != nil && !cloud.IsNotFound(err) { + if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil && !cloud.IsNotFound(err) { return err } - if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(sc, nil), cloud.Bastion{ - ServerID: sc.Status.Bastion.ServerID, - PublicIPID: sc.Status.Bastion.PublicIPID, - PublicIP: sc.Status.Bastion.PublicIP, - SecurityGroupID: sc.Status.Bastion.SecurityGroupID, + if err := cloudClient.DeleteBastion(ctx, bastionservice.Input(cluster, nil), cloud.Bastion{ + ServerID: cluster.Status.Bastion.ServerID, + PublicIPID: cluster.Status.Bastion.PublicIPID, + PublicIP: cluster.Status.Bastion.PublicIP, + SecurityGroupID: cluster.Status.Bastion.SecurityGroupID, }); err != nil && !cloud.IsNotFound(err) { return err } - if hasBastionStatus(sc.Status.Bastion) { - s.ClearBastionStatus() + if hasBastionStatus(cluster.Status.Bastion) { + 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") } } - controllerutil.RemoveFinalizer(sc, infrav1.ClusterFinalizer) + controllerutil.RemoveFinalizer(cluster, infrav1.ClusterFinalizer) return nil }