controller: trigger extproc pod rollout on injected-config drift by Aias00 · Pull Request #2427 · envoyproxy/ai-gateway

Aias00 · 2026-07-27T02:44:45Z

Description

The mutating webhook injects the extproc container config only at pod creation time, but the previous drift check only compared the container image, -logLevel, and -mcpAddr presence. Restarting the controller with changed extproc settings, or editing GatewayConfig.spec.extProc.kubernetes, could leave existing pods running stale injected config until they were recreated for some unrelated reason.

This PR computes a stable desired hash from the extproc injection inputs and writes it to the Deployment/DaemonSet pod template annotation aigateway.envoyproxy.io/extproc-config-hash. Kubernetes then owns the rollout whenever injected extproc config changes.

Upgrade behavior: workloads created before this annotation existed will not have the desired hash on their pod template. On the next Gateway reconcile, Deployments/DaemonSets with effective routes get the template hash annotation and Kubernetes performs one rollout to converge pods onto the current injected extproc config. After that first convergence rollout, reconciles are no-op unless the desired hash changes again.

The existing direct pod UUID annotation path remains for filter-config Secret refresh because it speeds projected-volume updates without restarting pods. Workload pod template patching is limited to cases where pod template state must change: sidecar add/remove, inconsistent pod state self-heal, or extproc config hash drift.

To avoid rollout loops from Kubernetes API shape/defaulting changes, the hash serializes an explicit digest of the inputs that affect extproc injection instead of hashing a full corev1.Container. The hash covers controller flags, GatewayConfig extproc settings, image pull secrets, and MCP input. It intentionally excludes secret-presence-driven -configPath / -configBundlePath args and legacy/bundle volume mounts.

Related Issues/PRs (if applicable)

Fixes #2395

Special notes for reviewers (if applicable)

The shared builder in internal/controller/extproc_builder.go is the single source of truth for the injected container and the desired template hash.
GatewayConfig.spec.extProc.kubernetes.image / imageRepository are used when checking whether an existing extproc sidecar is current.
The reconciler uses the cached route-list path; the no-cache fallback remains only on the admission webhook path.
Tests cover deterministic hashing, drift-relevant inputs, GatewayConfig-resolved image checks, and Deployment/DaemonSet template hash rollout behavior.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

codecov-commenter · 2026-07-27T02:52:51Z

Codecov Report

❌ Patch coverage is 96.69118% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.29%. Comparing base (25afc17) to head (7fcb556).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/controller/gateway.go	90.90%	2 Missing and 2 partials ⚠️
internal/controller/gateway_mutator.go	89.28%	0 Missing and 3 partials ⚠️
internal/controller/extproc_builder.go	99.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2427      +/-   ##
==========================================
+ Coverage   85.05%   85.29%   +0.24%     
==========================================
  Files         159      160       +1     
  Lines       22645    22704      +59     
==========================================
+ Hits        19261    19366     +105     
+ Misses       2217     2180      -37     
+ Partials     1167     1158       -9

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nacx · 2026-07-30T10:49:09Z

@kanurag94 can you take a look?

kanurag94 · 2026-07-31T07:01:59Z

 		for i := range podSpec.InitContainers {
 			// If there's an extproc sidecar container with the current target image, we don't need to roll out the deployment.
-			if podSpec.InitContainers[i].Name == extProcContainerName && podSpec.InitContainers[i].Image == c.extProcImage {
+			if podSpec.InitContainers[i].Name == extProcContainerName && podSpec.InitContainers[i].Image == c.image {


since this PR already does resolveExtProcImage, we can fix this and instead of using the global extproc image, use the one from gatewayConfig GatewayConfig.spec.extProc.kubernetes.image

Fixed in 6284456. The Gateway reconciler now resolves the desired extproc image from the same extProcContainerInput used for hashing, so GatewayConfig.spec.extProc.kubernetes.image / imageRepository are honored when checking whether the existing sidecar is current.

Added regression coverage for the GatewayConfig-resolved image path.

kanurag94 · 2026-07-31T07:19:25Z

+			require.Equal(t, h1, h2, "hash must be stable across recomputes")
+		})
+	}
+}


I think this test will be more useful if it really mutates a pod against a fake client and validates it. Currently it just is a UT to check hash calculation

Addressed in f3e96d2. Added GatewayController coverage that creates pods and deployments in the fake Kubernetes client, runs the rollout path, and validates the deployment template annotation is patched when the extproc config hash drifts.

The smaller hash unit test remains to pin deterministic hash behavior.

