Skip to content

Add agentgateway to CI & rename dataplane flag - #1598

Open
Keith Mattix II (keithmattix) wants to merge 5 commits into
agent-substrate:mainfrom
keithmattix:add-agentgateway-e2es-to-ci
Open

Add agentgateway to CI & rename dataplane flag#1598
Keith Mattix II (keithmattix) wants to merge 5 commits into
agent-substrate:mainfrom
keithmattix:add-agentgateway-e2es-to-ci

Conversation

@keithmattix

Copy link
Copy Markdown
Collaborator

Run agentgateway data plane tests as a part of substrate CI (non-blocking to start so we can confirm it's not flaky). Also, change the --atenet-router flag to --dataplane to make it clearer that the flag controls ingress and egress.

I've run the e2es locally across gVisor and microVM plus the MITM variants for both. The only skip we do for agentgateway is TestIngressProtocolDowngrade because 1. the behavior its testing only exists on the non-CONNECT atunnel ingress path and agentgateway only sends CONNECT to atunnel and 2. I'm not sure that we want this to be a part of the contract that substrate is bound by (e.g. do we really want to commit to atunnel always parsing HTTP?).

My goal with getting both dataplanes into CI is to start taking steps to codify the proxy (router + egress PEP) contract for substrate. The telemetry they emit, atunnel expectations, etc. are all important contracts to explicitly call out so that they don't become too coupled to a single dataplane implementation.

It's a good idea to open an issue first for discussion.

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

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
@keithmattix

Copy link
Copy Markdown
Collaborator Author

Current failure is a postgres timeout. I think CI just needs to be re-kicked

@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.

🤖 Relocated with AI assistance; the original inline comment wording is preserved.

Moved from my review on #1628, which is stacked on this PR. The three inline comments and the ingress-contract portion of that review belong here:

I have some clarifying questions about what the actual ingress contract is since there are definitely dataplane specific checks in the tests

The benchmark-specific questions remain on #1628. The CI matrix suggestion is already addressed here.

Comment thread internal/e2e/suites/networking/grpcingress_test.go
t.Fatalf("creating statusz client: %v", err)
var statusz *e2e.StatuszClient
if os.Getenv("E2E_DATAPLANE") != "agentgateway" {
statusz, err = e2e.NewStatuszClient(ctx)

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.

Moved from my comment on #1628; this feedback belongs to #1598.

Can we use a more generic solution here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My plan is to just get both data planes to a working state in CI then iterate on a contract that both data planes implement. New contracts need would need to be persisted there instead of one off. So agree with the comment but will do it in a follow up

t.Fatalf("timed out waiting for the parking gauge to satisfy the condition (last active=%d)", last)
}

func agentGatewayParkingCount(ctx context.Context) (int, bool) {

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.

Moved from my comment on #1628; this feedback belongs to #1598.

Same question about unifying the data gathering mechanism here

Keith’s reply on the original thread points to this PR’s description and its goal of defining a shared proxy contract.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah +1 to my comment above; we'll define it in a shared contract

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>
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.

2 participants