Audit consistent use of controllerNames and their enabled flags w/ test to verify #13017

josh-pritchard · 2025-12-02T02:19:46Z

Description

Some cleanup and proactive bug fixes with handling of controllerNames and enabled flags for Envoy and agentgateway. This is in preparation to add agentgateway specific gateway parameters & independent install paths so now these need to reliably disable when not enabled if they are needed to run side by side. New tests will help confirm nothing breaks with more intrusive changes to come.

Change Type

/kind cleanup

Changelog

Removed enabled from agentgateway in GatewayParameters as it should only use controllerName to know if its agentgateway or envoy

Additional Notes

Resolves #13023

… and enabled flags Signed-off-by: Joshua Pritchard <[email protected]>

Copilot

Pull request overview

This PR removes the Enabled flag from the Agentgateway struct in GatewayParameters and establishes controllerName from the GatewayClass as the authoritative source for determining whether a gateway uses the envoy or agentgateway data plane. This cleanup simplifies the API and prevents configuration inconsistencies.

Key changes:

Removed Agentgateway.Enabled field from the API and replaced its usage with controllerName-based detection
Updated defaultAgentgatewayParameters() to populate agentgateway-specific configuration only for agentgateway controllers
Enhanced gateway controller reconciliation logic to respect enable flags for envoy and agentgateway controllers

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
api/v1alpha1/gateway_parameters_types.go	Removed `Enabled` field from `Agentgateway` struct and updated documentation to clarify that controllerName determines gateway type
api/v1alpha1/zz_generated.deepcopy.go	Auto-generated code updated to remove deepcopy logic for the removed `Enabled` field
install/helm/kgateway-crds/templates/gateway.kgateway.dev_gatewayparameters.yaml	Removed `enabled` field from CRD schema and updated field descriptions
pkg/deployer/merge.go	Removed merging of `Enabled` field from `deepMergeAgentgateway()`
pkg/deployer/gateway_parameters.go	Refactored `defaultAgentgatewayParameters()` to build agentgateway config instead of just toggling a flag; removed agentgateway config from `defaultGatewayParameters()`
pkg/deployer/gateway_parameters_test.go	Updated test assertions to check for presence of agentgateway config rather than enabled flag
pkg/deployer/deployer_test.go	Added `AgentgatewayControllerName` field to test inputs for consistency
internal/kgateway/deployer/gateway_parameters.go	Updated `getValues()` to determine gateway type using `irGW.ControllerName` comparison instead of checking enabled flag
internal/kgateway/deployer/gateway_parameters_test.go	Removed `Enabled: ptr.To(false)` from test data
internal/kgateway/controller/controller.go	Added `EnableEnvoy` and `EnableAgentgateway` fields to `GatewayConfig`
internal/kgateway/controller/gw_controller.go	Added reconciliation logic to skip gateways for disabled controllers based on enable flags
internal/kgateway/controller/start.go	Passed enable flags from global settings to gateway controller configuration
internal/kgateway/proxy_syncer/proxy_syncer.go	Replaced manual filtering logic with comment explaining that filtering happens upstream in the gateway collection
test/deployer/testutils.go	Added both controller names to `ControllerNames` set and populated `ControllerName`/`AgentgatewayControllerName` fields in `CommonCollections`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Joshua Pritchard <[email protected]>

npolshakova · 2025-12-03T14:55:18Z

test/e2e/features/dualcontroller/README.md

+
+The `upgradeHelmWithFlags()` method:
+1. Gets local chart path
+2. Sets `envoy.enabled` and `agentgateway.enabled` flags (which map to `KGW_ENABLE_ENVOY` and `KGW_ENABLE_AGENTGATEWAY` env vars)


So we're still using KGW_ENABLE_AGENTGATEWAY for the dual controller mode?

npolshakova · 2025-12-03T14:59:47Z

