diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index bc4545a91e3cf..c1ade1087e365 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -888,18 +888,6 @@ void Utility::transformUpgradeResponseFromH2toH1(ResponseHeaderMap& headers, } } -const Router::RouteSpecificFilterConfig* -Utility::resolveMostSpecificPerFilterConfigGeneric(const std::string& filter_name, - const Router::RouteConstSharedPtr& route) { - - const Router::RouteSpecificFilterConfig* maybe_filter_config{}; - traversePerFilterConfigGeneric( - filter_name, route, [&maybe_filter_config](const Router::RouteSpecificFilterConfig& cfg) { - maybe_filter_config = &cfg; - }); - return maybe_filter_config; -} - void Utility::traversePerFilterConfigGeneric( const std::string& filter_name, const Router::RouteConstSharedPtr& route, std::function cb) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index b05ff1b0b5106..4c972a3fb4cf0 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -487,9 +487,10 @@ const ConfigType* resolveMostSpecificPerFilterConfig(const std::string& filter_n const Router::RouteConstSharedPtr& route) { static_assert(std::is_base_of::value, "ConfigType must be a subclass of Router::RouteSpecificFilterConfig"); - const Router::RouteSpecificFilterConfig* generic_config = - resolveMostSpecificPerFilterConfigGeneric(filter_name, route); - return dynamic_cast(generic_config); + if (!route || !route->routeEntry()) { + return nullptr; + } + return route->routeEntry()->mostSpecificPerFilterConfigTyped(filter_name); } /** diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc index ace0940304571..e1597a36ae95f 100644 --- a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc +++ b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc @@ -120,12 +120,8 @@ Filter::Filter(const FilterSettings& settings, const FilterStats& stats, : settings_(settings), stats_(stats), sigv4_signer_(sigv4_signer) {} absl::optional Filter::getRouteSpecificSettings() const { - if (!decoder_callbacks_->route() || !decoder_callbacks_->route()->routeEntry()) { - return absl::nullopt; - } - const auto* route_entry = decoder_callbacks_->route()->routeEntry(); - const auto* settings = route_entry->mostSpecificPerFilterConfigTyped( - HttpFilterNames::get().AwsLambda); + const auto* settings = Http::Utility::resolveMostSpecificPerFilterConfig( + HttpFilterNames::get().AwsLambda, decoder_callbacks_->route()); if (!settings) { return absl::nullopt; } diff --git a/source/extensions/filters/http/buffer/buffer_filter.cc b/source/extensions/filters/http/buffer/buffer_filter.cc index eeb4c81304fa7..18cf208e71b50 100644 --- a/source/extensions/filters/http/buffer/buffer_filter.cc +++ b/source/extensions/filters/http/buffer/buffer_filter.cc @@ -42,17 +42,10 @@ BufferFilter::BufferFilter(BufferFilterConfigSharedPtr config) void BufferFilter::initConfig() { ASSERT(!config_initialized_); config_initialized_ = true; - settings_ = config_->settings(); - if (!callbacks_->route() || !callbacks_->route()->routeEntry()) { - return; - } - - const std::string& name = HttpFilterNames::get().Buffer; - const auto* entry = callbacks_->route()->routeEntry(); - const auto* route_local = entry->mostSpecificPerFilterConfigTyped(name); - + const auto* route_local = Http::Utility::resolveMostSpecificPerFilterConfig( + HttpFilterNames::get().Buffer, callbacks_->route()); settings_ = route_local ? route_local : settings_; } diff --git a/source/extensions/filters/http/dynamic_forward_proxy/BUILD b/source/extensions/filters/http/dynamic_forward_proxy/BUILD index 5b0768fe9d2d8..e0501274f552a 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/BUILD +++ b/source/extensions/filters/http/dynamic_forward_proxy/BUILD @@ -15,6 +15,7 @@ envoy_cc_library( hdrs = ["proxy_filter.h"], deps = [ "//include/envoy/http:filter_interface", + "//source/common/http:header_utility_lib", "//source/extensions/clusters:well_known_names", "//source/extensions/common/dynamic_forward_proxy:dns_cache_interface", "//source/extensions/filters/http:well_known_names", diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc index 09856fa4f23ef..c1ee980393d50 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc @@ -4,6 +4,8 @@ #include "envoy/config/core/v3/base.pb.h" #include "envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.pb.h" +#include "common/http/utility.h" + #include "extensions/clusters/well_known_names.h" #include "extensions/common/dynamic_forward_proxy/dns_cache.h" #include "extensions/filters/http/well_known_names.h" @@ -87,8 +89,9 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea } // Check for per route filter config. - const auto* config = route_entry->mostSpecificPerFilterConfigTyped( - HttpFilterNames::get().DynamicForwardProxy); + const auto* config = Http::Utility::resolveMostSpecificPerFilterConfig( + HttpFilterNames::get().DynamicForwardProxy, route); + if (config != nullptr) { const auto& host_rewrite = config->hostRewrite(); if (!host_rewrite.empty()) { diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index de302a2216986..8c21d85f19413 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -113,14 +113,9 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::RequestHeaderMap& hea // faults. In other words, runtime is supported only when faults are // configured at the filter level. fault_settings_ = config_->settings(); - if (decoder_callbacks_->route() && decoder_callbacks_->route()->routeEntry()) { - const std::string& name = Extensions::HttpFilters::HttpFilterNames::get().Fault; - const auto* route_entry = decoder_callbacks_->route()->routeEntry(); - - const auto* per_route_settings = - route_entry->mostSpecificPerFilterConfigTyped(name); - fault_settings_ = per_route_settings ? per_route_settings : fault_settings_; - } + const auto* per_route_settings = Http::Utility::resolveMostSpecificPerFilterConfig( + Extensions::HttpFilters::HttpFilterNames::get().Fault, decoder_callbacks_->route()); + fault_settings_ = per_route_settings ? per_route_settings : fault_settings_; if (!matchesTargetUpstreamCluster()) { return Http::FilterHeadersStatus::Continue; diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 3eefd8bfb5210..a7d93f1b63429 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -419,14 +419,8 @@ JsonTranscoderConfig::translateProtoMessageToJson(const Protobuf::Message& messa JsonTranscoderFilter::JsonTranscoderFilter(JsonTranscoderConfig& config) : config_(config) {} void JsonTranscoderFilter::initPerRouteConfig() { - if (!decoder_callbacks_->route() || !decoder_callbacks_->route()->routeEntry()) { - per_route_config_ = &config_; - return; - } - - const std::string& name = HttpFilterNames::get().GrpcJsonTranscoder; - const auto* entry = decoder_callbacks_->route()->routeEntry(); - const auto* route_local = entry->mostSpecificPerFilterConfigTyped(name); + const auto* route_local = Http::Utility::resolveMostSpecificPerFilterConfig( + HttpFilterNames::get().GrpcJsonTranscoder, decoder_callbacks_->route()); per_route_config_ = route_local ? route_local : &config_; } diff --git a/source/extensions/filters/http/kill_request/kill_request_filter.cc b/source/extensions/filters/http/kill_request/kill_request_filter.cc index b278fabe235ca..b6269db1593bd 100644 --- a/source/extensions/filters/http/kill_request/kill_request_filter.cc +++ b/source/extensions/filters/http/kill_request/kill_request_filter.cc @@ -6,6 +6,7 @@ #include "envoy/extensions/filters/http/kill_request/v3/kill_request.pb.h" #include "envoy/http/header_map.h" +#include "common/http/utility.h" #include "common/protobuf/utility.h" #include "extensions/filters/http/well_known_names.h" @@ -52,19 +53,15 @@ Http::FilterHeadersStatus KillRequestFilter::decodeHeaders(Http::RequestHeaderMa } // Route-level configuration overrides filter-level configuration. - if (decoder_callbacks_->route() && decoder_callbacks_->route()->routeEntry()) { - const std::string& name = Extensions::HttpFilters::HttpFilterNames::get().KillRequest; - const auto* route_entry = decoder_callbacks_->route()->routeEntry(); - - const auto* per_route_kill_settings = - route_entry->mostSpecificPerFilterConfigTyped(name); - - if (per_route_kill_settings) { - is_correct_direction = per_route_kill_settings->getDirection() == KillRequest::REQUEST; - // Allocate the probability into kill_request in case the lifetime of - // per_route_kill_settings does not match that of kill_request_ - kill_request_.mutable_probability()->CopyFrom(per_route_kill_settings->getProbability()); - } + const auto* per_route_kill_settings = + Http::Utility::resolveMostSpecificPerFilterConfig( + Extensions::HttpFilters::HttpFilterNames::get().KillRequest, decoder_callbacks_->route()); + + if (per_route_kill_settings) { + is_correct_direction = per_route_kill_settings->getDirection() == KillRequest::REQUEST; + // Allocate the probability into kill_request in case the lifetime of + // per_route_kill_settings does not match that of kill_request_ + kill_request_.mutable_probability()->CopyFrom(per_route_kill_settings->getProbability()); } if (is_kill_request && is_correct_direction && isKillRequestEnabled()) { diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index 9d75db7f661fb..bd2b477b468c6 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -26,15 +26,8 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( const Filters::Common::RBAC::RoleBasedAccessControlEngineImpl* RoleBasedAccessControlFilterConfig::engine(const Router::RouteConstSharedPtr route, Filters::Common::RBAC::EnforcementMode mode) const { - if (!route || !route->routeEntry()) { - return engine(mode); - } - - const std::string& name = HttpFilterNames::get().Rbac; - const auto* entry = route->routeEntry(); - const auto* route_local = - entry->mostSpecificPerFilterConfigTyped( - name); + const auto* route_local = Http::Utility::resolveMostSpecificPerFilterConfig< + RoleBasedAccessControlRouteSpecificFilterConfig>(HttpFilterNames::get().Rbac, route); if (route_local) { return route_local->engine(mode); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index c8c9d311af5fc..75d1b72ca10ef 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -872,37 +872,21 @@ TEST(HttpUtility, ResetReasonToString) { Utility::resetReasonToString(Http::StreamResetReason::ConnectError)); } -// Verify that it resolveMostSpecificPerFilterConfigGeneric works with nil routes. -TEST(HttpUtility, ResolveMostSpecificPerFilterConfigNilRoute) { - EXPECT_EQ(nullptr, Utility::resolveMostSpecificPerFilterConfigGeneric("envoy.filter", nullptr)); -} - class TestConfig : public Router::RouteSpecificFilterConfig { public: int state_; void merge(const TestConfig& other) { state_ += other.state_; } }; -// Verify that resolveMostSpecificPerFilterConfig works and we get back the original type. -TEST(HttpUtility, ResolveMostSpecificPerFilterConfig) { - TestConfig testConfig; - - const std::string filter_name = "envoy.filter"; - NiceMock filter_callbacks; - - // make the file callbacks return our test config - ON_CALL(*filter_callbacks.route_, perFilterConfig(filter_name)) - .WillByDefault(Return(&testConfig)); - - // test the we get the same object back (as this goes through the dynamic_cast) - auto resolved_filter_config = Utility::resolveMostSpecificPerFilterConfig( - filter_name, filter_callbacks.route()); - EXPECT_EQ(&testConfig, resolved_filter_config); +// Verify that it resolveMostSpecificPerFilterConfig works with nil routes. +TEST(HttpUtility, ResolveMostSpecificPerFilterConfigNilRoute) { + EXPECT_EQ(nullptr, + Utility::resolveMostSpecificPerFilterConfig("envoy.filter", nullptr)); } -// Verify that resolveMostSpecificPerFilterConfigGeneric indeed returns the most specific per +// Verify that resolveMostSpecificPerFilterConfig indeed returns the most specific per // filter config. -TEST(HttpUtility, ResolveMostSpecificPerFilterConfigGeneric) { +TEST(HttpUtility, ResolveMostSpecificPerFilterConfig) { const std::string filter_name = "envoy.filter"; NiceMock filter_callbacks; @@ -911,28 +895,34 @@ TEST(HttpUtility, ResolveMostSpecificPerFilterConfigGeneric) { const Router::RouteSpecificFilterConfig three; // Test when there's nothing on the route - EXPECT_EQ(nullptr, Utility::resolveMostSpecificPerFilterConfigGeneric(filter_name, - filter_callbacks.route())); + EXPECT_EQ(nullptr, Utility::resolveMostSpecificPerFilterConfig( + filter_name, filter_callbacks.route())); // Testing in reverse order, so that the method always returns the last object. + // Testing per-virtualhost typed filter config ON_CALL(filter_callbacks.route_->route_entry_.virtual_host_, perFilterConfig(filter_name)) .WillByDefault(Return(&one)); - EXPECT_EQ(&one, Utility::resolveMostSpecificPerFilterConfigGeneric(filter_name, - filter_callbacks.route())); + EXPECT_EQ(&one, Utility::resolveMostSpecificPerFilterConfig( + filter_name, filter_callbacks.route())); + // Testing per-route typed filter config ON_CALL(*filter_callbacks.route_, perFilterConfig(filter_name)).WillByDefault(Return(&two)); - EXPECT_EQ(&two, Utility::resolveMostSpecificPerFilterConfigGeneric(filter_name, - filter_callbacks.route())); + ON_CALL(filter_callbacks.route_->route_entry_, perFilterConfig(filter_name)) + .WillByDefault(Invoke( + [&](const std::string& name) { return filter_callbacks.route_->perFilterConfig(name); })); + EXPECT_EQ(&two, Utility::resolveMostSpecificPerFilterConfig( + filter_name, filter_callbacks.route())); + // Testing per-route entry typed filter config ON_CALL(filter_callbacks.route_->route_entry_, perFilterConfig(filter_name)) .WillByDefault(Return(&three)); - EXPECT_EQ(&three, Utility::resolveMostSpecificPerFilterConfigGeneric(filter_name, - filter_callbacks.route())); + EXPECT_EQ(&three, Utility::resolveMostSpecificPerFilterConfig( + filter_name, filter_callbacks.route())); // Cover the case of no route entry ON_CALL(*filter_callbacks.route_, routeEntry()).WillByDefault(Return(nullptr)); - EXPECT_EQ(&two, Utility::resolveMostSpecificPerFilterConfigGeneric(filter_name, - filter_callbacks.route())); + EXPECT_EQ(nullptr, Utility::resolveMostSpecificPerFilterConfig( + filter_name, filter_callbacks.route())); } // Verify that traversePerFilterConfigGeneric traverses in the order of specificity. diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index e0f2ec53e031f..2544068ff083f 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -1498,7 +1498,8 @@ TEST_P(HttpFilterTestParam, ContextExtensions) { "value_route"; // Initialize the route's per filter config. FilterConfigPerRoute auth_per_route(settingsroute); - ON_CALL(*filter_callbacks_.route_, perFilterConfig(HttpFilterNames::get().ExtAuthorization)) + ON_CALL(filter_callbacks_.route_->route_entry_, + perFilterConfig(HttpFilterNames::get().ExtAuthorization)) .WillByDefault(Return(&auth_per_route)); prepareCheck(); @@ -1528,7 +1529,8 @@ TEST_P(HttpFilterTestParam, DisabledOnRoute) { prepareCheck(); - ON_CALL(*filter_callbacks_.route_, perFilterConfig(HttpFilterNames::get().ExtAuthorization)) + ON_CALL(filter_callbacks_.route_->route_entry_, + perFilterConfig(HttpFilterNames::get().ExtAuthorization)) .WillByDefault(Return(&auth_per_route)); auto test_disable = [&](bool disabled) { @@ -1559,7 +1561,8 @@ TEST_P(HttpFilterTestParam, DisabledOnRouteWithRequestBody) { envoy::extensions::filters::http::ext_authz::v3::ExtAuthzPerRoute settings; FilterConfigPerRoute auth_per_route(settings); - ON_CALL(*filter_callbacks_.route_, perFilterConfig(HttpFilterNames::get().ExtAuthorization)) + ON_CALL(filter_callbacks_.route_->route_entry_, + perFilterConfig(HttpFilterNames::get().ExtAuthorization)) .WillByDefault(Return(&auth_per_route)); auto test_disable = [&](bool disabled) { @@ -2137,7 +2140,8 @@ TEST_P(HttpFilterTestParam, NoCluster) { "value_route"; // Initialize the route's per filter config. FilterConfigPerRoute auth_per_route(settingsroute); - ON_CALL(*filter_callbacks_.route_, perFilterConfig(HttpFilterNames::get().ExtAuthorization)) + ON_CALL(filter_callbacks_.route_->route_entry_, + perFilterConfig(HttpFilterNames::get().ExtAuthorization)) .WillByDefault(Return(&auth_per_route)); prepareCheck(); @@ -2163,7 +2167,8 @@ TEST_P(HttpFilterTestParam, DisableRequestBodyBufferingOnRoute) { envoy::extensions::filters::http::ext_authz::v3::ExtAuthzPerRoute settings; FilterConfigPerRoute auth_per_route(settings); - ON_CALL(*filter_callbacks_.route_, perFilterConfig(HttpFilterNames::get().ExtAuthorization)) + ON_CALL(filter_callbacks_.route_->route_entry_, + perFilterConfig(HttpFilterNames::get().ExtAuthorization)) .WillByDefault(Return(&auth_per_route)); auto test_disable_request_body_buffering = [&](bool bypass) { diff --git a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc index 599e16a845f30..8f0f31ca60d4c 100644 --- a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc +++ b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc @@ -616,7 +616,7 @@ TEST_F(ReverseBridgeTest, FilterConfigPerRouteDisabled) { filter_config_per_route.set_disabled(true); FilterConfigPerRoute filterConfigPerRoute(filter_config_per_route); - ON_CALL(*decoder_callbacks_.route_, + ON_CALL(decoder_callbacks_.route_->route_entry_, perFilterConfig(HttpFilterNames::get().GrpcHttp1ReverseBridge)) .WillByDefault(testing::Return(&filterConfigPerRoute)); @@ -645,7 +645,7 @@ TEST_F(ReverseBridgeTest, FilterConfigPerRouteEnabled) { filter_config_per_route.set_disabled(false); FilterConfigPerRoute filterConfigPerRoute(filter_config_per_route); - ON_CALL(*decoder_callbacks_.route_, + ON_CALL(decoder_callbacks_.route_->route_entry_, perFilterConfig(HttpFilterNames::get().GrpcHttp1ReverseBridge)) .WillByDefault(testing::Return(&filterConfigPerRoute)); @@ -733,7 +733,7 @@ TEST_F(ReverseBridgeTest, RouteWithTrailers) { filter_config_per_route.set_disabled(false); FilterConfigPerRoute filterConfigPerRoute(filter_config_per_route); - ON_CALL(*decoder_callbacks_.route_, + ON_CALL(decoder_callbacks_.route_->route_entry_, perFilterConfig(HttpFilterNames::get().GrpcHttp1ReverseBridge)) .WillByDefault(testing::Return(&filterConfigPerRoute)); diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index b0b6f6a11f3e4..d2fe4aba64437 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -33,7 +33,7 @@ declare -a KNOWN_LOW_COVERAGE=( "source/extensions/filters/common/rbac:87.5" "source/extensions/filters/http/cache:92.4" "source/extensions/filters/http/cache/simple_http_cache:95.2" -"source/extensions/filters/http/grpc_json_transcoder:95.7" +"source/extensions/filters/http/grpc_json_transcoder:95.6" "source/extensions/filters/http/ip_tagging:91.2" "source/extensions/filters/http/kill_request:85.0" # Death tests don't report LCOV "source/extensions/filters/listener/tls_inspector:92.4"