Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Temporal-Namespace header on every gRPC request #475

Open
8 tasks
cretz opened this issue May 9, 2024 · 3 comments
Open
8 tasks

Set Temporal-Namespace header on every gRPC request #475

cretz opened this issue May 9, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@cretz
Copy link
Member

cretz commented May 9, 2024

Describe the solution you'd like

Every gRPC request that contains a namespace should have a Temporal-Namespace header (i.e. gRPC metadata) set on the request with the namespace value. This can/should be done with interceptors and needs to be future proof. This means if at all possible the namespace should be extracted out of the request without having to know about specific requests.

We have ensured that every namespace-specific gRPC request has a string namespace field and new RPCs will too.

In Go the extraction can be done with a simple interface assertion containing GetNamespace() string.

In Java and Core this may be a bit more difficult. Not sure if we want to use Java reflection or core macros or what. Or it can be done hardcoded with a the set of requests objects in big switch/match, but there must be a test to confirm nothing is missed when a new RPC is introduced. This could be done by starting a no-op/unavailable-response-only gRPC server that confirms via server-side interceptor that every call has a Temporal-Namespace header. Then simply make a client call for every RPC available, set a namespace on it (reflectively or whatever), and confirm set. But all this doesn't have to be done if there are other ways to make sure new RPCs break the build if not accounted for.

Per-SDK Tickets

@Quinn-With-Two-Ns
Copy link
Contributor

Does GetSystemInfo also need to have this header set?

@cretz
Copy link
Member Author

cretz commented May 13, 2024

@Quinn-With-Two-Ns - no I don't believe so. It is a cross-namespace call. I will confirm internally.

@Sushisource
Copy link
Member

Note, HTTP2 requires headers to be lowercase: https://www.rfc-editor.org/rfc/rfc7540#section-8.1.2

We should be using temporal-namespace and cloud should be aware of that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants