ext_proc: convert ext_proc filter into dual filter#33273
ext_proc: convert ext_proc filter into dual filter#33273yanavlasov merged 11 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
yanjunxiang-google
requested review from
htuch,
mattklein123 and
yanavlasov
as code owners
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/assign @tyxia @htuch @yanavlasov |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
The CI coverage complaints in factory_base.h is addressed by tests added in #33013. Will do a upstream merge after that PR is committed. |
|
/wait |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
Kind ping! |
|
King Ping! |
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
Thanks for working on this! (sorry for late review)
Could you also add a simple test upstream callout filter in next PR? It is good to have test for upstream match_delegate and composite + upstream callout filter in OSS in case this feature is accidentally broken by someone.
|
For the match_delegate + composite + ext_proc, this involves extension test refer another extension. I would like to avoid doing this in OSS. |
What I actually suggested is match_delegate + composite + test callout filter, which exactly try to avoid cross-reference |
|
Got it! Will add a test like that in next PR. |
--------- Signed-off-by: Yanjun Xiang <yanjunxiang@google.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
--------- Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
| if (config_->isUpstream()) { | ||
| stats_.send_immediate_resp_upstream_ignored_.inc(); | ||
| ENVOY_LOG(debug, "Ignoring send immediate response when ext_proc filter is in upstream"); |
There was a problem hiding this comment.
Hi, sorry for commenting on the old issue but what was the rationale for this? I actually hit this case, and would like to understand if this is the fundamental limitation of the envoy core level or it's just an extension side implementation matter. @yanjunxiang-google
There was a problem hiding this comment.
I went ahead and remove these lines, and tried it out for our use case. It works totally fine. Can I go ahead and remove this restriction, otherwise upstream filter extproc cannot fail literally as it will result in a timeout on the client side as well as this is not a documented behavior as far as i understand. cc @yanavlasov
There was a problem hiding this comment.
Hi @yanjunxiang-google @yanavlasov -- sorry to bump here but can you take a look at my comments above?
There was a problem hiding this comment.
Oh, that's an implementation choice . The goal for configuration ext_proc in upstream back then is just for ext_proc server logging purpose, and not planned for receiving immediate response. If you need it to be supported, please feel free to raise a PR.
There was a problem hiding this comment.
cool, thank you for clarifying!!! will do soon
There was a problem hiding this comment.
I think this is not the decision choice. local reply in upstream filter chain needs extra cautions and test/documentation
source/docs/upstream_filters.md
There was a problem hiding this comment.
Also, local reply is not seen by upstream http filter
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 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
6 participants
This PR converts ext_proc filter into dual filter so it can be created in the upstream filter chain.
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]