Add config store read path and split storage module by prk-Jr · Pull Request #548 · IABTechLab/trusted-server

prk-Jr · 2026-03-23T08:34:20Z

Summary

Split fastly_storage.rs into storage/{config_store,secret_store,api_client,mod}.rs for better separation of concerns
Add PlatformConfigStore read path in the Fastly adapter (FastlyPlatformConfigStore::get via ConfigStore::try_open/try_get)
Migrate get_active_jwks and handle_trusted_server_discovery to use &RuntimeServices instead of the legacy FastlyConfigStore directly

Changes

File	Change
`crates/trusted-server-core/src/storage/mod.rs`	New module root; re-exports `StoreName`, `StoreId`, `UnavailableKvStore`
`crates/trusted-server-core/src/storage/config_store.rs`	New: `PlatformConfigStore` stub with read support and `NotImplemented` write stubs
`crates/trusted-server-core/src/storage/secret_store.rs`	New: `PlatformSecretStore` with `NotImplemented` write stubs
`crates/trusted-server-core/src/storage/api_client.rs`	Renamed/trimmed from `fastly_storage.rs`; retains API client helpers
`crates/trusted-server-core/src/fastly_storage.rs`	Deleted; replaced by `storage/` module
`crates/trusted-server-core/src/lib.rs`	Export `storage` module; remove `fastly_storage` export
`crates/trusted-server-core/src/platform/error.rs`	Add `PlatformError::NotImplemented` variant
`crates/trusted-server-core/src/platform/traits.rs`	Document `NotImplemented` on write methods in trait doc comments
`crates/trusted-server-core/src/platform/types.rs`	Add `StoreName`/`StoreId` newtypes; add `UnavailableKvStore`; add `RuntimeServicesBuilder`
`crates/trusted-server-adapter-fastly/src/platform.rs`	Add `FastlyPlatformConfigStore::get`; stub write methods on config/secret store impls
`crates/trusted-server-adapter-fastly/src/main.rs`	Use `RuntimeServicesBuilder`; update import paths after storage module rename
`crates/trusted-server-core/src/request_signing/jwks.rs`	Migrate `get_active_jwks` to accept `&RuntimeServices`
`crates/trusted-server-core/src/request_signing/endpoints.rs`	Migrate `handle_trusted_server_discovery` to accept `&RuntimeServices`; add success-path test using `StubJwksConfigStore`
`crates/trusted-server-core/src/request_signing/rotation.rs`	Update call site (mechanical import rename)
`crates/trusted-server-core/src/request_signing/signing.rs`	Update call site (mechanical import rename)

Closes

Closes #484

Test plan

cargo test --workspace
cargo clippy --workspace --all-targets --all-features -- -D warnings
cargo fmt --all -- --check
JS tests: cd crates/js/lib && npx vitest run
JS format: cd crates/js/lib && npm run format
Docs format: cd docs && npm run format
WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1

Checklist

Changes follow CLAUDE.md conventions
No unwrap() in production code — use expect("should ...")
Uses tracing macros (not println!)
New code has tests
No secrets or credentials committed

Rename crates/common → crates/trusted-server-core and crates/fastly → crates/trusted-server-adapter-fastly following the EdgeZero naming convention. Add EdgeZero workspace dependencies pinned to rev 170b74b. Update all references across docs, CI workflows, scripts, agent files, and configuration.

Introduces trusted-server-core::platform with PlatformConfigStore, PlatformSecretStore, PlatformKvStore, PlatformBackend, PlatformHttpClient, and PlatformGeo traits alongside ClientInfo, PlatformError, and RuntimeServices. Wires the Fastly adapter implementations and threads RuntimeServices into route_request. Moves GeoInfo to platform/types as platform-neutral data and adds geo_from_fastly for field mapping.

…crate-rename

…o-pr2-platform-traits

- Defer KV store opening: replace early error return with a local UnavailableKvStore fallback so routes that do not need synthetic ID access succeed when the KV store is missing or temporarily unavailable - Use ConfigStore::try_open + try_get and SecretStore::try_get throughout FastlyPlatformConfigStore and FastlyPlatformSecretStore to honour the Result contract instead of panicking on open/lookup failure - Encapsulate RuntimeServices service fields as pub(crate) with public getter methods (config_store, secret_store, backend, http_client, geo) and a pub new() constructor; adapter updated to use new() - Reference #487 in FastlyPlatformHttpClient stub (PR 6 implements it) - Remove unused KvPage re-export from platform/mod.rs - Use super::KvHandle shorthand in RuntimeServices::kv_handle()

