From 53fbef6fd5e4b36d16a9c3ee2f3ae015e6b15729 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Sat, 6 Feb 2021 00:04:32 -0500 Subject: [PATCH 1/9] cookie and set-cookie Signed-off-by: Dan Zhang --- include/envoy/http/header_map.h | 3 +- source/common/http/header_map_impl.cc | 4 ++- .../quic_listeners/quiche/envoy_quic_utils.cc | 3 +- test/common/http/header_map_impl_test.cc | 14 +++++++++ .../quiche/envoy_quic_server_stream_test.cc | 8 +++++ .../integration/quic_http_integration_test.cc | 30 +++++++++++++++++++ 6 files changed, 59 insertions(+), 3 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index e3262e3ecfb0d..d0c43d50b87d8 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -294,7 +294,8 @@ class HeaderEntry { HEADER_FUNC(Protocol) \ HEADER_FUNC(Scheme) \ HEADER_FUNC(TE) \ - HEADER_FUNC(UserAgent) + HEADER_FUNC(UserAgent) \ + HEADER_FUNC(Cookie) /** * Default O(1) response headers. diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 6a4c00e5663ed..56c60de08c12b 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -362,8 +362,10 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) { if (*lookup.value().entry_ == nullptr) { maybeCreateInline(lookup.value().entry_, *lookup.value().key_, std::move(value)); } else { + const std::string delimiter = + (*lookup.value().key_ == Http::Headers::get().Cookie ? "; " : ","); const uint64_t added_size = - appendToHeader((*lookup.value().entry_)->value(), value.getStringView()); + appendToHeader((*lookup.value().entry_)->value(), value.getStringView(), delimiter); addSize(added_size); value.clear(); } diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc b/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc index 611a7a28d6f3c..abb70d9093f2e 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc @@ -54,7 +54,8 @@ spdy::SpdyHeaderBlock envoyHeadersToSpdyHeaderBlock(const Http::HeaderMap& heade spdy::SpdyHeaderBlock header_block; headers.iterate([&header_block](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { // The key-value pairs are copied. - header_block.insert({header.key().getStringView(), header.value().getStringView()}); + header_block.AppendValueOrAddHeader(header.key().getStringView(), + header.value().getStringView()); return Http::HeaderMap::Iterate::Continue; }); return header_block; diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 88888a7d67e0e..7b21443eaf216 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -741,6 +741,20 @@ TEST_P(HeaderMapImplTest, DoubleCookieAdd) { ASSERT_EQ(set_cookie_value[1]->value().getStringView(), "bar"); } +TEST_P(HeaderMapImplTest, CoalesceCookieHeadersWithSemicolon) { + TestRequestHeaderMapImpl headers; + const std::string foo("foo=1"); + const std::string bar("bar=2"); + const LowerCaseString& cookie = Http::Headers::get().Cookie; + headers.addReference(cookie, foo); + headers.addReference(cookie, bar); + EXPECT_EQ(1UL, headers.size()); + + const auto cookie_value = headers.get(LowerCaseString("cookie")); + ASSERT_EQ(cookie_value.size(), 1); + ASSERT_EQ(cookie_value[0]->value().getStringView(), "foo=1; bar=2"); +} + TEST_P(HeaderMapImplTest, DoubleInlineSet) { TestRequestHeaderMapImpl headers; headers.setReferenceKey(Headers::get().ContentType, "blah"); diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc index 15e986255dbbe..029f71742675b 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc @@ -183,6 +183,9 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) { request_headers.OnHeader(":authority", host_); request_headers.OnHeader(":method", "GET"); request_headers.OnHeader(":path", "/"); + // QUICHE stack doesn't coalesce Cookie headers for header compression optimization. + request_headers.OnHeader("cookie", "a=b"); + request_headers.OnHeader("cookie", "c=d"); request_headers.OnHeaderBlockEnd(/*uncompressed_header_bytes=*/0, /*compressed_header_bytes=*/0); @@ -192,6 +195,9 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) { EXPECT_EQ(host_, headers->getHostValue()); EXPECT_EQ("/", headers->getPathValue()); EXPECT_EQ(Http::Headers::get().MethodValues.Get, headers->getMethodValue()); + // Verify that the duplicated headers are handled correctly before passing to stream + // decoder. + EXPECT_EQ("a=b; c=d", headers->getCookieValue()); })); if (quic::VersionUsesHttp3(quic_version_.transport_version)) { EXPECT_CALL(stream_decoder_, decodeData(BufferStringEqual(""), /*end_stream=*/true)); @@ -199,6 +205,8 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) { spdy_headers[":authority"] = host_; spdy_headers[":method"] = "GET"; spdy_headers[":path"] = "/"; + spdy_headers.AppendValueOrAddHeader("cookie", "a=b"); + spdy_headers.AppendValueOrAddHeader("cookie", "c=d"); std::string payload = spdyHeaderToHttp3StreamPayload(spdy_headers); quic::QuicStreamFrame frame(stream_id_, true, 0, payload); quic_stream_->OnStreamFrame(frame); diff --git a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc index 40a5997fbe9e1..1ab123bb7b9a2 100644 --- a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc +++ b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc @@ -574,5 +574,35 @@ TEST_P(QuicHttpIntegrationTest, Reset101SwitchProtocolResponse) { EXPECT_FALSE(response->complete()); } +TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeader) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/dynamo/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"cookie", "a=b"}, + {"cookie", "c=d"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 0, true); + waitForNextUpstreamRequest(); + + EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(), "a=b; c=d"); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}, + {"set-cookie", "foo"}, + {"set-cookie", "bar"}}, + true); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + const auto out = response->headers().get(Http::LowerCaseString("set-cookie")); + ASSERT_EQ(out.size(), 2); + ASSERT_EQ(out[0]->value().getStringView(), "foo"); + ASSERT_EQ(out[1]->value().getStringView(), "bar"); +} + } // namespace Quic } // namespace Envoy From a80c27fd67a2479fd3e5a70d7886f0d0f03e2c7d Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Wed, 10 Feb 2021 16:33:07 -0500 Subject: [PATCH 2/9] add flag Signed-off-by: Dan Zhang --- include/envoy/http/header_map.h | 3 +-- source/common/http/header_map_impl.cc | 9 +++++---- source/common/http/header_map_impl.h | 2 ++ source/common/runtime/runtime_features.cc | 1 + .../quic_listeners/quiche/envoy_quic_utils.h | 16 ++++++++++++---- test/common/http/header_map_impl_test.cc | 8 ++++++-- .../quiche/envoy_quic_server_stream_test.cc | 10 +++++++--- 7 files changed, 34 insertions(+), 15 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index d0c43d50b87d8..e3262e3ecfb0d 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -294,8 +294,7 @@ class HeaderEntry { HEADER_FUNC(Protocol) \ HEADER_FUNC(Scheme) \ HEADER_FUNC(TE) \ - HEADER_FUNC(UserAgent) \ - HEADER_FUNC(Cookie) + HEADER_FUNC(UserAgent) /** * Default O(1) response headers. diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 56c60de08c12b..3b630f06bb247 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -362,10 +362,8 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) { if (*lookup.value().entry_ == nullptr) { maybeCreateInline(lookup.value().entry_, *lookup.value().key_, std::move(value)); } else { - const std::string delimiter = - (*lookup.value().key_ == Http::Headers::get().Cookie ? "; " : ","); const uint64_t added_size = - appendToHeader((*lookup.value().entry_)->value(), value.getStringView(), delimiter); + appendToHeader((*lookup.value().entry_)->value(), value.getStringView()); addSize(added_size); value.clear(); } @@ -430,7 +428,10 @@ void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view val // TODO(#9221): converge on and document a policy for coalescing multiple headers. auto entry = getExisting(key); if (!entry.empty()) { - const uint64_t added_size = appendToHeader(entry[0]->value(), value); + const std::string delimiter = (key == Http::Headers::get().Cookie ? "; " : ","); + const uint64_t added_size = header_map_coalesce_cookie_headers_ + ? appendToHeader(entry[0]->value(), value, delimiter) + : appendToHeader(entry[0]->value(), value); addSize(added_size); } else { addCopy(key, value); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 375abeca3658f..59adcd13c1522 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -325,6 +325,8 @@ class HeaderMapImpl : NonCopyable { HeaderList headers_; // This holds the internal byte size of the HeaderMap. uint64_t cached_byte_size_ = 0; + const bool header_map_coalesce_cookie_headers_ = Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_map_coalesce_cookie_headers"); }; /** diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9c5ea2805f03e..166561d22ac9f 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -89,6 +89,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.unify_grpc_handling", "envoy.reloadable_features.upstream_http2_flood_checks", "envoy.restart_features.use_apple_api_for_dns_lookups", + "envoy.reloadable_features.header_map_coalesce_cookie_headers", }; // This is a section for officially sanctioned runtime features which are too diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h index 3c29b28ef21f0..0259cb6d61b4d 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h @@ -42,8 +42,17 @@ template std::unique_ptr quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list) { auto headers = T::create(); for (const auto& entry : header_list) { - // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC. - headers->addCopy(Http::LowerCaseString(entry.first), entry.second); + auto key = Http::LowerCaseString(entry.first); + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_map_coalesce_cookie_headers") || + key != Http::Headers::get().Cookie) { + // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC. + headers->addCopy(key, entry.second); + } else { + // QUICHE breaks "cookie" header into crumbs. Coalesce them by appending current one to + // existing one if there is any. + headers->appendCopy(key, entry.second); + } } return headers; } @@ -54,8 +63,7 @@ std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& he for (auto entry : header_block) { // TODO(danzh): Avoid temporary strings and addCopy() with string_view. std::string key(entry.first); - std::string value(entry.second); - headers->addCopy(Http::LowerCaseString(key), value); + headers->addCopy(Http::LowerCaseString(key), entry.second); } return headers; } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 7b21443eaf216..ac7986273c67b 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -741,13 +741,17 @@ TEST_P(HeaderMapImplTest, DoubleCookieAdd) { ASSERT_EQ(set_cookie_value[1]->value().getStringView(), "bar"); } -TEST_P(HeaderMapImplTest, CoalesceCookieHeadersWithSemicolon) { +TEST_P(HeaderMapImplTest, AppendCookieHeadersWithSemicolon) { + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_map_coalesce_cookie_headers")) { + return; + } TestRequestHeaderMapImpl headers; const std::string foo("foo=1"); const std::string bar("bar=2"); const LowerCaseString& cookie = Http::Headers::get().Cookie; headers.addReference(cookie, foo); - headers.addReference(cookie, bar); + headers.appendCopy(cookie, bar); EXPECT_EQ(1UL, headers.size()); const auto cookie_value = headers.get(LowerCaseString("cookie")); diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc index 029f71742675b..b228a7e8ba302 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc @@ -195,9 +195,13 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) { EXPECT_EQ(host_, headers->getHostValue()); EXPECT_EQ("/", headers->getPathValue()); EXPECT_EQ(Http::Headers::get().MethodValues.Get, headers->getMethodValue()); - // Verify that the duplicated headers are handled correctly before passing to stream - // decoder. - EXPECT_EQ("a=b; c=d", headers->getCookieValue()); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_map_coalesce_cookie_headers")) { + // Verify that the duplicated headers are handled correctly before passing to stream + // decoder. + EXPECT_EQ("a=b; c=d", + headers->get(Http::Headers::get().Cookie)[0]->value().getStringView()); + } })); if (quic::VersionUsesHttp3(quic_version_.transport_version)) { EXPECT_CALL(stream_decoder_, decodeData(BufferStringEqual(""), /*end_stream=*/true)); From 57e94afa78e4128d8fb78b7524f38d7bc2c6daad Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Wed, 10 Feb 2021 16:40:35 -0500 Subject: [PATCH 3/9] flag protect e2e test Signed-off-by: Dan Zhang --- .../quiche/integration/quic_http_integration_test.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc index 1ab123bb7b9a2..002c82b2b1258 100644 --- a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc +++ b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc @@ -574,7 +574,7 @@ TEST_P(QuicHttpIntegrationTest, Reset101SwitchProtocolResponse) { EXPECT_FALSE(response->complete()); } -TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeader) { +TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeaders) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -589,8 +589,11 @@ TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeader) { auto response = std::move(encoder_decoder.second); codec_client_->sendData(*request_encoder_, 0, true); waitForNextUpstreamRequest(); - - EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(), "a=b; c=d"); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_map_coalesce_cookie_headers")) { + EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(), + "a=b; c=d"); + } upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}, {"set-cookie", "foo"}, From f0d1aad74b3566c3b23731e158656a1d063877f6 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 12 Feb 2021 14:19:31 -0500 Subject: [PATCH 4/9] rename runtime guard Signed-off-by: Dan Zhang --- source/common/http/header_map_impl.cc | 2 +- source/common/http/header_map_impl.h | 4 ++-- source/common/runtime/runtime_features.cc | 2 +- source/extensions/quic_listeners/quiche/envoy_quic_utils.h | 4 +--- test/common/http/header_map_impl_test.cc | 2 +- .../quic_listeners/quiche/envoy_quic_server_stream_test.cc | 2 +- .../quiche/integration/quic_http_integration_test.cc | 2 +- 7 files changed, 8 insertions(+), 10 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 3b630f06bb247..4dd22ee29b560 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -429,7 +429,7 @@ void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view val auto entry = getExisting(key); if (!entry.empty()) { const std::string delimiter = (key == Http::Headers::get().Cookie ? "; " : ","); - const uint64_t added_size = header_map_coalesce_cookie_headers_ + const uint64_t added_size = header_map_correctly_coalesce_cookies_ ? appendToHeader(entry[0]->value(), value, delimiter) : appendToHeader(entry[0]->value(), value); addSize(added_size); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 59adcd13c1522..dc71f6dfa8b0a 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -325,8 +325,8 @@ class HeaderMapImpl : NonCopyable { HeaderList headers_; // This holds the internal byte size of the HeaderMap. uint64_t cached_byte_size_ = 0; - const bool header_map_coalesce_cookie_headers_ = Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.header_map_coalesce_cookie_headers"); + const bool header_map_correctly_coalesce_cookies_ = Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_map_correctly_coalesce_cookies"); }; /** diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 166561d22ac9f..f6bd3970e8b61 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -89,7 +89,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.unify_grpc_handling", "envoy.reloadable_features.upstream_http2_flood_checks", "envoy.restart_features.use_apple_api_for_dns_lookups", - "envoy.reloadable_features.header_map_coalesce_cookie_headers", + "envoy.reloadable_features.header_map_correctly_coalesce_cookies", }; // This is a section for officially sanctioned runtime features which are too diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h index 0259cb6d61b4d..2e9c247d4722f 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h +++ b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h @@ -43,9 +43,7 @@ std::unique_ptr quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_ auto headers = T::create(); for (const auto& entry : header_list) { auto key = Http::LowerCaseString(entry.first); - if (!Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.header_map_coalesce_cookie_headers") || - key != Http::Headers::get().Cookie) { + if (key != Http::Headers::get().Cookie) { // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC. headers->addCopy(key, entry.second); } else { diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index ac7986273c67b..c8c46409aea38 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -743,7 +743,7 @@ TEST_P(HeaderMapImplTest, DoubleCookieAdd) { TEST_P(HeaderMapImplTest, AppendCookieHeadersWithSemicolon) { if (!Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.header_map_coalesce_cookie_headers")) { + "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) { return; } TestRequestHeaderMapImpl headers; diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc index b228a7e8ba302..eb0125b25fc32 100644 --- a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc +++ b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc @@ -196,7 +196,7 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) { EXPECT_EQ("/", headers->getPathValue()); EXPECT_EQ(Http::Headers::get().MethodValues.Get, headers->getMethodValue()); if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.header_map_coalesce_cookie_headers")) { + "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) { // Verify that the duplicated headers are handled correctly before passing to stream // decoder. EXPECT_EQ("a=b; c=d", diff --git a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc index 002c82b2b1258..488a5e81000e7 100644 --- a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc +++ b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc @@ -590,7 +590,7 @@ TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeaders) { codec_client_->sendData(*request_encoder_, 0, true); waitForNextUpstreamRequest(); if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.header_map_coalesce_cookie_headers")) { + "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) { EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(), "a=b; c=d"); } From e9349a1ffc28c07b45a819a22062e1dc45a29269 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 18 Feb 2021 16:06:24 -0500 Subject: [PATCH 5/9] document the fix Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 7ff53ec03f376..e599784d967d9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -36,6 +36,7 @@ Bug Fixes * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort. +* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. Removed Config or Runtime ------------------------- From d1be5192a380dd77420375a758507fb0aa4935cc Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 19 Feb 2021 17:27:57 -0500 Subject: [PATCH 6/9] format Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index e599784d967d9..a60f331d5fac0 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -33,10 +33,10 @@ Bug Fixes * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. * buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1. +* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort. -* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. Removed Config or Runtime ------------------------- From b0a0281a9408f80f1ed689a863cbf6ebbbc9e63c Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 22 Feb 2021 00:35:01 -0500 Subject: [PATCH 7/9] fix doc Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6e12b7f4d5058..3c6be4d5504d8 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -54,9 +54,9 @@ Bug Fixes * active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. * buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1. -* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. * fault injection: stop counting as active fault after delay elapsed. Previously fault injection filter continues to count the injected delay as an active fault even after it has elapsed. This produces incorrect output statistics and impacts the max number of consecutive faults allowed (e.g., for long-lived streams). This change decreases the active fault count when the delay fault is the only active and has gone finished. * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false. +* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled. From d432876c459b071c0b25fcf52df5fd547ede1444 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 25 Feb 2021 00:02:59 -0500 Subject: [PATCH 8/9] reorder bug fixes Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 061e928a3250e..a5fc1c03481bb 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -55,9 +55,9 @@ Bug Fixes * fault injection: stop counting as active fault after delay elapsed. Previously fault injection filter continues to count the injected delay as an active fault even after it has elapsed. This produces incorrect output statistics and impacts the max number of consecutive faults allowed (e.g., for long-lived streams). This change decreases the active fault count when the delay fault is the only active and has gone finished. * filter_chain: fix filter chain matching with the server name as the case-insensitive way. * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false. -* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. * grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream. * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. +* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled. * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration. From 6b341461ff0cf80ebd98c9faa48cfc45e3278bb9 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 25 Feb 2021 12:19:22 -0500 Subject: [PATCH 9/9] reorder again Signed-off-by: Dan Zhang --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a5fc1c03481bb..714934b448f53 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -56,8 +56,8 @@ Bug Fixes * filter_chain: fix filter chain matching with the server name as the case-insensitive way. * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false. * grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream. -* http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. * header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false. +* http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled. * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration.