Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decodeData() isn't call when using a direct response #14346

Closed
rgs1 opened this issue Dec 9, 2020 · 16 comments
Closed

decodeData() isn't call when using a direct response #14346

rgs1 opened this issue Dec 9, 2020 · 16 comments
Labels
question Questions that are neither investigations, bugs, nor enhancements

Comments

@rgs1
Copy link
Member

rgs1 commented Dec 9, 2020

We have the following routes config for an Envoy instance meant to act as /dev/null:

              "route_config": {
                "name": "routes",
                "virtual_hosts": [
                  {
                    "domains": [
                      "*"
                    ],
                    "name": "test",
                    "routes": [
                      {
                        "direct_response": {
                          "status": 200
                        },
                        "match": {
                          "prefix": "/"
                        }
                      }
                    ]
                  }
                ]
              },

Along with the above, we also have a filter that implements decodeHeaders() and decodeData() to analyze the incoming requests. However, we only see decodeHeaders() being called:

[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

Possible options to work around this:

a) Fix the way we handle direct responses to ensure decodeData() is called too (currently it apparently emits an early local response after decodeHeaders() has been called and then stops the filter chain)

b) Add docs stating that if you want to do the above, you should setup another listener with a direct response and use that as the upstream for the first (or some other similar trick, which sounds too convoluted tbh)

Thoughts?

cc: @alyssawilk @mattklein123

Somewhat related to #13737.

@rgs1 rgs1 added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Dec 9, 2020
@alyssawilk
Copy link
Contributor

Just one check before I dig into this, it looks like you didn't have a direct response body configured. Have you tried adding that and seeing if it fixes your problem? At first glance I wouldn't expect decodeData to be called for a headers only response.

@rgs1
Copy link
Member Author

rgs1 commented Dec 9, 2020

Ah! Let me try that.

@mattklein123
Copy link
Member

Do you mean encodeData()? I'm confused.

@rgs1
Copy link
Member Author

rgs1 commented Dec 9, 2020

Do you mean encodeData()? I'm confused.

you == Alyssa or me?

I meant decodeData(), per the above log excerpt. But now that you mention this, I actually not sure how adding a body response to DR would trigger decodeData() into running....

@alyssawilk
Copy link
Contributor

yeah, I had encode data and decode swapped, sorry, sigh. Let me actually take a look

@mattklein123
Copy link
Member

Yeah I'm asking you. :)

I'm confused about the issue you are reporting. Why would decodeData run here? I wouldn't expect encodeData to run either without a body.

@rgs1
Copy link
Member Author

rgs1 commented Dec 9, 2020

Yeah I'm asking you. :)

I'm confused about the issue you are reporting. Why would decodeData run here? I wouldn't expect encodeData to run either without a body.

If it's a POST request and I have a filter running that inspects the body... why would that not run just because it matches a route that is configured to have a direct response? That sounds counter intuitive no?

@alyssawilk
Copy link
Contributor

Yeah, so to make things clear,
when the router decodes the header, it immediately looks to see what the action is. If there's a direct response configured, it immediately starts sending the response, and calls StopIteration on the request pipeline. It's basically treated like a local reply, where we assume it's not worth processing the POST body. If you folks have a need for the body to pass through the filter chain, that'd take some work and probably need to be a config option since halting processing makes sense for the common case.

@rgs1
Copy link
Member Author

rgs1 commented Dec 9, 2020

Yeah I'm asking you. :)
I'm confused about the issue you are reporting. Why would decodeData run here? I wouldn't expect encodeData to run either without a body.

If it's a POST request and I have a filter running that inspects the body... why would that not run just because it matches a route that is configured to have a direct response? That sounds counter intuitive no?

Ok, maybe not that counter intuitive given the body is going to /dev/null.... Unless you have a filter that's doing inspection stuff (think a WAF).

@rgs1
Copy link
Member Author

rgs1 commented Dec 9, 2020

Yeah, so to make things clear,
when the router decodes the header, it immediately looks to see what the action is. If there's a direct response configured, it immediately starts sending the response, and calls StopIteration on the request pipeline. It's basically treated like a local reply, where we assume it's not worth processing the POST body. If you folks have a need for the body to pass through the filter chain, that'd take some work and probably need to be a config option since halting processing makes sense for the common case.

We do have a use case... but I am starting to think it might be a bit too contrived and maybe we shouldn't be using DR for it 🤔

@mattklein123
Copy link
Member

If you put the buffer filter in front of your filter, decodeData() will run. You may or may not be able to do that in your scenario.

@rgs1
Copy link
Member Author

rgs1 commented Dec 9, 2020

That's great, let me try that. Thanks!

@rgs1
Copy link
Member Author

rgs1 commented Dec 9, 2020

Adding the buffer filter did not do make decodeData() run....:

             {
              "name": "envoy.filters.http.buffer",
              "typed_config": {
               "@type": "type.googleapis.com/envoy.config.filter.http.buffer.v2.Buffer",
               "max_request_bytes": 209715200
              }
             },
             {
              "name": "pinterest.filters.http.test",
              "typed_config": {
               "@type": "type.googleapis.com/pinterest.http.test.TestFilter",
              }
             },
             {
              "name": "envoy.filters.http.router"
             }

@dio dio added help wanted Needs help! and removed triage Issue requires triage labels Dec 9, 2020
@mattklein123 mattklein123 added question Questions that are neither investigations, bugs, nor enhancements and removed enhancement Feature requests. Not bugs or questions. help wanted Needs help! labels Dec 9, 2020
@mattklein123
Copy link
Member

Oh hmm, you are right, sorry. The router will respond right away even if the data is buffered.

You could potentially buffer the response in your filter if the request is not complete?

@alyssawilk
Copy link
Contributor

yeah, I think you'd have to subclass the buffer filter, and block the headers from going to the router until the buffer filter got end stream. As soon as the router sees the headers it's all over :-)

@rgs1
Copy link
Member Author

rgs1 commented Dec 10, 2020

Alright we went with forwarding to a local process that acts as /dev/null for HTTP. Thanks nonetheless!

P.S.: an envoy.router filter replacement that acts as /dev/null could be fun/useful...

@rgs1 rgs1 closed this as completed Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Questions that are neither investigations, bugs, nor enhancements
Projects
None yet
Development

No branches or pull requests

4 participants