From 156d743c3717130f0adcf2daf82f8cf45744e549 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 25 Apr 2019 16:38:10 -0700 Subject: [PATCH] Reviewer comments --- BUILD | 2 - BUILD.gn | 2 - CMakeLists.txt | 6 - Makefile | 6 - build.yaml | 2 - config.m4 | 1 - config.w32 | 1 - gRPC-C++.podspec | 1 - gRPC-Core.podspec | 3 - grpc.gemspec | 2 - grpc.gyp | 4 - package.xml | 2 - .../filters/client_channel/client_channel.cc | 26 +- .../client_channel/client_channel_plugin.cc | 8 +- .../health/health_check_parser.cc | 84 ----- .../health/health_check_parser.h | 3 +- .../client_channel/lb_policy/grpclb/grpclb.cc | 39 ++- .../client_channel/lb_policy/xds/xds.cc | 32 +- .../client_channel/lb_policy_registry.cc | 34 +- .../client_channel/lb_policy_registry.h | 9 +- .../client_channel/resolver_result_parsing.cc | 312 ++++++++++-------- .../client_channel/resolver_result_parsing.h | 33 +- .../client_channel/resolving_lb_policy.cc | 2 +- .../client_channel/resolving_lb_policy.h | 2 +- .../filters/client_channel/service_config.cc | 8 +- .../filters/client_channel/service_config.h | 52 ++- .../message_size/message_size_filter.cc | 22 +- src/core/lib/gprpp/optional.h | 2 +- src/core/lib/iomgr/error.h | 19 ++ src/python/grpcio/grpc_core_dependencies.py | 1 - .../client_channel/service_config_test.cc | 116 +++---- tools/doxygen/Doxyfile.core.internal | 2 - .../generated/sources_and_headers.json | 3 - 33 files changed, 371 insertions(+), 470 deletions(-) delete mode 100644 src/core/ext/filters/client_channel/health/health_check_parser.cc diff --git a/BUILD b/BUILD index c9ff663201e..1735961eb61 100644 --- a/BUILD +++ b/BUILD @@ -1082,7 +1082,6 @@ grpc_cc_library( "src/core/ext/filters/client_channel/connector.cc", "src/core/ext/filters/client_channel/global_subchannel_pool.cc", "src/core/ext/filters/client_channel/health/health_check_client.cc", - "src/core/ext/filters/client_channel/health/health_check_parser.cc", "src/core/ext/filters/client_channel/http_connect_handshaker.cc", "src/core/ext/filters/client_channel/http_proxy.cc", "src/core/ext/filters/client_channel/lb_policy.cc", @@ -1109,7 +1108,6 @@ grpc_cc_library( "src/core/ext/filters/client_channel/connector.h", "src/core/ext/filters/client_channel/global_subchannel_pool.h", "src/core/ext/filters/client_channel/health/health_check_client.h", - "src/core/ext/filters/client_channel/health/health_check_parser.h", "src/core/ext/filters/client_channel/http_connect_handshaker.h", "src/core/ext/filters/client_channel/http_proxy.h", "src/core/ext/filters/client_channel/lb_policy.h", diff --git a/BUILD.gn b/BUILD.gn index 4452bfa2cae..10b0f1a7fdd 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -260,8 +260,6 @@ config("grpc_config") { "src/core/ext/filters/client_channel/global_subchannel_pool.h", "src/core/ext/filters/client_channel/health/health_check_client.cc", "src/core/ext/filters/client_channel/health/health_check_client.h", - "src/core/ext/filters/client_channel/health/health_check_parser.cc", - "src/core/ext/filters/client_channel/health/health_check_parser.h", "src/core/ext/filters/client_channel/http_connect_handshaker.cc", "src/core/ext/filters/client_channel/http_connect_handshaker.h", "src/core/ext/filters/client_channel/http_proxy.cc", diff --git a/CMakeLists.txt b/CMakeLists.txt index 15a8697c9a0..0edd7fd54ee 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1235,7 +1235,6 @@ add_library(grpc src/core/ext/filters/client_channel/connector.cc src/core/ext/filters/client_channel/global_subchannel_pool.cc src/core/ext/filters/client_channel/health/health_check_client.cc - src/core/ext/filters/client_channel/health/health_check_parser.cc src/core/ext/filters/client_channel/http_connect_handshaker.cc src/core/ext/filters/client_channel/http_proxy.cc src/core/ext/filters/client_channel/lb_policy.cc @@ -1591,7 +1590,6 @@ add_library(grpc_cronet src/core/ext/filters/client_channel/connector.cc src/core/ext/filters/client_channel/global_subchannel_pool.cc src/core/ext/filters/client_channel/health/health_check_client.cc - src/core/ext/filters/client_channel/health/health_check_parser.cc src/core/ext/filters/client_channel/http_connect_handshaker.cc src/core/ext/filters/client_channel/http_proxy.cc src/core/ext/filters/client_channel/lb_policy.cc @@ -1972,7 +1970,6 @@ add_library(grpc_test_util src/core/ext/filters/client_channel/connector.cc src/core/ext/filters/client_channel/global_subchannel_pool.cc src/core/ext/filters/client_channel/health/health_check_client.cc - src/core/ext/filters/client_channel/health/health_check_parser.cc src/core/ext/filters/client_channel/http_connect_handshaker.cc src/core/ext/filters/client_channel/http_proxy.cc src/core/ext/filters/client_channel/lb_policy.cc @@ -2298,7 +2295,6 @@ add_library(grpc_test_util_unsecure src/core/ext/filters/client_channel/connector.cc src/core/ext/filters/client_channel/global_subchannel_pool.cc src/core/ext/filters/client_channel/health/health_check_client.cc - src/core/ext/filters/client_channel/health/health_check_parser.cc src/core/ext/filters/client_channel/http_connect_handshaker.cc src/core/ext/filters/client_channel/http_proxy.cc src/core/ext/filters/client_channel/lb_policy.cc @@ -2635,7 +2631,6 @@ add_library(grpc_unsecure src/core/ext/filters/client_channel/connector.cc src/core/ext/filters/client_channel/global_subchannel_pool.cc src/core/ext/filters/client_channel/health/health_check_client.cc - src/core/ext/filters/client_channel/health/health_check_parser.cc src/core/ext/filters/client_channel/http_connect_handshaker.cc src/core/ext/filters/client_channel/http_proxy.cc src/core/ext/filters/client_channel/lb_policy.cc @@ -3508,7 +3503,6 @@ add_library(grpc++_cronet src/core/ext/filters/client_channel/connector.cc src/core/ext/filters/client_channel/global_subchannel_pool.cc src/core/ext/filters/client_channel/health/health_check_client.cc - src/core/ext/filters/client_channel/health/health_check_parser.cc src/core/ext/filters/client_channel/http_connect_handshaker.cc src/core/ext/filters/client_channel/http_proxy.cc src/core/ext/filters/client_channel/lb_policy.cc diff --git a/Makefile b/Makefile index 7a80e4c0731..57530500b15 100644 --- a/Makefile +++ b/Makefile @@ -3695,7 +3695,6 @@ LIBGRPC_SRC = \ src/core/ext/filters/client_channel/connector.cc \ src/core/ext/filters/client_channel/global_subchannel_pool.cc \ src/core/ext/filters/client_channel/health/health_check_client.cc \ - src/core/ext/filters/client_channel/health/health_check_parser.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/lb_policy.cc \ @@ -4045,7 +4044,6 @@ LIBGRPC_CRONET_SRC = \ src/core/ext/filters/client_channel/connector.cc \ src/core/ext/filters/client_channel/global_subchannel_pool.cc \ src/core/ext/filters/client_channel/health/health_check_client.cc \ - src/core/ext/filters/client_channel/health/health_check_parser.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/lb_policy.cc \ @@ -4419,7 +4417,6 @@ LIBGRPC_TEST_UTIL_SRC = \ src/core/ext/filters/client_channel/connector.cc \ src/core/ext/filters/client_channel/global_subchannel_pool.cc \ src/core/ext/filters/client_channel/health/health_check_client.cc \ - src/core/ext/filters/client_channel/health/health_check_parser.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/lb_policy.cc \ @@ -4732,7 +4729,6 @@ LIBGRPC_TEST_UTIL_UNSECURE_SRC = \ src/core/ext/filters/client_channel/connector.cc \ src/core/ext/filters/client_channel/global_subchannel_pool.cc \ src/core/ext/filters/client_channel/health/health_check_client.cc \ - src/core/ext/filters/client_channel/health/health_check_parser.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/lb_policy.cc \ @@ -5043,7 +5039,6 @@ LIBGRPC_UNSECURE_SRC = \ src/core/ext/filters/client_channel/connector.cc \ src/core/ext/filters/client_channel/global_subchannel_pool.cc \ src/core/ext/filters/client_channel/health/health_check_client.cc \ - src/core/ext/filters/client_channel/health/health_check_parser.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/lb_policy.cc \ @@ -5892,7 +5887,6 @@ LIBGRPC++_CRONET_SRC = \ src/core/ext/filters/client_channel/connector.cc \ src/core/ext/filters/client_channel/global_subchannel_pool.cc \ src/core/ext/filters/client_channel/health/health_check_client.cc \ - src/core/ext/filters/client_channel/health/health_check_parser.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/lb_policy.cc \ diff --git a/build.yaml b/build.yaml index 53fd95f5c06..80519946295 100644 --- a/build.yaml +++ b/build.yaml @@ -576,7 +576,6 @@ filegroups: - src/core/ext/filters/client_channel/connector.h - src/core/ext/filters/client_channel/global_subchannel_pool.h - src/core/ext/filters/client_channel/health/health_check_client.h - - src/core/ext/filters/client_channel/health/health_check_parser.h - src/core/ext/filters/client_channel/http_connect_handshaker.h - src/core/ext/filters/client_channel/http_proxy.h - src/core/ext/filters/client_channel/lb_policy.h @@ -606,7 +605,6 @@ filegroups: - src/core/ext/filters/client_channel/connector.cc - src/core/ext/filters/client_channel/global_subchannel_pool.cc - src/core/ext/filters/client_channel/health/health_check_client.cc - - src/core/ext/filters/client_channel/health/health_check_parser.cc - src/core/ext/filters/client_channel/http_connect_handshaker.cc - src/core/ext/filters/client_channel/http_proxy.cc - src/core/ext/filters/client_channel/lb_policy.cc diff --git a/config.m4 b/config.m4 index e7d3a5daf4e..ab2a717cbcd 100644 --- a/config.m4 +++ b/config.m4 @@ -348,7 +348,6 @@ if test "$PHP_GRPC" != "no"; then src/core/ext/filters/client_channel/connector.cc \ src/core/ext/filters/client_channel/global_subchannel_pool.cc \ src/core/ext/filters/client_channel/health/health_check_client.cc \ - src/core/ext/filters/client_channel/health/health_check_parser.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_proxy.cc \ src/core/ext/filters/client_channel/lb_policy.cc \ diff --git a/config.w32 b/config.w32 index 8a6529faae2..02eb773ebb8 100644 --- a/config.w32 +++ b/config.w32 @@ -323,7 +323,6 @@ if (PHP_GRPC != "no") { "src\\core\\ext\\filters\\client_channel\\connector.cc " + "src\\core\\ext\\filters\\client_channel\\global_subchannel_pool.cc " + "src\\core\\ext\\filters\\client_channel\\health\\health_check_client.cc " + - "src\\core\\ext\\filters\\client_channel\\health\\health_check_parser.cc " + "src\\core\\ext\\filters\\client_channel\\http_connect_handshaker.cc " + "src\\core\\ext\\filters\\client_channel\\http_proxy.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 7ca1477a16b..de1bb0ae57f 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -367,7 +367,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/connector.h', 'src/core/ext/filters/client_channel/global_subchannel_pool.h', 'src/core/ext/filters/client_channel/health/health_check_client.h', - 'src/core/ext/filters/client_channel/health/health_check_parser.h', 'src/core/ext/filters/client_channel/http_connect_handshaker.h', 'src/core/ext/filters/client_channel/http_proxy.h', 'src/core/ext/filters/client_channel/lb_policy.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 13c2a2cf9d0..5cf45c63cef 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -347,7 +347,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/connector.h', 'src/core/ext/filters/client_channel/global_subchannel_pool.h', 'src/core/ext/filters/client_channel/health/health_check_client.h', - 'src/core/ext/filters/client_channel/health/health_check_parser.h', 'src/core/ext/filters/client_channel/http_connect_handshaker.h', 'src/core/ext/filters/client_channel/http_proxy.h', 'src/core/ext/filters/client_channel/lb_policy.h', @@ -797,7 +796,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/connector.cc', 'src/core/ext/filters/client_channel/global_subchannel_pool.cc', 'src/core/ext/filters/client_channel/health/health_check_client.cc', - 'src/core/ext/filters/client_channel/health/health_check_parser.cc', 'src/core/ext/filters/client_channel/http_connect_handshaker.cc', 'src/core/ext/filters/client_channel/http_proxy.cc', 'src/core/ext/filters/client_channel/lb_policy.cc', @@ -991,7 +989,6 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/connector.h', 'src/core/ext/filters/client_channel/global_subchannel_pool.h', 'src/core/ext/filters/client_channel/health/health_check_client.h', - 'src/core/ext/filters/client_channel/health/health_check_parser.h', 'src/core/ext/filters/client_channel/http_connect_handshaker.h', 'src/core/ext/filters/client_channel/http_proxy.h', 'src/core/ext/filters/client_channel/lb_policy.h', diff --git a/grpc.gemspec b/grpc.gemspec index bca4674c761..15c26f11569 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -281,7 +281,6 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/filters/client_channel/connector.h ) s.files += %w( src/core/ext/filters/client_channel/global_subchannel_pool.h ) s.files += %w( src/core/ext/filters/client_channel/health/health_check_client.h ) - s.files += %w( src/core/ext/filters/client_channel/health/health_check_parser.h ) s.files += %w( src/core/ext/filters/client_channel/http_connect_handshaker.h ) s.files += %w( src/core/ext/filters/client_channel/http_proxy.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy.h ) @@ -734,7 +733,6 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/filters/client_channel/connector.cc ) s.files += %w( src/core/ext/filters/client_channel/global_subchannel_pool.cc ) s.files += %w( src/core/ext/filters/client_channel/health/health_check_client.cc ) - s.files += %w( src/core/ext/filters/client_channel/health/health_check_parser.cc ) s.files += %w( src/core/ext/filters/client_channel/http_connect_handshaker.cc ) s.files += %w( src/core/ext/filters/client_channel/http_proxy.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy.cc ) diff --git a/grpc.gyp b/grpc.gyp index d4db059dc0a..40dc88d7474 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -530,7 +530,6 @@ 'src/core/ext/filters/client_channel/connector.cc', 'src/core/ext/filters/client_channel/global_subchannel_pool.cc', 'src/core/ext/filters/client_channel/health/health_check_client.cc', - 'src/core/ext/filters/client_channel/health/health_check_parser.cc', 'src/core/ext/filters/client_channel/http_connect_handshaker.cc', 'src/core/ext/filters/client_channel/http_proxy.cc', 'src/core/ext/filters/client_channel/lb_policy.cc', @@ -795,7 +794,6 @@ 'src/core/ext/filters/client_channel/connector.cc', 'src/core/ext/filters/client_channel/global_subchannel_pool.cc', 'src/core/ext/filters/client_channel/health/health_check_client.cc', - 'src/core/ext/filters/client_channel/health/health_check_parser.cc', 'src/core/ext/filters/client_channel/http_connect_handshaker.cc', 'src/core/ext/filters/client_channel/http_proxy.cc', 'src/core/ext/filters/client_channel/lb_policy.cc', @@ -1041,7 +1039,6 @@ 'src/core/ext/filters/client_channel/connector.cc', 'src/core/ext/filters/client_channel/global_subchannel_pool.cc', 'src/core/ext/filters/client_channel/health/health_check_client.cc', - 'src/core/ext/filters/client_channel/health/health_check_parser.cc', 'src/core/ext/filters/client_channel/http_connect_handshaker.cc', 'src/core/ext/filters/client_channel/http_proxy.cc', 'src/core/ext/filters/client_channel/lb_policy.cc', @@ -1298,7 +1295,6 @@ 'src/core/ext/filters/client_channel/connector.cc', 'src/core/ext/filters/client_channel/global_subchannel_pool.cc', 'src/core/ext/filters/client_channel/health/health_check_client.cc', - 'src/core/ext/filters/client_channel/health/health_check_parser.cc', 'src/core/ext/filters/client_channel/http_connect_handshaker.cc', 'src/core/ext/filters/client_channel/http_proxy.cc', 'src/core/ext/filters/client_channel/lb_policy.cc', diff --git a/package.xml b/package.xml index 3af95c9a727..c3f4e17dd12 100644 --- a/package.xml +++ b/package.xml @@ -286,7 +286,6 @@ - @@ -739,7 +738,6 @@ - diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index f0972ef4c70..f0bc3417a2b 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -240,6 +240,7 @@ class ChannelData { const size_t per_rpc_retry_buffer_size_; grpc_channel_stack* owning_stack_; ClientChannelFactory* client_channel_factory_; + UniquePtr server_name_; // Initialized shortly after construction. channelz::ClientChannelNode* channelz_node_ = nullptr; @@ -263,7 +264,7 @@ class ChannelData { OrphanablePtr resolving_lb_policy_; grpc_connectivity_state_tracker state_tracker_; ExternalConnectivityWatcher::WatcherList external_connectivity_watcher_list_; - const char* health_check_service_name_ = nullptr; + UniquePtr health_check_service_name_; // // Fields accessed from both data plane and control plane combiners. @@ -762,12 +763,11 @@ class ChannelData::ConnectivityStateAndPickerSetter { class ChannelData::ServiceConfigSetter { public: ServiceConfigSetter( - ChannelData* chand, UniquePtr server_name, + ChannelData* chand, Optional retry_throttle_data, RefCountedPtr service_config) : chand_(chand), - server_name_(std::move(server_name)), retry_throttle_data_(retry_throttle_data), service_config_(std::move(service_config)) { GRPC_CHANNEL_STACK_REF(chand->owning_stack_, "ServiceConfigSetter"); @@ -785,7 +785,7 @@ class ChannelData::ServiceConfigSetter { if (self->retry_throttle_data_.has_value()) { chand->retry_throttle_data_ = internal::ServerRetryThrottleMap::GetDataForServer( - self->server_name_.get(), + chand->server_name_.get(), self->retry_throttle_data_.value().max_milli_tokens, self->retry_throttle_data_.value().milli_token_ratio); } @@ -803,7 +803,6 @@ class ChannelData::ServiceConfigSetter { } ChannelData* chand_; - UniquePtr server_name_; Optional retry_throttle_data_; RefCountedPtr service_config_; @@ -948,7 +947,7 @@ class ChannelData::ClientChannelControlHelper if (chand_->health_check_service_name_ != nullptr) { args_to_add[0] = grpc_channel_arg_string_create( const_cast("grpc.temp.health_check"), - const_cast(chand_->health_check_service_name_)); + const_cast(chand_->health_check_service_name_.get())); num_args_to_add++; } args_to_add[num_args_to_add++] = SubchannelPoolInterface::CreateChannelArg( @@ -1067,6 +1066,10 @@ ChannelData::ChannelData(grpc_channel_element_args* args, grpc_error** error) "filter"); return; } + grpc_uri* uri = grpc_uri_parse(server_uri, true); + GPR_ASSERT(uri->path[0] != '\0'); + server_name_.reset( + gpr_strdup(uri->path[0] == '/' ? uri->path + 1 : uri->path)); char* proxy_name = nullptr; grpc_channel_args* new_args = nullptr; grpc_proxy_mappers_map_name(server_uri, args->channel_args, &proxy_name, @@ -1135,11 +1138,12 @@ bool ChannelData::ProcessResolverResultLocked( gpr_log(GPR_INFO, "chand=%p: resolver returned service config: \"%s\"", chand, service_config_json); } + chand->health_check_service_name_.reset( + gpr_strdup(resolver_result.health_check_service_name())); // Create service config setter to update channel state in the data // plane combiner. Destroys itself when done. - New( - chand, UniquePtr(gpr_strdup(resolver_result.server_name())), - resolver_result.retry_throttle_data(), resolver_result.service_config()); + New(chand, resolver_result.retry_throttle_data(), + resolver_result.service_config()); // Swap out the data used by GetChannelInfo(). bool service_config_changed; { @@ -1156,8 +1160,6 @@ bool ChannelData::ProcessResolverResultLocked( // Return results. *lb_policy_name = chand->info_lb_policy_name_.get(); *lb_policy_config = resolver_result.lb_policy_config(); - chand->health_check_service_name_ = - resolver_result.health_check_service_name(); return service_config_changed; } @@ -3101,7 +3103,7 @@ void CallData::ApplyServiceConfigToCallLocked(grpc_call_element* elem) { // it. service_config_call_data_ = ServiceConfig::CallData(chand->service_config(), path_); - if (!service_config_call_data_.empty()) { + if (service_config_call_data_.service_config() != nullptr) { call_context_[GRPC_SERVICE_CONFIG_CALL_DATA].value = &service_config_call_data_; method_params_ = static_cast( diff --git a/src/core/ext/filters/client_channel/client_channel_plugin.cc b/src/core/ext/filters/client_channel/client_channel_plugin.cc index 877111b8a38..e564df8be33 100644 --- a/src/core/ext/filters/client_channel/client_channel_plugin.cc +++ b/src/core/ext/filters/client_channel/client_channel_plugin.cc @@ -27,7 +27,6 @@ #include "src/core/ext/filters/client_channel/client_channel.h" #include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/ext/filters/client_channel/global_subchannel_pool.h" -#include "src/core/ext/filters/client_channel/health/health_check_parser.h" #include "src/core/ext/filters/client_channel/http_connect_handshaker.h" #include "src/core/ext/filters/client_channel/http_proxy.h" #include "src/core/ext/filters/client_channel/lb_policy_registry.h" @@ -50,14 +49,9 @@ static bool append_filter(grpc_channel_stack_builder* builder, void* arg) { builder, static_cast(arg), nullptr, nullptr); } -void grpc_service_config_register_parsers() { - grpc_core::internal::ClientChannelServiceConfigParser::Register(); - grpc_core::HealthCheckParser::Register(); -} - void grpc_client_channel_init(void) { grpc_core::ServiceConfig::Init(); - grpc_service_config_register_parsers(); + grpc_core::internal::ClientChannelServiceConfigParser::Register(); grpc_core::LoadBalancingPolicyRegistry::Builder::InitRegistry(); grpc_core::ResolverRegistry::Builder::InitRegistry(); grpc_core::internal::ServerRetryThrottleMap::Init(); diff --git a/src/core/ext/filters/client_channel/health/health_check_parser.cc b/src/core/ext/filters/client_channel/health/health_check_parser.cc deleted file mode 100644 index 7760e22b686..00000000000 --- a/src/core/ext/filters/client_channel/health/health_check_parser.cc +++ /dev/null @@ -1,84 +0,0 @@ -/* - * - * Copyright 2019 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. - * - */ - -#include - -#include "src/core/ext/filters/client_channel/health/health_check_parser.h" - -namespace grpc_core { -namespace { -size_t g_health_check_parser_index; -} - -UniquePtr HealthCheckParser::ParseGlobalParams( - const grpc_json* json, grpc_error** error) { - GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE); - const char* service_name = nullptr; - InlinedVector error_list; - for (grpc_json* field = json->child; field != nullptr; field = field->next) { - if (field->key == nullptr) { - GPR_DEBUG_ASSERT(false); - continue; - } - if (strcmp(field->key, "healthCheckConfig") == 0) { - if (field->type != GRPC_JSON_OBJECT) { - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:healthCheckConfig error:should be of type object"); - return nullptr; - } - for (grpc_json* sub_field = field->child; sub_field != nullptr; - sub_field = sub_field->next) { - if (sub_field->key == nullptr) { - GPR_DEBUG_ASSERT(false); - continue; - } - if (strcmp(sub_field->key, "serviceName") == 0) { - if (service_name != nullptr) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:serviceName error:Duplicate " - "entry")); - continue; - } - if (sub_field->type != GRPC_JSON_STRING) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:serviceName error:should be of type string")); - continue; - } - service_name = sub_field->value; - } - } - } - } - if (error_list.empty()) { - if (service_name == nullptr) return nullptr; - return UniquePtr( - New(service_name)); - } - *error = ServiceConfig::CreateErrorFromVector("field:healthCheckConfig", - &error_list); - return nullptr; -} - -void HealthCheckParser::Register() { - g_health_check_parser_index = ServiceConfig::RegisterParser( - UniquePtr(New())); -} - -size_t HealthCheckParser::ParserIndex() { return g_health_check_parser_index; } - -} // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/health/health_check_parser.h b/src/core/ext/filters/client_channel/health/health_check_parser.h index 036cafc68b7..0143f147152 100644 --- a/src/core/ext/filters/client_channel/health/health_check_parser.h +++ b/src/core/ext/filters/client_channel/health/health_check_parser.h @@ -24,7 +24,7 @@ #include "src/core/ext/filters/client_channel/service_config.h" namespace grpc_core { - +#if 0 class HealthCheckParsedObject : public ServiceConfig::ParsedConfig { public: HealthCheckParsedObject(const char* service_name) @@ -47,5 +47,6 @@ class HealthCheckParser : public ServiceConfig::Parser { static size_t ParserIndex(); }; +#endif } // namespace grpc_core #endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_HEALTH_HEALTH_CHECK_PARSER_H */ diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index fe99e8bc7c0..4bf4dc0ac0b 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -120,7 +120,8 @@ constexpr char kGrpclb[] = "grpclb"; class ParsedGrpcLbConfig : public ParsedLoadBalancingConfig { public: - ParsedGrpcLbConfig(RefCountedPtr child_policy) + explicit ParsedGrpcLbConfig( + RefCountedPtr child_policy) : child_policy_(std::move(child_policy)) {} const char* name() const override { return kGrpclb; } @@ -1142,13 +1143,13 @@ void GrpcLb::BalancerCallState::OnBalancerStatusReceivedLocked( // we want to retry connecting. Otherwise, we have deliberately ended this // call and no further action is required. if (lb_calld == grpclb_policy->lb_calld_.get()) { - // If the fallback-at-startup checks are pending, go into fallback mode - // immediately. This short-circuits the timeout for the fallback-at-startup - // case. - if (grpclb_policy->fallback_at_startup_checks_pending_) { - GPR_ASSERT(!lb_calld->seen_serverlist_); + // If we did not receive a serverlist and the fallback-at-startup checks + // are pending, go into fallback mode immediately. This short-circuits + // the timeout for the fallback-at-startup case. + if (!lb_calld->seen_serverlist_ && + grpclb_policy->fallback_at_startup_checks_pending_) { gpr_log(GPR_INFO, - "[grpclb %p] Balancer call finished without receiving " + "[grpclb %p] balancer call finished without receiving " "serverlist; entering fallback mode", grpclb_policy); grpclb_policy->fallback_at_startup_checks_pending_ = false; @@ -1393,7 +1394,6 @@ void GrpcLb::UpdateLocked(UpdateArgs args) { } else { child_policy_config_ = nullptr; } - ProcessAddressesAndChannelArgsLocked(args.addresses, *args.args); // Update the existing child policy. if (child_policy_ != nullptr) CreateOrUpdateChildPolicyLocked(); @@ -1627,16 +1627,20 @@ void GrpcLb::OnFallbackTimerLocked(void* arg, grpc_error* error) { grpc_channel_args* GrpcLb::CreateChildPolicyArgsLocked( bool is_backend_from_grpclb_load_balancer) { - InlinedVector args_to_add; - args_to_add.emplace_back(grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_ADDRESS_IS_BACKEND_FROM_GRPCLB_LOAD_BALANCER), - is_backend_from_grpclb_load_balancer)); + grpc_arg args_to_add[2] = { + // A channel arg indicating if the target is a backend inferred from a + // grpclb load balancer. + grpc_channel_arg_integer_create( + const_cast( + GRPC_ARG_ADDRESS_IS_BACKEND_FROM_GRPCLB_LOAD_BALANCER), + is_backend_from_grpclb_load_balancer), + }; + size_t num_args_to_add = 1; if (is_backend_from_grpclb_load_balancer) { - args_to_add.emplace_back(grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_INHIBIT_HEALTH_CHECKING), 1)); + args_to_add[num_args_to_add++] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_INHIBIT_HEALTH_CHECKING), 1); } - return grpc_channel_args_copy_and_add(args_, args_to_add.data(), - args_to_add.size()); + return grpc_channel_args_copy_and_add(args_, args_to_add, num_args_to_add); } OrphanablePtr GrpcLb::CreateChildPolicyLocked( @@ -1830,8 +1834,7 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory { return RefCountedPtr( New(std::move(child_policy))); } else { - *error = - ServiceConfig::CreateErrorFromVector("GrpcLb Parser", &error_list); + *error = GRPC_ERROR_CREATE_FROM_VECTOR("GrpcLb Parser", &error_list); return nullptr; } } diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc index b25f66d1dfc..a71efa1f230 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc @@ -362,10 +362,9 @@ class XdsLb : public LoadBalancingPolicy { : parent_(std::move(parent)), locality_weight_(locality_weight) {} ~LocalityEntry() = default; - void UpdateLocked( - xds_grpclb_serverlist* serverlist, - const RefCountedPtr& child_policy_config, - const grpc_channel_args* args); + void UpdateLocked(xds_grpclb_serverlist* serverlist, + ParsedLoadBalancingConfig* child_policy_config, + const grpc_channel_args* args); void ShutdownLocked(); void ResetBackoffLocked(); void FillChildRefsForChannelz(channelz::ChildRefsList* child_subchannels, @@ -410,10 +409,9 @@ class XdsLb : public LoadBalancingPolicy { uint32_t locality_weight_; }; - void UpdateLocked( - const LocalityList& locality_list, - const RefCountedPtr& child_policy_config, - const grpc_channel_args* args, XdsLb* parent); + void UpdateLocked(const LocalityList& locality_list, + ParsedLoadBalancingConfig* child_policy_config, + const grpc_channel_args* args, XdsLb* parent); void ShutdownLocked(); void ResetBackoffLocked(); void FillChildRefsForChannelz(channelz::ChildRefsList* child_subchannels, @@ -1217,7 +1215,7 @@ void XdsLb::BalancerChannelState::BalancerCallState:: xdslb_policy->locality_serverlist_[0]->serverlist = serverlist; xdslb_policy->locality_map_.UpdateLocked( xdslb_policy->locality_serverlist_, - xdslb_policy->child_policy_config_, xdslb_policy->args_, + xdslb_policy->child_policy_config_.get(), xdslb_policy->args_, xdslb_policy); } } else { @@ -1530,8 +1528,8 @@ void XdsLb::UpdateLocked(UpdateArgs args) { return; } ProcessAddressesAndChannelArgsLocked(args.addresses, *args.args); - locality_map_.UpdateLocked(locality_serverlist_, child_policy_config_, args_, - this); + locality_map_.UpdateLocked(locality_serverlist_, child_policy_config_.get(), + args_, this); // Update the existing fallback policy. The fallback policy config and/or the // fallback addresses may be new. if (fallback_policy_ != nullptr) UpdateFallbackPolicyLocked(); @@ -1750,7 +1748,7 @@ void XdsLb::LocalityMap::PruneLocalities(const LocalityList& locality_list) { void XdsLb::LocalityMap::UpdateLocked( const LocalityList& locality_serverlist, - const RefCountedPtr& child_policy_config, + ParsedLoadBalancingConfig* child_policy_config, const grpc_channel_args* args, XdsLb* parent) { if (parent->shutting_down_) return; for (size_t i = 0; i < locality_serverlist.size(); i++) { @@ -1845,13 +1843,13 @@ XdsLb::LocalityMap::LocalityEntry::CreateChildPolicyLocked( void XdsLb::LocalityMap::LocalityEntry::UpdateLocked( xds_grpclb_serverlist* serverlist, - const RefCountedPtr& child_policy_config, + ParsedLoadBalancingConfig* child_policy_config, const grpc_channel_args* args_in) { if (parent_->shutting_down_) return; // Construct update args. UpdateArgs update_args; update_args.addresses = ProcessServerlist(serverlist); - update_args.config = child_policy_config; + update_args.config = child_policy_config->Ref(); update_args.args = CreateChildPolicyArgsLocked(args_in); // If the child policy name changes, we need to create a new child // policy. When this happens, we leave child_policy_ as-is and store @@ -2176,8 +2174,8 @@ class XdsFactory : public LoadBalancingPolicyFactory { InlinedVector error_list; const char* balancer_name = nullptr; - RefCountedPtr child_policy = nullptr; - RefCountedPtr fallback_policy = nullptr; + RefCountedPtr child_policy; + RefCountedPtr fallback_policy; for (const grpc_json* field = json->child; field != nullptr; field = field->next) { if (field->key == nullptr) continue; @@ -2229,7 +2227,7 @@ class XdsFactory : public LoadBalancingPolicyFactory { return RefCountedPtr(New( balancer_name, std::move(child_policy), std::move(fallback_policy))); } else { - *error = ServiceConfig::CreateErrorFromVector("Xds Parser", &error_list); + *error = GRPC_ERROR_CREATE_FROM_VECTOR("Xds Parser", &error_list); return nullptr; } } diff --git a/src/core/ext/filters/client_channel/lb_policy_registry.cc b/src/core/ext/filters/client_channel/lb_policy_registry.cc index b9ea97d4051..973aa26d0f6 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.cc +++ b/src/core/ext/filters/client_channel/lb_policy_registry.cc @@ -94,9 +94,21 @@ LoadBalancingPolicyRegistry::CreateLoadBalancingPolicy( return factory->CreateLoadBalancingPolicy(std::move(args)); } -bool LoadBalancingPolicyRegistry::LoadBalancingPolicyExists(const char* name) { +bool LoadBalancingPolicyRegistry::LoadBalancingPolicyExists( + const char* name, bool* requires_config) { GPR_ASSERT(g_state != nullptr); - return g_state->GetLoadBalancingPolicyFactory(name) != nullptr; + auto* factory = g_state->GetLoadBalancingPolicyFactory(name); + if (factory == nullptr) { + return false; + } + if (requires_config != nullptr) { + grpc_error* error = GRPC_ERROR_NONE; + // Check if the load balancing policy allows an empty config + *requires_config = + factory->ParseLoadBalancingConfig(nullptr, &error) == nullptr; + GRPC_ERROR_UNREF(error); + } + return true; } namespace { @@ -152,7 +164,8 @@ grpc_json* ParseLoadBalancingConfigHelper(const grpc_json* lb_config_array, return nullptr; } // If we support this policy, then select it. - if (LoadBalancingPolicyRegistry::LoadBalancingPolicyExists(policy->key)) { + if (LoadBalancingPolicyRegistry::LoadBalancingPolicyExists(policy->key, + nullptr)) { return policy; } } @@ -189,19 +202,4 @@ LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(const grpc_json* json, } } -grpc_error* LoadBalancingPolicyRegistry::CanCreateLoadBalancingPolicy( - const char* lb_policy_name) { - GPR_DEBUG_ASSERT(g_state != nullptr); - gpr_log(GPR_ERROR, "%s", lb_policy_name); - auto* factory = g_state->GetLoadBalancingPolicyFactory(lb_policy_name); - if (factory == nullptr) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:loadBalancingPolicy error:Unknown lb policy"); - } - grpc_error* error = GRPC_ERROR_NONE; - // Check if the load balancing policy allows an empty config - factory->ParseLoadBalancingConfig(nullptr, &error); - return error; -} - } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/lb_policy_registry.h b/src/core/ext/filters/client_channel/lb_policy_registry.h index 7fa52914cd9..6820cfc9334 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.h +++ b/src/core/ext/filters/client_channel/lb_policy_registry.h @@ -49,16 +49,15 @@ class LoadBalancingPolicyRegistry { const char* name, LoadBalancingPolicy::Args args); /// Returns true if the LB policy factory specified by \a name exists in this - /// registry. - static bool LoadBalancingPolicyExists(const char* name); + /// registry. If the load balancing policy requires a config to be specified + /// then sets \a requires_config to true. + static bool LoadBalancingPolicyExists(const char* name, + bool* requires_config); /// Returns a parsed object of the load balancing policy to be used from a /// LoadBalancingConfig array \a json. static RefCountedPtr ParseLoadBalancingConfig( const grpc_json* json, grpc_error** error); - - /// Validates if a load balancing policy can be created from \a lb_policy_name - static grpc_error* CanCreateLoadBalancingPolicy(const char* lb_policy_name); }; } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/resolver_result_parsing.cc b/src/core/ext/filters/client_channel/resolver_result_parsing.cc index a3bb6b9969b..82fe3c71a5f 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.cc +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.cc @@ -29,7 +29,6 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" -#include "src/core/ext/filters/client_channel/health/health_check_parser.h" #include "src/core/ext/filters/client_channel/lb_policy_registry.h" #include "src/core/ext/filters/client_channel/server_address.h" #include "src/core/lib/channel/channel_args.h" @@ -88,25 +87,13 @@ ProcessedResolverResult::ProcessedResolverResult( void ProcessedResolverResult::ProcessServiceConfig( const Resolver::Result& resolver_result, const ClientChannelGlobalParsedObject* parsed_object) { - auto* health_check = static_cast( - service_config_->GetParsedGlobalServiceConfigObject( - HealthCheckParser::ParserIndex())); - health_check_service_name_ = - health_check != nullptr ? health_check->service_name() : nullptr; + health_check_service_name_ = parsed_object->health_check_service_name(); service_config_json_ = service_config_->service_config_json(); if (!parsed_object) { return; } if (parsed_object->retry_throttling().has_value()) { - const grpc_arg* channel_arg = - grpc_channel_args_find(resolver_result.args, GRPC_ARG_SERVER_URI); - const char* server_uri = grpc_channel_arg_get_string(channel_arg); - GPR_ASSERT(server_uri != nullptr); - grpc_uri* uri = grpc_uri_parse(server_uri, true); - GPR_ASSERT(uri->path[0] != '\0'); - server_name_ = uri->path[0] == '/' ? uri->path + 1 : uri->path; retry_throttle_data_ = parsed_object->retry_throttling(); - grpc_uri_destroy(uri); } } @@ -122,12 +109,6 @@ void ProcessedResolverResult::ProcessLbPolicy( } else { lb_policy_name_.reset( gpr_strdup(parsed_object->parsed_deprecated_lb_policy())); - if (lb_policy_name_ != nullptr) { - char* lb_policy_name = lb_policy_name_.get(); - for (size_t i = 0; i < strlen(lb_policy_name); ++i) { - lb_policy_name[i] = tolower(lb_policy_name[i]); - } - } } } // Otherwise, find the LB policy name set by the client API. @@ -215,8 +196,7 @@ UniquePtr ParseRetryPolicy( if (retry_policy->max_attempts != 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxAttempts error:Duplicate entry")); - continue; - } // Duplicate. + } // Duplicate. Continue Parsing if (sub_field->type != GRPC_JSON_NUMBER) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxAttempts error:should be of type number")); @@ -238,8 +218,7 @@ UniquePtr ParseRetryPolicy( if (retry_policy->initial_backoff > 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:initialBackoff error:Duplicate entry")); - continue; - } + } // Duplicate, continue parsing. if (!ParseDuration(sub_field, &retry_policy->initial_backoff)) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:initialBackoff error:Failed to parse")); @@ -253,8 +232,7 @@ UniquePtr ParseRetryPolicy( if (retry_policy->max_backoff > 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxBackoff error:Duplicate entry")); - continue; - } + } // Duplicate, continue parsing. if (!ParseDuration(sub_field, &retry_policy->max_backoff)) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxBackoff error:failed to parse")); @@ -268,8 +246,7 @@ UniquePtr ParseRetryPolicy( if (retry_policy->backoff_multiplier != 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:backoffMultiplier error:Duplicate entry")); - continue; - } + } // Duplicate, continue parsing. if (sub_field->type != GRPC_JSON_NUMBER) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:backoffMultiplier error:should be of type number")); @@ -289,8 +266,7 @@ UniquePtr ParseRetryPolicy( if (!retry_policy->retryable_status_codes.Empty()) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:retryableStatusCodes error:Duplicate entry")); - continue; - } + } // Duplicate, continue parsing. if (sub_field->type != GRPC_JSON_ARRAY) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:retryableStatusCodes error:should be of type array")); @@ -329,10 +305,48 @@ UniquePtr ParseRetryPolicy( return nullptr; } } - *error = ServiceConfig::CreateErrorFromVector("retryPolicy", &error_list); + *error = GRPC_ERROR_CREATE_FROM_VECTOR("retryPolicy", &error_list); return *error == GRPC_ERROR_NONE ? std::move(retry_policy) : nullptr; } +const char* ParseHealthCheckConfig(const grpc_json* field, grpc_error** error) { + GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE); + const char* service_name = nullptr; + GPR_DEBUG_ASSERT(strcmp(field->key, "healthCheckConfig") == 0); + if (field->type != GRPC_JSON_OBJECT) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:healthCheckConfig error:should be of type object"); + return nullptr; + } + InlinedVector error_list; + for (grpc_json* sub_field = field->child; sub_field != nullptr; + sub_field = sub_field->next) { + if (sub_field->key == nullptr) { + GPR_DEBUG_ASSERT(false); + continue; + } + if (strcmp(sub_field->key, "serviceName") == 0) { + if (service_name != nullptr) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:serviceName error:Duplicate " + "entry")); + } // Duplicate. Continue parsing + if (sub_field->type != GRPC_JSON_STRING) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:serviceName error:should be of type string")); + continue; + } + service_name = sub_field->value; + } + } + if (!error_list.empty()) { + return nullptr; + } + *error = + GRPC_ERROR_CREATE_FROM_VECTOR("field:healthCheckConfig", &error_list); + return service_name; +} + } // namespace UniquePtr @@ -343,6 +357,7 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json, RefCountedPtr parsed_lb_config; UniquePtr lb_policy_name; Optional retry_throttling; + const char* health_check_service_name = nullptr; for (grpc_json* field = json->child; field != nullptr; field = field->next) { if (field->key == nullptr) { continue; // Not the LB config global parameter @@ -352,14 +367,12 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json, if (parsed_lb_config != nullptr) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingConfig error:Duplicate entry")); - } else { - grpc_error* parse_error = GRPC_ERROR_NONE; - parsed_lb_config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(field, - &parse_error); - if (parsed_lb_config == nullptr) { - error_list.push_back(parse_error); - } + } // Duplicate, continue parsing. + grpc_error* parse_error = GRPC_ERROR_NONE; + parsed_lb_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + field, &parse_error); + if (parsed_lb_config == nullptr) { + error_list.push_back(parse_error); } } // Parse deprecated loadBalancingPolicy @@ -367,8 +380,8 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json, if (lb_policy_name != nullptr) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:Duplicate entry")); - continue; - } else if (field->type != GRPC_JSON_STRING) { + } // Duplicate, continue parsing. + if (field->type != GRPC_JSON_STRING) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:type should be string")); continue; @@ -380,126 +393,141 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json, lb_policy[i] = tolower(lb_policy[i]); } } - if (!LoadBalancingPolicyRegistry::LoadBalancingPolicyExists(lb_policy)) { + bool requires_config = false; + if (!LoadBalancingPolicyRegistry::LoadBalancingPolicyExists( + lb_policy, &requires_config)) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:Unknown lb policy")); - } else { - grpc_error* parsing_error = - LoadBalancingPolicyRegistry::CanCreateLoadBalancingPolicy( - lb_policy); - if (parsing_error != GRPC_ERROR_NONE) { - error_list.push_back(parsing_error); - } + } else if (requires_config) { + char* error_msg; + gpr_asprintf(&error_msg, + "field:loadBalancingPolicy error:%s requires a config. " + "Please use loadBalancingConfig instead.", + lb_policy); + error_list.push_back(GRPC_ERROR_CREATE_FROM_COPIED_STRING(error_msg)); + gpr_free(error_msg); } } // Parse retry throttling if (strcmp(field->key, "retryThrottling") == 0) { + if (retry_throttling.has_value()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling error:Duplicate entry")); + } // Duplicate, continue parsing. if (field->type != GRPC_JSON_OBJECT) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:retryThrottling error:Type should be object")); - } else if (retry_throttling.has_value()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling error:Duplicate entry")); - } else { - Optional max_milli_tokens; - Optional milli_token_ratio; - for (grpc_json* sub_field = field->child; sub_field != nullptr; - sub_field = sub_field->next) { - if (sub_field->key == nullptr) continue; - if (strcmp(sub_field->key, "maxTokens") == 0) { - if (max_milli_tokens.has_value()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:maxTokens error:Duplicate " - "entry")); - } else if (sub_field->type != GRPC_JSON_NUMBER) { + continue; + } + Optional max_milli_tokens; + Optional milli_token_ratio; + for (grpc_json* sub_field = field->child; sub_field != nullptr; + sub_field = sub_field->next) { + if (sub_field->key == nullptr) continue; + if (strcmp(sub_field->key, "maxTokens") == 0) { + if (max_milli_tokens.has_value()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:maxTokens error:Duplicate " + "entry")); + } else if (sub_field->type != GRPC_JSON_NUMBER) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:maxTokens error:Type should be " + "number")); + } else { + max_milli_tokens.set(gpr_parse_nonnegative_int(sub_field->value) * + 1000); + if (max_milli_tokens.value() <= 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:maxTokens error:Type should be " - "number")); - } else { - max_milli_tokens.set(gpr_parse_nonnegative_int(sub_field->value) * - 1000); - if (max_milli_tokens.value() <= 0) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:maxTokens error:should be " - "greater than zero")); - } + "field:retryThrottling field:maxTokens error:should be " + "greater than zero")); } - } else if (strcmp(sub_field->key, "tokenRatio") == 0) { - if (milli_token_ratio.has_value()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:tokenRatio error:Duplicate " - "entry")); - } else if (sub_field->type != GRPC_JSON_NUMBER) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:tokenRatio error:type should be " - "number")); - } else { - // We support up to 3 decimal digits. - size_t whole_len = strlen(sub_field->value); - uint32_t multiplier = 1; - uint32_t decimal_value = 0; - const char* decimal_point = strchr(sub_field->value, '.'); - if (decimal_point != nullptr) { - whole_len = - static_cast(decimal_point - sub_field->value); - multiplier = 1000; - size_t decimal_len = strlen(decimal_point + 1); - if (decimal_len > 3) decimal_len = 3; - if (!gpr_parse_bytes_to_uint32(decimal_point + 1, decimal_len, - &decimal_value)) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:tokenRatio error:Failed " - "parsing")); - continue; - } - uint32_t decimal_multiplier = 1; - for (size_t i = 0; i < (3 - decimal_len); ++i) { - decimal_multiplier *= 10; - } - decimal_value *= decimal_multiplier; - } - uint32_t whole_value; - if (!gpr_parse_bytes_to_uint32(sub_field->value, whole_len, - &whole_value)) { + } + } else if (strcmp(sub_field->key, "tokenRatio") == 0) { + if (milli_token_ratio.has_value()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:tokenRatio error:Duplicate " + "entry")); + } // Duplicate, continue parsing. + if (sub_field->type != GRPC_JSON_NUMBER) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:tokenRatio error:type should be " + "number")); + } else { + // We support up to 3 decimal digits. + size_t whole_len = strlen(sub_field->value); + uint32_t multiplier = 1; + uint32_t decimal_value = 0; + const char* decimal_point = strchr(sub_field->value, '.'); + if (decimal_point != nullptr) { + whole_len = static_cast(decimal_point - sub_field->value); + multiplier = 1000; + size_t decimal_len = strlen(decimal_point + 1); + if (decimal_len > 3) decimal_len = 3; + if (!gpr_parse_bytes_to_uint32(decimal_point + 1, decimal_len, + &decimal_value)) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:retryThrottling field:tokenRatio error:Failed " "parsing")); continue; } - milli_token_ratio.set( - static_cast((whole_value * multiplier) + decimal_value)); - if (milli_token_ratio.value() <= 0) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:tokenRatio error:value should " - "be greater than 0")); + uint32_t decimal_multiplier = 1; + for (size_t i = 0; i < (3 - decimal_len); ++i) { + decimal_multiplier *= 10; } + decimal_value *= decimal_multiplier; + } + uint32_t whole_value; + if (!gpr_parse_bytes_to_uint32(sub_field->value, whole_len, + &whole_value)) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:tokenRatio error:Failed " + "parsing")); + continue; + } + milli_token_ratio.set( + static_cast((whole_value * multiplier) + decimal_value)); + if (milli_token_ratio.value() <= 0) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:tokenRatio error:value should " + "be greater than 0")); } } } - if (!max_milli_tokens.has_value()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:maxTokens error:Not found")); - } - if (!milli_token_ratio.has_value()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:retryThrottling field:tokenRatio error:Not found")); - } - if (error_list.size() == 0) { - ClientChannelGlobalParsedObject::RetryThrottling data; - data.max_milli_tokens = max_milli_tokens.value(); - data.milli_token_ratio = milli_token_ratio.value(); - retry_throttling.set(data); - } + } + if (!max_milli_tokens.has_value()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:maxTokens error:Not found")); + } + if (!milli_token_ratio.has_value()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:retryThrottling field:tokenRatio error:Not found")); + } + if (error_list.size() == 0) { + ClientChannelGlobalParsedObject::RetryThrottling data; + data.max_milli_tokens = max_milli_tokens.value(); + data.milli_token_ratio = milli_token_ratio.value(); + retry_throttling.set(data); + } + } + if (strcmp(field->key, "healthCheckConfig") == 0) { + if (health_check_service_name != nullptr) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:healthCheckConfig error:Duplicate entry")); + } // Duplicate continue parsing + grpc_error* parsing_error = GRPC_ERROR_NONE; + health_check_service_name = ParseHealthCheckConfig(field, &parsing_error); + if (parsing_error != GRPC_ERROR_NONE) { + error_list.push_back(parsing_error); } } } - *error = ServiceConfig::CreateErrorFromVector("Client channel global parser", - &error_list); + *error = GRPC_ERROR_CREATE_FROM_VECTOR("Client channel global parser", + &error_list); if (*error == GRPC_ERROR_NONE) { return UniquePtr( - New(std::move(parsed_lb_config), - std::move(lb_policy_name), - retry_throttling)); + New( + std::move(parsed_lb_config), std::move(lb_policy_name), + retry_throttling, health_check_service_name)); } return nullptr; } @@ -518,8 +546,7 @@ ClientChannelServiceConfigParser::ParsePerMethodParams(const grpc_json* json, if (wait_for_ready.has_value()) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:waitForReady error:Duplicate entry")); - continue; - } + } // Duplicate, continue parsing. if (field->type == GRPC_JSON_TRUE) { wait_for_ready.set(true); } else if (field->type == GRPC_JSON_FALSE) { @@ -532,8 +559,7 @@ ClientChannelServiceConfigParser::ParsePerMethodParams(const grpc_json* json, if (timeout > 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:timeout error:Duplicate entry")); - continue; - } + } // Duplicate, continue parsing. if (!ParseDuration(field, &timeout)) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:timeout error:Failed parsing")); @@ -542,8 +568,7 @@ ClientChannelServiceConfigParser::ParsePerMethodParams(const grpc_json* json, if (retry_policy != nullptr) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:retryPolicy error:Duplicate entry")); - continue; - } + } // Duplicate, continue parsing. grpc_error* error = GRPC_ERROR_NONE; retry_policy = ParseRetryPolicy(field, &error); if (retry_policy == nullptr) { @@ -551,8 +576,7 @@ ClientChannelServiceConfigParser::ParsePerMethodParams(const grpc_json* json, } } } - *error = ServiceConfig::CreateErrorFromVector("Client channel parser", - &error_list); + *error = GRPC_ERROR_CREATE_FROM_VECTOR("Client channel parser", &error_list); if (*error == GRPC_ERROR_NONE) { return UniquePtr( New(timeout, wait_for_ready, diff --git a/src/core/ext/filters/client_channel/resolver_result_parsing.h b/src/core/ext/filters/client_channel/resolver_result_parsing.h index 25830dd4642..1a49fa7be8e 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.h +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.h @@ -47,10 +47,12 @@ class ClientChannelGlobalParsedObject : public ServiceConfig::ParsedConfig { ClientChannelGlobalParsedObject( RefCountedPtr parsed_lb_config, UniquePtr parsed_deprecated_lb_policy, - const Optional& retry_throttling) + const Optional& retry_throttling, + const char* health_check_service_name) : parsed_lb_config_(std::move(parsed_lb_config)), parsed_deprecated_lb_policy_(std::move(parsed_deprecated_lb_policy)), - retry_throttling_(retry_throttling) {} + retry_throttling_(retry_throttling), + health_check_service_name_(health_check_service_name) {} Optional retry_throttling() const { return retry_throttling_; @@ -64,10 +66,15 @@ class ClientChannelGlobalParsedObject : public ServiceConfig::ParsedConfig { return parsed_deprecated_lb_policy_.get(); } + const char* health_check_service_name() const { + return health_check_service_name_; + } + private: RefCountedPtr parsed_lb_config_; UniquePtr parsed_deprecated_lb_policy_; Optional retry_throttling_; + const char* health_check_service_name_ = nullptr; }; class ClientChannelMethodParsedObject : public ServiceConfig::ParsedConfig { @@ -111,8 +118,9 @@ class ClientChannelServiceConfigParser : public ServiceConfig::Parser { static void Register(); }; -// A container of processed fields from the resolver result. Simplifies the -// usage of resolver result. +// TODO(yashykt): It would be cleaner to move this logic to the client_channel +// code. A container of processed fields from the resolver result. Simplifies +// the usage of resolver result. class ProcessedResolverResult { public: // Processes the resolver result and populates the relative members @@ -122,21 +130,19 @@ class ProcessedResolverResult { // Getters. Any managed object's ownership is transferred. const char* service_config_json() { return service_config_json_; } - const char* server_name() { return server_name_; } - - Optional - retry_throttle_data() { - return retry_throttle_data_; - } + RefCountedPtr service_config() { return service_config_; } UniquePtr lb_policy_name() { return std::move(lb_policy_name_); } RefCountedPtr lb_policy_config() { return lb_policy_config_; } - const char* health_check_service_name() { return health_check_service_name_; } + Optional + retry_throttle_data() { + return retry_throttle_data_; + } - RefCountedPtr service_config() { return service_config_; } + const char* health_check_service_name() { return health_check_service_name_; } private: // Finds the service config; extracts LB config and (maybe) retry throttle @@ -163,9 +169,8 @@ class ProcessedResolverResult { RefCountedPtr service_config_; // LB policy. UniquePtr lb_policy_name_; - RefCountedPtr lb_policy_config_ = nullptr; + RefCountedPtr lb_policy_config_; // Retry throttle data. - const char* server_name_ = nullptr; Optional retry_throttle_data_; const char* health_check_service_name_ = nullptr; diff --git a/src/core/ext/filters/client_channel/resolving_lb_policy.cc b/src/core/ext/filters/client_channel/resolving_lb_policy.cc index 79d8cf43bc9..193c9e256ed 100644 --- a/src/core/ext/filters/client_channel/resolving_lb_policy.cc +++ b/src/core/ext/filters/client_channel/resolving_lb_policy.cc @@ -530,7 +530,7 @@ void ResolvingLoadBalancingPolicy::OnResolverResultChangedLocked( const bool resolution_contains_addresses = result.addresses.size() > 0; // Process the resolver result. const char* lb_policy_name = nullptr; - RefCountedPtr lb_policy_config = nullptr; + RefCountedPtr lb_policy_config; bool service_config_changed = false; if (process_resolver_result_ != nullptr) { service_config_changed = diff --git a/src/core/ext/filters/client_channel/resolving_lb_policy.h b/src/core/ext/filters/client_channel/resolving_lb_policy.h index 9822c2d1dc2..dd8a1de6c7a 100644 --- a/src/core/ext/filters/client_channel/resolving_lb_policy.h +++ b/src/core/ext/filters/client_channel/resolving_lb_policy.h @@ -123,7 +123,7 @@ class ResolvingLoadBalancingPolicy : public LoadBalancingPolicy { ProcessResolverResultCallback process_resolver_result_ = nullptr; void* process_resolver_result_user_data_ = nullptr; UniquePtr child_policy_name_; - RefCountedPtr child_lb_config_ = nullptr; + RefCountedPtr child_lb_config_; // Resolver and associated state. OrphanablePtr resolver_; diff --git a/src/core/ext/filters/client_channel/service_config.cc b/src/core/ext/filters/client_channel/service_config.cc index 8db3df712e6..86d4f7368c0 100644 --- a/src/core/ext/filters/client_channel/service_config.cc +++ b/src/core/ext/filters/client_channel/service_config.cc @@ -98,7 +98,7 @@ grpc_error* ServiceConfig::ParseGlobalParams(const grpc_json* json_tree) { } parsed_global_service_config_objects_.push_back(std::move(parsed_obj)); } - return CreateErrorFromVector("Global Params", &error_list); + return GRPC_ERROR_CREATE_FROM_VECTOR("Global Params", &error_list); } grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( @@ -154,7 +154,7 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( ++*idx; } wrap_error: - return CreateErrorFromVector("methodConfig", &error_list); + return GRPC_ERROR_CREATE_FROM_VECTOR("methodConfig", &error_list); } grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { @@ -211,7 +211,7 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { num_entries, entries, nullptr); gpr_free(entries); } - return CreateErrorFromVector("Method Params", &error_list); + return GRPC_ERROR_CREATE_FROM_VECTOR("Method Params", &error_list); } ServiceConfig::~ServiceConfig() { grpc_json_destroy(json_tree_); } @@ -313,7 +313,7 @@ ServiceConfig::GetMethodServiceConfigObjectsVector(const grpc_slice& path) { return *value; } -size_t ServiceConfig::RegisterParser(UniquePtr parser) { +size_t ServiceConfig::RegisterParser(UniquePtr parser) { g_registered_parsers->push_back(std::move(parser)); return g_registered_parsers->size() - 1; } diff --git a/src/core/ext/filters/client_channel/service_config.h b/src/core/ext/filters/client_channel/service_config.h index 1372b50eb2a..e6f855ad934 100644 --- a/src/core/ext/filters/client_channel/service_config.h +++ b/src/core/ext/filters/client_channel/service_config.h @@ -71,14 +71,14 @@ class ServiceConfig : public RefCounted { public: virtual ~Parser() = default; - virtual UniquePtr ParseGlobalParams( - const grpc_json* json, grpc_error** error) { + virtual UniquePtr ParseGlobalParams(const grpc_json* json, + grpc_error** error) { GPR_DEBUG_ASSERT(error != nullptr); return nullptr; } - virtual UniquePtr ParsePerMethodParams( - const grpc_json* json, grpc_error** error) { + virtual UniquePtr ParsePerMethodParams(const grpc_json* json, + grpc_error** error) { GPR_DEBUG_ASSERT(error != nullptr); return nullptr; } @@ -87,10 +87,14 @@ class ServiceConfig : public RefCounted { }; static constexpr int kNumPreallocatedParsers = 4; - typedef InlinedVector, - kNumPreallocatedParsers> + typedef InlinedVector, kNumPreallocatedParsers> ServiceConfigObjectsVector; + /// When a service config is applied to a call in the client_channel_filter, + /// we create an instance of this object and store it in the call_data for + /// client_channel. A pointer to this object is also stored in the + /// call_context, so that future filters can easily access method and global + /// parameters for the call. class CallData { public: CallData() = default; @@ -102,20 +106,21 @@ class ServiceConfig : public RefCounted { } } - RefCountedPtr service_config() { return service_config_; } + ServiceConfig* service_config() { return service_config_.get(); } - ServiceConfig::ParsedConfig* GetMethodParsedObject(int index) const { + ParsedConfig* GetMethodParsedObject(size_t index) const { return method_params_vector_ != nullptr ? (*method_params_vector_)[index].get() : nullptr; } - bool empty() const { return service_config_ == nullptr; } + ParsedConfig* GetGlobalParsedObject(size_t index) const { + return service_config_->GetParsedGlobalServiceConfigObject(index); + } private: RefCountedPtr service_config_; - const ServiceConfig::ServiceConfigObjectsVector* method_params_vector_ = - nullptr; + const ServiceConfigObjectsVector* method_params_vector_ = nullptr; }; /// Creates a new service config from parsing \a json_string. @@ -130,8 +135,7 @@ class ServiceConfig : public RefCounted { /// Retrieves the parsed global service config object at index \a index. The /// lifetime of the returned object is tied to the lifetime of the /// ServiceConfig object. - ServiceConfig::ParsedConfig* GetParsedGlobalServiceConfigObject( - size_t index) { + ParsedConfig* GetParsedGlobalServiceConfigObject(size_t index) { GPR_DEBUG_ASSERT(index < parsed_global_service_config_objects_.size()); return parsed_global_service_config_objects_[index].get(); } @@ -148,30 +152,12 @@ class ServiceConfig : public RefCounted { /// registered parser. Each parser is responsible for reading the service /// config json and returning a parsed object. This parsed object can later be /// retrieved using the same index that was returned at registration time. - static size_t RegisterParser(UniquePtr parser); + static size_t RegisterParser(UniquePtr parser); static void Init(); static void Shutdown(); - // Consumes all the errors in the vector and forms a referencing error from - // them. If the vector is empty, return GRPC_ERROR_NONE. - template - static grpc_error* CreateErrorFromVector( - const char* desc, InlinedVector* error_list) { - grpc_error* error = GRPC_ERROR_NONE; - if (error_list->size() != 0) { - error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( - desc, error_list->data(), error_list->size()); - // Remove refs to all errors in error_list. - for (size_t i = 0; i < error_list->size(); i++) { - GRPC_ERROR_UNREF((*error_list)[i]); - } - error_list->clear(); - } - return error; - } - private: // So New() can call our private ctor. template @@ -203,7 +189,7 @@ class ServiceConfig : public RefCounted { UniquePtr json_string_; // Underlying storage for json_tree. grpc_json* json_tree_; - InlinedVector, kNumPreallocatedParsers> + InlinedVector, kNumPreallocatedParsers> parsed_global_service_config_objects_; // A map from the method name to the service config objects vector. Note that // we are using a raw pointer and not a unique pointer so that we can use the diff --git a/src/core/ext/filters/message_size/message_size_filter.cc b/src/core/ext/filters/message_size/message_size_filter.cc index e9140a7b1c0..a92fc0e2991 100644 --- a/src/core/ext/filters/message_size/message_size_filter.cc +++ b/src/core/ext/filters/message_size/message_size_filter.cc @@ -38,12 +38,12 @@ static void recv_message_ready(void* user_data, grpc_error* error); static void recv_trailing_metadata_ready(void* user_data, grpc_error* error); +namespace grpc_core { + namespace { size_t g_message_size_parser_index; } // namespace -namespace grpc_core { - UniquePtr MessageSizeParser::ParsePerMethodParams( const grpc_json* json, grpc_error** error) { GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE); @@ -56,8 +56,8 @@ UniquePtr MessageSizeParser::ParsePerMethodParams( if (max_request_message_bytes >= 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxRequestMessageBytes error:Duplicate entry")); - } else if (field->type != GRPC_JSON_STRING && - field->type != GRPC_JSON_NUMBER) { + } // Duplicate, continue parsing. + if (field->type != GRPC_JSON_STRING && field->type != GRPC_JSON_NUMBER) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxRequestMessageBytes error:should be of type number")); } else { @@ -71,8 +71,8 @@ UniquePtr MessageSizeParser::ParsePerMethodParams( if (max_response_message_bytes >= 0) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxResponseMessageBytes error:Duplicate entry")); - } else if (field->type != GRPC_JSON_STRING && - field->type != GRPC_JSON_NUMBER) { + } // Duplicate, conitnue parsing + if (field->type != GRPC_JSON_STRING && field->type != GRPC_JSON_NUMBER) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:maxResponseMessageBytes error:should be of type number")); } else { @@ -85,8 +85,7 @@ UniquePtr MessageSizeParser::ParsePerMethodParams( } } if (!error_list.empty()) { - *error = ServiceConfig::CreateErrorFromVector("Message size parser", - &error_list); + *error = GRPC_ERROR_CREATE_FROM_VECTOR("Message size parser", &error_list); return nullptr; } return UniquePtr(New( @@ -332,7 +331,12 @@ static grpc_error* init_channel_elem(grpc_channel_element* elem, channel_data* chand = static_cast(elem->channel_data); new (chand) channel_data(); chand->limits = get_message_size_limits(args->channel_args); - // Get method config table from channel args. + // TODO(yashykt): We only need to read GRPC_ARG_SERVICE_CONFIG in the case of + // direct channels. (Service config is otherwise stored in the call_context by + // client_channel filter.) If we ever need a second filter that also needs to + // parse GRPC_ARG_SERVICE_CONFIG, we should refactor this code and add a + // separate filter that reads GRPC_ARG_SERVICE_CONFIG and saves the parsed + // config in the call_context. Get service config from channel args. const grpc_arg* channel_arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVICE_CONFIG); const char* service_config_str = grpc_channel_arg_get_string(channel_arg); diff --git a/src/core/lib/gprpp/optional.h b/src/core/lib/gprpp/optional.h index 5bead0c88ee..2166fe4763a 100644 --- a/src/core/lib/gprpp/optional.h +++ b/src/core/lib/gprpp/optional.h @@ -26,7 +26,7 @@ namespace grpc_core { template class Optional { public: - Optional() { value_ = {}; } + Optional() { value_ = T(); } void set(const T& val) { value_ = val; set_ = true; diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h index fcc6f0761b3..a877f499fb1 100644 --- a/src/core/lib/iomgr/error.h +++ b/src/core/lib/iomgr/error.h @@ -30,6 +30,7 @@ #include #include "src/core/lib/debug/trace.h" +#include "src/core/lib/gprpp/inlined_vector.h" /// Opaque representation of an error. /// See https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md for a @@ -193,6 +194,24 @@ inline void grpc_error_unref(grpc_error* err) { #define GRPC_ERROR_UNREF(err) grpc_error_unref(err) #endif +// Consumes all the errors in the vector and forms a referencing error from +// them. If the vector is empty, return GRPC_ERROR_NONE. +template +static grpc_error* GRPC_ERROR_CREATE_FROM_VECTOR( + const char* desc, grpc_core::InlinedVector* error_list) { + grpc_error* error = GRPC_ERROR_NONE; + if (error_list->size() != 0) { + error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( + desc, error_list->data(), error_list->size()); + // Remove refs to all errors in error_list. + for (size_t i = 0; i < error_list->size(); i++) { + GRPC_ERROR_UNREF((*error_list)[i]); + } + error_list->clear(); + } + return error; +} + grpc_error* grpc_error_set_int(grpc_error* src, grpc_error_ints which, intptr_t value) GRPC_MUST_USE_RESULT; /// It is an error to pass nullptr as `p`. Caller should allocate a dummy diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 8060b5a0a8c..12a47e71d7e 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -322,7 +322,6 @@ CORE_SOURCE_FILES = [ 'src/core/ext/filters/client_channel/connector.cc', 'src/core/ext/filters/client_channel/global_subchannel_pool.cc', 'src/core/ext/filters/client_channel/health/health_check_client.cc', - 'src/core/ext/filters/client_channel/health/health_check_parser.cc', 'src/core/ext/filters/client_channel/http_connect_handshaker.cc', 'src/core/ext/filters/client_channel/http_proxy.cc', 'src/core/ext/filters/client_channel/lb_policy.cc', diff --git a/test/core/client_channel/service_config_test.cc b/test/core/client_channel/service_config_test.cc index 7a41283012e..9734304c9ac 100644 --- a/test/core/client_channel/service_config_test.cc +++ b/test/core/client_channel/service_config_test.cc @@ -21,7 +21,6 @@ #include #include -#include "src/core/ext/filters/client_channel/health/health_check_parser.h" #include "src/core/ext/filters/client_channel/resolver_result_parsing.h" #include "src/core/ext/filters/client_channel/service_config.h" #include "src/core/ext/filters/message_size/message_size_filter.h" @@ -528,13 +527,13 @@ TEST_F(ClientChannelParserTest, LoadBalancingPolicyXdsNotAllowed) { auto svc_cfg = ServiceConfig::Create(test_json, &error); gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); ASSERT_TRUE(error != GRPC_ERROR_NONE); - std::regex e(std::string( - "(Service config parsing " - "error)(.*)(referenced_errors)(.*)(Global " - "Params)(.*)(referenced_errors)(.*)(Client channel global " - "parser)(.*)(referenced_errors)(.*)(field:loadBalancingPolicy error:Xds " - "Parser has required field - balancerName. Please use " - "loadBalancingConfig field of service config instead.)")); + std::regex e( + std::string("(Service config parsing " + "error)(.*)(referenced_errors)(.*)(Global " + "Params)(.*)(referenced_errors)(.*)(Client channel global " + "parser)(.*)(referenced_errors)(.*)(field:" + "loadBalancingPolicy error:xds_experimental requires a " + "config. Please use loadBalancingConfig instead.)")); VerifyRegexMatch(error, e); } @@ -856,7 +855,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyBackoffMultiplier) { " \"retryPolicy\": {\n" " \"maxAttempts\": 1,\n" " \"initialBackoff\": \"1s\",\n" - " \"maxBackoff\": \"120sec\",\n" + " \"maxBackoff\": \"120s\",\n" " \"backoffMultiplier\": \"1.6\",\n" " \"retryableStatusCodes\": [ \"ABORTED\" ]\n" " }\n" @@ -886,7 +885,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyRetryableStatusCodes) { " \"retryPolicy\": {\n" " \"maxAttempts\": 1,\n" " \"initialBackoff\": \"1s\",\n" - " \"maxBackoff\": \"120sec\",\n" + " \"maxBackoff\": \"120s\",\n" " \"backoffMultiplier\": \"1.6\",\n" " \"retryableStatusCodes\": []\n" " }\n" @@ -906,6 +905,50 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyRetryableStatusCodes) { VerifyRegexMatch(error, e); } +TEST_F(ClientChannelParserTest, ValidHealthCheck) { + const char* test_json = + "{\n" + " \"healthCheckConfig\": {\n" + " \"serviceName\": \"health_check_service_name\"\n" + " }\n" + "}"; + grpc_error* error = GRPC_ERROR_NONE; + auto svc_cfg = ServiceConfig::Create(test_json, &error); + ASSERT_TRUE(error == GRPC_ERROR_NONE); + const auto* parsed_object = + static_cast( + svc_cfg->GetParsedGlobalServiceConfigObject(0)); + ASSERT_TRUE(parsed_object != nullptr); + EXPECT_EQ(strcmp(parsed_object->health_check_service_name(), + "health_check_service_name"), + 0); +} + +TEST_F(ClientChannelParserTest, InvalidHealthCheckMultipleEntries) { + const char* test_json = + "{\n" + " \"healthCheckConfig\": {\n" + " \"serviceName\": \"health_check_service_name\"\n" + " },\n" + " \"healthCheckConfig\": {\n" + " \"serviceName\": \"health_check_service_name1\"\n" + " }\n" + "}"; + grpc_error* error = GRPC_ERROR_NONE; + auto svc_cfg = ServiceConfig::Create(test_json, &error); + gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); + ASSERT_TRUE(error != GRPC_ERROR_NONE); + std::regex e( + std::string("(Service config parsing " + "error)(.*)(referenced_errors)(.*)(Global " + "Params)(.*)(referenced_errors)(.*)(field:healthCheckConfig " + "error:Duplicate entry)")); + std::smatch match; + std::string s(grpc_error_string(error)); + EXPECT_TRUE(std::regex_search(s, match, e)); + GRPC_ERROR_UNREF(error); +} + class MessageSizeParserTest : public ::testing::Test { protected: void SetUp() override { @@ -989,59 +1032,6 @@ TEST_F(MessageSizeParserTest, InvalidMaxResponseMessageBytes) { VerifyRegexMatch(error, e); } -class HealthCheckParserTest : public ::testing::Test { - protected: - void SetUp() override { - ServiceConfig::Shutdown(); - ServiceConfig::Init(); - EXPECT_TRUE(ServiceConfig::RegisterParser(UniquePtr( - New())) == 0); - } -}; - -TEST_F(HealthCheckParserTest, Valid) { - const char* test_json = - "{\n" - " \"healthCheckConfig\": {\n" - " \"serviceName\": \"health_check_service_name\"\n" - " }\n" - "}"; - grpc_error* error = GRPC_ERROR_NONE; - auto svc_cfg = ServiceConfig::Create(test_json, &error); - ASSERT_TRUE(error == GRPC_ERROR_NONE); - const auto* parsed_object = static_cast( - svc_cfg->GetParsedGlobalServiceConfigObject(0)); - ASSERT_TRUE(parsed_object != nullptr); - EXPECT_EQ(strcmp(parsed_object->service_name(), "health_check_service_name"), - 0); -} - -TEST_F(HealthCheckParserTest, MultipleEntries) { - const char* test_json = - "{\n" - " \"healthCheckConfig\": {\n" - " \"serviceName\": \"health_check_service_name\"\n" - " },\n" - " \"healthCheckConfig\": {\n" - " \"serviceName\": \"health_check_service_name1\"\n" - " }\n" - "}"; - grpc_error* error = GRPC_ERROR_NONE; - auto svc_cfg = ServiceConfig::Create(test_json, &error); - gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); - ASSERT_TRUE(error != GRPC_ERROR_NONE); - std::regex e( - std::string("(Service config parsing " - "error)(.*)(referenced_errors)(.*)(Global " - "Params)(.*)(referenced_errors)(.*)(field:healthCheckConfig)(" - ".*)(referenced_errors)(.*)" - "(field:serviceName error:Duplicate entry)")); - std::smatch match; - std::string s(grpc_error_string(error)); - EXPECT_TRUE(std::regex_search(s, match, e)); - GRPC_ERROR_UNREF(error); -} - } // namespace testing } // namespace grpc_core diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index eee3f777ea6..dad6c98269d 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -892,8 +892,6 @@ src/core/ext/filters/client_channel/health/health.pb.c \ src/core/ext/filters/client_channel/health/health.pb.h \ src/core/ext/filters/client_channel/health/health_check_client.cc \ src/core/ext/filters/client_channel/health/health_check_client.h \ -src/core/ext/filters/client_channel/health/health_check_parser.cc \ -src/core/ext/filters/client_channel/health/health_check_parser.h \ src/core/ext/filters/client_channel/http_connect_handshaker.cc \ src/core/ext/filters/client_channel/http_connect_handshaker.h \ src/core/ext/filters/client_channel/http_proxy.cc \ diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 0c2204a3f79..9b9ad7d430c 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -8732,7 +8732,6 @@ "src/core/ext/filters/client_channel/connector.h", "src/core/ext/filters/client_channel/global_subchannel_pool.h", "src/core/ext/filters/client_channel/health/health_check_client.h", - "src/core/ext/filters/client_channel/health/health_check_parser.h", "src/core/ext/filters/client_channel/http_connect_handshaker.h", "src/core/ext/filters/client_channel/http_proxy.h", "src/core/ext/filters/client_channel/lb_policy.h", @@ -8773,8 +8772,6 @@ "src/core/ext/filters/client_channel/global_subchannel_pool.h", "src/core/ext/filters/client_channel/health/health_check_client.cc", "src/core/ext/filters/client_channel/health/health_check_client.h", - "src/core/ext/filters/client_channel/health/health_check_parser.cc", - "src/core/ext/filters/client_channel/health/health_check_parser.h", "src/core/ext/filters/client_channel/http_connect_handshaker.cc", "src/core/ext/filters/client_channel/http_connect_handshaker.h", "src/core/ext/filters/client_channel/http_proxy.cc",