Add support for configuring client-wide gRPC binary metadata values, validate metadata earlier #997

jazev-stripe · 2025-08-30T01:13:18Z

What was changed

This PR allows configuring gRPC binary metadata key/values (where the key ends in -bin) on the Temporal client struct. It's already possible for Rust users of the Temporal client struct to set per-request gRPC binary metadata key/values; this allows setting client-wide headers.

The main API changes are:

Added: ClientOptions.binary_headers: Option<HashMap<String, Vec<u8>>>
Added: ConfiguredClient::set_binary_headers(&self, binary_headers: HashMap<String, Vec<u8>>) -> Result<(), InvalidHeaderError>
Added: InvalidHeaderError
Modified: ConfiguredClient::set_headers (for setting ASCII headers) now returns Err(InvalidHeaderError) if the keys or values are invalid, and no headers are modified. Before, it would just silently discard/skip invalid keys or values.
Modified: ClientOptions::connect (and the related methods) now returns an Err(ClientInitError::InvalidHeaders(InvalidHeaderError)) if any of the keys or values passed in ClientOptions are invalid

The approach taken is to parse the metadata keys/values early, and store an already-parsed representation (that can be infallibly applied to the outbound metadata, in ClientHeaders ::apply_to_metadata).

This PR is depended on by temporalio/sdk-python#1070

Why?

#991

