Skip to content

Commit 241df22

Browse files
committed
Correct debug/ file:line references to match the main-synced code
Several current-state pointers had drifted from the code they describe (ensureBastionSecurityGroupRules, bastionNeedsRecreate, ensureServer, the deletion-bug.md watch-wiring fix plan, and a couple of test/helper line refs) after independent commits shifted lines relative to main. Historical "as originally found" snapshots (the dated main/refactor comparison tables, and run-*.md protocols) are left untouched per this folder's own convention. Also records this sync in SUMMARY.md and re-confirms items 1, 2 and 8-10 are still unimplemented in main, not just carried over from memory.
1 parent 7467825 commit 241df22

2 files changed

Lines changed: 30 additions & 15 deletions

File tree

debug/SUMMARY.md

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,9 @@ found.** Every known defect reproduces from the same, relocated code:
4444
| Known defect | Location on `refactor` | Reproduced? |
4545
| --- | --- | --- |
4646
| Security group attached twice in `EnsureBastion` | `cloud/sdk_client.go:263` | ✅ yes — same transient 404 `BastionError` |
47-
| `allowedCIDRs` rules never removed | `cloud/sdk_client.go:692` | not exercised (needs a CIDR change) |
48-
| `bastionNeedsRecreate` only watches cloud-init | `controller/stackitcluster_bastion.go:136` | not exercised |
49-
| `ensureServer` recreates unconditionally | `controller/stackitmachine_infrastructure.go:205` | ✅ yes — see below |
47+
| `allowedCIDRs` rules never removed | `cloud/sdk_client.go:698` | not exercised (needs a CIDR change) |
48+
| `bastionNeedsRecreate` only watches cloud-init | `controller/stackitcluster_bastion.go:183` | not exercised |
49+
| `ensureServer` recreates unconditionally | `controller/stackitmachine_infrastructure.go:212` | ✅ yes — see below |
5050
| Template hardcodes `replicas: 3` | `templates/cluster-template-bastion.yaml:160` | ✅ yes |
5151
| Stuck deletion on simultaneous delete | `controller/stackitmachine_controller.go:87-94` | ✖️ did not trigger — but code path unchanged, so **not fixed** |
5252

@@ -173,6 +173,13 @@ the CIDR test.
173173
duplicate security-group attach, `allowedCIDRs` never revoked, and the
174174
unconditional server recreate. `cloud` coverage 39.1 % → 55.5 %,
175175
`controller` 70.2 % → 71.0 %.
176+
- **2026-08-19:** this branch had independently re-implemented PR #4's fixes
177+
(same behaviour, different code) instead of merging it; the 14 affected
178+
source files were replaced 1:1 with `main`'s actual merged version, and
179+
every `file:line` reference in this folder that pointed at current (not
180+
historical pre-fix) code was re-checked against the synced code and
181+
corrected where it had drifted. Items 8–10 below, and items 1–2, remain
182+
unimplemented in `main` as of this sync — re-verified, not just assumed.
176183

177184
### Open
178185

debug/deletion-bug.md

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,12 @@ prematurely); only once no `Machine` remains does
483483
`StackitCluster.reconcileDelete` clean up load balancer/bastion and remove
484484
its own finalizer.
485485

486-
### Code changes — `internal/controller/stackitcluster_controller.go`
486+
### Code changes — `controller/stackitcluster_infrastructure.go` /
487+
`controller/stackitcluster_controller.go`
488+
489+
`reconcileDelete` lives in `stackitcluster_infrastructure.go`; the new watch
490+
wiring (`stackitClusterRequestsForMachine`, `SetupWithManager`, RBAC marker)
491+
belongs in `stackitcluster_controller.go` as shown further below.
487492

488493
**New helper `listClusterMachines`** — filters server-side via the
489494
`clusterv1.ClusterNameLabel` (`cluster.x-k8s.io/cluster-name`) that CAPI
@@ -510,14 +515,14 @@ network/serialization overhead when many clusters share a namespace.
510515

