diff --git a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc index c9a29741a2e..d863a677d52 100644 --- a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc @@ -469,7 +469,7 @@ grpc_error_handle XdsResolver::XdsConfigSelector::CreateMethodConfig( RefCountedPtr* method_config) { std::vector fields; // Set retry policy if any. - if (route.retry_policy.has_value()) { + if (route.retry_policy.has_value() && !route.retry_policy->retry_on.Empty()) { std::vector retry_parts; retry_parts.push_back(absl::StrFormat( "\"retryPolicy\": {\n" diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index c321a92b9f2..d5425cc6df6 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -1552,10 +1552,6 @@ grpc_error_handle RetryPolicyParse( } } } - // TODO(donnadionne): when we add support for per_try_timeout, we will need to - // return a policy if per_try_timeout is set even if retry_on specified no - // supported policies. - if (retry_to_return.retry_on.Empty()) return GRPC_ERROR_NONE; const google_protobuf_UInt32Value* num_retries = envoy_config_route_v3_RetryPolicy_num_retries(retry_policy); if (num_retries != nullptr) { diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 813854e7e28..28a90d9d29a 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -5894,6 +5894,39 @@ TEST_P(LdsRdsTest, XdsRetryPolicyUnsupportedStatusCode) { EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); } +TEST_P(LdsRdsTest, + XdsRetryPolicyUnsupportedStatusCodeWithVirtualHostLevelRetry) { + const size_t kNumRetries = 3; + SetNextResolution({}); + SetNextResolutionForLbChannelAllBalancers(); + // Populate new EDS resources. + AdsServiceImpl::EdsResourceArgs args({ + {"locality0", CreateEndpointsForBackends(0, 1)}, + }); + balancers_[0]->ads_service()->SetEdsResource(BuildEdsResource(args)); + // Construct route config to set retry policy with no supported retry_on + // statuses. + RouteConfiguration new_route_config = default_route_config_; + auto* route1 = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); + auto* retry_policy = route1->mutable_route()->mutable_retry_policy(); + retry_policy->set_retry_on("5xx"); + retry_policy->mutable_num_retries()->set_value(kNumRetries); + // Construct a virtual host level retry policy with supported statuses. + auto* virtual_host_retry_policy = + new_route_config.mutable_virtual_hosts(0)->mutable_retry_policy(); + virtual_host_retry_policy->set_retry_on( + "cancelled,deadline-exceeded,internal,resource-exhausted,unavailable"); + virtual_host_retry_policy->mutable_num_retries()->set_value(kNumRetries); + SetRouteConfiguration(0, new_route_config); + // We expect no retry. + CheckRpcSendFailure( + CheckRpcSendFailureOptions() + .set_rpc_options(RpcOptions().set_server_expected_error( + StatusCode::DEADLINE_EXCEEDED)) + .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED)); + EXPECT_EQ(1, backends_[0]->backend_service()->request_count()); +} + TEST_P(LdsRdsTest, XdsRetryPolicyInvalidNumRetriesZero) { SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers();