fix(seaweedfs): make naming audit fail closed on kubectl and payload errors - #3436
Conversation
…errors hack/seaweedfs-naming-audit.sh was fail-open: any kubectl failure produced an empty table indistinguishable from an honestly clean fleet (namespace LIST failure = zero namespaces walked, secret/pvc/sts LIST failures = zero findings, all under 2>/dev/null with no error handling). The runbook uses this output as the gate before PVC deletion, as post-deletion verification, and as an upgrade precondition, so a transient API error could green-light destroying data (#3431). Route every kubectl call through a run_kubectl helper: non-zero exit prints a FATAL line naming the failed query and propagates the code up the whole chain (enumerations restructured to capture-then-check, since an exit inside $(...) dies with the subshell). By-name GETs distinguish legitimate absence from real errors via --ignore-not-found on a separate existence check; a Secret that exists but has no decodable release payload, or one that decodes without a chart name, is corrupt state and fails loudly instead of silently dropping the tenant. Incomplete PV-age evidence now marks the generation incomplete and falls to the safe no-direction branch, so a failed GET can no longer flip OVERLAP into a wrong Step-3 deletion candidate. A successful run with zero findings still prints the same bytes as before. Tests: 14 new cozytest cases — failure injection for every LIST and by-name GET site, corrupt-payload variants, and full-output golden diffs for the success paths (verified byte-identical to the pre-change script). 25/25 green. Fixes #3431 Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe SeaweedFS naming audit now fails closed on kubectl and Helm-state errors, distinguishes absent resources from corrupt data, propagates namespace and resource query failures, and avoids MIXED deletion candidates when PV-age evidence is incomplete. Bats tests cover failure, fallback, skip, and byte-stable success paths. ChangesSeaweedFS audit hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Operator
participant SeaweedfsAudit
participant Kubectl
participant KubernetesAPI
Operator->>SeaweedfsAudit: run audit
SeaweedfsAudit->>Kubectl: query namespaces, releases, PVCs, StatefulSets, and PVs
Kubectl->>KubernetesAPI: execute read-only requests
KubernetesAPI-->>Kubectl: results or errors
Kubectl-->>SeaweedfsAudit: query output or failure
SeaweedfsAudit-->>Operator: audit table or FATAL exit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
hack/seaweedfs-naming-audit.sh (1)
196-199: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winChart-name extraction depends on exact key adjacency; consider a more tolerant match before making it fatal.
s/.*"chart":{"metadata":{"name":"..."/requireschart→metadata→nameto be immediately adjacent with no intervening keys or whitespace. That holds for today's compact Helm payloads (Metadata.Nameis the first field), but it's now the difference between a normal audit and a hardaudit_fatalabort. A field-order change upstream would turn every SeaweedFS tenant into a fatal "corrupt Helm release".Consider narrowing to the chart object and then matching
nameindependently of position, or preferringjqwhen present.♻️ More tolerant extraction sketch
- _sr_chart=$(printf '%s' "$_sr_json" | sed -n 's/.*"chart":{"metadata":{"name":"\([^"]*\)".*/\1/p' | head -1) + _sr_chart=$(printf '%s' "$_sr_json" \ + | sed -n 's/.*"chart"[[:space:]]*:[[:space:]]*{[[:space:]]*"metadata"[[:space:]]*:[[:space:]]*{\(.*\)/\1/p' \ + | sed -n 's/.*"name"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' | head -1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/seaweedfs-naming-audit.sh` around lines 196 - 199, Update the _sr_chart extraction in the Helm release audit to tolerate whitespace, intervening fields, or reordered chart metadata instead of requiring exact key adjacency. Prefer the existing jq-based parsing when available, or otherwise narrow parsing to the chart object and extract its name independently; retain the audit_fatal path only when no chart name can actually be decoded.hack/seaweedfs-naming-audit.bats (2)
262-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the fake's catch-all fail instead of
exit 0.The fallthrough (and the
get pvc/get pvunmatched-case fallthroughs at Lines 250 and 263) returns success with empty stdout — precisely the fail-open shape these tests exist to detect. If a future change adds a new kubectl query, the golden tests would keep passing against an unmodelled empty response.♻️ Fail on unmodelled invocations
-exit 0 +echo "fake kubectl: unmodelled invocation: $args" >&2 +exit 97 FAKENote the
get pvc/get pvinnercaseblocks need the same treatment to be effective.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/seaweedfs-naming-audit.bats` around lines 262 - 266, Update the fake kubectl command in the test fixture so every unmodelled invocation fails instead of returning success with empty output. Change the top-level fallthrough and the unmatched inner cases for “get pvc” and “get pv” to return a nonzero status, while preserving the existing successful responses for modeled queries.
488-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp dir leaks when the golden diff fails.
Every other test in this section removes
$dbefore asserting; these two remove it afterdiff, so a failing golden run leaves the directory behind on each retry. If keeping the artifacts on failure is intentional, atrap 'rm -rf "$d"' EXITat creation gives both cleanup and thegot/wantecho already present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/seaweedfs-naming-audit.bats` around lines 488 - 493, Update the test cleanup flow around the temporary directory variable $d so cleanup is registered at creation time, such as via an EXIT trap, ensuring $d is removed even when diff fails while preserving the existing got/want diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@hack/seaweedfs-naming-audit.bats`:
- Around line 262-266: Update the fake kubectl command in the test fixture so
every unmodelled invocation fails instead of returning success with empty
output. Change the top-level fallthrough and the unmatched inner cases for “get
pvc” and “get pv” to return a nonzero status, while preserving the existing
successful responses for modeled queries.
- Around line 488-493: Update the test cleanup flow around the temporary
directory variable $d so cleanup is registered at creation time, such as via an
EXIT trap, ensuring $d is removed even when diff fails while preserving the
existing got/want diagnostics.
In `@hack/seaweedfs-naming-audit.sh`:
- Around line 196-199: Update the _sr_chart extraction in the Helm release audit
to tolerate whitespace, intervening fields, or reordered chart metadata instead
of requiring exact key adjacency. Prefer the existing jq-based parsing when
available, or otherwise narrow parsing to the chart object and extract its name
independently; retain the audit_fatal path only when no chart name can actually
be decoded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e51fa33-c3d9-4c24-abd4-499ebbf75518
📒 Files selected for processing (2)
hack/seaweedfs-naming-audit.batshack/seaweedfs-naming-audit.sh
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM — no blockers.
Confirmed the old version was fail-open (2>/dev/null on every kubectl) and the fix targets the real root cause. Exercised all fail-closed paths under a hermetic fake kubectl: kubectl failure / empty / non-JSON / corrupt Helm secret all produce a non-zero exit + FATAL, never a clean empty table with exit 0.
Non-blocking notes:
- hack/seaweedfs-naming-audit.sh:196-201 — the
system_releasesloop parses only the first retained revision andbreaks (inherited from the old code); on an empty payload from a concurrent-prune race the release silently drops from the report.continueon empty would be more robust. - hack/seaweedfs-naming-audit.sh:363-371 — in whole-cluster mode the table header and already-processed rows reach stdout before the FATAL on stderr, so a consumer parsing only stdout could see a partial table. Consumers must gate on the exit code, not table contents (by design, documented at :47-54).
- :197 — chart-name extraction via sed assumes compact (space-free) JSON; holds for Helm's json.Marshal payloads.
…odelled calls Review findings from #3436, plus what an adversarial re-check of the first attempt at them turned up. The chart-name extraction was a greedy sed, i.e. LAST match. Helm marshals "config" (the release`s values) after "chart", so a values subtree that spells chart.metadata.name shadowed the real chart name: the release then read as non-SeaweedFS, the tenant silently vanished from the report, and the script exited 0 -- the precise false clean this PR exists to prevent, reachable without any corruption at all. It now takes the FIRST match. The path stays adjacent on purpose: a looser "any name after metadata" matches chart.templates[].name, which Helm emits immediately after metadata on every healthy release, so the review`s suggested relaxation would have returned a template path for every tenant. Over-strictness fails loudly and recoverably; over-looseness deletes data quietly. Newlines are now folded before matching, so a pretty-printed payload parses at all rather than aborting, and whitespace around the punctuation is tolerated. first_deployed got the same treatment -- it had the identical greedy, single- line-only shape, and a spaced payload silently dropped the PV-vintage row from the report. The FATAL message names the path it read and the shape it expected, so a future Helm format change is diagnosable instead of looking like real corruption. Tests: three new payload shapes (spaced, pretty multi-line, and a values decoy) each byte-compared against the same golden as the compact payload, so the shape must make no difference to the report. Mutation-checked -- restoring last-match extraction fails the decoy test on its own, restoring the single-line matcher fails the pretty test. The test fake answered any unmodelled kubectl invocation with exit 0 and empty stdout, the fail-open shape this script was rewritten to reject, and enough to let a newly added query pass the goldens unnoticed. Unmodelled calls now exit 97 naming the invocation. Verified inert first by instrumenting the fake: 88 invocations across the suite, none unmodelled. Also added the missing test for the by-name PVC GET fatal path, whose FAIL mode existed with nothing behind it. The runbook now tells the operator to read the exit code: a non-zero exit means the table is incomplete and no step may be taken on it. That contract lived only in the script header, while the runbook is what the operator follows. 29/29 tests green; sh -n clean. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Successfully created backport PR for |
|
Successfully created backport PR for |
v1.6.0's own upgrade notes tell operators to run hack/seaweedfs-naming-audit.sh, and the copy that shipped in v1.6.0 silences every kubectl call with 2>/dev/null. A timeout or an RBAC denial therefore prints an empty "all clean" table indistinguishable from a genuinely clean fleet, and that output gates a runbook step that deletes PVCs. Anyone following those notes today can still get a false clean. Verified per tag: v1.6.0 has 11 blanket redirections and no run_kubectl wrapper; v1.6.1, v1.6.2 and v1.5.4 have 4 and 18 respectively, the fail-closed shape from #3436 and its backport #3474. Published changelogs are historical records and are not normally edited. This one is an exception on purpose, because the instruction it carries is still live and still wrong. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…udit note (#3910) Adds the changelog for `v1.5.4`, and corrects one still-live instruction in v1.6.0's. The generated changelog is a good inventory and every one of its bullets is kept. What it had no layer for is the operator: no required-actions section, no runnable pre-upgrade checks, and the SeaweedFS 4.31 rename left as three isolated fix bullets that never say what to do about it. That is what is added on top, in the shape v1.6.0's changelog uses. Base widened to `v1.5.2`. v1.5.3 was tagged but its release was never published and its changelog PR was closed unmerged, so `docs/changelogs/v1.5.3.md` exists on no ref and a v1.5.3 base leaves #3212 (persistent EFI/TPM state) and #3194 (filer postgres2 connection pool) documented nowhere at all. Coverage is 82/82 commits in range. Three claims corrected. The slot 45 divergence is a skip rather than an ordering problem: v1.5.4 is the first 1.5.x release stamped `targetVersion: 46`, and `run-migrations.sh` loops `seq CURRENT (TARGET - 1)`, so a cluster that reaches 46 runs slots 46 through 53 on the way to 1.6 and never executes 1.6's own slot 45. Its chart-side half was missing too and is not redundant, because a fresh v1.5.4 install is stamped 46 having never run any slot, so only the chart can reach that population. And the S3 checksum bullet named a barman-cloud plugin path that does not exist on `release-1.5`. The audit re-run warning names v1.6.0 rather than an earlier release, because the script does not exist at v1.5.2 at all. v1.5.4 is the first 1.5.x release to carry it, and it carries the fail-closed version. The second commit edits `docs/changelogs/v1.6.0.md`, which is deliberate rather than an accident of scope. Published changelogs are historical records and are normally left alone, but v1.6.0's upgrade notes still tell operators to run `hack/seaweedfs-naming-audit.sh`, and the copy that shipped in v1.6.0 silences every `kubectl` call with `2>/dev/null`. A timeout or an RBAC denial therefore prints an empty all-clean table indistinguishable from a genuinely clean fleet, and that output gates a runbook step that deletes PVCs. Verified per tag: v1.6.0 carries 11 blanket redirections and no `run_kubectl` wrapper, while v1.6.1, v1.6.2 and v1.5.4 carry 4 and 18, the fail-closed shape from #3436 and its backport #3474.
What this PR does
Fixes #3431.
hack/seaweedfs-naming-audit.shwas fail-open: any kubectl failure produced an empty table indistinguishable from an honestly clean fleet — a failed namespace LIST meant zero namespaces walked, failed secret/PVC/STS LISTs meant zero findings, all silenced by2>/dev/nullwith no error handling anywhere in the script. The runbook uses this output as the gate before PVC deletion, as post-deletion verification, and as an upgrade precondition, so a transient API error could green-light destroying data.Every kubectl call now routes through a
run_kubectlhelper: a non-zero exit prints a FATAL line naming the failed query and the code propagates up the whole chain (enumerations restructured to capture-then-check, since anexitinside$(...)dies with the subshell). By-name GETs separate existence from extraction: a--ignore-not-found -o nameprobe keeps legitimate absence (Helm-pruned revision, unbound PVC) a clean skip, while everything else fails loudly — including a release Secret that exists but has no decodable payload or decodes without a chart name, which previously dropped the whole tenant from the audit. Incomplete PV-age evidence now marks the generation incomplete and falls to the safe "direction cannot be established" branch, so a failed GET can no longer flip an OVERLAP verdict into a wrong Step-3 deletion candidate. A successful audit prints the same bytes as before.Tests: 14 new cozytest cases — failure injection for every LIST and by-name GET site, corrupt-payload variants (missing
.data.release, undecodable base64/gzip, chartless{}), and full-output golden diffs proving the success paths stay byte-identical to the pre-change script. 25/25 green viahack/cozytest.sh.Release note
Summary by CodeRabbit
Bug Fixes
Tests
Documentation