feat(reborn): route WASM HTTP through shared egress#3123
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the WasmRuntimeHttpAdapter, which bridges WASM WIT HTTP imports to the shared RuntimeHttpEgress service, handling header sanitization and error mapping. Feedback was provided to improve the robustness of header decoding for empty strings and to use a Vec instead of a Map for encoding headers to support duplicate keys. Additionally, the logic for determining if a request was sent should be refined to correctly account for bodyless requests like GET, ensuring accurate error reporting and resource tracking.
| let value = serde_json::from_str::<Value>(headers_json).map_err(|_| { | ||
| WasmHostError::Denied("WASM HTTP headers must be a JSON object".to_string()) | ||
| })?; |
There was a problem hiding this comment.
The serde_json::from_str call will fail with an error if headers_json is an empty string. While the WIT contract usually provides a JSON object string (like {}), it's safer to handle the empty string case explicitly. Per repository guidelines, when handling empty strings in the context of JSON schemas, they should be preserved unless the schema specifically requires null or disallows strings.
References
- When coercing an empty string to a value based on a JSON schema, convert it to null only if the schema allows null or does not allow string. Otherwise, preserve the empty string as it may be a valid value.
| let mut encoded = Map::new(); | ||
| for (name, value) in headers { | ||
| if sensitive_response_header(&name) { | ||
| continue; | ||
| } | ||
| encoded.insert(name, Value::String(value)); | ||
| } |
There was a problem hiding this comment.
Using a serde_json::Map to encode headers means that if the response from the egress service contains duplicate header names (e.g., multiple Set-Cookie headers), only the last one will be preserved. Consider using a Vec<(String, String)> to maintain all headers and their insertion order. For small collections, a Vec is often preferred over a Map or Set in this repository to ensure deterministic behavior and support duplicate keys.
References
- When choosing data structures, consider the trade-offs between performance, readability, and deterministic ordering for debugging. For small, non-performance-critical collections, a Vec might be preferred over a HashSet if insertion order is important for debugging.
| } | ||
|
|
||
| fn wasm_http_error(error: RuntimeHttpEgressError) -> WasmHostError { | ||
| let request_was_sent = error.request_bytes() > 0; |
There was a problem hiding this comment.
The logic error.request_bytes() > 0 to determine if a request was sent might be inaccurate for requests with no body (e.g., a simple GET request). If request_bytes only tracks the body size, then a failed GET request that was actually sent to the network will be reported as Denied instead of FailedAfterRequestSent. Ensure that any accumulated metrics like resource usage are correctly tracked and propagated even when an operation fails.
References
- When an operation can fail, ensure that any accumulated metrics (like resource usage) are propagated out along with the error, not discarded.
Summary
Follow-up after #3098 to route the Reborn WASM WIT
http-requesthost import through the sharedRuntimeHttpEgressboundary instead of a WASM-local/mock HTTP surface.What changed
WasmRuntimeHttpAdapter<E>inironclaw_wasm:RuntimeHttpEgressRequestWasmRuntimeCredentialProviderso credential injections are resolved from the actual method/URL/headers rather than reused adapter-wideheaders-jsonobject using the shared runtime-sensitive header vocabulary, including token/auth/secret/credential/password/cookie-bearing header names beyond exact matchestimeout_msthroughRuntimeHttpEgressRequest->NetworkHttpRequest->NetworkTransportRequest; reqwest-backed transport applies the smaller of the request timeout and the host transport default.43.0.1to43.0.2to clear the cargo-deny advisory gate.docs/reborn/contracts/wasm.md.Verification
Links
Completes #3085.
Part of #2987.