Skip to content

fix(helm): improve GatewayClass overrides - #4345

Merged
TaylorMutch merged 3 commits into
NVIDIA:mainfrom
danehans:fix/2469-gatewayclass-overrides/danehans
Oct 9, 2026
Merged

TaylorMutch merged 3 commits into
NVIDIA:mainfrom
danehans:fix/2469-gatewayclass-overrides/danehans

Conversation

@danehans

@danehans danehans commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Improve GatewayClass overrides and existing-Gateway attachment without changing the chart's established Gateway name default.

Related Issue

Closes #2469

Fixes: #2469

Changes

  • Preserve <fullname> as the default Gateway name and preserve explicit names unchanged.
  • Support arbitrary GatewayClasses and explicit non-conflicting Gateway names.
  • Support listener selection when attaching a GRPCRoute to an existing Gateway.
  • Resolve the rendered Gateway name dynamically in Kubernetes Envoy E2E.
  • Document custom GatewayClass and existing-Gateway configuration.

Testing

  • Checks appropriate to the affected code and behavior pass
  • Unit tests added/updated (279 Helm tests pass)
  • E2E tests added/updated
  • Helm lint, generated-doc checks, Markdown lint, and pre-commit pass
  • Manual agentgateway chart-created and existing-Gateway smoke tests reached Accepted=True and ResolvedRefs=True
  • Envoy HA E2E provisions the historical openshell Gateway, discovers its proxy Service, and runs conformance through Envoy; the local run later encountered an unrelated sandbox-lifecycle sandbox not found failure

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable; user and contributor documentation were updated)

Give chart-created Gateways a distinct default name and allow GRPCRoutes
to select a listener on an existing Gateway.

Fixes: NVIDIA#2469

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@copy-pr-bot

copy-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@TaylorMutch TaylorMutch self-assigned this Oct 8, 2026
@TaylorMutch TaylorMutch added the test:e2e Requires end-to-end coverage label Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/4345 does not exist yet. A maintainer needs to comment /ok to test 64f9a355a9933f9e1fdb5578d31ba561d137a70a to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@TaylorMutch

Copy link
Copy Markdown
Collaborator

/ok to test 64f9a35

@TaylorMutch TaylorMutch added the test:e2e-kubernetes Requires Kubernetes end-to-end coverage label Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

Label test:e2e-kubernetes applied for 64f9a35. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute Kubernetes HA and credential-driver E2E after building the required gateway, sandbox, and supervisor images once. This is an optional proof-of-life suite; failures are visible in the workflow run but do not publish a required CI gate status.

@TaylorMutch

Copy link
Copy Markdown
Collaborator

@danehans, I enabled test:e2e-kubernetes and reran the checks to validate the existing Envoy path. Kubernetes HA E2E failed before reaching the HA smoke tests:

ERROR: Envoy proxy Service for Gateway openshell was not ready.
gateway.gateway.networking.k8s.io/openshell-ingress   eg

The chart now creates openshell-ingress, but wait_for_envoy_service() still selects Services and pods using owning-gateway-name=${RELEASE_NAME} (openshell here). It finds no Service and subsequently attempts a port-forward with an empty Service name.

There is also an upgrade compatibility concern beyond the test wrapper. Before this PR, releases with grpcRoute.gateway.create=true and no explicit name created <fullname>. With this PR, an upgrade replaces that Gateway with <fullname>-ingress, which can recreate the controller's proxy Service, change its external address, or interrupt ingress. The migration note provides an opt-out, but existing values do not preserve the old resource identity automatically.

My preference is to retain the existing default and document/test an explicit grpcRoute.gateway.name=openshell-ingress override for controllers whose proxy Service would otherwise collide with the OpenShell Service. Envoy does not require the default rename, and the existing name option can address the collision without changing existing releases.

Separately, the new helper truncates explicit Gateway names to 63 characters. Previously those names rendered unchanged; for an existing Gateway with a longer valid name, the route now references a different object. Please preserve explicit names unchanged and apply any truncation only to generated defaults.

How would you like to proceed? Would you be willing to preserve the current default and make the distinct name explicit in the custom GatewayClass examples, or do you have another approach that preserves existing release behavior?

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans

danehans commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

🏗️ build-from-issue-agent

@TaylorMutch I updated the PR to follow your suggested approach. It now preserves <fullname> as the default Gateway name, leaves explicit names unchanged, and documents/tests openshell-ingress as an explicit override for controllers that would otherwise create a conflicting proxy Service. The dynamic E2E Gateway lookup remains so the wrapper follows the rendered GRPCRoute parent instead of assuming a name. When you have a moment, can you re-kick CI?

@danehans danehans mentioned this pull request Oct 8, 2026
2 of 9 tasks
@TaylorMutch

Copy link
Copy Markdown
Collaborator

/ok to test 17c7e7e

@danehans

danehans commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

The PR-specific ingress fix is working (port-forwarded the rendered Envoy Service successfully). The failure occurs later in generic conformance:

update sandbox lifecycle failed due to concurrent modification
(current resource_version: 19)

Only the sandbox-lifecycle failed and the other conformance tests passed.

@TaylorMutch can you rerun the failed jobs?

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

LGTM

@TaylorMutch
TaylorMutch added this pull request to the merge queue Oct 9, 2026
Merged via the queue into NVIDIA:main with commit 40e0142 Oct 9, 2026
111 of 113 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage test:e2e-kubernetes Requires Kubernetes end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(helm): support agentgateway ingress

2 participants