Repository navigation
router: Move from Host header to explicit actor target header - #1333
Conversation
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
Looking good to me overall. Just a few nits. Also I think we need to figure out how to message this to the community as it will definitely break existing demos and things.
3c0067d to
e5b20ec
Compare
|
I think this needs to be discussed because the hostname-based integration is the simplest way for a system to integrate into being able to "send to a specific actor". There are a couple of topics that are somewhat independent:
I would like to understand how this alternative ends up for our users. Reference material: |
|
Add DO NOT MERGE for the discussion |
Keith Mattix II (keithmattix)
changed the title
|
Keith Mattix II (@keithmattix) Thanks for organizing the call on Friday. Here's my take from the perspective of an operator of one of the higher order orchestrators the PR description mentions. Hopefully this helps close out the questions. Note Recommendation: one header, Why one header? One header is preferred so that it's easier for me to check and set as simply as possible. Why ate-target-actor? A header called just ate-actor is ambiguous about which end it names. x- headers are deprecated. Why not the alternatives, from where I sit:
I'd be happy to add a little backwards compatibility shim to set the ate-target-actor header to avoid disrupting existing demos. The shim would accept requests like https://my-actor.my-atespace.actors.resources.substrate.ate.dev then forward the request to ate-router after dynamically setting the ate-target-actor header derived from the host header. This could be implemented with a simple ext_authz provider, (or many other alternatives TBD) which is how I prefer to handle authentication and authorization at the ingress Gateway anyway. |
e5b20ec to
5b58047
Compare
|
Jeff McCune (@jeffmccune) Appreciate the feedback! Agreed; one (non-X-prefixed) header is better in several ways |
Keith Mattix II (keithmattix)
changed the title
5b58047 to
280c47d
Compare
|
This is a follow-up to the discussion on 026-09-04:
It would be good to be able to give users some "easy path" towards reaching their actors/agents. Mandating headers to be added is not possible with many clients without modification. We should keep a close watch on how many users end up building a shim anyway to use the system and incorporate the shim into Substrate if a common pattern emerges. |
Agree with this. I also think uid-based DNS is probably ideal as well, but there are concerns at the storage layer that shouldn't block this move |
Eitan Yarmush (EItanya)
changed the title
|
Removed DO NOT MERGE from the title as we met and came to a joint conclusion, I will still not merge and wait for more input, but reviews should continue from here |
7f7317e to
fdfc647
Compare
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
fdfc647 to
b2793e7
Compare
Bowei Du (bowei)
left a comment
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
Mostly looks ok. Some comments.
| Substrate routes HTTP traffic using the `Host` header. To make this easy without modifying local `/etc/hosts` files, this demo includes a lightweight NGINX reverse proxy (`jupyter-proxy`) that automatically injects the proper `Host` header (`jupyter-notebook.ate-demo-jupyter.actors.resources.substrate.ate.dev`) and forwards traffic internally to the Substrate router. | ||
| Substrate routes HTTP traffic using `Ate-Target-Actor: <atespace>/<actor>`. | ||
| A browser cannot add that header to an address-bar navigation or WebSocket | ||
| upgrade, so this demo puts a lightweight NGINX reverse proxy (`jupyter-proxy`) |
There was a problem hiding this comment.
We need to be careful about forcing all of our clients to do this.
Note: no action needed.
|
|
||
| Substrate is an early, fast moving product. It is full of debate and subject to change, including major architectural changes. It also has little to no security hardening at this time. Therefore, this threat model is focused on how to achieve security with respect to the general problem Substrate is trying to solve: Fast orchestration of a stateful AI agents. The threat model uses Substrate's current implementation and roadmap as a reference point, but focuses on threats that emerge from this overall class of system, rather than threats specific to the current implementation. | ||
|
|
||
| # Intended Outcome |
There was a problem hiding this comment.
Did this doc actually change a lot?
There was a problem hiding this comment.
Fixed; there are actually changes here, and I removed some of the whitespace to make that more obvious
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
|
Thanks for the changes. |
- [X] As required by the [Code of Conduct](https://github.com/agentgateway/agentgateway/blob/main/CODE_OF_CONDUCT.md#generative-ai-policy), the description, docs, and comments (words meant for humans) are written by a human, not by an LLM. agent-substrate/substrate#1333 removed the Host header parsing from substrate, so this PR moves agentgateway to respect the new `ate-target-actor` header. --------- Signed-off-by: Keith Mattix II <[email protected]>
- [X] As required by the [Code of Conduct](https://github.com/agentgateway/agentgateway/blob/main/CODE_OF_CONDUCT.md#generative-ai-policy), the description, docs, and comments (words meant for humans) are written by a human, not by an LLM. agent-substrate/substrate#1333 removed the Host header parsing from substrate, so this PR moves agentgateway to respect the new `ate-target-actor` header. --------- Signed-off-by: Keith Mattix II <[email protected]>
This PR is purposefully controversial to try and encourage movement on the DNS issue (#445).
There has been an emerging consensus that substrate is not a system that we expect end-users to interact with directly; we can assume a higher-level orchestrator is acting on the user's behalf to program substrate. Therefore, our networking scheme does not need to use the
Hostheader to encode an actor reference. This PR encodes that reference in one explicit routing header:ate-target-actor: <atespace>/<actor>. We expect the higher-order system originating substrate requests to set this header, removing the need for us to rewrite DNS. This also applies to actor-to-actor requests:atunneloriginates CONNECT to the egress PEP, so when addressing another actor, it can set the routing header on the CONNECT request and the PEP can forward it to the router.NOTE: CONNECT vs. direct HTTP (#1208) is a similar conversation; I see no reason not to lean 100% into CONNECT as the standard datapath for tunneling if we expect substrate to be controlled by a higher-order system. I only want to bite off one controversial issue at a time, so I'll save that for later.
Breaking change
Clients that previously selected an Actor with
Host: <actor>.<atespace>.actors.resources.substrate.ate.devmust now sendate-target-actor: <atespace>/<actor>. The custom Actor DNS suffix is removed.Hostand HTTP/2:authorityremain application metadata; CONNECT authority still carries the target host and port.