From b11c25fc3745af55b0d23de5237b22f5bf1b0886 Mon Sep 17 00:00:00 2001 From: Jose Ulises Nino Rivera Date: Sat, 10 Jul 2021 08:01:02 -0700 Subject: [PATCH] filter: add ability to resetIdleTimer (#17274) 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/integration/BUILD | 1 + test/integration/filters/BUILD | 15 +++++++ .../filters/reset_idle_timer_filter.cc | 39 +++++++++++++++++++ .../idle_timeout_integration_test.cc | 29 ++++++++++++++ test/mocks/http/mocks.h | 2 + 10 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 test/integration/filters/reset_idle_timer_filter.cc diff --git a/envoy/http/filter.h b/envoy/http/filter.h index e677c85e1b7d..6dcf6f2f42f3 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 d496e21bbefa..d7193564ef6d 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 187647125234..811807a9561d 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -270,6 +270,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 2445faed0a10..7c16f0edd1e2 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -145,6 +145,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 8587b0f50ac8..ed71a44fe5ff 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/integration/BUILD b/test/integration/BUILD index 7e6d0990dbf6..873ee7db78b2 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 52477e4957a2..524a26500ffd 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 000000000000..d1f7430b5525 --- /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-filter") {} + + 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 97d6ccd35403..9ba8b0ff29eb 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -427,6 +427,35 @@ 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 caused the filter to reset the timer. Thus, the stream should not be + // reset yet. + sleep(); + + EXPECT_FALSE(response->complete()); + + sleep(); + + waitForTimeout(*response, "downstream_rq_idle_timeout"); + + // 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 diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 55e35884ce9e..21c17e4f7ad9 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, ());