Skip to content

Commit 2920dd7

Browse files
HerbaertHerbaert
committed
add regression run protocols, and bug descriptions
1 parent bd32c5b commit 2920dd7

17 files changed

Lines changed: 3027 additions & 837 deletions

‎.gitignore‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,10 @@
44

55
# Local dev/debug artifacts
66
.DS_Store
7-
/cluster.yaml
7+
/cluster*.yaml
88
/kind-config.yaml
99
.devcontainer/devcontainer-lock.json
10+
.ssh/
1011

1112
# Binaries for programs and plugins
1213
*.exe

‎debug/SUMMARY.md‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
# Debug & regression notes — `main`
2+
3+
Entry point for this folder. Everything below happened against branch
4+
`debug/regression-test-main` (commit `bd32c5b`), which carries **no Go changes
5+
relative to `main`** — so all findings apply to `main`'s reconciler logic.
6+
7+
## Timeline
8+
9+
Read top to bottom; this is the order in which everything happened.
10+
11+
| # | When | Cluster | What | Documents |
12+
| --- | --- | --- | --- | --- |
13+
| **0** | 2026-07-31 … 08-03 | `stackit-workload` | Initial bug investigation, before any structured run | [deletion-bug.md](deletion-bug.md) |
14+
| **1** | 2026-08-05 | `stackit-workload` | **Run 1** — bootstrapping → HA → deletion | [run1-1-bootstrapping.md](run1-1-bootstrapping.md) · [run1-2-ha-controlplane.md](run1-2-ha-controlplane.md) · [run1-3-deletion.md](run1-3-deletion.md) |
15+
| **1e** | 2026-08-07 (earlier) | `stackit-capi-test` | **Run 1, extra** — bastion, on its own cluster | [run1-4-bastion.md](run1-4-bastion.md) |
16+
| **2** | 2026-08-07 (later) | `stackit-capi-test` | **Run 2** — same three packages, from scratch | [run2-1-bootstrapping.md](run2-1-bootstrapping.md) · [run2-2-ha-controlplane.md](run2-2-ha-controlplane.md) · [run2-3-deletion.md](run2-3-deletion.md) |
17+
| **2e** | 2026-08-07 (later) | `stackit-capi-test` | **Run 2, extra** — bastion | [run2-4-bastion.md](run2-4-bastion.md) |
18+
| **3** | 2026-08-10 | — | Code review of both runs; defects written up | [bastion-bug.md](bastion-bug.md) · [machine-recreate-bug.md](machine-recreate-bug.md) |
19+
20+
Run 1 and run 2 are **independent full runs**, not a plan and a report. Each
21+
`run*-*.md` is a complete protocol of its own run. Where the review later
22+
corrected a conclusion, the protocol is left as recorded and an
23+
`## Addendum` at the end points to the relevant bug document.
24+
25+
## Status matrix
26+
27+
| Topic | Run 1 (08-05/07) | Run 2 (08-07) |
28+
| --- | --- | --- |
29+
| Bootstrapping & networking | ✅ works — but cluster pre-existed, creation not evidenced | ✅ works — created from scratch, full evidence |
30+
| HA control-plane | ⚠️ partial — manual remediation needed | ⚠️ partial — same, **plus** silent VM recreate found |
31+
| Deletion & cleanup | ✅ works | ✅ works |
32+
| Bastion / jump host | ✅ works — full access path verified | ⚠️ blocked at SSH — but found a template bug |
33+
34+
Where the runs differ, both are right about what they saw: run 2 bootstrapped
35+
from zero and so evidences cluster creation that run 1 could not; run 1 got
36+
through the whole bastion path that run 2 could not reach. Neither document
37+
supersedes the other.
38+
39+
## Open bugs
40+
41+
| Document | Status | Summary |
42+
| --- | --- | --- |
43+
| [deletion-bug.md](deletion-bug.md) | ⚠️ open | Deleting `Cluster`+`StackitCluster`+`Machine`s simultaneously strands machines and orphans VMs. Root cause identified, fix specified, **not implemented**. Not reproduced in either run — the safe path (`kubectl delete cluster` alone) was used and works. |
44+
| [bastion-bug.md](bastion-bug.md) | ⚠️ open | Four code-confirmed defects: security group attached twice (causes both "transient" `BastionError`s and delays the public IP), `allowedCIDRs` rules never removed (**security-relevant**), `bastionNeedsRecreate` only watches cloud-init, and `cluster-template-bastion.yaml` hardcodes `replicas: 3`. |
45+
| [machine-recreate-bug.md](machine-recreate-bug.md) | ⚠️ open | `ensureServer()` recreates a missing server unconditionally, even for a machine that had already joined; the replacement can never rejoin and just consumes a VM + volume until an operator intervenes. |
46+
47+
**Not a code defect, but a gap:** no cluster template ships a
48+
`MachineHealthCheck`, so a node whose VM dies out-of-band is never
49+
automatically remediated at the Kubernetes level. Remediation is intentionally
50+
opt-in upstream — this is a decision to make, not a bug. Seen in both runs.
51+
52+
**Unresolved, environment-related:** bastion SSH failed in run 2 on 3 VMs
53+
across 3 IPs while working in run 1 from the same environment. Both runs
54+
together point at certain STACKIT public IP ranges being unreachable here,
55+
independent of port — see
56+
[bastion-bug.md](bastion-bug.md#open-not-a-code-defect-the-ssh-failures) for
57+
the evidence and a falsifiable test. This also retro-explains the outbound
58+
network failure that forced run 2's bootstrapping package to be restarted.
59+
60+
## What is confirmed working
61+
62+
Each claim notes which run evidences it.
63+
64+
- **Bootstrapping** (run 2, from scratch; run 1 consistent): cluster
65+
provisions, control-plane and worker nodes join and reach `Ready`, Cilium
66+
installs cleanly, deployments and pod scheduling work.
67+
- **Networking** (both runs): cross-node pod-to-pod communication works with
68+
0% packet loss; in-cluster DNS resolves service names.
69+
- **Worker scaling** (both runs): `MachineDeployment` scales 1↔2 with no
70+
leftover Machine/StackitMachine/Node objects. Which machine a scale-down
71+
removes is arbitrary — run 1 lost the original node, run 2 the newest.
72+
- **HA control-plane** (both runs): 3-node scale-up works one machine at a
73+
time; killing a control-plane VM does not take the API server down; after
74+
the dead `Machine` is deleted manually, the LB target is removed, no
75+
VM/volume leaks, KCP creates a replacement and a new leader is elected.
76+
- **Deletion** (both runs): `kubectl delete cluster` tears down machines in the
77+
right order (workers, then control-plane) before `StackitCluster` drops its
78+
finalizer; all VMs, volumes, load balancer, security groups and public IPs
79+
are removed with no leaks, in well under a minute. Run 2 confirmed this even
80+
after extra VMs and two forced bastion recreates.
81+
- **Bastion access path** (run 1 only): SSH to the bastion, jump to a workload
82+
node, kube-api through the tunnel, and a full remote CNI install; access
83+
from a network outside the allowed CIDR is refused.
84+
85+
## Shared prerequisites
86+
87+
Apply to every run document; package-specific extras are noted in each.
88+
89+
- Management cluster per
90+
[../docs/src/getting-started/management-cluster.md](../docs/src/getting-started/management-cluster.md).
91+
- STACKIT resources (network, security group, image, credentials secret) per
92+
[../docs/src/getting-started/cloud-resources.md](../docs/src/getting-started/cloud-resources.md)
93+
and [../docs/src/getting-started/credentials.md](../docs/src/getting-started/credentials.md).
94+
- `stackit` CLI authenticated **once per shell** — `.envrc` sets the key path
95+
but does not do this itself:
96+
```
97+
stackit auth activate-service-account --service-account-key-path "${STACKIT_SERVICE_ACCOUNT_KEY_PATH}"
98+
```
99+
- `.envrc` sourced **directly, never through a pipe** — `source .envrc | tail`
100+
runs it in a subshell and silently discards every export.
101+
- `CLUSTER_NAME` must be a lowercase RFC 1123 subdomain.
102+
- `.envrc` ships `STACKIT_SSH_KEY_NAME=""` — must be set explicitly before the
103+
bastion package.
104+
- Workload kubeconfig:
105+
```
106+
export KUBECONF_WORKERCLUSTER=/tmp/"${CLUSTER_NAME}".kubeconfig
107+
clusterctl get kubeconfig "${CLUSTER_NAME}" -n "${NAMESPACE}" > "${KUBECONF_WORKERCLUSTER}"
108+
```
109+
110+
**CLI papercuts** (so they are not rediscovered): `stackit key-pair create
111+
--public-key` needs an `@`-prefixed path; `STACKIT_BASTION_SSH_KEY_NAME="${STACKIT_SSH_KEY_NAME}"`
112+
is a snapshot, not a live reference — re-export both together after any change.
113+
114+
## Next steps
115+
116+
1. Decide whether the unconditional server recreate in `ensureServer()` is
117+
intended; guard it so an already-joined machine is not silently replaced
118+
([machine-recreate-bug.md](machine-recreate-bug.md)). Highest impact — it
119+
currently hides the moment an operator needs to step in.
120+
2. Remove the duplicate security-group attach in `EnsureBastion`
121+
([bastion-bug.md](bastion-bug.md#1-the-bastion-security-group-is-attached-twice)) —
122+
eliminates both recurring `BastionError`s and a reconcile of public-IP delay.
123+
3. Make `allowedCIDRs` reconcile in both directions
124+
([bastion-bug.md](bastion-bug.md#2-changing-allowedcidrs-never-revokes-the-old-access)) —
125+
security-relevant, and required before the CIDR-narrowing test is meaningful.
126+
4. Fix `replicas: 3` → `${WORKER_MACHINE_COUNT}` in
127+
`templates/cluster-template-bastion.yaml`.
128+
5. Land the deletion-order fix in
129+
[deletion-bug.md#fix-plan-not-yet-implemented](deletion-bug.md#fix-plan-not-yet-implemented);
130+
until then document `kubectl delete cluster` as the only supported teardown.
131+
6. Decide whether to ship a default `MachineHealthCheck`.
132+
7. Re-run the bastion access path from a network without the IP-range
133+
restriction found here, to re-verify run 1's result on current `main`.
134+
135+
## Convention for this folder
136+
137+
One flat Markdown document per topic, no subfolders.
138+
139+
- `run<N>-<M>-<topic>.md` — test-run protocols. `<N>` is the run, `<M>` the
140+
position within that run, so alphabetical order equals chronological order.
141+
A third run goes in as `run3-*`.
142+
- `*-bug.md` — investigations of a specific defect; they belong to no run.
143+
- `SUMMARY.md` — this file: timeline, status, open bugs, prerequisites.
144+
145+
Command blocks use a fenced block without a language tag containing `$ command`,
146+
a blank line, then the raw output, followed by a bolded `**Result:**` paragraph
147+
and a `---` separator.

‎debug/bastion-bug.md‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
# Bugs: bastion provisioning path
2+
3+
Date: 2026-08-10
4+
Source: code review of the bastion path after run 2 — see [SUMMARY.md](SUMMARY.md#timeline)
5+
Status: ⚠️ **open** — four defects confirmed in code, none implemented
6+
7+
Collects the defects found by reading the bastion code paths against the
8+
evidence from [run1-4-bastion.md](run1-4-bastion.md) and
9+
[run2-4-bastion.md](run2-4-bastion.md). All four are confirmed by reading the
10+
source, not inferred from behaviour alone.
11+
12+
**Why this document exists:** both bastion run protocols classify the two
13+
recurring `BastionError` messages as harmless, self-resolving STACKIT-API
14+
races and explicitly tell the reader not to worry about them. That
15+
classification is wrong — see bug 1. The protocols themselves are left as
16+
written (they record what was observed); the corrections live here.
17+
18+
## 1. The bastion security group is attached twice
19+
20+
**Location:** [../pkg/cloud/sdk_client.go](../pkg/cloud/sdk_client.go),
21+
`EnsureBastion`, lines 247-265.
22+
23+
```go
24+
server, err := c.CreateServer(ctx, CreateServerInput{
25+
...
26+
SecurityGroups: []string{securityGroup.ID}, // (1) already set here
27+
...
28+
})
29+
if err != nil {
30+
return nil, err
31+
}
32+
if err := c.addSecurityGroupToServer(ctx, server.ID, securityGroup.ID); err != nil { // (2) same SG again
33+
return nil, err
34+
}
35+
```
36+
37+
`CreateServer` writes the security group into the create payload
38+
(`payload.SetSecurityGroups(...)`, same file, lines 167-169). The call at (2)
39+
attaches the *same* group a second time. That produces exactly the two errors
40+
seen in both runs:
41+
42+
| Server state | Error observed | Cause |
43+
| --- | --- | --- |
44+
| still `CREATING` | `404 … could not be found as device id on any ports` | no network port exists yet, so (2) fails |
45+
| `ACTIVE` | `400 … Duplicate items in the list: 'f1d8050f-…'` | (1) already attached it, so (2) is a duplicate |
46+
47+
The `400 Duplicate items` error from
48+
[run2-4-bastion.md](run2-4-bastion.md) is direct proof that the group was
49+
already attached at that point — the call is genuinely redundant, not merely
50+
defensive.
51+
52+
`addSecurityGroupToServer` (same file, lines 802-812) only swallows
53+
`IsConflict`:
54+
55+
```go
56+
if !IsConflict(err) {
57+
return err // both 400 and 404 propagate
58+
}
59+
```
60+
61+
**Additional effect not noticed in either run:** when (2) fails,
62+
`EnsureBastion` returns immediately, so `ensurePublicIP()` and
63+
`AddPublicIpToServer()` (lines 267-284) never run in that reconcile. The
64+
bastion's public IP is therefore assigned a full reconcile cycle later than
65+
necessary — part of the delay complained about in
66+
[run2-4-bastion.md](run2-4-bastion.md) needs no external explanation.
67+
68+
**Fix:** drop the call at lines 263-265. If it is to be kept as a defensive
69+
re-attach for the `CreateServer`-found-existing-server path, it must also
70+
tolerate `IsNotFound` and the duplicate-`400`.
71+
72+
---
73+
74+
## 2. Changing `allowedCIDRs` never revokes the old access
75+
76+
**Location:** [../pkg/cloud/sdk_client.go](../pkg/cloud/sdk_client.go),
77+
`ensureBastionSecurityGroupRules`, lines 692-720.
78+
79+
```go
80+
for _, cidr := range cidrs {
81+
if hasSSHRule(existingRules, cidr) {
82+
continue
83+
}
84+
... CreateSecurityGroupRule(...) // additive only — nothing is ever removed
85+
}
86+
```
87+
88+
The function adds missing rules but never removes rules for CIDRs that are no
89+
longer in the spec. Changing `STACKIT_BASTION_ALLOWED_CIDRS` — because the
90+
egress IP rotated (a Zscaler-style proxy is called out as a risk in
91+
[run1-4-bastion.md](run1-4-bastion.md)), or deliberately to restrict access —
92+
leaves the **old** SSH rule in place permanently.
93+
94+
**Why this matters for the test results:** [run1-4-bastion.md](run1-4-bastion.md)
95+
step 5 certifies that the CIDR restriction works. That test was run from a
96+
mobile network that had never been in any rule, so it cannot detect this bug —
97+
it only proves "an IP that was never allowed is refused", not "a revoked IP is
98+
refused". The CIDR-narrowing variant proposed in
99+
[run2-4-bastion.md](run2-4-bastion.md) would, on current code, **falsely
100+
report success**: the old rule would still admit the tester.
101+
102+
**Fix:** reconcile the rule set in both directions — delete SSH rules whose
103+
`ipRange` is not in the desired CIDR list.
104+
105+
---
106+
107+
## 3. `bastionNeedsRecreate` only reacts to cloud-init changes
108+
109+
**Location:** [../internal/controller/stackitcluster_controller.go](../internal/controller/stackitcluster_controller.go),
110+
lines 470-476.
111+
112+
```go
113+
func bastionNeedsRecreate(sc *infrav1.StackitCluster, cloudInit []byte) bool {
114+
if !hasBastionStatus(sc.Status.Bastion) {
115+
return false
116+
}
117+
return sc.Status.Bastion.CloudInitHash != bastionCloudInitHash(cloudInit)
118+
}
119+
```
120+
121+
Changes to `sshKeyName`, `imageID`, `machineType` or `rootVolume` do **not**
122+
trigger a recreate. A running bastion with the wrong SSH key cannot be repaired
123+
by fixing the manifest — only by deleting the server or by touching the
124+
cloud-init ConfigMap.
125+
126+
**Relevance:** [run1-4-bastion.md](run1-4-bastion.md) step 2 hit a stale
127+
`STACKIT_BASTION_SSH_KEY_NAME` and fixed it by regenerating and re-applying.
128+
That worked only because the bastion had never been created (`keypair not
129+
found`). Had it come up with an existing-but-wrong key, re-applying would not
130+
have helped — a trap documented nowhere else.
131+
132+
**Fix:** include the relevant spec fields in the recreate decision, or document
133+
the limitation prominently.
134+
135+
---
136+
137+
## 4. `cluster-template-bastion.yaml` ignores `WORKER_MACHINE_COUNT`
138+
139+
**Location:** [../templates/cluster-template-bastion.yaml](../templates/cluster-template-bastion.yaml), line 160.
140+
141+
```
142+
$ grep -n replicas templates/cluster-template.yaml
143+
144+
49: replicas: ${CONTROL_PLANE_MACHINE_COUNT}
145+
123: replicas: ${WORKER_MACHINE_COUNT} # correctly parameterized
146+
147+
$ grep -n replicas templates/cluster-template-bastion.yaml
148+
149+
85: replicas: ${CONTROL_PLANE_MACHINE_COUNT}
150+
160: replicas: 3 # hardcoded
151+
```
152+
153+
Every bastion-enabled cluster gets 3 workers regardless of what is requested.
154+
Found and confirmed in [run2-4-bastion.md](run2-4-bastion.md); run 1 ran with
155+
4 nodes and did not recognise the discrepancy.
156+
157+
**Fix:** `replicas: ${WORKER_MACHINE_COUNT}`, matching the base template.
158+
159+
---
160+
161+
## Open, not a code defect: the SSH failures
162+
163+
The two runs give contradictory explanations for bastions refusing SSH
164+
(TCP connects, no banner):
165+
166+
- [run1-4-bastion.md](run1-4-bastion.md): "transient, tied to that specific
167+
public IP, unconfirmed" — 1 of 3 bastions affected.
168+
- [run2-4-bastion.md](run2-4-bastion.md): "environment blocks TCP/22 to STACKIT
169+
ranges" — 3 of 3 affected.
170+
171+
Neither holds up: run 2's explanation cannot be right because run 1 reached
172+
STACKIT bastions over SSH from the same environment; run 1's cannot be right
173+
because run 2 reproduced it across two different IPs.
174+
175+
Taking the IPs recorded in both documents together, a single consistent pattern
176+
emerges:
177+
178+
| IP | Run | Result |
179+
| --- | --- | --- |
180+
| `213.17.20.79` | 1 | ✅ SSH ok |
181+
| `213.17.23.100` | 1 | ✅ SSH ok |
182+
| `192.214.188.106` | 1 | ❌ no banner |
183+
| `188.34.73.218` | 2 | ❌ no banner (two different VMs) |
184+
| `192.214.181.43` | 2 | ❌ no banner |
185+
186+
`213.17.x.x` works, `192.214.x.x` and `188.34.x.x` do not — independently of
187+
the VM. The same split appears outside port 22: in run 2 the workload API was
188+
reachable on `213.17.23.218` and `213.17.23.143`, while the first bootstrapping
189+
attempt over `188.34.84.212` failed with the same kind of connection teardown
190+
(recorded as a transient outage in [SUMMARY.md](SUMMARY.md#timeline)).
191+
192+
That points to one shared cause — certain STACKIT public IP ranges are
193+
unreachable from this environment, regardless of port — rather than two
194+
unrelated ones. "TCP connect succeeds, zero bytes returned, immediate close" is
195+
the signature of a transparently intercepting proxy.
196+
197+
**Falsifiable test for the next run:** recreate the bastion until a
198+
`213.17.x.x` address is assigned, then retry SSH. If it succeeds, the cause is
199+
IP-range reachability and **not** port 22.

0 commit comments

Comments
 (0)