xds: remove env var protection for retries (#27039)

src/core/ext/xds/xds_api.cc

@@ -137,17 +137,6 @@ bool XdsSecurityEnabled() {
   return parse_succeeded && parsed_value;
 }
 
-// TODO(donnadionne): Check to see if retry policy is enabled, this will be
-// removed once retry functionality is fully integration-tested and enabled by
-// default.
-bool XdsRetryEnabled() {
-  char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
-  bool parsed_value;
-  bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value);
-  gpr_free(value);
-  return parse_succeeded && parsed_value;
-}
-
 //
 // XdsApi::Route::HashPolicy
 //
@@ -1839,15 +1828,13 @@ grpc_error_handle RouteActionParse(const EncodingContext& context,
     }
   }
   // Get retry policy
-  if (XdsRetryEnabled()) {
+  const envoy_config_route_v3_RetryPolicy* retry_policy =
-    const envoy_config_route_v3_RetryPolicy* retry_policy =
+      envoy_config_route_v3_RouteAction_retry_policy(route_action);
-        envoy_config_route_v3_RouteAction_retry_policy(route_action);
+  if (retry_policy != nullptr) {
-    if (retry_policy != nullptr) {
+    absl::optional<XdsApi::Route::RetryPolicy> retry;
-      absl::optional<XdsApi::Route::RetryPolicy> retry;
+    grpc_error_handle error = RetryPolicyParse(context, retry_policy, &retry);
-      grpc_error_handle error = RetryPolicyParse(context, retry_policy, &retry);
+    if (error != GRPC_ERROR_NONE) return error;
-      if (error != GRPC_ERROR_NONE) return error;
+    route->retry_policy = retry;
-      route->retry_policy = retry;
-    }
   }
   return GRPC_ERROR_NONE;
 }
@@ -1898,12 +1885,10 @@ grpc_error_handle RouteConfigParse(
     absl::optional<XdsApi::Route::RetryPolicy> virtual_host_retry_policy;
     const envoy_config_route_v3_RetryPolicy* retry_policy =
         envoy_config_route_v3_VirtualHost_retry_policy(virtual_hosts[i]);
-    if (XdsRetryEnabled()) {
+    if (retry_policy != nullptr) {
-      if (retry_policy != nullptr) {
+      grpc_error_handle error =
-        grpc_error_handle error =
+          RetryPolicyParse(context, retry_policy, &virtual_host_retry_policy);
-            RetryPolicyParse(context, retry_policy, &virtual_host_retry_policy);
+      if (error != GRPC_ERROR_NONE) return error;
-        if (error != GRPC_ERROR_NONE) return error;
-      }
     }
     // Parse routes.
     size_t num_routes;
@@ -1939,8 +1924,7 @@ grpc_error_handle RouteConfigParse(
       error = RouteActionParse(context, routes[j], &route, &ignore_route);
       if (error != GRPC_ERROR_NONE) return error;
       if (ignore_route) continue;
-      if (XdsRetryEnabled() && route.retry_policy == absl::nullopt &&
+      if (route.retry_policy == absl::nullopt && retry_policy != nullptr) {
-          retry_policy != nullptr) {
         route.retry_policy = virtual_host_retry_policy;
       }
       if (context.use_v3) {

test/cpp/end2end/xds_end2end_test.cc

@@ -5650,7 +5650,6 @@ TEST_P(LdsRdsTest, XdsRoutingWithOnlyApplicationTimeout) {
 }
 
 TEST_P(LdsRdsTest, XdsRetryPolicyNumRetries) {
-  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true");
   const size_t kNumRetries = 3;
   SetNextResolution({});
   SetNextResolutionForLbChannelAllBalancers();
@@ -5711,11 +5710,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyNumRetries) {
               StatusCode::UNAUTHENTICATED))
           .set_expected_error_code(StatusCode::UNAUTHENTICATED));
   EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
-  gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
 }
 
 TEST_P(LdsRdsTest, XdsRetryPolicyAtVirtualHostLevel) {
-  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true");
   const size_t kNumRetries = 3;
   SetNextResolution({});
   SetNextResolutionForLbChannelAllBalancers();
@@ -5739,11 +5736,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyAtVirtualHostLevel) {
               StatusCode::DEADLINE_EXCEEDED))
           .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED));
   EXPECT_EQ(kNumRetries + 1, backends_[0]->backend_service()->request_count());
