feat(storage): migrate to iii-sdk 0.19 with config-worker integration + hardened reload#236
Merged
Merged
Conversation
… + hardened reload Bump storage to iii-sdk 0.19.0 and adopt the configuration-worker pattern (mirroring the database worker): register a config schema, fetch live config over RPC, hot-reload backends on change, and register the storage::* functions inline. --config becomes an optional seed; manifest.rs and the rustfs sidecar / webhook receiver / SQS-PubSub-CF pollers are preserved. Config-reload security hardening (storage + database), fixing two adversarial -review findings: - on-config-change re-fetches the authoritative config via configuration::get and never trusts the trigger payload, closing an unauthenticated reconfiguration vector on a discoverable bus function. - storage refuses hot-reloads that change bucket/notification topology (WorkerConfig::topology signature compared in reloadable()), preventing a backend/notification split-brain; only backend-connection changes (credentials, endpoint, path-style) hot-apply. - iii-permissions.yaml denies agent invocation of the internal *::on-config-change functions as defense-in-depth. Verified: storage 91 lib + integration + clippy -D warnings; database 198 lib + 6 integration + clippy -D warnings, all green.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 14 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughStorage and database workers now delegate configuration management to a centralized ChangesConfiguration-Driven Backend Hot-Reload
🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@storage/src/config.rs`:
- Around line 35-52: The derived Debug on NotificationKey leaks
CfQueue.api_token; replace the auto-derive with a manual impl of std::fmt::Debug
for NotificationKey that prints Sqs, Pubsub, and RustfsWebhook normally but
redacts the CfQueue.api_token (e.g. show account_id and queue_id but replace
api_token with "<redacted>" or similar). Keep the existing Clone/PartialEq/Eq
derives intact and ensure the Debug impl covers all NotificationKey variants to
avoid accidental token exposure in logs or test-assert messages.
In `@storage/src/configuration.rs`:
- Around line 128-137: The registered async handler for CONFIG_FN_ID is
acknowledging success unconditionally; change RegisterFunction::new_async's
closure to await on_config_change(&engine, &st, &boot_topology) and inspect its
Result, returning Ok(json!({ "ok": true })) only on success and returning an
Err(IIIError) (with context/log message) when fetch_config, reloadable, or
apply_config fail; apply the same fix to the other registration block referenced
(the analogous code around the second handler) so storage::on-config-change
failures propagate back to the caller instead of being silently acknowledged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64e0e276-acf6-470a-8e11-921c992040c5
⛔ Files ignored due to path filters (1)
storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
database/src/configuration.rsiii-permissions.yamlstorage/Cargo.tomlstorage/README.mdstorage/src/backend/factory.rsstorage/src/config.rsstorage/src/configuration.rsstorage/src/handlers/delete_object.rsstorage/src/handlers/get_object.rsstorage/src/handlers/mod.rsstorage/src/handlers/presign_url.rsstorage/src/handlers/put_object.rsstorage/src/lib.rsstorage/src/main.rs
Comment on lines
+35
to
+52
| /// Canonical identity of a bucket's notification source — exactly what the | ||
| /// boot-time pollers/webhook key on. Compared by value; never logged. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum NotificationKey { | ||
| Sqs { | ||
| queue_url: String, | ||
| region: String, | ||
| }, | ||
| Pubsub { | ||
| subscription: String, | ||
| }, | ||
| CfQueue { | ||
| account_id: String, | ||
| queue_id: String, | ||
| api_token: String, | ||
| }, | ||
| RustfsWebhook, | ||
| } |
There was a problem hiding this comment.
Redact the CF Queue token from Debug.
NotificationKey now derives Debug while CfQueue carries api_token. Any {:?} on NotificationKey/BucketTopology/Topology or a failing equality assertion will print the raw token. The surrounding R2 config already redacts equivalent secrets, so this reintroduces a leak path.
🔐 Suggested fix
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Clone, PartialEq, Eq)]
pub enum NotificationKey {
Sqs {
queue_url: String,
region: String,
@@
RustfsWebhook,
}
+
+impl std::fmt::Debug for NotificationKey {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Self::Sqs { queue_url, region } => f
+ .debug_struct("Sqs")
+ .field("queue_url", queue_url)
+ .field("region", region)
+ .finish(),
+ Self::Pubsub { subscription } => f
+ .debug_struct("Pubsub")
+ .field("subscription", subscription)
+ .finish(),
+ Self::CfQueue {
+ account_id,
+ queue_id,
+ api_token,
+ } => f
+ .debug_struct("CfQueue")
+ .field("account_id", account_id)
+ .field("queue_id", queue_id)
+ .field("api_token", &redact_secret(api_token))
+ .finish(),
+ Self::RustfsWebhook => f.write_str("RustfsWebhook"),
+ }
+ }
+}🤖 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 `@storage/src/config.rs` around lines 35 - 52, The derived Debug on
NotificationKey leaks CfQueue.api_token; replace the auto-derive with a manual
impl of std::fmt::Debug for NotificationKey that prints Sqs, Pubsub, and
RustfsWebhook normally but redacts the CfQueue.api_token (e.g. show account_id
and queue_id but replace api_token with "<redacted>" or similar). Keep the
existing Clone/PartialEq/Eq derives intact and ensure the Debug impl covers all
NotificationKey variants to avoid accidental token exposure in logs or
test-assert messages.
Comment on lines
+128
to
+137
| iii.register_function( | ||
| CONFIG_FN_ID, | ||
| RegisterFunction::new_async(move |_payload: Value| { | ||
| let st = st.clone(); | ||
| let engine = engine.clone(); | ||
| let boot_topology = boot_topology.clone(); | ||
| async move { | ||
| on_config_change(&engine, &st, &boot_topology).await; | ||
| Ok::<Value, IIIError>(json!({ "ok": true })) | ||
| } |
There was a problem hiding this comment.
Don't ack failed config reloads as successful.
The registered storage::on-config-change function always returns success, even when fetch_config, reloadable, or apply_config fail. That drops the configuration:updated event after logging and leaves the worker on stale backends until some later config change happens to retrigger reload.
Also applies to: 164-193
🤖 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 `@storage/src/configuration.rs` around lines 128 - 137, The registered async
handler for CONFIG_FN_ID is acknowledging success unconditionally; change
RegisterFunction::new_async's closure to await on_config_change(&engine, &st,
&boot_topology) and inspect its Result, returning Ok(json!({ "ok": true })) only
on success and returning an Err(IIIError) (with context/log message) when
fetch_config, reloadable, or apply_config fail; apply the same fix to the other
registration block referenced (the analogous code around the second handler) so
storage::on-config-change failures propagate back to the caller instead of being
silently acknowledged.
sergiofilhowz
approved these changes
Jun 9, 2026
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
2 participants
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.
Summary
Migrate the
storageworker to iii-sdk 0.19.0 and adopt theconfiguration-worker pattern (mirroring thedatabaseworker), then harden the config-reload path against two adversarial-review findings.Migration
iii-sdk=0.16.0-next.2→=0.19.0(drop-in — verified against every call site).configurationworker, fetch live config over RPC, and hot-reload backends onconfiguration:updated.storage::*functions inline inmain.rs(drop theregister_allhelper).--configbecomes an optional seed (used only forinitial_valueon first register); the configuration worker is the live source of truth.manifest.rs/--manifest, the rustfs sidecar, webhook receiver, and SQS/Pub-Sub/CF-Queue pollers are preserved. Noiii-observabilityadded.Security hardening (storage + database)
*::on-config-changenow re-fetches the authoritative config viaconfiguration::getand never trusts the trigger payload — closes an unauthenticated reconfiguration vector on a discoverable bus function.WorkerConfig::topology()compared inreloadable()), preventing a backend/notification split-brain. Only backend-connection changes (credentials, endpoint, path-style) hot-apply; topology changes require a restart (logged).iii-permissions.yamldenies agent invocation of the internal*::on-config-changefunctions (defense-in-depth; does not affect engine trigger delivery).Test plan
cargo build,cargo test(91 lib + integrationput_then_get_round_trips_via_rustfs+ manifest + schema),cargo clippy --all-targets -- -D warnings— greencargo build,cargo test(198 lib + 6 integration),cargo clippy --all-targets -- -D warnings— greentopology()covers all boot-pinned wiring dimensions and theNotificationKey::CfQueueapi_tokeninclusion (CF-Queue poller is boot-pinned to it — restart required on rotation) is acceptableNotes
databasesecurity fix rides this branch but is logically separable (database was not part of the storage migration) — can be split into its own PR if preferred.Summary by CodeRabbit
New Features
Documentation
Security
Chores