Fix an issue with ext_proc filter configured in the upstream filter chain#41879
Fix an issue with ext_proc filter configured in the upstream filter chain#41879botengyao merged 12 commits intoenvoyproxy:mainfrom
Conversation
chain Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
yanjunxiang-google
requested review from
mattklein123,
tyxia and
yanavlasov
as code owners
|
/assign @yanavlasov @adisuissa @botengyao |
phlax
left a comment
phlax
left a comment
There was a problem hiding this comment.
could you add a changelog please
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
…eam_route_retry Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
…eam_route_retry Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
| // and it's encodeHeaders() is returning FilterHeadersStatus::StopIteration. | ||
| // In this case, onUpstreamData() might get called with | ||
| // upstream_requests_.size() == 0 and we should just return. | ||
| if (upstream_requests_.size() != 1) { |
There was a problem hiding this comment.
The original assertion implies the value must be 1, whereas the comment says it could be 0 or 1.
If that's the case, and assuming that 0 is a valid value, then it may be better to do the following:
// Explain when the size can be 0.
if (upstream_requests_.size() == 0) {
// Consider adding another assertion that captures when this can occur.
return;
}
// Explain why the size can only be 1 at this point.
ASSERT(upstream_requests_.size() == 1);
There was a problem hiding this comment.
Good idea, Done! For adding an assertion under size() == 0, my thought is that the upstream_request is already gone in that case, we probably should just return.
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
|
Would you be able to add a test to the https://github.com/envoyproxy/envoy/blob/main/test/integration/upstream_http_filter_integration_test.cc so that common functionality does not depend on a test in ext_proc extension, please? It is possible that https://github.com/envoyproxy/envoy/blob/main/test/integration/filters/encode_headers_return_stop_all_filter.cc could be used. /wait-any |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Good idea, done! |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
…eam_route_retry Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
kind ping |
|
/retest |
|
Kind ping! The Publish and verify CI failure seems not related to the PR. |
|
/retest |
|
verify issue is fixed on main - main merge would resolve - but we can just push past it i think |
…eam_route_retry Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
|
I think |
|
i think its fixed on main |
|
/retest |
…eam_route_retry Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
@phlax merge upstream still can not fix the CodeQL CI error. |
|
apologies - see #42106 - ill look at it first thing tomorrow |
…eam_route_retry Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
|
/retest |
1 similar comment
|
/retest |
|
Thanks @phlax ! The CI now passed. |
…upstream filter chain (envoyproxy#41879) Fixes an issue when an HTTP filter is in the upstream filter chain, and its encodeHeaders() method is returning StopIteration() , and route retry policy is configured with retry on 5xx. In this case, when a 503 response is received, onUpstreamHeaders() will be called first, and the UpstreamRequest object will be removed from the upstream_request_ list. onUpstreamData() will be called right after, and should just return. --------- Signed-off-by: Yanjun Xiang <yanjunxiang@google.com> Signed-off-by: Gustavo <grnmeira@gmail.com>
7 participants
Fixes an issue when an HTTP filter is in the upstream filter chain, and its encodeHeaders() method is returning StopIteration() , and route retry policy is configured with retry on 5xx. In this case, when a 503 response is received, onUpstreamHeaders() will be called first, and the UpstreamRequest object will be removed from the upstream_request_ list. onUpstreamData() will be called right after, and should just return.