Support Internal Nexus Callback Routing #8308
Conversation
pdoerner
left a comment
pdoerner
left a comment
There was a problem hiding this comment.
I don't think you need to worry about whether to allow insecure connections for temporal://system since those internally-routed requests will always use our internal local frontend client which should have already been configured with the correct secure/insecure setting.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
We also discussed setting temporal://system only for worker targets in nexusoperations/executors.go, external targets would still require a URL to be configured.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Overall LGTM, there's a couple of minor blocking comments.
Note that any comment that is marked as not blocking or nit is up for you to consider. These are typically based on my personal experience and preference.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Had some comments, I still need to review the test but wanted to go ahead and submit.
chaptersix
force-pushed
the
alex/nexus_ootb
branch
from
00c6a04 to
673b3ab
Compare
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
In general it looks like we are missing functional test coverage for the new /nexus/callback route. Correct me if I'm wrong.
explicitly yes but any test with a worker target for the callback exercises that new endpoint. I'll add a few tests to explicitly test the new endpoint. |
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Approved with one blocking suggestion. I'm just going to apply it myself.
## What was changed - enabled routing of nexus callback via system url ## Why? temporalio/temporal#8308 ## Checklist <!--- add/delete as needed ---> 1. Closes <!-- add issue number here --> 2. How was this tested: <!--- Please describe how you tested your changes/how we can test them --> 3. Any docs updates needed? <!--- update README if applicable or point out where to update docs.temporal.io --> --------- Co-authored-by: Roey Berman <[email protected]>
3 participants
What changed?
This PR introduces two key features to enable out-of-the-box Nexus functionality with zero configuration:
Always allow
temporal://systemcallback URLs: Thecomponent.callbacks.allowedAddressesconfiguration now automatically allowstemporal://systemURLs regardless of configured address rules. This special scheme+host combination bypasses normal URL validation and is always permitted.New UseSystemCallbackURL toggle: Added
component.nexusoperations.useSystemCallbackURLdynamic config that controls callback URL generation for worker targets:false(default): Uses the existing callback URL template approachtrue: Worker targets automatically gettemporal://systemtrueHTTPCallerProvider must be able to route internal requests based on theSource and Token headers. It should also override the path to/nexus/callback. See the HTTPCallerProvider code changes here.Callback URL Validation has been refactored. The
AddressMatchRulesstruct now includes aValidatemethod that handles the specialtemporal://systemcase and applies configured rules for other URLs.Callback Source Header: will no longer be populated for external Targets.
Why?
Once
component.nexusoperations.useSystemCallbackURLis enabled by default in a future release, these changes will allow Self-Hosted Temporal Servers to support Nexus with zero dynamic configurationSee this issue: #8298
How did you test it?