test/deployer/testutils.go

 	gateways := krtcollections.NewGatewayIndex(gatewayIndexConfig)
 	commonCols := &collections.CommonCollections{
-		GatewayIndex: gateways,
+		GatewayIndex:               gateways,


Are these gateways filtered out based on the controller name? Will the dual controller have both gateways in the collection?

no its just a mock collection, I actually had to work around this with some custom check in the test as the built in checks we have were expecting it to come up. I don't think its a big deal as we likely are not testing this in other suites

npolshakova · 2025-12-03T15:00:41Z

pkg/kgateway/proxy_syncer/proxy_syncer.go

-			return nil
-		}
-
+		// Note: s.commonCols.GatewayIndex.Gateways is already filtered to only include Gateways


Will this work with the dual controller? Or is common collection GatewayIndex only for the envoy-based Gateways?

yes for agw its done here

Signed-off-by: Joshua Pritchard <[email protected]>

josh-pritchard · 2025-12-03T21:40:13Z

/kick-ci

josh-pritchard · 2025-12-03T21:40:32Z

/retest

Signed-off-by: Joshua Pritchard <[email protected]>

chandler-solo · 2025-12-02T19:59:40Z

AGENTS.md

+- **Agentgateway Controller**: `kgateway.dev/agentgateway` (defined in `wellknown.DefaultAgwControllerName`)
+
+**Critical Requirements:**
+1. Controllers MUST respect `GatewayClass.spec.controllerName` - NOT the GatewayClass name


GatewayClass.spec.controllerName takes precedence, but the GatewayClass name can matter too, as in the case of the waypoint class.

Do we need to also update the deployer to check if the flags are enabled since we're currently relying on the gateway class?

kgateway/pkg/deployer/deployer.go

Line 301 in a317fe8

if gatewayClassName == d.agwGatewayClassName {

chandler-solo · 2025-12-04T03:28:36Z

test/e2e/features/dualcontroller/suite.go

+	// Each test method will manually apply its manifests after helm upgrade
+}
+
+// AfterTest overrides the base suite's AfterTest for manual cleanup


nit: delete this func?

chandler-solo · 2025-12-04T03:34:23Z

test/e2e/features/dualcontroller/suite.go

+	}
+
+	// Merge with existing extra args from test installation, but filter out conflicting flags
+	// ExtraHelmArgs comes in pairs: "--set" followed by "key=value"


nit: you can also just say --set=foo=bar

chandler-solo · 2025-12-04T03:37:36Z

test/e2e/features/parallelcontrollers/suite.go

+			ValuesFiles:     []string{s.TestInstallation.Metadata.ProfileValuesManifestFile, s.TestInstallation.Metadata.ValuesManifestFile},
+			ReleaseName:     helmutils.ChartName,
+			ChartUri:        chartUri,
+			ExtraArgs:       extraArgs,


FYI you could say 'helm --wait [--timeout=N]' and catch some 'controller pod is erroring persistently' issues with less code than EventuallyPodsRunning(controller).

left it just to be consistent with framework wide timeout settings

chandler-solo · 2025-12-04T03:40:39Z

test/e2e/features/parallelcontrollers/suite.go