I couldn't find the change. Can you please point me?
We need something to mutate the pod and calculate hash

Added this directly to the mutator fake-client test. TestGatewayMutator_mutatePod_UsesNoCacheReader now calls mutatePod and verifies the pod's extproc-config-hash annotation equals
g.extProcContainerHash(extProcContainerInput{}).

kanurag94 · 2026-07-31T07:25:36Z

I think it differs from gateway_mutator.go. This only uses the cached index while webhook uses the fallback. There can be a small window where cache is not updated.

Fixed in 17673ee. The Gateway reconciler now gets mgr.GetAPIReader() and uses the same cache-index + no-cache fallback route lookup as the mutating webhook for both AIGatewayRoute and MCPRoute. Added controller tests for the cache-empty/API-reader-has-route window.

Verified with:

go test ./internal/controller -run 'TestGatewayController_listAIGatewayRoutesForGateway_NoCacheReaderFallback|TestGatewayController_listMCPRoutesForGateway_NoCacheReaderFallback|TestGatewayMutator_listAIGatewayRoutesForGateway_NoCacheReaderFallback|TestGatewayMutator_listMCPRoutesForGateway_NoCacheReaderFallback|TestGatewayController_annotateGatewayPods_ConfigHashDrift'

go test ./internal/controller

kanurag94 · 2026-07-31T07:28:43Z

