From 2dc58be3b5bd2de024a9f3d4f54a38f8e9725ff2 Mon Sep 17 00:00:00 2001 From: Donna Dionne Date: Tue, 10 Nov 2020 11:15:54 -0800 Subject: [PATCH] Protect timeout feature under a flag and added testing. --- src/core/ext/xds/xds_api.cc | 40 ++++++++---- test/cpp/end2end/xds_end2end_test.cc | 97 ++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 13 deletions(-) diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index 6335bac94fd..31d862d6c2d 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -88,6 +88,17 @@ namespace grpc_core { +// TODO (donnadionne): Check to see if timeout is enabled, this will be +// removed once timeout feature is fully integration-tested and enabled by +// default. +bool XdsTimeoutEnabled() { + char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); + bool parsed_value; + bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value); + gpr_free(value); + return parse_succeeded && parsed_value; +} + // // XdsApi::Route::Matchers::PathMatcher // @@ -1129,7 +1140,7 @@ grpc_error* RouteActionParse(const envoy_config_route_v3_Route* route_msg, // No cluster or weighted_clusters found in RouteAction, ignore this route. *ignore_route = true; } - if (!*ignore_route) { + if (XdsTimeoutEnabled() && !*ignore_route) { const envoy_config_route_v3_RouteAction_MaxStreamDuration* max_stream_duration = envoy_config_route_v3_RouteAction_max_stream_duration(route_action); @@ -1278,18 +1289,21 @@ grpc_error* LdsResponseParse( return GRPC_ERROR_CREATE_FROM_STATIC_STRING( "Could not parse HttpConnectionManager config from ApiListener"); } - // Obtain max_stream_duration from Http Protocol Options. - const envoy_config_core_v3_HttpProtocolOptions* options = - envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_common_http_protocol_options( - http_connection_manager); - if (options != nullptr) { - const google_protobuf_Duration* duration = - envoy_config_core_v3_HttpProtocolOptions_max_stream_duration(options); - if (duration != nullptr) { - lds_update.http_max_stream_duration.seconds = - google_protobuf_Duration_seconds(duration); - lds_update.http_max_stream_duration.nanos = - google_protobuf_Duration_nanos(duration); + if (XdsTimeoutEnabled()) { + // Obtain max_stream_duration from Http Protocol Options. + const envoy_config_core_v3_HttpProtocolOptions* options = + envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_common_http_protocol_options( + http_connection_manager); + if (options != nullptr) { + const google_protobuf_Duration* duration = + envoy_config_core_v3_HttpProtocolOptions_max_stream_duration( + options); + if (duration != nullptr) { + lds_update.http_max_stream_duration.seconds = + google_protobuf_Duration_seconds(duration); + lds_update.http_max_stream_duration.nanos = + google_protobuf_Duration_nanos(duration); + } } } // Found inlined route_config. Parse it to find the cluster_name. diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index b614b113f10..1c31bcfdde1 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -4251,6 +4251,7 @@ TEST_P(LdsRdsTest, XdsRoutingApplyXdsTimeout) { // TODO(https://github.com/grpc/grpc/issues/24549): TSAN won't work here. if (BuiltUnderAsan() || BuiltUnderTsan()) return; + gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutNano = 500000000; const int64_t kTimeoutGrpcTimeoutHeaderMaxSecond = 1; const int64_t kTimeoutMaxStreamDurationSecond = 2; @@ -4393,9 +4394,100 @@ TEST_P(LdsRdsTest, XdsRoutingApplyXdsTimeout) { kTimeoutHttpMaxStreamDurationSecond * 1000000000 + kTimeoutNano); EXPECT_LT(ellapsed_nano_seconds.count(), kTimeoutApplicationSecond * 1000000000); + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); +} + +TEST_P(LdsRdsTest, XdsRoutingXdsTimeoutDisabled) { + const int64_t kTimeoutMillis = 500; + const int64_t kTimeoutNano = kTimeoutMillis * 1000000; + const int64_t kTimeoutGrpcTimeoutHeaderMaxSecond = 1; + const int64_t kTimeoutApplicationSecond = 4; + SetNextResolution({}); + SetNextResolutionForLbChannelAllBalancers(); + // Populate new EDS resources. + AdsServiceImpl::EdsResourceArgs args({ + {"locality0", {g_port_saver->GetPort()}}, + }); + balancers_[0]->ads_service()->SetEdsResource(BuildEdsResource(args)); + RouteConfiguration new_route_config = + balancers_[0]->ads_service()->default_route_config(); + // route 1: Set grpc_timeout_header_max of 1.5 + auto* route1 = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); + auto* max_stream_duration = + route1->mutable_route()->mutable_max_stream_duration(); + auto* duration = max_stream_duration->mutable_grpc_timeout_header_max(); + duration->set_seconds(kTimeoutGrpcTimeoutHeaderMaxSecond); + duration->set_nanos(kTimeoutNano); + SetRouteConfiguration(0, new_route_config); + // Test grpc_timeout_header_max of 1.5 seconds is not applied + gpr_timespec t0 = gpr_now(GPR_CLOCK_MONOTONIC); + gpr_timespec est_timeout_time = gpr_time_add( + t0, gpr_time_from_millis( + kTimeoutGrpcTimeoutHeaderMaxSecond * 1000 + kTimeoutMillis, + GPR_TIMESPAN)); + CheckRpcSendFailure(1, + RpcOptions() + .set_rpc_service(SERVICE_ECHO1) + .set_rpc_method(METHOD_ECHO1) + .set_wait_for_ready(true) + .set_timeout_ms(kTimeoutApplicationSecond * 1000), + StatusCode::DEADLINE_EXCEEDED); + gpr_timespec timeout_time = gpr_now(GPR_CLOCK_MONOTONIC); + EXPECT_GT(gpr_time_cmp(timeout_time, est_timeout_time), 0); +} + +TEST_P(LdsRdsTest, XdsRoutingHttpTimeoutDisabled) { + const int64_t kTimeoutMillis = 500; + const int64_t kTimeoutNano = kTimeoutMillis * 1000000; + const int64_t kTimeoutHttpMaxStreamDurationSecond = 3; + const int64_t kTimeoutApplicationSecond = 4; + SetNextResolution({}); + SetNextResolutionForLbChannelAllBalancers(); + // Populate new EDS resources. + AdsServiceImpl::EdsResourceArgs args({ + {"locality0", {g_port_saver->GetPort()}}, + }); + HttpConnectionManager http_connection_manager; + // Set up HTTP max_stream_duration of 3.5 seconds + auto* duration = + http_connection_manager.mutable_common_http_protocol_options() + ->mutable_max_stream_duration(); + duration->set_seconds(kTimeoutHttpMaxStreamDurationSecond); + duration->set_nanos(kTimeoutNano); + RouteConfiguration new_route_config = + balancers_[0]->ads_service()->default_route_config(); + if (GetParam().enable_rds_testing()) { + auto* rds = http_connection_manager.mutable_rds(); + rds->set_route_config_name(kDefaultRouteConfigurationName); + rds->mutable_config_source()->mutable_ads(); + auto listener = balancers_[0]->ads_service()->default_listener(); + listener.mutable_api_listener()->mutable_api_listener()->PackFrom( + http_connection_manager); + balancers_[0]->ads_service()->SetLdsResource(listener); + SetRouteConfiguration(0, new_route_config); + } else { + *http_connection_manager.mutable_route_config() = new_route_config; + auto listener = balancers_[0]->ads_service()->default_listener(); + listener.mutable_api_listener()->mutable_api_listener()->PackFrom( + http_connection_manager); + balancers_[0]->ads_service()->SetLdsResource(listener); + } + // Test http_stream_duration of 3.5 seconds is not applied + auto t0 = gpr_now(GPR_CLOCK_MONOTONIC); + auto est_timeout_time = gpr_time_add( + t0, gpr_time_from_millis( + kTimeoutHttpMaxStreamDurationSecond * 1000 + kTimeoutMillis, + GPR_TIMESPAN)); + CheckRpcSendFailure(1, + RpcOptions().set_wait_for_ready(true).set_timeout_ms( + kTimeoutApplicationSecond * 1000), + StatusCode::DEADLINE_EXCEEDED); + auto timeout_time = gpr_now(GPR_CLOCK_MONOTONIC); + EXPECT_GT(gpr_time_cmp(timeout_time, est_timeout_time), 0); } TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenXdsTimeoutExplicit0) { + gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutNano = 500000000; const int64_t kTimeoutMaxStreamDurationSecond = 2; const int64_t kTimeoutHttpMaxStreamDurationSecond = 3; @@ -4505,9 +4597,11 @@ TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenXdsTimeoutExplicit0) { system_clock::now() - t0); EXPECT_GT(ellapsed_nano_seconds.count(), kTimeoutApplicationSecond * 1000000000); + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); } TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenHttpTimeoutExplicit0) { + gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutApplicationSecond = 4; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -4552,11 +4646,13 @@ TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenHttpTimeoutExplicit0) { t0); EXPECT_GT(ellapsed_nano_seconds.count(), kTimeoutApplicationSecond * 1000000000); + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); } // Test to ensure application-specified deadline won't be affected when // the xDS config does not specify a timeout. TEST_P(LdsRdsTest, XdsRoutingWithOnlyApplicationTimeout) { + gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutApplicationSecond = 4; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -4575,6 +4671,7 @@ TEST_P(LdsRdsTest, XdsRoutingWithOnlyApplicationTimeout) { t0); EXPECT_GT(ellapsed_nano_seconds.count(), kTimeoutApplicationSecond * 1000000000); + gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); } TEST_P(LdsRdsTest, XdsRoutingHeadersMatching) {