- Split fastly_storage.rs into storage/{config_store,secret_store,api_client,mod}.rs - Add PlatformConfigStore read path via FastlyPlatformConfigStore::get using ConfigStore::try_open/try_get - Add PlatformError::NotImplemented variant; stub write methods on FastlyPlatformConfigStore and FastlyPlatformSecretStore - Add StoreName/StoreId newtypes with From<String>, From<&str>, AsRef<str> - Add UnavailableKvStore to core platform module - Add RuntimeServicesBuilder replacing 7-arg constructor - Migrate get_active_jwks and handle_trusted_server_discovery to use &RuntimeServices - Update call sites in signing.rs, rotation.rs, main.rs - Add success-path test for handle_trusted_server_discovery using StubJwksConfigStore - Fix test_parse_cookies_to_jar_empty typo (was emtpy)

ChristianPavilonis

Review Summary

Well-structured PR — the storage module split is clean, the PlatformConfigStore read path is correctly implemented, and the migration to &RuntimeServices preserves error context properly. No blockers. CI is fully green.

Highlights:

Strong newtype pattern for StoreName/StoreId prevents mix-up bugs
RuntimeServicesBuilder with exhaustive expect("should ...") messages
Graceful KV store degradation with UnavailableKvStore fallback
Excellent test_handle_trusted_server_discovery_returns_jwks_document test

Findings: 0 blockers, 2 high, 4 medium, 3 low

Findings placed in body (line not in diff)

🤔 [P2] Value not URL-encoded (pre-existing) — crates/trusted-server-core/src/storage/api_client.rs line 122

The payload format!("item_value={}", value) sends application/x-www-form-urlencoded content but doesn't actually URL-encode value. If value contains &, =, +, spaces, or JSON characters ({, }, "), the Fastly API may misinterpret it. This is pre-existing code (moved from fastly_storage.rs) but worth flagging since it's used in key rotation.

Consider: let payload = format!("item_value={}", urlencoding::encode(value));

ChristianPavilonis · 2026-03-24T19:17:20Z

crates/trusted-server-core/src/storage/config_store.rs

+
+use crate::error::TrustedServerError;
+
+trait ConfigStoreReader {


🔧 [P1] Duplicated ConfigStoreReader trait and helper

This ConfigStoreReader trait and the load_config_value helper have a near-identical counterpart in crates/trusted-server-adapter-fastly/src/platform.rs (get_config_value). They only differ in error type (TrustedServerError vs PlatformError).

Since the legacy types are documented as "will be removed once all call sites have migrated," this is acceptable as transitional duplication. Consider adding a TODO comment here referencing the adapter's version so the duplication is cleaned up when FastlyConfigStore is removed.

ChristianPavilonis · 2026-03-24T19:17:20Z

crates/trusted-server-core/src/request_signing/jwks.rs

+        }
+    }
+
+    struct NoopSecretStore;


🔧 [P1] Test double boilerplate repeated across 3 files

The Noop* test doubles and build_services_with_config helper are copy-pasted ~120 lines each here, in endpoints.rs, and in platform/mod.rs. As more modules migrate to &RuntimeServices, this will spread further.

