fix: bastion and machine reconcile defects - #4
Open
Herbaert wants to merge 10 commits into
Open
Conversation
… and deduplicate repeated allowedCIDRs, each with a regression test
…th an envtest regression test
… repaired instead of aborting bastion reconciliation
… instead of hanging in Terminating
…leave it running while reporting itself disabled
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
requested changes
Aug 14, 2026
| 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."}`)) |
Collaborator
There was a problem hiding this comment.
Couldn't we use writeJSON for this instead?
Comment on lines
+58
to
+60
| sweep := hasBastionStatus(sc.Status.Bastion) || | ||
| meta.FindStatusCondition(sc.Status.Conditions, infrav1.ClusterBastionReadyCondition) == nil | ||
| if sweep { |
Collaborator
There was a problem hiding this comment.
Suggested change
| 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 { |
Comment on lines
+76
to
+77
| // 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. |
Collaborator
There was a problem hiding this comment.
Why does this say "legacy" boolean?
| 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 |
Collaborator
There was a problem hiding this comment.
and by the whay the machine scope has a SetNotReady method
…written JSON body
…signing Status.Ready directly
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
maintoo.What was broken
Silent resource leaks
ensureServerrecreated a server for machines that had already bootstrappedand joined. The replacement replays the original bootstrap data, so it either
never rejoins (different IP) or rejoins while
MachineandNodekeeppointing 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.
enabled: false, wereboth 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
allowedCIDRsnever revoked the old rule: the rule set only evergrew, so narrowing the CIDR did not take access away from the previously
allowed range. Rules are now reconciled in both directions.
Reconcile correctness
CreateServerhad already included it, failing with 404 (no port yet) or 400 ("Duplicate
items in the list") and aborting
EnsureBastionbefore the public IP wasassigned. The attach is kept —
CreateServershort-circuits on an existingserver, making it the only path that repairs a detached group — but is now
idempotent.
Terminatingforever. A missing Secret can never be recovered from (itcommonly disappears first during namespace teardown), so deletion now
finalizes and emits a
CleanupSkippedwarning; invalid — i.e. fixable —credentials still block as before.
Status consistency
status.readybooleantruewhile theconditions said
False.Configuration
cluster-template-bastion.yamlhardcodedreplicas: 3, ignoringWORKER_MACHINE_COUNT.Testing
Every fix ships with a test at the cheapest level that can actually observe it:
cloudunit 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:
cloud39.1% → 56.3%,controller70.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
failure, so without a
MachineHealthCheckthe Machine is never replaced;status.instanceStateandaddressesalso keep describing the deletedserver.
allowedCIDRsare compared as strings, so a non-canonical prefix the APIstores masked causes create/delete churn.
re-reconciles the cluster.
StackitCluster→Machinewatch matchesMachine.spec.clusterNameagainst the
StackitClustername, which only holds when both share a name.