ext_proc: allow immediate response from upstream filter#41231
ext_proc: allow immediate response from upstream filter#41231tyxia merged 6 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
mathetake
requested review from
mattklein123,
tyxia and
yanavlasov
as code owners
|
@yanjunxiang-google PTAL |
|
Thanks for working on this. Please add an integration test to verify the functionality works end-to-end. |
|
/wait Can you have a e2e integration test for this? Also could you also test and document how it will play with hedging / retries https://github.com/envoyproxy/envoy/blob/main/source/docs/upstream_filters.md Also, per comment here local reply is not seen by upstream http filter , e2e test here will help demonstrate how it works |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
@yanjunxiang-google added an integration test which works as expected (I also manually verified it's failing without the change in this PR juuust in case) |
| config_option.downstream_filter = false; | ||
|
|
||
| initializeConfig(config_option); | ||
| // Add ext_proc as upstream filter. |
There was a problem hiding this comment.
Now there are two tests are creating ext_proc filter in upstream:
. Probably making this as function and call it at both places?There was a problem hiding this comment.
548aadb done. JFYI the similar code exists in observability_integration(?) but not exactly the same so decided to limit the sharing in this file.
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
Will wait for @yanjunxiang-google LGTM. /wait-any |
|
LGTM |
|
@mathetake Don't want to block this PR, but have addressed this requirement: This is required here and i think it will make upstream filter more robust in the long term. At least, i think we should have a todo or warning comment |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
@tyxia added a commit to address you concern. |
| HttpIntegrationTest::initialize(); | ||
| auto response = sendDownstreamRequest(absl::nullopt); | ||
| auto response = sendDownstreamRequest([](Http::RequestHeaderMap& headers) { | ||
| // Setting this header to ensure that Envoy does not retry the request |
There was a problem hiding this comment.
Is this comment accurate?
Setting x-envoy-retry-on header ----> should the comment be: ensure that Envoy does retry the request in case of a 5xx immediate response from the upstream
There was a problem hiding this comment.
ah i mean yes and no lol. "Setting this header (which causes Envoy to retry usually but this case is ext_proc is the one sending immediate response so) to ensure Envoy won't result in retry" is the accurate comment
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
Nice! Thanks for working on this
4 participants
Commit Message: ext_proc: allow immediate response from upstream filter
Additional Description:
This is from the discussion here: #33273 (comment)
Previously, sending local replies from an upstream ext_proc filter was prohibited just because of the implementation choice. This prevented users from returning errors from there, which results in clients indefinitely waiting for the response until timeout without any feedback. In short, ext_proc was previously unable to fail.
Closes envoyproxy/ai-gateway#840
Risk Level: low (previously considered invalid code paths)
Testing: done
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a