diff --git a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc index 51869ca4dcc..039ce9a7213 100644 --- a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc @@ -34,7 +34,6 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/exec_ctx.h" -#include "src/core/lib/surface/lame_client.h" #include "src/core/lib/transport/timeout_encoding.h" namespace grpc_core { @@ -243,7 +242,6 @@ class XdsResolver : public Resolver { RouteTable route_table_; std::map> clusters_; std::vector filters_; - grpc_error_handle filter_error_ = GRPC_ERROR_NONE; }; void OnListenerUpdate(XdsApi::LdsUpdate listener); @@ -422,16 +420,8 @@ XdsResolver::XdsConfigSelector::XdsConfigSelector( } } // Populate filter list. - bool found_router = false; for (const auto& http_filter : resolver_->current_listener_.http_connection_manager.http_filters) { - // Stop at the router filter. It's a no-op for us, and we ignore - // anything that may come after it, for compatibility with Envoy. - if (http_filter.config.config_proto_type_name == - kXdsHttpRouterFilterConfigName) { - found_router = true; - break; - } // Find filter. This is guaranteed to succeed, because it's checked // at config validation time in the XdsApi code. const XdsHttpFilterImpl* filter_impl = @@ -439,16 +429,9 @@ XdsResolver::XdsConfigSelector::XdsConfigSelector( http_filter.config.config_proto_type_name); GPR_ASSERT(filter_impl != nullptr); // Add C-core filter to list. - filters_.push_back(filter_impl->channel_filter()); - } - // For compatibility with Envoy, if the router filter is not - // configured, we fail all RPCs. - if (!found_router) { - filter_error_ = - grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "no xDS HTTP router filter configured"), - GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAVAILABLE); - filters_.push_back(&grpc_lame_filter); + if (filter_impl->channel_filter() != nullptr) { + filters_.push_back(filter_impl->channel_filter()); + } } } @@ -459,7 +442,6 @@ XdsResolver::XdsConfigSelector::~XdsConfigSelector() { } clusters_.clear(); resolver_->MaybeRemoveUnusedClusters(); - GRPC_ERROR_UNREF(filter_error_); } const XdsHttpFilterImpl::FilterConfig* FindFilterConfigOverride( @@ -499,18 +481,15 @@ grpc_error_handle XdsResolver::XdsConfigSelector::CreateMethodConfig( grpc_channel_args* args = grpc_channel_args_copy(resolver_->args_); for (const auto& http_filter : resolver_->current_listener_.http_connection_manager.http_filters) { - // Stop at the router filter. It's a no-op for us, and we ignore - // anything that may come after it, for compatibility with Envoy. - if (http_filter.config.config_proto_type_name == - kXdsHttpRouterFilterConfigName) { - break; - } // Find filter. This is guaranteed to succeed, because it's checked // at config validation time in the XdsApi code. const XdsHttpFilterImpl* filter_impl = XdsHttpFilterRegistry::GetFilterForType( http_filter.config.config_proto_type_name); GPR_ASSERT(filter_impl != nullptr); + // If there is not actually any C-core filter associated with this + // xDS filter, then it won't need any config, so skip it. + if (filter_impl->channel_filter() == nullptr) continue; // Allow filter to add channel args that may affect service config // parsing. args = filter_impl->ModifyChannelArgs(args); @@ -558,12 +537,7 @@ grpc_error_handle XdsResolver::XdsConfigSelector::CreateMethodConfig( grpc_channel_args* XdsResolver::XdsConfigSelector::ModifyChannelArgs( grpc_channel_args* args) { - if (filter_error_ == GRPC_ERROR_NONE) return args; - grpc_arg error_arg = MakeLameClientErrorArg(filter_error_); - grpc_channel_args* new_args = - grpc_channel_args_copy_and_add(args, &error_arg, 1); - grpc_channel_args_destroy(args); - return new_args; + return args; } void XdsResolver::XdsConfigSelector::MaybeAddCluster(const std::string& name) { diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index a221b675969..0dbc5981546 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -2008,6 +2008,23 @@ grpc_error_handle HttpConnectionManagerParse( is_client ? "clients" : "servers") .c_str()); } + if (i < num_filters - 1) { + // Filters before the last filter must not be terminal. + if (filter_impl->IsTerminalFilter()) { + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat("terminal filter for config type ", filter_type, + " must be the last filter in the chain") + .c_str()); + } + } else { + // The last filter must be terminal. + if (!filter_impl->IsTerminalFilter()) { + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat("non-terminal filter for config type ", filter_type, + " is the last filter in the chain") + .c_str()); + } + } absl::StatusOr filter_config = filter_impl->GenerateFilterConfig(google_protobuf_Any_value(any), context.arena); diff --git a/src/core/ext/xds/xds_http_filters.cc b/src/core/ext/xds/xds_http_filters.cc index 9bd4858b2cb..4cf95eb2bdb 100644 --- a/src/core/ext/xds/xds_http_filters.cc +++ b/src/core/ext/xds/xds_http_filters.cc @@ -52,10 +52,9 @@ class XdsHttpRouterFilter : public XdsHttpFilterImpl { "router filter does not support config override"); } - // No-op -- this filter is special-cased by the xds resolver. const grpc_channel_filter* channel_filter() const override { return nullptr; } - // No-op -- this filter is special-cased by the xds resolver. + // No-op. This will never be called, since channel_filter() returns null. absl::StatusOr GenerateServiceConfig( const FilterConfig& /*hcm_filter_config*/, const FilterConfig* /*filter_config_override*/) const override { @@ -65,6 +64,8 @@ class XdsHttpRouterFilter : public XdsHttpFilterImpl { bool IsSupportedOnClients() const override { return true; } bool IsSupportedOnServers() const override { return true; } + + bool IsTerminalFilter() const override { return true; } }; using FilterOwnerList = std::vector>; diff --git a/src/core/ext/xds/xds_http_filters.h b/src/core/ext/xds/xds_http_filters.h index 332419681ed..74f4a0c6267 100644 --- a/src/core/ext/xds/xds_http_filters.h +++ b/src/core/ext/xds/xds_http_filters.h @@ -107,6 +107,9 @@ class XdsHttpFilterImpl { // Returns true if the filter is supported on servers; false otherwise virtual bool IsSupportedOnServers() const = 0; + + // Returns true if the filter must be the last filter in the chain. + virtual bool IsTerminalFilter() const { return false; } }; class XdsHttpFilterRegistry { diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index cd2ce0fb113..9fffbd0cbc7 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -3547,7 +3547,6 @@ TEST_P(LdsTest, NoApiListener) { auto listener = default_listener_; listener.clear_api_listener(); balancers_[0]->ads_service()->SetLdsResource(listener); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3569,7 +3568,6 @@ TEST_P(LdsTest, WrongRouteSpecifier) { listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); balancers_[0]->ads_service()->SetLdsResource(listener); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3593,7 +3591,6 @@ TEST_P(LdsTest, RdsMissingConfigSource) { listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); balancers_[0]->ads_service()->SetLdsResource(listener); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3618,7 +3615,6 @@ TEST_P(LdsTest, RdsConfigSourceDoesNotSpecifyAds) { listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); balancers_[0]->ads_service()->SetLdsResource(listener); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3629,55 +3625,53 @@ TEST_P(LdsTest, RdsConfigSourceDoesNotSpecifyAds) { "RDS does not specify ADS.")); } -// Tests that we ignore filters after the router filter. -TEST_P(LdsTest, IgnoresHttpFiltersAfterRouterFilter) { +// Tests that we NACK non-terminal filters at the end of the list. +TEST_P(LdsTest, NacksNonTerminalHttpFilterAtEndOfList) { SetNextResolutionForLbChannelAllBalancers(); auto listener = default_listener_; HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + auto* filter = http_connection_manager.mutable_http_filters(0); filter->set_name("unknown"); filter->mutable_typed_config()->set_type_url( "grpc.testing.client_only_http_filter"); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - AdsServiceImpl::EdsResourceArgs args({ - {"locality0", CreateEndpointsForBackends()}, - }); - balancers_[0]->ads_service()->SetEdsResource( - BuildEdsResource(args, DefaultEdsServiceName())); - WaitForAllBackends(); + SetNextResolutionForLbChannelAllBalancers(); + ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; + const auto response_state = + balancers_[0]->ads_service()->lds_response_state(); + EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::NACKED); + EXPECT_THAT(response_state.error_message, + ::testing::HasSubstr( + "non-terminal filter for config type grpc.testing" + ".client_only_http_filter is the last filter in the chain")); } -// Test that we fail RPCs if there is no router filter. -TEST_P(LdsTest, FailRpcsIfNoHttpRouterFilter) { +// Test that we NACK terminal filters that are not at the end of the list. +TEST_P(LdsTest, NacksTerminalFilterBeforeEndOfList) { SetNextResolutionForLbChannelAllBalancers(); auto listener = default_listener_; HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - http_connection_manager.clear_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - AdsServiceImpl::EdsResourceArgs args({ - {"locality0", CreateEndpointsForBackends()}, - }); - balancers_[0]->ads_service()->SetEdsResource( - BuildEdsResource(args, DefaultEdsServiceName())); - Status status = SendRpc(); - EXPECT_EQ(status.error_code(), StatusCode::UNAVAILABLE); - EXPECT_EQ(status.error_message(), "no xDS HTTP router filter configured"); - // Wait until xDS server sees ACK. - while (balancers_[0]->ads_service()->lds_response_state().state == - AdsServiceImpl::ResponseState::SENT) { - CheckRpcSendFailure(); - } + SetNextResolutionForLbChannelAllBalancers(); + ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = balancers_[0]->ads_service()->lds_response_state(); - EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::ACKED); + EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::NACKED); + EXPECT_THAT( + response_state.error_message, + ::testing::HasSubstr( + "terminal filter for config type envoy.extensions.filters.http" + ".router.v3.Router must be the last filter in the chain")); } // Test that we NACK empty filter names. @@ -3686,19 +3680,21 @@ TEST_P(LdsTest, RejectsEmptyHttpFilterName) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); + filter->Clear(); filter->mutable_typed_config()->PackFrom(Listener()); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = balancers_[0]->ads_service()->lds_response_state(); EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::NACKED); EXPECT_THAT(response_state.error_message, - ::testing::HasSubstr("empty filter name at index 1")); + ::testing::HasSubstr("empty filter name at index 0")); } // Test that we NACK duplicate HTTP filter names. @@ -3709,10 +3705,12 @@ TEST_P(LdsTest, RejectsDuplicateHttpFilterName) { &http_connection_manager); *http_connection_manager.add_http_filters() = http_connection_manager.http_filters(0); + http_connection_manager.mutable_http_filters(0) + ->mutable_typed_config() + ->PackFrom(HTTPFault()); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3728,13 +3726,14 @@ TEST_P(LdsTest, RejectsUnknownHttpFilterType) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); filter->set_name("unknown"); filter->mutable_typed_config()->PackFrom(Listener()); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3751,7 +3750,9 @@ TEST_P(LdsTest, IgnoresOptionalUnknownHttpFilterType) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); filter->set_name("unknown"); filter->mutable_typed_config()->PackFrom(Listener()); filter->set_is_optional(true); @@ -3775,12 +3776,14 @@ TEST_P(LdsTest, RejectsHttpFilterWithoutConfig) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); + filter->Clear(); filter->set_name("unknown"); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3797,7 +3800,10 @@ TEST_P(LdsTest, IgnoresOptionalHttpFilterWithoutConfig) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); + filter->Clear(); filter->set_name("unknown"); filter->set_is_optional(true); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( @@ -3820,15 +3826,16 @@ TEST_P(LdsTest, RejectsUnparseableHttpFilterType) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); filter->set_name("unknown"); filter->mutable_typed_config()->PackFrom(listener); filter->mutable_typed_config()->set_type_url( - "type.googleapis.com/envoy.extensions.filters.http.router.v3.Router"); + "type.googleapis.com/envoy.extensions.filters.http.fault.v3.HTTPFault"); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3838,7 +3845,7 @@ TEST_P(LdsTest, RejectsUnparseableHttpFilterType) { response_state.error_message, ::testing::HasSubstr( "filter config for type " - "envoy.extensions.filters.http.router.v3.Router failed to parse")); + "envoy.extensions.filters.http.fault.v3.HTTPFault failed to parse")); } // Test that we NACK HTTP filters unsupported on client-side. @@ -3847,14 +3854,15 @@ TEST_P(LdsTest, RejectsHttpFiltersNotSupportedOnClients) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); filter->set_name("grpc.testing.server_only_http_filter"); filter->mutable_typed_config()->set_type_url( "grpc.testing.server_only_http_filter"); listener.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); SetListenerAndRouteConfiguration(0, listener, default_route_config_); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); ASSERT_TRUE(WaitForLdsNack()) << "timed out waiting for NACK"; const auto response_state = @@ -3872,7 +3880,9 @@ TEST_P(LdsTest, IgnoresOptionalHttpFiltersNotSupportedOnClients) { HttpConnectionManager http_connection_manager; listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( &http_connection_manager); - auto* filter = http_connection_manager.add_http_filters(); + *http_connection_manager.add_http_filters() = + http_connection_manager.http_filters(0); + auto* filter = http_connection_manager.mutable_http_filters(0); filter->set_name("grpc.testing.server_only_http_filter"); filter->mutable_typed_config()->set_type_url( "grpc.testing.server_only_http_filter"); @@ -3885,7 +3895,6 @@ TEST_P(LdsTest, IgnoresOptionalHttpFiltersNotSupportedOnClients) { }); balancers_[0]->ads_service()->SetEdsResource( BuildEdsResource(args, DefaultEdsServiceName())); - SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); WaitForBackend(0); EXPECT_EQ(balancers_[0]->ads_service()->lds_response_state().state, @@ -8581,6 +8590,10 @@ TEST_P(XdsEnabledServerTest, HttpFilterNotSupportedOnServer) { http_filter->set_name("grpc.testing.client_only_http_filter"); http_filter->mutable_typed_config()->set_type_url( "grpc.testing.client_only_http_filter"); + http_filter = http_connection_manager.add_http_filters(); + http_filter->set_name("router"); + http_filter->mutable_typed_config()->PackFrom( + envoy::extensions::filters::http::router::v3::Router()); listener.add_filter_chains()->add_filters()->mutable_typed_config()->PackFrom( http_connection_manager); balancers_[0]->ads_service()->SetLdsResource(listener);