From d2c2d66a03fcc2a8244361ea3479c60ac81055da Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 11 Mar 2021 17:05:16 -0800 Subject: [PATCH] xds: Ignore HTTP filters if LDS resource is v2. (#25694) --- src/core/ext/xds/xds_api.cc | 165 +++++++++++++++------------ test/cpp/end2end/xds_end2end_test.cc | 72 ++++++++++-- 2 files changed, 153 insertions(+), 84 deletions(-) diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index f9a59aa2a3c..43201b6adce 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -688,8 +688,13 @@ const char* kCdsV2TypeUrl = "type.googleapis.com/envoy.api.v2.Cluster"; const char* kEdsV2TypeUrl = "type.googleapis.com/envoy.api.v2.ClusterLoadAssignment"; -bool IsLds(absl::string_view type_url) { - return type_url == XdsApi::kLdsTypeUrl || type_url == kLdsV2TypeUrl; +bool IsLds(absl::string_view type_url, bool* is_v2 = nullptr) { + if (type_url == XdsApi::kLdsTypeUrl) return true; + if (type_url == kLdsV2TypeUrl) { + if (is_v2 != nullptr) *is_v2 = true; + return true; + } + return false; } bool IsRds(absl::string_view type_url) { @@ -1664,6 +1669,7 @@ grpc_error* HttpConnectionManagerParse( bool is_client, const EncodingContext& context, const envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager* http_connection_manager_proto, + bool is_v2, XdsApi::LdsUpdate::HttpConnectionManager* http_connection_manager) { MaybeLogHttpConnectionManager(context, http_connection_manager_proto); if (XdsTimeoutEnabled()) { @@ -1683,70 +1689,81 @@ grpc_error* HttpConnectionManagerParse( } } // Parse filters. - if ((XdsSecurityEnabled() || XdsFaultInjectionEnabled()) && context.use_v3) { - size_t num_filters = 0; - const auto* http_filters = - envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_http_filters( - http_connection_manager_proto, &num_filters); - std::set names_seen; - for (size_t i = 0; i < num_filters; ++i) { - const auto* http_filter = http_filters[i]; - absl::string_view name = UpbStringToAbsl( - envoy_extensions_filters_network_http_connection_manager_v3_HttpFilter_name( - http_filter)); - if (name.empty()) { - return GRPC_ERROR_CREATE_FROM_COPIED_STRING( - absl::StrCat("empty filter name at index ", i).c_str()); - } - if (names_seen.find(name) != names_seen.end()) { - return GRPC_ERROR_CREATE_FROM_COPIED_STRING( - absl::StrCat("duplicate HTTP filter name: ", name).c_str()); - } - names_seen.insert(name); - const bool is_optional = - envoy_extensions_filters_network_http_connection_manager_v3_HttpFilter_is_optional( - http_filter); - const google_protobuf_Any* any = - envoy_extensions_filters_network_http_connection_manager_v3_HttpFilter_typed_config( - http_filter); - if (any == nullptr) { - if (is_optional) continue; - return GRPC_ERROR_CREATE_FROM_COPIED_STRING( - absl::StrCat("no filter config specified for filter name ", name) - .c_str()); - } - absl::string_view filter_type; - grpc_error* error = ExtractHttpFilterTypeName(context, any, &filter_type); - if (error != GRPC_ERROR_NONE) return error; - const XdsHttpFilterImpl* filter_impl = - XdsHttpFilterRegistry::GetFilterForType(filter_type); - if (filter_impl == nullptr) { - if (is_optional) continue; - return GRPC_ERROR_CREATE_FROM_COPIED_STRING( - absl::StrCat("no filter registered for config type ", filter_type) - .c_str()); - } - if ((is_client && !filter_impl->IsSupportedOnClients()) || - (!is_client && !filter_impl->IsSupportedOnServers())) { - if (is_optional) continue; - return GRPC_ERROR_CREATE_FROM_COPIED_STRING( - absl::StrFormat("Filter %s is not supported on %s", filter_type, - is_client ? "clients" : "servers") - .c_str()); - } - absl::StatusOr filter_config = - filter_impl->GenerateFilterConfig(google_protobuf_Any_value(any), - context.arena); - if (!filter_config.ok()) { - return GRPC_ERROR_CREATE_FROM_COPIED_STRING( - absl::StrCat( - "filter config for type ", filter_type, - " failed to parse: ", filter_config.status().ToString()) - .c_str()); + if (XdsSecurityEnabled() || XdsFaultInjectionEnabled()) { + if (!is_v2) { + size_t num_filters = 0; + const auto* http_filters = + envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_http_filters( + http_connection_manager_proto, &num_filters); + std::set names_seen; + for (size_t i = 0; i < num_filters; ++i) { + const auto* http_filter = http_filters[i]; + absl::string_view name = UpbStringToAbsl( + envoy_extensions_filters_network_http_connection_manager_v3_HttpFilter_name( + http_filter)); + if (name.empty()) { + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat("empty filter name at index ", i).c_str()); + } + if (names_seen.find(name) != names_seen.end()) { + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat("duplicate HTTP filter name: ", name).c_str()); + } + names_seen.insert(name); + const bool is_optional = + envoy_extensions_filters_network_http_connection_manager_v3_HttpFilter_is_optional( + http_filter); + const google_protobuf_Any* any = + envoy_extensions_filters_network_http_connection_manager_v3_HttpFilter_typed_config( + http_filter); + if (any == nullptr) { + if (is_optional) continue; + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat("no filter config specified for filter name ", name) + .c_str()); + } + absl::string_view filter_type; + grpc_error* error = + ExtractHttpFilterTypeName(context, any, &filter_type); + if (error != GRPC_ERROR_NONE) return error; + const XdsHttpFilterImpl* filter_impl = + XdsHttpFilterRegistry::GetFilterForType(filter_type); + if (filter_impl == nullptr) { + if (is_optional) continue; + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat("no filter registered for config type ", filter_type) + .c_str()); + } + if ((is_client && !filter_impl->IsSupportedOnClients()) || + (!is_client && !filter_impl->IsSupportedOnServers())) { + if (is_optional) continue; + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrFormat("Filter %s is not supported on %s", filter_type, + is_client ? "clients" : "servers") + .c_str()); + } + absl::StatusOr filter_config = + filter_impl->GenerateFilterConfig(google_protobuf_Any_value(any), + context.arena); + if (!filter_config.ok()) { + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat( + "filter config for type ", filter_type, + " failed to parse: ", filter_config.status().ToString()) + .c_str()); + } + http_connection_manager->http_filters.emplace_back( + XdsApi::LdsUpdate::HttpConnectionManager::HttpFilter{ + std::string(name), std::move(*filter_config)}); } + } else { + // If using a v2 config, we just hard-code a list containing only the + // router filter without actually looking at the config. This ensures + // that the right thing happens in the xds resolver without having + // to expose whether the resource we received was v2 or v3. http_connection_manager->http_filters.emplace_back( XdsApi::LdsUpdate::HttpConnectionManager::HttpFilter{ - std::string(name), std::move(*filter_config)}); + "router", {kXdsHttpRouterFilterConfigName, Json()}}); } } if (is_client) { @@ -1792,7 +1809,7 @@ grpc_error* HttpConnectionManagerParse( grpc_error* LdsResponseParseClient( const EncodingContext& context, - const envoy_config_listener_v3_ApiListener* api_listener, + const envoy_config_listener_v3_ApiListener* api_listener, bool is_v2, XdsApi::LdsUpdate* lds_update) { lds_update->type = XdsApi::LdsUpdate::ListenerType::kHttpApiListener; const upb_strview encoded_api_listener = google_protobuf_Any_value( @@ -1805,7 +1822,7 @@ grpc_error* LdsResponseParseClient( "Could not parse HttpConnectionManager config from ApiListener"); } return HttpConnectionManagerParse(true /* is_client */, context, - http_connection_manager, + http_connection_manager, is_v2, &lds_update->http_connection_manager); } @@ -1926,7 +1943,7 @@ grpc_error* DownstreamTlsContextParse( grpc_error* FilterChainParse( const EncodingContext& context, - const envoy_config_listener_v3_FilterChain* filter_chain_proto, + const envoy_config_listener_v3_FilterChain* filter_chain_proto, bool is_v2, XdsApi::LdsUpdate::FilterChain* filter_chain) { grpc_error* error = GRPC_ERROR_NONE; auto* filter_chain_match = @@ -1971,7 +1988,7 @@ grpc_error* FilterChainParse( "typed_config"); } error = HttpConnectionManagerParse(false /* is_client */, context, - http_connection_manager, + http_connection_manager, is_v2, &filter_chain->http_connection_manager); if (error != GRPC_ERROR_NONE) return error; // Get the DownstreamTlsContext for the filter chain @@ -2013,7 +2030,7 @@ grpc_error* AddressParse(const envoy_config_core_v3_Address* address_proto, grpc_error* LdsResponseParseServer( const EncodingContext& context, - const envoy_config_listener_v3_Listener* listener, + const envoy_config_listener_v3_Listener* listener, bool is_v2, XdsApi::LdsUpdate* lds_update) { lds_update->type = XdsApi::LdsUpdate::ListenerType::kTcpListener; grpc_error* error = @@ -2035,7 +2052,7 @@ grpc_error* LdsResponseParseServer( lds_update->filter_chains.reserve(size); for (size_t i = 0; i < size; i++) { XdsApi::LdsUpdate::FilterChain filter_chain; - error = FilterChainParse(context, filter_chains[0], &filter_chain); + error = FilterChainParse(context, filter_chains[0], is_v2, &filter_chain); if (error != GRPC_ERROR_NONE) return error; lds_update->filter_chains.push_back(std::move(filter_chain)); } @@ -2043,7 +2060,8 @@ grpc_error* LdsResponseParseServer( envoy_config_listener_v3_Listener_default_filter_chain(listener); if (default_filter_chain != nullptr) { XdsApi::LdsUpdate::FilterChain filter_chain; - error = FilterChainParse(context, default_filter_chain, &filter_chain); + error = + FilterChainParse(context, default_filter_chain, is_v2, &filter_chain); if (error != GRPC_ERROR_NONE) return error; lds_update->default_filter_chain = std::move(filter_chain); } @@ -2068,7 +2086,8 @@ grpc_error* LdsResponseParse( // Check the type_url of the resource. absl::string_view type_url = UpbStringToAbsl(google_protobuf_Any_type_url(resources[i])); - if (!IsLds(type_url)) { + bool is_v2 = false; + if (!IsLds(type_url, &is_v2)) { errors.push_back(GRPC_ERROR_CREATE_FROM_COPIED_STRING( absl::StrCat("resource index ", i, ": Resource is not LDS.") .c_str())); @@ -2125,9 +2144,9 @@ grpc_error* LdsResponseParse( } grpc_error* error = GRPC_ERROR_NONE; if (api_listener != nullptr) { - error = LdsResponseParseClient(context, api_listener, &lds_update); + error = LdsResponseParseClient(context, api_listener, is_v2, &lds_update); } else { - error = LdsResponseParseServer(context, listener, &lds_update); + error = LdsResponseParseServer(context, listener, is_v2, &lds_update); } if (error != GRPC_ERROR_NONE) { errors.push_back(grpc_error_add_child( diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 0f86c544122..80046d9f531 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -1638,10 +1638,12 @@ class XdsEnd2endTest : public ::testing::TestWithParam { // Construct LDS resource. default_listener_.set_name(kServerName); HttpConnectionManager http_connection_manager; - auto* filter = http_connection_manager.add_http_filters(); - filter->set_name("router"); - filter->mutable_typed_config()->PackFrom( - envoy::extensions::filters::http::router::v3::Router()); + if (!GetParam().use_v2()) { + auto* filter = http_connection_manager.add_http_filters(); + filter->set_name("router"); + filter->mutable_typed_config()->PackFrom( + envoy::extensions::filters::http::router::v3::Router()); + } default_listener_.mutable_api_listener()->mutable_api_listener()->PackFrom( http_connection_manager); // Construct RDS resource. @@ -3292,6 +3294,30 @@ TEST_P(LdsTest, HttpFiltersEnabled) { gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION"); } +// Tests that we ignore filters after the router filter. +TEST_P(LdsTest, IgnoresHttpFiltersAfterRouterFilter) { + gpr_setenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION", "true"); + 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(); + 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", GetBackendPorts()}, + }); + balancers_[0]->ads_service()->SetEdsResource( + BuildEdsResource(args, DefaultEdsServiceName())); + WaitForAllBackends(); + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION"); +} + // Test that we fail RPCs if there is no router filter. TEST_P(LdsTest, FailRpcsIfNoHttpRouterFilter) { gpr_setenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION", "true"); @@ -3323,9 +3349,6 @@ TEST_P(LdsTest, FailRpcsIfNoHttpRouterFilter) { gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION"); } -// TODO(lidiz): As part of adding the fault injection filter, add a test -// that we ignore filters after the router filter. - // Test that we NACK empty filter names. TEST_P(LdsTest, RejectsEmptyHttpFilterName) { gpr_setenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION", "true"); @@ -3580,6 +3603,34 @@ TEST_P(LdsTest, IgnoresOptionalHttpFiltersNotSupportedOnClients) { gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION"); } +using LdsV2Test = LdsTest; + +// Tests that we ignore the HTTP filter list in v2. +// TODO(roth): The test framework is not set up to allow us to test +// the server sending v2 resources when the client requests v3, so this +// just tests a pure v2 setup. When we have time, fix this. +TEST_P(LdsV2Test, IgnoresHttpFilters) { + gpr_setenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION", "true"); + 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(); + 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_); + AdsServiceImpl::EdsResourceArgs args({ + {"locality0", GetBackendPorts(0, 1)}, + }); + balancers_[0]->ads_service()->SetEdsResource( + BuildEdsResource(args, DefaultEdsServiceName())); + SetNextResolutionForLbChannelAllBalancers(); + CheckRpcSendOk(); + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION"); +} + using LdsRdsTest = BasicTest; // Tests that LDS client should send an ACK upon correct LDS response (with @@ -6338,10 +6389,6 @@ TEST_P(LdsRdsTest, RejectsUnparseableHttpFilterTypeInClusterWeight) { gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION"); } -// TODO(lidiz): As part of adding the fault injection filter, add tests -// for overriding filter configs in the typed_per_filter_config fields in -// each of VirtualHost, Route, and ClusterWeight. - using CdsTest = BasicTest; // Tests that CDS client should send an ACK upon correct CDS response. @@ -9963,6 +10010,9 @@ INSTANTIATE_TEST_SUITE_P(XdsTest, SecureNamingTest, // LDS depends on XdsResolver. INSTANTIATE_TEST_SUITE_P(XdsTest, LdsTest, ::testing::Values(TestType()), &TestTypeName); +INSTANTIATE_TEST_SUITE_P(XdsTest, LdsV2Test, + ::testing::Values(TestType().set_use_v2()), + &TestTypeName); // LDS/RDS commmon tests depend on XdsResolver. INSTANTIATE_TEST_SUITE_P(