+// signal can never diverge from the injected config.
+func (b *extProcBuilder) extProcContainerHash(input extProcContainerInput) string {
+	digest := extProcConfigDigest{
+		Container:        b.buildExtProcContainer(input),


What happens when k8s api changes for some reason? This rolls out all the pods. Maybe we could hash the actual inputs. WDYT?

Fixed in 6284456. The drift hash now serializes an explicit extProcConfigDigest of the inputs that affect extproc injection instead of serializing the full corev1.Container object. That keeps Kubernetes Container API shape/defaulting changes from changing the rollout signal, while controller flags, GatewayConfig extproc settings, image pull secrets, and MCP input still move the hash.

Verified with go test ./internal/controller.

kanurag94

This is close, but we need to ensure this doesn't break anything. The comments are too verbose right now. If possible, can we make them just what and why.

Alternative to the approach in this PR, wdyt about just patching pod annotations through deployment so kubernetes takes care of rollout itself?

Aias00 · 2026-07-31T07:46:59Z

Addressed the review summary in 6284456: comments are trimmed to what/why, the hash no longer serializes the full Kubernetes Container API object, and GatewayConfig-resolved extproc images are used when checking existing sidecars.

On rollout behavior: I kept direct pod annotation patching for ordinary filter-config Secret updates because it speeds projected-volume refresh without restarting pods. Deployment/DaemonSet template patching is still limited to cases where the pod template must change: missing sidecar, sidecar removal, inconsistent pod state, or extproc config drift.

Verified with go test ./internal/controller.

kanurag94 · 2026-07-31T14:33:17Z

+// checkPodHasSideCar checks the extproc container against the controller-global
+// image. It is kept for tests and legacy callers without GatewayConfig input.
+func (c *GatewayController) checkPodHasSideCar(pod *corev1.Pod, needMCP bool, desiredHash string) (hasSideCar, needsRollout bool) {
+	return c.checkPodHasSideCarWithImage(pod, needMCP, c.image, desiredHash)
+}
+
+// checkPodHasSideCarWithImage returns whether the extproc container is current,
+// and whether a stamped config hash mismatch requires a rollout.
+func (c *GatewayController) checkPodHasSideCarWithImage(pod *corev1.Pod, needMCP bool, desiredImage, desiredHash string) (hasSideCar, needsRollout bool) {


This is unnecessary duplicated I think. Is there a reason for it?

Removed the duplicate wrapper. checkPodHasSideCar now takes the desired image directly

kanurag94 · 2026-07-31T14:34:52Z

@@ -1164,38 +1169,54 @@
 	uuid string,
 	hasEffectiveRoute bool,
 	needMCP bool,
+	desiredHash string,
+) (ctrl.Result, error) {
+	return c.annotateGatewayPodsWithDesiredImage(ctx, pods, deployments, daemonSets, uuid, hasEffectiveRoute, needMCP, c.image, desiredHash)
+}
+
+func (c *GatewayController) annotateGatewayPodsWithDesiredImage(ctx context.Context,
+	pods []corev1.Pod,


Again, this is duplicated

Removed the duplicate annotate wrapper.

kanurag94 · 2026-07-31T14:36:42Z

 func NewGatewayController(
 	client client.Client, kube kubernetes.Interface, logger logr.Logger, envoyGatewayNamespace string,
-	extProcImage string, extProcLogLevel string, standAlone bool, uuidFn func() string, extProcAsSideCar bool,
+	standAlone bool, uuidFn func() string, options *Options, extProcAsSideCar bool,
+) *GatewayController {
+	return NewGatewayControllerWithAPIReader(client, nil, kube, logger, envoyGatewayNamespace, standAlone, uuidFn, options, extProcAsSideCar)
+}
+
+func NewGatewayControllerWithAPIReader(
+	client client.Client, noCacheReader client.Reader, kube kubernetes.Interface, logger logr.Logger, envoyGatewayNamespace string,
+	standAlone bool, uuidFn func() string, options *Options, extProcAsSideCar bool,
 ) *GatewayController {


this seems unnecessary, and just passes nil

Removed NewGatewayControllerWithAPIReader and the noCacheReader field from GatewayController. The reconciler now uses the cached client path only.

kanurag94 · 2026-07-31T14:56:26Z

+	if err := noCacheReader.List(ctx, &all); err != nil {
 		return routes, fmt.Errorf("failed to list routes: %w", err)
 	}


I think it's fine to not have noCache reader here. Reconcile can be hit multiple times and it may make listing without cache too expensive. But the current callers are fine.
Eventual consistency should be okay here so we can avoid it as well.

Updated the reconciler route-list helpers to pass nil for the no-cache reader. The no-cache fallback remains only on the admission webhook/mutator path.

kanurag94 · 2026-07-31T15:06:40Z

+	if hasSideCar {
+		if stamped := pod.Annotations[extProcConfigHashAnnotationKey]; stamped != "" && stamped != desiredHash {
+			needsRollout = true
+			c.logger.Info("extproc config hash mismatch, triggering rollout",


Can you please add stamped and desiredHash here?
If these two ever disagree permanently, this rollout repeats forever with a fresh UUID each time, and this log line is the only signal.
A counter metric would be even better

Added both fields to the hash-mismatch log line: stamped and desiredHash.

kanurag94 · 2026-07-31T15:07:42Z

+		require.False(t, hasSideCar, "logLevel mismatch must clear hasSideCar")
+	})
+
+	t.Run("sidecar mode: uses GatewayConfig resolved image", func(t *testing.T) {


This passes desiredImage in by hand, so it tests checkPodHasSideCarWithImage but not the wiring that produces the image. We should test c.extProcImage(input) here

Updated the GatewayConfig image test to build an extProcContainerInput and use c.extProcImage(input) as the desired image, instead of passing a hand-written image value.

kanurag94 · 2026-07-31T15:09:31Z


-	return hasSideCar
+	if hasSideCar {
+		if stamped := pod.Annotations[extProcConfigHashAnnotationKey]; stamped != "" && stamped != desiredHash {


The stamped != "" check is the decision that a pod with no annotation is never rolled by the hash.
It means an operator who upgrades and then changes a flag sees nothing happen until something else recreates the pods.
Could we add the comment that was previously there?

Restored the comment explaining that a missing hash means a pre-upgrade pod and should not trigger a rollout only because the annotation is absent.

kanurag94 · 2026-07-31T15:10:16Z

-	// We annotate the deployments and daemonsets under three scenarios:
-	// 1. If there's an effective route but no sidecar container, we need to add the sidecar container.
-	// 2. If there's no effective route but has sidecar container,
-	//    we need to roll out the deployment to trigger the mutation webhook to remove the sidecar container.
-	// 3. If pods are inconsistent even when rollout isn't in progress, force rollout to self-heal.


Can we keep this explanation please?

kanurag94 · 2026-07-31T15:18:53Z

Addressed the review summary in 6284456: comments are trimmed to what/why, the hash no longer serializes the full Kubernetes Container API object, and GatewayConfig-resolved extproc images are used when checking existing sidecars.

On rollout behavior: I kept direct pod annotation patching for ordinary filter-config Secret updates because it speeds projected-volume refresh without restarting pods. Deployment/DaemonSet template patching is still limited to cases where the pod template must change: missing sidecar, sidecar removal, inconsistent pod state, or extproc config drift.

Verified with go test ./internal/controller.

Thanks. I agree on keeping the pod-annotation fast path for filter-config Secret refresh.
My question was about the hash specifically. The reconciler already computes desiredHash. Could it write that to Deployment.spec.template.metadata.annotations and let Kubernetes handle the rollout, instead of the webhook stamping it on pods and the reconciler comparing?

I am just curious to know if that was thought of and rejected as an approach. If so, why?

Aias00 · 2026-08-01T09:45:15Z

Addressed the review summary in 6284456: comments are trimmed to what/why, the hash no longer serializes the full Kubernetes Container API object, and GatewayConfig-resolved extproc images are used when checking existing sidecars.
On rollout behavior: I kept direct pod annotation patching for ordinary filter-config Secret updates because it speeds projected-volume refresh without restarting pods. Deployment/DaemonSet template patching is still limited to cases where the pod template must change: missing sidecar, sidecar removal, inconsistent pod state, or extproc config drift.
Verified with go test ./internal/controller.

Thanks. I agree on keeping the pod-annotation fast path for filter-config Secret refresh. My question was about the hash specifically. The reconciler already computes desiredHash. Could it write that to Deployment.spec.template.metadata.annotations and let Kubernetes handle the rollout, instead of the webhook stamping it on pods and the reconciler comparing?

I am just curious to know if that was thought of and rejected as an approach. If so, why?

Updated this to follow the pod template annotation approach.

The webhook no longer stamps the config hash on individual pods. The reconciler now writes the desired extproc config hash to the Deployment/DaemonSet pod template annotation, so Kubernetes handles the rollout
when the injected config changes.

The existing UUID annotation is still used only for sidecar add/remove and inconsistent pod self-heal cases.

kanurag94 · 2026-08-02T04:07:41Z

+			require.Equal(t, tt.expected, imageTagOrDigest(tt.image))
+		})
+	}
+}


Can we add test for asserting number of fields?

require.Equal(t, 19, reflect.TypeOf(extProcBuilder{}).NumField(),...)

Added the field-count check and expanded the drift coverage for the builder fields.

kanurag94 · 2026-08-02T04:21:05Z

+func (c *GatewayController) listAIGatewayRoutesForGateway(ctx context.Context, gatewayName, gatewayNamespace string) (aigv1b1.AIGatewayRouteList, error) {
+	return listAIGatewayRoutesForGateway(ctx, c.client, nil, gatewayName, gatewayNamespace)
+}
+
+func (c *GatewayController) listMCPRoutesForGateway(ctx context.Context, gatewayName, gatewayNamespace string) (aigv1b1.MCPRouteList, error) {
+	return listMCPRoutesForGateway(ctx, c.client, nil, gatewayName, gatewayNamespace)
+}


nit: I think reconciler will never need a non cached reader as it waits for warmup. We can drop these methods and call them inline.

kanurag94 · 2026-08-02T04:22:30Z

-	result, err := c.annotateGatewayPods(ctx, pods, deployments, daemonSets, uid, hasEffectiveRoutes, len(mcpRoutes.Items) > 0)
+	input := extProcContainerInput{gatewayConfig: gwConfig, needMCP: len(mcpRoutes.Items) > 0}
+	result, err := c.annotateGatewayPods(ctx, pods, deployments, daemonSets, uid,
+		hasEffectiveRoutes, input.needMCP, c.extProcImage(input), c.extProcContainerHash(input))


Can you add one test which goes through Reconcile with a Gateway whose GatewayConfig overrides the image, with pods already running the overridden image, and assert no workload patch happens?

kanurag94 · 2026-08-02T04:25:24Z

+	for i := range daemonSets {
+		daemonSet := &daemonSets[i]
+		// Hash changes are written to the pod template so Kubernetes owns the rollout.
+		needsHashRollout := hasEffectiveRoute && desiredHash != "" && daemonSet.Spec.Template.Annotations[extProcConfigHashAnnotationKey] != desiredHash


I couldn't find a test for desiredHash not being nil for daemonSet. WDYT about a test for this as well?

Added a DaemonSet test for non-empty desiredHash.

kanurag94 · 2026-08-02T04:29:22Z

+// TestExtProcContainerHash_ExcludesConfigRoutingArgs ensures the secret-
+// presence-driven -configPath / -configBundlePath args are NOT part of the hash
+// (they are added by the webhook after buildExtProcContainer returns). The base
+// container's Args must contain neither flag, so secret-existence transitions
+// do not spuriously trigger rollouts.


I think this test only does the second part of this comment and not computes hash

kanurag94 · 2026-08-02T04:31:39Z

+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			h1 := b.extProcContainerHash(tc.input)
+			h2 := b.extProcContainerHash(tc.input)


nit: You can do something like

for range 20 { require.Equal(t, h1, b.extProcContainerHash(tc.input)) }

to ensure determinism

kanurag94

Very good shape now. Can you please add the upgrade rollout behavior in PR description? We can also think about it a little more if the first rollout should do restarts or not.

The mutating webhook injects the extproc container config (env vars, imagePullSecrets, endpointPrefixes, enableRedaction, maxRecvMsgSize, header-attribute flags, and GatewayConfig resources/securityContext/ volumeMounts) only at pod-creation time, but the drift check that decides whether to roll existing pods — checkPodHasSideCar — only compares the container image, -logLevel, and -mcpAddr presence. So restarting the controller with a changed --extProcExtraEnvVars (or editing GatewayConfig.spec.extProc.kubernetes.env) has no effect on running pods; new values apply only to pods recreated for unrelated reasons, leaving the fleet in a mixed state. Fix this by having the webhook stamp a hash of the injected extproc config as a pod annotation, and having the reconciler recompute the desired hash with the same builder and trigger the existing rollout path on mismatch. To rule out an infinite rollout loop (the failure mode called out in the issue), both sides share literally one build function (extProcBuilder.buildExtProcContainer) and hash its output. The hash uses stdlib encoding/json rather than sonic, because sonic does not guarantee stable field order and a non-deterministic hash would cause exactly the rollout loop we are guarding against. Scope: the hash covers config derived from controller flags + GatewayConfig + needMCP. It intentionally excludes the secret-presence- driven -configPath / -configBundlePath args and the legacy/bundle volumeMounts, so secret-existence transitions do not drive rollouts (the UUID-annotation fast path handles filter-config content). A pod created before this annotation existed has no hash; "missing" is treated as "no drift signal" (not a rollout trigger) so upgrading the controller does not roll every managed proxy once. Fixes envoyproxy#2395 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>

Signed-off-by: liuhy <liuhongyu@apache.org>

Aias00 · 2026-08-02T08:13:44Z

Updated the PR description with the upgrade behavior: workloads without the template hash get one convergence rollout on the next Gateway reconcile, then future reconciles are no-op unless the desired hash changes again.

Aias00 requested a review from a team as a code owner July 27, 2026 02:44

Copilot AI review requested due to automatic review settings July 27, 2026 02:44

dosubot Bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Jul 27, 2026

Copilot AI reviewed Jul 27, 2026

Aias00 force-pushed the fix/extproc-config-drift-rollout-2395 branch from 2c15b86 to 7211191 Compare July 27, 2026 03:07

kanurag94 reviewed Jul 31, 2026

View reviewed changes

kanurag94 reviewed Aug 2, 2026

View reviewed changes

Aias00 and others added 9 commits August 2, 2026 01:11

test: cover mixed extproc config drift rollout

e62ea34

Signed-off-by: liuhy <liuhongyu@apache.org>

fix: avoid gateway route cache race

fdf7ebe

Signed-off-by: liuhy <liuhongyu@apache.org>

fix: hash extproc inputs for drift

55d0f06

Signed-off-by: liuhy <liuhongyu@apache.org>

fix: address extproc rollout review feedback

f39f8db

Signed-off-by: liuhy <liuhongyu@apache.org>

fix: restore rollout explanation comment

ea14207

Signed-off-by: liuhy <liuhongyu@apache.org>

fix: drive extproc hash rollout from pod template

d7590e3

Signed-off-by: liuhy <liuhongyu@apache.org>

fix: clarify extproc workload rollout reasons

a713cdb

Signed-off-by: liuhy <liuhongyu@apache.org>

test: address extproc rollout review coverage

7fcb556

Signed-off-by: liuhy <liuhongyu@apache.org>

Aias00 force-pushed the fix/extproc-config-drift-rollout-2395 branch from eb22cde to 7fcb556 Compare August 2, 2026 08:13

Uh oh!

Conversation

Aias00 commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nacx commented Jul 30, 2026

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!

kanurag94 left a comment

Choose a reason for hiding this comment

Uh oh!

Aias00 commented Jul 31, 2026

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!

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!

kanurag94 commented Jul 31, 2026

Uh oh!

Aias00 commented Aug 1, 2026

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

Aias00 commented Jul 27, 2026 •

edited

Loading

codecov-commenter commented Jul 27, 2026 •

edited

Loading

kanurag94 Aug 2, 2026 •

edited

Loading

Aias00 commented Aug 2, 2026 •

edited

Loading