diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc index 7ce706724ee..7b5eb311393 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc @@ -308,8 +308,11 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) { if (service_config_string != nullptr) { 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 = - ServiceConfig::Create(service_config_string, nullptr); + ServiceConfig::Create(service_config_string, &service_config_error); + // Error is currently unused. + GRPC_ERROR_UNREF(service_config_error); } gpr_free(service_config_string); } 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 f523ffe1610..7148e16e70b 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.cc +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.cc @@ -52,7 +52,10 @@ ProcessedResolverResult::ProcessedResolverResult( 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) { - service_config_ = ServiceConfig::Create(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); } } else { // Add the service config JSON to channel args so that it's diff --git a/src/core/ext/filters/client_channel/service_config.cc b/src/core/ext/filters/client_channel/service_config.cc index 30b5981da84..360b4cd81bf 100644 --- a/src/core/ext/filters/client_channel/service_config.cc +++ b/src/core/ext/filters/client_channel/service_config.cc @@ -38,30 +38,16 @@ RefCountedPtr ServiceConfig::Create(const char* json, grpc_error** error) { UniquePtr service_config_json(gpr_strdup(json)); UniquePtr json_string(gpr_strdup(json)); + GPR_DEBUG_ASSERT(error != nullptr); grpc_json* json_tree = grpc_json_parse_string(json_string.get()); if (json_tree == nullptr) { - if (error != nullptr) { - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "failed to parse JSON for service config"); - } + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "failed to parse JSON for service config"); return nullptr; } - grpc_error* create_error; auto return_value = MakeRefCounted( - std::move(service_config_json), std::move(json_string), json_tree, - &create_error); - if (create_error != GRPC_ERROR_NONE) { - if (error != nullptr) { - *error = create_error; - } else { - GRPC_ERROR_UNREF(create_error); - } - return nullptr; - } - if (error != nullptr) { - *error = GRPC_ERROR_NONE; - } - return return_value; + std::move(service_config_json), std::move(json_string), json_tree, error); + return *error == GRPC_ERROR_NONE ? return_value : nullptr; } ServiceConfig::ServiceConfig(UniquePtr service_config_json, @@ -115,22 +101,26 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( if (child->key == nullptr) continue; if (strcmp(child->key, "name") == 0) { if (child->type != GRPC_JSON_ARRAY) { - return grpc_error_add_child(error, GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "name should be of type Array")); + error = grpc_error_add_child(error, + GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:name error:not of type Array")); + goto wrap_error; } for (grpc_json* name = child->child; name != nullptr; name = name->next) { - UniquePtr path = ParseJsonMethodName(name); + grpc_error* parse_error = GRPC_ERROR_NONE; + UniquePtr path = ParseJsonMethodName(name, &parse_error); if (path == nullptr) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING("Failed to parse name"); + error = grpc_error_add_child(error, parse_error); + } else { + GPR_DEBUG_ASSERT(parse_error == GRPC_ERROR_NONE); } paths.push_back(std::move(path)); } } } if (paths.size() == 0) { - return grpc_error_add_child(error, - GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "No names specified in methodConfig")); + error = grpc_error_add_child( + error, GRPC_ERROR_CREATE_FROM_STATIC_STRING("No names specified")); } // Add entry for each path. for (size_t i = 0; i < paths.size(); ++i) { @@ -138,6 +128,11 @@ grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable( entries[*idx].value = vector_ptr; // Takes a new ref. ++*idx; } +wrap_error: + if (error != GRPC_ERROR_NONE) { + error = grpc_error_add_child( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("field:methodConfig"), error); + } return error; } @@ -150,8 +145,9 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { for (grpc_json* field = json_tree->child; field != nullptr; field = field->next) { if (field->key == nullptr) { - error = grpc_error_add_child(error, GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "Illegal key value - NULL")); + error = + grpc_error_add_child(error, GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "error:Illegal key value - NULL")); continue; } if (strcmp(field->key, "methodConfig") == 0) { @@ -159,17 +155,17 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { GPR_ASSERT(false); } if (field->type != GRPC_JSON_ARRAY) { - return grpc_error_add_child(error, - GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "methodConfig is not of type Array")); + return grpc_error_add_child( + error, GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:methodConfig error:not of type Array")); } for (grpc_json* method = field->child; method != nullptr; method = method->next) { int count = CountNamesInMethodConfig(method); if (count <= 0) { - error = grpc_error_add_child(error, - GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "No names found in methodConfig")); + error = grpc_error_add_child( + error, GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:methodConfig error:No names found")); } num_entries += static_cast(count); } @@ -184,6 +180,7 @@ grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) { error, ParseJsonMethodConfigToServiceConfigObjectsTable( method, entries, &idx)); } + GPR_DEBUG_ASSERT(num_entries == idx); break; } } @@ -229,24 +226,56 @@ int ServiceConfig::CountNamesInMethodConfig(grpc_json* json) { return num_names; } -UniquePtr ServiceConfig::ParseJsonMethodName(grpc_json* json) { - if (json->type != GRPC_JSON_OBJECT) return nullptr; +UniquePtr ServiceConfig::ParseJsonMethodName(grpc_json* json, + grpc_error** error) { + if (json->type != GRPC_JSON_OBJECT) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Field name should be of type object"); + return nullptr; + } const char* service_name = nullptr; const char* method_name = nullptr; for (grpc_json* child = json->child; child != nullptr; child = child->next) { - if (child->key == nullptr) return nullptr; - if (child->type != GRPC_JSON_STRING) return nullptr; + if (child->key == nullptr) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Child entry with no key"); + return nullptr; + } + if (child->type != GRPC_JSON_STRING) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Child entry should of type string"); + return nullptr; + } if (strcmp(child->key, "service") == 0) { - if (service_name != nullptr) return nullptr; // Duplicate. - if (child->value == nullptr) return nullptr; + if (service_name != nullptr) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:service error:Multiple entries"); + return nullptr; // Duplicate. + } + if (child->value == nullptr) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:service error:empty value"); + return nullptr; + } service_name = child->value; } else if (strcmp(child->key, "method") == 0) { - if (method_name != nullptr) return nullptr; // Duplicate. - if (child->value == nullptr) return nullptr; + if (method_name != nullptr) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:method error:multiple entries"); + return nullptr; // Duplicate. + } + if (child->value == nullptr) { + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:method error:empty value"); + return nullptr; + } method_name = child->value; } } - if (service_name == nullptr) return nullptr; // Required field. + if (service_name == nullptr) { + *error = + GRPC_ERROR_CREATE_FROM_STATIC_STRING("field:service error:not found"); + return nullptr; // Required field. + } char* path; gpr_asprintf(&path, "/%s/%s", service_name, method_name == nullptr ? "*" : method_name); diff --git a/src/core/ext/filters/client_channel/service_config.h b/src/core/ext/filters/client_channel/service_config.h index 3eb3bd54644..9738cc82d30 100644 --- a/src/core/ext/filters/client_channel/service_config.h +++ b/src/core/ext/filters/client_channel/service_config.h @@ -185,8 +185,9 @@ class ServiceConfig : public RefCounted { static int CountNamesInMethodConfig(grpc_json* json); // Returns a path string for the JSON name object specified by \a json. - // Returns null on error. - static UniquePtr ParseJsonMethodName(grpc_json* json); + // Returns null on error, and stores error in \a error. + static UniquePtr ParseJsonMethodName(grpc_json* json, + grpc_error** error); // Parses the method config from \a json. Adds an entry to \a entries for // each name found, incrementing \a idx for each entry added. @@ -209,8 +210,13 @@ class ServiceConfig : public RefCounted { 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 + // same vector for multiple names. RefCountedPtr> parsed_method_service_config_objects_table_; + // Storage for all the vectors that are being used in + // parsed_method_service_config_objects_table_. InlinedVector, 32> service_config_objects_vectors_storage_; }; @@ -247,7 +253,10 @@ bool ServiceConfig::ParseJsonMethodConfig( if (strcmp(child->key, "name") == 0) { if (child->type != GRPC_JSON_ARRAY) return false; for (grpc_json* name = child->child; name != nullptr; name = name->next) { - UniquePtr path = ParseJsonMethodName(name); + grpc_error* error = GRPC_ERROR_NONE; + UniquePtr path = ParseJsonMethodName(name, &error); + // We are not reporting the error here. + GRPC_ERROR_UNREF(error); if (path == nullptr) return false; paths.push_back(std::move(path)); } diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index d66779e6192..20a6023f71c 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -590,8 +590,11 @@ Subchannel::Subchannel(SubchannelKey* key, grpc_connector* connector, const char* service_config_json = grpc_channel_arg_get_string( grpc_channel_args_find(args_, GRPC_ARG_SERVICE_CONFIG)); if (service_config_json != nullptr) { + grpc_error* service_config_error = GRPC_ERROR_NONE; RefCountedPtr service_config = - ServiceConfig::Create(service_config_json, nullptr); + ServiceConfig::Create(service_config_json, &service_config_error); + // service_config_error is currently unused. + GRPC_ERROR_UNREF(service_config_error); if (service_config != nullptr) { HealthCheckParams params; service_config->ParseGlobalParams(HealthCheckParams::Parse, ¶ms); 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 8c2a32cc618..4d120c0eb76 100644 --- a/src/core/ext/filters/message_size/message_size_filter.cc +++ b/src/core/ext/filters/message_size/message_size_filter.cc @@ -319,8 +319,11 @@ static grpc_error* init_channel_elem(grpc_channel_element* elem, grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVICE_CONFIG); const char* service_config_str = grpc_channel_arg_get_string(channel_arg); if (service_config_str != nullptr) { + grpc_error* service_config_error = GRPC_ERROR_NONE; grpc_core::RefCountedPtr service_config = - grpc_core::ServiceConfig::Create(service_config_str, nullptr); + grpc_core::ServiceConfig::Create(service_config_str, + &service_config_error); + GRPC_ERROR_UNREF(service_config_error); if (service_config != nullptr) { chand->method_limit_table = service_config->CreateMethodConfigTable( grpc_core::MessageSizeLimits::CreateFromJson); diff --git a/test/core/client_channel/service_config_test.cc b/test/core/client_channel/service_config_test.cc index d8cb3532cb5..50e99ccfc10 100644 --- a/test/core/client_channel/service_config_test.cc +++ b/test/core/client_channel/service_config_test.cc @@ -46,14 +46,14 @@ class TestParser1 : public ServiceConfigParser { field = field->next) { if (strcmp(field->key, "global_param") == 0) { if (field->type != GRPC_JSON_NUMBER) { - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "global_param value type should be a number"); + *error = + GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidTypeErrorMessage()); return nullptr; } int value = gpr_parse_nonnegative_int(field->value); if (value == -1) { - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "global_param value type should be non-negative"); + *error = + GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidValueErrorMessage()); return nullptr; } return UniquePtr( @@ -62,6 +62,14 @@ class TestParser1 : public ServiceConfigParser { } return nullptr; } + + static const char* InvalidTypeErrorMessage() { + return "global_param value type should be a number"; + } + + static const char* InvalidValueErrorMessage() { + return "global_param value type should be non-negative"; + } }; class TestParser2 : public ServiceConfigParser { @@ -76,14 +84,14 @@ class TestParser2 : public ServiceConfigParser { } if (strcmp(field->key, "method_param") == 0) { if (field->type != GRPC_JSON_NUMBER) { - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "method_param value type should be a number"); + *error = + GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidTypeErrorMessage()); return nullptr; } int value = gpr_parse_nonnegative_int(field->value); if (value == -1) { - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "method_param value type should be non-negative"); + *error = + GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidValueErrorMessage()); return nullptr; } return UniquePtr( @@ -92,6 +100,14 @@ class TestParser2 : public ServiceConfigParser { } return nullptr; } + + static const char* InvalidTypeErrorMessage() { + return "method_param value type should be a number"; + } + + static const char* InvalidValueErrorMessage() { + return "method_param value type should be non-negative"; + } }; class ServiceConfigTest : public ::testing::Test { @@ -110,7 +126,10 @@ TEST_F(ServiceConfigTest, ErrorCheck1) { const char* test_json = ""; grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(test_json, &error); + gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); EXPECT_TRUE(error != GRPC_ERROR_NONE); + EXPECT_TRUE(strstr(grpc_error_string(error), + "failed to parse JSON for service config") != nullptr); } TEST_F(ServiceConfigTest, BasicTest1) { @@ -126,6 +145,7 @@ TEST_F(ServiceConfigTest, ErrorNoNames) { auto svc_cfg = ServiceConfig::Create(test_json, &error); gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); EXPECT_TRUE(error != GRPC_ERROR_NONE); + EXPECT_TRUE(strstr(grpc_error_string(error), "No names found") != nullptr); } TEST_F(ServiceConfigTest, ErrorNoNamesWithMultipleMethodConfigs) { @@ -135,6 +155,7 @@ TEST_F(ServiceConfigTest, ErrorNoNamesWithMultipleMethodConfigs) { auto svc_cfg = ServiceConfig::Create(test_json, &error); gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); EXPECT_TRUE(error != GRPC_ERROR_NONE); + EXPECT_TRUE(strstr(grpc_error_string(error), "No names found") != nullptr); } TEST_F(ServiceConfigTest, ValidMethodConfig) { @@ -171,6 +192,8 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidType) { auto svc_cfg = ServiceConfig::Create(test_json, &error); gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); EXPECT_TRUE(error != GRPC_ERROR_NONE); + EXPECT_TRUE(strstr(grpc_error_string(error), + TestParser1::InvalidTypeErrorMessage()) != nullptr); } TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) { @@ -179,6 +202,8 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) { auto svc_cfg = ServiceConfig::Create(test_json, &error); gpr_log(GPR_ERROR, "%s", grpc_error_string(error)); EXPECT_TRUE(error != GRPC_ERROR_NONE); + EXPECT_TRUE(strstr(grpc_error_string(error), + TestParser1::InvalidValueErrorMessage()) != nullptr); } TEST_F(ServiceConfigTest, Parser2BasicTest) { @@ -188,6 +213,12 @@ TEST_F(ServiceConfigTest, Parser2BasicTest) { grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(test_json, &error); EXPECT_TRUE(error == GRPC_ERROR_NONE); + const auto* const* vector_ptr = svc_cfg->GetMethodServiceConfigObjectsVector( + grpc_slice_from_static_string("/TestServ/TestMethod")); + ASSERT_TRUE(vector_ptr != nullptr); + const auto* vector = *vector_ptr; + auto parsed_object = ((*vector)[1]).get(); + EXPECT_TRUE(static_cast(parsed_object)->value() == 5); } TEST_F(ServiceConfigTest, Parser2ErrorInvalidType) { @@ -197,6 +228,8 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidType) { grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(test_json, &error); EXPECT_TRUE(error != GRPC_ERROR_NONE); + EXPECT_TRUE(strstr(grpc_error_string(error), + TestParser2::InvalidTypeErrorMessage()) != nullptr); } TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) { @@ -206,6 +239,8 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) { grpc_error* error = GRPC_ERROR_NONE; auto svc_cfg = ServiceConfig::Create(test_json, &error); EXPECT_TRUE(error != GRPC_ERROR_NONE); + EXPECT_TRUE(strstr(grpc_error_string(error), + TestParser2::InvalidValueErrorMessage()) != nullptr); } } // namespace testing diff --git a/test/cpp/end2end/grpclb_end2end_test.cc b/test/cpp/end2end/grpclb_end2end_test.cc index 7c6432379a6..096482543ca 100644 --- a/test/cpp/end2end/grpclb_end2end_test.cc +++ b/test/cpp/end2end/grpclb_end2end_test.cc @@ -542,8 +542,10 @@ class GrpclbEnd2endTest : public ::testing::Test { grpc_core::Resolver::Result result; result.addresses = CreateLbAddressesFromAddressDataList(address_data); if (service_config_json != nullptr) { + grpc_error* error = GRPC_ERROR_NONE; result.service_config = - grpc_core::ServiceConfig::Create(service_config_json); + grpc_core::ServiceConfig::Create(service_config_json, &error); + GRPC_ERROR_UNREF(error); } response_generator_->SetResponse(std::move(result)); } diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 61e759c61b5..fc343939041 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -524,8 +524,10 @@ class XdsEnd2endTest : public ::testing::Test { grpc_core::Resolver::Result result; result.addresses = CreateLbAddressesFromPortList(ports); if (service_config_json != nullptr) { + grpc_error* error = GRPC_ERROR_NONE; result.service_config = grpc_core::ServiceConfig::Create(service_config_json); + GRPC_ERROR_UNREF(error); } grpc_arg arg = grpc_core::FakeResolverResponseGenerator::MakeChannelArg( lb_channel_response_generator == nullptr @@ -555,8 +557,10 @@ class XdsEnd2endTest : public ::testing::Test { grpc_core::Resolver::Result result; result.addresses = CreateLbAddressesFromPortList(ports); if (service_config_json != nullptr) { + grpc_error* error = GRPC_ERROR_NONE; result.service_config = grpc_core::ServiceConfig::Create(service_config_json); + GRPC_ERROR_UNREF(error); } if (lb_channel_response_generator == nullptr) { lb_channel_response_generator = lb_channel_response_generator_.get();