diff --git a/cloud/sdk_client.go b/cloud/sdk_client.go index 01c35c8..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 { @@ -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 } } diff --git a/cloud/sdk_client_test.go b/cloud/sdk_client_test.go index b22ee46..11b0888 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,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) + } +} diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index c495ee8..fd95fb3 100644 --- a/controller/controller_test_helpers_test.go +++ b/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, diff --git a/controller/stackitcluster_bastion.go b/controller/stackitcluster_bastion.go index 508f4d6..5e07cb4 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" @@ -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 } diff --git a/controller/stackitcluster_controller_test.go b/controller/stackitcluster_controller_test.go index a853bcc..ef24ac0 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()) @@ -202,6 +202,177 @@ 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") + 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") + 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("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("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 25cf061..1380c0c 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" @@ -30,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, @@ -66,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, @@ -89,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, @@ -139,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 } @@ -191,55 +222,75 @@ 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 - if sc.Status.APIServerLoadBalancerID != "" || hasBastionStatus(sc.Status.Bastion) || sc.Spec.APIServerLoadBalancer.Enabled { - cloudClient, err := util.BuildCloudClient(ctx, r.Client, r.CloudClientFactory, sc) - if err != nil { - util.SetConditions( - &sc.Status.Conditions, - sc.Generation, - metav1.ConditionFalse, - "CredentialsInvalid", - err.Error(), - infrav1.ClusterCredentialsReadyCondition, - ) - return err +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 + // 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, cluster) + 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(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(cluster, infrav1.ClusterFinalizer) + return nil } - loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, sc) - if err != nil { + util.SetConditions( + &cluster.Status.Conditions, + cluster.Generation, + metav1.ConditionFalse, + "CredentialsInvalid", + err.Error(), + infrav1.ClusterCredentialsReadyCondition, + ) + return err + } + loadBalancerID, err := loadbalancerservice.ResolveID(ctx, cloudClient, cluster) + 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, - ) - } + cluster.Status.APIServerLoadBalancerID = "" + if r.Recorder != nil { + r.Recorder.Eventf( + cluster, nil, corev1.EventTypeNormal, "LoadBalancerDeleted", "Delete", + "Deleted API server load balancer %s", loadBalancerID, + ) } - if hasBastionStatus(sc.Status.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 - } - s.ClearBastionStatus() - if r.Recorder != nil { - r.Recorder.Eventf(sc, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") - } + } + if err := cloudClient.DeleteNodeSSHAccess(ctx, bastionservice.NodeSSHAccessTags(cluster)); err != nil && !cloud.IsNotFound(err) { + return err + } + 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(cluster.Status.Bastion) { + clusterScope.ClearBastionStatus() + if r.Recorder != nil { + r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "BastionDeleted", "Delete", "Deleted bastion") } } - controllerutil.RemoveFinalizer(sc, infrav1.ClusterFinalizer) + controllerutil.RemoveFinalizer(cluster, infrav1.ClusterFinalizer) return nil } diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 3cee372..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) @@ -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..863b4e2 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -73,6 +73,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope server, created, err := r.ensureServer(ctx, cloudClient, s, bootstrapData) if err != nil { + s.SetNotReady("InstanceError", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, @@ -100,6 +101,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if err := r.reconcileBastionNodeSSHAccess(ctx, cloudClient, s, server); err != nil { + s.SetNotReady("BastionSSHAccessError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, @@ -112,6 +114,7 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope } if err := r.reconcileAPIServerLoadBalancerTarget(ctx, cloudClient, s, server); err != nil { + s.SetNotReady("LoadBalancerTargetError", err.Error(), infrav1.MachineReadyCondition) return util.CloudFailureResult( &sm.Status.Conditions, sm.Generation, 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}