From 73fa1384329c8d82d9604ccbf12127c89229e1dc Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Wed, 11 Jan 2023 10:29:38 -0800 Subject: [PATCH] xds_override_host LB: move XdsOverrideHostLbConfig to `internal` namespace (#32061) --- build_autogenerated.yaml | 1 + gRPC-C++.podspec | 2 + gRPC-Core.podspec | 2 + grpc.gemspec | 1 + package.xml | 1 + src/core/BUILD | 3 + .../lb_policy/xds/xds_override_host.cc | 96 ++++++++----------- .../lb_policy/xds/xds_override_host.h | 60 ++++++++++++ ...xds_override_host_lb_config_parser_test.cc | 46 +++++++++ tools/doxygen/Doxyfile.c++.internal | 1 + tools/doxygen/Doxyfile.core.internal | 1 + 11 files changed, 156 insertions(+), 58 deletions(-) create mode 100644 src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index a5a6c78b9be..8ce0d179b51 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -353,6 +353,7 @@ libs: - src/core/ext/filters/client_channel/lb_policy/subchannel_list.h - src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h - src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h + - src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h - src/core/ext/filters/client_channel/local_subchannel_pool.h - src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h - src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index c9fd9ecfdfb..132dee3180c 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -265,6 +265,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/subchannel_list.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h', 'src/core/ext/filters/client_channel/local_subchannel_pool.h', 'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h', 'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h', @@ -1194,6 +1195,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/subchannel_list.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h', 'src/core/ext/filters/client_channel/local_subchannel_pool.h', 'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h', 'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index f1ddc814b1d..6eda95ee7dd 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -270,6 +270,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc', 'src/core/ext/filters/client_channel/local_subchannel_pool.cc', 'src/core/ext/filters/client_channel/local_subchannel_pool.h', @@ -1883,6 +1884,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/subchannel_list.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.h', 'src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h', 'src/core/ext/filters/client_channel/local_subchannel_pool.h', 'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h', 'src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h', diff --git a/grpc.gemspec b/grpc.gemspec index 93e24b8e1ee..09e8474ccaf 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -181,6 +181,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc ) + s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc ) s.files += %w( src/core/ext/filters/client_channel/local_subchannel_pool.cc ) s.files += %w( src/core/ext/filters/client_channel/local_subchannel_pool.h ) diff --git a/package.xml b/package.xml index 25ac6867a93..8715c0d645c 100644 --- a/package.xml +++ b/package.xml @@ -163,6 +163,7 @@ + diff --git a/src/core/BUILD b/src/core/BUILD index 4271e525424..012e302a6d6 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -4444,6 +4444,9 @@ grpc_cc_library( srcs = [ "ext/filters/client_channel/lb_policy/xds/xds_override_host.cc", ], + hdrs = [ + "ext/filters/client_channel/lb_policy/xds/xds_override_host.h", + ], external_deps = [ "absl/base:core_headers", "absl/status", diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc index 1f59cc58e35..c03a4a93aa6 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc @@ -16,6 +16,8 @@ #include +#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h" + #include #include #include @@ -72,40 +74,13 @@ namespace { // // xds_override_host LB policy // - -constexpr absl::string_view kXdsOverrideHost = "xds_override_host_experimental"; - -// Config for stateful session LB policy. -class XdsOverrideHostLbConfig : public LoadBalancingPolicy::Config { - public: - XdsOverrideHostLbConfig() = default; - - XdsOverrideHostLbConfig(const XdsOverrideHostLbConfig&) = delete; - XdsOverrideHostLbConfig& operator=(const XdsOverrideHostLbConfig&) = delete; - - XdsOverrideHostLbConfig(XdsOverrideHostLbConfig&& other) = delete; - XdsOverrideHostLbConfig& operator=(XdsOverrideHostLbConfig&& other) = delete; - - absl::string_view name() const override { return kXdsOverrideHost; } - - RefCountedPtr child_config() const { - return child_config_; - } - - static const JsonLoaderInterface* JsonLoader(const JsonArgs&); - void JsonPostLoad(const Json& json, const JsonArgs&, - ValidationErrors* errors); - - private: - RefCountedPtr child_config_; -}; - -// xDS Cluster Impl LB policy. class XdsOverrideHostLb : public LoadBalancingPolicy { public: explicit XdsOverrideHostLb(Args args); - absl::string_view name() const override { return kXdsOverrideHost; } + absl::string_view name() const override { + return XdsOverrideHostLbConfig::Name(); + } absl::Status UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -610,33 +585,6 @@ void XdsOverrideHostLb::SubchannelWrapper::CancelConnectivityStateWatch( // // factory // -const JsonLoaderInterface* XdsOverrideHostLbConfig::JsonLoader( - const JsonArgs&) { - static const auto kJsonLoader = - JsonObjectLoader() - // Child policy config is parsed in JsonPostLoad - .Finish(); - return kJsonLoader; -} - -void XdsOverrideHostLbConfig::JsonPostLoad(const Json& json, const JsonArgs&, - ValidationErrors* errors) { - ValidationErrors::ScopedField field(errors, ".childPolicy"); - auto it = json.object_value().find("childPolicy"); - if (it == json.object_value().end()) { - errors->AddError("field not present"); - } else { - auto child_policy_config = - CoreConfiguration::Get().lb_policy_registry().ParseLoadBalancingConfig( - it->second); - if (!child_policy_config.ok()) { - errors->AddError(child_policy_config.status().message()); - } else { - child_config_ = std::move(*child_policy_config); - } - } -} - class XdsOverrideHostLbFactory : public LoadBalancingPolicyFactory { public: OrphanablePtr CreateLoadBalancingPolicy( @@ -644,7 +592,9 @@ class XdsOverrideHostLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kXdsOverrideHost; } + absl::string_view name() const override { + return XdsOverrideHostLbConfig::Name(); + } absl::StatusOr> ParseLoadBalancingConfig(const Json& json) const override { @@ -668,4 +618,34 @@ void RegisterXdsOverrideHostLbPolicy(CoreConfiguration::Builder* builder) { builder->lb_policy_registry()->RegisterLoadBalancingPolicyFactory( std::make_unique()); } + +// XdsOverrideHostLbConfig + +const JsonLoaderInterface* XdsOverrideHostLbConfig::JsonLoader( + const JsonArgs&) { + static const auto kJsonLoader = + JsonObjectLoader() + // Child policy config is parsed in JsonPostLoad + .Finish(); + return kJsonLoader; +} + +void XdsOverrideHostLbConfig::JsonPostLoad(const Json& json, const JsonArgs&, + ValidationErrors* errors) { + ValidationErrors::ScopedField field(errors, ".childPolicy"); + auto it = json.object_value().find("childPolicy"); + if (it == json.object_value().end()) { + errors->AddError("field not present"); + } else { + auto child_policy_config = + CoreConfiguration::Get().lb_policy_registry().ParseLoadBalancingConfig( + it->second); + if (!child_policy_config.ok()) { + errors->AddError(child_policy_config.status().message()); + } else { + child_config_ = std::move(*child_policy_config); + } + } +} + } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h new file mode 100644 index 00000000000..568f03ccbbb --- /dev/null +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h @@ -0,0 +1,60 @@ +// +// Copyright 2023 gRPC authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#ifndef GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_OVERRIDE_HOST_H +#define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_OVERRIDE_HOST_H + +#include + +#include "absl/strings/string_view.h" + +#include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/validation_errors.h" +#include "src/core/lib/json/json.h" +#include "src/core/lib/json/json_args.h" +#include "src/core/lib/json/json_object_loader.h" +#include "src/core/lib/load_balancing/lb_policy.h" + +namespace grpc_core { + +// Config for stateful session LB policy. +class XdsOverrideHostLbConfig : public LoadBalancingPolicy::Config { + public: + XdsOverrideHostLbConfig() = default; + + XdsOverrideHostLbConfig(const XdsOverrideHostLbConfig&) = delete; + XdsOverrideHostLbConfig& operator=(const XdsOverrideHostLbConfig&) = delete; + + XdsOverrideHostLbConfig(XdsOverrideHostLbConfig&& other) = delete; + XdsOverrideHostLbConfig& operator=(XdsOverrideHostLbConfig&& other) = delete; + + static absl::string_view Name() { return "xds_override_host_experimental"; } + + absl::string_view name() const override { return Name(); } + + RefCountedPtr child_config() const { + return child_config_; + } + + static const JsonLoaderInterface* JsonLoader(const JsonArgs&); + void JsonPostLoad(const Json& json, const JsonArgs&, + ValidationErrors* errors); + + private: + RefCountedPtr child_config_; +}; +} // namespace grpc_core +#endif // GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_OVERRIDE_HOST_H diff --git a/test/core/client_channel/lb_policy/xds_override_host_lb_config_parser_test.cc b/test/core/client_channel/lb_policy/xds_override_host_lb_config_parser_test.cc index 3487f89369a..e3f7cbe19b3 100644 --- a/test/core/client_channel/lb_policy/xds_override_host_lb_config_parser_test.cc +++ b/test/core/client_channel/lb_policy/xds_override_host_lb_config_parser_test.cc @@ -20,6 +20,8 @@ #include +#include "src/core/ext/filters/client_channel/client_channel_service_config.h" +#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/service_config/service_config.h" @@ -30,6 +32,9 @@ namespace grpc_core { namespace testing { namespace { +using internal::ClientChannelGlobalParsedConfig; +using internal::ClientChannelServiceConfigParser; + TEST(XdsOverrideHostConfigParsingTest, ValidConfig) { const char* service_config_json = "{\n" @@ -45,6 +50,47 @@ TEST(XdsOverrideHostConfigParsingTest, ValidConfig) { ServiceConfigImpl::Create(ChannelArgs(), service_config_json); ASSERT_TRUE(service_config.ok()); EXPECT_NE(*service_config, nullptr); + auto global_config = static_cast( + (*service_config) + ->GetGlobalParsedConfig( + ClientChannelServiceConfigParser::ParserIndex())); + ASSERT_NE(global_config, nullptr); + auto lb_config = global_config->parsed_lb_config(); + ASSERT_NE(lb_config, nullptr); + ASSERT_EQ(lb_config->name(), XdsOverrideHostLbConfig::Name()); + auto override_host_lb_config = + static_cast>(lb_config); + ASSERT_NE(override_host_lb_config->child_config(), nullptr); + ASSERT_EQ(override_host_lb_config->child_config()->name(), "grpclb"); +} + +TEST(XdsOverrideHostConfigParsingTest, ValidConfigWithRR) { + const char* service_config_json = + "{\n" + " \"loadBalancingConfig\":[{\n" + " \"xds_override_host_experimental\":{\n" + " \"childPolicy\":[\n" + " {\"round_robin\":{}}\n" + " ]\n" + " }\n" + " }]\n" + "}\n"; + auto service_config = + ServiceConfigImpl::Create(ChannelArgs(), service_config_json); + ASSERT_TRUE(service_config.ok()); + EXPECT_NE(*service_config, nullptr); + auto global_config = static_cast( + (*service_config) + ->GetGlobalParsedConfig( + ClientChannelServiceConfigParser::ParserIndex())); + ASSERT_NE(global_config, nullptr); + auto lb_config = global_config->parsed_lb_config(); + ASSERT_NE(lb_config, nullptr); + ASSERT_EQ(lb_config->name(), XdsOverrideHostLbConfig::Name()); + auto override_host_lb_config = + static_cast>(lb_config); + ASSERT_NE(override_host_lb_config->child_config(), nullptr); + ASSERT_EQ(override_host_lb_config->child_config()->name(), "round_robin"); } TEST(XdsOverrideHostConfigParsingTest, ReportsMissingChildPolicyField) { diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index b1624c9138b..2d2d9e81874 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1137,6 +1137,7 @@ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc \ +src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h \ src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc \ src/core/ext/filters/client_channel/local_subchannel_pool.cc \ src/core/ext/filters/client_channel/local_subchannel_pool.h \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index ccb66e9aab5..1e53748aa1b 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -944,6 +944,7 @@ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.cc \ +src/core/ext/filters/client_channel/lb_policy/xds/xds_override_host.h \ src/core/ext/filters/client_channel/lb_policy/xds/xds_wrr_locality.cc \ src/core/ext/filters/client_channel/local_subchannel_pool.cc \ src/core/ext/filters/client_channel/local_subchannel_pool.h \