Add config store read path and split storage module#548
Add config store read path and split storage module#548prk-Jr wants to merge 8 commits intofeature/edgezero-pr2-platform-traitsfrom
Conversation
- 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
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
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/StoreIdprevents mix-up bugs RuntimeServicesBuilderwith exhaustiveexpect("should ...")messages- Graceful KV store degradation with
UnavailableKvStorefallback - Excellent
test_handle_trusted_server_discovery_returns_jwks_documenttest
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));
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // TODO: Deduplicate this transitional helper with | ||
| // trusted-server-adapter-fastly/src/platform.rs:get_config_value once | ||
| // FastlyConfigStore is removed. |
There was a problem hiding this comment.
🤔 thinking — This duplicated ConfigStoreReader trait (also in platform.rs) is acknowledged by the TODO. Worth tracking in an issue to make sure the cleanup happens soon — if the legacy removal drifts beyond the next 1-2 PRs, these could diverge silently.
Same applies to SecretStoreReader in secret_store.rs.
| /// Explicit port, or `None` to use the scheme default. | ||
| pub port: Option<u16>, | ||
| /// Whether to verify the TLS certificate. | ||
| pub certificate_check: bool, |
There was a problem hiding this comment.
🤔 thinking — certificate_check is security-sensitive and defaults to true in BackendConfig, but PlatformBackendSpec is a plain struct where a caller could construct it and forget to set this field (getting false from Default). A constructor with a safe default would prevent that.
impl PlatformBackendSpec {
pub fn new(scheme: String, host: String) -> Self {
Self {
scheme,
host,
port: None,
certificate_check: true,
first_byte_timeout: Duration::from_secs(30),
}
}
}| /// Geographic information lookup. | ||
| pub(crate) geo: Arc<dyn PlatformGeo>, | ||
| /// Per-request client metadata extracted at the entry point. | ||
| pub client_info: ClientInfo, |
There was a problem hiding this comment.
🤔 thinking — Minor inconsistency: client_info is pub (directly mutable from outside the crate) while all other fields are pub(crate) with pub read-only accessors. Intentional since ClientInfo is a simple data struct?
| /// preserved as context in the error chain. | ||
| pub fn get_active_jwks(services: &RuntimeServices) -> Result<String, Report<TrustedServerError>> { | ||
| let active_kids_str = services | ||
| .config_store() |
There was a problem hiding this comment.
🤔 thinking — This reads active-kids, then reads each kid individually — each config_store().get() call opens the store fresh in the Fastly adapter. For the typical 1-2 kids this is fine, but worth noting if the kid count ever grows.
…zero-pr3-config-store
prk-Jr
changed the base branch from
main
to
feature/edgezero-pr2-platform-traits
3 participants
Summary
fastly_storage.rsintostorage/{config_store,secret_store,api_client,mod}.rsfor better separation of concernsPlatformConfigStoreread path in the Fastly adapter (FastlyPlatformConfigStore::getviaConfigStore::try_open/try_get)get_active_jwksandhandle_trusted_server_discoveryto use&RuntimeServicesinstead of the legacyFastlyConfigStoredirectlyChanges
crates/trusted-server-core/src/storage/mod.rsStoreName,StoreId,UnavailableKvStorecrates/trusted-server-core/src/storage/config_store.rsPlatformConfigStorestub with read support andNotImplementedwrite stubscrates/trusted-server-core/src/storage/secret_store.rsPlatformSecretStorewithNotImplementedwrite stubscrates/trusted-server-core/src/storage/api_client.rsfastly_storage.rs; retains API client helperscrates/trusted-server-core/src/fastly_storage.rsstorage/modulecrates/trusted-server-core/src/lib.rsstoragemodule; removefastly_storageexportcrates/trusted-server-core/src/platform/error.rsPlatformError::NotImplementedvariantcrates/trusted-server-core/src/platform/traits.rsNotImplementedon write methods in trait doc commentscrates/trusted-server-core/src/platform/types.rsStoreName/StoreIdnewtypes; addUnavailableKvStore; addRuntimeServicesBuildercrates/trusted-server-adapter-fastly/src/platform.rsFastlyPlatformConfigStore::get; stub write methods on config/secret store implscrates/trusted-server-adapter-fastly/src/main.rsRuntimeServicesBuilder; update import paths after storage module renamecrates/trusted-server-core/src/request_signing/jwks.rsget_active_jwksto accept&RuntimeServicescrates/trusted-server-core/src/request_signing/endpoints.rshandle_trusted_server_discoveryto accept&RuntimeServices; add success-path test usingStubJwksConfigStorecrates/trusted-server-core/src/request_signing/rotation.rscrates/trusted-server-core/src/request_signing/signing.rsCloses
Closes #484
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)