From 5c9a971904ad3a8587d356b43a07c52111e91efc Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 18 May 2022 15:03:01 -0700 Subject: [PATCH] xds: use federation env var to guard new-style resource name parsing (#29725) --- src/core/ext/xds/xds_bootstrap.cc | 4 +- src/core/ext/xds/xds_bootstrap.h | 2 + src/core/ext/xds/xds_client.cc | 6 ++- src/core/ext/xds/xds_client.h | 3 +- test/cpp/end2end/xds/xds_core_end2end_test.cc | 46 +++++++++++++++++++ 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/core/ext/xds/xds_bootstrap.cc b/src/core/ext/xds/xds_bootstrap.cc index a2d8e36cec2..b61a58e1eb3 100644 --- a/src/core/ext/xds/xds_bootstrap.cc +++ b/src/core/ext/xds/xds_bootstrap.cc @@ -44,8 +44,6 @@ namespace grpc_core { -namespace { - // TODO(donnadionne): check to see if federation is enabled, this will be // removed once federation is fully integrated and enabled by default. bool XdsFederationEnabled() { @@ -56,6 +54,8 @@ bool XdsFederationEnabled() { return parse_succeeded && parsed_value; } +namespace { + grpc_error_handle ParseChannelCreds(const Json::Object& json, size_t idx, XdsBootstrap::XdsServer* server) { std::vector error_list; diff --git a/src/core/ext/xds/xds_bootstrap.h b/src/core/ext/xds/xds_bootstrap.h index 181b3320be4..36f38d99945 100644 --- a/src/core/ext/xds/xds_bootstrap.h +++ b/src/core/ext/xds/xds_bootstrap.h @@ -37,6 +37,8 @@ namespace grpc_core { +bool XdsFederationEnabled(); + class XdsClient; class XdsBootstrap { diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc index 1298a786c72..b2200071eb0 100644 --- a/src/core/ext/xds/xds_client.cc +++ b/src/core/ext/xds/xds_client.cc @@ -802,7 +802,7 @@ void XdsClient::ChannelState::AdsCallState::AdsResponseParser::ParseResource( } // Check the resource name. auto resource_name = - XdsClient::ParseXdsResourceName(result->name, result_.type); + xds_client()->ParseXdsResourceName(result->name, result_.type); if (!resource_name.ok()) { result_.errors.emplace_back(absl::StrCat( "resource index ", idx, ": Cannot parse xDS resource name \"", @@ -1802,6 +1802,7 @@ XdsClient::XdsClient(std::unique_ptr bootstrap, bootstrap_(std::move(bootstrap)), args_(ModifyChannelArgs(args)), request_timeout_(GetRequestTimeout(args)), + xds_federation_enabled_(XdsFederationEnabled()), interested_parties_(grpc_pollset_set_create()), certificate_provider_store_(MakeOrphanable( bootstrap_->certificate_providers())), @@ -1942,6 +1943,7 @@ void XdsClient::CancelResourceWatch(const XdsResourceType* type, // authority_state_map_, so we check both, just to be safe. invalid_watchers_.erase(watcher); // Find authority. + if (!resource_name.ok()) return; auto authority_it = authority_state_map_.find(resource_name->authority); if (authority_it == authority_state_map_.end()) return; AuthorityState& authority_state = authority_it->second; @@ -1994,7 +1996,7 @@ absl::StatusOr XdsClient::ParseXdsResourceName( absl::string_view name, const XdsResourceType* type) { // Old-style names use the empty string for authority. // authority is prefixed with "old:" to indicate that it's an old-style name. - if (!absl::StartsWith(name, "xdstp:")) { + if (!xds_federation_enabled_ || !absl::StartsWith(name, "xdstp:")) { return XdsResourceName{"old:", {std::string(name), {}}}; } // New style name. Parse URI. diff --git a/src/core/ext/xds/xds_client.h b/src/core/ext/xds/xds_client.h index 9ffd3364d11..5fe050cc1b6 100644 --- a/src/core/ext/xds/xds_client.h +++ b/src/core/ext/xds/xds_client.h @@ -286,7 +286,7 @@ class XdsClient : public DualRefCounted { const XdsResourceType* GetResourceTypeLocked(absl::string_view resource_type) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); - static absl::StatusOr ParseXdsResourceName( + absl::StatusOr ParseXdsResourceName( absl::string_view name, const XdsResourceType* type); static std::string ConstructFullXdsResourceName( absl::string_view authority, absl::string_view resource_type, @@ -302,6 +302,7 @@ class XdsClient : public DualRefCounted { std::unique_ptr bootstrap_; grpc_channel_args* args_; const Duration request_timeout_; + const bool xds_federation_enabled_; grpc_pollset_set* interested_parties_; OrphanablePtr certificate_provider_store_; XdsApi api_; diff --git a/test/cpp/end2end/xds/xds_core_end2end_test.cc b/test/cpp/end2end/xds/xds_core_end2end_test.cc index 83fb203ceb1..2150251a0b0 100644 --- a/test/cpp/end2end/xds/xds_core_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_core_end2end_test.cc @@ -1057,6 +1057,52 @@ TEST_P(XdsFederationTest, FederationServer) { WaitForAllBackends(DEBUG_LOCATION); } +// +// XdsFederationDisabledTest +// + +using XdsFederationDisabledTest = XdsEnd2endTest; + +// Runs with RDS so that we know all resource types work properly. +INSTANTIATE_TEST_SUITE_P( + XdsTest, XdsFederationDisabledTest, + ::testing::Values(XdsTestType().set_enable_rds_testing()), + &XdsTestType::Name); + +TEST_P(XdsFederationDisabledTest, FederationDisabledWithNewStyleNames) { + const char* kNewRouteConfigName = + "xdstp://xds.example.com/envoy.config.route.v3.RouteConfiguration/" + "new_route_config_name"; + const char* kNewClusterName = + "xdstp://xds.example.com/envoy.config.cluster.v3.Cluster/" + "cluster_name"; + const char* kNewEdsResourceName = + "xdstp://xds.example.com/envoy.config.endpoint.v3.ClusterLoadAssignment/" + "edsservice_name"; + InitClient(); + CreateAndStartBackends(1); + EdsResourceArgs args({{"locality0", CreateEndpointsForBackends()}}); + balancer_->ads_service()->SetEdsResource( + BuildEdsResource(args, kNewEdsResourceName)); + // New cluster + Cluster new_cluster = default_cluster_; + new_cluster.set_name(kNewClusterName); + new_cluster.mutable_eds_cluster_config()->set_service_name( + kNewEdsResourceName); + balancer_->ads_service()->SetCdsResource(new_cluster); + // New RouteConfig + RouteConfiguration new_route_config = default_route_config_; + new_route_config.set_name(kNewRouteConfigName); + new_route_config.mutable_virtual_hosts(0) + ->mutable_routes(0) + ->mutable_route() + ->set_cluster(kNewClusterName); + SetListenerAndRouteConfiguration(balancer_.get(), default_listener_, + new_route_config); + // Channel should work. + CheckRpcSendOk(DEBUG_LOCATION); +} + // // XdsFederationLoadReportingTest - xDS federation and load reporting //