http: fixing connection close behavior #10957
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
source/common/http/header_utility.cc
Outdated
| // Keep-Alive is present. | ||
| if (protocol == Protocol::Http10 && | ||
| (!request_headers.Connection() || | ||
| !Envoy::StringUtil::caseFindToken(request_headers.Connection()->value().getStringView(), ",", |
There was a problem hiding this comment.
Hmm, the standard is a bit fuzzy, but it does not seem to say that hop-by-hop headers must be listed in the Connection header. So is it possible that Keep-Alive header is present but not listed in Connection? Do we even care about this case?
There was a problem hiding this comment.
AFIK the tokens keep-alive and close are special, and can be present in Connection without indicating a hop by hop header. I don't think I've ever sent keep-alive sent as a header, so I'm inclined to let this stand unless someone sends a feature request :-P
| // Both saw_connection_close_ and is_head_request_ affect local replies: set | ||
| // them as early as possible. | ||
| Protocol protocol = connection_manager_.codec_->protocol(); | ||
| bool fixed_connection_close = |
There was a problem hiding this comment.
The behavior seems to have only changes for H/1.0. Would it be easier to move the runtime flag check into the shouldCloseConnection() and then get rid of the if (!fixed_connection_close ...) cases below?
| @@ -2525,6 +2526,103 @@ TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationCallbackResetStream) { | |||
| EXPECT_EQ(1U, stats_.named_.downstream_rq_rx_reset_.value()); | |||
| } | |||
|
|
|||
| TEST_F(HttpConnectionManagerImplTest, Http10Rejected) { | |||
| setup(false, ""); | |||
| RequestDecoder* decoder = nullptr; | |||
There was a problem hiding this comment.
decoder seems to be unused.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Oops looks like this needs a master merge. Can you do that and I will take a look? /wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| Protocol protocol = connection_manager_.codec_->protocol(); | ||
| bool fixed_connection_close = |
| HeaderUtility::shouldCloseConnection(protocol, *request_headers_); | ||
| } | ||
| if (Http::Headers::get().MethodValues.Head == | ||
| request_headers_->Method()->value().getStringView()) { |
There was a problem hiding this comment.
(Moved code) This has come up in other reviews but I think we are being inconsistent about whether we check Method for null or not. I'm fine either way but I think we should be consistent about this and/or have an ASSERT?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| const bool fixed_connection_close = | ||
| Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fixed_connection_close"); | ||
| if (fixed_connection_close) { | ||
| state_.saw_connection_close_ = |
There was a problem hiding this comment.
nit: s/saw_connection_close_/should_close_connection_ or something since I think the meaning of this over time is now different?
| Headers::get().ConnectionValues.Close)))) { | ||
| parent_.parent_.host_->cluster().stats().upstream_cx_close_notify_.inc(); | ||
| close_connection_ = true; | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fixed_connection_close")) { |
There was a problem hiding this comment.
If we are going to runtime guard this can we add tests for both cases for the conn pool and health checker?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
LGTM but needs master merge. /wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Fixing "Connection: Close" behavior for HTTP/1.x responses for correctness.
Now closing the connection on early HTTP/1.0 responses, or if incoming headers have the "close" token mixed with other Connection or Proxy-Connection tokens.
These changes are applied to the HttpConnectionManager, HTTP/1.1 connection pool and HTTP/1.1. health checker.
Risk Level: Medium (L7 changes)
Testing: New unit tests, fixed integration test
Docs Changes: n/a
Release Notes: yes
Runtime guard: envoy.reloadable_features.fixed_connection_close