diff --git a/CMakeLists.txt b/CMakeLists.txt index ac99b4ca7a5..65c67339e57 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -20604,6 +20604,10 @@ add_executable(xds_cluster_resource_type_test ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/regex.grpc.pb.cc ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/regex.pb.h ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/regex.grpc.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/round_robin.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/round_robin.grpc.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/round_robin.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/round_robin.grpc.pb.h ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/string.pb.cc ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/string.grpc.pb.cc ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/string.pb.h @@ -20616,6 +20620,10 @@ add_executable(xds_cluster_resource_type_test ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/typed_struct.grpc.pb.cc ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/typed_struct.pb.h ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/typed_struct.grpc.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/wrr_locality.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/wrr_locality.grpc.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/wrr_locality.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/wrr_locality.grpc.pb.h test/core/xds/xds_cluster_resource_type_test.cc third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 6f1cb11fead..e73711d2908 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -11308,7 +11308,8 @@ targets: gtest: true build: test language: c++ - headers: [] + headers: + - test/core/util/scoped_env_var.h src: - src/proto/grpc/testing/xds/v3/address.proto - src/proto/grpc/testing/xds/v3/aggregate_cluster.proto @@ -11320,9 +11321,11 @@ targets: - src/proto/grpc/testing/xds/v3/outlier_detection.proto - src/proto/grpc/testing/xds/v3/percent.proto - src/proto/grpc/testing/xds/v3/regex.proto + - src/proto/grpc/testing/xds/v3/round_robin.proto - src/proto/grpc/testing/xds/v3/string.proto - src/proto/grpc/testing/xds/v3/tls.proto - src/proto/grpc/testing/xds/v3/typed_struct.proto + - src/proto/grpc/testing/xds/v3/wrr_locality.proto - test/core/xds/xds_cluster_resource_type_test.cc deps: - grpc_test_util diff --git a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc index e822e201f9d..fb58248be89 100644 --- a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc +++ b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc @@ -82,8 +82,8 @@ UniqueTypeName RequestHashAttributeName() { const JsonLoaderInterface* RingHashConfig::JsonLoader(const JsonArgs&) { static const auto* loader = JsonObjectLoader() - .OptionalField("min_ring_size", &RingHashConfig::min_ring_size) - .OptionalField("max_ring_size", &RingHashConfig::max_ring_size) + .OptionalField("minRingSize", &RingHashConfig::min_ring_size) + .OptionalField("maxRingSize", &RingHashConfig::max_ring_size) .Finish(); return loader; } @@ -91,14 +91,14 @@ const JsonLoaderInterface* RingHashConfig::JsonLoader(const JsonArgs&) { void RingHashConfig::JsonPostLoad(const Json&, const JsonArgs&, ValidationErrors* errors) { { - ValidationErrors::ScopedField field(errors, ".min_ring_size"); + ValidationErrors::ScopedField field(errors, ".minRingSize"); if (!errors->FieldHasErrors() && (min_ring_size == 0 || min_ring_size > 8388608)) { errors->AddError("must be in the range [1, 8388608]"); } } { - ValidationErrors::ScopedField field(errors, ".max_ring_size"); + ValidationErrors::ScopedField field(errors, ".maxRingSize"); if (!errors->FieldHasErrors() && (max_ring_size == 0 || max_ring_size > 8388608)) { errors->AddError("must be in the range [1, 8388608]"); diff --git a/src/core/ext/xds/xds_bootstrap_grpc.h b/src/core/ext/xds/xds_bootstrap_grpc.h index e88bec69013..5d489df8aa3 100644 --- a/src/core/ext/xds/xds_bootstrap_grpc.h +++ b/src/core/ext/xds/xds_bootstrap_grpc.h @@ -33,6 +33,7 @@ #include "src/core/ext/xds/xds_bootstrap.h" #include "src/core/ext/xds/xds_cluster_specifier_plugin.h" #include "src/core/ext/xds/xds_http_filters.h" +#include "src/core/ext/xds/xds_lb_policy_registry.h" #include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/json/json.h" #include "src/core/lib/json/json_args.h" @@ -157,6 +158,9 @@ class GrpcXdsBootstrap : public XdsBootstrap { const { return cluster_specifier_plugin_registry_; } + const XdsLbPolicyRegistry& lb_policy_registry() const { + return lb_policy_registry_; + } // Exposed for testing purposes only. const std::map& authorities() const { @@ -172,6 +176,7 @@ class GrpcXdsBootstrap : public XdsBootstrap { CertificateProviderStore::PluginDefinitionMap certificate_providers_; XdsHttpFilterRegistry http_filter_registry_; XdsClusterSpecifierPluginRegistry cluster_specifier_plugin_registry_; + XdsLbPolicyRegistry lb_policy_registry_; }; } // namespace grpc_core diff --git a/src/core/ext/xds/xds_cluster.cc b/src/core/ext/xds/xds_cluster.cc index 86ea34c1fb2..4a12918c9c5 100644 --- a/src/core/ext/xds/xds_cluster.cc +++ b/src/core/ext/xds/xds_cluster.cc @@ -48,16 +48,31 @@ #include #include "src/core/ext/xds/upb_utils.h" +#include "src/core/ext/xds/xds_client.h" #include "src/core/ext/xds/xds_common_types.h" +#include "src/core/ext/xds/xds_lb_policy_registry.h" #include "src/core/ext/xds/xds_resource_type.h" +#include "src/core/lib/config/core_configuration.h" #include "src/core/lib/debug/trace.h" +#include "src/core/lib/gpr/string.h" +#include "src/core/lib/gprpp/env.h" #include "src/core/lib/gprpp/host_port.h" #include "src/core/lib/gprpp/time.h" #include "src/core/lib/gprpp/validation_errors.h" +#include "src/core/lib/load_balancing/lb_policy_registry.h" #include "src/core/lib/matchers/matchers.h" namespace grpc_core { +// TODO(roth): Remove once custom LB policy support is no longer experimental. +bool XdsCustomLbPolicyEnabled() { + auto value = GetEnv("GRPC_EXPERIMENTAL_XDS_CUSTOM_LB_CONFIG"); + if (!value.has_value()) return false; + bool parsed_value; + bool parse_succeeded = gpr_parse_bool_value(value->c_str(), &parsed_value); + return parse_succeeded && parsed_value; +} + // // XdsClusterResource // @@ -270,58 +285,39 @@ void AggregateClusterParse(const XdsResourceType::DecodeContext& context, } } -absl::StatusOr CdsResourceParse( - const XdsResourceType::DecodeContext& context, - const envoy_config_cluster_v3_Cluster* cluster) { - XdsClusterResource cds_update; - ValidationErrors errors; - // Check the cluster discovery type. - if (envoy_config_cluster_v3_Cluster_type(cluster) == - envoy_config_cluster_v3_Cluster_EDS) { - cds_update.cluster_type = XdsClusterResource::ClusterType::EDS; - EdsConfigParse(cluster, &cds_update, &errors); - } else if (envoy_config_cluster_v3_Cluster_type(cluster) == - envoy_config_cluster_v3_Cluster_LOGICAL_DNS) { - cds_update.cluster_type = XdsClusterResource::ClusterType::LOGICAL_DNS; - LogicalDnsParse(cluster, &cds_update, &errors); - } else if (envoy_config_cluster_v3_Cluster_has_cluster_type(cluster)) { - ValidationErrors::ScopedField field(&errors, ".cluster_type"); - const auto* custom_cluster_type = - envoy_config_cluster_v3_Cluster_cluster_type(cluster); - GPR_ASSERT(custom_cluster_type != nullptr); - ValidationErrors::ScopedField field2(&errors, ".typed_config"); - const auto* typed_config = - envoy_config_cluster_v3_Cluster_CustomClusterType_typed_config( - custom_cluster_type); - if (typed_config == nullptr) { - errors.AddError("field not present"); - } else { - absl::string_view type_url = absl::StripPrefix( - UpbStringToAbsl(google_protobuf_Any_type_url(typed_config)), - "type.googleapis.com/"); - if (type_url != "envoy.extensions.clusters.aggregate.v3.ClusterConfig") { - ValidationErrors::ScopedField field(&errors, ".type_url"); - errors.AddError( - absl::StrCat("unknown cluster_type extension: ", type_url)); - } else { - cds_update.cluster_type = XdsClusterResource::ClusterType::AGGREGATE; - // Retrieve aggregate clusters. - ValidationErrors::ScopedField field( - &errors, - ".value[envoy.extensions.clusters.aggregate.v3.ClusterConfig]"); - absl::string_view serialized_config = - UpbStringToAbsl(google_protobuf_Any_value(typed_config)); - AggregateClusterParse(context, serialized_config, &cds_update, &errors); +void ParseLbPolicyConfig(const XdsResourceType::DecodeContext& context, + const envoy_config_cluster_v3_Cluster* cluster, + XdsClusterResource* cds_update, + ValidationErrors* errors) { + // First, check the new load_balancing_policy field. + if (XdsCustomLbPolicyEnabled()) { + const auto* load_balancing_policy = + envoy_config_cluster_v3_Cluster_load_balancing_policy(cluster); + if (load_balancing_policy != nullptr) { + const auto& registry = + static_cast(context.client->bootstrap()) + .lb_policy_registry(); + ValidationErrors::ScopedField field(errors, ".load_balancing_policy"); + const size_t original_error_count = errors->size(); + cds_update->lb_policy_config = registry.ConvertXdsLbPolicyConfig( + context, load_balancing_policy, errors); + // If there were no conversion errors, validate that the converted config + // parses with the gRPC LB policy registry. + if (original_error_count == errors->size()) { + auto config = + CoreConfiguration::Get() + .lb_policy_registry() + .ParseLoadBalancingConfig(cds_update->lb_policy_config); + if (!config.ok()) errors->AddError(config.status().message()); } + return; } - } else { - ValidationErrors::ScopedField field(&errors, ".type"); - errors.AddError("unknown discovery type"); } - // Check the LB policy. + // Didn't find load_balancing_policy field, so fall back to the old + // lb_policy enum field. if (envoy_config_cluster_v3_Cluster_lb_policy(cluster) == envoy_config_cluster_v3_Cluster_ROUND_ROBIN) { - cds_update.lb_policy_config = { + cds_update->lb_policy_config = { Json::Object{ {"xds_wrr_locality_experimental", Json::Object{ @@ -342,50 +338,102 @@ absl::StatusOr CdsResourceParse( uint64_t min_ring_size = 1024; uint64_t max_ring_size = 8388608; if (ring_hash_config != nullptr) { - ValidationErrors::ScopedField field(&errors, ".ring_hash_lb_config"); + ValidationErrors::ScopedField field(errors, ".ring_hash_lb_config"); const google_protobuf_UInt64Value* uint64_value = envoy_config_cluster_v3_Cluster_RingHashLbConfig_maximum_ring_size( ring_hash_config); if (uint64_value != nullptr) { - ValidationErrors::ScopedField field(&errors, ".maximum_ring_size"); + ValidationErrors::ScopedField field(errors, ".maximum_ring_size"); max_ring_size = google_protobuf_UInt64Value_value(uint64_value); if (max_ring_size > 8388608 || max_ring_size == 0) { - errors.AddError("must be in the range of 1 to 8388608"); + errors->AddError("must be in the range of 1 to 8388608"); } } uint64_value = envoy_config_cluster_v3_Cluster_RingHashLbConfig_minimum_ring_size( ring_hash_config); if (uint64_value != nullptr) { - ValidationErrors::ScopedField field(&errors, ".minimum_ring_size"); + ValidationErrors::ScopedField field(errors, ".minimum_ring_size"); min_ring_size = google_protobuf_UInt64Value_value(uint64_value); if (min_ring_size > 8388608 || min_ring_size == 0) { - errors.AddError("must be in the range of 1 to 8388608"); + errors->AddError("must be in the range of 1 to 8388608"); } if (min_ring_size > max_ring_size) { - errors.AddError("cannot be greater than maximum_ring_size"); + errors->AddError("cannot be greater than maximum_ring_size"); } } if (envoy_config_cluster_v3_Cluster_RingHashLbConfig_hash_function( ring_hash_config) != envoy_config_cluster_v3_Cluster_RingHashLbConfig_XX_HASH) { - ValidationErrors::ScopedField field(&errors, ".hash_function"); - errors.AddError("invalid hash function"); + ValidationErrors::ScopedField field(errors, ".hash_function"); + errors->AddError("invalid hash function"); } } - cds_update.lb_policy_config = { + cds_update->lb_policy_config = { Json::Object{ {"ring_hash_experimental", Json::Object{ - {"min_ring_size", min_ring_size}, - {"max_ring_size", max_ring_size}, + {"minRingSize", min_ring_size}, + {"maxRingSize", max_ring_size}, }}, }, }; } else { - ValidationErrors::ScopedField field(&errors, ".lb_policy"); - errors.AddError("LB policy is not supported"); + ValidationErrors::ScopedField field(errors, ".lb_policy"); + errors->AddError("LB policy is not supported"); } +} + +absl::StatusOr CdsResourceParse( + const XdsResourceType::DecodeContext& context, + const envoy_config_cluster_v3_Cluster* cluster) { + XdsClusterResource cds_update; + ValidationErrors errors; + // Check the cluster discovery type. + if (envoy_config_cluster_v3_Cluster_type(cluster) == + envoy_config_cluster_v3_Cluster_EDS) { + cds_update.cluster_type = XdsClusterResource::ClusterType::EDS; + EdsConfigParse(cluster, &cds_update, &errors); + } else if (envoy_config_cluster_v3_Cluster_type(cluster) == + envoy_config_cluster_v3_Cluster_LOGICAL_DNS) { + cds_update.cluster_type = XdsClusterResource::ClusterType::LOGICAL_DNS; + LogicalDnsParse(cluster, &cds_update, &errors); + } else if (envoy_config_cluster_v3_Cluster_has_cluster_type(cluster)) { + ValidationErrors::ScopedField field(&errors, ".cluster_type"); + const auto* custom_cluster_type = + envoy_config_cluster_v3_Cluster_cluster_type(cluster); + GPR_ASSERT(custom_cluster_type != nullptr); + ValidationErrors::ScopedField field2(&errors, ".typed_config"); + const auto* typed_config = + envoy_config_cluster_v3_Cluster_CustomClusterType_typed_config( + custom_cluster_type); + if (typed_config == nullptr) { + errors.AddError("field not present"); + } else { + absl::string_view type_url = absl::StripPrefix( + UpbStringToAbsl(google_protobuf_Any_type_url(typed_config)), + "type.googleapis.com/"); + if (type_url != "envoy.extensions.clusters.aggregate.v3.ClusterConfig") { + ValidationErrors::ScopedField field(&errors, ".type_url"); + errors.AddError( + absl::StrCat("unknown cluster_type extension: ", type_url)); + } else { + cds_update.cluster_type = XdsClusterResource::ClusterType::AGGREGATE; + // Retrieve aggregate clusters. + ValidationErrors::ScopedField field( + &errors, + ".value[envoy.extensions.clusters.aggregate.v3.ClusterConfig]"); + absl::string_view serialized_config = + UpbStringToAbsl(google_protobuf_Any_value(typed_config)); + AggregateClusterParse(context, serialized_config, &cds_update, &errors); + } + } + } else { + ValidationErrors::ScopedField field(&errors, ".type"); + errors.AddError("unknown discovery type"); + } + // Check the LB policy. + ParseLbPolicyConfig(context, cluster, &cds_update, &errors); // transport_socket auto* transport_socket = envoy_config_cluster_v3_Cluster_transport_socket(cluster); diff --git a/src/core/ext/xds/xds_cluster.h b/src/core/ext/xds/xds_cluster.h index 2e3503c9dc1..830fc052d74 100644 --- a/src/core/ext/xds/xds_cluster.h +++ b/src/core/ext/xds/xds_cluster.h @@ -44,6 +44,8 @@ namespace grpc_core { +bool XdsCustomLbPolicyEnabled(); + struct XdsClusterResource : public XdsResourceType::ResourceData { enum ClusterType { EDS, LOGICAL_DNS, AGGREGATE }; ClusterType cluster_type; diff --git a/src/core/ext/xds/xds_lb_policy_registry.cc b/src/core/ext/xds/xds_lb_policy_registry.cc index 3f0fb25e150..ad99b012d05 100644 --- a/src/core/ext/xds/xds_lb_policy_registry.cc +++ b/src/core/ext/xds/xds_lb_policy_registry.cc @@ -19,25 +19,19 @@ #include "src/core/ext/xds/xds_lb_policy_registry.h" #include +#include #include #include -#include -#include "absl/status/status.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_format.h" #include "absl/types/optional.h" #include "absl/types/variant.h" #include "envoy/config/core/v3/extension.upb.h" #include "envoy/extensions/load_balancing_policies/ring_hash/v3/ring_hash.upb.h" #include "envoy/extensions/load_balancing_policies/wrr_locality/v3/wrr_locality.upb.h" -#include "google/protobuf/any.upb.h" #include "google/protobuf/wrappers.upb.h" -#include - -#include "src/core/ext/xds/upb_utils.h" #include "src/core/ext/xds/xds_common_types.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/validation_errors.h" @@ -47,42 +41,80 @@ namespace grpc_core { namespace { +class RoundRobinLbPolicyConfigFactory + : public XdsLbPolicyRegistry::ConfigFactory { + public: + Json::Object ConvertXdsLbPolicyConfig( + const XdsLbPolicyRegistry* /*registry*/, + const XdsResourceType::DecodeContext& /*context*/, + absl::string_view /*configuration*/, ValidationErrors* /*errors*/, + int /*recursion_depth*/) override { + return Json::Object{{"round_robin", Json::Object()}}; + } + + absl::string_view type() override { return Type(); } + + static absl::string_view Type() { + return "envoy.extensions.load_balancing_policies.round_robin.v3.RoundRobin"; + } +}; + class RingHashLbPolicyConfigFactory : public XdsLbPolicyRegistry::ConfigFactory { public: - absl::StatusOr ConvertXdsLbPolicyConfig( + Json::Object ConvertXdsLbPolicyConfig( + const XdsLbPolicyRegistry* /*registry*/, const XdsResourceType::DecodeContext& context, - absl::string_view configuration, int /* recursion_depth */) override { + absl::string_view configuration, ValidationErrors* errors, + int /*recursion_depth*/) override { const auto* resource = envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_parse( configuration.data(), configuration.size(), context.arena); if (resource == nullptr) { - return absl::InvalidArgumentError( - "Can't decode RingHash loadbalancing policy"); + errors->AddError("can't decode RingHash LB policy config"); + return {}; } if (envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_hash_function( resource) != - envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_XX_HASH) { - return absl::InvalidArgumentError( - "Invalid hash function provided for RingHash loadbalancing policy. " - "Only XX_HASH is supported."); + envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_XX_HASH && + envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_hash_function( + resource) != + envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_DEFAULT_HASH) { + ValidationErrors::ScopedField field(errors, ".hash_function"); + errors->AddError("unsupported value (must be XX_HASH)"); } - Json::Object json; - const auto* min_ring_size = - envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_minimum_ring_size( + uint64_t max_ring_size = 8388608; + const auto* uint64_value = + envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_maximum_ring_size( resource); - if (min_ring_size != nullptr) { - json.emplace("minRingSize", - google_protobuf_UInt64Value_value(min_ring_size)); + if (uint64_value != nullptr) { + max_ring_size = google_protobuf_UInt64Value_value(uint64_value); + if (max_ring_size == 0 || max_ring_size > 8388608) { + ValidationErrors::ScopedField field(errors, ".maximum_ring_size"); + errors->AddError("value must be in the range [1, 8388608]"); + } } - const auto* max_ring_size = - envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_maximum_ring_size( + uint64_t min_ring_size = 1024; + uint64_value = + envoy_extensions_load_balancing_policies_ring_hash_v3_RingHash_minimum_ring_size( resource); - if (max_ring_size != nullptr) { - json.emplace("maxRingSize", - google_protobuf_UInt64Value_value(max_ring_size)); + if (uint64_value != nullptr) { + min_ring_size = google_protobuf_UInt64Value_value(uint64_value); + ValidationErrors::ScopedField field(errors, ".minimum_ring_size"); + if (min_ring_size == 0 || min_ring_size > 8388608) { + errors->AddError("value must be in the range [1, 8388608]"); + } + if (min_ring_size > max_ring_size) { + errors->AddError("cannot be greater than maximum_ring_size"); + } } - return Json::Object{{"ring_hash_experimental", std::move(json)}}; + return Json::Object{ + {"ring_hash_experimental", + Json::Object{ + {"minRingSize", min_ring_size}, + {"maxRingSize", max_ring_size}, + }}, + }; } absl::string_view type() override { return Type(); } @@ -92,53 +124,34 @@ class RingHashLbPolicyConfigFactory } }; -class RoundRobinLbPolicyConfigFactory - : public XdsLbPolicyRegistry::ConfigFactory { - public: - absl::StatusOr ConvertXdsLbPolicyConfig( - const XdsResourceType::DecodeContext& /* context */, - absl::string_view /* configuration */, - int /* recursion_depth */) override { - return Json::Object{{"round_robin", Json::Object()}}; - } - - absl::string_view type() override { return Type(); } - - static absl::string_view Type() { - return "envoy.extensions.load_balancing_policies.round_robin.v3.RoundRobin"; - } -}; - class WrrLocalityLbPolicyConfigFactory : public XdsLbPolicyRegistry::ConfigFactory { public: - absl::StatusOr ConvertXdsLbPolicyConfig( + Json::Object ConvertXdsLbPolicyConfig( + const XdsLbPolicyRegistry* registry, const XdsResourceType::DecodeContext& context, - absl::string_view configuration, int recursion_depth) override { + absl::string_view configuration, ValidationErrors* errors, + int recursion_depth) override { const auto* resource = envoy_extensions_load_balancing_policies_wrr_locality_v3_WrrLocality_parse( configuration.data(), configuration.size(), context.arena); if (resource == nullptr) { - return absl::InvalidArgumentError( - "Can't decode WrrLocality loadbalancing policy"); + errors->AddError("can't decode WrrLocality LB policy config"); + return {}; } + ValidationErrors::ScopedField field(errors, ".endpoint_picking_policy"); const auto* endpoint_picking_policy = envoy_extensions_load_balancing_policies_wrr_locality_v3_WrrLocality_endpoint_picking_policy( resource); if (endpoint_picking_policy == nullptr) { - return absl::InvalidArgumentError( - "WrrLocality: endpoint_picking_policy not found"); - } - auto child_policy = XdsLbPolicyRegistry::ConvertXdsLbPolicyConfig( - context, endpoint_picking_policy, recursion_depth + 1); - if (!child_policy.ok()) { - return absl::InvalidArgumentError( - absl::StrCat("Error parsing WrrLocality load balancing policy: ", - child_policy.status().message())); + errors->AddError("field not present"); + return {}; } + auto child_policy = registry->ConvertXdsLbPolicyConfig( + context, endpoint_picking_policy, errors, recursion_depth + 1); return Json::Object{ {"xds_wrr_locality_experimental", - Json::Object{{"child_policy", *std::move(child_policy)}}}}; + Json::Object{{"childPolicy", std::move(child_policy)}}}}; } absl::string_view type() override { return Type(); } @@ -155,96 +168,76 @@ class WrrLocalityLbPolicyConfigFactory // XdsLbPolicyRegistry // -absl::StatusOr XdsLbPolicyRegistry::ConvertXdsLbPolicyConfig( +XdsLbPolicyRegistry::XdsLbPolicyRegistry() { + policy_config_factories_.emplace( + RingHashLbPolicyConfigFactory::Type(), + std::make_unique()); + policy_config_factories_.emplace( + RoundRobinLbPolicyConfigFactory::Type(), + std::make_unique()); + policy_config_factories_.emplace( + WrrLocalityLbPolicyConfigFactory::Type(), + std::make_unique()); +} + +Json::Array XdsLbPolicyRegistry::ConvertXdsLbPolicyConfig( const XdsResourceType::DecodeContext& context, const envoy_config_cluster_v3_LoadBalancingPolicy* lb_policy, - int recursion_depth) { + ValidationErrors* errors, int recursion_depth) const { constexpr int kMaxRecursionDepth = 16; if (recursion_depth >= kMaxRecursionDepth) { - return absl::InvalidArgumentError( - absl::StrFormat("LoadBalancingPolicy configuration has a recursion " - "depth of more than %d.", - kMaxRecursionDepth)); + errors->AddError( + absl::StrCat("exceeded max recursion depth of ", kMaxRecursionDepth)); + return {}; } + const size_t original_error_size = errors->size(); size_t size = 0; const auto* policies = envoy_config_cluster_v3_LoadBalancingPolicy_policies(lb_policy, &size); for (size_t i = 0; i < size; ++i) { - absl::StatusOr policy; + ValidationErrors::ScopedField field( + errors, absl::StrCat(".policies[", i, "].typed_extension_config")); const auto* typed_extension_config = envoy_config_cluster_v3_LoadBalancingPolicy_Policy_typed_extension_config( policies[i]); if (typed_extension_config == nullptr) { - return absl::InvalidArgumentError( - "Error parsing LoadBalancingPolicy::Policy - Missing " - "typed_extension_config field"); + errors->AddError("field not present"); + return {}; } + ValidationErrors::ScopedField field2(errors, ".typed_config"); const auto* typed_config = envoy_config_core_v3_TypedExtensionConfig_typed_config( typed_extension_config); if (typed_config == nullptr) { - return absl::InvalidArgumentError( - "Error parsing LoadBalancingPolicy::Policy - Missing " - "TypedExtensionConfig::typed_config field"); + errors->AddError("field not present"); + return {}; } - ValidationErrors errors; - auto extension = ExtractXdsExtension(context, typed_config, &errors); - if (!errors.ok()) { - return errors.status( - "Error parsing " - "LoadBalancingPolicy::Policy::TypedExtensionConfig::typed_config"); - } - GPR_ASSERT(extension.has_value()); - absl::string_view value = - UpbStringToAbsl(google_protobuf_Any_value(typed_config)); - auto config_factory_it = - Get()->policy_config_factories_.find(extension->type); - if (config_factory_it != Get()->policy_config_factories_.end()) { - policy = config_factory_it->second->ConvertXdsLbPolicyConfig( - context, value, recursion_depth); - if (!policy.ok()) { - return absl::InvalidArgumentError( - absl::StrCat("Error parsing " - "LoadBalancingPolicy::Policy::TypedExtensionConfig::" - "typed_config to JSON: ", - policy.status().message())); - } - } else if (absl::holds_alternative(extension->value)) { - // Custom lb policy config - if (!CoreConfiguration::Get() - .lb_policy_registry() - .LoadBalancingPolicyExists(extension->type, nullptr)) { - // Skip unsupported custom lb policy. - continue; + auto extension = ExtractXdsExtension(context, typed_config, errors); + if (!extension.has_value()) return {}; + // Check for registered LB policy type. + absl::string_view* serialized_value = + absl::get_if(&extension->value); + if (serialized_value != nullptr) { + auto config_factory_it = policy_config_factories_.find(extension->type); + if (config_factory_it != policy_config_factories_.end()) { + return Json::Array{config_factory_it->second->ConvertXdsLbPolicyConfig( + this, context, *serialized_value, errors, recursion_depth)}; } - Json* json = absl::get_if(&extension->value); - policy = Json::Object{{std::string(extension->type), std::move(*json)}}; - } else { - // Unsupported type. Skipping entry. - continue; } - return Json::Array{std::move(policy.value())}; + // Check for custom LB policy type. + Json* json = absl::get_if(&extension->value); + if (json != nullptr && + CoreConfiguration::Get().lb_policy_registry().LoadBalancingPolicyExists( + extension->type, nullptr)) { + return Json::Array{ + Json::Object{{std::string(extension->type), std::move(*json)}}}; + } + // Unsupported type. Continue to next entry. } - return absl::InvalidArgumentError( - "No supported load balancing policy config found."); -} - -XdsLbPolicyRegistry::XdsLbPolicyRegistry() { - policy_config_factories_.emplace( - RingHashLbPolicyConfigFactory::Type(), - std::make_unique()); - policy_config_factories_.emplace( - RoundRobinLbPolicyConfigFactory::Type(), - std::make_unique()); - policy_config_factories_.emplace( - WrrLocalityLbPolicyConfigFactory::Type(), - std::make_unique()); -} - -XdsLbPolicyRegistry* XdsLbPolicyRegistry::Get() { - // This is thread-safe since C++11 - static XdsLbPolicyRegistry* instance = new XdsLbPolicyRegistry(); - return instance; + if (original_error_size == errors->size()) { + errors->AddError("no supported load balancing policy config found"); + } + return {}; } } // namespace grpc_core diff --git a/src/core/ext/xds/xds_lb_policy_registry.h b/src/core/ext/xds/xds_lb_policy_registry.h index aafe5bac989..37c2c560b0c 100644 --- a/src/core/ext/xds/xds_lb_policy_registry.h +++ b/src/core/ext/xds/xds_lb_policy_registry.h @@ -22,11 +22,11 @@ #include #include -#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "envoy/config/cluster/v3/cluster.upb.h" #include "src/core/ext/xds/xds_resource_type.h" +#include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/json/json.h" namespace grpc_core { @@ -37,29 +37,28 @@ class XdsLbPolicyRegistry { public: class ConfigFactory { public: - virtual ~ConfigFactory() {} - virtual absl::StatusOr ConvertXdsLbPolicyConfig( + virtual ~ConfigFactory() = default; + virtual Json::Object ConvertXdsLbPolicyConfig( + const XdsLbPolicyRegistry* registry, const XdsResourceType::DecodeContext& context, - absl::string_view configuration, int recursion_depth) = 0; - + absl::string_view configuration, ValidationErrors* errors, + int recursion_depth) = 0; virtual absl::string_view type() = 0; }; + XdsLbPolicyRegistry(); + // Converts an xDS cluster load balancing policy message to gRPC's JSON // format. An error is returned if none of the lb policies in the list are // supported, or if a supported lb policy configuration conversion fails. \a // recursion_depth indicates the current depth of the tree if lb_policy // configuration recursively holds other lb policies. - static absl::StatusOr ConvertXdsLbPolicyConfig( + Json::Array ConvertXdsLbPolicyConfig( const XdsResourceType::DecodeContext& context, const envoy_config_cluster_v3_LoadBalancingPolicy* lb_policy, - int recursion_depth = 0); + ValidationErrors* errors, int recursion_depth = 0) const; private: - XdsLbPolicyRegistry(); - - static XdsLbPolicyRegistry* Get(); - // A map of config factories that goes from the type of the lb policy config // to the config factory. std::map(**decode_result.resource); EXPECT_EQ(Json{resource.lb_policy_config}.Dump(), "[{\"ring_hash_experimental\":{" - "\"max_ring_size\":8388608,\"min_ring_size\":1024}}]"); + "\"maxRingSize\":8388608,\"minRingSize\":1024}}]"); } -TEST_F(LbPolicyTest, LbPolicyRingHashSetMinAndMaxRingSize) { +TEST_F(LbPolicyTest, EnumLbPolicyRingHashSetMinAndMaxRingSize) { Cluster cluster; cluster.set_name("foo"); cluster.set_type(cluster.EDS); @@ -608,10 +615,10 @@ TEST_F(LbPolicyTest, LbPolicyRingHashSetMinAndMaxRingSize) { auto& resource = static_cast(**decode_result.resource); EXPECT_EQ(Json{resource.lb_policy_config}.Dump(), "[{\"ring_hash_experimental\":{" - "\"max_ring_size\":4096,\"min_ring_size\":2048}}]"); + "\"maxRingSize\":4096,\"minRingSize\":2048}}]"); } -TEST_F(LbPolicyTest, LbPolicyRingHashSetMinAndMaxRingSizeToZero) { +TEST_F(LbPolicyTest, EnumLbPolicyRingHashSetMinAndMaxRingSizeToZero) { Cluster cluster; cluster.set_name("foo"); cluster.set_type(cluster.EDS); @@ -638,7 +645,7 @@ TEST_F(LbPolicyTest, LbPolicyRingHashSetMinAndMaxRingSizeToZero) { << decode_result.resource.status(); } -TEST_F(LbPolicyTest, LbPolicyRingHashSetMinAndMaxRingSizeTooLarge) { +TEST_F(LbPolicyTest, EnumLbPolicyRingHashSetMinAndMaxRingSizeTooLarge) { Cluster cluster; cluster.set_name("foo"); cluster.set_type(cluster.EDS); @@ -665,7 +672,7 @@ TEST_F(LbPolicyTest, LbPolicyRingHashSetMinAndMaxRingSizeTooLarge) { << decode_result.resource.status(); } -TEST_F(LbPolicyTest, LbPolicyRingHashSetMinRingSizeLargerThanMaxRingSize) { +TEST_F(LbPolicyTest, EnumLbPolicyRingHashSetMinRingSizeLargerThanMaxRingSize) { Cluster cluster; cluster.set_name("foo"); cluster.set_type(cluster.EDS); @@ -690,7 +697,7 @@ TEST_F(LbPolicyTest, LbPolicyRingHashSetMinRingSizeLargerThanMaxRingSize) { << decode_result.resource.status(); } -TEST_F(LbPolicyTest, LbPolicyRingHashUnsupportedHashFunction) { +TEST_F(LbPolicyTest, EnumLbPolicyRingHashUnsupportedHashFunction) { Cluster cluster; cluster.set_name("foo"); cluster.set_type(cluster.EDS); @@ -714,7 +721,7 @@ TEST_F(LbPolicyTest, LbPolicyRingHashUnsupportedHashFunction) { << decode_result.resource.status(); } -TEST_F(LbPolicyTest, UnsupportedPolicy) { +TEST_F(LbPolicyTest, EnumUnsupportedPolicy) { Cluster cluster; cluster.set_name("foo"); cluster.set_type(cluster.EDS); @@ -735,6 +742,128 @@ TEST_F(LbPolicyTest, UnsupportedPolicy) { << decode_result.resource.status(); } +TEST_F(LbPolicyTest, LoadBalancingPolicyField) { + ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_CUSTOM_LB_CONFIG"); + Cluster cluster; + cluster.set_name("foo"); + cluster.set_type(cluster.EDS); + cluster.mutable_eds_cluster_config()->mutable_eds_config()->mutable_self(); + WrrLocality wrr_locality; + wrr_locality.mutable_endpoint_picking_policy() + ->add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(RoundRobin()); + cluster.mutable_load_balancing_policy() + ->add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(wrr_locality); + cluster.set_lb_policy(cluster.MAGLEV); // Will be ignored. + std::string serialized_resource; + ASSERT_TRUE(cluster.SerializeToString(&serialized_resource)); + auto* resource_type = XdsClusterResourceType::Get(); + auto decode_result = + resource_type->Decode(decode_context_, serialized_resource); + ASSERT_TRUE(decode_result.resource.ok()) << decode_result.resource.status(); + ASSERT_TRUE(decode_result.name.has_value()); + EXPECT_EQ(*decode_result.name, "foo"); + auto& resource = static_cast(**decode_result.resource); + EXPECT_EQ(Json{resource.lb_policy_config}.Dump(), + "[{\"xds_wrr_locality_experimental\":{" + "\"childPolicy\":[{\"round_robin\":{}}]}}]"); +} + +TEST_F(LbPolicyTest, LoadBalancingPolicyFieldIgnoredUnlessEnabled) { + Cluster cluster; + cluster.set_name("foo"); + cluster.set_type(cluster.EDS); + cluster.mutable_eds_cluster_config()->mutable_eds_config()->mutable_self(); + // New field says to use round_robin, but the old enum field says to use + // ring_hash. The env var is not enabled, so we use the old enum field. + cluster.mutable_load_balancing_policy() + ->add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(RoundRobin()); + cluster.set_lb_policy(cluster.RING_HASH); + std::string serialized_resource; + ASSERT_TRUE(cluster.SerializeToString(&serialized_resource)); + auto* resource_type = XdsClusterResourceType::Get(); + auto decode_result = + resource_type->Decode(decode_context_, serialized_resource); + ASSERT_TRUE(decode_result.resource.ok()) << decode_result.resource.status(); + ASSERT_TRUE(decode_result.name.has_value()); + EXPECT_EQ(*decode_result.name, "foo"); + auto& resource = static_cast(**decode_result.resource); + EXPECT_EQ(Json{resource.lb_policy_config}.Dump(), + "[{\"ring_hash_experimental\":{" + "\"maxRingSize\":8388608,\"minRingSize\":1024}}]"); +} + +// This tests that we're passing along errors from XdsLbPolicyRegistry. +// A complete list of error cases for that class is in +// xds_lb_policy_registry_test. +TEST_F(LbPolicyTest, XdsLbPolicyRegistryConversionFails) { + ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_CUSTOM_LB_CONFIG"); + Cluster cluster; + cluster.set_name("foo"); + cluster.set_type(cluster.EDS); + cluster.mutable_eds_cluster_config()->mutable_eds_config()->mutable_self(); + cluster.mutable_load_balancing_policy() + ->add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(WrrLocality()); + std::string serialized_resource; + ASSERT_TRUE(cluster.SerializeToString(&serialized_resource)); + auto* resource_type = XdsClusterResourceType::Get(); + auto decode_result = + resource_type->Decode(decode_context_, serialized_resource); + ASSERT_TRUE(decode_result.name.has_value()); + EXPECT_EQ(*decode_result.name, "foo"); + EXPECT_EQ(decode_result.resource.status().code(), + absl::StatusCode::kInvalidArgument); + EXPECT_EQ(decode_result.resource.status().message(), + "errors validating Cluster resource: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".wrr_locality.v3.WrrLocality].endpoint_picking_policy " + "error:field not present]") + << decode_result.resource.status(); +} + +TEST_F(LbPolicyTest, ConvertedCustomPolicyFailsValidation) { + ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_CUSTOM_LB_CONFIG"); + Cluster cluster; + cluster.set_name("foo"); + cluster.set_type(cluster.EDS); + cluster.mutable_eds_cluster_config()->mutable_eds_config()->mutable_self(); + TypedStruct typed_struct; + typed_struct.set_type_url( + "type.googleapis.com/xds_wrr_locality_experimental"); + cluster.mutable_load_balancing_policy() + ->add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(typed_struct); + std::string serialized_resource; + ASSERT_TRUE(cluster.SerializeToString(&serialized_resource)); + auto* resource_type = XdsClusterResourceType::Get(); + auto decode_result = + resource_type->Decode(decode_context_, serialized_resource); + ASSERT_TRUE(decode_result.name.has_value()); + EXPECT_EQ(*decode_result.name, "foo"); + EXPECT_EQ(decode_result.resource.status().code(), + absl::StatusCode::kInvalidArgument); + EXPECT_EQ(decode_result.resource.status().message(), + "errors validating Cluster resource: [" + "field:load_balancing_policy " + "error:errors validating xds_wrr_locality LB policy config: [" + "field:childPolicy error:field not present]]") + << decode_result.resource.status(); +} + // // TLS config tests // diff --git a/test/core/xds/xds_lb_policy_registry_test.cc b/test/core/xds/xds_lb_policy_registry_test.cc index bf9334514d8..3b2b92f67cf 100644 --- a/test/core/xds/xds_lb_policy_registry_test.cc +++ b/test/core/xds/xds_lb_policy_registry_test.cc @@ -27,6 +27,7 @@ #include #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include "upb/def.hpp" @@ -39,6 +40,7 @@ #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/load_balancing/lb_policy.h" #include "src/core/lib/load_balancing/lb_policy_factory.h" #include "src/core/lib/load_balancing/lb_policy_registry.h" @@ -47,10 +49,8 @@ #include "src/proto/grpc/testing/xds/v3/ring_hash.pb.h" #include "src/proto/grpc/testing/xds/v3/round_robin.pb.h" #include "src/proto/grpc/testing/xds/v3/typed_struct.pb.h" -#include "src/proto/grpc/testing/xds/v3/udpa_typed_struct.pb.h" #include "src/proto/grpc/testing/xds/v3/wrr_locality.pb.h" #include "test/core/util/test_config.h" -#include "test/cpp/util/config_grpc_cli.h" namespace grpc_core { namespace testing { @@ -66,7 +66,7 @@ using ::xds::type::v3::TypedStruct; // Uses XdsLbPolicyRegistry to convert // envoy::config::cluster::v3::LoadBalancingPolicy to gRPC's JSON form. -absl::StatusOr ConvertXdsPolicy(LoadBalancingPolicyProto policy) { +absl::StatusOr ConvertXdsPolicy(LoadBalancingPolicyProto policy) { std::string serialized_policy = policy.SerializeAsString(); upb::Arena arena; upb::SymbolTable symtab; @@ -75,117 +75,174 @@ absl::StatusOr ConvertXdsPolicy(LoadBalancingPolicyProto policy) { nullptr, symtab.ptr(), arena.ptr()}; auto* upb_policy = envoy_config_cluster_v3_LoadBalancingPolicy_parse( serialized_policy.data(), serialized_policy.size(), arena.ptr()); - return XdsLbPolicyRegistry::ConvertXdsLbPolicyConfig(context, upb_policy); + ValidationErrors errors; + ValidationErrors::ScopedField field(&errors, ".load_balancing_policy"); + auto config = XdsLbPolicyRegistry().ConvertXdsLbPolicyConfig( + context, upb_policy, &errors); + if (!errors.ok()) return errors.status("validation errors"); + EXPECT_EQ(config.size(), 1); + return Json{config[0]}.Dump(); } -TEST(XdsLbPolicyRegistryTest, EmptyLoadBalancingPolicy) { - auto result = ConvertXdsPolicy(LoadBalancingPolicyProto()); - EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT( - std::string(result.status().message()), - ::testing::HasSubstr("No supported load balancing policy config found")); -} +// A gRPC LB policy factory for a custom policy. None of the methods +// will actually be used; we just need it to be present in the gRPC LB +// policy registry. +class CustomLbPolicyFactory : public LoadBalancingPolicyFactory { + public: + OrphanablePtr CreateLoadBalancingPolicy( + LoadBalancingPolicy::Args /*args*/) const override { + GPR_ASSERT(false); + return nullptr; + } -TEST(XdsLbPolicyRegistryTest, UnsupportedBuiltinType) { + absl::string_view name() const override { return "test.CustomLb"; } + + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) const override { + return nullptr; + } +}; + +// +// RoundRobin +// + +TEST(RoundRobin, Basic) { LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - LoadBalancingPolicyProto()); + RoundRobin()); auto result = ConvertXdsPolicy(policy); - EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT( - std::string(result.status().message()), - ::testing::HasSubstr("No supported load balancing policy config found")); + ASSERT_TRUE(result.ok()) << result.status(); + EXPECT_EQ(*result, "{\"round_robin\":{}}"); } -TEST(XdsLbPolicyRegistryTest, MissingTypedExtensionConfig) { +// +// RingHash +// + +TEST(RingHashConfig, DefaultConfig) { LoadBalancingPolicyProto policy; - policy.add_policies(); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(RingHash()); auto result = ConvertXdsPolicy(policy); - EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT(std::string(result.status().message()), - ::testing::HasSubstr("Error parsing LoadBalancingPolicy::Policy " - "- Missing typed_extension_config field")); + ASSERT_TRUE(result.ok()) << result.status(); + EXPECT_EQ(*result, + "{\"ring_hash_experimental\":{" + "\"maxRingSize\":8388608,\"minRingSize\":1024}}"); } -TEST(XdsLbPolicyRegistryTest, MissingTypedConfig) { +TEST(RingHashConfig, FieldsExplicitlySet) { + RingHash ring_hash; + ring_hash.set_hash_function(RingHash::XX_HASH); + ring_hash.mutable_minimum_ring_size()->set_value(1234); + ring_hash.mutable_maximum_ring_size()->set_value(4567); LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config(); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(ring_hash); auto result = ConvertXdsPolicy(policy); - EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT( - std::string(result.status().message()), - ::testing::HasSubstr("Error parsing LoadBalancingPolicy::Policy - " - "Missing TypedExtensionConfig::typed_config field")); + ASSERT_TRUE(result.ok()) << result.status(); + EXPECT_EQ(*result, + "{\"ring_hash_experimental\":{" + "\"maxRingSize\":4567,\"minRingSize\":1234}}"); } -TEST(XdsLbPolicyRegistryTest, RingHashInvalidHash) { +TEST(RingHashConfig, InvalidHashFunction) { RingHash ring_hash; - ring_hash.set_hash_function(RingHash::DEFAULT_HASH); + ring_hash.set_hash_function(RingHash::MURMUR_HASH_2); LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - ring_hash); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(ring_hash); auto result = ConvertXdsPolicy(policy); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT( - std::string(result.status().message()), - ::testing::HasSubstr("Invalid hash function provided for RingHash " - "loadbalancing policy. Only XX_HASH is supported")); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".ring_hash.v3.RingHash].hash_function " + "error:unsupported value (must be XX_HASH)]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, RingHashRingSizeDefaults) { +TEST(RingHashConfig, RingSizesTooHigh) { RingHash ring_hash; - ring_hash.set_hash_function(RingHash::XX_HASH); + ring_hash.mutable_minimum_ring_size()->set_value(8388609); + ring_hash.mutable_maximum_ring_size()->set_value(8388609); LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - ring_hash); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(ring_hash); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{" - "\"ring_hash_experimental\": {" - "}}") - .value()); + EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".ring_hash.v3.RingHash].maximum_ring_size " + "error:value must be in the range [1, 8388608]; " + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".ring_hash.v3.RingHash].minimum_ring_size " + "error:value must be in the range [1, 8388608]]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, RingHashRingSizeCustom) { +TEST(RingHashConfig, RingSizesTooLow) { RingHash ring_hash; - ring_hash.set_hash_function(RingHash::XX_HASH); - ring_hash.mutable_minimum_ring_size()->set_value(1234); - ring_hash.mutable_maximum_ring_size()->set_value(4567); + ring_hash.mutable_minimum_ring_size()->set_value(0); + ring_hash.mutable_maximum_ring_size()->set_value(0); LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - ring_hash); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(ring_hash); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{" - "\"ring_hash_experimental\": {" - " \"minRingSize\": 1234," - " \"maxRingSize\": 4567" - "}}") - .value()); + EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".ring_hash.v3.RingHash].maximum_ring_size " + "error:value must be in the range [1, 8388608]; " + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".ring_hash.v3.RingHash].minimum_ring_size " + "error:value must be in the range [1, 8388608]]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, RoundRobin) { +TEST(RingHashConfig, MinRingSizeGreaterThanMaxRingSize) { + RingHash ring_hash; + ring_hash.mutable_minimum_ring_size()->set_value(1000); + ring_hash.mutable_maximum_ring_size()->set_value(999); LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - RoundRobin()); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(ring_hash); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{" - "\"round_robin\": {}" - "}") - .value()); + EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".ring_hash.v3.RingHash].minimum_ring_size " + "error:cannot be greater than maximum_ring_size]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, WrrLocality) { +// +// WrrLocality +// + +TEST(WrrLocality, RoundRobinChild) { WrrLocality wrr_locality; wrr_locality.mutable_endpoint_picking_policy() ->add_policies() @@ -197,52 +254,78 @@ TEST(XdsLbPolicyRegistryTest, WrrLocality) { lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( wrr_locality); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{" - "\"xds_wrr_locality_experimental\": {" - " \"child_policy\": [{" - " \"round_robin\": {}" - " }]" - "}}") - .value()); + ASSERT_TRUE(result.ok()) << result.status(); + EXPECT_EQ(*result, + "{\"xds_wrr_locality_experimental\":{" + "\"childPolicy\":[{\"round_robin\":{}}]}}"); } -TEST(XdsLbPolicyRegistryTest, WrrLocalityMissingEndpointPickingPolicy) { +TEST(WrrLocality, MissingEndpointPickingPolicy) { LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( WrrLocality()); auto result = ConvertXdsPolicy(policy); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT(std::string(result.status().message()), - ::testing::ContainsRegex( - "Error parsing LoadBalancingPolicy.*WrrLocality: " - "endpoint_picking_policy not found")); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".wrr_locality.v3.WrrLocality].endpoint_picking_policy " + "error:field not present]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, WrrLocalityChildPolicyError) { +TEST(WrrLocality, ChildPolicyInvalid) { + RingHash ring_hash; + ring_hash.set_hash_function(RingHash::MURMUR_HASH_2); WrrLocality wrr_locality; wrr_locality.mutable_endpoint_picking_policy() ->add_policies() ->mutable_typed_extension_config() ->mutable_typed_config() - ->PackFrom(RingHash()); + ->PackFrom(ring_hash); LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( wrr_locality); auto result = ConvertXdsPolicy(policy); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT(std::string(result.status().message()), - ::testing::ContainsRegex( - "Error parsing LoadBalancingPolicy.*Error parsing " - "WrrLocality load balancing policy.*Error parsing " - "LoadBalancingPolicy.*Invalid hash function provided for " - "RingHash loadbalancing policy. Only XX_HASH is supported.")); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".wrr_locality.v3.WrrLocality].endpoint_picking_policy.policies[0]" + ".typed_extension_config.typed_config.value[" + "envoy.extensions.load_balancing_policies.ring_hash.v3.RingHash]" + ".hash_function " + "error:unsupported value (must be XX_HASH)]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, WrrLocalityUnsupportedTypeSkipped) { +TEST(WrrLocality, NoSupportedChildPolicy) { + WrrLocality wrr_locality; + wrr_locality.mutable_endpoint_picking_policy() + ->add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(LoadBalancingPolicyProto()); + LoadBalancingPolicyProto policy; + auto* lb_policy = policy.add_policies(); + lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( + wrr_locality); + auto result = ConvertXdsPolicy(policy); + EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[envoy.extensions.load_balancing_policies" + ".wrr_locality.v3.WrrLocality].endpoint_picking_policy " + "error:no supported load balancing policy config found]") + << result.status(); +} + +TEST(WrrLocality, UnsupportedChildPolicyTypeSkipped) { // Create WrrLocality policy and add two policies to its list, an unsupported // type and then a known RoundRobin type. Expect that the unsupported type is // skipped and RoundRobin is selected. @@ -262,234 +345,130 @@ TEST(XdsLbPolicyRegistryTest, WrrLocalityUnsupportedTypeSkipped) { lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( wrr_locality); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{" - "\"xds_wrr_locality_experimental\": {" - " \"child_policy\": [{" - " \"round_robin\": {}" - " }]" - "}}") - .value()); + EXPECT_EQ(*result, + "{\"xds_wrr_locality_experimental\":{" + "\"childPolicy\":[{\"round_robin\":{}}]}}"); } -class CustomLbPolicyFactory : public LoadBalancingPolicyFactory { - public: - OrphanablePtr CreateLoadBalancingPolicy( - LoadBalancingPolicy::Args /* args */) const override { - GPR_ASSERT(false); - return nullptr; - } - - absl::string_view name() const override { return "test.CustomLb"; } - - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { - return nullptr; - } -}; +// +// CustomPolicy +// -TEST(XdsLbPolicyRegistryTest, CustomLbPolicy) { +TEST(CustomPolicy, Basic) { TypedStruct typed_struct; typed_struct.set_type_url("type.googleapis.com/test.CustomLb"); + auto* fields = typed_struct.mutable_value()->mutable_fields(); + (*fields)["foo"].set_string_value("bar"); LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( typed_struct); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{\"test.CustomLb\": {}}").value()); + ASSERT_TRUE(result.ok()) << result.status(); + EXPECT_EQ(*result, "{\"test.CustomLb\":{\"foo\":\"bar\"}}"); } -TEST(XdsLbPolicyRegistryTest, CustomLbPolicyUdpaTyped) { - ::udpa::type::v1::TypedStruct typed_struct; - typed_struct.set_type_url("type.googleapis.com/test.CustomLb"); - LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - typed_struct); - auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{\"test.CustomLb\": {}}").value()); -} +// +// XdsLbPolicyRegistryTest +// -TEST(XdsLbPolicyRegistryTest, UnsupportedCustomTypeError) { - TypedStruct typed_struct; - typed_struct.set_type_url("myorg/foo/bar/test.UnknownLb"); - LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - typed_struct); - auto result = ConvertXdsPolicy(policy); +TEST(XdsLbPolicyRegistryTest, EmptyLoadBalancingPolicy) { + auto result = ConvertXdsPolicy(LoadBalancingPolicyProto()); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_THAT( - std::string(result.status().message()), - ::testing::HasSubstr("No supported load balancing policy config found")); + EXPECT_EQ(result.status().message(), + "validation errors: [field:load_balancing_policy " + "error:no supported load balancing policy config found]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, CustomTypeInvalidUrlMissingSlash) { - TypedStruct typed_struct; - typed_struct.set_type_url("test.UnknownLb"); +TEST(XdsLbPolicyRegistryTest, MissingTypedExtensionConfig) { LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - typed_struct); + policy.add_policies(); auto result = ConvertXdsPolicy(policy); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); EXPECT_EQ(result.status().message(), - "Error parsing " - "LoadBalancingPolicy::Policy::TypedExtensionConfig::" - "typed_config: [" - "field:value[xds.type.v3.TypedStruct].type_url " - "error:invalid value \"test.UnknownLb\"]") + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config " + "error:field not present]") << result.status(); } -TEST(XdsLbPolicyRegistryTest, CustomTypeInvalidUrlEmptyType) { - TypedStruct typed_struct; - typed_struct.set_type_url("myorg/"); +TEST(XdsLbPolicyRegistryTest, MissingTypedConfig) { LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - typed_struct); + policy.add_policies()->mutable_typed_extension_config(); auto result = ConvertXdsPolicy(policy); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); EXPECT_EQ(result.status().message(), - "Error parsing " - "LoadBalancingPolicy::Policy::TypedExtensionConfig::" - "typed_config: " - "[field:value[xds.type.v3.TypedStruct].type_url " - "error:invalid value \"myorg/\"]") + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config error:field not present]") << result.status(); } -TEST(XdsLbPolicyRegistryTest, CustomLbPolicyJsonConversion) { +// This tests that we pass along errors that are generated by +// ExtractXdsExtension(). An exhaustive list of error cases caught by +// ExtractXdsExtension() are covered in xds_common_types_test. +TEST(XdsLbPolicyRegistryTest, ErrorExtractingExtension) { TypedStruct typed_struct; - ASSERT_TRUE(grpc::protobuf::TextFormat::ParseFromString( - R"pb( - type_url: "type.googleapis.com/test.CustomLb" - value { - fields { - key: "key" - value { null_value: NULL_VALUE } - } - fields { - key: "number" - value { number_value: 123 } - } - fields { - key: "string" - value { string_value: "value" } - } - fields { - key: "struct" - value { - struct_value { - fields { - key: "key" - value { null_value: NULL_VALUE } - } - } - } - } - fields { - key: "list" - value { - list_value { - values { null_value: NULL_VALUE } - values { number_value: 234 } - } - } - } - } - )pb", - &typed_struct)); + typed_struct.set_type_url("type.googleapis.com/"); LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( typed_struct); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse( - R"json({ - "test.CustomLb":{ - "key": null, - "number": 123, - "string": "value", - "struct": { - "key": null - }, - "list": [null, 234] - } - })json") - .value()); + EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.status().message(), + "validation errors: [" + "field:load_balancing_policy.policies[0].typed_extension_config" + ".typed_config.value[xds.type.v3.TypedStruct].type_url " + "error:invalid value \"type.googleapis.com/\"]") + << result.status(); } -TEST(XdsLbPolicyRegistryTest, CustomLbPolicyListError) { - TypedStruct typed_struct; - typed_struct.set_type_url("type.googleapis.com/test.CustomLb"); - auto* fields = typed_struct.mutable_value()->mutable_fields(); - google::protobuf::Value value; - value.mutable_list_value()->add_values(); - (*fields)["key"] = value; +TEST(XdsLbPolicyRegistryTest, NoSupportedType) { LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - typed_struct); + // Unsupported built-in type. + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(LoadBalancingPolicyProto()); + // Unsupported custom type. + TypedStruct typed_struct; + typed_struct.set_type_url("myorg/foo/bar/test.UnknownLb"); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(typed_struct); auto result = ConvertXdsPolicy(policy); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); EXPECT_EQ(result.status().message(), - "Error parsing " - "LoadBalancingPolicy::Policy::TypedExtensionConfig::typed_config: [" - "field:value[xds.type.v3.TypedStruct].value[test.CustomLb] " - "error:error encoding google::Protobuf::Struct as JSON: " - "No value set in Value proto]") + "validation errors: [field:load_balancing_policy " + "error:no supported load balancing policy config found]") << result.status(); } -TEST(XdsLbPolicyRegistryTest, UnsupportedBuiltInTypeSkipped) { - // Add two policies to list, an unsupported type and then a known RoundRobin - // type. Expect that the unsupported type is skipped and RoundRobin is - // selected. +TEST(XdsLbPolicyRegistryTest, UnsupportedTypesSkipped) { LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - LoadBalancingPolicyProto()); - lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - RoundRobin()); - auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{" - "\"round_robin\": {}" - "}") - .value()); -} - -TEST(XdsLbPolicyRegistryTest, UnsupportedCustomTypeSkipped) { - // Add two policies to list, an unsupported custom type and then a known - // RoundRobin type. Expect that the unsupported type is skipped and RoundRobin - // is selected. + // Unsupported built-in type. + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(LoadBalancingPolicyProto()); + // Unsupported custom type. TypedStruct typed_struct; typed_struct.set_type_url("myorg/foo/bar/test.UnknownLb"); - LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - typed_struct); - lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - RoundRobin()); + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(typed_struct); + // Supported type. + policy.add_policies() + ->mutable_typed_extension_config() + ->mutable_typed_config() + ->PackFrom(RoundRobin()); auto result = ConvertXdsPolicy(policy); - EXPECT_TRUE(result.ok()); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0], Json::Parse("{" - "\"round_robin\": {}" - "}") - .value()); + ASSERT_TRUE(result.ok()) << result.status(); + EXPECT_EQ(*result, "{\"round_robin\":{}}"); } // Build a recurse load balancing policy that goes beyond the max allowable @@ -517,8 +496,7 @@ TEST(XdsLbPolicyRegistryTest, MaxRecursion) { auto result = ConvertXdsPolicy(BuildRecursiveLoadBalancingPolicy(0)); EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); EXPECT_THAT(std::string(result.status().message()), - ::testing::ContainsRegex("LoadBalancingPolicy configuration has " - "a recursion depth of more than 16")); + ::testing::EndsWith("error:exceeded max recursion depth of 16]")); } } // namespace @@ -533,7 +511,6 @@ int main(int argc, char** argv) { builder->lb_policy_registry()->RegisterLoadBalancingPolicyFactory( std::make_unique()); }); - grpc_init(); auto result = RUN_ALL_TESTS(); grpc_shutdown();