Add an explicit state for whether filter chain should continue or not.

[saumoh]

Fix for ext_authz filter crash

Description:
This is a fix for #3259
The problem was when the ext_authz filter's gRPC cluster is incorrect. In this case the onComplete callback is on the stack (with Error) but the filter chain continues to the next filter (router). That is incorrect. Instead the filter chain should be stopped from moving on to the next filter in the chain.

The fix is to explicitly track whether the filterchain should be allowed to continue or not.

Risk Level: Low

Testing:
Added UT's for the on-stack error return case.

Docs Changes: N/A

[Optional Fixes #Issue]
#3259

cc @gsagula @julia-stripe

[htuch]

Thanks for the fix. IMHO, we have a lot of complexity here, since we now have 3 state variables, and I can't see the clear relationship between them. The variables are filter_return_, initiating_call_ and state_. Can you write a comment explaining how they relate? Is there no way to simplify this?

[saumoh]

@htuch (cc @gsagula ) I added comments about these 3 states. I think it is possible to combine all of these and initially i had done some of that (see: #3259 (comment)). But then i found it harder to reason about all the states at the various locations that we use them.
I think its more important for others to read and understand the use of these states (and not just me) so any recommendations/suggestion from either of you would be quite helpful.

[gsagula]

@saumoh It looks clear to me. I will merge it with #3162 later tonight. Thanks!

[htuch]

@saumoh thanks, this is quite easy to understand now. Some additional questions:

1. Do you think there is any way to make it clearer to filter authors in documentation or comments regarding the semantics of stream termination (encode/decode) and how to manage the StopIteration returns to ensure this kind of blocking behavior when making RPCs and after?
2. Largely unrelated, but given some recent discussion in other PRs about it, would we benefit from using using sendLocalReply() in the HTTP filter?

[saumoh]

@htuch i am not sure about point 1 in your note. I'll have to think about it. Largely the filter chain return values are well documented (i feel). The async and synchronous duality of the call backs is a bit trick. If there was a abstraction that hid these for async callers then it would be useful for the general async usage.
For 2 yes. I think it would make sense to use sendLocalReply; Thanks for pointer as i wasn't aware of it. cc @gsagula since he will be relying on it more heavily and i am not sure if he's already doing it in his pr. And I can make the change to use it in this pr as well.

[gsagula]

Hi @saumoh, I'm no using sendLocalReply. Would you mind to provide some context on how it will affect the current implementation? I've just finished the HTTP client. Should be updating that PR soon. Thanks!

[saumoh]

@gsagula see

envoy/source/extensions/filters/http/ext_authz/ext_authz.cc

Line 105 in 6ae5612

callbacks_->sendLocalReply(Http::Code::Forbidden, "", nullptr);

It's a way to respond back from the proxy itself and add additional headers, body and text.

It was already added upstream :-)

[gsagula]

Awesome, thanks 👍
This means that I should merge master. Hopefully not many conflicts :)

[htuch]

Thanks.