(for the specific parts in Core SDK that are needed for the Python SDK; this doesn't cover all Core-SDK changes that are needed (for e.g. the Rust SDK or the C bridge)).

Checklist

~~Closes~~
- This is a partial implementation of [Feature Request] Ensure gRPC binary metadata headers are supported in C bridge #991, and is a prereq to closing [Feature Request] Ensure gRPC binary metadata headers are supported sdk-python#1063
How was this tested:
- See added/modified unit tests in client/src/lib.rs
- Existing tests pass (so the behavior for ASCII headers shouldn't have regressed)
- In Add support for gRPC binary metadata values sdk-python#1070, I modified the client tests to cover both per-request gRPC binary metadata and client-wide gRPC binary metadata
Any docs updates needed?
- I don't think so? Afaik there are no docs on the Core SDK from a "Rust integrators" perspective. The updates to docs.rs should be sufficient IMO.

CLAassistant · 2025-08-30T01:13:24Z

All committers have signed the CLA.

CLAassistant · 2025-08-30T01:13:24Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Sushisource · 2025-09-03T22:44:19Z

@jazev-stripe I haven't reviewed this since it's still DRAFT. Thanks for contributing though! Anything you need to un-draft? Happy to review it once it is.

jazev-stripe · 2025-09-04T15:50:05Z

@Sushisource sorry about the lack of details; I've been meaning to finish this PR (and the corresponding Python one).

I have a thread in the Stripe support channel in the Temporal Slack about this with @cretz where we've discussed the shape of the change, and I think we're mostly aligned [0]. I'm aiming to get this PR in a more reviewable state by the end of the week (once I get a spare couple hours at work🤞).

[0] One change I think I'll make based on our discussion is to make ClientHeaders.user_binary_headers and ClientHeaders.user_headers store pre-validated tonic metadata keys/values, so that applying them to the metadata is infallible (and instead return errors earlier, when the client is constructed or when set_headers/set_binary_headers is called).

cretz

This LGTM, would like @Sushisource to take a peek before merging

cretz · 2025-09-05T17:33:59Z

client/src/lib.rs


+/// Errors thrown when a gRPC metadata header is invalid.
+#[derive(thiserror::Error, Debug)]
+pub enum InvalidHeaderError {


Would have also been ok w/ just anyhow::Error (if it works here) to keep it simple since this is rare, but this more verbose one is probably fine too

Happy to switch over to an anyhow::Error if you feel strongly; otherwise I'll leave it as-is

Part of why I biased towards wrapper errors is that the tonic errors don't include the key/value that was invalid (though we could always add one/both to the anyhow error context if we wanted)

jazev-stripe · 2025-09-05T17:57:13Z

client/src/lib.rs

+        let key = match AsciiMetadataKey::from_str(&k) {
+            Ok(key) => key,
+            Err(err) => {
+                return Err(InvalidHeaderError::InvalidAsciiHeaderKey {


The match statement here is somewhat verbose, but I used it instead of .map_err to avoid cloning the key, in the case that we need to return an error

jazev-stripe · 2025-09-05T17:57:32Z

client/src/lib.rs

+            }
+        };
+        let value = match MetadataValue::from_str(&v) {
+            Ok(value) => value,


Same here (for the value)

jazev-stripe · 2025-09-05T17:58:28Z

client/src/lib.rs

+        source: tonic::metadata::errors::InvalidMetadataKey,
+    },
+    /// An ASCII header value was invalid
+    #[error("Invalid ASCII header value for key '{key}': {source}")]


I didn't include the invalid value here because I was a bit worried that including it in error messages may lead to a credentials logging risk for users (if headers are used for security values, for example).

Let me know if that makes sense, or if I should make any changes

Makes sense to me

Sushisource

Looking good to me. Thanks for the contribution! Just one potential docstring update.

Sushisource · 2025-09-05T17:57:29Z

client/src/lib.rs

+    /// HTTP headers to include on every RPC call as binary gRPC metadata (typically encoded as
+    /// base64).


Not sure what "typically" means here. Presumably the encoding will be happening when the client transmits them, so could we make a more definitive statement? Or is it dependent on some negotiation with the server?

When I was doing research, I saw https://github.com/grpc/proposal/blob/HEAD/G1-true-binary-metadata.md, which involves sometimes not using base64 for binary headers, if negotiated. However, it's unclear to me if that proposal is actually used in practice (or what the state of it is). It doesn't seem accepted based on the header of the doc, but I also see references to that symbol in the gRPC C++ codebase (possibly in a partially-implemented state).

Either way, I think it's almost always encoded as base64, and whatever the encoding is, it's internal to the gRPC transport mechanism. I'll just remove "typically"; this distinction probably doesn't matter much for consumers. The doc-comments on tonic seem to reflect that it only uses base64 too: https://docs.rs/tonic/latest/tonic/metadata/struct.MetadataValue.html

jazev-stripe mentioned this pull request Aug 30, 2025

Add support for gRPC binary metadata values temporalio/sdk-python#1070

Merged

jazev-stripe added 5 commits September 5, 2025 10:26

add binary_headers option to core Client options

baa525a

update import format

7d04df9

store already-parsed metadata, return an error when invalid

170767f

improve wording of doc comment

5a0a7ba

use Display for nested header error message

deeb57a

jazev-stripe force-pushed the jazev-stripe/binary-grpc-headers branch from 741c6ba to deeb57a Compare September 5, 2025 17:26

jazev-stripe changed the title ~~[WIP] Add support for gRPC binary metadata values~~ Add support for configuring client-wide gRPC binary metadata values, validate metadata earlier Sep 5, 2025

jazev-stripe marked this pull request as ready for review September 5, 2025 17:27

jazev-stripe requested a review from a team as a code owner September 5, 2025 17:27

cretz approved these changes Sep 5, 2025

View reviewed changes

jazev-stripe commented Sep 5, 2025

View reviewed changes

Sushisource approved these changes Sep 5, 2025

View reviewed changes

jazev-stripe added 2 commits September 5, 2025 11:11

consume result in calling codepaths

ff8d189

remove 'typically' from doc comment

b33e7bc

cretz merged commit 042372d into temporalio:master Sep 5, 2025
18 checks passed

		/// HTTP headers to include on every RPC call as binary gRPC metadata (typically encoded as
		/// base64).

Add support for configuring client-wide gRPC binary metadata values, validate metadata earlier #997

Add support for configuring client-wide gRPC binary metadata values, validate metadata earlier #997

Uh oh!

Conversation

jazev-stripe commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

CLAassistant commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Aug 30, 2025

Uh oh!

Sushisource commented Sep 3, 2025

Uh oh!

jazev-stripe commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

cretz Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Sushisource Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Sushisource left a comment

Choose a reason for hiding this comment

Uh oh!

Sushisource Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jazev-stripe commented Aug 30, 2025 •

edited

Loading

CLAassistant commented Aug 30, 2025 •

edited

Loading

jazev-stripe commented Sep 4, 2025 •

edited

Loading

jazev-stripe Sep 5, 2025 •

edited

Loading