511516
*Why a label selector instead of a field indexer:* a field indexer on
512517
`spec.clusterName` would work but requires a cache-backed client. The
513-
existing test suite client (`internal/controller/suite_test.go:90`) is a
518+
existing test suite client (`controller/suite_test.go:90`) is a
514519
direct, non-cached API-server client (`client.New(cfg, ...)`) with no
515520
manager or cache. Switching would affect all existing tests in
516521
`stackitcluster_controller_test.go` / `stackitmachine_controller_test.go`
517522
and risk cache-sync flakiness. `client.MatchingLabels` works identically
518523
against envtest and a real cluster with no `suite_test.go` changes, and the
519524
label is set by CAPI itself on every `Machine` (the test helper
520-
`createOwnerMachine` in `controller_test_helpers_test.go:94-101` sets it
525+
`createOwnerMachine` in `controller_test_helpers_test.go:90-101` sets it
521526
too).
522527

523528
**Guard at the start of `reconcileDelete`:**
@@ -565,15 +570,18 @@ func (r *StackitClusterReconciler) stackitClusterRequestsForMachine(ctx context.
565570
}
566571
```
567572

568-
Uses the existing `isStackitClusterRef` (`stackitcluster_controller.go:588-592`).
573+
Uses the same `InfrastructureRef` `Kind`/`APIGroup`/`Name` check already inlined
574+
in `stackitClusterRequestsForCluster` (`stackitcluster_controller.go:107-108`) —
575+
there is no standalone `isStackitClusterRef` helper today, so implementing this
576+
fix means extracting that check into a shared function (or duplicating it).
569577

570-
**Register the watch** in `SetupWithManager` (`stackitcluster_controller.go:595-603`):
578+
**Register the watch** in `SetupWithManager` (`stackitcluster_controller.go:153-161`):
571579

572580
```go
573581
Watches(&clusterv1.Machine{}, handler.EnqueueRequestsFromMapFunc(r.stackitClusterRequestsForMachine)).
574582
```
575583

576-
**Add the RBAC marker** above `Reconcile` (`stackitcluster_controller.go:68-73`)
584+
**Add the RBAC marker** above `Reconcile` (`stackitcluster_controller.go:53-59`)
577585
— permission to list `Machine`s is currently missing:
578586

579587
```go
@@ -582,31 +590,31 @@ Watches(&clusterv1.Machine{}, handler.EnqueueRequestsFromMapFunc(r.stackitCluste
582590

583591
Then run `make manifests` so `config/rbac/role.yaml` picks up the new rule.
584592

585-
### Tests — `internal/controller/stackitcluster_controller_test.go`
593+
### Tests — `controller/stackitcluster_controller_test.go`
586594

587595
1. **"keeps the finalizer while Machines still exist for the cluster"**
588596
setup as in the existing `"deletes the provider-managed load balancer and
589-
removes the finalizer"` (line 423-442), but create a `Machine` for the
597+
removes the finalizer"` (line 562-580), but create a `Machine` for the
590598
same `clusterName` via `createOwnerMachine` before the `Delete`. After
591599
`k8sClient.Delete` + `reconciler.Reconcile`, expect `err == nil`, the
592600
finalizer still set, and `fakeCloud.LoadBalancerCount() == 1` (LB **not**
593601
cleaned up yet).
594-
2. **Regression:** the existing test at line 423-442 must keep passing
602+
2. **Regression:** the existing test at line 562-580 must keep passing
595603
unchanged (no `Machine` exists for the cluster there).
596604
3. **"removes the finalizer once the last Machine is gone"** — like test 1,
597605
but after the first reconcile delete the `Machine` and reconcile again;
598606
expect the finalizer removed, LB cleaned up, `StackitCluster` gone
599-
(analogous to the `Eventually(...IsNotFound...)` at line 438-441).
607+
(analogous to the `Eventually(...IsNotFound...)` at line 577-579).
600608
4. **"maps Machine events to StackitCluster reconcile requests"**
601609
analogous to the existing `"maps owning Cluster events..."` (line
602-
462-468): create a `Machine` with `spec.clusterName = clusterName`, call
610+
601-607): create a `Machine` with `spec.clusterName = clusterName`, call
603611
`stackitClusterRequestsForMachine`, expect `[]reconcile.Request{request}`;
604612
plus a case where a different `clusterName` returns `nil`.
605613

606614
### Verification
607615

608616
1. `go build ./...` — compiles without new errors.
609-
2. `go test ./internal/controller/...` — new and existing tests pass, in
617+
2. `go test ./controller/...` — new and existing tests pass, in
610618
particular the four above plus all existing deletion tests in
611619
`stackitmachine_controller_test.go`.
612620
3. `golangci-lint run` — no new findings.

0 commit comments

Comments
 (0)