http: delaying filter stack creation until full headers have been received#3574
http: delaying filter stack creation until full headers have been received#3574alyssawilk merged 2 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Matt, if you think this is a reasonable point to delay filter creation, could someone on your side canary this? I'm reaaally surprised this didn't crash anything. Alternately feel free to suggest and and all additional testing. |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
@alyssawilk I actually expected it to be about this easy, so I'm not surprised nothing crashed. I have a question though. Once we decide on the proposed implementation I'm happy to do a quick smoke test.
| void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { | ||
| maybeEndDecode(end_stream); | ||
| request_headers_ = std::move(headers); | ||
| lazilyCreateFilterChain(); |
There was a problem hiding this comment.
Q: Why do we need the lazy init functionality here? IIRC this function will only be called once? Also, I think we will eventually want to do this lower down in this function once we decide if it's WS or not? Should we position it in the final place now? Perhaps above here:
envoy/source/common/http/conn_manager_impl.cc
Line 696 in 95d6286
(The downside of putting it there is any local replies in this function would not go through encoding filters... which I guess is an issue, but it seems like if we instantiate here we would need to blow away and recreate for WS which seems worse? Thoughts?)
There was a problem hiding this comment.
So for local replies, there's a TODO to start encoding from the last filter which saw the request. If in the long run we want filters to only see the response if they saw the request I think this is fine. The other option is we can lazily create the chain on any local reply for consistency until we fix that TODO. I'd assumed we'd have other entry points which is one reason I made it a function :-)
I'd prefer we create the filter chain early on in the function. I'd planned on lazilyCreateFilterChain being refactored to look at request_headers_ (and config, to see if the new path is configured) then deciding if it should create websocket or http filter chain, which is why I stuck it immediately after we latch request_headers_. Happy to move it if you think that's better but I would like to land it without the refactor just to keep things as low risk as possible :-)
There was a problem hiding this comment.
If you think you are going to put inspection logic in the creation function, I think it's fine to keep it where it is. My preference though is to remove the lazy creation logic though until we actually need it? So maybe just a function callout with a comment about lazy creation, etc.?
There was a problem hiding this comment.
Sorry, are you just asking that we create the filter chain when this function is called, kill off the boolean, and just rename it to createFilterChain? Or to also keep the function and call it where things are created now?
I'm working on the websocket inspection right PR now. I'd hoped to land the deferred creation as a standalone as I saw it as higher risk, but I'm up for whatever
There was a problem hiding this comment.
Sorry, are you just asking that we create the filter chain when this function is called, kill off the boolean, and just rename it to createFilterChain?
Yes that's what I mean. Unless I'm missing something, in the current code the lazy creation checking logic will never be exercised (we will only call the creation function once).
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM. Honestly I think this is safe to merge, but I'm happy to smoke test if you want.
…been received (envoyproxy#3574)" This reverts commit 1d14b9e. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
…rs have been received (envoyproxy#3574)" (envoyproxy#3590)" This reverts commit 28cea91. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
2 participants
This is a precursor to creating a custom filter stack for websocket vs HTTP.
Risk Level: High
All of the integration test pass. I refuse to believe it is this easy.
Testing: suggestions appreciated
Docs Changes: n/a
Release Notes: should we call out we won't create the filter chain for super early errors?
Part of #3301