From baa525a6904e89a93e216c922202026d48dc88f4 Mon Sep 17 00:00:00 2001 From: Joseph Azevedo Date: Fri, 29 Aug 2025 17:55:58 -0700 Subject: [PATCH 1/7] add binary_headers option to core Client options --- client/src/lib.rs | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index 6135acdcb..a35d15807 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -32,6 +32,9 @@ pub use temporal_sdk_core_protos::temporal::api::{ }, }; pub use tonic; +use tonic::metadata::{ + AsciiMetadataKey, AsciiMetadataValue, BinaryMetadataKey, BinaryMetadataValue, +}; pub use worker_registry::{Slot, SlotManager, SlotProvider, WorkerKey}; pub use workflow_handle::{ GetWorkflowResultOpts, WorkflowExecutionInfo, WorkflowExecutionResult, WorkflowHandle, @@ -149,6 +152,13 @@ pub struct ClientOptions { #[builder(default)] pub headers: Option>, + /// HTTP headers to include on every RPC call as binary gRPC metadata (typically encoded as + /// base64). + /// + /// These must be valid binary gRPC metadata keys (and end with a `-bin` suffix). + #[builder(default)] + pub binary_headers: Option>>, + /// API key which is set as the "Authorization" header with "Bearer " prepended. This will only /// be applied if the headers don't already have an "Authorization" header. #[builder(default)] @@ -344,11 +354,20 @@ pub struct ConfiguredClient { } impl ConfiguredClient { - /// Set HTTP request headers overwriting previous headers + /// Set HTTP request headers overwriting previous headers. + /// + /// This will not affect headers set via [ClientOptions::binary_headers]. pub fn set_headers(&self, headers: HashMap) { self.headers.write().user_headers = headers; } + /// Set binary HTTP request headers overwriting previous headers. + /// + /// This will not affect headers set via [ClientOptions::headers]. + pub fn set_binary_headers(&self, binary_headers: HashMap>) { + self.headers.write().user_binary_headers = binary_headers; + } + /// Set API key, overwriting previous pub fn set_api_key(&self, api_key: Option) { self.headers.write().api_key = api_key; @@ -374,6 +393,7 @@ impl ConfiguredClient { #[derive(Debug)] struct ClientHeaders { user_headers: HashMap, + user_binary_headers: HashMap>, api_key: Option, } @@ -383,11 +403,23 @@ impl ClientHeaders { // Only if not already present if !metadata.contains_key(key) { // Ignore invalid keys/values - if let (Ok(key), Ok(val)) = (MetadataKey::from_str(key), val.parse()) { + if let (Ok(key), Ok(val)) = ( + AsciiMetadataKey::from_str(key), + AsciiMetadataValue::from_str(val), + ) { metadata.insert(key, val); } } } + for (key, val) in self.user_binary_headers.iter() { + // Only if not already present + if !metadata.contains_key(key) { + // Ignore invalid keys + if let Ok(key) = BinaryMetadataKey::from_str(key) { + metadata.insert_bin(key, BinaryMetadataValue::from_bytes(val)); + } + } + } if let Some(api_key) = &self.api_key { // Only if not already present if !metadata.contains_key("authorization") @@ -492,6 +524,7 @@ impl ClientOptions { let headers = Arc::new(RwLock::new(ClientHeaders { user_headers: self.headers.clone().unwrap_or_default(), + user_binary_headers: self.binary_headers.clone().unwrap_or_default(), api_key: self.api_key.clone(), })); let interceptor = ServiceCallInterceptor { @@ -1770,6 +1803,7 @@ mod tests { // Initial header set let headers = Arc::new(RwLock::new(ClientHeaders { user_headers: HashMap::new(), + user_binary_headers: HashMap::new(), api_key: Some("my-api-key".to_owned()), })); headers From 7d04df9d1e1766fa87d9ab23aa1de4e61a3c084b Mon Sep 17 00:00:00 2001 From: Joseph Azevedo Date: Fri, 29 Aug 2025 18:04:27 -0700 Subject: [PATCH 2/7] update import format --- client/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index a35d15807..9c346ec1c 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -32,9 +32,6 @@ pub use temporal_sdk_core_protos::temporal::api::{ }, }; pub use tonic; -use tonic::metadata::{ - AsciiMetadataKey, AsciiMetadataValue, BinaryMetadataKey, BinaryMetadataValue, -}; pub use worker_registry::{Slot, SlotManager, SlotProvider, WorkerKey}; pub use workflow_handle::{ GetWorkflowResultOpts, WorkflowExecutionInfo, WorkflowExecutionResult, WorkflowHandle, @@ -81,7 +78,10 @@ use tonic::{ body::Body, client::GrpcService, codegen::InterceptedService, - metadata::{MetadataKey, MetadataMap, MetadataValue}, + metadata::{ + AsciiMetadataKey, AsciiMetadataValue, BinaryMetadataKey, BinaryMetadataValue, MetadataMap, + MetadataValue, + }, service::Interceptor, transport::{Certificate, Channel, Endpoint, Identity}, }; From 170767f5562e86a40139874a5c748afbe54f29dd Mon Sep 17 00:00:00 2001 From: Joseph Azevedo Date: Thu, 4 Sep 2025 22:43:56 -0700 Subject: [PATCH 3/7] store already-parsed metadata, return an error when invalid --- client/src/lib.rs | 221 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 191 insertions(+), 30 deletions(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index 9c346ec1c..478f48caf 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -149,13 +149,18 @@ pub struct ClientOptions { pub keep_alive: Option, /// HTTP headers to include on every RPC call. + /// + /// These must be valid gRPC metadata keys, and must not end with a `-bin` suffix (to set binary + /// headers, see [ClientOptions::binary_headers]). Invalid header keys will cause an error to be + /// returned when connecting. #[builder(default)] pub headers: Option>, /// HTTP headers to include on every RPC call as binary gRPC metadata (typically encoded as /// base64). /// - /// These must be valid binary gRPC metadata keys (and end with a `-bin` suffix). + /// These must be valid binary gRPC metadata keys (and end with a `-bin` suffix). Invalid + /// header keys will cause an error to be returned when connecting. #[builder(default)] pub binary_headers: Option>>, @@ -332,6 +337,9 @@ pub enum ClientInitError { /// Invalid URI. Configuration error, fatal. #[error("Invalid URI: {0:?}")] InvalidUri(#[from] InvalidUri), + /// Invalid gRPC metadata headers. Configuration error. + #[error("Invalid headers: {0:?}")] + InvalidHeaders(#[from] InvalidHeaderError), /// Server connection error. Crashing and restarting the worker is likely best. #[error("Server connection error: {0:?}")] TonicTransportError(#[from] tonic::transport::Error), @@ -341,6 +349,37 @@ pub enum ClientInitError { SystemInfoCallError(tonic::Status), } +/// Errors thrown when a gRPC metadata header is invalid. +#[derive(thiserror::Error, Debug)] +pub enum InvalidHeaderError { + /// A binary header key was invalid + #[error("Invalid binary header key '{key}': {source}")] + InvalidBinaryHeaderKey { + /// The invalid key + key: String, + /// The source error from tonic + source: tonic::metadata::errors::InvalidMetadataKey, + }, + /// An ASCII header key was invalid + #[error("Invalid ASCII header key '{key}': {source}")] + InvalidAsciiHeaderKey { + /// The invalid key + key: String, + /// The source error from tonic + source: tonic::metadata::errors::InvalidMetadataKey, + }, + /// An ASCII header value was invalid + #[error("Invalid ASCII header value for key '{key}': {source}")] + InvalidAsciiHeaderValue { + /// The key + key: String, + /// The invalid value + value: String, + /// The source error from tonic + source: tonic::metadata::errors::InvalidMetadataValue, + }, +} + /// A client with [ClientOptions] attached, which can be passed to initialize workers, /// or can be used directly. Is cheap to clone. #[derive(Clone, Debug)] @@ -357,15 +396,30 @@ impl ConfiguredClient { /// Set HTTP request headers overwriting previous headers. /// /// This will not affect headers set via [ClientOptions::binary_headers]. - pub fn set_headers(&self, headers: HashMap) { - self.headers.write().user_headers = headers; + /// + /// # Errors + /// + /// Will return an error if any of the provided keys or values are not valid gRPC metadata. + /// If an error is returned, the previous headers will remain unchanged. + pub fn set_headers(&self, headers: HashMap) -> Result<(), InvalidHeaderError> { + self.headers.write().user_headers = parse_ascii_headers(headers)?; + Ok(()) } /// Set binary HTTP request headers overwriting previous headers. /// /// This will not affect headers set via [ClientOptions::headers]. - pub fn set_binary_headers(&self, binary_headers: HashMap>) { - self.headers.write().user_binary_headers = binary_headers; + /// + /// # Errors + /// + /// Will return an error if any of the provided keys are not valid gRPC binary metadata keys. + /// If an error is returned, the previous headers will remain unchanged. + pub fn set_binary_headers( + &self, + binary_headers: HashMap>, + ) -> Result<(), InvalidHeaderError> { + self.headers.write().user_binary_headers = parse_binary_headers(binary_headers)?; + Ok(()) } /// Set API key, overwriting previous @@ -392,8 +446,8 @@ impl ConfiguredClient { #[derive(Debug)] struct ClientHeaders { - user_headers: HashMap, - user_binary_headers: HashMap>, + user_headers: HashMap, + user_binary_headers: HashMap, api_key: Option, } @@ -402,22 +456,13 @@ impl ClientHeaders { for (key, val) in self.user_headers.iter() { // Only if not already present if !metadata.contains_key(key) { - // Ignore invalid keys/values - if let (Ok(key), Ok(val)) = ( - AsciiMetadataKey::from_str(key), - AsciiMetadataValue::from_str(val), - ) { - metadata.insert(key, val); - } + metadata.insert(key, val.clone()); } } for (key, val) in self.user_binary_headers.iter() { // Only if not already present if !metadata.contains_key(key) { - // Ignore invalid keys - if let Ok(key) = BinaryMetadataKey::from_str(key) { - metadata.insert_bin(key, BinaryMetadataValue::from_bytes(val)); - } + metadata.insert_bin(key, val.clone()); } } if let Some(api_key) = &self.api_key { @@ -523,8 +568,10 @@ impl ClientOptions { }; let headers = Arc::new(RwLock::new(ClientHeaders { - user_headers: self.headers.clone().unwrap_or_default(), - user_binary_headers: self.binary_headers.clone().unwrap_or_default(), + user_headers: parse_ascii_headers(self.headers.clone().unwrap_or_default())?, + user_binary_headers: parse_binary_headers( + self.binary_headers.clone().unwrap_or_default(), + )?, api_key: self.api_key.clone(), })); let interceptor = ServiceCallInterceptor { @@ -591,6 +638,57 @@ impl ClientOptions { } } +fn parse_ascii_headers( + headers: HashMap, +) -> Result, InvalidHeaderError> { + let mut parsed_headers = HashMap::with_capacity(headers.len()); + for (k, v) in headers.into_iter() { + let key = match AsciiMetadataKey::from_str(&k) { + Ok(key) => key, + Err(err) => { + return Err(InvalidHeaderError::InvalidAsciiHeaderKey { + key: k, + source: err, + }); + } + }; + let value = match MetadataValue::from_str(&v) { + Ok(value) => value, + Err(err) => { + return Err(InvalidHeaderError::InvalidAsciiHeaderValue { + key: k, + value: v, + source: err, + }); + } + }; + parsed_headers.insert(key, value); + } + + Ok(parsed_headers) +} + +fn parse_binary_headers( + headers: HashMap>, +) -> Result, InvalidHeaderError> { + let mut parsed_headers = HashMap::with_capacity(headers.len()); + for (k, v) in headers.into_iter() { + let key = match BinaryMetadataKey::from_str(&k) { + Ok(key) => key, + Err(err) => { + return Err(InvalidHeaderError::InvalidBinaryHeaderKey { + key: k, + source: err, + }); + } + }; + let value = BinaryMetadataValue::from_bytes(&v); + parsed_headers.insert(key, value); + } + + Ok(parsed_headers) +} + /// Interceptor which attaches common metadata (like "client-name") to every outgoing call #[derive(Clone)] pub struct ServiceCallInterceptor { @@ -1806,11 +1904,14 @@ mod tests { user_binary_headers: HashMap::new(), api_key: Some("my-api-key".to_owned()), })); - headers - .clone() - .write() - .user_headers - .insert("my-meta-key".to_owned(), "my-meta-val".to_owned()); + headers.clone().write().user_headers.insert( + "my-meta-key".parse().unwrap(), + "my-meta-val".parse().unwrap(), + ); + headers.clone().write().user_binary_headers.insert( + "my-bin-meta-key-bin".parse().unwrap(), + vec![1, 2, 3].try_into().unwrap(), + ); let mut interceptor = ServiceCallInterceptor { opts, headers: headers.clone(), @@ -1823,6 +1924,10 @@ mod tests { req.metadata().get("authorization").unwrap(), "Bearer my-api-key" ); + assert_eq!( + req.metadata().get_bin("my-bin-meta-key-bin").unwrap(), + vec![1, 2, 3].as_slice() + ); // Overwrite at request time let mut req = tonic::Request::new(()); @@ -1830,26 +1935,33 @@ mod tests { .insert("my-meta-key", "my-meta-val2".parse().unwrap()); req.metadata_mut() .insert("authorization", "my-api-key2".parse().unwrap()); + req.metadata_mut() + .insert_bin("my-bin-meta-key-bin", vec![4, 5, 6].try_into().unwrap()); let req = interceptor.call(req).unwrap(); assert_eq!(req.metadata().get("my-meta-key").unwrap(), "my-meta-val2"); assert_eq!(req.metadata().get("authorization").unwrap(), "my-api-key2"); + assert_eq!( + req.metadata().get_bin("my-bin-meta-key-bin").unwrap(), + vec![4, 5, 6].as_slice() + ); // Overwrite auth on header - headers - .clone() - .write() - .user_headers - .insert("authorization".to_owned(), "my-api-key3".to_owned()); + headers.clone().write().user_headers.insert( + "authorization".parse().unwrap(), + "my-api-key3".parse().unwrap(), + ); let req = interceptor.call(tonic::Request::new(())).unwrap(); assert_eq!(req.metadata().get("my-meta-key").unwrap(), "my-meta-val"); assert_eq!(req.metadata().get("authorization").unwrap(), "my-api-key3"); // Remove headers and auth and confirm gone headers.clone().write().user_headers.clear(); + headers.clone().write().user_binary_headers.clear(); headers.clone().write().api_key.take(); let req = interceptor.call(tonic::Request::new(())).unwrap(); assert!(!req.metadata().contains_key("my-meta-key")); assert!(!req.metadata().contains_key("authorization")); + assert!(!req.metadata().contains_key("my-bin-meta-key-bin")); // Timeout header not overriden let mut req = tonic::Request::new(()); @@ -1862,6 +1974,55 @@ mod tests { ); } + #[test] + fn invalid_ascii_header_key() { + let invalid_headers = { + let mut h = HashMap::new(); + h.insert("x-binary-key-bin".to_owned(), "value".to_owned()); + h + }; + + let result = parse_ascii_headers(invalid_headers); + assert!(result.is_err()); + assert_eq!( + result.err().unwrap().to_string(), + "Invalid ASCII header key 'x-binary-key-bin': invalid gRPC metadata key name" + ); + } + + #[test] + fn invalid_ascii_header_value() { + let invalid_headers = { + let mut h = HashMap::new(); + // Nul bytes are valid UTF-8, but not valid ascii gRPC headers: + h.insert("x-ascii-key".to_owned(), "\x00value".to_owned()); + h + }; + + let result = parse_ascii_headers(invalid_headers); + assert!(result.is_err()); + assert_eq!( + result.err().unwrap().to_string(), + "Invalid ASCII header value for key 'x-ascii-key': failed to parse metadata value" + ); + } + + #[test] + fn invalid_binary_header_key() { + let invalid_headers = { + let mut h = HashMap::new(); + h.insert("x-ascii-key".to_owned(), vec![1, 2, 3]); + h + }; + + let result = parse_binary_headers(invalid_headers); + assert!(result.is_err()); + assert_eq!( + result.err().unwrap().to_string(), + "Invalid binary header key 'x-ascii-key': invalid gRPC metadata key name" + ); + } + #[test] fn keep_alive_defaults() { let mut builder = ClientOptionsBuilder::default(); From 5a0a7ba2ef087e9512a65865bd152592e3c01e73 Mon Sep 17 00:00:00 2001 From: Joseph Azevedo Date: Thu, 4 Sep 2025 22:47:35 -0700 Subject: [PATCH 4/7] improve wording of doc comment --- client/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index 478f48caf..92b1d0f76 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -150,9 +150,9 @@ pub struct ClientOptions { /// HTTP headers to include on every RPC call. /// - /// These must be valid gRPC metadata keys, and must not end with a `-bin` suffix (to set binary - /// headers, see [ClientOptions::binary_headers]). Invalid header keys will cause an error to be - /// returned when connecting. + /// These must be valid gRPC metadata keys, and must not be binary metadata keys (ending in + /// `-bin). To set binary headers, use [ClientOptions::binary_headers]. Invalid header keys or + /// values will cause an error to be returned when connecting. #[builder(default)] pub headers: Option>, From deeb57a717c754fcb3086e2cf52d9c59f9bb00f2 Mon Sep 17 00:00:00 2001 From: Joseph Azevedo Date: Thu, 4 Sep 2025 22:51:18 -0700 Subject: [PATCH 5/7] use Display for nested header error message --- client/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index 92b1d0f76..984e37b59 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -338,7 +338,7 @@ pub enum ClientInitError { #[error("Invalid URI: {0:?}")] InvalidUri(#[from] InvalidUri), /// Invalid gRPC metadata headers. Configuration error. - #[error("Invalid headers: {0:?}")] + #[error("Invalid headers: {0}")] InvalidHeaders(#[from] InvalidHeaderError), /// Server connection error. Crashing and restarting the worker is likely best. #[error("Server connection error: {0:?}")] From ff8d18991cab6499f3657db1e978eb68225bf7b0 Mon Sep 17 00:00:00 2001 From: Joseph Azevedo Date: Fri, 5 Sep 2025 11:11:18 -0700 Subject: [PATCH 6/7] consume result in calling codepaths --- core-c-bridge/src/client.rs | 2 +- tests/integ_tests/client_tests.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core-c-bridge/src/client.rs b/core-c-bridge/src/client.rs index f7e6a7cef..3ae47e6f4 100644 --- a/core-c-bridge/src/client.rs +++ b/core-c-bridge/src/client.rs @@ -240,7 +240,7 @@ pub extern "C" fn temporal_core_client_update_metadata( metadata: ByteArrayRef, ) { let client = unsafe { &*client }; - client + let _result = client .core .get_client() .set_headers(metadata.to_string_map_on_newlines()); diff --git a/tests/integ_tests/client_tests.rs b/tests/integ_tests/client_tests.rs index bf456f6ed..bd3f6e15d 100644 --- a/tests/integ_tests/client_tests.rs +++ b/tests/integ_tests/client_tests.rs @@ -77,7 +77,7 @@ async fn per_call_timeout_respected_whole_client() { let mut raw_client = opts.connect_no_namespace(None).await.unwrap(); let mut hm = HashMap::new(); hm.insert("grpc-timeout".to_string(), "0S".to_string()); - raw_client.get_client().set_headers(hm); + raw_client.get_client().set_headers(hm).unwrap(); let err = raw_client .describe_namespace(DescribeNamespaceRequest { namespace: NAMESPACE.to_string(), From b33e7bc007ba4558baaa6711a199bc570de53e7a Mon Sep 17 00:00:00 2001 From: Joseph Azevedo Date: Fri, 5 Sep 2025 11:33:56 -0700 Subject: [PATCH 7/7] remove 'typically' from doc comment --- client/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index 984e37b59..41cd2b79f 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -156,8 +156,7 @@ pub struct ClientOptions { #[builder(default)] pub headers: Option>, - /// HTTP headers to include on every RPC call as binary gRPC metadata (typically encoded as - /// base64). + /// HTTP headers to include on every RPC call as binary gRPC metadata (encoded as base64). /// /// These must be valid binary gRPC metadata keys (and end with a `-bin` suffix). Invalid /// header keys will cause an error to be returned when connecting.