Skip to content

fix: bastion and machine reconcile defects - #4

Merged
tuunit merged 14 commits into
mainfrom
fix/bastion-and-machine-reconcile-defects
Aug 18, 2026
Merged

fix: bastion and machine reconcile defects#4
tuunit merged 14 commits into
mainfrom
fix/bastion-and-machine-reconcile-defects

Conversation

@Herbaert

@Herbaert Herbaert commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes seven defects in the bastion and machine reconcile paths, each with a
regression test. All were found by running the provider against real STACKIT
infrastructure and by the Copilot review on #1; none are introduced by the
refactor — they exist on main too.

What was broken

Silent resource leaks

  • ensureServer recreated a server for machines that had already bootstrapped
    and joined. The replacement replays the original bootstrap data, so it either
    never rejoins (different IP) or rejoins while Machine and Node keep
    pointing at the deleted server (same IP) — the latter reports a healthy 3/3
    cluster while carrying a dangling provider ID. Now surfaces an error instead
    of recreating.
  • Bastion cleanup on deletion, and bastion teardown on enabled: false, were
    both gated on persisted status. A bastion whose status patch never landed was
    therefore never removed — in the disable case while the condition reported
    "bastion disabled" and port 22 stayed open. Both now follow intent; the
    tag-based lookups make this safe.

Security

  • Changing allowedCIDRs never revoked the old rule: the rule set only ever
    grew, so narrowing the CIDR did not take access away from the previously
    allowed range. Rules are now reconciled in both directions.

Reconcile correctness

  • The bastion security group was attached a second time after CreateServer
    had already included it, failing with 404 (no port yet) or 400 ("Duplicate
    items in the list") and aborting EnsureBastion before the public IP was
    assigned. The attach is kept — CreateServer short-circuits on an existing
    server, making it the only path that repairs a detached group — but is now
    idempotent.
  • A CIDR listed twice produced two identical rules and a failing reconcile.
  • Deleting a cluster whose credentials Secret was already gone left it in
    Terminating forever. A missing Secret can never be recovered from (it
    commonly disappears first during namespace teardown), so deletion now
    finalizes and emits a CleanupSkipped warning; invalid — i.e. fixable —
    credentials still block as before.

Status consistency

  • Machine cloud failures left the legacy status.ready boolean true while the
    conditions said False.

Configuration

  • cluster-template-bastion.yaml hardcoded replicas: 3, ignoring
    WORKER_MACHINE_COUNT.

Testing

Every fix ships with a test at the cheapest level that can actually observe it:
cloud unit tests (httptest against the STACKIT API) for the client defects,
envtest specs for the controller defects.

Each test was verified by reverting its fix and confirming the test fails at
the documented assertion
— a test that passes for unrelated reasons proves
nothing. Coverage: cloud 39.1% → 56.3%, controller 70.2% → 71.1%.

The suite was also run end to end against real STACKIT infrastructure (cluster
lifecycle, providerID alignment, bastion), all green with no leaked resources.

Follow-ups not in this PR

  • The recreate guard returns a non-retryable error but nothing sets a terminal
    failure, so without a MachineHealthCheck the Machine is never replaced;
    status.instanceState and addresses also keep describing the deleted
    server.
  • allowedCIDRs are compared as strings, so a non-canonical prefix the API
    stores masked causes create/delete churn.
  • The credentials Secret is not watched, so correcting an invalid one never
    re-reconciles the cluster.
  • The StackitClusterMachine watch matches Machine.spec.clusterName
    against the StackitCluster name, which only holds when both share a name.

@Herbaert
Herbaert requested a review from tuunit August 14, 2026 07:46
@Herbaert Herbaert self-assigned this Aug 14, 2026
@Herbaert Herbaert added the bug Something isn't working label Aug 14, 2026
Herbaert added a commit that referenced this pull request Aug 14, 2026
…h fix, disable-teardown and credentials-Secret finalize as fixed, three new open items (recreate guard has no terminal state, CIDR string comparison, status.ready gaps)
@tuunit tuunit changed the title Fix/bastion and machine reconcile defects fix: bastion and machine reconcile defects Aug 14, 2026
Comment thread cloud/sdk_client_test.go Outdated
Comment thread controller/stackitcluster_bastion.go Outdated
Comment thread controller/stackitmachine_infrastructure.go Outdated
Comment thread controller/stackitmachine_infrastructure.go Outdated
Comment thread controller/controller_test_helpers_test.go Outdated
Comment thread controller/stackitcluster_infrastructure.go Outdated
Comment thread controller/stackitcluster_bastion.go Outdated
Comment thread controller/stackitcluster_infrastructure.go Outdated
@tuunit
tuunit merged commit 18cb596 into main Aug 18, 2026
2 checks passed
@tuunit
tuunit deleted the fix/bastion-and-machine-reconcile-defects branch August 18, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants