extproc: skip CONTINUE_AND_REPLACE when no body change is needed#2170
extproc: skip CONTINUE_AND_REPLACE when no body change is needed#2170ChrisJBurns wants to merge 2 commits into
Conversation
The upstream cluster ext_proc filter's `upstreamProcessor.ProcessRequestHeaders`
unconditionally returns `CONTINUE_AND_REPLACE` with a `BodyMutation`, even when
nothing in the request pipeline actually needs to change the body. The
replacement body it sends is the original request bytes captured by the
downstream router phase.
That clobbers the request body when an earlier ext_proc filter in the listener
chain has already returned `CONTINUE_AND_REPLACE` with a mutated body: Envoy
applies that filter's mutation, then the upstream filter replaces the body
again with the original captured bytes — silently undoing the earlier
mutation.
The existing safety net `if bodyMutator == nil` in `applyBodyMutation` does
not fire in practice: `bodymutator.NewBodyMutator(nil, ...)` returns a
non-nil struct even when its `*filterapi.HTTPBodyMutation` argument is nil,
so the early-return is dead code. The function falls through, calls
`Mutate(originalRequestBodyRaw)` on a no-config mutator (which returns its
input unchanged), wraps that in a `BodyMutation`, and the upstream filter
sends `CONTINUE_AND_REPLACE` with the original body.
Fix:
* Add `BodyMutator.HasMutations()` predicate. `NewBodyMutator`'s contract is
unchanged (still returns non-nil); callers can ask whether the mutator
actually has anything to do.
* In `upstreamProcessor.ProcessRequestHeaders`, compute
`wantBodyReplace := bodyMutation != nil || forceBodyMutation
|| (u.bodyMutator != nil && u.bodyMutator.HasMutations())`
before calling `applyBodyMutation`. When false, skip the body-mutation work
and emit `CONTINUE` instead of `CONTINUE_AND_REPLACE`, preserving translator
header mutations, header-mutator output, and auth-handler header additions.
Skip the content-length dynamic-metadata restamp on the `CONTINUE` branch
since the body length did not change.
All existing CONTINUE_AND_REPLACE callers stay on the existing path: translator
emits a body (Bedrock/SigV4, OpenAI to Anthropic, OpenAI to Vertex, etc.) sets
bodyMutation != nil; backend HTTPBodyMutation sets HasMutations() == true;
retries and streaming-with-IncludeUsage=false set forceBodyMutation == true;
model override emits newBody via sjson.SetBytesOptions.
Tests:
* Added Test_chatCompletionProcessorUpstreamFilter_ProcessRequestHeaders_BodyReplaceContract
with two subtests:
* "no translator body, no mutator, no force -> CONTINUE" exercises the
previously untested path. Asserts Status == CONTINUE, no BodyMutation,
preserved translator and auth-handler header mutations, and no
content-length dynamic metadata.
* "translator body present -> CONTINUE_AND_REPLACE" pins existing behavior
so a future refactor cannot quietly flip the contract back.
The coverage gap that let this ship: every existing t.Run("ok", ...) subtest
under Test_chatCompletionProcessorUpstreamFilter_ProcessRequestHeaders set
retBodyMutation on the mock translator, so the translator-returns-nil-body
path was never exercised.
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
|
if |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2170 +/- ##
==========================================
+ Coverage 84.42% 84.44% +0.02%
==========================================
Files 134 134
Lines 19162 19179 +17
==========================================
+ Hits 16177 16196 +19
+ Misses 1998 1997 -1
+ Partials 987 986 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Correct - when |
Description
Scope: this PR fixes the narrow case where the upstream cluster ext_proc
filter issued
CONTINUE_AND_REPLACEeven though nothing in the requestpipeline needed to mutate the body — no translation, no model override, no
retry, no backend
HTTPBodyMutation. In that path, the upstream filterreplayed
u.parent.originalRequestBodyRaw(snapshotted at the router phase),which clobbered any body mutation applied by an earlier ext_proc filter in the
listener chain. After this PR, that path emits
CONTINUEand leaves the bodyalone.
The broader question of "preserve an intermediate filter's body mutation
across translation / retries /
HTTPBodyMutation" is not addressed bythis PR. When
wantBodyReplace == true, the replacement body is still derivedfrom the router-phase snapshot, so any intermediate filter's mutation is lost
on those branches. That's an architectural property of the snapshot-at-router-
phase design — the upstream filter has no view of Envoy's current body, by
design, to avoid re-buffering through ext_proc at the upstream phase. Fixing
it requires a separate design (either re-buffering at upstream, or
propagating a diff via dynamic metadata) and should be its own PR.
Root cause of the narrow bug. The existing safety net in
applyBodyMutationlooks like it covers the no-config case:but it does not fire in practice.
bodymutator.NewBodyMutator(nil, ...)returns a non-nil struct even when its
*filterapi.HTTPBodyMutationargumentis nil. So the early-return is dead code; the function falls through, calls
Mutate(originalRequestBodyRaw)on a no-config mutator (which returns itsinput unchanged), wraps that in a
BodyMutation, and the upstream filtersends
CONTINUE_AND_REPLACEwith the original body — even though nothingasked for a body replacement.
Fix:
Add
BodyMutator.HasMutations()predicate.NewBodyMutator's contract isunchanged (it still returns non-nil); callers can ask whether the mutator
actually has anything to do.
In
upstreamProcessor.ProcessRequestHeaders, computebefore calling
applyBodyMutation. When false, skip the body-mutation workand emit
CONTINUEinstead ofCONTINUE_AND_REPLACE, preserving translatorheader mutations (e.g. path rewrite), header-mutator output, and auth-handler
header additions. Skip the content-length dynamic-metadata restamp on the
CONTINUEbranch since the body length did not change.All existing
CONTINUE_AND_REPLACEcallers stay on the existing path (theseare the branches where this PR explicitly does not change behavior, including
the loss-of-earlier-mutation property documented above):
Vertex, etc.) —
bodyMutation != nil.HTTPBodyMutationconfigured —HasMutations() == true.forceBodyMutation == trueviau.onRetry().IncludeUsage=false—forceBodyMutation == true.newBodyviasjson.SetBytesOptions, sobodyMutation != nil.Tests:
Test_chatCompletionProcessorUpstreamFilter_ProcessRequestHeaders_BodyReplaceContractwith two subtests:
no translator body, no mutator, no force -> CONTINUEexercises thepreviously untested path. Asserts
Status == CONTINUE, noBodyMutation,preserved translator and auth-handler header mutations, and
DynamicMetadata == nil(no content-length restamp).translator body present -> CONTINUE_AND_REPLACEpins the existingbehavior so a future refactor cannot quietly flip the contract back.
The coverage gap that let this ship: every existing
t.Run("ok", ...)subtestunder
Test_chatCompletionProcessorUpstreamFilter_ProcessRequestHeaderssetretBodyMutationon the mock translator, so the "translator returns nil body"path was never exercised.
Related Issues/PRs (if applicable)
None.
Special notes for reviewers (if applicable)
NewBodyMutator's contract is intentionally not changed (it still returnsnon-nil even when given a nil
*filterapi.HTTPBodyMutation). A predicate isadded instead, to keep the blast radius local to
ProcessRequestHeaders. Ifreviewers prefer changing the constructor to return nil in the no-config
case, that is a larger refactor and a separate PR.
upstreamProcessor[...], so the fix applies toevery endpoint spec (chat completions, messages, embeddings, completions,
GCP, AWS Bedrock, etc.). The new tests exercise one endpoint spec for the
same reason the existing
ProcessRequestHeaderstable-test does — lockingthe generic method's contract.