http: allow any FilterHeaderStatus::Stop* after sending a local reply. #15496

PiotrSikora · 2021-03-15T19:23:59Z

Signed-off-by: Piotr Sikora piotrsikora@google.com

Fixes envoyproxy#14957. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

asraa

Thanks! No idea why this went out of my radar.

Went through some history, this would now match up with continue_iteration which returns false in all three of these cases so I think this is more consistent, and at least won't cause the bug I was trying to avoid (where it returns true, but local is complete)

envoy/source/common/http/filter_manager.cc

Line 143 in a240824

if (stoppedAll() || status == FilterHeadersStatus::StopIteration) {

Could we simplify it by making this change:

 const auto continue_iteration =
        (*entry)->commonHandleAfterHeadersCallback(status, end_stream);
ENVOY_BUG(continue_iteration && state_.local_complete_, "Filter did not return StopAll or StopIteration after sending a local reply");

and then adjust so that we still return early at local complete?

if ((!continue_iteration || state_.local_complete_) && std::next(entry) != decoder_filters_.end()) {

asraa · 2021-03-22T12:56:45Z

@rgs1

rgs1 · 2021-03-22T13:32:54Z

Ah interesting, so this didn't break for us because we always do StopIteration after a local reply. OOI, what's the use case for Stop and Buffer after a local reply?

…iteration Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-03-22T23:34:45Z

Could we simplify it by making this change:

const auto continue_iteration =
        (*entry)->commonHandleAfterHeadersCallback(status, end_stream);
ENVOY_BUG(continue_iteration && state_.local_complete_, "Filter did not return StopAll or StopIteration after sending a local reply");

and then adjust so that we still return early at local complete?

if ((!continue_iteration || state_.local_complete_) && std::next(entry) != decoder_filters_.end()) {

Done, with correct assert (!continue_iteration || !state_.local_complete_).

PiotrSikora · 2021-03-22T23:43:52Z

Ah interesting, so this didn't break for us because we always do StopIteration after a local reply. OOI, what's the use case for Stop and Buffer after a local reply?

The use case is the same for all 3 values - to stop request processing.

Some extensions (e.g. Wasm, ext_authz) already return FilterHeadersStatus::StopAllIterationAndWatermark since it provides better behavior than FilterHeadersStatus::StopIteration, and it incorrectly triggerred this ENVOY_BUG.

rgs1 · 2021-03-22T23:53:01Z

Ah interesting, so this didn't break for us because we always do StopIteration after a local reply. OOI, what's the use case for Stop and Buffer after a local reply?

The use case is the same for all 3 values - to stop request processing.

Some extensions (e.g. Wasm, ext_authz) already return FilterHeadersStatus::StopAllIterationAndWatermark since it provides better behavior than FilterHeadersStatus::StopIteration, and it incorrectly triggerred this ENVOY_BUG.

What does better behavior mean (besides not triggering ENVOY_BUG)? Wondering if there's any docs we could improve (maybe the filters contract doc), given that this stuff has bitten us a few times.

PiotrSikora · 2021-03-23T00:05:52Z

What does better behavior mean (besides not triggering ENVOY_BUG)? Wondering if there's any docs we could improve (maybe the filters contract doc), given that this stuff has bitten us a few times.

FilterHeadersStatus::StopIteration stops processing of HTTP request/response headers, but not of the HTTP request/response body, which means that if your extension returns FilterHeadersStatus::StopIteration from HTTP request headers (e.g. to consult external service), then it might be called to process HTTP request body (if such exists) even before receiving response from the external service and resuming processing of HTTP request headers. This doesn't follow HTTP flow and it's a pretty unusual behavior.

FilterHeadersStatus::StopAllIterationAnd{Buffer,Watermark} stop processing of both: HTTP request/response headers and body.

rgs1 · 2021-03-23T00:12:53Z

What does better behavior mean (besides not triggering ENVOY_BUG)? Wondering if there's any docs we could improve (maybe the filters contract doc), given that this stuff has bitten us a few times.

FilterHeadersStatus::StopIteration stops processing of HTTP request/response headers, but not of the HTTP request/response body, which means that if your extension returns FilterHeadersStatus::StopIteration from HTTP request headers (e.g. to consult external service), then it might be called to process HTTP request body (if such exists) even before receiving response from the external service and resuming processing of HTTP request headers. This doesn't follow HTTP flow and it's a pretty unusual behavior.

FilterHeadersStatus::StopAllIterationAnd{Buffer,Watermark} stop processing of both: HTTP request/response headers and body.

... but none of that should matter after a local reply, in that case they should all be the same? Which takes me back to my original question, after a local reply the most idiomatic thing would be a plain StopIteration given that the behavior is the same across Stop* once a local reply was sent.

PiotrSikora · 2021-03-23T00:24:32Z

... but none of that should matter after a local reply, in that case they should all be the same? Which takes me back to my original question, after a local reply the most idiomatic thing would be a plain StopIteration given that the behavior is the same across Stop* once a local reply was sent.

Right, it doesn't really matter which value do you return in this particular case... Although, I'd argue that the plain StopIteration shouldn't be the most idiomatic thing, considering that the intent is to stop all processing.

However, my point was that because of this weird behavior, we completely removed StopIteration from the Wasm APIs, so users have no way (or reason, really) to return it after sending a local reply.

rgs1 · 2021-03-23T00:39:20Z

... but none of that should matter after a local reply, in that case they should all be the same? Which takes me back to my original question, after a local reply the most idiomatic thing would be a plain StopIteration given that the behavior is the same across Stop* once a local reply was sent.

Right, it doesn't really matter which value do you return in this particular case... Although, I'd argue that the plain StopIteration shouldn't be the most idiomatic thing, considering that the intent is to stop all processing.

However, my point was that because of this weird behavior, we completely removed StopIteration from the Wasm APIs, so users have no way (or reason, really) to return it after sending a local reply.

Ah, I see -- that makes sense. Linking this to #13737 for potential follow-ups on docs & comments.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

asraa

Thanks! LGTM (and thanks for the explanations)

asraa · 2021-03-23T13:49:45Z

@envoyproxy/senior-maintainers For a quick merge (and FYI)

alyssawilk

Good catch - thanks for fixing this one

source/common/http/filter_manager.cc

…iteration

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2021-03-24T05:02:36Z

/retest

repokitteh-read-only · 2021-03-24T05:02:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15496 (comment) was created by @PiotrSikora.

see: more, trace.

alyssawilk

one last question, otherwise good to go

source/common/http/filter_manager.cc

alyssawilk · 2021-03-24T15:41:10Z

source/common/http/filter_manager.cc

-        (*entry)->commonHandleAfterHeadersCallback(status, end_stream) && !state_.local_complete_;
+    const auto continue_iteration = (*entry)->commonHandleAfterHeadersCallback(status, end_stream);
+    ENVOY_BUG(!continue_iteration || !state_.local_complete_,
+              "Filter did not return StopAll or StopIteration after sending a local reply.");

    // If this filter ended the stream, decodeComplete() should be called for it.
    if ((*entry)->end_stream_) {


Do we really want to do any of this work (handle new metadata, add decoded data, etc).
Would it make more sense to early return or does that break something else?

I honestly don't know the codebase well enough to answer that. Commenting out maybeContinueDecoding() below breaks a bunch of tests, but I don't know if those are real failures or tests that are not representative of the production code.

In any case, I don't think such risky change should be a part of this bugfix PR.

given it's unlikely folks will be doing sendLocalReply and add decoded data / metadata I buy that as an argument.

Would you instead either throw a TODO my way or, if you want to avoid another round of CI, open a tracking issue to look into it?

Let's skip the 3h wait on the CI, I filled #15654.

mattklein123

Thanks LGTM other than Alyssa's remaining comment.

alyssawilk

LGTM. Please do fill out release notes (risk, tests etc) next time!

PiotrSikora · 2021-05-26T21:24:18Z

/backport

http: allow any FilterHeaderStatus::Stop* after sending a local reply.

6d1e6a0

Fixes envoyproxy#14957. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora requested a review from asraa March 15, 2021 19:23

PiotrSikora mentioned this pull request Mar 15, 2021

Make ENVOY_BUG only log critical in official debug builds #14957

Closed

jmarantz assigned asraa Mar 16, 2021

mathetake mentioned this pull request Mar 18, 2021

Is it possible reject http request OnHttpRequestHeaders tetratelabs/proxy-wasm-go-sdk#122

Closed

asraa reviewed Mar 22, 2021

View reviewed changes

PiotrSikora added 2 commits March 22, 2021 22:11

Merge remote-tracking branch 'origin/main' into PiotrSikora/fix_stop_…

a779c3c

…iteration Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: address comments from Asra.

71fba33

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora requested a review from asraa March 22, 2021 23:33

reviwe: reorder to fix metadata processing.

0062c85

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

asraa previously approved these changes Mar 23, 2021

View reviewed changes

alyssawilk reviewed Mar 23, 2021

View reviewed changes

source/common/http/filter_manager.cc Show resolved Hide resolved

alyssawilk self-assigned this Mar 23, 2021

mattklein123 added the waiting:any label Mar 23, 2021

PiotrSikora added 2 commits March 24, 2021 00:23

Merge remote-tracking branch 'origin/main' into PiotrSikora/fix_stop_…

6b9bf28

…iteration

review: add test with headers and body.

be883a1

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora dismissed asraa’s stale review via be883a1 March 24, 2021 02:46

repokitteh-read-only bot removed the waiting:any label Mar 24, 2021

alyssawilk assigned mattklein123 Mar 24, 2021

alyssawilk reviewed Mar 24, 2021

View reviewed changes

mattklein123 approved these changes Mar 24, 2021

View reviewed changes

PiotrSikora mentioned this pull request Mar 24, 2021

Skip unnecssary work after sending a local reply. #15654

Closed

alyssawilk approved these changes Mar 24, 2021

View reviewed changes

alyssawilk merged commit f7bbd31 into envoyproxy:main Mar 24, 2021

bladedancer mentioned this pull request Mar 31, 2021

ext_proc: Returning StopAllIterationAndWatermark prevents whole request processing #15793

Closed

repokitteh-read-only bot added the backport/review Request to backport to stable releases label May 26, 2021

PiotrSikora mentioned this pull request May 26, 2021

Change callbacks to return specific response types proxy-wasm/proxy-wasm-rust-sdk#39

Closed

phlax removed the backport/review Request to backport to stable releases label Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: allow any FilterHeaderStatus::Stop* after sending a local reply. #15496

http: allow any FilterHeaderStatus::Stop* after sending a local reply. #15496

PiotrSikora commented Mar 15, 2021

asraa left a comment

asraa commented Mar 22, 2021

rgs1 commented Mar 22, 2021

PiotrSikora commented Mar 22, 2021

PiotrSikora commented Mar 22, 2021

rgs1 commented Mar 22, 2021

PiotrSikora commented Mar 23, 2021

rgs1 commented Mar 23, 2021

PiotrSikora commented Mar 23, 2021

rgs1 commented Mar 23, 2021

asraa left a comment

asraa commented Mar 23, 2021

alyssawilk left a comment

PiotrSikora commented Mar 24, 2021

repokitteh-read-only bot commented Mar 24, 2021

alyssawilk left a comment

alyssawilk Mar 24, 2021

PiotrSikora Mar 24, 2021

alyssawilk Mar 24, 2021

PiotrSikora Mar 24, 2021

mattklein123 left a comment

alyssawilk left a comment

PiotrSikora commented May 26, 2021

http: allow any FilterHeaderStatus::Stop* after sending a local reply. #15496

http: allow any FilterHeaderStatus::Stop* after sending a local reply. #15496

Conversation

PiotrSikora commented Mar 15, 2021

asraa left a comment

Choose a reason for hiding this comment

asraa commented Mar 22, 2021

rgs1 commented Mar 22, 2021

PiotrSikora commented Mar 22, 2021

PiotrSikora commented Mar 22, 2021

rgs1 commented Mar 22, 2021

PiotrSikora commented Mar 23, 2021

rgs1 commented Mar 23, 2021

PiotrSikora commented Mar 23, 2021

rgs1 commented Mar 23, 2021

asraa left a comment

Choose a reason for hiding this comment

asraa commented Mar 23, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

PiotrSikora commented Mar 24, 2021

repokitteh-read-only bot commented Mar 24, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk Mar 24, 2021

Choose a reason for hiding this comment

PiotrSikora Mar 24, 2021

Choose a reason for hiding this comment

alyssawilk Mar 24, 2021

Choose a reason for hiding this comment

PiotrSikora Mar 24, 2021

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

PiotrSikora commented May 26, 2021