feat(gax): request-level custom HTTP/gRPC headers - #6260
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for injecting custom HTTP headers into requests via RequestOptionsBuilder::with_custom_header, along with comprehensive integration and unit tests. The review feedback highlights two key issues: first, the current logic for merging system headers silently discards multi-valued headers due to how HeaderMap iteration works; second, swallowing conversion errors when parsing custom header names and values violates the "Demand Explosive Correctness" principle, and should instead fail loudly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6260 +/- ##
==========================================
- Coverage 96.24% 96.24% -0.01%
==========================================
Files 280 280
Lines 72146 72201 +55
==========================================
+ Hits 69440 69491 +51
- Misses 2706 2710 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
37c3082 to
0c1facd
Compare
0c1facd to
7a54bb5
Compare
| .await? | ||
| .into_body(); | ||
|
|
||
| assert!( |
There was a problem hiding this comment.
Is 'authorization' not a system header too?
There was a problem hiding this comment.
Yes, added it and explained in another comment.
| .get_extension::<http::HeaderMap>() | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
| headers.insert(name, value); |
There was a problem hiding this comment.
How about repeated headers, e.g. Cache-Control or custom repeated headers?
There was a problem hiding this comment.
No code changes are needed for this, as standard repeated headers work out-of-the-box using RFC-compliant comma-separated strings:
- Under HTTP RFC 9110 (Section 5.3), standard repeated headers like
Cache-Control,Accept, orAlloware defined as comma-separated lists (e.g.,Cache-Control: no-cache, no-store). - Callers can supply repeated values natively as a single comma-separated
HeaderValuewithout needing multiple calls:builder.with_custom_header( HeaderName::from_static("cache-control"), HeaderValue::from_static("no-cache, no-store"), )
- I checked other language SDK implementations (such as Python), and they follow this same pattern.
|
|
||
| const X_GOOG_USER_PROJECT: &str = "x-goog-user-project"; | ||
|
|
||
| fn with_custom_header( |
There was a problem hiding this comment.
Why is the logic reimplemented here instead of using the implementation in RequestOptionsBuilder?
There was a problem hiding this comment.
No, this entire test file src/gax-internal/tests/http_custom_header.rs is not testing the RequestOptionsBuilder function in gax/src/options.rs. Instead, it is an integration test for the HTTP wire-level transport logic in src/gax-internal/src/http.rs.
The function with_custom_header here is a test helper function that inserts custom headers into the extensions. The test verifies that when client.execute(..., options) runs, ReqwestClient::request:
- Retrieves the custom
HeaderMapfromRequestOptionsextensions - Sanitizes and strips conflicting system/auth headers
- Correctly merges custom headers with credential headers and system headers on the wire.
| unimplemented!(); | ||
| } | ||
|
|
||
| /// Injects a custom HTTP header into this specific request. |
There was a problem hiding this comment.
Let's include more information about how custom headers behave:
- precedence vs system headers
- behaviour when used with with_user_project/with_user_agent
There was a problem hiding this comment.
Done, added detailed explanation to the with_custom_header function.
| } | ||
|
|
||
| let mut headers = match self.cred.headers(Extensions::new()).await { | ||
| let cred_headers = match self.cred.headers(Extensions::new()).await { |
There was a problem hiding this comment.
How about x-goog-user-project?
Are there other reserved headers that should be removed as well? What is the behaviour of the other SDKs?
There was a problem hiding this comment.
x-goog-user-project is removed at line 435.
| options = options.insert_extension(headers); | ||
| *self.request_options() = options; | ||
| } | ||
| _ => panic!("invalid header name or value"), |
There was a problem hiding this comment.
Instead of panicking, I think either:
- Have name and value be HeaderName/Value and make callers do the validation, or
- Return a Result instead, renaming the function to
try_with_custom_header
Otherwise, right now there's no way for the application to fail gracefully on malformed input.
There was a problem hiding this comment.
Done.
I chose to implement your suggested Option 1 to shift HeaderName and HeaderValue to the function signature, letting callers to do the validation. It's better than Option 2 as it preserves Fluent Builder ergonomics.
| .unwrap_or_default(); | ||
|
|
||
| // Strip any custom headers that collide with system headers or credential headers. | ||
| for key in cred_headers |
There was a problem hiding this comment.
Can you double check this logic in the gRPC path (
)? IfAuthorization is suppled as a custom header, in HTTP it'll be removed, but it doesn't look like it'll be removed there.
There was a problem hiding this comment.
Done.
I align the gRPC path and HTTP/JSON path as follows:
- Ensure the order of "adding authorization headers" and "adding custom headers" in both paths are the same: add custom headers, strip away system headers, then add authorization headers. Previously the order is different in gRPC and JSON, resulting in different implementations in both sites; now they are the same.
- In the sanitization code, now I add "authorization" and other authorization-related headers into the list of system headers. They would all be stripped away from user-input custom headers, before any kinds of authorization/system headers are added.
- The current implementation is aligned with other language SDKs.
…gRPC - In http.rs, reorder custom headers retrieval to the beginning of request building and sanitize custom headers against a hardcoded list of system and authentication header keys (USER_AGENT, AUTHORIZATION, X_GOOG_API_KEY, X_GOOG_USER_PROJECT). - In grpc_helpers.rs, add AUTHORIZATION, X_GOOG_API_KEY, and X_GOOG_API_CLIENT to the custom header sanitization loop in make_headers(). - Update unit tests in grpc_helpers.rs and http_custom_header.rs to test sanitization and precedence of authorization and API key custom headers.
- Change RequestOptionsBuilder::with_custom_header to accept http::header::HeaderName and http::header::HeaderValue directly, avoiding panics on malformed string input. - Callers are now responsible for validating dynamic headers via try_from/from_str and handling errors gracefully in their application. - Update unit tests in src/gax/src/options.rs to test static headers and assert TryFrom/from_str errors on invalid inputs.
Overview
Adds support for request-level custom HTTP/gRPC headers via
RequestOptionsBuilder::with_custom_header, allowing developers to inject tracking or tracing headers (e.g.,x-client-tracking-id,X-B3-TraceId) on individual API calls without signature changes.DD: go/rust-sdk-custom-header
Part of #5997
Key Changes
google-cloud-gax:with_custom_header<K, V>(mut self, name: K, value: V) -> SelftoRequestOptionsBuilder.google-cloud-gax-internal:ReqwestClient::request()to extract custom headers and enforce a "System-Wins" precedence policy.RequestOptionsBuilder::with_custom_header.http_custom_header.rs(viaecho_server).