extproc: skip CONTINUE_AND_REPLACE when no body change is needed - #2170
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.72% 84.73% +0.01%
==========================================
Files 144 144
Lines 21172 21189 +17
==========================================
+ Hits 17938 17955 +17
Misses 2152 2152
Partials 1082 1082 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Correct - when |
nacx
left a comment
There was a problem hiding this comment.
Overall LGTM. @mathetake could you take a look as well?
|
/retest |
…oyproxy#2170) **Description** **Scope:** this PR fixes the narrow case where the upstream cluster ext_proc filter issued `CONTINUE_AND_REPLACE` even though *nothing in the request pipeline needed to mutate the body* — no translation, no model override, no retry, no backend `HTTPBodyMutation`. In that path, the upstream filter replayed `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 `CONTINUE` and leaves the body alone. The broader question of "preserve an intermediate filter's body mutation across translation / retries / `HTTPBodyMutation`" is **not** addressed by this PR. When `wantBodyReplace == true`, the replacement body is still derived from 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 `applyBodyMutation` looks like it covers the no-config case: if bodyMutator == nil { return bodyMutation } but it 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 — even though nothing asked for a body replacement. **Fix:** * Add `BodyMutator.HasMutations()` predicate. `NewBodyMutator`'s contract is unchanged (it still returns non-nil); callers can ask whether the mutator actually has anything to do. * In `upstreamProcessor.ProcessRequestHeaders`, compute mutatorHasMutations := u.bodyMutator != nil && u.bodyMutator.HasMutations() wantBodyReplace := bodyMutation != nil || forceBodyMutation || mutatorHasMutations before calling `applyBodyMutation`. When false, skip the body-mutation work and emit `CONTINUE` instead of `CONTINUE_AND_REPLACE`, preserving translator header mutations (e.g. path rewrite), 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 (these are the branches where this PR explicitly does not change behavior, including the loss-of-earlier-mutation property documented above): * Translator emits a body (AWS Bedrock/SigV4, OpenAI to Anthropic, OpenAI to Vertex, etc.) — `bodyMutation != nil`. * Backend `HTTPBodyMutation` configured — `HasMutations() == true`. * Retry — `forceBodyMutation == true` via `u.onRetry()`. * Streaming with `IncludeUsage=false` — `forceBodyMutation == true`. * Model override — translator emits `newBody` via `sjson.SetBytesOptions`, so `bodyMutation != nil`. **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 `DynamicMetadata == nil` (no content-length restamp). * `translator body present -> CONTINUE_AND_REPLACE` pins the 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. **Related Issues/PRs (if applicable)** None. **Special notes for reviewers (if applicable)** * The fix is request-side only. Response-side code is untouched. * `NewBodyMutator`'s contract is intentionally not changed (it still returns non-nil even when given a nil `*filterapi.HTTPBodyMutation`). A predicate is added instead, to keep the blast radius local to `ProcessRequestHeaders`. If reviewers prefer changing the constructor to return nil in the no-config case, that is a larger refactor and a separate PR. * The bug is in the generic `upstreamProcessor[...]`, so the fix applies to every endpoint spec (chat completions, messages, embeddings, completions, GCP, AWS Bedrock, etc.). The new tests exercise one endpoint spec for the same reason the existing `ProcessRequestHeaders` table-test does — locking the generic method's contract. Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io> Signed-off-by: yxia216 <yxia216@bloomberg.net>
Resolve conflict in internal/bodymutator/body_mutator.go: adopt main's HasMutations() semantics (true when an HTTPBodyMutation config is present), which correctly covers both the request path (Set + Remove, per envoyproxy#2170's CONTINUE_AND_REPLACE short-circuit) and the response path (Remove-only). The PR's prior 'len(Remove) > 0' narrowing would have silently disabled Set-only request mutations. Signed-off-by: Nicolas <nl@blackfuel.ai>
…oyproxy#2170) **Description** **Scope:** this PR fixes the narrow case where the upstream cluster ext_proc filter issued `CONTINUE_AND_REPLACE` even though *nothing in the request pipeline needed to mutate the body* — no translation, no model override, no retry, no backend `HTTPBodyMutation`. In that path, the upstream filter replayed `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 `CONTINUE` and leaves the body alone. The broader question of "preserve an intermediate filter's body mutation across translation / retries / `HTTPBodyMutation`" is **not** addressed by this PR. When `wantBodyReplace == true`, the replacement body is still derived from 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 `applyBodyMutation` looks like it covers the no-config case: if bodyMutator == nil { return bodyMutation } but it 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 — even though nothing asked for a body replacement. **Fix:** * Add `BodyMutator.HasMutations()` predicate. `NewBodyMutator`'s contract is unchanged (it still returns non-nil); callers can ask whether the mutator actually has anything to do. * In `upstreamProcessor.ProcessRequestHeaders`, compute mutatorHasMutations := u.bodyMutator != nil && u.bodyMutator.HasMutations() wantBodyReplace := bodyMutation != nil || forceBodyMutation || mutatorHasMutations before calling `applyBodyMutation`. When false, skip the body-mutation work and emit `CONTINUE` instead of `CONTINUE_AND_REPLACE`, preserving translator header mutations (e.g. path rewrite), 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 (these are the branches where this PR explicitly does not change behavior, including the loss-of-earlier-mutation property documented above): * Translator emits a body (AWS Bedrock/SigV4, OpenAI to Anthropic, OpenAI to Vertex, etc.) — `bodyMutation != nil`. * Backend `HTTPBodyMutation` configured — `HasMutations() == true`. * Retry — `forceBodyMutation == true` via `u.onRetry()`. * Streaming with `IncludeUsage=false` — `forceBodyMutation == true`. * Model override — translator emits `newBody` via `sjson.SetBytesOptions`, so `bodyMutation != nil`. **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 `DynamicMetadata == nil` (no content-length restamp). * `translator body present -> CONTINUE_AND_REPLACE` pins the 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. **Related Issues/PRs (if applicable)** None. **Special notes for reviewers (if applicable)** * The fix is request-side only. Response-side code is untouched. * `NewBodyMutator`'s contract is intentionally not changed (it still returns non-nil even when given a nil `*filterapi.HTTPBodyMutation`). A predicate is added instead, to keep the blast radius local to `ProcessRequestHeaders`. If reviewers prefer changing the constructor to return nil in the no-config case, that is a larger refactor and a separate PR. * The bug is in the generic `upstreamProcessor[...]`, so the fix applies to every endpoint spec (chat completions, messages, embeddings, completions, GCP, AWS Bedrock, etc.). The new tests exercise one endpoint spec for the same reason the existing `ProcessRequestHeaders` table-test does — locking the generic method's contract. Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io> Signed-off-by: yxia216 <yxia216@bloomberg.net>
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.