diff --git a/api/envoy/api/v2/route/route.proto b/api/envoy/api/v2/route/route.proto index 87232a78eadce..10ba8f6b4b7b4 100644 --- a/api/envoy/api/v2/route/route.proto +++ b/api/envoy/api/v2/route/route.proto @@ -765,6 +765,15 @@ message RouteAction { // time gaps between gRPC request and response in gRPC streaming mode. google.protobuf.Duration max_grpc_timeout = 23 [(gogoproto.stdduration) = true]; + // If present, Envoy will adjust the timeout provided by the `grpc-timeout` header by subtracting + // the provided duration from the header. This is useful in allowing Envoy to set its global + // timeout to be less than that of the deadline imposed by the calling client, which makes it more + // likely that Envoy will handle the timeout instead of having the call canceled by the client. + // The offset will only be applied if the provided grpc_timeout is greater than the offset. This + // ensures that the offset will only ever decrease the timeout and never set it to 0 (meaning + // infinity). + google.protobuf.Duration grpc_timeout_offset = 28 [(gogoproto.stdduration) = true]; + // Allows enabling and disabling upgrades on a per-route basis. // This overrides any enabled/disabled upgrade filter chain specified in the // HttpConnectionManager diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 43be66773e8d1..ffae6ad19ad54 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -13,6 +13,7 @@ Version history * redis: added :ref:`max_buffer_size_before_flush ` to batch commands together until the encoder buffer hits a certain size, and :ref:`buffer_flush_timeout ` to control how quickly the buffer is flushed if it is not full. +* router: add support for configuring a :ref:`grpc timeout offset ` on incoming requests. * router: added ability to control retry back-off intervals via :ref:`retry policy `. * upstream: added :ref:`upstream_cx_pool_overflow ` for the connection pool circuit breaker. diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index acd9738ab08ee..0eed655a2ec8b 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -621,6 +621,13 @@ class RouteEntry : public ResponseEntry { */ virtual absl::optional maxGrpcTimeout() const PURE; + /** + * @return absl::optional the timeout offset to apply to the timeout + * provided by the 'grpc-timeout' header of a gRPC request. This value will be positive and should + * be subtracted from the value provided by the header. + */ + virtual absl::optional grpcTimeoutOffset() const PURE; + /** * Determine whether a specific request path belongs to a virtual cluster for use in stats, etc. * @param headers supplies the request headers. diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 3e359099b22af..b288664db86a9 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -234,6 +234,9 @@ class AsyncStreamImpl : public AsyncClient::Stream, absl::optional maxGrpcTimeout() const override { return absl::nullopt; } + absl::optional grpcTimeoutOffset() const override { + return absl::nullopt; + } const Router::VirtualCluster* virtualCluster(const Http::HeaderMap&) const override { return nullptr; } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index ca8b703c57291..29f06492dc124 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -337,6 +337,7 @@ RouteEntryImplBase::RouteEntryImplBase(const VirtualHostImpl& vhost, timeout_(PROTOBUF_GET_MS_OR_DEFAULT(route.route(), timeout, DEFAULT_ROUTE_TIMEOUT_MS)), idle_timeout_(PROTOBUF_GET_OPTIONAL_MS(route.route(), idle_timeout)), max_grpc_timeout_(PROTOBUF_GET_OPTIONAL_MS(route.route(), max_grpc_timeout)), + grpc_timeout_offset_(PROTOBUF_GET_OPTIONAL_MS(route.route(), grpc_timeout_offset)), loader_(factory_context.runtime()), runtime_(loadRuntimeData(route.match())), scheme_redirect_(route.redirect().scheme_redirect()), host_redirect_(route.redirect().host_redirect()), diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 2ded20d38e4fe..2cf26dbbf3f1b 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -393,6 +393,9 @@ class RouteEntryImplBase : public RouteEntry, absl::optional maxGrpcTimeout() const override { return max_grpc_timeout_; } + absl::optional grpcTimeoutOffset() const override { + return grpc_timeout_offset_; + } const VirtualHost& virtualHost() const override { return vhost_; } bool autoHostRewrite() const override { return auto_host_rewrite_; } const std::multimap& opaqueConfig() const override { @@ -481,6 +484,9 @@ class RouteEntryImplBase : public RouteEntry, absl::optional maxGrpcTimeout() const override { return parent_->maxGrpcTimeout(); } + absl::optional grpcTimeoutOffset() const override { + return parent_->maxGrpcTimeout(); + } const MetadataMatchCriteria* metadataMatchCriteria() const override { return parent_->metadataMatchCriteria(); } @@ -604,6 +610,7 @@ class RouteEntryImplBase : public RouteEntry, const std::chrono::milliseconds timeout_; const absl::optional idle_timeout_; const absl::optional max_grpc_timeout_; + const absl::optional grpc_timeout_offset_; Runtime::Loader& loader_; const absl::optional runtime_; const std::string scheme_redirect_; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 3ea401798ad52..54afd5a1480a1 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -125,6 +125,15 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he if (grpc_request && route.maxGrpcTimeout()) { const std::chrono::milliseconds max_grpc_timeout = route.maxGrpcTimeout().value(); std::chrono::milliseconds grpc_timeout = Grpc::Common::getGrpcTimeout(request_headers); + if (route.grpcTimeoutOffset()) { + // We only apply the offset if it won't result in grpc_timeout hitting 0 or below, as + // setting it to 0 means infinity and a negative timeout makes no sense. + const auto offset = *route.grpcTimeoutOffset(); + if (offset < grpc_timeout) { + grpc_timeout -= offset; + } + } + // Cap gRPC timeout to the configured maximum considering that 0 means infinity. if (max_grpc_timeout != std::chrono::milliseconds(0) && (grpc_timeout == std::chrono::milliseconds(0) || grpc_timeout > max_grpc_timeout)) { diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index b2d2a1589262d..f7a9e36ce6e4f 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1878,6 +1878,36 @@ TEST_F(RouteMatcherTest, ContentType) { } } +TEST_F(RouteMatcherTest, GrpcTimeoutOffset) { + const std::string yaml = R"EOF( +virtual_hosts: +- name: local_service + domains: + - "*" + routes: + - match: + prefix: "/foo" + route: + cluster: local_service_grpc + - match: + prefix: "/" + route: + grpc_timeout_offset: 0.01s + cluster: local_service_grpc + )EOF"; + + TestConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, true); + + { + EXPECT_EQ( + absl::make_optional(std::chrono::milliseconds(10)), + config.route(genHeaders("www.lyft.com", "/", "GET"), 0)->routeEntry()->grpcTimeoutOffset()); + } + EXPECT_EQ(absl::nullopt, config.route(genHeaders("www.lyft.com", "/foo", "GET"), 0) + ->routeEntry() + ->grpcTimeoutOffset()); +} + TEST_F(RouteMatcherTest, FractionalRuntime) { const std::string yaml = R"EOF( virtual_hosts: diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 431b80aecc9e4..1a71d70994ddf 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -2571,6 +2571,28 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_EQ("999m", headers.get_("grpc-timeout")); } + { + NiceMock route; + EXPECT_CALL(route, maxGrpcTimeout()) + .WillRepeatedly(Return(absl::optional(999))); + EXPECT_CALL(route, grpcTimeoutOffset()) + .WillRepeatedly(Return(absl::optional(10))); + Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "100m"}}; + FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout(route, headers, true, true); + EXPECT_EQ(std::chrono::milliseconds(90), timeout.global_timeout_); + EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); + } + { + NiceMock route; + EXPECT_CALL(route, maxGrpcTimeout()) + .WillRepeatedly(Return(absl::optional(999))); + EXPECT_CALL(route, grpcTimeoutOffset()) + .WillRepeatedly(Return(absl::optional(10))); + Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "1m"}}; + FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout(route, headers, true, true); + EXPECT_EQ(std::chrono::milliseconds(1), timeout.global_timeout_); + EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); + } { NiceMock route; EXPECT_CALL(route, maxGrpcTimeout()) diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index f49cd5f719363..2a5d99650c8a5 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -272,6 +272,7 @@ class MockRouteEntry : public RouteEntry { MOCK_CONST_METHOD0(timeout, std::chrono::milliseconds()); MOCK_CONST_METHOD0(idleTimeout, absl::optional()); MOCK_CONST_METHOD0(maxGrpcTimeout, absl::optional()); + MOCK_CONST_METHOD0(grpcTimeoutOffset, absl::optional()); MOCK_CONST_METHOD1(virtualCluster, const VirtualCluster*(const Http::HeaderMap& headers)); MOCK_CONST_METHOD0(virtualHostName, const std::string&()); MOCK_CONST_METHOD0(virtualHost, const VirtualHost&());