+		s.TestInstallation.Assertions.Gomega.Eventually(func(g gomega.Gomega) {
+			gc := &gwv1.GatewayClass{}
+			err := s.TestInstallation.ClusterContext.Client.Get(s.Ctx, client.ObjectKey{
+				Name: "kgateway",


nit: the waypoint class could also be tested

Its an alpha feature right now we are not moving forward for now, either way it will work fine as when EnableEnvoy=false then no envoy gateway translation collection is created, ProxySyncer doesn't run, plugins don't execute for envoy gateways and waypoint functionality is all done through plugins

chandler-solo · 2025-12-04T03:47:36Z

test/e2e/features/dualcontroller/README.md

+
+### Run the entire suite
+```bash
+go test -v -timeout 30m -tags e2e ./test/e2e/tests/... -run "^TestDualController$"


nit: ./hack/run-test.sh TestDualController is how I'd run the suite outside of CI. It'll create a kind cluster, or not, build docker images, etc.

chandler-solo · 2025-12-04T03:48:18Z

test/e2e/features/dualcontroller/README.md

+go test -v -timeout 30m -tags e2e ./test/e2e/tests/... -run "^TestDualController$"
+```
+
+### Run specific test


nit: delete the rest of the file?

yeah idk why I committed this .md, it was AI slop

chandler-solo · 2025-12-04T04:00:28Z

test/e2e/features/agentgateway/jwtauth/suite.go

+// waitForGatewayReady waits for the gateway to be fully ready before making HTTP requests.
+// This prevents flakes where requests fail with "Connection refused" because the gateway
+// isn't ready yet.
+func (s *testingSuite) waitForGatewayReady() {


why have this function instead of doing this once in SetupTest() (it's compatible with BeforeTest but if BaseTestingSuite skips the test, SetupTest() will have already run)?

But also, why single out this e2e test suite? We have a minority of suites that wait the the GW to be programmed and I could imagine doing it in BaseTestingSuite so that it's done by default.

Fair, I singled it out because it flaked on my PR

I did this because I actually found a number of other tests doing similar checks like this. This test is pretty new so i figured it was a real flake because it also was not following patterns I found elsewhere in agentgateway tests

chandler-solo · 2025-12-04T04:03:22Z

pkg/deployer/gateway_parameters.go

 func defaultAgentgatewayParameters(imageInfo *ImageInfo, omitDefaultSecurityContext bool) *kgateway.GatewayParameters {
 	gwp := defaultGatewayParameters(imageInfo, omitDefaultSecurityContext)
-	gwp.Spec.Kube.Agentgateway.Enabled = ptr.To(true)
+


As the author of #13018, I want to understand this block better. Seems like we don't need this block or the test/deployer test cases would change with its addition.

Ah; you moved it in this file. You moved it because it was only in use if this function was using it, and because without 'Enabled' it smelled a little funny remaining. There's no point in having the same thing in both places. Looks good.

yeah it would have been dead code here

chandler-solo · 2025-12-04T04:23:22Z

pkg/deployer/gateway_parameters.go

+			Registry:   ptr.To(AgentgatewayRegistry),
+			Tag:        ptr.To(AgentgatewayDefaultTag),
+			Repository: ptr.To(AgentgatewayImage),
+			PullPolicy: (*corev1.PullPolicy)(ptr.To(imageInfo.PullPolicy)),


We want to default to omitting the pull policy so that k8s can use it's unsurprising logic about 'latest means Always, and otherwise...' (see https://kubernetes.io/docs/concepts/containers/images/#imagepullpolicy-defaulting).

This is confusing and maybe even casts in the wrong place, but also turns the empty string into "something" when I'd prefer it were nil. There's also validation to consider since we're using a string in ImageInfo, not corev1.PullPolicy. Perhaps use this (which will never error because imageInfo is something we control completely):

func ToPullPolicyPtr(input string) (*corev1.PullPolicy, error) { policy := corev1.PullPolicy(input) switch policy { case corev1.PullAlways, corev1.PullIfNotPresent, corev1.PullNever: return ptr.To(policy), nil case "": // let K8s nuanced default apply (see https://kubernetes.io/docs/concepts/containers/images/#imagepullpolicy-defaulting) return nil, nil default: return nil, fmt.Errorf("invalid image pull policy: %q", input) } }

nvm; you just moved this and it's working well enough with a pointer to the empty string because both data plane helm charts say '{{- if $gateway.image.pullPolicy }}'. I don't love the cast on the outside but corev1.PullPolicy is a string like imageInfo.PullPolicy...

FWIW I agree, maybe a followup?

.github/workflows/e2e.yaml

npolshakova · 2025-12-04T19:08:42Z

test/deployer/testutils.go


 	krtopts := krtutil.NewKrtOptions(ctx.Done(), nil)

 	gatewayIndexConfig := krtcollections.GatewayIndexConfig{


This can be done in a follow up, but we should also filter Gateways on the agentgateway collection side: https://github.com/kgateway-dev/kgateway/blob/main/pkg/agentgateway/plugins/collection.go#L156. This will make the policy attachment cleaner (limit what gateways you can attach to based on the controller now that dual mode is not going to be supported after the helm split)

Signed-off-by: Joshua Pritchard <[email protected]>

… only way to configure agentgateway. And after PR kgateway-dev#13017 (commit d4fc1e9), 'disabling' agentgateway is probably nonsensical. Signed-off-by: David L. Chandler <[email protected]>

Copilot AI review requested due to automatic review settings December 2, 2025 02:19

Some cleanup and proactive bug fixes with handling of controllerNames…

e70276a

… and enabled flags Signed-off-by: Joshua Pritchard <[email protected]>

Copilot started reviewing on behalf of josh-pritchard December 2, 2025 02:20 View session

gateway-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note labels Dec 2, 2025

josh-pritchard force-pushed the always-respect-controller-name branch from a1f9e20 to e70276a Compare December 2, 2025 02:20

Copilot finished reviewing on behalf of josh-pritchard December 2, 2025 02:22

Copilot AI reviewed Dec 2, 2025

View reviewed changes

new test

ed7a10b

Signed-off-by: Joshua Pritchard <[email protected]>

josh-pritchard force-pushed the always-respect-controller-name branch from 3bb058f to ed7a10b Compare December 2, 2025 05:00

Fix more tests

1bf62b1

Signed-off-by: Joshua Pritchard <[email protected]>

josh-pritchard changed the title ~~Audit consistent use of of controllerNames and their enabled flags~~ Audit consistent use of of controllerNames and their enabled flags w/ test to verify Dec 2, 2025

josh-pritchard changed the title ~~Audit consistent use of of controllerNames and their enabled flags w/ test to verify~~ Audit consistent use of controllerNames and their enabled flags w/ test to verify Dec 2, 2025

josh-pritchard added 2 commits December 2, 2025 16:44

Merge branch 'main' into always-respect-controller-name

321c3ae

Signed-off-by: Joshua Pritchard <[email protected]>

Fix more tests

5bba48e

Signed-off-by: Joshua Pritchard <[email protected]>

josh-pritchard force-pushed the always-respect-controller-name branch from 9ac0561 to 5bba48e Compare December 2, 2025 22:44

npolshakova reviewed Dec 3, 2025

View reviewed changes

Merge branch 'main' into always-respect-controller-name

967af71

Signed-off-by: Joshua Pritchard <[email protected]>

Fix from bad merge

1ec265e

Signed-off-by: Joshua Pritchard <[email protected]>

chandler-solo reviewed Dec 4, 2025

View reviewed changes

chandler-solo approved these changes Dec 4, 2025

View reviewed changes

npolshakova reviewed Dec 4, 2025

View reviewed changes

.github/workflows/e2e.yaml Outdated Show resolved Hide resolved

npolshakova reviewed Dec 4, 2025

View reviewed changes

chandler-solo approved these changes Dec 5, 2025

View reviewed changes

npolshakova approved these changes Dec 5, 2025

View reviewed changes

PR feedback

5b2eb14

Signed-off-by: Joshua Pritchard <[email protected]>

josh-pritchard force-pushed the always-respect-controller-name branch from 9db369b to 5b2eb14 Compare December 5, 2025 14:21

npolshakova approved these changes Dec 5, 2025

View reviewed changes

npolshakova added this pull request to the merge queue Dec 5, 2025

Merged via the queue into kgateway-dev:main with commit d4fc1e9 Dec 5, 2025
30 checks passed


		krtopts := krtutil.NewKrtOptions(ctx.Done(), nil)

		gatewayIndexConfig := krtcollections.GatewayIndexConfig{

Audit consistent use of controllerNames and their enabled flags w/ test to verify #13017

Audit consistent use of controllerNames and their enabled flags w/ test to verify #13017

Uh oh!

Conversation

josh-pritchard commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Change Type

Changelog

Additional Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josh-pritchard commented Dec 3, 2025

Uh oh!

josh-pritchard commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josh-pritchard Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

josh-pritchard commented Dec 2, 2025 •

edited

Loading

josh-pritchard Dec 5, 2025 •

edited

Loading