Suggestion: Extract a shared crate::platform::test_support module (behind #[cfg(test)]) providing noop_services(), build_services_with_config(), and the Noop stubs. This is consistent with the existing crate::test_support::tests pattern used for create_test_settings().

ChristianPavilonis · 2026-03-24T19:17:20Z

crates/trusted-server-core/src/request_signing/jwks.rs

+/// cannot be read. The underlying [`crate::platform::PlatformError`] is
+/// preserved as context in the error chain.
+pub fn get_active_jwks(services: &RuntimeServices) -> Result<String, Report<TrustedServerError>> {
+    let store_name = StoreName::from(JWKS_CONFIG_STORE_NAME);


🤔 [P2] Static string allocates on every call

StoreName::from(JWKS_CONFIG_STORE_NAME) allocates a new String each time get_active_jwks is called. Since JWKS_CONFIG_STORE_NAME is &'static str, consider adding StoreName::from_static(&'static str) or accepting AsRef<str> in PlatformConfigStore::get to avoid this. Minor perf impact per-request but noted since project conventions say "minimize allocations."

ChristianPavilonis · 2026-03-24T19:17:20Z

crates/trusted-server-core/src/storage/secret_store.rs

+    /// Returns an error if the secret store cannot be opened, the key is not
+    /// found, or the plaintext cannot be retrieved.
+    pub fn get(&self, key: &str) -> Result<Vec<u8>, Report<TrustedServerError>> {
+        let store = SecretStore::open(&self.store_name).map_err(|_| {


🤔 [P2] Original error discarded

.map_err(|_| discards the underlying error from SecretStore::open. The new platform implementation in platform.rs correctly preserves it via attach(format!(...)). Consider changing to .map_err(|error| and including error in the message for debuggability.

ChristianPavilonis · 2026-03-24T19:17:20Z

crates/trusted-server-core/src/request_signing/endpoints.rs

+    let jwks_json = crate::request_signing::jwks::get_active_jwks(services).change_context(
        TrustedServerError::Configuration {
-            message: "Failed to retrieve JWKS".into(),
+            message: "failed to retrieve JWKS".into(),


🤔 [P2] Inconsistent error message casing

Pre-existing error messages in this file use "Failed to ..." (capital F) while the new code consistently uses "failed to ..." (lowercase). The Rust convention is lowercase error messages. Consider normalizing as these files are touched.

For example, line 39 still has "Failed to parse JWKS JSON" and line 46 has "Failed to serialize discovery document" — both nearby and easily normalized in this PR.

ChristianPavilonis · 2026-03-24T19:17:20Z

crates/trusted-server-core/src/storage/config_store.rs

+        );
+    }
+
+    #[test]


⛏ [P3] Legacy tests use println! and assert nothing

These tests pass regardless of outcome and use println! instead of log::debug!. Since this module is marked legacy, consider either converting to meaningful assertions or marking with #[ignore] noting they require a live Fastly environment.

aram356

👍 Looks good. Ship it

prk-Jr and others added 13 commits March 18, 2026 16:54

Merge remote-tracking branch 'origin/main' into feature/edgezero-pr1-…

020e88c

…crate-rename

Merge branch 'feature/edgezero-pr1-crate-rename' into feature/edgezer…

37c8fbf

…o-pr2-platform-traits

Merge branch 'main' into feature/edgezero-pr2-platform-traits

7495d96

Reject host strings containing control characters in BackendConfig

46e3360

Fix clippy error

2f40b4c

Validate scheme and host for control characters in BackendConfig

8210a85

Address review findings on platform abstraction layer

99d7bee

Address review findings on platform abstraction layer

a2597e5

Merge branch 'main' into feature/edgezero-pr2-platform-traits

d7a35a1

prk-Jr self-assigned this Mar 23, 2026

prk-Jr and others added 3 commits March 23, 2026 22:17

Merge branch 'main' into feature/edgezero-pr3-config-store

ce456a9

Merge branch 'main' into feature/edgezero-pr3-config-store

ed57b14

Harden legacy config-store reads and align Fastly adapter stubs

a8c5648

prk-Jr requested review from ChristianPavilonis and aram356 March 24, 2026 15:55

ChristianPavilonis reviewed Mar 24, 2026

View reviewed changes

aram356 approved these changes Mar 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config store read path and split storage module#548

Add config store read path and split storage module#548
prk-Jr wants to merge 16 commits intomainfrom
feature/edgezero-pr3-config-store

prk-Jr commented Mar 23, 2026

Uh oh!

ChristianPavilonis left a comment

Uh oh!

ChristianPavilonis Mar 24, 2026

Uh oh!

ChristianPavilonis Mar 24, 2026

Uh oh!

ChristianPavilonis Mar 24, 2026

Uh oh!

ChristianPavilonis Mar 24, 2026

Uh oh!

ChristianPavilonis Mar 24, 2026

Uh oh!

ChristianPavilonis Mar 24, 2026

Uh oh!

aram356 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		use crate::error::TrustedServerError;

		trait ConfigStoreReader {

+                      );
+                  }
+                  #[test]

Conversation

prk-Jr commented Mar 23, 2026

Summary

Changes

Closes

Test plan

Checklist

Uh oh!

ChristianPavilonis left a comment

Choose a reason for hiding this comment

Review Summary

Findings placed in body (line not in diff)

Uh oh!

ChristianPavilonis Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ChristianPavilonis Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ChristianPavilonis Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ChristianPavilonis Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ChristianPavilonis Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

ChristianPavilonis Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants