From 96cf219d7564d77f49c781bc9fd887b47a8b5455 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 8 Jul 2021 14:30:10 -0700 Subject: [PATCH 1/5] filter: add ability to resetIdleTimer Signed-off-by: Jose Nino --- envoy/http/filter.h | 5 +++++ source/common/http/async_client_impl.h | 9 ++++----- source/common/http/filter_manager.cc | 4 ++++ source/common/http/filter_manager.h | 1 + test/common/http/filter_manager_test.cc | 17 +++++++++++++++++ test/mocks/http/mocks.h | 2 ++ 6 files changed, 33 insertions(+), 5 deletions(-) diff --git a/envoy/http/filter.h b/envoy/http/filter.h index e677c85e1b7de..6dcf6f2f42f33 100644 --- a/envoy/http/filter.h +++ b/envoy/http/filter.h @@ -296,6 +296,11 @@ class StreamFilterCallbacks { * added to. */ virtual void restoreContextOnContinue(ScopeTrackedObjectStack& tracked_object_stack) PURE; + + /** + * Called when filter activity indicates that the stream idle timeout should be reset. + */ + virtual void resetIdleTimer() PURE; }; /** diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index d496e21bbefad..d7193564ef6d9 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -84,11 +84,6 @@ class AsyncStreamImpl : public AsyncClient::Stream, AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCallbacks& callbacks, const AsyncClient::StreamOptions& options); - // Http::StreamDecoderFilterCallbacks - void requestRouteConfigUpdate(Http::RouteConfigUpdatedCallbackSharedPtr) override { - NOT_IMPLEMENTED_GCOVR_EXCL_LINE; - } - // Http::AsyncClient::Stream void sendHeaders(RequestHeaderMap& headers, bool end_stream) override; void sendData(Buffer::Instance& data, bool end_stream) override; @@ -436,6 +431,10 @@ class AsyncStreamImpl : public AsyncClient::Stream, } void addUpstreamSocketOptions(const Network::Socket::OptionsSharedPtr&) override {} Network::Socket::OptionsSharedPtr getUpstreamSocketOptions() const override { return {}; } + void requestRouteConfigUpdate(Http::RouteConfigUpdatedCallbackSharedPtr) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void resetIdleTimer() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } // ScopeTrackedObject void dumpState(std::ostream& os, int indent_level) const override { diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 8f855eba2495d..7e851195ddc3b 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -268,6 +268,10 @@ void ActiveStreamFilterBase::clearRouteCache() { parent_.filter_manager_callbacks_.clearRouteCache(); } +void ActiveStreamFilterBase::resetIdleTimer() { + parent_.filter_manager_callbacks_.resetIdleTimer(); +} + void FilterMatchState::evaluateMatchTreeWithNewData(MatchDataUpdateFunc update_func) { if (match_tree_evaluated_ || !matching_data_) { return; diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index f8e3e1326e025..6717a85b56046 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -144,6 +144,7 @@ struct ActiveStreamFilterBase : public virtual StreamFilterCallbacks, Tracing::Config& tracingConfig() override; const ScopeTrackedObject& scope() override; void restoreContextOnContinue(ScopeTrackedObjectStack& tracked_object_stack) override; + void resetIdleTimer() override; // Functions to set or get iteration state. bool canIterate() { return iteration_state_ == IterationState::Continue; } diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index 8587b0f50ac85..ed71a44fe5ff4 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -550,6 +550,23 @@ TEST_F(FilterManagerTest, MultipleOnLocalReply) { filter_manager_->destroyFilters(); } +TEST_F(FilterManagerTest, ResetIdleTimer) { + initialize(); + + std::shared_ptr decoder_filter(new NiceMock()); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillRepeatedly(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(decoder_filter); + })); + filter_manager_->createFilterChain(); + + EXPECT_CALL(filter_manager_callbacks_, resetIdleTimer()); + decoder_filter->callbacks_->resetIdleTimer(); + + filter_manager_->destroyFilters(); +} + } // namespace } // namespace Http } // namespace Envoy diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 55e35884ce9ef..21c17e4f7ad9d 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -202,6 +202,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD(const Network::Connection*, connection, ()); MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); MOCK_METHOD(void, resetStream, ()); + MOCK_METHOD(void, resetIdleTimer, ()); MOCK_METHOD(Upstream::ClusterInfoConstSharedPtr, clusterInfo, ()); MOCK_METHOD(Router::RouteConstSharedPtr, route, ()); MOCK_METHOD(Router::RouteConstSharedPtr, route, (const Router::RouteCallback&)); @@ -292,6 +293,7 @@ class MockStreamEncoderFilterCallbacks : public StreamEncoderFilterCallbacks, MOCK_METHOD(const Network::Connection*, connection, ()); MOCK_METHOD(Event::Dispatcher&, dispatcher, ()); MOCK_METHOD(void, resetStream, ()); + MOCK_METHOD(void, resetIdleTimer, ()); MOCK_METHOD(Upstream::ClusterInfoConstSharedPtr, clusterInfo, ()); MOCK_METHOD(void, requestRouteConfigUpdate, (std::function)); MOCK_METHOD(bool, canRequestRouteConfigUpdate, ()); From fe2dec52b8232be00f1bcbddb0dfb5772f01be55 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 8 Jul 2021 16:41:53 -0700 Subject: [PATCH 2/5] integration test Signed-off-by: Jose Nino --- test/integration/BUILD | 1 + test/integration/filters/BUILD | 15 +++++++ .../filters/reset_idle_timer_filter.cc | 39 +++++++++++++++++++ .../idle_timeout_integration_test.cc | 27 +++++++++++++ 4 files changed, 82 insertions(+) create mode 100644 test/integration/filters/reset_idle_timer_filter.cc diff --git a/test/integration/BUILD b/test/integration/BUILD index 7e6d0990dbf6a..873ee7db78b20 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -626,6 +626,7 @@ envoy_cc_test( deps = [ ":http_protocol_integration_lib", "//test/integration/filters:backpressure_filter_config_lib", + "//test/integration/filters:reset_idle_timer_filter_lib", "//test/test_common:test_time_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 52477e4957a21..524a26500ffd5 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -262,6 +262,21 @@ envoy_cc_test_library( ], ) +envoy_cc_test_library( + name = "reset_idle_timer_filter_lib", + srcs = [ + "reset_idle_timer_filter.cc", + ], + deps = [ + ":common_lib", + "//envoy/http:filter_interface", + "//envoy/registry", + "//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 = "set_response_code_filter_lib", srcs = [ diff --git a/test/integration/filters/reset_idle_timer_filter.cc b/test/integration/filters/reset_idle_timer_filter.cc new file mode 100644 index 0000000000000..676e522542b11 --- /dev/null +++ b/test/integration/filters/reset_idle_timer_filter.cc @@ -0,0 +1,39 @@ +#include + +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "source/extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" + +namespace Envoy { + +// A test filter that resets the stream idle timer via callbacks. +class ResetIdleTimerFilter : public Http::PassThroughFilter { +public: + Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override { + decoder_callbacks_->resetIdleTimer(); + return Http::FilterDataStatus::Continue; + } +}; + +class ResetIdleTimerFilterConfig : public Extensions::HttpFilters::Common::EmptyHttpFilterConfig { +public: + ResetIdleTimerFilterConfig() : EmptyHttpFilterConfig("reset-idle-timer") {} + + Http::FilterFactoryCb createFilter(const std::string&, + Server::Configuration::FactoryContext&) override { + return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared<::Envoy::ResetIdleTimerFilter>()); + }; + } +}; + +// perform static registration +static Registry::RegisterFactory + register_; + +} // namespace Envoy diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index d640d54c5da92..d276fa1856485 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -411,6 +411,33 @@ TEST_P(IdleTimeoutIntegrationTest, RequestTimeoutIsNotDisarmedByEncode100Continu EXPECT_EQ("request timeout", response->body()); } +// Per-stream idle timeout reset from within a filter. +TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutResetFromFilter) { + config_helper_.addFilter(R"EOF( + name: reset-idle-timer-filter + )EOF"); + enable_per_stream_idle_timeout_ = true; + + auto response = setupPerStreamIdleTimeoutTest(); + + sleep(); + codec_client_->sendData(*request_encoder_, 1, false); + + // Two sleeps should trigger the timer, as each advances time by timeout / 2. However, the data + // frame above would have used the filter to reset the timer. Thus, the stream should not be reset + // yet. + sleep(); + + EXPECT_FALSE(response->complete()); + + sleep(); + + // Now the timer should have triggered. + EXPECT_TRUE(response->complete()); + EXPECT_EQ("408", response->headers().getStatusValue()); + EXPECT_EQ("stream timeout", response->body()); +} + // TODO(auni53) create a test filter that hangs and does not send data upstream, which would // trigger a configured request_timer From d19c6a168593e36ccc9b808eebeff413995467c6 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 8 Jul 2021 16:46:31 -0700 Subject: [PATCH 3/5] integration test Signed-off-by: Jose Nino --- test/integration/idle_timeout_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index d276fa1856485..724eeb148f029 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -424,8 +424,8 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutResetFromFilter) { codec_client_->sendData(*request_encoder_, 1, false); // Two sleeps should trigger the timer, as each advances time by timeout / 2. However, the data - // frame above would have used the filter to reset the timer. Thus, the stream should not be reset - // yet. + // frame above would have caused the filter to reset the timer. Thus, the stream should not be + // reset yet. sleep(); EXPECT_FALSE(response->complete()); From 4fb10fe0a2ac15e4a1e20a167a6cfdf76116156d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 9 Jul 2021 10:06:25 -0700 Subject: [PATCH 4/5] name Signed-off-by: Jose Nino --- test/integration/filters/reset_idle_timer_filter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/filters/reset_idle_timer_filter.cc b/test/integration/filters/reset_idle_timer_filter.cc index 676e522542b11..d1f7430b5525c 100644 --- a/test/integration/filters/reset_idle_timer_filter.cc +++ b/test/integration/filters/reset_idle_timer_filter.cc @@ -21,7 +21,7 @@ class ResetIdleTimerFilter : public Http::PassThroughFilter { class ResetIdleTimerFilterConfig : public Extensions::HttpFilters::Common::EmptyHttpFilterConfig { public: - ResetIdleTimerFilterConfig() : EmptyHttpFilterConfig("reset-idle-timer") {} + ResetIdleTimerFilterConfig() : EmptyHttpFilterConfig("reset-idle-timer-filter") {} Http::FilterFactoryCb createFilter(const std::string&, Server::Configuration::FactoryContext&) override { From a58b30e7ebc2d165977ad4349db60f919a6b9489 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 9 Jul 2021 10:37:03 -0700 Subject: [PATCH 5/5] and verify Signed-off-by: Jose Nino --- test/integration/idle_timeout_integration_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 724eeb148f029..9a8e0f75e1972 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -432,6 +432,8 @@ TEST_P(IdleTimeoutIntegrationTest, PerStreamIdleTimeoutResetFromFilter) { sleep(); + waitForTimeout(*response, "downstream_rq_idle_timeout"); + // Now the timer should have triggered. EXPECT_TRUE(response->complete()); EXPECT_EQ("408", response->headers().getStatusValue());