✨ feat(lab): allowlist lab substrates so Kind-only scenarios run on Talos (LAB-DEKIND) - #301
Merged
Merged
Conversation
…alos
The lab harness gated on `[[ "${ctx}" == kind-* ]]` and refused everything else,
which left P-06 (pprof under load) and U-08 (webhook rejection) unrunnable on the
bare-metal Talos lab. Widening that check — or defaulting `--allow-non-kind` — would
have turned a safety gate into a hole: these scripts port-forward, install Helm
values and apply load, and a maintainer's ambient context is routinely a production
cluster.
Replace it with a default-deny ALLOWLIST in hack/lab/substrates.conf, enforced by
lib/substrate.sh: `kind-*` and the kumulus lab (`kumulus-lab`, cluster `kumulus`)
are permitted, everything else is refused with exit 2. Patterns are validated
wherever they come from — the checked-in file, KOLLECT_LAB_SUBSTRATES_FILE, or
KOLLECT_LAB_ALLOWED_CONTEXTS — so `*`, `*-prod` and `k*` fail the load closed
instead of admitting every cluster. An entry's expected cluster name can only
refuse a mismatch, never admit. `--allow-non-kind` survives as an explicit
maintainer override but is no longer the mechanism by which the lab works.
Substrate also decides image delivery. `kind load docker-image` has no Talos
equivalent, so a non-Kind substrate must name a pinned registry reference; a
local-only or mutable tag (`:dev`, `:latest`, untagged) is refused before helm is
called rather than silently reusing whatever the nodes cached — the stale-image
trap that blocked U-02's re-test.
For U-08, `kollect_e2e_select_context` separates "create a kind cluster" from "run
the assertions": with KOLLECT_E2E_EXISTING_CLUSTER=1 the webhook scenario asserts
against the current allowlisted context using server-side dry runs only, skips the
cert-manager Certificate wait when the release does not use one, and reports a
missing webhook configuration as a precondition (exit 4) instead of a 300s timeout.
Without that env var the Kind/CI path is unchanged.
perf-kind.sh gains --release so the port-forward targets
deploy/<release>-controller-manager (the lab runs kollect-op1, not kollect), and
perf-kind:quick becomes an alias for the substrate-neutral perf-lab:quick.
hack/test/lab_*_meta_test.sh was never wired into a workflow, so the offline lab harness suite only ever ran on a maintainer's laptop. That suite now contains the regression guard for the substrate allowlist (a production-lookalike context must stay refused) and for the non-Kind image-delivery policy — a guard nobody runs is not a guard. Add it to the lint job, and add a webhook-existing-cluster meta job to E2E extended that proves both halves of U-08: the Kind path still switches context and applies for real, and the existing-cluster path refuses an off-allowlist context without touching it.
…pansion)
`for item in ${extra//,/ }` is subject to PATHNAME expansion, not just word
splitting. Run from a directory containing files named after refused contexts,
`KOLLECT_LAB_ALLOWED_CONTEXTS='*'` was rewritten into those filenames BEFORE
lab_substrate_valid_pattern ever saw them — the allowlist loaded rc=0 with
`gke-prod-example` and `kumulus-lab-prod` admitted, and perf-kind.sh walked
straight past the gate. Two documented guarantees were false: "there is no
'allow everything' value" and "a wildcard-only pattern fails the load closed".
I audited the glob on the matching side (the SC2053 suppression, which is
bounded and correct) and missed the one on the input side.
Split with `read -a`, which never globs, and iterate the array quoted. The
parser additionally runs under `set -f`, so no future edit inside it can turn a
pattern into a list of filenames; the two layers are independent and both tested.
The regression test was worthless: it ran in the caller's CWD, so creating a
three-character directory flipped it from red to green, and it passed in CI only
because api/, bin/ and ui/ are shorter than the validator's 4-character minimum.
It now runs from a decoy directory seeded with exactly the names that must stay
refused (an EMPTY dir would hide the bug — an unmatched glob stays literal and is
correctly rejected), asserts the REASON of each refusal rather than only its exit
code, and calls the inner parser directly with globbing on so the noglob wrapper
cannot mask a missing quote. Verified both ways: reintroducing the unquoted loop
turns the suite red and names the leaked entry.
Two smaller inconsistencies in the same file:
- Digest references were accepted, and the remediation hint recommended them,
while kollect_helm_install rejects any `@` because the chart renders
repository:tag. Advice that cannot work is worse than no advice: refuse
digests where the operator can act on it, and stop suggesting them.
- lab_substrate_allowlist_summary swallowed a failed load and printed the
partially-parsed list, which appears inside refusal messages. A load that
failed closed admits nothing; say that instead.
…own clusters perf-kind.sh still referenced LAB_PERF_KIND_PF_RESOURCE on the port-forward failure branch after the rename to PF_RESOURCE, so `set -u` aborted the script with exit 1 instead of the documented exit 3 — and lab_pprof_write_findings_blocked never ran, meaning the BLOCKED findings register designed to prevent a silent stale result was the one thing a failed run did not produce. `shellcheck --severity=warning` cannot catch this (an all-caps unset name is assumed environment-supplied), so the gap was a missing test, not a missing linter: the port-forward-start failure path is now exercised end to end and asserts both the exit code and the register contents. While in that path: the BLOCKED remediation text hardcoded `kollect-system` and `deploy/kollect-controller-manager`, so a run that passed --release/--namespace told the operator to look in the wrong place. It now reports the target that was actually used. Cleanup runs `kubectl delete all,cm,secret,sa,role,rolebinding -A -l kollect.dev/lab-run=<id>` — a cluster-wide write. It was reachable through --allow-non-kind, so an override meant for profiling an unusual substrate also authorized deleting across every namespace of a cluster the allowlist does not recognise. Gate it on the allowlist instead: off-allowlist runs skip cleanup and print the label to remove by hand.
KOLLECT_E2E_TEST_NAMESPACE was interpolated into a heredoc that is piped straight to the API server, so an operator-supplied value could carry YAML into the manifest. Restore the quoted heredoc, pass the namespace via `kubectl -n`, and validate it as a DNS-1123 label before anything is built — with a test that an injection-shaped value is refused before any apply. Also correct the CI comment added with this suite: the lab harness meta-tests were not completely unwired — hack/docs/verify.sh runs them — but that workflow is path-filtered and hack/lab/** is not in the filter, so a change to the harness itself never triggered them.
Gating cleanup on the substrate allowlist could have disarmed Ctrl-C cleanup on a legitimate live lab run. --simulate-interrupt is dry-run only, so nothing covered the live path: lab_perf_kind_on_interrupt with DRY_RUN=0 and an allowlisted context must still issue the label-scoped delete. It does; now it stays that way.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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
1 participant
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.
LAB-DEKIND
Two perftest scenarios were NOT RUN purely because the harness was hardcoded to Kind: P-06 (pprof under load) and U-08 (webhook rejection).
hack/lab/perf-kind.shgated on[[ "$ctx" == kind-* ]]and exited 2 otherwise, with--allow-non-kindas the only escape.The safety-critical design choice
"Make it not Kind-only" was not implemented by widening the
--allow-non-kindbypass. The operator's ambient kubectl context is a production GKE cluster, and this script port-forwards, installs Helm values, applies load, and (on cleanup) issues a cluster-wide labelled delete. Widening the bypass would have pointed all of that at prod.Instead: a default-deny allowlist (
hack/lab/substrates.conf—kind-*andkumulus-lab). Unknown contexts are still refused with exit 2.--allow-non-kindsurvives as a maintainer override but is explicitly not how the lab works, and it no longer unlocks the cluster-wide cleanup delete.A P0 fail-open, found in review and closed
The first implementation leaked:
for item in ${extra//,/ }is subject to pathname expansion, soKOLLECT_LAB_ALLOWED_CONTEXTS='*'silently became a CWD-derived allowlist. From a directory containing same-named files it admittedgke-prod-exampleandkumulus-lab-prod— contexts the suite asserts must be refused.Worse, the regression test flipped red/green depending on the caller's CWD; it passed in CI only because
api/,bin/,ui/happen to be shorter than the validator's minimum.Fixed in two independent layers (
IFS=', ' read -r -a—readnever globs — plus the parser running underset -f), and the guard was rebuilt to catch it: the suite now runs from a decoy CWD seeded with the names that must stay refused, and asserts the reason for refusal, not just the exit code. Deliberately not an empty directory — an empty CWD hides the bug, since an unmatched glob stays literal and is rejected for the wrong reason.The reviewer re-ran the full bypass matrix against this head and re-introduced the bug four ways — including renaming the variable to dodge the structural check — and the suite goes red every time.
Also closed
LAB_PERF_KIND_PF_RESOURCEmade the script abort with exit 1 and never write the BLOCKED findings file. ShellCheck structurally cannot catch this class, so it now has a behavioural test.:dev/:latest/untagged are refused. A stale side-loaded image is exactly why U-02 was unretestable.ci.yaml; previously no workflow reached it.Scope
P-06 and U-08 are unblocked at harness level. Both remain blocked live (the lab release serves no pprof; webhooks are disabled, no cert-manager) and the harness exits 3/4 with a precondition message rather than fabricating profiles — verified: zero
.pb.gzwritten. Mutating e2e scenarios stay Kind-only by design and are documented as such.