Add PlatformHttpClient and PlatformBackend traits (EdgeZero PR6)#581
Add PlatformHttpClient and PlatformBackend traits (EdgeZero PR6)#581
Conversation
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.
…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)
- Make StoreName and StoreId inner fields private; From/AsRef provide all needed construction and access - Add #[deprecated] to GeoInfo::from_request with #[allow(deprecated)] at the three legacy call sites to track migration progress - Enumerate the six platform traits in the platform module doc comment - Extract backend_config_from_spec helper to remove duplicate BackendConfig construction in predict_name and ensure - Replace .into_iter().collect() with .to_vec() on secret plaintext bytes - Remove unused bytes dependency from trusted-server-adapter-fastly - Add comment on SecretStore::open clarifying it already returns Result (unlike ConfigStore::open which panics)
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Well-structured PR introducing a platform abstraction layer that decouples trusted-server-core from Fastly-specific HTTP, backend, config/secret/KV store, and geo APIs. Design quality, test coverage, and convention compliance are strong. Two issues should be addressed before merge.
Blocking
🔧 wrench
backend_namelost on remaining pending requests inselect(): Whenselect()returns, remaining requests lose theirbackend_namemetadata. Violates the trait contract and will break non-Fastly adapters. (crates/trusted-server-adapter-fastly/src/platform.rs:350)- Duplicated
platform_response_to_fastly: Identical copies inproxy.rs:40andorchestrator.rs:32that silently dropBody::Stream. Extract to a sharedpub(crate)helper now to prevent silent divergence. (crates/trusted-server-core/src/proxy.rs:40)
Non-blocking
🤔 thinking
- Silent empty string on missing backend name:
unwrap_or_default()inselect()produces""when backend name is missing, causing silent correlation failure in the orchestrator. Consider logging a warning. (crates/trusted-server-adapter-fastly/src/platform.rs:358) servicesparameter vsProxyRequestConfig: Consider foldingservicesinto the existing config struct to keepproxy_requeststable as more platform services arrive. (crates/trusted-server-core/src/proxy.rs:429)
🌱 seedling
Body::Streamsilently dropped: Three conversion sites log a warning but produce empty bodies. Considerdebug_assert!in tests. (platform.rs:263,proxy.rs:44,orchestrator.rs:34)StubHttpClientmissingsend_async/select: Auction orchestrator can't be unit-tested with current stub. Consider queue-based fan-out support.
👍 praise
StoreName/StoreIdnewtypes — excellent compile-time guardRuntimeServicesBuilderwithexpect("should ...")— follows conventions perfectly- Object safety assertions — catches trait changes early
- Control character validation in
BackendConfig— good defensive coding - Graceful
UnavailableKvStoredegradation - Strong test coverage across all new code paths
…zero-pr3-config-store
…o-pr4-secret-store
…o-pr6-backend-http-client
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
PR #581 Review: Add PlatformHttpClient and PlatformBackend traits
Overall: Well-structured PR with strong abstractions and thorough test coverage. The previous review round's findings were all addressed. Two new findings warrant attention before merge (header handling and the select() ordering assumption); the rest are improvements for follow-ups.
Praise
PlatformPendingRequestdowncast API returningErr(self)to preserve backend metadata is a thoughtful design!Sendrationale documentation onPlatformPendingRequestclearly explains the wasm32 constraintStubHttpClientfan-out test infrastructure withStubPendingResponseis clean and comprehensiveRuntimeServicesBuilderwithexpect("should ...")messages follows project conventions perfectly
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Good PR — the platform abstraction layer is well-designed with thoughtful type erasure, thorough test coverage, and clean RuntimeServices threading. A few items to address before merge:
P0 — Stale doc comment: orchestrator.rs run_providers_parallel doc (line 244) still references fastly::http::request::select() but the implementation now uses services.http_client().select(). (This line is not in the diff, so noting here instead of inline.)
P1 — Silent body truncation in release builds: The debug_assert! + log::warn! pattern for Body::Stream in edge_request_to_fastly, platform_response_to_fastly, and fastly_response_to_platform is a no-op in release. If a streaming body reaches these paths, requests/responses are sent with empty bodies. Recommend upgrading to log::error! or returning Result.
P1 — Downcast failure drops all pending requests: In FastlyPlatformHttpClient::select(), if any single PlatformPendingRequest fails downcast, the entire function errors and all remaining in-flight requests are lost. Consider adding the backend name to the error message for diagnostics.
P2 — Pre-existing: rebuild_response_with_body still uses set_header (drops multi-value headers like Set-Cookie), while the new conversion functions correctly use append_header.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Implements PlatformHttpClient with send/send_async/select and threads RuntimeServices through all proxy-layer handlers. Good test coverage for the new platform abstractions, but the allowed_domains removal from ProxyRequestConfig introduces a correctness regression for integration proxies.
Blocking
🔧 wrench
- Integration proxies blocked by first-party allowed_domains: Removing
allowed_domainsfromProxyRequestConfigmeans GTM, testlight, and other integration proxies now havesettings.proxy.allowed_domainsenforced on their upstream requests. Previously they operated in open mode (&[]). In production this will block integrations from contacting their upstream domains (crates/trusted-server-core/src/proxy.rs:582,:702)
Non-blocking
♻️ refactor
- Header forwarding test removed without replacement: The
header_copy_copies_curated_settest was deleted butproxy_request_calls_platform_http_client_sendusescopy_request_headers: false, leaving the header forwarding path untested (crates/trusted-server-core/src/proxy.rs:1977)
🤔 thinking
select()used for single-request mediator wait: Wrapping a single pending request inselect(vec![...])adds unnecessary complexity vs a dedicatedwait()method (crates/trusted-server-core/src/auction/orchestrator.rs:161)
⛏ nitpick
PROXY_FORWARD_HEADERSconst vs inlineAccept-Encoding: The 5 forwarded headers are in a const array but Accept-Encoding override is added separately inline — minor inconsistency vs the old unifiedcopy_proxy_forward_headersfunction
CI Status
- fmt: PASS
- clippy: PASS
- cargo test: PASS
- vitest: PASS
- integration tests: PASS
- CodeQL: PASS
3 participants
Summary
PlatformHttpClienttrait withsend(),send_async(), andselect()for auction fan-out — a superset of EdgeZero'sProxyClientthat covers the async/select paths not yet in EdgeZero upstreamPlatformBackendtrait withpredict_name()andensure()to decouple backend registration from Fastly-specific APIsRuntimeServicesthrough all proxy-layer handlers (IntegrationProxy::handle, endpoint handlers,proxy_request) so the HTTP client and backend are reachable without global stateChanges
platform/http.rs(new)PlatformHttpClienttrait +PlatformHttpRequest,PlatformResponse,PlatformPendingRequest,PlatformSelectResulttypesplatform/backend.rs(new)PlatformBackendtrait +PlatformBackendSpecplatform/types.rsRuntimeServicesextended withhttp_clientandbackendfieldsplatform/test_support.rsStubHttpClient,StubBackend,build_services_with_http_clienttest helpersintegrations/registry.rsIntegrationProxy::handle+IntegrationRegistry::handle_proxyaccept&RuntimeServices_servicesthroughIntegrationProxy::handleproxy.rsservices.http_client().send(); addProxyRequestHeadersstruct to stay under 7-arg limit; addproxy_request_calls_platform_http_client_sendtestauction/orchestrator.rsservicesthrough auction handlerplatform.rs(Fastly adapter)FastlyPlatformHttpClientimpl; documentBody::Streamlimitation with warning logmain.rs(Fastly adapter)runtime_servicesto all route handlersCloses
Closes #487
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-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)Notes for reviewer
This branch carries PRs 2–6 cumulatively (crate rename, platform traits, config store, secret store, HTTP client). The focused diff for this PR is the latest commit (
571656c) plus the PR6-specific commits. ThePlatformHttpClient::send_asyncandselectmethods intentionally returnErr(Unsupported)in the Fastly adapter for now — the fan-out path inorchestrator.rsstill usesfastly::http::request::selectdirectly and will be migrated in a follow-up once EdgeZero adds upstream fan-out support (issues #147–148).