Skip to content

fix(seaweedfs): make naming audit fail closed on kubectl and payload errors - #3436

Merged
myasnikovdaniil merged 2 commits into
mainfrom
fix/seaweedfs-audit-fail-closed
Jul 28, 2026
Merged

fix(seaweedfs): make naming audit fail closed on kubectl and payload errors#3436
myasnikovdaniil merged 2 commits into
mainfrom
fix/seaweedfs-audit-fail-closed

Conversation

@myasnikovdaniil

@myasnikovdaniil myasnikovdaniil commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Fixes #3431.

hack/seaweedfs-naming-audit.sh was 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 by 2>/dev/null with 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_kubectl helper: 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 an exit inside $(...) dies with the subshell). By-name GETs separate existence from extraction: a --ignore-not-found -o name probe 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 via hack/cozytest.sh.

Release note

fix(seaweedfs): the naming-audit script fails loudly when a kubectl query or a Helm release payload read fails, instead of printing an empty table indistinguishable from a clean fleet

Summary by CodeRabbit

  • Bug Fixes

    • Audits now fail clearly (non-zero exit + fatal messaging) when Kubernetes queries or Helm release data is unavailable or corrupt, instead of producing incomplete results.
    • Missing release revisions are skipped safely; missing/unreadable PV age evidence no longer leads to incorrect deletion candidates, and MIXED now reports conservative “direction cannot be established” when evidence is incomplete.
    • Non-SeaweedFS releases remain silently ignored.
  • Tests

    • Added end-to-end acceptance coverage for fail-closed behavior and byte-for-byte stable output across key naming scenarios.
  • Documentation

    • Updated the runbook to require relying on the audit script exit code before proceeding, with a dedicated MIXED re-check step.

…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>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: cc15f113-0911-4288-bc6b-22f567444255

📥 Commits

Reviewing files that changed from the base of the PR and between 3dc6cbd and e02adad.

📒 Files selected for processing (3)
  • docs/operations/seaweedfs-431-rename-recovery.md
  • hack/seaweedfs-naming-audit.bats
  • hack/seaweedfs-naming-audit.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • hack/seaweedfs-naming-audit.sh

📝 Walkthrough

Walkthrough

The 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.

Changes

SeaweedFS audit hardening

Layer / File(s) Summary
Fail-closed kubectl and Helm inspection
hack/seaweedfs-naming-audit.sh, hack/seaweedfs-naming-audit.bats
Adds centralized fatal handling for kubectl failures, validates Helm release payloads and chart metadata, and tests absent, corrupt, undecodable, non-SeaweedFS, and formatting-variation cases.
Resource enumeration failure propagation
hack/seaweedfs-naming-audit.sh, hack/seaweedfs-naming-audit.bats
Routes namespace, PVC, StatefulSet, and PV queries through fail-closed execution, with tests for fatal query errors and absent-PV fallback behavior.
Conservative MIXED classification and runbook checks
hack/seaweedfs-naming-audit.sh, hack/seaweedfs-naming-audit.bats, docs/operations/seaweedfs-431-rename-recovery.md
Tracks incomplete PV-age evidence, suppresses uncertain deletion candidates, verifies exact L and MIXED output, and requires successful audit exit codes before recovery steps continue.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested labels: area/uncategorized

Suggested reviewers: lexfrei

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
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: fail-closed naming audit behavior for kubectl and payload errors.
Linked Issues check ✅ Passed The script and tests address the issue by failing on kubectl LIST/GET errors and adding failure-injection coverage for affected enumerations.
Out of Scope Changes check ✅ Passed The extra payload-hardening and runbook updates are directly related to the audit failure mode and do not appear unrelated.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/seaweedfs-audit-fail-closed

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/bug Categorizes issue or PR as related to a bug size/XL This PR changes 500-999 lines, ignoring generated files labels Jul 23, 2026
@myasnikovdaniil
myasnikovdaniil marked this pull request as ready for review July 27, 2026 06:10
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@dosubot dosubot Bot added the area/testing Issues or PRs related to testing (e2e, bats, unit tests) label Jul 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
hack/seaweedfs-naming-audit.sh (1)

196-199: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Chart-name extraction depends on exact key adjacency; consider a more tolerant match before making it fatal.

s/.*"chart":{"metadata":{"name":"..."/ requires chartmetadataname to be immediately adjacent with no intervening keys or whitespace. That holds for today's compact Helm payloads (Metadata.Name is the first field), but it's now the difference between a normal audit and a hard audit_fatal abort. 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 name independently of position, or preferring jq when 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 win

Make the fake's catch-all fail instead of exit 0.

The fallthrough (and the get pvc / get pv unmatched-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
 FAKE

Note the get pvc/get pv inner case blocks 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 value

Temp dir leaks when the golden diff fails.

Every other test in this section removes $d before asserting; these two remove it after diff, so a failing golden run leaves the directory behind on each retry. If keeping the artifacts on failure is intentional, a trap 'rm -rf "$d"' EXIT at creation gives both cleanup and the got/want echo 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

📥 Commits

Reviewing files that changed from the base of the PR and between 18e459a and 3dc6cbd.

📒 Files selected for processing (2)
  • hack/seaweedfs-naming-audit.bats
  • hack/seaweedfs-naming-audit.sh

IvanHunters
IvanHunters previously approved these changes Jul 27, 2026

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_releases loop parses only the first retained revision and breaks (inherited from the old code); on an empty payload from a concurrent-prune race the release silently drops from the report. continue on 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>
@myasnikovdaniil
myasnikovdaniil merged commit 2c45377 into main Jul 28, 2026
22 of 25 checks passed
@myasnikovdaniil
myasnikovdaniil deleted the fix/seaweedfs-audit-fail-closed branch July 28, 2026 11:56
@myasnikovdaniil myasnikovdaniil added the backport Should change be backported on previous release label Jul 28, 2026
@github-actions

Copy link
Copy Markdown

@myasnikovdaniil myasnikovdaniil added the backport-previous Backport target — previous release line label Aug 17, 2026
@github-actions

Copy link
Copy Markdown

myasnikovdaniil added a commit that referenced this pull request Aug 19, 2026
…on kubectl and payload errors (#3878)

# Description
Backport of #3436 to `release-1.5`.
myasnikovdaniil added a commit that referenced this pull request Aug 19, 2026
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>
myasnikovdaniil added a commit that referenced this pull request Aug 19, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) area/testing Issues or PRs related to testing (e2e, bats, unit tests) backport Should change be backported on previous release backport-previous Backport target — previous release line kind/bug Categorizes issue or PR as related to a bug size/XL This PR changes 500-999 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(hack): seaweedfs naming audit reports a false clean when a kubectl LIST fails

2 participants