From cc2439135b8308dd60f8427650f959ccb33fe069 Mon Sep 17 00:00:00 2001 From: Long Dai Date: Wed, 19 May 2021 20:51:46 +0800 Subject: [PATCH 01/21] docs: fix format issue (#16555) Fix format issue to pass pre-push check. Signed-off-by: Long Dai --- docs/root/version_history/current.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2ba5d8a0489a..bd265aaccff6 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -60,12 +60,11 @@ Removed Config or Runtime New Features ------------ +* crash support: restore crash context when continuing to processing requests or responses as a result of an asynchronous callback that invokes a filter directly. This is unlike the call stacks that go through the various network layers, to eventually reach the filter. For a concrete example see: ``Envoy::Extensions::HttpFilters::Cache::CacheFilter::getHeaders`` which posts a callback on the dispatcher that will invoke the filter directly. * http: a new field `is_optional` is added to `extensions.filters.network.http_connection_manager.v3.HttpFilter`. When value is `true`, the unsupported http filter will be ignored by envoy. This is also same with unsupported http filter in the typed per filter config. For more information, please reference :ref:`HttpFilter `. - -* crash support: restore crash context when continuing to processing requests or responses as a result of an asynchronous callback that invokes a filter directly. This is unlike the call stacks that go through the various network layers, to eventually reach the filter. For a concrete example see: ``Envoy::Extensions::HttpFilters::Cache::CacheFilter::getHeaders`` which posts a callback on the dispatcher that will invoke the filter directly. * http: added support for :ref:`original IP detection extensions`. Two initial extensions were added, the :ref:`custom header ` extension and the :ref:`xff ` extension. From 8baff9a6aed37fc039f493682ec66cf7128f7c6e Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 19 May 2021 10:27:31 -0400 Subject: [PATCH 02/21] quic: adjusting coverage (#16570) Fixing a merge race Signed-off-by: Alyssa Wilk --- test/per_file_coverage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index ef5e2a6bb6ad..b0b6f6a11f3e 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -18,7 +18,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/common/singleton:95.1" "source/common/thread:0.0" # Death tests don't report LCOV "source/common/matcher:93.3" -"source/common/quic:88.4" +"source/common/quic:87.8" "source/common/tracing:95.7" "source/common/watchdog:42.9" # Death tests don't report LCOV "source/exe:94.3" From 92416beb1d970a994ce716b74c9d2ed2f3295d51 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Wed, 19 May 2021 08:11:34 -0700 Subject: [PATCH 03/21] http: remove HeaderUtility::addHeaders (duplicate). (#16509) Both HeaderMapImpl::copyFrom() and HeaderUtility::addHeaders() have exactly the same implementation, so remove the latter. Signed-off-by: Piotr Sikora --- source/common/http/header_utility.cc | 11 ----------- source/common/http/header_utility.h | 7 ------- source/extensions/filters/http/ratelimit/ratelimit.cc | 6 +++--- test/common/http/header_utility_test.cc | 2 +- 4 files changed, 4 insertions(+), 22 deletions(-) diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 527bd05f52af..e504a21ad8a2 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -210,17 +210,6 @@ bool HeaderUtility::requestShouldHaveNoBody(const RequestHeaderMap& headers) { headers.Method()->value() == Http::Headers::get().MethodValues.Connect)); } -void HeaderUtility::addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add) { - headers_to_add.iterate([&headers](const HeaderEntry& header) -> HeaderMap::Iterate { - HeaderString k; - k.setCopy(header.key().getStringView()); - HeaderString v; - v.setCopy(header.value().getStringView()); - headers.addViaMove(std::move(k), std::move(v)); - return HeaderMap::Iterate::Continue; - }); -} - bool HeaderUtility::isEnvoyInternalRequest(const RequestHeaderMap& headers) { const HeaderEntry* internal_request_header = headers.EnvoyInternalRequest(); return internal_request_header != nullptr && diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index e84f38ed5347..20719003733c 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -153,13 +153,6 @@ class HeaderUtility { static bool requestShouldHaveNoBody(const RequestHeaderMap& headers); - /** - * Add headers from one HeaderMap to another - * @param headers target where headers will be added - * @param headers_to_add supplies the headers to be added - */ - static void addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add); - /** * @brief a helper function to determine if the headers represent an envoy internal request */ diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index 07c11b088e2d..d97b6ea489cb 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -193,7 +193,7 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, if (response_headers_to_add_ == nullptr) { response_headers_to_add_ = Http::ResponseHeaderMapImpl::create(); } - Http::HeaderUtility::addHeaders(*response_headers_to_add_, *rate_limit_headers); + Http::HeaderMapImpl::copyFrom(*response_headers_to_add_, *rate_limit_headers); } else { descriptor_statuses = nullptr; } @@ -255,14 +255,14 @@ void Filter::populateResponseHeaders(Http::HeaderMap& response_headers, bool fro if (from_local_reply && !response_headers_to_add_->getContentTypeValue().empty()) { response_headers.remove(Http::Headers::get().ContentType); } - Http::HeaderUtility::addHeaders(response_headers, *response_headers_to_add_); + Http::HeaderMapImpl::copyFrom(response_headers, *response_headers_to_add_); response_headers_to_add_ = nullptr; } } void Filter::appendRequestHeaders(Http::HeaderMapPtr& request_headers_to_add) { if (request_headers_to_add && request_headers_) { - Http::HeaderUtility::addHeaders(*request_headers_, *request_headers_to_add); + Http::HeaderMapImpl::copyFrom(*request_headers_, *request_headers_to_add); request_headers_to_add = nullptr; } } diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 7664aaee4c80..a529f7011b01 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -687,7 +687,7 @@ TEST(HeaderAddTest, HeaderAdd) { TestRequestHeaderMapImpl headers{{"myheader1", "123value"}}; TestRequestHeaderMapImpl headers_to_add{{"myheader2", "456value"}}; - HeaderUtility::addHeaders(headers, headers_to_add); + HeaderMapImpl::copyFrom(headers, headers_to_add); headers_to_add.iterate([&headers](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; From 5f3fbf64bb9660cef3ec573176021d07cede5c97 Mon Sep 17 00:00:00 2001 From: Long Dai Date: Wed, 19 May 2021 23:37:32 +0800 Subject: [PATCH 04/21] examples: unify apt and cleanup unused installation (#16519) Signed-off-by: Long Dai --- examples/cache/Dockerfile-frontenvoy | 2 -- examples/cors/backend/Dockerfile-frontenvoy | 2 -- examples/cors/frontend/Dockerfile-frontenvoy | 2 -- examples/csrf/crosssite/Dockerfile-frontenvoy | 2 -- examples/csrf/samesite/Dockerfile-frontenvoy | 2 -- examples/dynamic-config-cp/Dockerfile-control-plane | 7 ++++++- examples/dynamic-config-cp/Dockerfile-proxy | 1 - examples/ext_authz/Dockerfile-frontenvoy | 2 -- examples/fault-injection/Dockerfile-envoy | 6 +++++- examples/front-proxy/Dockerfile-frontenvoy | 7 +++++-- examples/jaeger-native-tracing/Dockerfile-frontenvoy | 8 ++++++-- examples/skywalking-tracing/Dockerfile-frontenvoy | 2 -- examples/zipkin-tracing/Dockerfile-frontenvoy | 2 -- 13 files changed, 22 insertions(+), 23 deletions(-) diff --git a/examples/cache/Dockerfile-frontenvoy b/examples/cache/Dockerfile-frontenvoy index 0b2e25a0de1b..80d3fd894e59 100644 --- a/examples/cache/Dockerfile-frontenvoy +++ b/examples/cache/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./front-envoy.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD /usr/local/bin/envoy -c /etc/front-envoy.yaml --service-cluster front-proxy diff --git a/examples/cors/backend/Dockerfile-frontenvoy b/examples/cors/backend/Dockerfile-frontenvoy index 31ee1c2e7432..67ab98915d01 100644 --- a/examples/cors/backend/Dockerfile-frontenvoy +++ b/examples/cors/backend/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./front-envoy.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD ["/usr/local/bin/envoy", "-c", "/etc/front-envoy.yaml", "--service-cluster", "front-proxy"] diff --git a/examples/cors/frontend/Dockerfile-frontenvoy b/examples/cors/frontend/Dockerfile-frontenvoy index 31ee1c2e7432..67ab98915d01 100644 --- a/examples/cors/frontend/Dockerfile-frontenvoy +++ b/examples/cors/frontend/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./front-envoy.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD ["/usr/local/bin/envoy", "-c", "/etc/front-envoy.yaml", "--service-cluster", "front-proxy"] diff --git a/examples/csrf/crosssite/Dockerfile-frontenvoy b/examples/csrf/crosssite/Dockerfile-frontenvoy index 31ee1c2e7432..67ab98915d01 100644 --- a/examples/csrf/crosssite/Dockerfile-frontenvoy +++ b/examples/csrf/crosssite/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./front-envoy.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD ["/usr/local/bin/envoy", "-c", "/etc/front-envoy.yaml", "--service-cluster", "front-proxy"] diff --git a/examples/csrf/samesite/Dockerfile-frontenvoy b/examples/csrf/samesite/Dockerfile-frontenvoy index 799a5721130a..597da0315b78 100644 --- a/examples/csrf/samesite/Dockerfile-frontenvoy +++ b/examples/csrf/samesite/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./front-envoy.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD ["/usr/local/bin/envoy", "-c", "/etc/front-envoy.yaml", "--service-cluster", "front-proxy"] diff --git a/examples/dynamic-config-cp/Dockerfile-control-plane b/examples/dynamic-config-cp/Dockerfile-control-plane index 3c5bd17826ef..39c7f2ca4223 100644 --- a/examples/dynamic-config-cp/Dockerfile-control-plane +++ b/examples/dynamic-config-cp/Dockerfile-control-plane @@ -1,6 +1,11 @@ FROM golang -RUN apt-get -y update && apt-get install -y -qq --no-install-recommends netcat +RUN apt-get update \ + && apt-get install --no-install-recommends -y netcat \ + && apt-get autoremove -y \ + && apt-get clean \ + && rm -rf /tmp/* /var/tmp/* /var/lib/apt/lists/* + RUN git clone https://github.com/envoyproxy/go-control-plane ADD ./resource.go /go/go-control-plane/internal/example/resource.go RUN cd go-control-plane && make bin/example diff --git a/examples/dynamic-config-cp/Dockerfile-proxy b/examples/dynamic-config-cp/Dockerfile-proxy index 6e1c4d08b6cb..f70f44311461 100644 --- a/examples/dynamic-config-cp/Dockerfile-proxy +++ b/examples/dynamic-config-cp/Dockerfile-proxy @@ -1,6 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get -y update && apt-get install -y -qq --no-install-recommends netcat COPY ./envoy.yaml /etc/envoy.yaml RUN chmod go+r /etc/envoy.yaml CMD ["/usr/local/bin/envoy", "-c /etc/envoy.yaml", "-l", "debug"] diff --git a/examples/ext_authz/Dockerfile-frontenvoy b/examples/ext_authz/Dockerfile-frontenvoy index 815937798a83..dc225158365b 100644 --- a/examples/ext_authz/Dockerfile-frontenvoy +++ b/examples/ext_authz/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./config /etc/envoy-config COPY ./run_envoy.sh /run_envoy.sh RUN chmod go+r -R /etc/envoy-config \ diff --git a/examples/fault-injection/Dockerfile-envoy b/examples/fault-injection/Dockerfile-envoy index 13dec2521a99..17e350d7d535 100644 --- a/examples/fault-injection/Dockerfile-envoy +++ b/examples/fault-injection/Dockerfile-envoy @@ -1,6 +1,10 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get install -y curl tree +RUN apt-get update \ + && apt-get install --no-install-recommends -y tree curl \ + && apt-get autoremove -y \ + && apt-get clean \ + && rm -rf /tmp/* /var/tmp/* /var/lib/apt/lists/* COPY ./envoy.yaml /etc/envoy.yaml RUN chmod go+r /etc/envoy.yaml COPY enable_delay_fault_injection.sh disable_delay_fault_injection.sh enable_abort_fault_injection.sh disable_abort_fault_injection.sh send_request.sh / diff --git a/examples/front-proxy/Dockerfile-frontenvoy b/examples/front-proxy/Dockerfile-frontenvoy index 31ee1c2e7432..183d46c7a5c3 100644 --- a/examples/front-proxy/Dockerfile-frontenvoy +++ b/examples/front-proxy/Dockerfile-frontenvoy @@ -1,7 +1,10 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl +RUN apt-get update \ + && apt-get install --no-install-recommends -y curl \ + && apt-get autoremove -y \ + && apt-get clean \ + && rm -rf /tmp/* /var/tmp/* /var/lib/apt/lists/* COPY ./front-envoy.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD ["/usr/local/bin/envoy", "-c", "/etc/front-envoy.yaml", "--service-cluster", "front-proxy"] diff --git a/examples/jaeger-native-tracing/Dockerfile-frontenvoy b/examples/jaeger-native-tracing/Dockerfile-frontenvoy index d92faf3834eb..ef40a9365ed2 100644 --- a/examples/jaeger-native-tracing/Dockerfile-frontenvoy +++ b/examples/jaeger-native-tracing/Dockerfile-frontenvoy @@ -1,7 +1,11 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl +RUN apt-get update \ + && apt-get install --no-install-recommends -y curl \ + && apt-get autoremove -y \ + && apt-get clean \ + && rm -rf /tmp/* /var/tmp/* /var/lib/apt/lists/* + COPY ./front-envoy-jaeger.yaml /etc/front-envoy.yaml # # for discussion on jaeger binary compatibility, and the source of the file, see here: diff --git a/examples/skywalking-tracing/Dockerfile-frontenvoy b/examples/skywalking-tracing/Dockerfile-frontenvoy index 86d0a6b91b8b..a4be8e806034 100644 --- a/examples/skywalking-tracing/Dockerfile-frontenvoy +++ b/examples/skywalking-tracing/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./front-envoy-skywalking.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD /usr/local/bin/envoy -c /etc/front-envoy.yaml --service-cluster front-proxy diff --git a/examples/zipkin-tracing/Dockerfile-frontenvoy b/examples/zipkin-tracing/Dockerfile-frontenvoy index cd80edc3ba04..700a6b78eac4 100644 --- a/examples/zipkin-tracing/Dockerfile-frontenvoy +++ b/examples/zipkin-tracing/Dockerfile-frontenvoy @@ -1,7 +1,5 @@ FROM envoyproxy/envoy-dev:latest -RUN apt-get update && apt-get -q install -y \ - curl COPY ./front-envoy-zipkin.yaml /etc/front-envoy.yaml RUN chmod go+r /etc/front-envoy.yaml CMD ["/usr/local/bin/envoy", "-c", "/etc/front-envoy.yaml", "--service-cluster", "front-proxy"] From 67bfb7c4afbb8c0930d7f8d9685104c4151c315b Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 19 May 2021 15:03:56 -0400 Subject: [PATCH 05/21] quic: use sds for upstream http/3 (#16462) Risk Level: Medium (some data plane refactors, mostly no-ops for !HTTP/3) Testing: turned up HTTP/3 upstream SDS integration tests Docs Changes: n/a Release Notes: n/a part of #14829 Signed-off-by: Alyssa Wilk --- source/common/conn_pool/conn_pool_base.cc | 18 ++++- source/common/conn_pool/conn_pool_base.h | 1 + source/common/http/http3/conn_pool.cc | 9 ++- .../quic/client_connection_factory_impl.cc | 31 ++++++-- .../quic/client_connection_factory_impl.h | 20 ++++-- .../common/quic/envoy_quic_client_session.cc | 6 +- .../common/quic/envoy_quic_client_session.h | 3 +- .../quic/quic_transport_socket_factory.h | 10 +-- .../quic/envoy_quic_client_session_test.cc | 9 +-- test/config/utility.cc | 3 +- .../integration/quic_http_integration_test.cc | 2 +- .../sds_dynamic_integration_test.cc | 71 +++++++++++-------- 12 files changed, 124 insertions(+), 59 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index 4a85f885c8b9..a1982b7fa0fe 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -142,6 +142,10 @@ ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) { (ready_clients_.empty() && busy_clients_.empty() && connecting_clients_.empty())) { ENVOY_LOG(debug, "creating a new connection"); ActiveClientPtr client = instantiateActiveClient(); + if (client.get() == nullptr) { + ENVOY_LOG(trace, "connection creation failed"); + return ConnectionResult::FailedToCreateConnection; + } ASSERT(client->state() == ActiveClient::State::CONNECTING); ASSERT(std::numeric_limits::max() - connecting_stream_capacity_ >= client->effectiveConcurrentStreamLimit()); @@ -249,9 +253,19 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) // increase capacity is if the connection limits are exceeded. ENVOY_BUG(pending_streams_.size() <= connecting_stream_capacity_ || connecting_stream_capacity_ > old_capacity || - result == ConnectionResult::NoConnectionRateLimited, + (result == ConnectionResult::NoConnectionRateLimited || + result == ConnectionResult::FailedToCreateConnection), fmt::format("Failed to create expected connection: {}", *this)); - return pending; + if (result == ConnectionResult::FailedToCreateConnection) { + // This currently only happens for HTTP/3 if secrets aren't yet loaded. + // Trigger connection failure. + pending->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess); + onPoolFailure(nullptr, absl::string_view(), ConnectionPool::PoolFailureReason::Overflow, + context); + return nullptr; + } else { + return pending; + } } else { ENVOY_LOG(debug, "max pending streams overflow"); onPoolFailure(nullptr, absl::string_view(), ConnectionPool::PoolFailureReason::Overflow, diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index 364e2d01fc48..dd72cc83a30b 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -248,6 +248,7 @@ class ConnPoolImplBase : protected Logger::Loggable { virtual void onConnected(Envoy::ConnectionPool::ActiveClient&) {} enum class ConnectionResult { + FailedToCreateConnection, CreatedNewConnection, ShouldNotConnect, NoConnectionRateLimited, diff --git a/source/common/http/http3/conn_pool.cc b/source/common/http/http3/conn_pool.cc index 703b5b1c110f..3f1fbcbe5c1a 100644 --- a/source/common/http/http3/conn_pool.cc +++ b/source/common/http/http3/conn_pool.cc @@ -15,6 +15,7 @@ #ifdef ENVOY_ENABLE_QUIC #include "common/quic/client_connection_factory_impl.h" #include "common/quic/envoy_quic_utils.h" +#include "common/quic/quic_transport_socket_factory.h" #else #error "http3 conn pool should not be built with QUIC disabled" #endif @@ -64,7 +65,13 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_ Upstream::ClusterConnectivityState& state, TimeSource& time_source) { return std::make_unique( host, priority, dispatcher, options, transport_socket_options, random_generator, state, - [](HttpConnPoolImplBase* pool) { + [](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr { + // If there's no ssl context, the secrets are not loaded. Fast-fail by returning null. + auto factory = &pool->host()->transportSocketFactory(); + ASSERT(dynamic_cast(factory) != nullptr); + if (static_cast(factory)->sslCtx() == nullptr) { + return nullptr; + } Http3ConnPoolImpl* h3_pool = reinterpret_cast(pool); Upstream::Host::CreateConnectionData data{}; data.host_description_ = pool->host(); diff --git a/source/common/quic/client_connection_factory_impl.cc b/source/common/quic/client_connection_factory_impl.cc index b8c3c2bd9190..9dcda4745520 100644 --- a/source/common/quic/client_connection_factory_impl.cc +++ b/source/common/quic/client_connection_factory_impl.cc @@ -19,19 +19,34 @@ getContext(Network::TransportSocketFactory& transport_socket_factory) { auto* quic_socket_factory = dynamic_cast(&transport_socket_factory); ASSERT(quic_socket_factory != nullptr); - ASSERT(quic_socket_factory->sslCtx() != nullptr); return quic_socket_factory->sslCtx(); } +std::shared_ptr PersistentQuicInfoImpl::cryptoConfig() { + auto context = getContext(transport_socket_factory_); + // If the secrets haven't been loaded, there is no crypto config. + if (context == nullptr) { + return nullptr; + } + + // If the secret has been updated, update the proof source. + if (context.get() != client_context_.get()) { + client_context_ = context; + client_config_ = std::make_shared( + std::make_unique(getContext(transport_socket_factory_)), + std::make_unique((time_source_))); + } + // Return the latest client config. + return client_config_; +} + PersistentQuicInfoImpl::PersistentQuicInfoImpl( Event::Dispatcher& dispatcher, Network::TransportSocketFactory& transport_socket_factory, TimeSource& time_source, Network::Address::InstanceConstSharedPtr server_addr) : conn_helper_(dispatcher), alarm_factory_(dispatcher, *conn_helper_.GetClock()), server_id_{getConfig(transport_socket_factory).serverNameIndication(), static_cast(server_addr->ip()->port()), false}, - crypto_config_(std::make_unique( - std::make_unique(getContext(transport_socket_factory)), - std::make_unique(time_source))) { + transport_socket_factory_(transport_socket_factory), time_source_(time_source) { quiche::FlagRegistry::getInstance(); } @@ -53,6 +68,10 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d // This flag fix a QUICHE issue which may crash Envoy during connection close. SetQuicReloadableFlag(quic_single_ack_in_packet2, true); PersistentQuicInfoImpl* info_impl = reinterpret_cast(&info); + auto config = info_impl->cryptoConfig(); + if (config == nullptr) { + return nullptr; // no secrets available yet. + } auto connection = std::make_unique( quic::QuicUtils::CreateRandomConnectionId(), server_addr, info_impl->conn_helper_, @@ -66,8 +85,8 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d // send_buffer_limit instead of using 0. auto ret = std::make_unique( info_impl->quic_config_, info_impl->supported_versions_, std::move(connection), - info_impl->server_id_, info_impl->crypto_config_.get(), &static_info.push_promise_index_, - dispatcher, /*send_buffer_limit=*/0); + info_impl->server_id_, std::move(config), &static_info.push_promise_index_, dispatcher, + /*send_buffer_limit=*/0); return ret; } diff --git a/source/common/quic/client_connection_factory_impl.h b/source/common/quic/client_connection_factory_impl.h index 3f736f596cdb..bf56a95f3e84 100644 --- a/source/common/quic/client_connection_factory_impl.h +++ b/source/common/quic/client_connection_factory_impl.h @@ -22,15 +22,25 @@ struct PersistentQuicInfoImpl : public Http::PersistentQuicInfo { TimeSource& time_source, Network::Address::InstanceConstSharedPtr server_addr); + // Returns the most recent crypto config from transport_socket_factory_; + std::shared_ptr cryptoConfig(); + EnvoyQuicConnectionHelper conn_helper_; EnvoyQuicAlarmFactory alarm_factory_; - // server-id and server address can change over the lifetime of Envoy but will be consistent for a + // server-id can change over the lifetime of Envoy but will be consistent for a // given connection pool. quic::QuicServerId server_id_; - quic::ParsedQuicVersionVector supported_versions_{quic::CurrentSupportedVersions()}; - // TODO(danzh) move this into client transport socket factory so that it can - // be updated with SDS. - std::unique_ptr crypto_config_; + // Latch the transport socket factory, to get the latest crypto config and the + // time source to create it. + Network::TransportSocketFactory& transport_socket_factory_; + TimeSource& time_source_; + // Latch the latest crypto config, to determine if it has updated since last + // checked. + Envoy::Ssl::ClientContextSharedPtr client_context_; + // If client context changes, client config will be updated as well. + std::shared_ptr client_config_; + const quic::ParsedQuicVersionVector supported_versions_{quic::CurrentSupportedVersions()}; + // TODO(alyssawilk) actually set this up properly. quic::QuicConfig quic_config_; }; diff --git a/source/common/quic/envoy_quic_client_session.cc b/source/common/quic/envoy_quic_client_session.cc index 6f0484c9535e..0763a5f00932 100644 --- a/source/common/quic/envoy_quic_client_session.cc +++ b/source/common/quic/envoy_quic_client_session.cc @@ -8,14 +8,14 @@ namespace Quic { EnvoyQuicClientSession::EnvoyQuicClientSession( const quic::QuicConfig& config, const quic::ParsedQuicVersionVector& supported_versions, std::unique_ptr connection, const quic::QuicServerId& server_id, - quic::QuicCryptoClientConfig* crypto_config, + std::shared_ptr crypto_config, quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit) : QuicFilterManagerConnectionImpl(*connection, connection->connection_id(), dispatcher, send_buffer_limit), quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id, - crypto_config, push_promise_index), - host_name_(server_id.host()) {} + crypto_config.get(), push_promise_index), + host_name_(server_id.host()), crypto_config_(crypto_config) {} EnvoyQuicClientSession::~EnvoyQuicClientSession() { ASSERT(!connection()->connected()); diff --git a/source/common/quic/envoy_quic_client_session.h b/source/common/quic/envoy_quic_client_session.h index 8aecb34b9938..c4fe12ffd706 100644 --- a/source/common/quic/envoy_quic_client_session.h +++ b/source/common/quic/envoy_quic_client_session.h @@ -33,7 +33,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, const quic::ParsedQuicVersionVector& supported_versions, std::unique_ptr connection, const quic::QuicServerId& server_id, - quic::QuicCryptoClientConfig* crypto_config, + std::shared_ptr crypto_config, quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit); @@ -89,6 +89,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl, // them. Http::ConnectionCallbacks* http_connection_callbacks_{nullptr}; const absl::string_view host_name_; + std::shared_ptr crypto_config_; }; } // namespace Quic diff --git a/source/common/quic/quic_transport_socket_factory.h b/source/common/quic/quic_transport_socket_factory.h index 18fa00a7b034..9fad6a0b256d 100644 --- a/source/common/quic/quic_transport_socket_factory.h +++ b/source/common/quic/quic_transport_socket_factory.h @@ -96,9 +96,7 @@ class QuicClientTransportSocketFactory : public QuicTransportSocketFactoryBase { Ssl::ClientContextConfigPtr config, Server::Configuration::TransportSocketFactoryContext& factory_context); - void initialize() override { - // TODO(14829) fallback_factory_ needs to call onSecretUpdated() upon SDS update. - } + void initialize() override {} // As documented above for QuicTransportSocketFactoryBase, the actual HTTP/3 // code does not create transport sockets. @@ -118,10 +116,8 @@ class QuicClientTransportSocketFactory : public QuicTransportSocketFactoryBase { } protected: - void onSecretUpdated() override { - // fallback_factory_ will update the stats. - // TODO(14829) Client transport socket factory may also need to update quic crypto. - } + // fallback_factory_ will update the context. + void onSecretUpdated() override {} private: // The QUIC client transport socket can create TLS sockets for fallback to TCP. diff --git a/test/common/quic/envoy_quic_client_session_test.cc b/test/common/quic/envoy_quic_client_session_test.cc index 9facb2058a79..fbcc51df8c5c 100644 --- a/test/common/quic/envoy_quic_client_session_test.cc +++ b/test/common/quic/envoy_quic_client_session_test.cc @@ -80,7 +80,7 @@ class TestEnvoyQuicClientSession : public EnvoyQuicClientSession { const quic::ParsedQuicVersionVector& supported_versions, std::unique_ptr connection, const quic::QuicServerId& server_id, - quic::QuicCryptoClientConfig* crypto_config, + std::shared_ptr crypto_config, quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher, uint32_t send_buffer_limit) : EnvoyQuicClientSession(config, supported_versions, std::move(connection), server_id, @@ -109,10 +109,11 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { quic_connection_(new TestEnvoyQuicClientConnection( quic::test::TestConnectionId(), connection_helper_, alarm_factory_, writer_, quic_version_, *dispatcher_, createConnectionSocket(peer_addr_, self_addr_, nullptr))), - crypto_config_(quic::test::crypto_test_utils::ProofVerifierForTesting()), + crypto_config_(std::make_shared( + quic::test::crypto_test_utils::ProofVerifierForTesting())), envoy_quic_session_(quic_config_, quic_version_, std::unique_ptr(quic_connection_), - quic::QuicServerId("example.com", 443, false), &crypto_config_, nullptr, + quic::QuicServerId("example.com", 443, false), crypto_config_, nullptr, *dispatcher_, /*send_buffer_limit*/ 1024 * 1024), stats_({ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(scope_, "http3."), @@ -173,7 +174,7 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam { Network::Address::InstanceConstSharedPtr self_addr_; TestEnvoyQuicClientConnection* quic_connection_; quic::QuicConfig quic_config_; - quic::QuicCryptoClientConfig crypto_config_; + std::shared_ptr crypto_config_; TestEnvoyQuicClientSession envoy_quic_session_; Network::MockConnectionCallbacks network_connection_callbacks_; Http::MockServerConnectionCallbacks http_connection_callbacks_; diff --git a/test/config/utility.cc b/test/config/utility.cc index b2a8e1600f9e..0ce0a4313b5a 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -768,7 +768,8 @@ void ConfigHelper::setProtocolOptions(envoy::config::cluster::v3::Cluster& clust HttpProtocolOptions old_options = MessageUtil::anyConvert( (*cluster.mutable_typed_extension_protocol_options()) ["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"]); - protocol_options.MergeFrom(old_options); + old_options.MergeFrom(protocol_options); + protocol_options.CopyFrom(old_options); } (*cluster.mutable_typed_extension_protocol_options()) ["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"] diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 585bbc5beab0..8f6ce0c4bc69 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -113,7 +113,7 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, public QuicMultiVers auto& persistent_info = static_cast(*quic_connection_persistent_info_); auto session = std::make_unique( persistent_info.quic_config_, supported_versions_, std::move(connection), - persistent_info.server_id_, persistent_info.crypto_config_.get(), &push_promise_index_, + persistent_info.server_id_, persistent_info.cryptoConfig(), &push_promise_index_, *dispatcher_, // Use smaller window than the default one to have test coverage of client codec buffer // exceeding high watermark. diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 44c38873b31c..19b2636fcb5f 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -59,18 +59,16 @@ std::string sdsTestParamsToString(const ::testing::TestParamInfo& p) p.param.test_quic ? "UsesQuic" : "UsesTcp"); } -std::vector getSdsTestsParams(bool test_quic) { +std::vector getSdsTestsParams() { std::vector ret; for (auto ip_version : TestEnvironment::getIpVersionsForTest()) { for (auto sds_grpc_type : TestEnvironment::getsGrpcVersionsForTest()) { ret.push_back(TestParams{ip_version, sds_grpc_type, false}); - if (test_quic) { #ifdef ENVOY_ENABLE_QUIC - ret.push_back(TestParams{ip_version, sds_grpc_type, true}); + ret.push_back(TestParams{ip_version, sds_grpc_type, true}); #else - ENVOY_LOG_MISC(warn, "Skipping HTTP/3 as support is compiled out"); + ENVOY_LOG_MISC(warn, "Skipping HTTP/3 as support is compiled out"); #endif - } } } return ret; @@ -86,7 +84,7 @@ class SdsDynamicIntegrationBaseTest : public Grpc::BaseGrpcClientIntegrationPara SdsDynamicIntegrationBaseTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam().ip_version), server_cert_("server_cert"), validation_secret_("validation_secret"), - client_cert_("client_cert") {} + client_cert_("client_cert"), test_quic_(GetParam().test_quic) {} SdsDynamicIntegrationBaseTest(Http::CodecClient::Type downstream_protocol, Network::Address::IpVersion version, const std::string& config) @@ -286,7 +284,7 @@ class SdsDynamicDownstreamIntegrationTest : public SdsDynamicIntegrationBaseTest }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, SdsDynamicDownstreamIntegrationTest, - testing::ValuesIn(getSdsTestsParams(true)), sdsTestParamsToString); + testing::ValuesIn(getSdsTestsParams()), sdsTestParamsToString); class SdsDynamicKeyRotationIntegrationTest : public SdsDynamicDownstreamIntegrationTest { protected: @@ -307,7 +305,7 @@ class SdsDynamicKeyRotationIntegrationTest : public SdsDynamicDownstreamIntegrat // We don't care about multiple gRPC types here, Envoy gRPC is fine, the // interest is on the filesystem. INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, SdsDynamicKeyRotationIntegrationTest, - testing::ValuesIn(getSdsTestsParams(true)), sdsTestParamsToString); + testing::ValuesIn(getSdsTestsParams()), sdsTestParamsToString); // Validate that a basic key-cert rotation works via symlink rename. TEST_P(SdsDynamicKeyRotationIntegrationTest, BasicRotation) { @@ -507,7 +505,7 @@ class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstrea void createUpstreams() override { // Fake upstream with SSL/TLS for the first cluster. - addFakeUpstream(createUpstreamSslContext(), FakeHttpConnection::Type::HTTP1); + addFakeUpstream(createUpstreamSslContext(), upstreamProtocol()); create_xds_upstream_ = true; } @@ -544,7 +542,7 @@ class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstrea }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, SdsDynamicDownstreamCertValidationContextTest, - testing::ValuesIn(getSdsTestsParams(true)), sdsTestParamsToString); + testing::ValuesIn(getSdsTestsParams()), sdsTestParamsToString); // A test that SDS server send a good certificate validation context for a static listener. // The first ssl request should be OK. @@ -640,10 +638,13 @@ TEST_P(SdsDynamicDownstreamCertValidationContextTest, CombinedValidationContextW } // Upstream SDS integration test: a static cluster has ssl cert from SDS. -// TODO(15034) Enable SDS support in QUIC upstream. class SdsDynamicUpstreamIntegrationTest : public SdsDynamicIntegrationBaseTest { public: void initialize() override { + if (test_quic_) { + upstream_tls_ = true; + setUpstreamProtocol(FakeHttpConnection::Type::HTTP3); + } config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { // add sds cluster first. auto* sds_cluster = bootstrap.mutable_static_resources()->add_clusters(); @@ -651,16 +652,29 @@ class SdsDynamicUpstreamIntegrationTest : public SdsDynamicIntegrationBaseTest { sds_cluster->set_name("sds_cluster"); ConfigHelper::setHttp2(*sds_cluster); + // Unwind Quic for sds cluster. + if (test_quic_) { + sds_cluster->clear_transport_socket(); + } + // change the first cluster with ssl and sds. auto* transport_socket = bootstrap.mutable_static_resources()->mutable_clusters(0)->mutable_transport_socket(); envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; + tls_context.set_sni("lyft.com"); auto* secret_config = tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); setUpSdsConfig(secret_config, "client_cert"); - transport_socket->set_name("envoy.transport_sockets.tls"); - transport_socket->mutable_typed_config()->PackFrom(tls_context); + if (test_quic_) { + envoy::extensions::transport_sockets::quic::v3::QuicUpstreamTransport quic_context; + quic_context.mutable_upstream_tls_context()->CopyFrom(tls_context); + transport_socket->set_name("envoy.transport_sockets.quic"); + transport_socket->mutable_typed_config()->PackFrom(quic_context); + } else { + transport_socket->set_name("envoy.transport_sockets.tls"); + transport_socket->mutable_typed_config()->PackFrom(tls_context); + } }); HttpIntegrationTest::initialize(); @@ -679,14 +693,14 @@ class SdsDynamicUpstreamIntegrationTest : public SdsDynamicIntegrationBaseTest { void createUpstreams() override { // This is for backend with ssl - addFakeUpstream(createUpstreamSslContext(context_manager_, *api_), - FakeHttpConnection::Type::HTTP1); + addFakeUpstream(createUpstreamSslContext(context_manager_, *api_, test_quic_), + upstreamProtocol()); create_xds_upstream_ = true; } }; INSTANTIATE_TEST_SUITE_P(IpVersions, SdsDynamicUpstreamIntegrationTest, - testing::ValuesIn(getSdsTestsParams(false)), sdsTestParamsToString); + testing::ValuesIn(getSdsTestsParams()), sdsTestParamsToString); // To test a static cluster with sds. SDS send a good client secret first. // The first request should work. @@ -731,24 +745,25 @@ TEST_P(SdsDynamicUpstreamIntegrationTest, WrongSecretFirst) { ASSERT_TRUE(response->complete()); EXPECT_EQ("503", response->headers().getStatusValue()); - // To flush out the reset connection from the first request in upstream. - FakeRawConnectionPtr fake_upstream_connection; - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); - ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + // Wait for the raw TCP connection with bad credentials and close it. + if (upstreamProtocol() != FakeHttpConnection::Type::HTTP3) { + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + } - // Failure + test_server_->waitForCounterGe("sds.client_cert.update_rejected", 1); EXPECT_EQ(0, test_server_->counter("sds.client_cert.update_success")->value()); - EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_rejected")->value()); sendSdsResponse(getClientSecret()); test_server_->waitForCounterGe( "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); - testRouterHeaderOnlyRequestAndResponse(); - - // Success - EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_success")->value()); + test_server_->waitForCounterGe("sds.client_cert.update_success", 1); EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_rejected")->value()); + + // Verify the update succeeded. + testRouterHeaderOnlyRequestAndResponse(); } // Test CDS with SDS. A cluster provided by CDS raises new SDS request for upstream cert. @@ -842,8 +857,8 @@ class SdsCdsIntegrationTest : public SdsDynamicIntegrationBaseTest { FakeStreamPtr sds_stream_; }; -INSTANTIATE_TEST_SUITE_P(IpVersions, SdsCdsIntegrationTest, - testing::ValuesIn(getSdsTestsParams(true)), sdsTestParamsToString); +INSTANTIATE_TEST_SUITE_P(IpVersions, SdsCdsIntegrationTest, testing::ValuesIn(getSdsTestsParams()), + sdsTestParamsToString); TEST_P(SdsCdsIntegrationTest, BasicSuccess) { on_server_init_function_ = [this]() { From bf3e6a24080b67b13867f649cdd7898d971d8d32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Rial?= <19420029+rialg@users.noreply.github.com> Date: Wed, 19 May 2021 22:28:21 +0200 Subject: [PATCH 06/21] safe_memcpy_test: Explicit type for arguments of the vector constructor (#16549) Signed-off-by: rialg --- test/common/common/safe_memcpy_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/common/safe_memcpy_test.cc b/test/common/common/safe_memcpy_test.cc index bbe8010205ec..1f5f39b5518e 100644 --- a/test/common/common/safe_memcpy_test.cc +++ b/test/common/common/safe_memcpy_test.cc @@ -37,7 +37,7 @@ TEST(SafeMemcpyUnsafeDstTest, PrependGrpcFrameHeader) { uint8_t* dst = new uint8_t[4]; memset(dst, 0, 4); safeMemcpyUnsafeDst(dst, &src); - EXPECT_THAT(std::vector(dst, dst + sizeof(src)), ElementsAre(1, 2, 3, 4)); + EXPECT_THAT(std::vector(dst, dst + sizeof(src)), ElementsAre(1, 2, 3, 4)); delete[] dst; } From 94d11374c57dcfb32cdb6f61bb93b64784cb4b34 Mon Sep 17 00:00:00 2001 From: James Peach Date: Thu, 20 May 2021 07:32:52 +1000 Subject: [PATCH 07/21] quic: reduce socket option header exposure (#16541) Move the inclusion of `socket_option_impl.h` from the header to the source to reduce its exposure. Signed-off-by: James Peach --- source/common/quic/active_quic_listener.cc | 5 +++-- source/common/quic/active_quic_listener.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/source/common/quic/active_quic_listener.cc b/source/common/quic/active_quic_listener.cc index 2421b70e168b..ff1f8d2c6462 100644 --- a/source/common/quic/active_quic_listener.cc +++ b/source/common/quic/active_quic_listener.cc @@ -8,15 +8,16 @@ #include -#include "common/runtime/runtime_features.h" #include "common/http/utility.h" +#include "common/network/socket_option_impl.h" #include "common/quic/envoy_quic_alarm_factory.h" #include "common/quic/envoy_quic_connection_helper.h" #include "common/quic/envoy_quic_dispatcher.h" +#include "common/quic/envoy_quic_packet_writer.h" #include "common/quic/envoy_quic_proof_source.h" #include "common/quic/envoy_quic_utils.h" -#include "common/quic/envoy_quic_packet_writer.h" #include "common/quic/envoy_quic_utils.h" +#include "common/runtime/runtime_features.h" namespace Envoy { namespace Quic { diff --git a/source/common/quic/active_quic_listener.h b/source/common/quic/active_quic_listener.h index a923afe09acb..fb3f1b78cf4c 100644 --- a/source/common/quic/active_quic_listener.h +++ b/source/common/quic/active_quic_listener.h @@ -3,9 +3,9 @@ #include "envoy/config/listener/v3/quic_config.pb.h" #include "envoy/network/connection_handler.h" #include "envoy/network/listener.h" +#include "envoy/network/socket.h" #include "envoy/runtime/runtime.h" -#include "common/network/socket_option_impl.h" #include "common/protobuf/utility.h" #include "common/quic/envoy_quic_dispatcher.h" #include "common/runtime/runtime_protos.h" From c468e573a45ceae3b503267a440960f6a374729a Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 19 May 2021 21:49:25 -0400 Subject: [PATCH 08/21] http3: cleaning up TODO (#16547) Signed-off-by: Alyssa Wilk --- .../common/quic/client_connection_factory_impl.cc | 14 +------------- .../common/quic/client_connection_factory_impl.h | 3 +++ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/source/common/quic/client_connection_factory_impl.cc b/source/common/quic/client_connection_factory_impl.cc index 9dcda4745520..829ea0ddc67e 100644 --- a/source/common/quic/client_connection_factory_impl.cc +++ b/source/common/quic/client_connection_factory_impl.cc @@ -50,17 +50,6 @@ PersistentQuicInfoImpl::PersistentQuicInfoImpl( quiche::FlagRegistry::getInstance(); } -namespace { -// TODO(alyssawilk, danzh2010): This is mutable static info that is required for the QUICHE code. -// This was preexisting but should either be removed or potentially moved inside -// PersistentQuicInfoImpl. -struct StaticInfo { - quic::QuicClientPushPromiseIndex push_promise_index_; - - static StaticInfo& get() { MUTABLE_CONSTRUCT_ON_FIRST_USE(StaticInfo); } -}; -} // namespace - std::unique_ptr createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr server_addr, @@ -77,7 +66,6 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d quic::QuicUtils::CreateRandomConnectionId(), server_addr, info_impl->conn_helper_, info_impl->alarm_factory_, quic::ParsedQuicVersionVector{info_impl->supported_versions_[0]}, local_addr, dispatcher, nullptr); - auto& static_info = StaticInfo::get(); ASSERT(!info_impl->supported_versions_.empty()); // QUICHE client session always use the 1st version to start handshake. @@ -85,7 +73,7 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d // send_buffer_limit instead of using 0. auto ret = std::make_unique( info_impl->quic_config_, info_impl->supported_versions_, std::move(connection), - info_impl->server_id_, std::move(config), &static_info.push_promise_index_, dispatcher, + info_impl->server_id_, std::move(config), &info_impl->push_promise_index_, dispatcher, /*send_buffer_limit=*/0); return ret; } diff --git a/source/common/quic/client_connection_factory_impl.h b/source/common/quic/client_connection_factory_impl.h index bf56a95f3e84..ad8334589af3 100644 --- a/source/common/quic/client_connection_factory_impl.h +++ b/source/common/quic/client_connection_factory_impl.h @@ -42,6 +42,9 @@ struct PersistentQuicInfoImpl : public Http::PersistentQuicInfo { const quic::ParsedQuicVersionVector supported_versions_{quic::CurrentSupportedVersions()}; // TODO(alyssawilk) actually set this up properly. quic::QuicConfig quic_config_; + // This arguably should not be shared across connections but as Envoy doesn't + // support push promise it's really moot point. + quic::QuicClientPushPromiseIndex push_promise_index_; }; std::unique_ptr From fe580239279e5df11071edc00f7aefc70b142918 Mon Sep 17 00:00:00 2001 From: Long Dai Date: Thu, 20 May 2021 11:14:16 +0800 Subject: [PATCH 09/21] PULL_REQUEST_TEMPLATE.md: hide example (#16538) Signed-off-by: Long Dai --- PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md index f9425aec96d7..73e13c82eea3 100644 --- a/PULL_REQUEST_TEMPLATE.md +++ b/PULL_REQUEST_TEMPLATE.md @@ -8,9 +8,9 @@ Thank you in advance for helping to keep Envoy secure. !!!ATTENTION!!! ---> For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md) +--> Commit Message: Additional Description: From d304a2f3260c0aa8839c4623ebadf1265b748a3d Mon Sep 17 00:00:00 2001 From: Omri Zohar Date: Thu, 20 May 2021 06:17:43 +0300 Subject: [PATCH 10/21] Fixing GRPC initial metadata validation (#16414) According to the GRPC spec https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md The values in the initial metadata can be any valid ASCII character in case of a non binary header type. Risk Level: Low Testing: Unit Tests Docs Changes:N/A Release Notes: Fixing GRPC initial metadata validation for ASCII characters Signed-off-by: Omri Zohar --- .../common/grpc/async_client_manager_impl.cc | 26 +++++++++++++++++-- .../grpc/async_client_manager_impl_test.cc | 24 +++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index 14866aa25d0e..7b122e8bd59e 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -3,8 +3,11 @@ #include "envoy/config/core/v3/grpc_service.pb.h" #include "envoy/stats/scope.h" +#include "common/common/base64.h" #include "common/grpc/async_client_impl.h" +#include "absl/strings/match.h" + #ifdef ENVOY_GOOGLE_GRPC #include "common/grpc/google_async_client_impl.h" #endif @@ -24,6 +27,15 @@ bool validateGrpcHeaderChars(absl::string_view key) { return true; } +bool validateGrpcCompatibleAsciiHeaderValue(absl::string_view h_value) { + for (auto ch : h_value) { + if (ch < 0x20 || ch > 0x7e) { + return false; + } + } + return true; +} + } // namespace AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm, @@ -84,8 +96,18 @@ GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl( // Check metadata for gRPC API compliance. Uppercase characters are lowered in the HeaderParser. // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md for (const auto& header : config.initial_metadata()) { - if (!validateGrpcHeaderChars(header.key()) || !validateGrpcHeaderChars(header.value())) { - throw EnvoyException("Illegal characters in gRPC initial metadata."); + // Validate key + if (!validateGrpcHeaderChars(header.key())) { + throw EnvoyException( + fmt::format("Illegal characters in gRPC initial metadata header key: {}.", header.key())); + } + + // Validate value + // Binary base64 encoded - handled by the GRPC library + if (!::absl::EndsWith(header.key(), "-bin") && + !validateGrpcCompatibleAsciiHeaderValue(header.value())) { + throw EnvoyException(fmt::format( + "Illegal ASCII value for gRPC initial metadata header key: {}.", header.key())); } } } diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index 76c7c869a3b8..72d8507ed81e 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -88,7 +88,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpc) { #endif } -TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) { +TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInKey) { EXPECT_CALL(scope_, createScope_("grpc.foo.")); envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); @@ -100,7 +100,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) { #ifdef ENVOY_GOOGLE_GRPC EXPECT_THROW_WITH_MESSAGE( async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, - "Illegal characters in gRPC initial metadata."); + "Illegal characters in gRPC initial metadata header key: illegalcharacter;."); #else EXPECT_THROW_WITH_MESSAGE( async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, @@ -126,6 +126,26 @@ TEST_F(AsyncClientManagerImplTest, LegalGoogleGrpcChar) { #endif } +TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInValue) { + EXPECT_CALL(scope_, createScope_("grpc.foo.")); + envoy::config::core::v3::GrpcService grpc_service; + grpc_service.mutable_google_grpc()->set_stat_prefix("foo"); + + auto& metadata = *grpc_service.mutable_initial_metadata()->Add(); + metadata.set_key("legal-key"); + metadata.set_value("NonAsciValue.भारत"); + +#ifdef ENVOY_GOOGLE_GRPC + EXPECT_THROW_WITH_MESSAGE( + async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, + "Illegal ASCII value for gRPC initial metadata header key: legal-key."); +#else + EXPECT_THROW_WITH_MESSAGE( + async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, + "Google C++ gRPC client is not linked"); +#endif +} + TEST_F(AsyncClientManagerImplTest, EnvoyGrpcUnknownOk) { envoy::config::core::v3::GrpcService grpc_service; grpc_service.mutable_envoy_grpc()->set_cluster_name("foo"); From c307494627e6f52154c3437fc7aea4c47dce07f0 Mon Sep 17 00:00:00 2001 From: Greg Brail Date: Thu, 20 May 2021 04:50:00 -0700 Subject: [PATCH 11/21] ext_proc: Support CONTINUE_AND_REPLACE from header callbacks (#16437) This makes it possible for a processor to add a body to a request or response that does not have one, or replace the entire body in the response from a header callback without otherwise touching it. Signed-off-by: Gregory Brail --- .../ext_proc/v3alpha/external_processor.proto | 21 ++-- .../ext_proc/v3alpha/external_processor.proto | 21 ++-- source/extensions/filters/http/ext_proc/BUILD | 1 + .../filters/http/ext_proc/ext_proc.cc | 49 ++++----- .../filters/http/ext_proc/ext_proc.h | 4 +- .../filters/http/ext_proc/mutation_utils.cc | 34 ++++-- .../filters/http/ext_proc/mutation_utils.h | 11 +- .../filters/http/ext_proc/processor_state.cc | 67 ++++++++--- .../filters/http/ext_proc/processor_state.h | 9 +- test/extensions/filters/http/ext_proc/BUILD | 1 + .../ext_proc/ext_proc_integration_test.cc | 87 ++++++++++++++- .../filters/http/ext_proc/filter_test.cc | 104 ++++++++++++++++++ .../http/ext_proc/mutation_utils_test.cc | 4 +- 13 files changed, 326 insertions(+), 87 deletions(-) diff --git a/api/envoy/service/ext_proc/v3alpha/external_processor.proto b/api/envoy/service/ext_proc/v3alpha/external_processor.proto index e57ea2da9e40..09572331aa42 100644 --- a/api/envoy/service/ext_proc/v3alpha/external_processor.proto +++ b/api/envoy/service/ext_proc/v3alpha/external_processor.proto @@ -231,15 +231,18 @@ message CommonResponse { // stream as normal. This is the default. CONTINUE = 0; - // [#not-implemented-hide:] - // Replace the request or response with the contents - // of this message. If header_mutation is set, apply it to the - // headers. If body_mutation is set and contains a body, then add that - // body to the request or response, even if one does not already exist -- - // otherwise, clear the body. Any additional body and trailers - // received from downstream or upstream will be ignored. - // This can be used to add a body to a request or response that does not - // have one already. + // Apply the specified header mutation, replace the body with the body + // specified in the body mutation (if present), and do not send any + // further messages for this request or response even if the processing + // mode is configured to do so. + // + // When used in response to a request_headers or response_headers message, + // this status makes it possible to either completely replace the body + // while discarding the original body, or to add a body to a message that + // formerly did not have one. + // + // In other words, this response makes it possible to turn an HTTP GET + // into a POST, PUT, or PATCH. CONTINUE_AND_REPLACE = 1; } diff --git a/generated_api_shadow/envoy/service/ext_proc/v3alpha/external_processor.proto b/generated_api_shadow/envoy/service/ext_proc/v3alpha/external_processor.proto index e57ea2da9e40..09572331aa42 100644 --- a/generated_api_shadow/envoy/service/ext_proc/v3alpha/external_processor.proto +++ b/generated_api_shadow/envoy/service/ext_proc/v3alpha/external_processor.proto @@ -231,15 +231,18 @@ message CommonResponse { // stream as normal. This is the default. CONTINUE = 0; - // [#not-implemented-hide:] - // Replace the request or response with the contents - // of this message. If header_mutation is set, apply it to the - // headers. If body_mutation is set and contains a body, then add that - // body to the request or response, even if one does not already exist -- - // otherwise, clear the body. Any additional body and trailers - // received from downstream or upstream will be ignored. - // This can be used to add a body to a request or response that does not - // have one already. + // Apply the specified header mutation, replace the body with the body + // specified in the body mutation (if present), and do not send any + // further messages for this request or response even if the processing + // mode is configured to do so. + // + // When used in response to a request_headers or response_headers message, + // this status makes it possible to either completely replace the body + // while discarding the original body, or to add a body to a message that + // formerly did not have one. + // + // In other words, this response makes it possible to turn an HTTP GET + // into a POST, PUT, or PATCH. CONTINUE_AND_REPLACE = 1; } diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index 1468c4d9e0f8..1a0dbe8a05a5 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -26,6 +26,7 @@ envoy_cc_library( "//include/envoy/http:filter_interface", "//include/envoy/http:header_map_interface", "//include/envoy/stats:stats_macros", + "//source/common/buffer:buffer_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "@com_google_absl//absl/strings:str_format", "@envoy_api//envoy/extensions/filters/http/ext_proc/v3alpha:pkg_cc_proto", diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 941ecf4c1b1c..3467dfd01fd2 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -60,8 +60,8 @@ void Filter::onDestroy() { } } -FilterHeadersStatus Filter::onHeaders(ProcessorState& state, Http::HeaderMap& headers, - bool end_stream) { +FilterHeadersStatus Filter::onHeaders(ProcessorState& state, + Http::RequestOrResponseHeaderMap& headers, bool end_stream) { switch (openStream()) { case StreamOpenState::Error: return FilterHeadersStatus::StopIteration; @@ -75,7 +75,7 @@ FilterHeadersStatus Filter::onHeaders(ProcessorState& state, Http::HeaderMap& he state.setHeaders(&headers); ProcessingRequest req; auto* headers_req = state.mutableHeaders(req); - MutationUtils::buildHttpHeaders(headers, *headers_req->mutable_headers()); + MutationUtils::headersToProto(headers, *headers_req->mutable_headers()); headers_req->set_end_of_stream(end_stream); state.setCallbackState(ProcessorState::CallbackState::HeadersCallback); state.startMessageTimer(std::bind(&Filter::onMessageTimeout, this), config_->messageTimeout()); @@ -101,12 +101,22 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st return status; } -Http::FilterDataStatus Filter::onData(ProcessorState& state, Buffer::Instance& data, - bool end_stream) { +FilterDataStatus Filter::onData(ProcessorState& state, Buffer::Instance& data, bool end_stream) { if (end_stream) { state.setCompleteBodyAvailable(true); } + if (state.bodyReplaced()) { + ENVOY_LOG(trace, "Clearing body chunk because CONTINUE_AND_REPLACE was returned"); + data.drain(data.length()); + return FilterDataStatus::Continue; + } + + if (processing_complete_) { + ENVOY_LOG(trace, "Continuing (processing complete)"); + return FilterDataStatus::Continue; + } + bool just_added_trailers = false; Http::HeaderMap* new_trailers = nullptr; if (end_stream && state.sendTrailers()) { @@ -192,17 +202,17 @@ Http::FilterDataStatus Filter::onData(ProcessorState& state, Buffer::Instance& d FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_stream) { ENVOY_LOG(trace, "decodeData({}): end_stream = {}", data.length(), end_stream); - if (processing_complete_) { - ENVOY_LOG(trace, "decodeData: Continue (complete)"); - return FilterDataStatus::Continue; - } - const auto status = onData(decoding_state_, data, end_stream); ENVOY_LOG(trace, "decodeData returning {}", status); return status; } FilterTrailersStatus Filter::onTrailers(ProcessorState& state, Http::HeaderMap& trailers) { + if (processing_complete_) { + ENVOY_LOG(trace, "trailers: Continue"); + return FilterTrailersStatus::Continue; + } + bool body_delivered = state.completeBodyAvailable(); state.setCompleteBodyAvailable(true); state.setTrailersAvailable(true); @@ -243,11 +253,6 @@ FilterTrailersStatus Filter::onTrailers(ProcessorState& state, Http::HeaderMap& FilterTrailersStatus Filter::decodeTrailers(RequestTrailerMap& trailers) { ENVOY_LOG(trace, "decodeTrailers"); - if (processing_complete_) { - ENVOY_LOG(trace, "decodeTrailers: Continue"); - return FilterTrailersStatus::Continue; - } - const auto status = onTrailers(decoding_state_, trailers); ENVOY_LOG(trace, "encodeTrailers returning {}", status); return status; @@ -271,11 +276,6 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s FilterDataStatus Filter::encodeData(Buffer::Instance& data, bool end_stream) { ENVOY_LOG(trace, "encodeData({}): end_stream = {}", data.length(), end_stream); - if (processing_complete_) { - ENVOY_LOG(trace, "encodeData: Continue (complete)"); - return FilterDataStatus::Continue; - } - const auto status = onData(encoding_state_, data, end_stream); ENVOY_LOG(trace, "encodeData returning {}", status); return status; @@ -283,11 +283,6 @@ FilterDataStatus Filter::encodeData(Buffer::Instance& data, bool end_stream) { FilterTrailersStatus Filter::encodeTrailers(ResponseTrailerMap& trailers) { ENVOY_LOG(trace, "encodeTrailers"); - if (processing_complete_) { - ENVOY_LOG(trace, "encodeTrailers: Continue"); - return FilterTrailersStatus::Continue; - } - const auto status = onTrailers(encoding_state_, trailers); ENVOY_LOG(trace, "encodeTrailers returning {}", status); return status; @@ -308,7 +303,7 @@ void Filter::sendBodyChunk(ProcessorState& state, const Buffer::Instance& data, void Filter::sendTrailers(ProcessorState& state, const Http::HeaderMap& trailers) { ProcessingRequest req; auto* trailers_req = state.mutableTrailers(req); - MutationUtils::buildHttpHeaders(trailers, *trailers_req->mutable_trailers()); + MutationUtils::headersToProto(trailers, *trailers_req->mutable_trailers()); state.setCallbackState(ProcessorState::CallbackState::TrailersCallback); state.startMessageTimer(std::bind(&Filter::onMessageTimeout, this), config_->messageTimeout()); ENVOY_LOG(debug, "Sending trailers message"); @@ -459,7 +454,7 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { : absl::nullopt; const auto mutate_headers = [&response](Http::ResponseHeaderMap& headers) { if (response.has_headers()) { - MutationUtils::applyHeaderMutations(response.headers(), headers); + MutationUtils::applyHeaderMutations(response.headers(), headers, false); } }; diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 37dc63d646ae..0ccb23162113 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -133,8 +133,8 @@ class Filter : public Logger::Loggable, void sendImmediateResponse(const envoy::service::ext_proc::v3alpha::ImmediateResponse& response); void sendBodyChunk(ProcessorState& state, const Buffer::Instance& data, bool end_stream); - Http::FilterHeadersStatus onHeaders(ProcessorState& state, Http::HeaderMap& headers, - bool end_stream); + Http::FilterHeadersStatus onHeaders(ProcessorState& state, + Http::RequestOrResponseHeaderMap& headers, bool end_stream); Http::FilterDataStatus onData(ProcessorState& state, Buffer::Instance& data, bool end_stream); Http::FilterTrailersStatus onTrailers(ProcessorState& state, Http::HeaderMap& trailers); diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 111e7a537d10..e0a5602030a2 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -16,13 +16,14 @@ using Http::LowerCaseString; using envoy::service::ext_proc::v3alpha::BodyMutation; using envoy::service::ext_proc::v3alpha::BodyResponse; +using envoy::service::ext_proc::v3alpha::CommonResponse; using envoy::service::ext_proc::v3alpha::HeaderMutation; using envoy::service::ext_proc::v3alpha::HeadersResponse; -void MutationUtils::buildHttpHeaders(const Http::HeaderMap& headers_in, - envoy::config::core::v3::HeaderMap& headers_out) { - headers_in.iterate([&headers_out](const Http::HeaderEntry& e) -> Http::HeaderMap::Iterate { - auto* new_header = headers_out.add_headers(); +void MutationUtils::headersToProto(const Http::HeaderMap& headers_in, + envoy::config::core::v3::HeaderMap& proto_out) { + headers_in.iterate([&proto_out](const Http::HeaderEntry& e) -> Http::HeaderMap::Iterate { + auto* new_header = proto_out.add_headers(); new_header->set_key(std::string(e.key().getStringView())); new_header->set_value(std::string(e.value().getStringView())); return Http::HeaderMap::Iterate::Continue; @@ -34,12 +35,14 @@ void MutationUtils::applyCommonHeaderResponse(const HeadersResponse& response, if (response.has_response()) { const auto& common_response = response.response(); if (common_response.has_header_mutation()) { - applyHeaderMutations(common_response.header_mutation(), headers); + applyHeaderMutations(common_response.header_mutation(), headers, + common_response.status() == CommonResponse::CONTINUE_AND_REPLACE); } } } -void MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, Http::HeaderMap& headers) { +void MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, Http::HeaderMap& headers, + bool replacing_message) { for (const auto& remove_header : mutation.remove_headers()) { if (Http::HeaderUtility::isRemovableHeader(remove_header)) { ENVOY_LOG(trace, "Removing header {}", remove_header); @@ -53,7 +56,7 @@ void MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, Http::H if (!sh.has_header()) { continue; } - if (isSettableHeader(sh.header().key())) { + if (isSettableHeader(sh.header().key(), replacing_message)) { // Make "false" the default. This is logical and matches the ext_authz // filter. However, the router handles this same protobuf and uses "true" // as the default instead. @@ -70,14 +73,20 @@ void MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, Http::H } } -void MutationUtils::applyCommonBodyResponse(const BodyResponse& response, Http::HeaderMap* headers, +void MutationUtils::applyCommonBodyResponse(const BodyResponse& response, + Http::RequestOrResponseHeaderMap* headers, Buffer::Instance& buffer) { if (response.has_response()) { const auto& common_response = response.response(); if (headers != nullptr && common_response.has_header_mutation()) { - applyHeaderMutations(common_response.header_mutation(), *headers); + applyHeaderMutations(common_response.header_mutation(), *headers, + common_response.status() == CommonResponse::CONTINUE_AND_REPLACE); } if (common_response.has_body_mutation()) { + if (headers != nullptr) { + // Always clear content length if we can before modifying body + headers->removeContentLength(); + } applyBodyMutations(common_response.body_mutation(), buffer); } } @@ -87,10 +96,13 @@ void MutationUtils::applyBodyMutations(const BodyMutation& mutation, Buffer::Ins switch (mutation.mutation_case()) { case BodyMutation::MutationCase::kClearBody: if (mutation.clear_body()) { + ENVOY_LOG(trace, "Clearing HTTP body"); buffer.drain(buffer.length()); } break; case BodyMutation::MutationCase::kBody: + ENVOY_LOG(trace, "Replacing body of {} bytes with new body of {} bytes", buffer.length(), + mutation.body().size()); buffer.drain(buffer.length()); buffer.add(mutation.body()); break; @@ -103,11 +115,11 @@ void MutationUtils::applyBodyMutations(const BodyMutation& mutation, Buffer::Ins // Ignore attempts to set certain sensitive headers that can break later processing. // We may re-enable some of these after further testing. This logic is specific // to the ext_proc filter so it is not shared with HeaderUtils. -bool MutationUtils::isSettableHeader(absl::string_view key) { +bool MutationUtils::isSettableHeader(absl::string_view key, bool replacing_message) { const auto& headers = Headers::get(); return !absl::EqualsIgnoreCase(key, headers.HostLegacy.get()) && !absl::EqualsIgnoreCase(key, headers.Host.get()) && - !absl::EqualsIgnoreCase(key, headers.Method.get()) && + (!absl::EqualsIgnoreCase(key, headers.Method.get()) || replacing_message) && !absl::EqualsIgnoreCase(key, headers.Scheme.get()) && !absl::StartsWithIgnoreCase(key, headers.prefix()); } diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.h b/source/extensions/filters/http/ext_proc/mutation_utils.h index 7a71c6ad3388..cb700019c5a7 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.h +++ b/source/extensions/filters/http/ext_proc/mutation_utils.h @@ -14,8 +14,8 @@ namespace ExternalProcessing { class MutationUtils : public Logger::Loggable { public: // Convert a header map until a protobuf - static void buildHttpHeaders(const Http::HeaderMap& headers_in, - envoy::config::core::v3::HeaderMap& headers_out); + static void headersToProto(const Http::HeaderMap& headers_in, + envoy::config::core::v3::HeaderMap& proto_out); // Apply mutations that are common to header responses. static void @@ -25,19 +25,20 @@ class MutationUtils : public Logger::Loggable { // Modify header map based on a set of mutations from a protobuf static void applyHeaderMutations(const envoy::service::ext_proc::v3alpha::HeaderMutation& mutation, - Http::HeaderMap& headers); + Http::HeaderMap& headers, bool replacing_message); // Apply mutations that are common to body responses. // Mutations will be applied to the header map if it is not null. static void applyCommonBodyResponse(const envoy::service::ext_proc::v3alpha::BodyResponse& body, - Http::HeaderMap* headers, Buffer::Instance& buffer); + Http::RequestOrResponseHeaderMap* headers, + Buffer::Instance& buffer); // Modify a buffer based on a set of mutations from a protobuf static void applyBodyMutations(const envoy::service::ext_proc::v3alpha::BodyMutation& mutation, Buffer::Instance& buffer); private: - static bool isSettableHeader(absl::string_view key); + static bool isSettableHeader(absl::string_view key, bool replacing_message); }; } // namespace ExternalProcessing diff --git a/source/extensions/filters/http/ext_proc/processor_state.cc b/source/extensions/filters/http/ext_proc/processor_state.cc index 19b0dc8c4ec1..f02faf3e267a 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.cc +++ b/source/extensions/filters/http/ext_proc/processor_state.cc @@ -1,5 +1,8 @@ #include "extensions/filters/http/ext_proc/processor_state.h" +#include "common/buffer/buffer_impl.h" +#include "common/protobuf/utility.h" + #include "extensions/filters/http/ext_proc/ext_proc.h" #include "extensions/filters/http/ext_proc/mutation_utils.h" @@ -11,6 +14,7 @@ namespace ExternalProcessing { using envoy::extensions::filters::http::ext_proc::v3alpha::ProcessingMode; using envoy::service::ext_proc::v3alpha::BodyResponse; +using envoy::service::ext_proc::v3alpha::CommonResponse; using envoy::service::ext_proc::v3alpha::HeadersResponse; using envoy::service::ext_proc::v3alpha::TrailersResponse; @@ -24,6 +28,7 @@ void ProcessorState::startMessageTimer(Event::TimerCb cb, std::chrono::milliseco bool ProcessorState::handleHeadersResponse(const HeadersResponse& response) { if (callback_state_ == CallbackState::HeadersCallback) { ENVOY_LOG(debug, "applying headers response"); + const auto& common_response = response.response(); MutationUtils::applyCommonHeaderResponse(response, *headers_); if (response.response().clear_route_cache()) { filter_callbacks_->clearRouteCache(); @@ -32,26 +37,52 @@ bool ProcessorState::handleHeadersResponse(const HeadersResponse& response) { clearWatermark(); message_timer_->disableTimer(); - if (body_mode_ == ProcessingMode::BUFFERED) { - if (complete_body_available_) { - // If we get here, then all the body data came in before the header message - // was complete, and the server wants the body. So, don't continue filter - // processing, but send the buffered request body now. - ENVOY_LOG(debug, "Sending buffered request body message"); - filter_.sendBufferedData(*this, true); + if (common_response.status() == CommonResponse::CONTINUE_AND_REPLACE) { + ENVOY_LOG(debug, "Replacing complete message"); + // Completely replace the body that may already exist. + if (common_response.has_body_mutation()) { + // Always remove the content-length header if changing the body. + // The proxy can restore it later if it needs to. + headers_->removeContentLength(); + body_replaced_ = true; + if (bufferedData() == nullptr) { + Buffer::OwnedImpl new_body; + MutationUtils::applyBodyMutations(common_response.body_mutation(), new_body); + addBufferedData(new_body); + } else { + modifyBufferedData([&common_response](Buffer::Instance& buf) { + MutationUtils::applyBodyMutations(common_response.body_mutation(), buf); + }); + } } - // Otherwise, we're not ready to continue processing because then - // we won't be able to modify the headers any more, so do nothing and - // let the doData callback handle body chunks until the end is reached. - return true; - } + // Once this message is received, we won't send anything more on this request + // or response to the processor. Clear flags to make sure. + body_mode_ = ProcessingMode::NONE; + send_trailers_ = false; + + } else { + if (body_mode_ == ProcessingMode::BUFFERED) { + if (complete_body_available_) { + // If we get here, then all the body data came in before the header message + // was complete, and the server wants the body. So, don't continue filter + // processing, but send the buffered request body now. + ENVOY_LOG(debug, "Sending buffered request body message"); + filter_.sendBufferedData(*this, true); + } + + // Otherwise, we're not ready to continue processing because then + // we won't be able to modify the headers any more, so do nothing and + // let the doData callback handle body chunks until the end is reached. + return true; + } - if (send_trailers_ && trailers_available_) { - // Trailers came in while we were waiting for this response, and the server - // is not interested in the body, so send them now. - filter_.sendTrailers(*this, *trailers_); - return true; + if (send_trailers_ && trailers_available_) { + // Trailers came in while we were waiting for this response, and the server + // is not interested in the body, so send them now. + filter_.sendTrailers(*this, *trailers_); + return true; + } } // If we got here, then the processor doesn't care about the body or is not ready for @@ -93,7 +124,7 @@ bool ProcessorState::handleTrailersResponse(const TrailersResponse& response) { if (callback_state_ == CallbackState::TrailersCallback) { ENVOY_LOG(debug, "Applying response to buffered trailers"); if (response.has_header_mutation()) { - MutationUtils::applyHeaderMutations(response.header_mutation(), *trailers_); + MutationUtils::applyHeaderMutations(response.header_mutation(), *trailers_, false); } trailers_ = nullptr; callback_state_ = CallbackState::Idle; diff --git a/source/extensions/filters/http/ext_proc/processor_state.h b/source/extensions/filters/http/ext_proc/processor_state.h index 61473e03c0bc..fc3914c66c1d 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.h +++ b/source/extensions/filters/http/ext_proc/processor_state.h @@ -32,7 +32,7 @@ class ProcessorState : public Logger::Loggable { explicit ProcessorState(Filter& filter) : filter_(filter), watermark_requested_(false), complete_body_available_(false), - trailers_available_(false) {} + trailers_available_(false), body_replaced_(false) {} ProcessorState(const ProcessorState&) = delete; virtual ~ProcessorState() = default; ProcessorState& operator=(const ProcessorState&) = delete; @@ -43,6 +43,7 @@ class ProcessorState : public Logger::Loggable { bool completeBodyAvailable() const { return complete_body_available_; } void setCompleteBodyAvailable(bool d) { complete_body_available_ = d; } void setTrailersAvailable(bool d) { trailers_available_ = d; } + bool bodyReplaced() const { return body_replaced_; } virtual void setProcessingMode( const envoy::extensions::filters::http::ext_proc::v3alpha::ProcessingMode& mode) PURE; @@ -53,7 +54,7 @@ class ProcessorState : public Logger::Loggable { return body_mode_; } - void setHeaders(Http::HeaderMap* headers) { headers_ = headers; } + void setHeaders(Http::RequestOrResponseHeaderMap* headers) { headers_ = headers; } void setTrailers(Http::HeaderMap* trailers) { trailers_ = trailers; } void startMessageTimer(Event::TimerCb cb, std::chrono::milliseconds timeout); @@ -95,6 +96,8 @@ class ProcessorState : public Logger::Loggable { bool complete_body_available_ : 1; // If true, then the filter received the trailers bool trailers_available_ : 1; + // If true, then a CONTINUE_AND_REPLACE status was used on a response + bool body_replaced_ : 1; // If true, the server wants to see the headers bool send_headers_ : 1; @@ -104,7 +107,7 @@ class ProcessorState : public Logger::Loggable { // The specific mode for body handling envoy::extensions::filters::http::ext_proc::v3alpha::ProcessingMode_BodySendMode body_mode_; - Http::HeaderMap* headers_ = nullptr; + Http::RequestOrResponseHeaderMap* headers_ = nullptr; Http::HeaderMap* trailers_ = nullptr; Event::TimerPtr message_timer_; }; diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index ae3eeeeb6e90..898226637fad 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -37,6 +37,7 @@ envoy_extension_cc_test( "//test/mocks/event:event_mocks", "//test/mocks/server:factory_context_mocks", "//test/test_common:test_runtime_lib", + "@envoy_api//envoy/service/ext_proc/v3alpha:pkg_cc_proto", ], ) diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 41c432e1dd70..0e3786158db1 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -17,6 +17,7 @@ namespace Envoy { using envoy::extensions::filters::http::ext_proc::v3alpha::ProcessingMode; using envoy::service::ext_proc::v3alpha::BodyResponse; +using envoy::service::ext_proc::v3alpha::CommonResponse; using envoy::service::ext_proc::v3alpha::HeadersResponse; using envoy::service::ext_proc::v3alpha::HttpBody; using envoy::service::ext_proc::v3alpha::HttpHeaders; @@ -568,7 +569,6 @@ TEST_P(ExtProcIntegrationTest, GetAndSetBodyAndHeadersOnResponse) { }); verifyDownstreamResponse(*response, 200); - EXPECT_THAT(response->headers(), SingleHeaderValueIs("content-length", "13")); EXPECT_THAT(response->headers(), SingleHeaderValueIs("x-testing-response-header", "Yes")); EXPECT_EQ("Hello, World!", response->body()); } @@ -834,6 +834,91 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyOnResponseBody) { EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body()); } +// Test the ability of the filter to turn a GET into a POST by adding a body +// and changing the method. +TEST_P(ExtProcIntegrationTest, ConvertGetToPost) { + initializeConfig(); + HttpIntegrationTest::initialize(); + + auto response = sendDownstreamRequest(absl::nullopt); + + processRequestHeadersMessage(true, [](const HttpHeaders&, HeadersResponse& headers_resp) { + auto* header_mut = headers_resp.mutable_response()->mutable_header_mutation(); + auto* method = header_mut->add_set_headers(); + method->mutable_header()->set_key(":method"); + method->mutable_header()->set_value("POST"); + auto* content_type = header_mut->add_set_headers(); + content_type->mutable_header()->set_key("content-type"); + content_type->mutable_header()->set_value("text/plain"); + headers_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, Server!"); + // This special status tells us to replace the whole request + headers_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); + return true; + }); + + handleUpstreamRequest(); + + EXPECT_THAT(upstream_request_->headers(), SingleHeaderValueIs(":method", "POST")); + EXPECT_THAT(upstream_request_->headers(), SingleHeaderValueIs("content-type", "text/plain")); + EXPECT_EQ(upstream_request_->bodyLength(), 14); + EXPECT_EQ(upstream_request_->body().toString(), "Hello, Server!"); + + processResponseHeadersMessage(false, absl::nullopt); + verifyDownstreamResponse(*response, 200); +} + +// Test the ability of the filter to completely replace a request message with a new +// request message. +TEST_P(ExtProcIntegrationTest, ReplaceCompleteRequest) { + initializeConfig(); + HttpIntegrationTest::initialize(); + + auto response = sendDownstreamRequestWithBody("Replace this!", absl::nullopt); + + processRequestHeadersMessage(true, [](const HttpHeaders&, HeadersResponse& headers_resp) { + headers_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, Server!"); + // This special status tells us to replace the whole request + headers_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); + return true; + }); + + handleUpstreamRequest(); + + // Ensure that we replaced and did not append to the request. + EXPECT_EQ(upstream_request_->body().toString(), "Hello, Server!"); + + processResponseHeadersMessage(false, absl::nullopt); + verifyDownstreamResponse(*response, 200); +} + +// Test the ability of the filter to completely replace a request message with a new +// request message. +TEST_P(ExtProcIntegrationTest, ReplaceCompleteRequestBuffered) { + proto_config_.mutable_processing_mode()->set_request_body_mode(ProcessingMode::BUFFERED); + initializeConfig(); + HttpIntegrationTest::initialize(); + + auto response = sendDownstreamRequestWithBody("Replace this!", absl::nullopt); + + processRequestHeadersMessage(true, [](const HttpHeaders&, HeadersResponse& headers_resp) { + headers_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, Server!"); + // This special status tells us to replace the whole request + headers_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); + return true; + }); + + // Even though we set the body mode to BUFFERED, we should receive no callback because + // we returned CONTINUE_AND_REPLACE. + + handleUpstreamRequest(); + + // Ensure that we replaced and did not append to the request. + EXPECT_EQ(upstream_request_->body().toString(), "Hello, Server!"); + + processResponseHeadersMessage(false, absl::nullopt); + verifyDownstreamResponse(*response, 200); +} + // Send a request, but wait longer than the "message timeout" before sending a response // from the external processor. This should trigger the timeout and result // in a 500 error. diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 5fb454b97f1c..804d1cb4ba4c 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -1,3 +1,5 @@ +#include "envoy/service/ext_proc/v3alpha/external_processor.pb.h" + #include "extensions/filters/http/ext_proc/ext_proc.h" #include "test/common/http/common.h" @@ -24,6 +26,7 @@ namespace { using envoy::extensions::filters::http::ext_proc::v3alpha::ProcessingMode; using envoy::service::ext_proc::v3alpha::BodyResponse; +using envoy::service::ext_proc::v3alpha::CommonResponse; using envoy::service::ext_proc::v3alpha::HeadersResponse; using envoy::service::ext_proc::v3alpha::HttpBody; using envoy::service::ext_proc::v3alpha::HttpHeaders; @@ -1319,6 +1322,107 @@ TEST_F(HttpFilterTest, ClearRouteCache) { EXPECT_EQ(1, config_->stats().streams_closed_.value()); } +// Using the default configuration, turn a GET into a POST. +TEST_F(HttpFilterTest, ReplaceRequest) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + )EOF"); + + HttpTestUtility::addDefaultHeaders(request_headers_); + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + + Buffer::OwnedImpl req_buffer; + setUpDecodingBuffering(req_buffer); + processRequestHeaders( + false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& hdrs_resp) { + hdrs_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); + auto* hdr = hdrs_resp.mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->mutable_header()->set_key(":method"); + hdr->mutable_header()->set_value("POST"); + hdrs_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, World!"); + }); + + Http::TestRequestHeaderMapImpl expected_request{ + {":scheme", "http"}, {":authority", "host"}, {":path", "/"}, {":method", "POST"}}; + EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected_request)); + EXPECT_EQ(req_buffer.toString(), "Hello, World!"); + + response_headers_.addCopy(LowerCaseString(":status"), "200"); + response_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); + response_headers_.addCopy(LowerCaseString("content-length"), "200"); + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + processResponseHeaders(false, absl::nullopt); + + Buffer::OwnedImpl resp_data_1; + TestUtility::feedBufferWithRandomCharacters(resp_data_1, 100); + EXPECT_EQ(FilterDataStatus::Continue, filter_->encodeData(resp_data_1, true)); + + filter_->onDestroy(); + + EXPECT_EQ(1, config_->stats().streams_started_.value()); + EXPECT_EQ(2, config_->stats().stream_msgs_sent_.value()); + EXPECT_EQ(2, config_->stats().stream_msgs_received_.value()); + EXPECT_EQ(1, config_->stats().streams_closed_.value()); +} + +// Using a configuration with response mode set up for buffering, replace the complete response. +// This should result in none of the actual response coming back and no callbacks being +// fired. +TEST_F(HttpFilterTest, ReplaceCompleteResponseBuffered) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + processing_mode: + response_body_mode: "BUFFERED" + )EOF"); + + HttpTestUtility::addDefaultHeaders(request_headers_); + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + processRequestHeaders(false, absl::nullopt); + + response_headers_.addCopy(LowerCaseString(":status"), "200"); + response_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); + response_headers_.addCopy(LowerCaseString("content-length"), "200"); + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + + Buffer::OwnedImpl resp_data_1; + TestUtility::feedBufferWithRandomCharacters(resp_data_1, 100); + Buffer::OwnedImpl resp_data_2; + TestUtility::feedBufferWithRandomCharacters(resp_data_2, 100); + Buffer::OwnedImpl buffered_resp_data; + setUpEncodingBuffering(buffered_resp_data); + + processResponseHeaders( + false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& hdrs_resp) { + hdrs_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); + auto* hdr = hdrs_resp.mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->mutable_header()->set_key("x-test-header"); + hdr->mutable_header()->set_value("true"); + hdrs_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, World!"); + }); + + // Ensure buffered data was updated + EXPECT_EQ(buffered_resp_data.toString(), "Hello, World!"); + + // Since we did CONTINUE_AND_REPLACE, later data is cleared + EXPECT_EQ(FilterDataStatus::Continue, filter_->encodeData(resp_data_1, false)); + EXPECT_EQ(resp_data_1.length(), 0); + EXPECT_EQ(FilterDataStatus::Continue, filter_->encodeData(resp_data_2, true)); + EXPECT_EQ(resp_data_2.length(), 0); + + // No additional messages should come in since we replaced, although they + // are configured. + filter_->onDestroy(); + + EXPECT_EQ(1, config_->stats().streams_started_.value()); + EXPECT_EQ(2, config_->stats().stream_msgs_sent_.value()); + EXPECT_EQ(2, config_->stats().stream_msgs_received_.value()); + EXPECT_EQ(1, config_->stats().streams_closed_.value()); +} + // Using the default configuration, test the filter with a processor that // replies to the request_headers message incorrectly by sending a // request_body message, which should result in the stream being closed diff --git a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc index 6eb0da487c4b..4096fcc68a26 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -28,7 +28,7 @@ TEST(MutationUtils, TestBuildHeaders) { headers.addCopy(LowerCaseString("x-number"), 9999); envoy::config::core::v3::HeaderMap proto_headers; - MutationUtils::buildHttpHeaders(headers, proto_headers); + MutationUtils::headersToProto(headers, proto_headers); Http::TestRequestHeaderMapImpl expected{{":method", "GET"}, {":path", "/foo/the/bar?size=123"}, @@ -101,7 +101,7 @@ TEST(MutationUtils, TestApplyMutations) { s->mutable_header()->set_key("X-Envoy-StrangeThing"); s->mutable_header()->set_value("Yes"); - MutationUtils::applyHeaderMutations(mutation, headers); + MutationUtils::applyHeaderMutations(mutation, headers, false); Http::TestRequestHeaderMapImpl expected_headers{ {":scheme", "https"}, From 17aa841711edf91a84e6f8e5e360ee5673d4fa18 Mon Sep 17 00:00:00 2001 From: phlax Date: Thu, 20 May 2021 12:54:29 +0100 Subject: [PATCH 12/21] docker: Use entrypoint for distroless image (#16383) Signed-off-by: Ryan Northey --- ci/Dockerfile-envoy-distroless | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ci/Dockerfile-envoy-distroless b/ci/Dockerfile-envoy-distroless index 98bc2cfca8a9..630ccac08304 100644 --- a/ci/Dockerfile-envoy-distroless +++ b/ci/Dockerfile-envoy-distroless @@ -7,4 +7,5 @@ ADD linux/amd64/build_release${ENVOY_BINARY_SUFFIX}/* /usr/local/bin/ EXPOSE 10000 -CMD ["envoy", "-c", "/etc/envoy/envoy.yaml"] +ENTRYPOINT ["envoy"] +CMD ["-c", "/etc/envoy/envoy.yaml"] From 75aecf22afb376874be0d55b03f14034dca96557 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 20 May 2021 08:28:36 -0400 Subject: [PATCH 13/21] quic: improve coverage (#16569) Risk Level: n/a (test only) Testing: yes Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk --- .../quic_filter_manager_connection_impl.h | 3 +- source/common/quic/udp_gso_batch_writer.cc | 12 +-- test/common/http/conn_pool_grid_test.cc | 1 + test/common/quic/BUILD | 24 ++++++ .../client_connection_factory_impl_test.cc | 47 +++++++++++ .../quic/quic_io_handle_wrapper_test.cc | 81 +++++++++++++++++++ 6 files changed, 159 insertions(+), 9 deletions(-) create mode 100644 test/common/quic/client_connection_factory_impl_test.cc diff --git a/source/common/quic/quic_filter_manager_connection_impl.h b/source/common/quic/quic_filter_manager_connection_impl.h index c33bc5959d7f..7922810e362c 100644 --- a/source/common/quic/quic_filter_manager_connection_impl.h +++ b/source/common/quic/quic_filter_manager_connection_impl.h @@ -77,7 +77,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, absl::optional unixSocketPeerCredentials() const override { // Unix domain socket is not supported. - NOT_REACHED_GCOVR_EXCL_LINE; + return absl::nullopt; } void setConnectionStats(const Network::Connection::ConnectionStats& stats) override { // TODO(danzh): populate stats. @@ -113,6 +113,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, const StreamInfo::StreamInfo& streamInfo() const override { return stream_info_; } absl::string_view transportFailureReason() const override { return transport_failure_reason_; } bool startSecureTransport() override { return false; } + // TODO(#2557) Implement this. absl::optional lastRoundTripTime() const override { return {}; } // Network::FilterManagerConnection diff --git a/source/common/quic/udp_gso_batch_writer.cc b/source/common/quic/udp_gso_batch_writer.cc index d54ceaf790a9..8f46df35f2ea 100644 --- a/source/common/quic/udp_gso_batch_writer.cc +++ b/source/common/quic/udp_gso_batch_writer.cc @@ -8,7 +8,7 @@ namespace Quic { namespace { Api::IoCallUint64Result convertQuicWriteResult(quic::WriteResult quic_result, size_t payload_len) { switch (quic_result.status) { - case quic::WRITE_STATUS_OK: { + case quic::WRITE_STATUS_OK: if (quic_result.bytes_written == 0) { ENVOY_LOG_MISC(trace, "sendmsg successful, message buffered to send"); } else { @@ -18,23 +18,20 @@ Api::IoCallUint64Result convertQuicWriteResult(quic::WriteResult quic_result, si return Api::IoCallUint64Result( /*rc=*/payload_len, /*err=*/Api::IoErrorPtr(nullptr, Network::IoSocketError::deleteIoError)); - } - case quic::WRITE_STATUS_BLOCKED_DATA_BUFFERED: { + case quic::WRITE_STATUS_BLOCKED_DATA_BUFFERED: // Data was buffered, Return payload_len as rc & nullptr as error ENVOY_LOG_MISC(trace, "sendmsg blocked, message buffered to send"); return Api::IoCallUint64Result( /*rc=*/payload_len, /*err=*/Api::IoErrorPtr(nullptr, Network::IoSocketError::deleteIoError)); - } - case quic::WRITE_STATUS_BLOCKED: { + case quic::WRITE_STATUS_BLOCKED: // Writer blocked, return error ENVOY_LOG_MISC(trace, "sendmsg blocked, message not buffered"); return Api::IoCallUint64Result( /*rc=*/0, /*err=*/Api::IoErrorPtr(Network::IoSocketError::getIoSocketEagainInstance(), Network::IoSocketError::deleteIoError)); - } - default: { + default: // Write Failed, return {0 and error_code} ENVOY_LOG_MISC(trace, "sendmsg failed with error code {}", static_cast(quic_result.error_code)); @@ -43,7 +40,6 @@ Api::IoCallUint64Result convertQuicWriteResult(quic::WriteResult quic_result, si /*err=*/Api::IoErrorPtr(new Network::IoSocketError(quic_result.error_code), Network::IoSocketError::deleteIoError)); } - } } } // namespace diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index 115009c37b56..e25551aaaf5d 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -610,6 +610,7 @@ TEST_F(ConnectivityGridTest, RealGrid) { Envoy::Ssl::ClientContextConfigPtr config(new NiceMock()); auto factory = std::make_unique(std::move(config)); factory->initialize(); + ASSERT_FALSE(factory->usesProxyProtocolOptions()); auto& matcher = static_cast(*cluster_->transport_socket_matcher_); EXPECT_CALL(matcher, resolve(_)) diff --git a/test/common/quic/BUILD b/test/common/quic/BUILD index f187dc762949..9f432b55c200 100644 --- a/test/common/quic/BUILD +++ b/test/common/quic/BUILD @@ -249,6 +249,29 @@ envoy_cc_test_library( ], ) +envoy_cc_test( + name = "client_connection_factory_impl_test", + srcs = ["client_connection_factory_impl_test.cc"], + tags = ["nofips"], + deps = [ + "//source/common/event:dispatcher_lib", + "//source/common/http/http3:conn_pool_lib", + "//source/common/network:utility_lib", + "//source/common/upstream:upstream_includes", + "//source/common/upstream:upstream_lib", + "//test/common/http:common_lib", + "//test/common/upstream:utility_lib", + "//test/mocks/event:event_mocks", + "//test/mocks/http:http_mocks", + "//test/mocks/network:network_mocks", + "//test/mocks/runtime:runtime_mocks", + "//test/mocks/server:transport_socket_factory_context_mocks", + "//test/mocks/upstream:cluster_info_mocks", + "//test/mocks/upstream:transport_socket_match_mocks", + "//test/test_common:test_runtime_lib", + ], +) + envoy_cc_test( name = "quic_io_handle_wrapper_test", srcs = ["quic_io_handle_wrapper_test.cc"], @@ -256,6 +279,7 @@ envoy_cc_test( deps = [ "//source/common/quic:quic_io_handle_wrapper_lib", "//test/mocks/api:api_mocks", + "//test/mocks/network:io_handle_mocks", "//test/mocks/network:network_mocks", "//test/test_common:threadsafe_singleton_injector_lib", ], diff --git a/test/common/quic/client_connection_factory_impl_test.cc b/test/common/quic/client_connection_factory_impl_test.cc new file mode 100644 index 000000000000..c919aa9165b0 --- /dev/null +++ b/test/common/quic/client_connection_factory_impl_test.cc @@ -0,0 +1,47 @@ +#include "common/quic/client_connection_factory_impl.h" +#include "common/quic/quic_transport_socket_factory.h" + +#include "test/common/upstream/utility.h" +#include "test/mocks/common.h" +#include "test/mocks/event/mocks.h" +#include "test/mocks/server/transport_socket_factory_context.h" +#include "test/mocks/ssl/mocks.h" +#include "test/mocks/upstream/cluster_info.h" +#include "test/mocks/upstream/host.h" +#include "test/test_common/simulated_time_system.h" + +namespace Envoy { +namespace Quic { + +class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public testing::Test { +protected: + NiceMock dispatcher_; + std::shared_ptr cluster_{new NiceMock()}; + Upstream::HostSharedPtr host_{new NiceMock}; + NiceMock random_; + Upstream::ClusterConnectivityState state_; + Network::Address::InstanceConstSharedPtr test_address_ = + Network::Utility::resolveUrl("tcp://127.0.0.1:3000"); + NiceMock context_; + Quic::QuicClientTransportSocketFactory factory_{ + std::unique_ptr(new NiceMock), + context_}; +}; + +TEST_F(QuicNetworkConnectionTest, BufferLimits) { + PersistentQuicInfoImpl info{dispatcher_, factory_, simTime(), test_address_}; + + std::unique_ptr client_connection = + createQuicNetworkConnection(info, dispatcher_, test_address_, test_address_); + EnvoyQuicClientSession* session = static_cast(client_connection.get()); + session->Initialize(); + client_connection->connect(); + EXPECT_TRUE(client_connection->connecting()); + ASSERT(session != nullptr); + EXPECT_EQ(absl::nullopt, session->unixSocketPeerCredentials()); + EXPECT_EQ(absl::nullopt, session->lastRoundTripTime()); + client_connection->close(Network::ConnectionCloseType::NoFlush); +} + +} // namespace Quic +} // namespace Envoy diff --git a/test/common/quic/quic_io_handle_wrapper_test.cc b/test/common/quic/quic_io_handle_wrapper_test.cc index d5ee4d081474..2ef58610f06f 100644 --- a/test/common/quic/quic_io_handle_wrapper_test.cc +++ b/test/common/quic/quic_io_handle_wrapper_test.cc @@ -7,12 +7,14 @@ #include "common/quic/quic_io_handle_wrapper.h" #include "test/mocks/api/mocks.h" +#include "test/mocks/network/io_handle.h" #include "test/mocks/network/mocks.h" #include "test/test_common/threadsafe_singleton_injector.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::ByMove; using testing::Return; namespace Envoy { @@ -119,5 +121,84 @@ TEST_F(QuicIoHandleWrapperTest, DelegateIoHandleCalls) { wrapper_->supportsMmsg(); } +TEST(QuicIoHandleWrapper, DelegateWithMocks) { + Network::MockIoHandle mock_io; + QuicIoHandleWrapper wrapper(mock_io); + Buffer::OwnedImpl buffer; + Event::MockDispatcher dispatcher; + Event::FileReadyCb cb; + Event::FileTriggerType trigger = Event::PlatformDefaultTriggerType; + + { + EXPECT_CALL(mock_io, fdDoNotUse()); + wrapper.fdDoNotUse(); + + EXPECT_CALL(mock_io, read(_, _)) + .WillOnce(testing::Return(ByMove(Api::ioCallUint64ResultNoError()))); + wrapper.read(buffer, 5); + + EXPECT_CALL(mock_io, write(_)).WillOnce(Return(ByMove(Api::ioCallUint64ResultNoError()))); + wrapper.write(buffer); + + EXPECT_CALL(mock_io, recv(_, _, _)).WillOnce(Return(ByMove(Api::ioCallUint64ResultNoError()))); + wrapper.recv(nullptr, 10, 0); + + EXPECT_CALL(mock_io, bind(_)).WillOnce(Return(Api::SysCallIntResult{0, 0})); + wrapper.bind(nullptr); + + EXPECT_CALL(mock_io, listen(_)).WillOnce(Return(Api::SysCallIntResult{0, 0})); + wrapper.listen(0); + + EXPECT_CALL(mock_io, accept(_, _)); + wrapper.accept(nullptr, nullptr); + + EXPECT_CALL(mock_io, connect(_)).WillOnce(Return(Api::SysCallIntResult{0, 0})); + wrapper.connect(nullptr); + + EXPECT_CALL(mock_io, setOption(_, _, _, _)).WillOnce(Return(Api::SysCallIntResult{0, 0})); + wrapper.setOption(0, 0, nullptr, 0); + + EXPECT_CALL(mock_io, ioctl(_, _, _, _, _, _)).WillOnce(Return(Api::SysCallIntResult{0, 0})); + wrapper.ioctl(0, nullptr, 0, nullptr, 0, nullptr); + + EXPECT_CALL(mock_io, setBlocking(_)).WillOnce(Return(Api::SysCallIntResult{0, 0})); + wrapper.setBlocking(false); + + EXPECT_CALL(mock_io, createFileEvent_(_, _, _, _)); + wrapper.initializeFileEvent(dispatcher, cb, trigger, 0); + + EXPECT_CALL(mock_io, duplicate); + wrapper.duplicate(); + + EXPECT_CALL(mock_io, activateFileEvents(_)); + wrapper.activateFileEvents(0); + + EXPECT_CALL(mock_io, enableFileEvents(_)); + wrapper.enableFileEvents(0); + + EXPECT_CALL(mock_io, resetFileEvents()); + wrapper.resetFileEvents(); + + EXPECT_CALL(mock_io, shutdown(_)); + wrapper.shutdown(0); + + EXPECT_CALL(mock_io, lastRoundTripTime()).Times(0); + wrapper.lastRoundTripTime(); + } + + wrapper.close(); + + { + EXPECT_CALL(mock_io, read(_, _)).Times(0); + wrapper.read(buffer, 5); + + EXPECT_CALL(mock_io, write(_)).Times(0); + wrapper.write(buffer); + + EXPECT_CALL(mock_io, recv(_, _, _)).Times(0); + ASSERT_DEBUG_DEATH(wrapper.recv(nullptr, 10, 0), "recv called after close"); + } +} + } // namespace Quic } // namespace Envoy From 521843628752b2b554fb1e8cb1b9f8cdf4338125 Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Thu, 20 May 2021 18:55:39 +0530 Subject: [PATCH 14/21] Added default connect_timeout in cluster config (#16453) Signed-off-by: Manish Kumar --- api/envoy/config/cluster/v3/cluster.proto | 1 + api/envoy/config/cluster/v4alpha/cluster.proto | 1 + docs/root/faq/configuration/timeouts.rst | 4 ++-- docs/root/version_history/current.rst | 1 + .../envoy/config/cluster/v3/cluster.proto | 1 + .../envoy/config/cluster/v4alpha/cluster.proto | 1 + source/common/upstream/upstream_impl.cc | 2 +- test/common/upstream/upstream_impl_test.cc | 14 ++++++++++++++ 8 files changed, 22 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/cluster/v3/cluster.proto b/api/envoy/config/cluster/v3/cluster.proto index 7012f4ef5c43..957c2b3341e3 100644 --- a/api/envoy/config/cluster/v3/cluster.proto +++ b/api/envoy/config/cluster/v3/cluster.proto @@ -709,6 +709,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/api/envoy/config/cluster/v4alpha/cluster.proto b/api/envoy/config/cluster/v4alpha/cluster.proto index 807b70a0ce8b..8adf5ea460e4 100644 --- a/api/envoy/config/cluster/v4alpha/cluster.proto +++ b/api/envoy/config/cluster/v4alpha/cluster.proto @@ -718,6 +718,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index fd530474ea34..43a228e0c27a 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -104,8 +104,8 @@ TCP --- * The cluster :ref:`connect_timeout ` specifies the amount - of time Envoy will wait for an upstream TCP connection to be established. This timeout has no - default, but is required in the configuration. + of time Envoy will wait for an upstream TCP connection to be established. If this value is not set, + a default value of 5 seconds will be used. .. attention:: diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index bd265aaccff6..332df9444950 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,6 +20,7 @@ Minor Behavior Changes requests to S3, ES or Glacier, which used the literal string ``UNSIGNED-PAYLOAD``. Buffering can be now be disabled in favor of using unsigned payloads with compatible services via the new `use_unsigned_payload` filter option (default false). +* cluster: added default value of 5 seconds for :ref:`connect_timeout `. * http: disable the integration between :ref:`ExtensionWithMatcher ` and HTTP filters by default to reflects its experimental status. This feature can be enabled by seting ``envoy.reloadable_features.experimental_matching_api`` to true. diff --git a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto index 0e8a711900dd..d9e64b44ce88 100644 --- a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto @@ -710,6 +710,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto index 6cbc477cee88..d637591a2251 100644 --- a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto @@ -718,6 +718,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 52affaa72b46..a4dbd0eeb21a 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -741,7 +741,7 @@ ClusterInfoImpl::ClusterInfoImpl( runtime_.snapshot().getInteger(Http::MaxResponseHeadersCountOverrideKey, Http::DEFAULT_MAX_HEADERS_COUNT))), connect_timeout_( - std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, connect_timeout))), + std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, connect_timeout, 5000))), per_upstream_preconnect_ratio_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( config.preconnect_policy(), per_upstream_preconnect_ratio, 1.0)), peekahead_ratio_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.preconnect_policy(), diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index a72407a7981b..ef1e342c7a7d 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -2612,6 +2612,20 @@ TEST_F(ClusterInfoImplTest, TestTrackRemainingResourcesGauges) { EXPECT_EQ(4U, high_remaining_retries.value()); } +TEST_F(ClusterInfoImplTest, DefaultConnectTimeout) { + const std::string yaml = R"EOF( + name: cluster1 + type: STRICT_DNS + lb_policy: ROUND_ROBIN +)EOF"; + + auto cluster = makeCluster(yaml); + envoy::config::cluster::v3::Cluster cluster_config = parseClusterFromV3Yaml(yaml); + + EXPECT_FALSE(cluster_config.has_connect_timeout()); + EXPECT_EQ(std::chrono::seconds(5), cluster->info()->connectTimeout()); +} + TEST_F(ClusterInfoImplTest, Timeouts) { const std::string yaml = R"EOF( name: name From 2b9fb47ccc8740929de473c553690de295d97aee Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Thu, 20 May 2021 11:11:39 -0400 Subject: [PATCH 15/21] test: fix merge brekage (#16597) Signed-off-by: Alyssa Wilk --- .../client_connection_factory_impl_test.cc | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/test/common/quic/client_connection_factory_impl_test.cc b/test/common/quic/client_connection_factory_impl_test.cc index c919aa9165b0..87e6111cc870 100644 --- a/test/common/quic/client_connection_factory_impl_test.cc +++ b/test/common/quic/client_connection_factory_impl_test.cc @@ -10,11 +10,23 @@ #include "test/mocks/upstream/host.h" #include "test/test_common/simulated_time_system.h" +using testing::Return; + namespace Envoy { namespace Quic { class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public testing::Test { protected: + void initialize() { + Ssl::ClientContextSharedPtr context{new Ssl::MockClientContext()}; + EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _, _)) + .WillOnce(Return(context)); + factory_ = std::make_unique( + std::unique_ptr( + new NiceMock), + context_); + } + NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; Upstream::HostSharedPtr host_{new NiceMock}; @@ -23,13 +35,13 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public t Network::Address::InstanceConstSharedPtr test_address_ = Network::Utility::resolveUrl("tcp://127.0.0.1:3000"); NiceMock context_; - Quic::QuicClientTransportSocketFactory factory_{ - std::unique_ptr(new NiceMock), - context_}; + std::unique_ptr factory_; }; TEST_F(QuicNetworkConnectionTest, BufferLimits) { - PersistentQuicInfoImpl info{dispatcher_, factory_, simTime(), test_address_}; + initialize(); + + PersistentQuicInfoImpl info{dispatcher_, *factory_, simTime(), test_address_}; std::unique_ptr client_connection = createQuicNetworkConnection(info, dispatcher_, test_address_, test_address_); From 2174fd0e05ebb8979ca0d5b44fe334f9e30f81dd Mon Sep 17 00:00:00 2001 From: OutOfControl Date: Fri, 21 May 2021 01:24:34 +0800 Subject: [PATCH 16/21] add a helper class for runtime-derived uint32 (#16398) Adding runtime helper protobuf messages for RuntimeUInt32. TODO(WeavingGao): use for Risk Level: Low Testing: Unit Test Docs Changes: N/A Release Notes: N/A part of #16392 Signed-off-by: gaoweiwen --- source/common/runtime/runtime_protos.h | 28 ++++++++++++++++++++++ test/common/runtime/runtime_protos_test.cc | 21 ++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/source/common/runtime/runtime_protos.h b/source/common/runtime/runtime_protos.h index aa5c7f219438..b9d7d2550e9f 100644 --- a/source/common/runtime/runtime_protos.h +++ b/source/common/runtime/runtime_protos.h @@ -11,6 +11,34 @@ namespace Envoy { namespace Runtime { +// TODO(WeavingGao): use for #16392 +// Helper class for runtime-derived uint32. +class UInt32 : Logger::Loggable { +public: + UInt32(const envoy::config::core::v3::RuntimeUInt32& uint32_proto, Runtime::Loader& runtime) + : runtime_key_(uint32_proto.runtime_key()), default_value_(uint32_proto.default_value()), + runtime_(runtime) {} + + const std::string& runtimeKey() const { return runtime_key_; } + + uint32_t value() const { + uint64_t raw_value = runtime_.snapshot().getInteger(runtime_key_, default_value_); + if (raw_value > std::numeric_limits::max()) { + ENVOY_LOG_EVERY_POW_2( + warn, + "parsed runtime value:{} of {} is larger than uint32 max, returning default instead", + raw_value, runtime_key_); + return default_value_; + } + return static_cast(raw_value); + } + +private: + const std::string runtime_key_; + const uint32_t default_value_; + Runtime::Loader& runtime_; +}; + // Helper class for runtime-derived boolean feature flags. class FeatureFlag { public: diff --git a/test/common/runtime/runtime_protos_test.cc b/test/common/runtime/runtime_protos_test.cc index 97eb4b89ec1e..95cdf87dee84 100644 --- a/test/common/runtime/runtime_protos_test.cc +++ b/test/common/runtime/runtime_protos_test.cc @@ -24,6 +24,27 @@ class RuntimeProtosTest : public testing::Test { NiceMock runtime_; }; +TEST_F(RuntimeProtosTest, UInt32Test) { + envoy::config::core::v3::RuntimeUInt32 uint32_proto; + std::string yaml(R"EOF( +runtime_key: "foo.bar" +default_value: 99 +)EOF"); + TestUtility::loadFromYamlAndValidate(yaml, uint32_proto); + UInt32 test_uint32(uint32_proto, runtime_); + + EXPECT_EQ("foo.bar", test_uint32.runtimeKey()); + + EXPECT_CALL(runtime_.snapshot_, getInteger("foo.bar", 99)); + EXPECT_EQ(99, test_uint32.value()); + + EXPECT_CALL(runtime_.snapshot_, getInteger("foo.bar", 99)).WillOnce(Return(1024)); + EXPECT_EQ(1024, test_uint32.value()); + + EXPECT_CALL(runtime_.snapshot_, getInteger("foo.bar", 99)).WillOnce(Return(1ull << 33)); + EXPECT_EQ(99, test_uint32.value()); +} + TEST_F(RuntimeProtosTest, PercentBasicTest) { envoy::config::core::v3::RuntimePercent percent_proto; std::string yaml(R"EOF( From aee42fddc1176ccf85f555711e98ec0d826109df Mon Sep 17 00:00:00 2001 From: antonio Date: Thu, 20 May 2021 13:14:12 -0700 Subject: [PATCH 17/21] event: Remove obsolete runtime guard for 'envoy.reloadable_features.activate_timers_next_event_loop' (#16221) Commit Message: event: Remove obsolete runtime guard for 'envoy.reloadable_features.activate_timers_next_event_loop' Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: added Platform Specific Features: n/a Fixes #16003 Signed-off-by: Antonio Vicente --- docs/root/version_history/current.rst | 1 + source/common/event/BUILD | 1 - source/common/event/libevent_scheduler.h | 2 - source/common/event/timer_impl.cc | 18 +-- source/common/event/timer_impl.h | 5 - source/common/runtime/runtime_features.cc | 1 - test/common/event/dispatcher_impl_test.cc | 142 ++++++------------ test/test_common/simulated_time_system.cc | 25 +-- .../test_common/simulated_time_system_test.cc | 132 ++++++---------- 9 files changed, 101 insertions(+), 226 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 332df9444950..449d76c1bd2c 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -50,6 +50,7 @@ Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` +* event: removed ``envoy.reloadable_features.activate_timers_next_event_loop`` runtime guard and legacy code path. * http: removed ``envoy.reloadable_features.allow_500_after_100`` runtime guard and the legacy code path. * http: removed ``envoy.reloadable_features.always_apply_route_header_rules`` runtime guard and legacy code path. * http: removed ``envoy.reloadable_features.hcm_stream_error_on_invalid_message`` for disabling closing HTTP/1.1 connections on error. Connection-closing can still be disabled by setting the HTTP/1 configuration :ref:`override_stream_error_on_invalid_http_message `. diff --git a/source/common/event/BUILD b/source/common/event/BUILD index 333755eeac35..35e5bb6601b1 100644 --- a/source/common/event/BUILD +++ b/source/common/event/BUILD @@ -186,7 +186,6 @@ envoy_cc_library( "//include/envoy/event:timer_interface", "//source/common/common:scope_tracker", "//source/common/common:utility_lib", - "//source/common/runtime:runtime_features_lib", ], ) diff --git a/source/common/event/libevent_scheduler.h b/source/common/event/libevent_scheduler.h index d9aeecd387ad..063f3061a5ce 100644 --- a/source/common/event/libevent_scheduler.h +++ b/source/common/event/libevent_scheduler.h @@ -43,8 +43,6 @@ namespace Event { // The same mechanism implements both of these operations, so they are invoked as a group. // - Event::SchedulableCallback::scheduleCallbackCurrentIteration(). Each of these callbacks is // scheduled and invoked independently. -// - Event::Timer::enableTimer(0) if "envoy.reloadable_features.activate_timers_next_event_loop" -// runtime feature is disabled. // // Event::FileEvent::activate and Event::SchedulableCallback::scheduleCallbackNextIteration are // implemented as libevent timers with a deadline of 0. Both of these actions are moved to the work diff --git a/source/common/event/timer_impl.cc b/source/common/event/timer_impl.cc index 41a63daf98d3..3a2189f95c40 100644 --- a/source/common/event/timer_impl.cc +++ b/source/common/event/timer_impl.cc @@ -3,7 +3,6 @@ #include #include "common/common/assert.h" -#include "common/runtime/runtime_features.h" #include "event2/event.h" @@ -11,16 +10,7 @@ namespace Envoy { namespace Event { TimerImpl::TimerImpl(Libevent::BasePtr& libevent, TimerCb cb, Dispatcher& dispatcher) - : cb_(cb), dispatcher_(dispatcher), - activate_timers_next_event_loop_( - // Only read the runtime feature if the runtime loader singleton has already been created. - // Accessing runtime features too early in the initialization sequence triggers logging - // and the logging code itself depends on the use of timers. Attempts to log while - // initializing the logging subsystem will result in a crash. - Runtime::LoaderSingleton::getExisting() - ? Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop") - : true) { + : cb_(cb), dispatcher_(dispatcher) { ASSERT(cb_); evtimer_assign( &raw_event_, libevent.get(), @@ -59,11 +49,7 @@ void TimerImpl::internalEnableTimer(const timeval& tv, const ScopeTrackedObject* ASSERT(dispatcher_.isThreadSafe()); object_ = object; - if (!activate_timers_next_event_loop_ && tv.tv_sec == 0 && tv.tv_usec == 0) { - event_active(&raw_event_, EV_TIMEOUT, 0); - } else { - event_add(&raw_event_, &tv); - } + event_add(&raw_event_, &tv); } bool TimerImpl::enabled() { diff --git a/source/common/event/timer_impl.h b/source/common/event/timer_impl.h index f7513dda996c..bd56d528409e 100644 --- a/source/common/event/timer_impl.h +++ b/source/common/event/timer_impl.h @@ -70,11 +70,6 @@ class TimerImpl : public Timer, ImplBase { // example if the DispatcherImpl::post is called by two threads, they race to // both set this to null. std::atomic object_{}; - - // Latched "envoy.reloadable_features.activate_timers_next_event_loop" runtime feature. If true, - // timers scheduled with a 0 time delta are evaluated in the next iteration of the event loop - // after polling and activating new fd events. - const bool activate_timers_next_event_loop_; }; } // namespace Event diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index c44cc2a55ad3..0f8995fdb3f1 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -56,7 +56,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.test_feature_true", // Begin alphabetically sorted section. "envoy.deprecated_features.allow_deprecated_extension_names", - "envoy.reloadable_features.activate_timers_next_event_loop", "envoy.reloadable_features.add_and_validate_scheme_header", "envoy.reloadable_features.allow_preconnect", "envoy.reloadable_features.allow_response_for_timeout", diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index 4194c46b6b67..fab86c343f1b 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -828,20 +828,15 @@ TEST_F(DispatcherMonotonicTimeTest, ApproximateMonotonicTime) { dispatcher_->run(Dispatcher::RunType::Block); } -class TimerImplTest : public testing::TestWithParam { +class TimerImplTest : public testing::Test { protected: TimerImplTest() { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.activate_timers_next_event_loop", - activateTimersNextEventLoop() ? "true" : "false"}}); // Hook into event loop prepare and check events. evwatch_prepare_new(&libevent_base_, onWatcherReady, &prepare_watcher_); evwatch_check_new(&libevent_base_, onCheck, this); } ~TimerImplTest() override { ASSERT(check_callbacks_.empty()); } - bool activateTimersNextEventLoop() { return GetParam(); } - // Run a callback inside the event loop. The libevent monotonic time used for timer registration // is frozen while within this callback, so timers enabled within this callback end up with the // requested relative registration times. The callback can invoke advanceLibeventTime() to force @@ -918,9 +913,7 @@ class TimerImplTest : public testing::TestWithParam { bool in_event_loop_{}; }; -INSTANTIATE_TEST_SUITE_P(DelayActivation, TimerImplTest, testing::Bool()); - -TEST_P(TimerImplTest, TimerEnabledDisabled) { +TEST_F(TimerImplTest, TimerEnabledDisabled) { InSequence s; Event::TimerPtr timer = dispatcher_->createTimer([] {}); @@ -937,7 +930,7 @@ TEST_P(TimerImplTest, TimerEnabledDisabled) { EXPECT_FALSE(timer->enabled()); } -TEST_P(TimerImplTest, ChangeTimerBackwardsBeforeRun) { +TEST_F(TimerImplTest, ChangeTimerBackwardsBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -965,26 +958,18 @@ TEST_P(TimerImplTest, ChangeTimerBackwardsBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeTimerForwardsToZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeTimerForwardsToZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); ReadyWatcher watcher2; Event::TimerPtr timer2 = dispatcher_->createTimer([&] { watcher2.ready(); }); - if (activateTimersNextEventLoop()) { - // Expect watcher1 to trigger first because timer1's deadline was moved forward. - InSequence s; - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher2, ready()); - } else { - // Timers execute in the wrong order. - InSequence s; - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher1, ready()); - } + // Expect watcher1 to trigger first because timer1's deadline was moved forward. + InSequence s; + EXPECT_CALL(prepare_watcher_, ready()); + EXPECT_CALL(watcher1, ready()); + EXPECT_CALL(watcher2, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(2)); timer2->enableTimer(std::chrono::milliseconds(1)); @@ -995,7 +980,7 @@ TEST_P(TimerImplTest, ChangeTimerForwardsToZeroBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeTimerForwardsToNonZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeTimerForwardsToNonZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1017,7 +1002,7 @@ TEST_P(TimerImplTest, ChangeTimerForwardsToNonZeroBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeLargeTimerForwardToZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeLargeTimerForwardToZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1036,7 +1021,7 @@ TEST_P(TimerImplTest, ChangeLargeTimerForwardToZeroBeforeRun) { }); } -TEST_P(TimerImplTest, ChangeLargeTimerForwardToNonZeroBeforeRun) { +TEST_F(TimerImplTest, ChangeLargeTimerForwardToNonZeroBeforeRun) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1059,7 +1044,7 @@ TEST_P(TimerImplTest, ChangeLargeTimerForwardToNonZeroBeforeRun) { } // Timers scheduled at different times execute in order. -TEST_P(TimerImplTest, TimerOrdering) { +TEST_F(TimerImplTest, TimerOrdering) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1071,17 +1056,10 @@ TEST_P(TimerImplTest, TimerOrdering) { // Expect watcher calls to happen in order since timers have different times. InSequence s; - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } else { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); + EXPECT_CALL(watcher1, ready()); + EXPECT_CALL(watcher2, ready()); + EXPECT_CALL(watcher3, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(0)); @@ -1098,7 +1076,7 @@ TEST_P(TimerImplTest, TimerOrdering) { } // Alarms that are scheduled to execute and are cancelled do not trigger. -TEST_P(TimerImplTest, TimerOrderAndDisableAlarm) { +TEST_F(TimerImplTest, TimerOrderAndDisableAlarm) { ReadyWatcher watcher3; Event::TimerPtr timer3 = dispatcher_->createTimer([&] { watcher3.ready(); }); @@ -1132,7 +1110,7 @@ TEST_P(TimerImplTest, TimerOrderAndDisableAlarm) { // Change the registration time for a timer that is already activated by disabling and re-enabling // the timer. Verify that execution is delayed. -TEST_P(TimerImplTest, TimerOrderDisableAndReschedule) { +TEST_F(TimerImplTest, TimerOrderDisableAndReschedule) { ReadyWatcher watcher4; Event::TimerPtr timer4 = dispatcher_->createTimer([&] { watcher4.ready(); }); @@ -1154,29 +1132,17 @@ TEST_P(TimerImplTest, TimerOrderDisableAndReschedule) { // timer1 is expected to run first and reschedule timers 2 and 3. timer4 should fire before // timer2 and timer3 since timer4's registration is unaffected. InSequence s; - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher4, ready()); - // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure - // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two - // timers could execute in different loop iterations. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } else { - EXPECT_CALL(prepare_watcher_, ready()); - EXPECT_CALL(watcher1, ready()); - EXPECT_CALL(watcher4, ready()); - EXPECT_CALL(watcher2, ready()); - // Sleep in prepare cb to avoid flakiness if epoll_wait returns before the timer timeout. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher3, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); + EXPECT_CALL(watcher1, ready()); + EXPECT_CALL(watcher4, ready()); + // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure + // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two + // timers could execute in different loop iterations. + EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { + advanceLibeventTimeNextIteration(absl::Milliseconds(10)); + })); + EXPECT_CALL(watcher2, ready()); + EXPECT_CALL(watcher3, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(0)); timer2->enableTimer(std::chrono::milliseconds(1)); @@ -1195,7 +1161,7 @@ TEST_P(TimerImplTest, TimerOrderDisableAndReschedule) { // Change the registration time for a timer that is already activated by re-enabling the timer // without calling disableTimer first. -TEST_P(TimerImplTest, TimerOrderAndReschedule) { +TEST_F(TimerImplTest, TimerOrderAndReschedule) { ReadyWatcher watcher4; Event::TimerPtr timer4 = dispatcher_->createTimer([&] { watcher4.ready(); }); @@ -1218,25 +1184,15 @@ TEST_P(TimerImplTest, TimerOrderAndReschedule) { InSequence s; EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher1, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(watcher4, ready()); - // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure - // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two - // timers could execute in different loop iterations. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher3, ready()); - } else { - EXPECT_CALL(watcher2, ready()); - EXPECT_CALL(watcher4, ready()); - // Sleep in prepare cb to avoid flakiness if epoll_wait returns before the timer timeout. - EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { - advanceLibeventTimeNextIteration(absl::Milliseconds(10)); - })); - EXPECT_CALL(watcher3, ready()); - } + EXPECT_CALL(watcher4, ready()); + // Sleep during prepare to ensure that enough time has elapsed before timer evaluation to ensure + // that timers 2 and 3 are picked up by the same loop iteration. Without the sleep the two + // timers could execute in different loop iterations. + EXPECT_CALL(prepare_watcher_, ready()).WillOnce(testing::InvokeWithoutArgs([&]() { + advanceLibeventTimeNextIteration(absl::Milliseconds(10)); + })); + EXPECT_CALL(watcher2, ready()); + EXPECT_CALL(watcher3, ready()); runInEventLoop([&]() { timer1->enableTimer(std::chrono::milliseconds(0)); timer2->enableTimer(std::chrono::milliseconds(1)); @@ -1253,7 +1209,7 @@ TEST_P(TimerImplTest, TimerOrderAndReschedule) { }); } -TEST_P(TimerImplTest, TimerChaining) { +TEST_F(TimerImplTest, TimerChaining) { ReadyWatcher watcher1; Event::TimerPtr timer1 = dispatcher_->createTimer([&] { watcher1.ready(); }); @@ -1284,17 +1240,11 @@ TEST_P(TimerImplTest, TimerChaining) { InSequence s; EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher4, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher3, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher2, ready()); - if (activateTimersNextEventLoop()) { - EXPECT_CALL(prepare_watcher_, ready()); - } + EXPECT_CALL(prepare_watcher_, ready()); EXPECT_CALL(watcher1, ready()); dispatcher_->run(Dispatcher::RunType::NonBlock); @@ -1304,7 +1254,7 @@ TEST_P(TimerImplTest, TimerChaining) { EXPECT_FALSE(timer4->enabled()); } -TEST_P(TimerImplTest, TimerChainDisable) { +TEST_F(TimerImplTest, TimerChainDisable) { ReadyWatcher watcher; Event::TimerPtr timer1; Event::TimerPtr timer2; @@ -1335,7 +1285,7 @@ TEST_P(TimerImplTest, TimerChainDisable) { dispatcher_->run(Dispatcher::RunType::NonBlock); } -TEST_P(TimerImplTest, TimerChainDelete) { +TEST_F(TimerImplTest, TimerChainDelete) { ReadyWatcher watcher; Event::TimerPtr timer1; Event::TimerPtr timer2; diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index af27468a6a79..b01f13f6e0b3 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -107,12 +107,7 @@ class SimulatedTimeSystemHelper::SimulatedScheduler : public Scheduler { } if (!run_alarms_cb_->enabled()) { - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop")) { - run_alarms_cb_->scheduleCallbackNextIteration(); - } else { - run_alarms_cb_->scheduleCallbackCurrentIteration(); - } + run_alarms_cb_->scheduleCallbackNextIteration(); } } @@ -213,14 +208,12 @@ class SimulatedTimeSystemHelper::SimulatedScheduler : public Scheduler { // True if the SimulatedTimeSystemHelper is waiting for the scheduler to process expired alarms // and call decPending after an update to monotonic time. bool pending_dec_ ABSL_GUARDED_BY(mutex_) = false; - // Used to randomize the ordering of alarms scheduled for the same time when the runtime feature - // envoy.reloadable_features.activate_timers_next_event_loop is enabled. This mimics the trigger + // Used to randomize the ordering of alarms scheduled for the same time. This mimics the trigger // order of real timers scheduled for the same absolute time is non-deterministic. // Each simulated scheduler has it's own TestRandomGenerator with the same seed to improve test // failure reproducibility when running against a specific seed by minimizing cross scheduler // interactions. TestRandomGenerator random_source_ ABSL_GUARDED_BY(mutex_); - uint64_t legacy_next_idx_ ABSL_GUARDED_BY(mutex_) = 0; }; // Our simulated alarm inherits from TimerImpl so that the same dispatching @@ -269,24 +262,14 @@ void SimulatedTimeSystemHelper::SimulatedScheduler::enableAlarm( absl::MutexLock lock(&mutex_); if (duration.count() == 0 && triggered_alarms_.contains(alarm)) { return; - } else if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop")) { - disableAlarmLockHeld(alarm); - registered_alarms_.add({monotonic_time_ + duration, random_source_.random(), alarm}); } else { disableAlarmLockHeld(alarm); - AlarmSet& alarm_set = (duration.count() != 0) ? registered_alarms_ : triggered_alarms_; - alarm_set.add({monotonic_time_ + duration, ++legacy_next_idx_, alarm}); + registered_alarms_.add({monotonic_time_ + duration, random_source_.random(), alarm}); } } if (duration.count() == 0) { - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.activate_timers_next_event_loop")) { - run_alarms_cb_->scheduleCallbackNextIteration(); - } else { - run_alarms_cb_->scheduleCallbackCurrentIteration(); - } + run_alarms_cb_->scheduleCallbackNextIteration(); } } diff --git a/test/test_common/simulated_time_system_test.cc b/test/test_common/simulated_time_system_test.cc index e46284925deb..f518a1a24985 100644 --- a/test/test_common/simulated_time_system_test.cc +++ b/test/test_common/simulated_time_system_test.cc @@ -17,20 +17,12 @@ namespace Event { namespace Test { namespace { -enum class ActivateMode { DelayActivateTimers, EagerlyActivateTimers }; - -class SimulatedTimeSystemTest : public testing::TestWithParam { +class SimulatedTimeSystemTest : public testing::Test { protected: SimulatedTimeSystemTest() : scheduler_(time_system_.createScheduler(base_scheduler_, base_scheduler_)), start_monotonic_time_(time_system_.monotonicTime()), - start_system_time_(time_system_.systemTime()) { - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.activate_timers_next_event_loop", - activateMode() == ActivateMode::DelayActivateTimers ? "true" : "false"}}); - } - - ActivateMode activateMode() { return GetParam(); } + start_system_time_(time_system_.systemTime()) {} void trackPrepareCalls() { base_scheduler_.registerOnPrepareCallback([this]() { output_.append(1, 'p'); }); @@ -78,11 +70,7 @@ class SimulatedTimeSystemTest : public testing::TestWithParam { SystemTime start_system_time_; }; -INSTANTIATE_TEST_SUITE_P(DelayTimerActivation, SimulatedTimeSystemTest, - testing::Values(ActivateMode::DelayActivateTimers, - ActivateMode::EagerlyActivateTimers)); - -TEST_P(SimulatedTimeSystemTest, AdvanceTimeAsync) { +TEST_F(SimulatedTimeSystemTest, AdvanceTimeAsync) { EXPECT_EQ(start_monotonic_time_, time_system_.monotonicTime()); EXPECT_EQ(start_system_time_, time_system_.systemTime()); advanceMsAndLoop(5); @@ -90,7 +78,7 @@ TEST_P(SimulatedTimeSystemTest, AdvanceTimeAsync) { EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(5), time_system_.systemTime()); } -TEST_P(SimulatedTimeSystemTest, TimerTotalOrdering) { +TEST_F(SimulatedTimeSystemTest, TimerTotalOrdering) { trackPrepareCalls(); addTask(0, '0'); @@ -104,7 +92,7 @@ TEST_P(SimulatedTimeSystemTest, TimerTotalOrdering) { EXPECT_EQ("p012", output_); } -TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering) { +TEST_F(SimulatedTimeSystemTest, TimerPartialOrdering) { trackPrepareCalls(); std::set outputs; @@ -124,16 +112,12 @@ TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering) { timers_.clear(); } - if (activateMode() == ActivateMode::DelayActivateTimers) { - // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled - // for the same time. Verify that both orderings were observed. - EXPECT_THAT(outputs, testing::ElementsAre("p0123", "p0213")); - } else { - EXPECT_THAT(outputs, testing::ElementsAre("p0123")); - } + // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled + // for the same time. Verify that both orderings were observed. + EXPECT_THAT(outputs, testing::ElementsAre("p0123", "p0213")); } -TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering2) { +TEST_F(SimulatedTimeSystemTest, TimerPartialOrdering2) { trackPrepareCalls(); std::set outputs; @@ -154,17 +138,13 @@ TEST_P(SimulatedTimeSystemTest, TimerPartialOrdering2) { timers_.clear(); } - if (activateMode() == ActivateMode::DelayActivateTimers) { - // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled - // for the same time. Verify that both orderings were observed. - EXPECT_THAT(outputs, testing::ElementsAre("p0p123", "p0p213")); - } else { - EXPECT_THAT(outputs, testing::ElementsAre("p0p123")); - } + // Execution order of timers 1 and 2 is non-deterministic because the two timers were scheduled + // for the same time. Verify that both orderings were observed. + EXPECT_THAT(outputs, testing::ElementsAre("p0p123", "p0p213")); } // Timers that are scheduled to execute and but are disabled first do not trigger. -TEST_P(SimulatedTimeSystemTest, TimerOrderAndDisableTimer) { +TEST_F(SimulatedTimeSystemTest, TimerOrderAndDisableTimer) { trackPrepareCalls(); // Create 3 timers. The first timer should disable the second, so it doesn't trigger. @@ -181,7 +161,7 @@ TEST_P(SimulatedTimeSystemTest, TimerOrderAndDisableTimer) { } // Capture behavior of timers which are rescheduled without being disabled first. -TEST_P(SimulatedTimeSystemTest, TimerOrderAndRescheduleTimer) { +TEST_F(SimulatedTimeSystemTest, TimerOrderAndRescheduleTimer) { trackPrepareCalls(); // Reschedule timers 1, 2 and 4 without disabling first. @@ -201,34 +181,26 @@ TEST_P(SimulatedTimeSystemTest, TimerOrderAndRescheduleTimer) { // Timer 4 runs as part of the first wakeup since its new schedule time has a delta of 0. Timer 2 // is delayed since it is rescheduled with a non-zero delta. advanceMsAndLoop(5); - if (activateMode() == ActivateMode::DelayActivateTimers) { - if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { - // Force it to run again to pick up next iteration callbacks. - // The event loop runs for a single iteration in NonBlock mode on Windows as a hack to work - // around LEVEL trigger fd registrations constantly firing events and preventing the NonBlock - // event loop from ever reaching the no-fd event and no-expired timers termination condition. - // It is not possible to get consistent event loop behavior since the time system does not - // override the base scheduler's run behavior, and libevent does not provide a mode where it - // runs at most N iterations before breaking out of the loop for us to prefer over the single - // iteration mode used on Windows. - advanceMsAndLoop(0); - } - EXPECT_EQ("p013p4", output_); - } else { - EXPECT_EQ("p0134", output_); + if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { + // Force it to run again to pick up next iteration callbacks. + // The event loop runs for a single iteration in NonBlock mode on Windows as a hack to work + // around LEVEL trigger fd registrations constantly firing events and preventing the NonBlock + // event loop from ever reaching the no-fd event and no-expired timers termination condition. + // It is not possible to get consistent event loop behavior since the time system does not + // override the base scheduler's run behavior, and libevent does not provide a mode where it + // runs at most N iterations before breaking out of the loop for us to prefer over the single + // iteration mode used on Windows. + advanceMsAndLoop(0); } + EXPECT_EQ("p013p4", output_); advanceMsAndLoop(100); - if (activateMode() == ActivateMode::DelayActivateTimers) { - EXPECT_EQ("p013p4p2", output_); - } else { - EXPECT_EQ("p0134p2", output_); - } + EXPECT_EQ("p013p4p2", output_); } // Disable and re-enable timers that is already pending execution and verify that execution is // delayed. -TEST_P(SimulatedTimeSystemTest, TimerOrderDisableAndRescheduleTimer) { +TEST_F(SimulatedTimeSystemTest, TimerOrderDisableAndRescheduleTimer) { trackPrepareCalls(); // Disable and reschedule timers 1, 2 and 4 when timer 0 triggers. @@ -251,26 +223,18 @@ TEST_P(SimulatedTimeSystemTest, TimerOrderDisableAndRescheduleTimer) { // because it is scheduled with zero delay. Timer 2 executes in a later iteration because it is // re-enabled with a non-zero timeout. advanceMsAndLoop(5); - if (activateMode() == ActivateMode::DelayActivateTimers) { - if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { - // The event loop runs for a single iteration in NonBlock mode on Windows. Force it to run - // again to pick up next iteration callbacks. - advanceMsAndLoop(0); - } - EXPECT_THAT(output_, testing::AnyOf("p03p14", "p03p41")); - } else { - EXPECT_EQ("p0314", output_); + if constexpr (Event::PlatformDefaultTriggerType == FileTriggerType::Level) { + // The event loop runs for a single iteration in NonBlock mode on Windows. Force it to run + // again to pick up next iteration callbacks. + advanceMsAndLoop(0); } + EXPECT_THAT(output_, testing::AnyOf("p03p14", "p03p41")); advanceMsAndLoop(100); - if (activateMode() == ActivateMode::DelayActivateTimers) { - EXPECT_THAT(output_, testing::AnyOf("p03p14p2", "p03p41p2")); - } else { - EXPECT_EQ("p0314p2", output_); - } + EXPECT_THAT(output_, testing::AnyOf("p03p14p2", "p03p41p2")); } -TEST_P(SimulatedTimeSystemTest, AdvanceTimeWait) { +TEST_F(SimulatedTimeSystemTest, AdvanceTimeWait) { EXPECT_EQ(start_monotonic_time_, time_system_.monotonicTime()); EXPECT_EQ(start_system_time_, time_system_.systemTime()); @@ -292,7 +256,7 @@ TEST_P(SimulatedTimeSystemTest, AdvanceTimeWait) { EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(5), time_system_.systemTime()); } -TEST_P(SimulatedTimeSystemTest, WaitFor) { +TEST_F(SimulatedTimeSystemTest, WaitFor) { EXPECT_EQ(start_monotonic_time_, time_system_.monotonicTime()); EXPECT_EQ(start_system_time_, time_system_.systemTime()); @@ -350,7 +314,7 @@ TEST_P(SimulatedTimeSystemTest, WaitFor) { EXPECT_EQ(MonotonicTime(std::chrono::seconds(60)), time_system_.monotonicTime()); } -TEST_P(SimulatedTimeSystemTest, Monotonic) { +TEST_F(SimulatedTimeSystemTest, Monotonic) { // Setting time forward works. time_system_.setMonotonicTime(start_monotonic_time_ + std::chrono::milliseconds(5)); EXPECT_EQ(start_monotonic_time_ + std::chrono::milliseconds(5), time_system_.monotonicTime()); @@ -360,7 +324,7 @@ TEST_P(SimulatedTimeSystemTest, Monotonic) { EXPECT_EQ(start_monotonic_time_ + std::chrono::milliseconds(5), time_system_.monotonicTime()); } -TEST_P(SimulatedTimeSystemTest, System) { +TEST_F(SimulatedTimeSystemTest, System) { // Setting time forward works. time_system_.setSystemTime(start_system_time_ + std::chrono::milliseconds(5)); EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(5), time_system_.systemTime()); @@ -370,7 +334,7 @@ TEST_P(SimulatedTimeSystemTest, System) { EXPECT_EQ(start_system_time_ + std::chrono::milliseconds(3), time_system_.systemTime()); } -TEST_P(SimulatedTimeSystemTest, Ordering) { +TEST_F(SimulatedTimeSystemTest, Ordering) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -381,7 +345,7 @@ TEST_P(SimulatedTimeSystemTest, Ordering) { EXPECT_EQ("356", output_); } -TEST_P(SimulatedTimeSystemTest, SystemTimeOrdering) { +TEST_F(SimulatedTimeSystemTest, SystemTimeOrdering) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -395,7 +359,7 @@ TEST_P(SimulatedTimeSystemTest, SystemTimeOrdering) { EXPECT_EQ("356", output_); // callbacks don't get replayed. } -TEST_P(SimulatedTimeSystemTest, DisableTimer) { +TEST_F(SimulatedTimeSystemTest, DisableTimer) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -407,7 +371,7 @@ TEST_P(SimulatedTimeSystemTest, DisableTimer) { EXPECT_EQ("36", output_); } -TEST_P(SimulatedTimeSystemTest, IgnoreRedundantDisable) { +TEST_F(SimulatedTimeSystemTest, IgnoreRedundantDisable) { addTask(5, '5'); timers_[0]->disableTimer(); timers_[0]->disableTimer(); @@ -415,7 +379,7 @@ TEST_P(SimulatedTimeSystemTest, IgnoreRedundantDisable) { EXPECT_EQ("", output_); } -TEST_P(SimulatedTimeSystemTest, OverrideEnable) { +TEST_F(SimulatedTimeSystemTest, OverrideEnable) { addTask(5, '5'); timers_[0]->enableTimer(std::chrono::milliseconds(6)); advanceMsAndLoop(5); @@ -424,7 +388,7 @@ TEST_P(SimulatedTimeSystemTest, OverrideEnable) { EXPECT_EQ("5", output_); } -TEST_P(SimulatedTimeSystemTest, DeleteTime) { +TEST_F(SimulatedTimeSystemTest, DeleteTime) { addTask(5, '5'); addTask(3, '3'); addTask(6, '6'); @@ -437,7 +401,7 @@ TEST_P(SimulatedTimeSystemTest, DeleteTime) { } // Regression test for issues documented in https://github.com/envoyproxy/envoy/pull/6956 -TEST_P(SimulatedTimeSystemTest, DuplicateTimer) { +TEST_F(SimulatedTimeSystemTest, DuplicateTimer) { // Set one alarm two times to test that pending does not get duplicated.. std::chrono::milliseconds delay(0); TimerPtr zero_timer = scheduler_->createTimer([this]() { output_.append(1, '2'); }, dispatcher_); @@ -448,7 +412,7 @@ TEST_P(SimulatedTimeSystemTest, DuplicateTimer) { } // Regression test for issues documented in https://github.com/envoyproxy/envoy/pull/6956 -TEST_P(SimulatedTimeSystemTest, DuplicateTimer2) { +TEST_F(SimulatedTimeSystemTest, DuplicateTimer2) { // Now set an alarm which requires 10s of progress and make sure advanceTimeWait and waitFor // works. absl::Mutex mutex; @@ -490,13 +454,13 @@ TEST_P(SimulatedTimeSystemTest, DuplicateTimer2) { thread->join(); } -TEST_P(SimulatedTimeSystemTest, Enabled) { +TEST_F(SimulatedTimeSystemTest, Enabled) { TimerPtr timer = scheduler_->createTimer({}, dispatcher_); timer->enableTimer(std::chrono::milliseconds(0)); EXPECT_TRUE(timer->enabled()); } -TEST_P(SimulatedTimeSystemTest, DeleteTimerFromThread) { +TEST_F(SimulatedTimeSystemTest, DeleteTimerFromThread) { TimerPtr timer = scheduler_->createTimer([]() {}, dispatcher_); timer->enableTimer(std::chrono::milliseconds(0)); auto thread = Thread::threadFactoryForTest().createThread([&timer]() { timer.reset(); }); @@ -504,7 +468,7 @@ TEST_P(SimulatedTimeSystemTest, DeleteTimerFromThread) { thread->join(); } -TEST_P(SimulatedTimeSystemTest, DeleteTimerFromThread2) { +TEST_F(SimulatedTimeSystemTest, DeleteTimerFromThread2) { TimerPtr timer = scheduler_->createTimer([]() {}, dispatcher_); timer->enableTimer(std::chrono::milliseconds(1)); auto thread = Thread::threadFactoryForTest().createThread([&timer]() { timer.reset(); }); From c63cbab1683dc105c5c543e545ff06cdccf92406 Mon Sep 17 00:00:00 2001 From: Paul Gallagher Date: Thu, 20 May 2021 16:58:06 -0400 Subject: [PATCH 18/21] Update ConfigDump documentation. (#16491) Signed-off-by: Paul Gallagher --- api/envoy/admin/v3/config_dump.proto | 2 ++ api/envoy/admin/v4alpha/config_dump.proto | 2 ++ generated_api_shadow/envoy/admin/v3/config_dump.proto | 2 ++ generated_api_shadow/envoy/admin/v4alpha/config_dump.proto | 2 ++ 4 files changed, 8 insertions(+) diff --git a/api/envoy/admin/v3/config_dump.proto b/api/envoy/admin/v3/config_dump.proto index 21e3e8c32672..ddafb56b3936 100644 --- a/api/envoy/admin/v3/config_dump.proto +++ b/api/envoy/admin/v3/config_dump.proto @@ -57,7 +57,9 @@ message ConfigDump { // * *clusters*: :ref:`ClustersConfigDump ` // * *endpoints*: :ref:`EndpointsConfigDump ` // * *listeners*: :ref:`ListenersConfigDump ` + // * *scoped_routes*: :ref:`ScopedRoutesConfigDump ` // * *routes*: :ref:`RoutesConfigDump ` + // * *secrets*: :ref:`SecretsConfigDump ` // // EDS Configuration will only be dumped by using parameter `?include_eds` // diff --git a/api/envoy/admin/v4alpha/config_dump.proto b/api/envoy/admin/v4alpha/config_dump.proto index 32c7e2d07ab3..2e36bc16f9b6 100644 --- a/api/envoy/admin/v4alpha/config_dump.proto +++ b/api/envoy/admin/v4alpha/config_dump.proto @@ -57,7 +57,9 @@ message ConfigDump { // * *clusters*: :ref:`ClustersConfigDump ` // * *endpoints*: :ref:`EndpointsConfigDump ` // * *listeners*: :ref:`ListenersConfigDump ` + // * *scoped_routes*: :ref:`ScopedRoutesConfigDump ` // * *routes*: :ref:`RoutesConfigDump ` + // * *secrets*: :ref:`SecretsConfigDump ` // // EDS Configuration will only be dumped by using parameter `?include_eds` // diff --git a/generated_api_shadow/envoy/admin/v3/config_dump.proto b/generated_api_shadow/envoy/admin/v3/config_dump.proto index 21e3e8c32672..ddafb56b3936 100644 --- a/generated_api_shadow/envoy/admin/v3/config_dump.proto +++ b/generated_api_shadow/envoy/admin/v3/config_dump.proto @@ -57,7 +57,9 @@ message ConfigDump { // * *clusters*: :ref:`ClustersConfigDump ` // * *endpoints*: :ref:`EndpointsConfigDump ` // * *listeners*: :ref:`ListenersConfigDump ` + // * *scoped_routes*: :ref:`ScopedRoutesConfigDump ` // * *routes*: :ref:`RoutesConfigDump ` + // * *secrets*: :ref:`SecretsConfigDump ` // // EDS Configuration will only be dumped by using parameter `?include_eds` // diff --git a/generated_api_shadow/envoy/admin/v4alpha/config_dump.proto b/generated_api_shadow/envoy/admin/v4alpha/config_dump.proto index 32c7e2d07ab3..2e36bc16f9b6 100644 --- a/generated_api_shadow/envoy/admin/v4alpha/config_dump.proto +++ b/generated_api_shadow/envoy/admin/v4alpha/config_dump.proto @@ -57,7 +57,9 @@ message ConfigDump { // * *clusters*: :ref:`ClustersConfigDump ` // * *endpoints*: :ref:`EndpointsConfigDump ` // * *listeners*: :ref:`ListenersConfigDump ` + // * *scoped_routes*: :ref:`ScopedRoutesConfigDump ` // * *routes*: :ref:`RoutesConfigDump ` + // * *secrets*: :ref:`SecretsConfigDump ` // // EDS Configuration will only be dumped by using parameter `?include_eds` // From 02f316252f79427bf6ea813753b45d5055def945 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 20 May 2021 14:11:10 -0700 Subject: [PATCH 19/21] bazel: add a few flags to --config=clang-msan. (#16603) Signed-off-by: Piotr Sikora --- .bazelrc | 3 +++ bazel/external/wee8.genrule_cmd | 1 + 2 files changed, 4 insertions(+) diff --git a/.bazelrc b/.bazelrc index 95330f8ddbbd..21d0db6bb1a6 100644 --- a/.bazelrc +++ b/.bazelrc @@ -105,9 +105,12 @@ build:clang-msan --config=sanitizer build:clang-msan --define ENVOY_CONFIG_MSAN=1 build:clang-msan --copt -fsanitize=memory build:clang-msan --linkopt -fsanitize=memory +build:clang-msan --linkopt -fuse-ld=lld build:clang-msan --copt -fsanitize-memory-track-origins=2 +build:clang-msan --test_env=MSAN_SYMBOLIZER_PATH # MSAN needs -O1 to get reasonable performance. build:clang-msan --copt -O1 +build:clang-msan --copt -fno-optimize-sibling-calls # Clang with libc++ build:libc++ --config=clang diff --git a/bazel/external/wee8.genrule_cmd b/bazel/external/wee8.genrule_cmd index 11fb9c6ed8d9..c5ff69049a57 100644 --- a/bazel/external/wee8.genrule_cmd +++ b/bazel/external/wee8.genrule_cmd @@ -54,6 +54,7 @@ if [[ $${ENVOY_UBSAN_VPTR-} == "1" ]]; then fi if [[ $${ENVOY_MSAN-} == "1" ]]; then WEE8_BUILD_ARGS+=" is_msan=true" + WEE8_BUILD_ARGS+=" msan_track_origins=2" export LDFLAGS="$${LDFLAGS} -L/opt/libcxx_msan/lib -Wl,-rpath,/opt/libcxx_msan/lib" fi if [[ $${ENVOY_TSAN-} == "1" ]]; then From 25574b48d94f739731eea627c6accd5ba00b9db7 Mon Sep 17 00:00:00 2001 From: James Peach Date: Fri, 21 May 2021 10:01:38 +1000 Subject: [PATCH 20/21] ci: exclude Google Test macros from clang-tidy (#16557) Signed-off-by: James Peach --- .clang-tidy | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.clang-tidy b/.clang-tidy index 693858657d47..02ac33e00da5 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -59,6 +59,10 @@ CheckOptions: - key: readability-identifier-naming.EnumConstantCase value: 'CamelCase' + # Ignore GoogleTest function macros. + - key: readability-identifier-naming.FunctionIgnoredRegexp + value: '(TEST|TEST_F|TEST_P|INSTANTIATE_TEST_SUITE_P|MOCK_METHOD|TYPED_TEST)' + - key: readability-identifier-naming.ParameterCase value: 'lower_case' From 5c28e9548761baa13f2076d3b688d98228e90ec0 Mon Sep 17 00:00:00 2001 From: Yiming Jin Date: Thu, 20 May 2021 18:53:01 -0700 Subject: [PATCH 21/21] Skip metadata processing after sending local reply (#16154) Signed-off-by: Yiming Jin --- source/common/http/filter_manager.cc | 9 ++++- test/integration/BUILD | 1 + test/integration/filters/BUILD | 15 +++++++ .../local_reply_with_metadata_filter.cc | 40 +++++++++++++++++++ test/integration/protocol_integration_test.cc | 18 +++++++++ 5 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 test/integration/filters/local_reply_with_metadata_filter.cc diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 592463ea2aee..982dd80a864c 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -545,6 +545,12 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead (*entry)->handle_->decodeComplete(); } + // Skip processing metadata after sending local reply + if (state_.local_complete_ && std::next(entry) != decoder_filters_.end()) { + maybeContinueDecoding(continue_data_entry); + return; + } + const bool new_metadata_added = processNewlyAddedMetadata(); // If end_stream is set in headers, and a filter adds new metadata, we need to delay end_stream // in headers by inserting an empty data frame with end_stream set. The empty data frame is sent @@ -558,8 +564,7 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead addDecodedData(*((*entry).get()), empty_data, true); } - if ((!continue_iteration || state_.local_complete_) && - std::next(entry) != decoder_filters_.end()) { + if (!continue_iteration && std::next(entry) != decoder_filters_.end()) { // Stop iteration IFF this is not the last filter. If it is the last filter, continue with // processing since we need to handle the case where a terminal filter wants to buffer, but // a previous filter has added body. diff --git a/test/integration/BUILD b/test/integration/BUILD index 3648cea48ffa..41c7e0d44e2e 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -453,6 +453,7 @@ envoy_cc_test_library( "//test/integration/filters:invalid_header_filter_lib", "//test/integration/filters:local_reply_during_encoding_data_filter_lib", "//test/integration/filters:local_reply_during_encoding_filter_lib", + "//test/integration/filters:local_reply_with_metadata_filter_lib", "//test/integration/filters:random_pause_filter_lib", "//test/test_common:logging_lib", "//test/test_common:utility_lib", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index dcc6c91d6782..3c5a4fa79d66 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -69,6 +69,21 @@ envoy_cc_test_library( ], ) +envoy_cc_test_library( + name = "local_reply_with_metadata_filter_lib", + srcs = [ + "local_reply_with_metadata_filter.cc", + ], + deps = [ + ":common_lib", + "//include/envoy/http:filter_interface", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], +) + envoy_cc_test_library( name = "continue_headers_only_inject_body", srcs = [ diff --git a/test/integration/filters/local_reply_with_metadata_filter.cc b/test/integration/filters/local_reply_with_metadata_filter.cc new file mode 100644 index 000000000000..07580be29c6a --- /dev/null +++ b/test/integration/filters/local_reply_with_metadata_filter.cc @@ -0,0 +1,40 @@ +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" +#include "test/integration/filters/common.h" + +#include "gtest/gtest.h" + +namespace Envoy { + +// A filter that calls Http::FilterHeadersStatus::StopAll and set metadata after a local reply. +class LocalReplyWithMetadataFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "local-reply-with-metadata-filter"; + + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override { + Http::MetadataMap metadata_map = {{"headers", "headers"}, {"duplicate", "duplicate"}}; + Http::MetadataMapPtr metadata_map_ptr = std::make_unique(metadata_map); + decoder_callbacks_->addDecodedMetadata().emplace_back(std::move(metadata_map_ptr)); + decoder_callbacks_->sendLocalReply(Envoy::Http::Code::OK, "", nullptr, absl::nullopt, + "LocalReplyWithMetadataFilter is ready"); + return Http::FilterHeadersStatus::StopIteration; + } + + // Adds new metadata to metadata_map directly + Http::FilterMetadataStatus decodeMetadata(Http::MetadataMap& metadata_map) override { + metadata_map["data"] = "data"; + ASSERT(false); + return Http::FilterMetadataStatus::Continue; + } +}; + +constexpr char LocalReplyWithMetadataFilter::name[]; +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; + +} // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index b8ab19fda6c9..3c799a0566a5 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -2314,4 +2314,22 @@ TEST_P(DownstreamProtocolIntegrationTest, HeaderNormalizationRejection) { EXPECT_EQ("400", response->headers().getStatusValue()); } +// Tests a filter that returns a FilterHeadersStatus::Continue after a local reply without +// processing new metadata generated in decodeHeader +TEST_P(DownstreamProtocolIntegrationTest, LocalReplyWithMetadata) { + config_helper_.addFilter(R"EOF( + name: local-reply-with-metadata-filter + typed_config: + "@type": type.googleapis.com/google.protobuf.Empty + )EOF"); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + // Send a headers only request. + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + ASSERT_EQ("200", response->headers().getStatusValue()); +} + } // namespace Envoy