HTTP filters that put after envoy.router don't work but envoy can still boot up with that configuration

[KenjiChao]

Title: HTTP filters that put after envoy.router don't work but envoy can still boot up with that configuration

Description:

I'm trying to add the buffer filter to the http filters list so that we can set max request size. What I observed is that if I put the buffer filter after envoy.router filter, I don't get a 413 response when sending a request with a large file, but envoy can still boot with that configuration. On the other hand, if I reverse the order and put envoy.router filter after the buffer filter, it works as expected.

It's weird to me that yaml configuration still compiles and envoy is able to boot up with that configuration while the router filter doesn't propagate requests to subsequent filters. I can't seem to find anything in the docs implying this behavior. At least I would expect the router filter doc be updated saying that it should be the last one in the http filters list.

Config:

The one that's not working:

...
                http_filters:
                  - name: envoy.router
                    typed_config: {}
                  - name: envoy.buffer
                    config:
                      max_request_bytes: 30000000
...

The one that's working:

...
                http_filters:
                  - name: envoy.buffer
                    config:
                      max_request_bytes: 30000000
                  - name: envoy.router
                    typed_config: {}
...

[snowp]

@mattklein123 Do you think it would make sense to add some kind of (maybe optional) validation that errors out if there are filters after a terminal filter? Or should we just document the requirements?

[mattklein123]

@snowp see #7779.

@alyssawilk in thinking about this a bit more, I wonder if we should do this:

1. Add a virtual function on the filter factory, isTerminalFilter() which defaults to false.
2. Set it to true for terminal filters
3. Base the checks you added on that? WDYT?

[alyssawilk]

Darn it, you called me on my slippery slope.
My main concern for this is that I'm not actually sure which filters are terminal other than tcp_proxy, router, and HCM. If you can advise there, I'll accept doing the less hacky solution :-)

[mattklein123]

@alyssawilk I think those three, and it's a pretty trivial change that then puts this in the hands of the filter writer, so it seems more future proof to me?

[snowp]

+1 on making it part of the API, I imagine the redis/thrift filters also fall into this category

[mattklein123]

Ah yes, redis/thrift also.

[mattklein123]

Also Dubbo

[alyssawilk]

This is where I knew I'd miss one :-P

OK, new PR coming up. Ironically I hadn't even seen this issue filed, I'd been commenting over on 7738 where the router was the first of three on the first pass, and second of three on their next try. So yeah, this comes up a bunch.