filters: improve docs & protections around callbacks

[rgs1]

We recently ran into an internal problem where an in-house filter was returning a local reply and immediately after returning FilterHeaderStatus::Continue, which ends up confusing the Router filter machinery.

A few things can be done to improve the experience for filter developers:

1. improve docs around the contract with filters (e.g.: filter: clarify comment #13678, I'll send another one for docs/root/faq/extensions/contract.rst as well)

2. enforce more checks to ensure callbacks cannot be called after a local reply has been sent

3. more assertions around the filter chain state machine

cc: @alyssawilk @mattklein123

[alyssawilk]

cc @snowp who has been tweaking the filter manager logic most recently.
I wonder if there's simple checks and error handling we can put in there to catch continue-after-end-stream

[snowp]

I think we could set a flag (or ASSERT on an existing flag?) that prevents filter iteration after we've issued a local reply. Conceptually I think any filter that issues a local reply is presenting the full response for this stream, so continuation at this point is unnecessary.

[mattklein123]

+1 I think we should have more ASSERTS around bad behavior. We can start there and then potentially think about more robust error handling to protect against bad filters. I know that @asraa and @yanavlasov have been thinking about this issue in the context of header removal.

[rgs1]

Also relevant: #13873

[rgs1]

Hmm I was just debugging a filter used a long with direct responses and noted that:

[2020-12-08 00:36:34.568][34][debug][filter] [pinterest/filters/http/test/filter.cc:111] TestFilter::decodeHeaders
[2020-12-08 00:36:34.568][34][debug][http] [external/envoy/source/common/http/filter_manager.cc:783] [C134124][S4840569554702352551] Sending local reply with details direct_response
[2020-12-08 00:36:34.568][34][debug][filter] [pinterest/filters/http/test/filter.cc:229] TestFilter::encodeHeaders

after a direct response in a route match, encodeHeaders() for the filter still ran.

So filter writers should explicitly check if they are within a route->directResponseEntry() before sending yet another local response?

cc: @mattklein123

[mattklein123]

I think running encodeHeaders() for a direct response makes sense. How is this different from any other response that is received from upstream from the perspective of your filter?

[rgs1]

I think running encodeHeaders() for a direct response makes sense. How is this different from any other response that is received from upstream from the perspective of your filter?

When running encodeHeaders you could, in theory, still return a local response and decide to ignore the upstream response from your filter. Whereas from a DR it's too late, because the local response already happened after the decodeHeaders() calls (per the above log)... no?

[mattklein123]

If that's the case I would call that a bug and not expected behavior. It seems like it should be able to modify a local response of a filter that ran before the current filter.

[rgs1]

Opened #14346 to follow-up on direct responses behavior wrt decodeData().

[rgs1]

This is somewhat related, so commenting here but happy to move to a new issue if that makes more sense.

So, we got bitten by another part of the filter contract that could use some extra protection. From https://github.com/envoyproxy/envoy/blame/main/source/docs/flow_control.md:

Each HTTP and HTTP/2 filter has an opportunity to call `decoderBufferLimit()` or
`encoderBufferLimit()` on creation. No filter should buffer more than the
configured bytes without calling the appropriate watermark callbacks or sending
an error response.

Would it make sense to make addEncodedData() return false or something if a 413 response
has been emitted so the filter can give up without worrying about doing the buffer limit check
and emitting a local response? There's already this:

https://github.com/envoyproxy/envoy/blob/main/source/common/http/filter_manager.cc#L1535

to handle the buffer limit check and local response so no reason really to repeat that work in the filter...

cc: @alyssawilk @snowp

[alyssawilk]

yeah, I think changing the return value makes sense there