-  gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
 }
 
 TEST_P(LdsRdsTest, XdsRetryPolicyLongBackOff) {
-  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true");
   // Set num retries to 3, but due to longer back off, we expect only 1 retry
   // will take place.
   const size_t kNumRetries = 3;
@@ -5777,11 +5772,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyLongBackOff) {
                   StatusCode::CANCELLED))
           .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED));
   EXPECT_EQ(1 + 1, backends_[0]->backend_service()->request_count());
-  gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
 }
 
 TEST_P(LdsRdsTest, XdsRetryPolicyMaxBackOff) {
-  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true");
   // Set num retries to 3, but due to longer back off, we expect only 2 retry
   // will take place, while the 2nd one will obey the max backoff.
   const size_t kNumRetries = 3;
@@ -5822,11 +5815,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyMaxBackOff) {
                   StatusCode::CANCELLED))
           .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED));
   EXPECT_EQ(2 + 1, backends_[0]->backend_service()->request_count());
-  gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
 }
 
 TEST_P(LdsRdsTest, XdsRetryPolicyUnsupportedStatusCode) {
-  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true");
   const size_t kNumRetries = 3;
   SetNextResolution({});
   SetNextResolutionForLbChannelAllBalancers();
@@ -5849,11 +5840,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyUnsupportedStatusCode) {
               StatusCode::DEADLINE_EXCEEDED))
           .set_expected_error_code(StatusCode::DEADLINE_EXCEEDED));
   EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
-  gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
 }
 
 TEST_P(LdsRdsTest, XdsRetryPolicyInvalidNumRetriesZero) {
-  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true");
   SetNextResolution({});
   SetNextResolutionForLbChannelAllBalancers();
   // Populate new EDS resources.
@@ -5876,11 +5865,9 @@ TEST_P(LdsRdsTest, XdsRetryPolicyInvalidNumRetriesZero) {
       response_state.error_message,
       ::testing::HasSubstr(
           "RouteAction RetryPolicy num_retries set to invalid value 0."));
-  gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
 }
 
 TEST_P(LdsRdsTest, XdsRetryPolicyRetryBackOffMissingBaseInterval) {
-  gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY", "true");
   SetNextResolution({});
   SetNextResolutionForLbChannelAllBalancers();
   // Populate new EDS resources.
@@ -5907,70 +5894,6 @@ TEST_P(LdsRdsTest, XdsRetryPolicyRetryBackOffMissingBaseInterval) {
       response_state.error_message,
       ::testing::HasSubstr(
           "RouteAction RetryPolicy RetryBackoff missing base interval."));
-  gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY");
-}
-
-TEST_P(LdsRdsTest, XdsRetryPolicyDisabled) {
-  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.
-  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,cancelled,deadline-exceeded,internal,resource-exhausted,"
-      "unavailable");
-  retry_policy->mutable_num_retries()->set_value(kNumRetries);
-  SetRouteConfiguration(0, new_route_config);
-  // Ensure we don't retry on supported statuses.
-  CheckRpcSendFailure(
-      CheckRpcSendFailureOptions()
-          .set_rpc_options(
-              RpcOptions().set_server_expected_error(StatusCode::CANCELLED))
-          .set_expected_error_code(StatusCode::CANCELLED));
-  EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
-  ResetBackendCounters();
-  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());
-  ResetBackendCounters();
-  CheckRpcSendFailure(
-      CheckRpcSendFailureOptions()
-          .set_rpc_options(
-              RpcOptions().set_server_expected_error(StatusCode::INTERNAL))
-          .set_expected_error_code(StatusCode::INTERNAL));
-  EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
-  ResetBackendCounters();
-  CheckRpcSendFailure(
-      CheckRpcSendFailureOptions()
-          .set_rpc_options(RpcOptions().set_server_expected_error(
-              StatusCode::RESOURCE_EXHAUSTED))
-          .set_expected_error_code(StatusCode::RESOURCE_EXHAUSTED));
-  EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
-  ResetBackendCounters();
-  CheckRpcSendFailure(
-      CheckRpcSendFailureOptions()
-          .set_rpc_options(
-              RpcOptions().set_server_expected_error(StatusCode::UNAVAILABLE))
-          .set_expected_error_code(StatusCode::UNAVAILABLE));
-  EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
-  ResetBackendCounters();
-  // Ensure we don't retry on an unsupported status.
-  CheckRpcSendFailure(
-      CheckRpcSendFailureOptions()
-          .set_rpc_options(RpcOptions().set_server_expected_error(
-              StatusCode::UNAUTHENTICATED))
-          .set_expected_error_code(StatusCode::UNAUTHENTICATED));
-  EXPECT_EQ(1, backends_[0]->backend_service()->request_count());
 }
 
 TEST_P(LdsRdsTest, XdsRoutingHeadersMatching) {