honour routes timeout if max stream duration is set with out stream duration #15585

ramaraochavali · 2021-03-20T09:06:31Z

Commit Message: honour routes timeout if maxstream duration is set with out specifying max stream duration inside it.
Additional Description:
Risk Level: Low
Testing: Added
Docs Changes: N/A
Release Notes: N/A (There is already a bug fix mentioned for MaxStreamDuration in current.rst)
Platform Specific Features: N/A
Fixes #15530

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali · 2021-03-20T09:07:04Z

@mattklein123 @markdroth PTAL

Signed-off-by: Rama Chavali <[email protected]>

jmarantz · 2021-03-21T14:35:16Z

@adamsjob FYI

mattklein123 · 2021-03-22T16:48:57Z

Assigning over to @alyssawilk who I think reviewed the other timeout PRs.

alyssawilk

Thanks for catching and fixing this!

alyssawilk · 2021-03-22T16:53:42Z

source/common/router/router.cc

@@ -156,6 +156,13 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::RequestHeaderMap& req
    } else {
      timeout.global_timeout_ = route.timeout();
    }
+  } else {


When we switched timeouts, we moved the instantiation of the timeout to
ConnectionManagerImpl::ActiveStream::refreshDurationTimeout()

Mind moving this logic there?

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali · 2021-03-24T09:27:16Z

test/common/http/conn_manager_impl_test.cc

+// Test timeout when route timeout needs to be used if max_stream_duration is not specified.
+// Regression test for https://github.com/envoyproxy/envoy/issues/15530.
+// TODO(ramaraochavali): move this test to DurationTimeout test.
+TEST_F(HttpConnectionManagerImplTest, DurationTimeoutFromRouteTimeout) {


I had to create a separate test for this because clusterInfo is repeatedly called and it keeps triggering the clearRouteCache so the timer gets fired multiple times. The same problem exists for https://github.com/envoyproxy/envoy/blob/main/test/common/http/conn_manager_impl_test.cc#L2173 where timer is repeatedly enabled. Having two such tests in the same test is not working. So I made it as separate test and TODO to look at the issue later.

ramaraochavali · 2021-03-24T09:27:54Z

@alyssawilk moved to refreshDurationTimeout. PTAL.

Signed-off-by: Rama Chavali <[email protected]>

markdroth · 2021-03-24T14:59:41Z

Alyssa knows this code a lot better than I do, so I'll let her review.

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali · 2021-03-25T11:05:29Z

@alyssawilk can you PTAL when you get chance? Also the failure seems not related to this change

alyssawilk · 2021-03-25T16:53:39Z

source/common/http/conn_manager_impl.cc

+    // If we are using new timeouts and MaxStreamDuration is not set, use the route's timeout
+    // value. This is the case when route's MaxStreamDuration is specified without inner
+    // MaxStreamDuration i.e. only grpc_timeout_header_max is set.
+    if (!route->maxStreamDuration()) {


so if grpc_timeout_header_max is set when do we use it? usingNewTimeouts would be true and we'd ignore all the code below, no?

Can you please fix and regression test?

Oh, Yeah.. Missed that. Will add a test

@alyssawilk Can you PTAL at the fix to see if this is correct? One thing I found was this needs changes to most of the test cases because now we will be initiating request timer most of the times. So I just want to ensure this is the right direction before I make changes the tests(some of which I have already done - please take a look at the changes)

Signed-off-by: Rama Chavali <[email protected]>

alyssawilk · 2021-03-30T16:40:07Z

source/common/http/conn_manager_impl.cc

  std::chrono::milliseconds timeout;
  bool disable_timer = false;
+  auto grpc_timeout = Grpc::Common::getGrpcTimeout(*request_headers_);


Offhand I recall debate around the original PR about how the new fields should be applied and it was the consensus from the gRPC folks that the difference wasn't "is this a gRPC request or not" but "is the gRPC timeout header present or not" so I think switching to this is against the intent of the folks who added this to the API.

Got it. This is good to know. I will change it accordingly

Awesome. It's super complicated and confusing, so thanks for continuing to plug away at it!

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali · 2021-04-02T09:25:46Z

@alyssawilk Sorry for the delay. Fixed. PTAL. I have added a regression test for this and left a TODO to merge to DurationTimeout test later as I am having some issues with mock timer (even for existing tests also this problem exists).

Signed-off-by: Rama Chavali <[email protected]>

source/common/http/conn_manager_impl.cc

antoniovicente

Thanks for the fix to timeout config handling.

ramaraochavali · 2021-04-08T04:09:11Z

@alyssawilk Can you PTAL when you get chance?

alyssawilk

Ah sorry, I thought you meant to address the test TODO in this PR. I'm fine with it being a follow-up!

ramaraochavali · 2021-04-21T15:31:13Z

@alyssawilk @antoniovicente one thing I realized is when timeout happens with new timeouts we call https://github.com/envoyproxy/envoy/blob/main/source/common/http/conn_manager_impl.cc#L762 and that sets http status to 408 (RequestTimeout) instead of gateway timeout (504) that router sets. Is this expected ? or do we need to change it to return 504 for timeouts? if it is expected - should we document it some where?

Adds a runtime flag for disabling the behavior introduced in envoyproxy#15585 Signed-off-by: Snow Pettersen <[email protected]>

…stream duration (envoyproxy#15585)" This reverts commit ffa1680. Signed-off-by: Snow Pettersen <[email protected]>

…stream duration (#15585)" (#16133) This reverts commit ffa1680. Signed-off-by: Snow Pettersen <[email protected]>

rgs1 · 2021-04-27T22:20:33Z

FYI this broke web sockets for us (by forcing them to terminate earlier), wondering if we can contribute more integration tests here to catch these sort of regressions.

…stream duration (#15585)" (#16289) This reverts commit ffa1680. Signed-off-by: Dmitri Dolguikh <[email protected]>

…stream duration (envoyproxy#15585)" (envoyproxy#16133) This reverts commit ffa1680. Signed-off-by: Snow Pettersen <[email protected]> Signed-off-by: Gokul Nair <[email protected]>

fix timeout

8228faa

Signed-off-by: Rama Chavali <[email protected]>

Merge remote-tracking branch 'upstream/main' into fix/timeout

92d3a18

Signed-off-by: Rama Chavali <[email protected]>

jmarantz assigned markdroth Mar 21, 2021

mattklein123 assigned alyssawilk Mar 22, 2021

alyssawilk reviewed Mar 22, 2021

View reviewed changes

alyssawilk added the waiting label Mar 23, 2021

move to refreshduration

e68a3cf

Signed-off-by: Rama Chavali <[email protected]>

repokitteh-read-only bot removed the waiting label Mar 24, 2021

ramaraochavali added 2 commits March 24, 2021 14:51

remove unnecessary couts

e5d8667

Signed-off-by: Rama Chavali <[email protected]>

remove unnecessary remove

fe417ff

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali commented Mar 24, 2021

View reviewed changes

ramaraochavali added 2 commits March 24, 2021 16:19

kick ci

48afbe0

Signed-off-by: Rama Chavali <[email protected]>

Merge remote-tracking branch 'upstream/main' into fix/timeout

43642eb

Signed-off-by: Rama Chavali <[email protected]>

markdroth removed their assignment Mar 24, 2021

Merge remote-tracking branch 'upstream/main' into fix/timeout

9f7e3ca

Signed-off-by: Rama Chavali <[email protected]>

alyssawilk reviewed Mar 25, 2021

View reviewed changes

alyssawilk added the waiting label Mar 25, 2021

wip fix

c1be6ae

Signed-off-by: Rama Chavali <[email protected]>

repokitteh-read-only bot removed the waiting label Mar 30, 2021

alyssawilk reviewed Mar 30, 2021

View reviewed changes

fix properly and add test

9a02bf9

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali added 2 commits April 2, 2021 16:46

comment

4f894bb

Signed-off-by: Rama Chavali <[email protected]>

remove unnecessary removal

30630fc

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali added 2 commits April 2, 2021 18:45

format

b7e2b4d

Signed-off-by: Rama Chavali <[email protected]>

fix

d621a4b

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali requested a review from dio as a code owner April 2, 2021 13:18

unnecessary removal

0314715

Signed-off-by: Rama Chavali <[email protected]>

alyssawilk reviewed Apr 5, 2021

View reviewed changes

source/common/http/conn_manager_impl.cc Show resolved Hide resolved

alyssawilk assigned antoniovicente Apr 5, 2021

antoniovicente approved these changes Apr 7, 2021

View reviewed changes

ramaraochavali mentioned this pull request Apr 7, 2021

Regression: istio.io egress test case gets 408 instead of 504 istio/istio#31616

Closed

alyssawilk approved these changes Apr 8, 2021

View reviewed changes

alyssawilk merged commit ffa1680 into envoyproxy:main Apr 8, 2021

ramaraochavali deleted the fix/timeout branch April 8, 2021 14:51

ericvn mentioned this pull request Apr 14, 2021

[release-1.10] Update Envoy version - Manual CP of #3284 istio/proxy#3287

Merged

TAOXUY mentioned this pull request Apr 20, 2021

b/184142099: upgrade envoy to 18.2 GoogleCloudPlatform/esp-v2#501

Merged

snowp mentioned this pull request Apr 22, 2021

Inconsistent timeout handling between max_stream_timeout and router global timeout #16129

Open

snowp added a commit to snowp/envoy that referenced this pull request Apr 22, 2021

http: retroactively add runtime flag for using timeout change

a758746

Adds a runtime flag for disabling the behavior introduced in envoyproxy#15585 Signed-off-by: Snow Pettersen <[email protected]>

snowp mentioned this pull request Apr 22, 2021

http: retroactively add runtime flag for timeout change #16131

Closed

snowp added a commit to snowp/envoy that referenced this pull request Apr 22, 2021

Revert "honour routes timeout if max stream duration is set with out …

8d7355d

…stream duration (envoyproxy#15585)" This reverts commit ffa1680. Signed-off-by: Snow Pettersen <[email protected]>

snowp mentioned this pull request Apr 22, 2021

Revert "honour routes timeout if max stream duration is set with out … #16133

Merged

snowp added a commit that referenced this pull request Apr 23, 2021

Revert "honour routes timeout if max stream duration is set with out …

f43a78e

…stream duration (#15585)" (#16133) This reverts commit ffa1680. Signed-off-by: Snow Pettersen <[email protected]>

dmitri-d mentioned this pull request May 3, 2021

[1.18] Revert "honour routes timeout if max stream duration is set with out … #16289

Merged

htuch pushed a commit that referenced this pull request May 4, 2021

Revert "honour routes timeout if max stream duration is set with out …

6bcb3a0

…stream duration (#15585)" (#16289) This reverts commit ffa1680. Signed-off-by: Dmitri Dolguikh <[email protected]>

sunjayBhatia mentioned this pull request May 20, 2021

Websocket Timeout projectcontour/contour#3692

Closed

ramaraochavali mentioned this pull request Nov 17, 2022

Fix setting route timeout through envoyfilter istio/istio#42035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

honour routes timeout if max stream duration is set with out stream duration #15585

honour routes timeout if max stream duration is set with out stream duration #15585

ramaraochavali commented Mar 20, 2021

ramaraochavali commented Mar 20, 2021

jmarantz commented Mar 21, 2021

mattklein123 commented Mar 22, 2021

alyssawilk left a comment

alyssawilk Mar 22, 2021

ramaraochavali Mar 24, 2021

ramaraochavali commented Mar 24, 2021

markdroth commented Mar 24, 2021

ramaraochavali commented Mar 25, 2021 •

edited

Loading

alyssawilk Mar 25, 2021

ramaraochavali Mar 26, 2021

ramaraochavali Mar 30, 2021

alyssawilk Mar 30, 2021

ramaraochavali Mar 31, 2021

alyssawilk Mar 31, 2021

ramaraochavali commented Apr 2, 2021 •

edited

Loading

antoniovicente left a comment

ramaraochavali commented Apr 8, 2021

alyssawilk left a comment

ramaraochavali commented Apr 21, 2021

rgs1 commented Apr 27, 2021

honour routes timeout if max stream duration is set with out stream duration #15585

honour routes timeout if max stream duration is set with out stream duration #15585

Conversation

ramaraochavali commented Mar 20, 2021

ramaraochavali commented Mar 20, 2021

jmarantz commented Mar 21, 2021

mattklein123 commented Mar 22, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Mar 24, 2021

markdroth commented Mar 24, 2021

ramaraochavali commented Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Apr 2, 2021 • edited Loading

antoniovicente left a comment

Choose a reason for hiding this comment

ramaraochavali commented Apr 8, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

ramaraochavali commented Apr 21, 2021

rgs1 commented Apr 27, 2021

ramaraochavali commented Mar 25, 2021 •

edited

Loading

ramaraochavali commented Apr 2, 2021 •

edited

Loading