feat(rc-admin)!: add safe server configuration management#283
Conversation
|
Validation complete at exact head e16d366.
This remains mock-contract tested. Live multi-subsystem beta.10 validation is still recommended, especially for restore behavior and dynamic module application. |
overtrue
left a comment
There was a problem hiding this comment.
Review: Request changes
A well-structured server-config workflow (get/set/delete/help/history/restore/export/import + module-switch) behind a new core::ConfigApi trait, with client-side dry-run diffs, defensive redaction, confirmation gates on full replacement, owner-private no-overwrite export, and schema-validated v3 envelopes. Layering, prohibitions, and the protected-file/BREAKING process are all satisfied. Two things need attention: the module-switch set diff emits incorrect machine-readable data for a compliance control, and the "secret-safe" framing has real gaps.
Findings
[MAJOR] crates/cli/src/commands/admin/config.rs:529 — module-switch set diff mixes planes
- Why:
beforereads effective state (current.notify_enabled/current.audit_enabled) whilerequested_switches(:690) derivesafterfrom the persisted value for any omitted switch. Soset --notify onwhile audit has an env override (effective=on, persisted=off) emitsSwitchDiff{ audit_before:true, audit_after:false }andchanged:true— a phantom "audit disabled" in the v3 result — or hides a real persisted change in the opposite direction. Audit is a compliance control and this is machine-consumed output. The mutation sent is correct; the reported diff/changedis wrong. Untested (bothTestApiand the preserve-test use effective==persisted). - Fix: Compute the diff on one plane — set
notify_before: current.persisted_notify_enabled,audit_before: current.persisted_audit_enabled— and surface effective separately if desired.
[MINOR] crates/cli/src/commands/admin/config.rs:52 — set/delete take values as positional argv (client_secret=…)
- Why: Secrets appear in shell history and
ps; only bulkimportreads from a file. Consistent withmc, but undercuts the "secret-safe" framing. - Fix: Support
KEY=@fileor a stdin/value-file path for secret-bearing keys.
[MINOR] crates/core/src/admin/configuration.rs:218 — redaction is a hand-maintained key-name denylist
- Why: All currently-shipped RustFS secret keys are covered (no current leak), but the server already declares subsystems
notify_postgres/mysql/redis/nats/kafka/amqp, whose future keys (connection_string,dsn,api_key,account_key) would print in cleartext viaget/export/history/diff. Since redaction is the headline guarantee, the denylist is fragile. - Fix: Add those names now, and/or push server-side sensitivity metadata (tracked in backlog#1399).
[MINOR] crates/cli/src/commands/admin/config.rs:420 — restore hardcodes config_history(1000) to build the preview
- Why: A valid
restore_idolder than the 1000 newest entries fails with "no restorable data" even though the server route would accept it. - Fix: On miss, page further or fall back to apply-by-id with a "diff unavailable" note (still
--yes-gated).
[MINOR] crates/cli/src/commands/admin/config.rs:666 — validate_server_document/validate_server_keys fire one sequential config_help round-trip per key and treat any error as fatal
- Why: A full
importbecomes O(fields) sequential signed requests (slow), and any valid key absent from the server's help metadata blocks the whole import/set even though the server would accept it. - Fix: Batch, or downgrade a help-miss to a warning.
[MINOR] crates/core/src/admin/configuration.rs:290 — get/export return <redacted invalid configuration> on any parse miss, with no --raw escape hatch
- Why: The client re-implements the server's config grammar; any unmodeled syntax yields a useless placeholder (fails closed, but fails hard).
- Fix: Best-effort per-line redaction that preserves unparseable non-secret lines, or document the limitation.
Test coverage
Adequate — ≥2 exit-code scenarios per command, mutation-count assertions, and integration tests validating emitted JSON against the real schemas/output_v3.json. Gaps: no test for the module-switch diff under an env override (would catch the MAJOR); the full-target delete (empty-keys) diff branch is not directly unit-tested.
Nice work
Custom Debug impls redact ConfigDocument/ConfigHistoryEntry; safe_config_error discards server response bodies so a secret echoed in a 403 never surfaces (tested); export uses create_new + 0o600 and refuses to overwrite; import rejects *redacted* placeholders; secrets stay in request bodies, never in URLs/query strings.
Related issues
Closes rustfs/backlog#1375
Parent roadmap: rustfs/backlog#1361
Server safety follow-ups: rustfs/backlog#1398 and rustfs/backlog#1399
Background and user impact
RustFS 1.0.0-beta.10 exposes native configuration, history, import/export, and module-switch routes, but rc did not provide a typed workflow. Operators had no client-side dry run, secret-safe output, destructive confirmation, or stable output-v3 result.
The server also returns original history mutation data that can contain secrets, exposes no revision/ETag token, and restores directive-style history as a complete configuration. The client must defend against these limitations without claiming preservation-safe rollback.
Root cause
rc had no core trait boundary or S3 adapter for the RustFS configuration API. The CLI therefore could not validate server-owned keys, calculate a preflight diff, classify sensitive fields, or preserve typed error and output contracts.
Solution
Server limitations
Validation
Passed locally at exact head e16d366:
BREAKING process
BREAKING marker required because this additive command support updates the protected docs/reference/rc/admin.md behavior contract. It does not change the local rc config schema, exit codes, output_v1, or output_v2. It uses the existing additive output_v3 admin_operations envelope, so no schema migration or version bump is required.
Review notes
This draft PR is intentionally stacked on #268 because it requires output schema v3 and runtime capability discovery.