diff --git a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto index a6c5140cc924..74cc5d334019 100644 --- a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto +++ b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto @@ -250,7 +250,6 @@ message ExternalProcessor { bool disable_clear_route_cache = 11 [(udpa.annotations.field_migrate).oneof_promotion = "clear_route_cache_type"]; - // [#not-implemented-hide:] // Specifies the action to be taken when an external processor response is // received in response to request headers. It is recommended to set this field than set // :ref:`disable_clear_route_cache `. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 05dacd358a02..0d6985e8d957 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -7,6 +7,11 @@ behavior_changes: Changes the behavior of the ``SlotImpl`` class destructor. With this change the destructor can be called on any thread. This behavior can be reverted by setting the runtime flag ``envoy.reloadable_features.allow_slot_destroy_on_worker_threads`` to false. +- area: ext_proc + change: | + Adding support for + :ref:`route_cache_action `. + It specifies the route action to be taken when an external processor response is received in response to request headers. - area: http2 change: | Changes the default value of ``envoy.reloadable_features.http2_use_oghttp2`` to true. This changes the codec used for HTTP/2 diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 4cfe69f72fdd..ff97f25c12ee 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -154,8 +154,8 @@ class FilterConfig { Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder, Server::Configuration::CommonFactoryContext& context) : failure_mode_allow_(config.failure_mode_allow()), - disable_clear_route_cache_(config.disable_clear_route_cache()), - message_timeout_(message_timeout), max_message_timeout_ms_(max_message_timeout_ms), + route_cache_action_(config.route_cache_action()), message_timeout_(message_timeout), + max_message_timeout_ms_(max_message_timeout_ms), stats_(generateStats(stats_prefix, config.stat_prefix(), scope)), processing_mode_(config.processing_mode()), mutation_checker_(config.mutation_rules(), context.regexEngine()), @@ -177,7 +177,18 @@ class FilterConfig { config.metadata_options().receiving_namespaces().untyped().end()), expression_manager_(builder, context.localInfo(), config.request_attributes(), config.response_attributes()), - immediate_mutation_checker_(context.regexEngine()) {} + immediate_mutation_checker_(context.regexEngine()) { + if (config.disable_clear_route_cache() && + (route_cache_action_ != + envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::DEFAULT)) { + ExceptionUtil::throwEnvoyException("disable_clear_route_cache and route_cache_action can not " + "be set to none-default at the same time."); + } + if (config.disable_clear_route_cache()) { + route_cache_action_ = + envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::RETAIN; + } + } bool failureModeAllow() const { return failure_mode_allow_; } @@ -198,7 +209,10 @@ class FilterConfig { return mutation_checker_; } - bool disableClearRouteCache() const { return disable_clear_route_cache_; } + envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::RouteCacheAction + routeCacheAction() const { + return route_cache_action_; + } const std::vector& allowedHeaders() const { return allowed_headers_; } const std::vector& disallowedHeaders() const { @@ -234,7 +248,8 @@ class FilterConfig { return {ALL_EXT_PROC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; } const bool failure_mode_allow_; - const bool disable_clear_route_cache_; + envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::RouteCacheAction + route_cache_action_; const std::chrono::milliseconds message_timeout_; const uint32_t max_message_timeout_ms_; diff --git a/source/extensions/filters/http/ext_proc/processor_state.cc b/source/extensions/filters/http/ext_proc/processor_state.cc index f2b5cb60fa9c..88fb2b2e6333 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.cc +++ b/source/extensions/filters/http/ext_proc/processor_state.cc @@ -420,27 +420,45 @@ void DecodingProcessorState::clearWatermark() { } void DecodingProcessorState::clearRouteCache(const CommonResponse& common_response) { - if (!common_response.clear_route_cache()) { - return; - } + bool response_clear_route_cache = common_response.clear_route_cache(); if (filter_.config().isUpstream()) { - filter_.stats().clear_route_cache_upstream_ignored_.inc(); - ENVOY_LOG(debug, "NOT clearing route cache. The filter is in upstream filter chain."); + if (response_clear_route_cache) { + filter_.stats().clear_route_cache_upstream_ignored_.inc(); + ENVOY_LOG(debug, "NOT clearing route cache. The filter is in upstream filter chain."); + } return; } - // Only clear the route cache if there is a mutation to the header and clearing is allowed. - if (filter_.config().disableClearRouteCache()) { - filter_.stats().clear_route_cache_disabled_.inc(); - ENVOY_LOG(debug, "NOT clearing route cache, it is disabled in the config"); + + if (!common_response.has_header_mutation()) { + if (response_clear_route_cache) { + filter_.stats().clear_route_cache_ignored_.inc(); + ENVOY_LOG(debug, "NOT clearing route cache. No header mutation in the response"); + } return; } - if (common_response.has_header_mutation()) { - ENVOY_LOG(debug, "clearing route cache"); + + // Filter is in downstream and response has header mutation. + switch (filter_.config().routeCacheAction()) { + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; + case envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::DEFAULT: + if (response_clear_route_cache) { + ENVOY_LOG(debug, "Clearing route cache due to the filter RouterCacheAction is configured " + "with DEFAULT and response has clear_route_cache set."); + decoder_callbacks_->downstreamCallbacks()->clearRouteCache(); + } + break; + case envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::CLEAR: + ENVOY_LOG(debug, + "Clearing route cache due to the filter RouterCacheAction is configured with CLEAR"); decoder_callbacks_->downstreamCallbacks()->clearRouteCache(); - return; + break; + case envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor::RETAIN: + if (response_clear_route_cache) { + filter_.stats().clear_route_cache_disabled_.inc(); + ENVOY_LOG(debug, "NOT clearing route cache, it is disabled by the filter config"); + } + break; } - filter_.stats().clear_route_cache_ignored_.inc(); - ENVOY_LOG(debug, "NOT clearing route cache, no header mutations detected"); } void EncodingProcessorState::setProcessingModeInternal(const ProcessingMode& mode) { diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index bd7bf8766732..b362ca708143 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -2714,6 +2714,7 @@ TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutation) { EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0); EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_upstream_ignored_.value(), 0); EXPECT_EQ(config_->stats().streams_started_.value(), 1); EXPECT_EQ(config_->stats().stream_msgs_sent_.value(), 3); EXPECT_EQ(config_->stats().stream_msgs_received_.value(), 3); @@ -2803,12 +2804,211 @@ TEST_F(HttpFilterTest, ClearRouteCacheUnchanged) { EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 1); EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_upstream_ignored_.value(), 0); EXPECT_EQ(config_->stats().streams_started_.value(), 1); EXPECT_EQ(config_->stats().stream_msgs_sent_.value(), 3); EXPECT_EQ(config_->stats().stream_msgs_received_.value(), 3); EXPECT_EQ(config_->stats().streams_closed_.value(), 1); } +// Using the default configuration, "clear_route_cache" flag not set. No header mutation. +TEST_F(HttpFilterTest, ClearRouteCacheUnchangedNoClearFlag) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + processing_mode: + response_body_mode: "BUFFERED" + )EOF"); + + // Do not call ClearRouteCache() for inbound traffic without header mutation. + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + processRequestHeaders(false, absl::nullopt); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + processResponseHeaders(true, absl::nullopt); + + Buffer::OwnedImpl resp_data("foo"); + Buffer::OwnedImpl buffered_response_data; + setUpEncodingBuffering(buffered_response_data); + + // There is no ClearRouteCache() call for outbound traffic regardless. + EXPECT_EQ(FilterDataStatus::StopIterationNoBuffer, filter_->encodeData(resp_data, true)); + processResponseBody([](const HttpBody&, ProcessingResponse&, BodyResponse& resp) { + resp.mutable_response()->set_clear_route_cache(true); + }); + + filter_->onDestroy(); + + EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_upstream_ignored_.value(), 0); +} + +// Verify that the "disable_route_cache_clearing" and "route_cache_action" setting +// can not be set at the same time. +TEST_F(HttpFilterTest, ClearRouteCacheDisableRouteCacheActionBothSet) { + std::string yaml = R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + processing_mode: + response_body_mode: "BUFFERED" + disable_clear_route_cache: true + route_cache_action: CLEAR + )EOF"; + + envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor proto_config{}; + TestUtility::loadFromYaml(yaml, proto_config); + EXPECT_THROW_WITH_MESSAGE( + { + auto config = std::make_shared( + proto_config, 200ms, 10000, *stats_store_.rootScope(), "", false, + std::make_shared( + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr)), + factory_context_); + }, + EnvoyException, + "disable_clear_route_cache and route_cache_action can not be set to none-default at the same " + "time."); +} + +// Verify that with header mutation in response, setting route_cache_action to CLEAR +// will clear route cache even the response does not set clear_route_cache. +TEST_F(HttpFilterTest, FilterRouteCacheActionSetToClearHeaderMutation) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + route_cache_action: CLEAR + )EOF"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + EXPECT_CALL(decoder_callbacks_.downstream_callbacks_, clearRouteCache()); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { + auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); + auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->mutable_header()->set_key("x-new-header"); + resp_add->mutable_header()->set_value("new"); + }); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + processResponseHeaders(true, absl::nullopt); + + filter_->onDestroy(); + + EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_upstream_ignored_.value(), 0); + EXPECT_EQ(config_->stats().streams_started_.value(), 1); + EXPECT_EQ(config_->stats().stream_msgs_sent_.value(), 2); + EXPECT_EQ(config_->stats().stream_msgs_received_.value(), 2); + EXPECT_EQ(config_->stats().streams_closed_.value(), 1); +} + +// Verify that without header mutation in response, setting route_cache_action to CLEAR +// and set the clear_route_cache flag to true in the response will not clear route cache. +TEST_F(HttpFilterTest, FilterRouteCacheActionSetToClearNoHeaderMutation) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + route_cache_action: CLEAR + )EOF"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { + resp.mutable_response()->set_clear_route_cache(true); + }); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + processResponseHeaders(true, absl::nullopt); + + filter_->onDestroy(); + + EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 1); +} + +// Verify that without header mutation in response, setting route_cache_action to CLEAR and not +// set the clear_route_cache flag to true in the response will not clear route cache. +TEST_F(HttpFilterTest, FilterRouteCacheActionSetToClearResponseNotSetNoHeaderMutation) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + route_cache_action: CLEAR + )EOF"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + // There is no clear_route_cache set in the response. clear_route_cache_ignored_ is zero in this + // case. + processRequestHeaders(false, absl::nullopt); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + processResponseHeaders(true, absl::nullopt); + + filter_->onDestroy(); + + EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 0); +} + +// Verify that setting route_cache_action to RETAIN will not clear route cache. +TEST_F(HttpFilterTest, FilterRouteCacheActionSetToRetainWithHeaderMutation) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + route_cache_action: RETAIN + )EOF"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { + auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); + auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->mutable_header()->set_key("x-new-header"); + resp_add->mutable_header()->set_value("new"); + resp.mutable_response()->set_clear_route_cache(true); + }); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + processResponseHeaders(true, absl::nullopt); + + filter_->onDestroy(); + + EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 1); + EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 0); +} + +// Verify that setting route_cache_action to RETAIN, response clear_route_cache flag not set, +// will not clear route cache. +TEST_F(HttpFilterTest, FilterRouteCacheActionSetToRetainResponseNotWithHeaderMutation) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + route_cache_action: RETAIN + )EOF"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { + auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); + auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->mutable_header()->set_key("x-new-header"); + resp_add->mutable_header()->set_value("new"); + }); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + processResponseHeaders(true, absl::nullopt); + + filter_->onDestroy(); + + // This counter will not increase as clear_route_cache flag in the response is not set. + EXPECT_EQ(config_->stats().clear_route_cache_disabled_.value(), 0); + EXPECT_EQ(config_->stats().clear_route_cache_ignored_.value(), 0); +} + // Using the default configuration, turn a GET into a POST. TEST_F(HttpFilterTest, ReplaceRequest) { initialize(R"EOF(