Skip to content

atenet: add option to run agentgateway - #652

Open
John Howard (howardjohn) wants to merge 3 commits into
agent-substrate:mainfrom
howardjohn:net/agw-option-s
Open

atenet: add option to run agentgateway#652
John Howard (howardjohn) wants to merge 3 commits into
agent-substrate:mainfrom
howardjohn:net/agw-option-s

Conversation

@howardjohn

Copy link
Copy Markdown
Collaborator

Agentgateway is a popular AAIF project designed for agentic systems. It is of particular interest for substrate users as it was designed specifically to serve the role of an egress proxy for agents and other AI-adjacent workloads, with strong support for credential injection, token exchanges, TLS MITM, and other authentication and authorization schemes.

Agentgateway supports the ext_proc protocol, like Envoy, so we use that here. There has been some debate in the community around the long term end state of ext_proc vs other options, but for now this maintains the status quo.

While Agentgateway can be configured dynamically over XDS, it does not take the same configuration as Envoy. However, none of the config is dynamic anyways, so we use just a static configuration file (configmap) for now to keep things simple.

While we don't have specific per-component/file OWNERS in the project at the moment, I can informally commit to myself and Eitan maintaining this integration. Given the goals around velocity in the project we can commit to either rapidly fixing any issues that may arise or removing/disabling the integration if this ends up slowing things down.

  • Tests pass
  • Appropriate changes to documentation are included in the PR

@LiorLieberman

Copy link
Copy Markdown
Collaborator

John - this would require #559 to be merged first, wouldnt it?

@howardjohn

Copy link
Copy Markdown
Collaborator Author

John - this would require #559 to be merged first, wouldnt it?

No, there is no dependency and it works end to end currently. With 559 we will need a couple line change to make it send to the atunnel instead of directly but either one can merge first

@EItanya

Copy link
Copy Markdown
Collaborator

While Agentgateway can be configured dynamically over XDS, it does not take the same configuration as Envoy. However, none of the config is dynamic anyways, so we use just a static configuration file (configmap) for now to keep things simple.

I was actually wondering if we could do the same for Envoy as well, the dynamic config seems like more complexity than it's worth right now

@EItanya Eitan Yarmush (EItanya) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of small naming things on my end, overall LGTM

Comment thread cmd/atenet/internal/router/cmd.go Outdated
Comment thread cmd/atenet/internal/router/config.go Outdated
Agentgateway is a popular [AAIF](https://aaif.io/) project designed for
agentic systems. It is of particular interest for substrate users as it
was designed specifically to serve the role of an egress proxy for
agents and other AI-adjacent workloads, with strong support for
credential injection, token exchanges, TLS MITM, and other
authentication and authorization schemes.

Agentgateway supports the ext_proc protocol, like Envoy, so we use that
here. There has been some debate in the community around the long term
end state of ext_proc vs other options, but for now this maintains the
status quo.

While Agentgateway can be configured dynamically over XDS, it does not
take the same configuration as Envoy. However, none of the config is
dynamic anyways, so we use just a static configuration file (configmap)
for now to keep things simple.

While we don't have specific per-component/file OWNERS in the project at
the moment, I can informally commit to myself and Eitan maintaining this
integration. Given the goals around velocity in the project we can
commit to either rapidly fixing any issues that may arise or
removing/disabling the integration if this ends up slowing things down.
@howardjohn

Copy link
Copy Markdown
Collaborator Author

John - this would require #559 to be merged first, wouldnt it?

Rebased on 559

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good to me, lots of switch statements/if statements throughout. Is there a way we can make it more self-contained? that most dataplanes logic happens within specific packages?

Comment thread cmd/atenet/internal/router/config.go Outdated
Comment on lines +135 to +137
if c.atenetRouter() == atenetRouterAgentgateway {
return nil
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment why this is not relevant for agw and/or what it does instead?

Comment on lines +33 to +34
name: "agentgateway is valid",
cfg: routerConfig{AtenetRouter: string(atenetRouterAgentgateway), ParkedRequest: ParkedRequestConfig{Max: defaultParkedRequestMax}},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a testcase for atenetRouterEnvoy? (both as explicit, and both as a default). Maybe add a atenetRouter field in the test case or this could be better tested on a different func.

nit: change the name of the test to atenet-router set to agentgateway is valid, atenet-router set to envoy is valid.

mut.SetHeaders = append(mut.SetHeaders, &corev3.HeaderValueOption{
AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD,
Header: &corev3.HeaderValue{
Key: ":authority",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we have a const for authority likely?

Comment thread internal/atunnel/server.go Outdated

// Do not expose the router-only routing header to actor code. Restore Host
// so dataplanes that route dynamically on worker IP still give the actor its
// stable mesh hostname.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mesh?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why embedding this under a gke specific director?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants