Service Config Changes to set channel in transient failure on invalid…

… service config

CMakeLists.txt

@@ -703,6 +703,7 @@ add_dependencies(buildtests_cxx server_crash_test_client)
 add_dependencies(buildtests_cxx server_early_return_test)
 add_dependencies(buildtests_cxx server_interceptors_end2end_test)
 add_dependencies(buildtests_cxx server_request_call_test)
+add_dependencies(buildtests_cxx service_config_end2end_test)
 add_dependencies(buildtests_cxx service_config_test)
 add_dependencies(buildtests_cxx shutdown_test)
 add_dependencies(buildtests_cxx slice_hash_table_test)
@@ -15936,6 +15937,53 @@ target_link_libraries(server_request_call_test
 )
 
 
+endif (gRPC_BUILD_TESTS)
+if (gRPC_BUILD_TESTS)
+
+add_executable(service_config_end2end_test
+  ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/lb/v1/load_balancer.pb.cc
+  ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/lb/v1/load_balancer.grpc.pb.cc
+  ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/lb/v1/load_balancer.pb.h
+  ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/lb/v1/load_balancer.grpc.pb.h
+  test/cpp/end2end/service_config_end2end_test.cc
+  third_party/googletest/googletest/src/gtest-all.cc
+  third_party/googletest/googlemock/src/gmock-all.cc
+)
+
+protobuf_generate_grpc_cpp(
+  src/proto/grpc/lb/v1/load_balancer.proto
+)
+
+target_include_directories(service_config_end2end_test
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}
+  PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include
+  PRIVATE ${_gRPC_SSL_INCLUDE_DIR}
+  PRIVATE ${_gRPC_PROTOBUF_INCLUDE_DIR}
+  PRIVATE ${_gRPC_ZLIB_INCLUDE_DIR}
+  PRIVATE ${_gRPC_BENCHMARK_INCLUDE_DIR}
+  PRIVATE ${_gRPC_CARES_INCLUDE_DIR}
+  PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR}
+  PRIVATE ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR}
+  PRIVATE ${_gRPC_NANOPB_INCLUDE_DIR}
+  PRIVATE third_party/googletest/googletest/include
+  PRIVATE third_party/googletest/googletest
+  PRIVATE third_party/googletest/googlemock/include
+  PRIVATE third_party/googletest/googlemock
+  PRIVATE ${_gRPC_PROTO_GENS_DIR}
+)
+
+target_link_libraries(service_config_end2end_test
+  ${_gRPC_PROTOBUF_LIBRARIES}
+  ${_gRPC_ALLTARGETS_LIBRARIES}
+  grpc++_test_util
+  grpc_test_util
+  grpc++
+  grpc
+  gpr
+  ${_gRPC_GFLAGS_LIBRARIES}
+)
+
+
 endif (gRPC_BUILD_TESTS)
 if (gRPC_BUILD_TESTS)
 

Makefile

@@ -1265,6 +1265,7 @@ server_crash_test_client: $(BINDIR)/$(CONFIG)/server_crash_test_client
 server_early_return_test: $(BINDIR)/$(CONFIG)/server_early_return_test
 server_interceptors_end2end_test: $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test
 server_request_call_test: $(BINDIR)/$(CONFIG)/server_request_call_test
+service_config_end2end_test: $(BINDIR)/$(CONFIG)/service_config_end2end_test
 service_config_test: $(BINDIR)/$(CONFIG)/service_config_test
 shutdown_test: $(BINDIR)/$(CONFIG)/shutdown_test
 slice_hash_table_test: $(BINDIR)/$(CONFIG)/slice_hash_table_test
@@ -1736,6 +1737,7 @@ buildtests_cxx: privatelibs_cxx \
   $(BINDIR)/$(CONFIG)/server_early_return_test \
   $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test \
   $(BINDIR)/$(CONFIG)/server_request_call_test \
+  $(BINDIR)/$(CONFIG)/service_config_end2end_test \
   $(BINDIR)/$(CONFIG)/service_config_test \
   $(BINDIR)/$(CONFIG)/shutdown_test \
   $(BINDIR)/$(CONFIG)/slice_hash_table_test \
@@ -1882,6 +1884,7 @@ buildtests_cxx: privatelibs_cxx \
   $(BINDIR)/$(CONFIG)/server_early_return_test \
   $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test \
   $(BINDIR)/$(CONFIG)/server_request_call_test \
+  $(BINDIR)/$(CONFIG)/service_config_end2end_test \
   $(BINDIR)/$(CONFIG)/service_config_test \
   $(BINDIR)/$(CONFIG)/shutdown_test \
   $(BINDIR)/$(CONFIG)/slice_hash_table_test \
@@ -2402,6 +2405,8 @@ test_cxx: buildtests_cxx
 	$(Q) $(BINDIR)/$(CONFIG)/server_interceptors_end2end_test || ( echo test server_interceptors_end2end_test failed ; exit 1 )
 	$(E) "[RUN]     Testing server_request_call_test"
 	$(Q) $(BINDIR)/$(CONFIG)/server_request_call_test || ( echo test server_request_call_test failed ; exit 1 )
+	$(E) "[RUN]     Testing service_config_end2end_test"
+	$(Q) $(BINDIR)/$(CONFIG)/service_config_end2end_test || ( echo test service_config_end2end_test failed ; exit 1 )
 	$(E) "[RUN]     Testing service_config_test"
 	$(Q) $(BINDIR)/$(CONFIG)/service_config_test || ( echo test service_config_test failed ; exit 1 )
 	$(E) "[RUN]     Testing shutdown_test"
@@ -18895,6 +18900,53 @@ endif
 $(OBJDIR)/$(CONFIG)/test/cpp/server/server_request_call_test.o: $(GENDIR)/src/proto/grpc/testing/echo_messages.pb.cc $(GENDIR)/src/proto/grpc/testing/echo_messages.grpc.pb.cc $(GENDIR)/src/proto/grpc/testing/echo.pb.cc $(GENDIR)/src/proto/grpc/testing/echo.grpc.pb.cc
 
 
+SERVICE_CONFIG_END2END_TEST_SRC = \
+    $(GENDIR)/src/proto/grpc/lb/v1/load_balancer.pb.cc $(GENDIR)/src/proto/grpc/lb/v1/load_balancer.grpc.pb.cc \
+    test/cpp/end2end/service_config_end2end_test.cc \
+
+SERVICE_CONFIG_END2END_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(SERVICE_CONFIG_END2END_TEST_SRC))))
+ifeq ($(NO_SECURE),true)
+
+# You can't build secure targets if you don't have OpenSSL.
+
+$(BINDIR)/$(CONFIG)/service_config_end2end_test: openssl_dep_error
+
+else
+
+
+
+
+ifeq ($(NO_PROTOBUF),true)
+
+# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.5.0+.
+
+$(BINDIR)/$(CONFIG)/service_config_end2end_test: protobuf_dep_error
+
+else
+
+$(BINDIR)/$(CONFIG)/service_config_end2end_test: $(PROTOBUF_DEP) $(SERVICE_CONFIG_END2END_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a
+	$(E) "[LD]      Linking $@"
+	$(Q) mkdir -p `dirname $@`
+	$(Q) $(LDXX) $(LDFLAGS) $(SERVICE_CONFIG_END2END_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/service_config_end2end_test
+
+endif
+
+endif
+
+$(OBJDIR)/$(CONFIG)/src/proto/grpc/lb/v1/load_balancer.o:  $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a
+
+$(OBJDIR)/$(CONFIG)/test/cpp/end2end/service_config_end2end_test.o:  $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a
+
+deps_service_config_end2end_test: $(SERVICE_CONFIG_END2END_TEST_OBJS:.o=.dep)
+
+ifneq ($(NO_SECURE),true)
+ifneq ($(NO_DEPS),true)
+-include $(SERVICE_CONFIG_END2END_TEST_OBJS:.o=.dep)
+endif
+endif
+$(OBJDIR)/$(CONFIG)/test/cpp/end2end/service_config_end2end_test.o: $(GENDIR)/src/proto/grpc/lb/v1/load_balancer.pb.cc $(GENDIR)/src/proto/grpc/lb/v1/load_balancer.grpc.pb.cc
+
+
 SERVICE_CONFIG_TEST_SRC = \
     test/core/client_channel/service_config_test.cc \
 

build.yaml

@@ -5520,6 +5520,19 @@ targets:
   - grpc++_unsecure
   - grpc_unsecure
   - gpr
+- name: service_config_end2end_test
+  gtest: true
+  build: test
+  language: c++
+  src:
+  - src/proto/grpc/lb/v1/load_balancer.proto
+  - test/cpp/end2end/service_config_end2end_test.cc
+  deps:
+  - grpc++_test_util
+  - grpc_test_util
+  - grpc++
+  - grpc
+  - gpr
 - name: service_config_test
   gtest: true
   build: test

src/core/ext/filters/client_channel/client_channel.cc

@@ -224,7 +224,8 @@ class ChannelData {
 
   static bool ProcessResolverResultLocked(
       void* arg, const Resolver::Result& result, const char** lb_policy_name,
-      RefCountedPtr<ParsedLoadBalancingConfig>* lb_policy_config);
+      RefCountedPtr<ParsedLoadBalancingConfig>* lb_policy_config,
+      grpc_error** service_config_error);
 
   grpc_error* DoPingLocked(grpc_transport_op* op);
 
@@ -1132,9 +1133,16 @@ ChannelData::~ChannelData() {
 // resolver result update.
 bool ChannelData::ProcessResolverResultLocked(
     void* arg, const Resolver::Result& result, const char** lb_policy_name,
-    RefCountedPtr<ParsedLoadBalancingConfig>* lb_policy_config) {
+    RefCountedPtr<ParsedLoadBalancingConfig>* lb_policy_config,
+    grpc_error** service_config_error) {
   ChannelData* chand = static_cast<ChannelData*>(arg);
   ProcessedResolverResult resolver_result(result);
+
+  *service_config_error = resolver_result.service_config_error();
+  if (*service_config_error != GRPC_ERROR_NONE) {
+    // We got an invalid service config.
+    return false;
+  }
   char* service_config_json = gpr_strdup(resolver_result.service_config_json());
   if (grpc_client_channel_routing_trace.enabled()) {
     gpr_log(GPR_INFO, "chand=%p: resolver returned service config: \"%s\"",

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc

@@ -110,6 +110,8 @@ class AresDnsResolver : public Resolver {
   UniquePtr<ServerAddressList> addresses_;
   /// currently resolving service config
   char* service_config_json_ = nullptr;
+  /// last valid service config
+  RefCountedPtr<ServiceConfig> saved_service_config_;
   // has shutdown been initiated
   bool shutdown_initiated_ = false;
   // timeout in milliseconds for active DNS queries
@@ -310,13 +312,29 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) {
         GRPC_CARES_TRACE_LOG("resolver:%p selected service config choice: %s",
                              r, service_config_string);
         grpc_error* service_config_error = GRPC_ERROR_NONE;
-        result.service_config =
+        auto new_service_config =
             ServiceConfig::Create(service_config_string, &service_config_error);
-        // Error is currently unused.
-        GRPC_ERROR_UNREF(service_config_error);
+        if (service_config_error == GRPC_ERROR_NONE) {
+          // Valid service config receivd
+          r->saved_service_config_ = std::move(new_service_config);
+        } else {
+          if (r->saved_service_config_ != nullptr) {
+            // Ignore the new service config error, since we have a previously
+            // saved service config
+            GRPC_ERROR_UNREF(service_config_error);
+          } else {
+            // No previously valid service config found.
+            // service_config_error is passed to the channel.
+            result.service_config_error = service_config_error;
+          }
+        }
       }
       gpr_free(service_config_string);
+    } else {
+      // No service config received
+      r->saved_service_config_.reset();
     }
+    result.service_config = r->saved_service_config_;
     result.args = grpc_channel_args_copy(r->channel_args_);
     r->result_handler()->ReturnResult(std::move(result));
     r->addresses_.reset();

src/core/ext/filters/client_channel/resolver_result_parsing.cc

@@ -61,17 +61,24 @@ void ClientChannelServiceConfigParser::Register() {
 ProcessedResolverResult::ProcessedResolverResult(
     const Resolver::Result& resolver_result)
     : service_config_(resolver_result.service_config) {
-  // If resolver did not return a service config, use the default
+  // If resolver did not return a service config or returned an invalid service config, use the default
   // specified via the client API.
   if (service_config_ == nullptr) {
     const char* service_config_json = grpc_channel_arg_get_string(
         grpc_channel_args_find(resolver_result.args, GRPC_ARG_SERVICE_CONFIG));
     if (service_config_json != nullptr) {
-      grpc_error* error = GRPC_ERROR_NONE;
-      service_config_ = ServiceConfig::Create(service_config_json, &error);
-      // Error is currently unused.
-      GRPC_ERROR_UNREF(error);
+      service_config_ =
+          ServiceConfig::Create(service_config_json, &service_config_error_);
+    } else {
+      service_config_error_ = GRPC_ERROR_REF(resolver_result.service_config_error);
     }
+  } else {
+    service_config_error_ =
+        GRPC_ERROR_REF(resolver_result.service_config_error);
+  }
+  if (service_config_error_ != GRPC_ERROR_NONE) {
+    // We got an invalid service config. Don't process any further.
+    return;
   }
   // Process service config.
   const ClientChannelGlobalParsedObject* parsed_object = nullptr;

src/core/ext/filters/client_channel/resolver_result_parsing.h

@@ -127,6 +127,8 @@ class ProcessedResolverResult {
   // for later consumption.
   ProcessedResolverResult(const Resolver::Result& resolver_result);
 
+  ~ProcessedResolverResult() { GRPC_ERROR_UNREF(service_config_error_); }
+
   // Getters. Any managed object's ownership is transferred.
   const char* service_config_json() { return service_config_json_; }
 
@@ -144,6 +146,12 @@ class ProcessedResolverResult {
 
   const char* health_check_service_name() { return health_check_service_name_; }
 
+  grpc_error* service_config_error() {
+    grpc_error* return_error = service_config_error_;
+    service_config_error_ = GRPC_ERROR_NONE;
+    return return_error;
+  }
+
  private:
   // Finds the service config; extracts LB config and (maybe) retry throttle
   // params from it.
@@ -167,6 +175,7 @@ class ProcessedResolverResult {
   // Service config.
   const char* service_config_json_ = nullptr;
   RefCountedPtr<ServiceConfig> service_config_;
+  grpc_error* service_config_error_ = GRPC_ERROR_NONE;
   // LB policy.
   UniquePtr<char> lb_policy_name_;
   RefCountedPtr<ParsedLoadBalancingConfig> lb_policy_config_;

src/core/ext/filters/client_channel/resolving_lb_policy.cc

@@ -533,9 +533,13 @@ void ResolvingLoadBalancingPolicy::OnResolverResultChangedLocked(
   RefCountedPtr<ParsedLoadBalancingConfig> lb_policy_config;
   bool service_config_changed = false;
   if (process_resolver_result_ != nullptr) {
-    service_config_changed =
-        process_resolver_result_(process_resolver_result_user_data_, result,
-                                 &lb_policy_name, &lb_policy_config);
+    grpc_error* service_config_error = GRPC_ERROR_NONE;
+    service_config_changed = process_resolver_result_(
+        process_resolver_result_user_data_, result, &lb_policy_name,
+        &lb_policy_config, &service_config_error);
+    if (service_config_error != GRPC_ERROR_NONE) {
+      return OnResolverError(service_config_error);
+    }
   } else {
     lb_policy_name = child_policy_name_.get();
     lb_policy_config = child_lb_config_;

src/core/ext/filters/client_channel/resolving_lb_policy.h

@@ -68,7 +68,8 @@ class ResolvingLoadBalancingPolicy : public LoadBalancingPolicy {
   typedef bool (*ProcessResolverResultCallback)(
       void* user_data, const Resolver::Result& result,
       const char** lb_policy_name,
-      RefCountedPtr<ParsedLoadBalancingConfig>* lb_policy_config);
+      RefCountedPtr<ParsedLoadBalancingConfig>* lb_policy_config,
+      grpc_error** service_config_error);
   // If error is set when this returns, then construction failed, and
   // the caller may not use the new object.
   ResolvingLoadBalancingPolicy(

test/cpp/end2end/BUILD

@@ -423,6 +423,27 @@ grpc_cc_test(
     ],
 )
 
+grpc_cc_test(
+    name = "service_config_end2end_test",
+    srcs = ["service_config_end2end_test.cc"],
+    external_deps = [
+        "gmock",
+        "gtest",
+    ],
+    deps = [
+        ":test_service_impl",
+        "//:gpr",
+        "//:grpc",
+        "//:grpc++",
+        "//src/proto/grpc/testing:echo_messages_proto",
+        "//src/proto/grpc/testing:echo_proto",
+        "//src/proto/grpc/testing/duplicate:echo_duplicate_proto",
+        "//test/core/util:grpc_test_util",
+        "//test/core/util:test_lb_policies",
+        "//test/cpp/util:test_util",
+    ],
+)
+
 grpc_cc_test(
     name = "grpclb_end2end_test",
     srcs = ["grpclb_end2end_test.cc"],