http: add request header timer by akonradi · Pull Request #13341 · envoyproxy/envoy

akonradi · 2020-09-30T22:11:49Z

Commit Message: Add request header timeout to HTTP connection manager
Additional Description:
Add a timer and config option to enforce a maximum allowed duration between when a downstream starts a new stream and when it finishes sending headers on that stream.
Risk Level: low - behavior is disabled unless the config option is specified
Testing: added tests
Docs Changes: autogenerated proto docs
Release Notes: documented new config field

Towards #11427

Add a new config field for an additional timeout that will cancel streams that take too long to send headers, and implement it in the HTTP Connection Manager. Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only · 2020-09-30T22:11:56Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13341 was opened by akonradi.

see: more, trace.

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

alyssawilk

Very cool to see work towards more fine grained (and maybe adaptive) timeouts!

Starting out with a few questions since I want to make sure I know what we're aiming for both here and in the long run

alyssawilk · 2020-10-01T13:29:16Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+  // when the request is initiated, and is disarmed when the last byte of the headers is sent
+  // upstream (i.e. all decoding filters have processed the headers). If not specified or set to 0,
+  // this timeout is disabled.
+  google.protobuf.Duration request_headers_timeout = 41


Hm, this sounds like a different timeout than described in the issue. I think issue is about the client sending headers (not hogging resources with a trickle attack). Going through the filter chain seems to mix Envoy processing time with potentially malicious clients. Did we want the latter as well?

Also, do we think this is a fixed timer long term, or do we hope it will evolve into the ranged timeout in the linked isssue? If so I think we want to start with a different API and hide it until the range timeout is implemented.

I'm going to tag antonio for first pass here as I think he has a bit more context than I do.

+1 I would not recommend making this involve filters at all. I think we want a timer that starts at stream init (1st header block), and ends after all blocks/continuations are complete?

Given ^ it might be easier to configure this at the codec/protocol level?

Agreed, the upstreams bit was a mistake on my part. I think the existing implementation that disables the timer in onHeadersReceived does what we want.

docs/root/version_history/current.rst

antoniovicente · 2020-10-01T14:52:07Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

  google.protobuf.Duration request_timeout = 28
      [(udpa.annotations.security).configure_for_untrusted_downstream = true];

+  // The amount of time that Envoy will wait for the headers to be received. The timer is activated


nit: the request headers

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

docs/protodoc_manifest.yaml

source/common/http/conn_manager_impl.cc

Signed-off-by: Alex Konradi <akonradi@google.com>

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

Call disableTimer() before setting deleting for testing Signed-off-by: Alex Konradi <akonradi@google.com>

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-10-13T20:49:51Z

Windows failure looks to be unrelated.

antoniovicente · 2020-10-14T01:11:15Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+  // The amount of time that Envoy will wait for the request headers to be received. The timer is
+  // activated when the request is initiated, and is disarmed when the last byte of the headers has
+  // been received (i.e. all decoding filters have processed the headers). If not specified or set
+  // to 0, this timeout is disabled.


What happens if this duration is configured to a negative number? Does a proto validator to ensure that durations are >= 0 exist?

It does, fixed.

antoniovicente · 2020-10-14T01:13:15Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

      [(udpa.annotations.security).configure_for_untrusted_downstream = true];

+  // The amount of time that Envoy will wait for the request headers to be received. The timer is
+  // activated when the request is initiated, and is disarmed when the last byte of the headers has


"when the request is initiated" -> "when the first byte of the headers is received"

source/common/http/conn_manager_impl.cc

antoniovicente · 2020-10-14T21:24:46Z

test/common/http/conn_manager_impl_test.cc

+  EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
+    Event::MockTimer* request_header_timer = setUpTimer();
+    EXPECT_CALL(*request_header_timer, enableTimer(request_headers_timeout_, _));
+    EXPECT_CALL(*request_header_timer, disableTimer());


What triggers this disable? the request not parsing and triggering an error reply?

Yeah that was the actual issue. I made the test more restrictive to verify that the correct behavior is being checked.

antoniovicente · 2020-10-14T21:27:33Z

test/common/http/conn_manager_impl_test.cc

+    return Http::okStatus();
+  }));
+
+  Buffer::OwnedImpl fake_input("1234");


Should this input be a partial valid request to avoid potential for header parse error?

Yep, this test is more realistic now.

antoniovicente · 2020-10-14T21:36:11Z

test/common/http/conn_manager_impl_test.cc

+
+    conn_manager_->newStream(response_encoder_);
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_)).Times(2);
+    request_header_timer->invokeCallback();


Invoking this callback from within dispatch doesn't seem like something that would normally happen. Please call this callback outside codec dispatch.

antoniovicente · 2020-10-14T21:37:46Z

test/common/http/conn_manager_impl_test.cc

+        new TestRequestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}};
+
+    // the second parameter 'false' leaves the stream open
+    decoder->decodeHeaders(std::move(headers), false);


There's just too much mocking happening on these "tests". I don't follow what you're testing here or how it relates to the call to conn_manager_->onData below.

That's reasonable, because the tests were bad. PTAL at the cleaned-up version.

Signed-off-by: Alex Konradi <akonradi@google.com>

test/common/http/conn_manager_impl_test.cc

antoniovicente · 2020-10-19T20:57:35Z

source/common/http/conn_manager_impl.h

    Event::TimerPtr stream_idle_timer_;
-    // Per-stream request timeout.
+    // Per-stream request timeout. This timer is enabled when the stream is created and disabled
+    // when the downstream closes the connection. If triggered, it will close the stream.


I think there are some errors in this comment. This seems to be a stream timeout, but there are references to downstream closing the connection.

Thanks, fixed.

This is what was decided on in the PR and I forgot to change it back before now. Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only · 2020-10-29T15:47:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13341 (comment) was created by @akonradi.

see: more, trace.

akonradi · 2020-10-29T17:33:03Z

@alyssawilk did you want to take another pass at this?

alyssawilk

Looks good overall! I've added a few comments but I'm going to throw it over to snow for the non-googler pass

alyssawilk · 2020-10-29T19:21:16Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto


+  // The amount of time that Envoy will wait for the request headers to be received. The timer is
+  // activated when the first byte of the headers is received, and is disarmed when the last byte of
+  // the headers has been received (i.e. all decoding filters have processed the headers). If not


I think the info in parens is not accurate. Maybe just remove?

Thanks, done.

alyssawilk · 2020-10-29T19:24:32Z

source/common/http/conn_manager_impl.h

+    // stream.
+    Event::TimerPtr request_header_timer_;
+    // Per-stream alive duration. This timer is enabled once when the stream is created and, if
+    // triggered, will close the stream.


optional, think it's worth mentioning which of these try to send a reply?

I'm going to punt on this since whether or not a reply is sent for some of these is dependent on a runtime override.

alyssawilk · 2020-10-29T19:26:28Z

test/integration/http_timeout_integration_test.cc

+        request_headers_timeout->set_seconds(1);
+        request_headers_timeout->set_nanos(0);
+      });
+  setDownstreamProtocol(Http::CodecClient::Type::HTTP1);


I'd be inclined to return if the protocol isn't HTTP/1, so we don't run this twice (and the H2 test confusingly runs HTTP/1)

alyssawilk · 2020-10-29T19:29:06Z

test/integration/http_timeout_integration_test.cc

+  // Track locally queued bytes, to make sure the outbound client queue doesn't back up.
+  uint64_t bytes_to_send = send_buffer.length();
+  raw_connection->addBytesSentCallback([&](uint64_t bytes) { bytes_to_send -= bytes; });
+  raw_connection->write(send_buffer, false);


I think you can replace most of this with a call to
sendRawHttpAndWaitForResponse()

This test needs to do stuff to the connection while the dispatcher would otherwise be waiting, but I cribbed the use of RawConnectionDriver and that simplified things.

Signed-off-by: Alex Konradi <akonradi@google.com>

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-11-03T15:50:16Z

@snowp can I get a review on this?

alyssawilk · 2020-11-03T16:03:20Z

Oops - snow is out today - I'll update the maintainer calendar and hopefully he can take a pass tomorrow.

snowp

Thanks this seems good to me, just one test suggestion

snowp · 2020-11-04T15:27:31Z

test/integration/http_timeout_integration_test.cc

+  EXPECT_TRUE(connection_driver->closed());
+  EXPECT_THAT(response, HasSubstr("408"));


Maybe check for the error text as well to make sure that we're hitting the timeout we think we are here? Or check stats

Signed-off-by: Alex Konradi <akonradi@google.com>

snowp

Thanks!

akonradi · 2020-11-04T18:42:20Z

@envoyproxy/api-shepherds review needed for the new timer config

mattklein123

API LGTM. Can you please update https://www.envoyproxy.io/docs/envoy/latest/faq/configuration/timeouts? Thank you.

/wait

Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123

Thanks!

Add request headers timeout to HCM

ba55e0a

Add a new config field for an additional timeout that will cancel streams that take too long to send headers, and implement it in the HTTP Connection Manager. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi requested review from alyssawilk and mattklein123 as code owners September 30, 2020 22:11

repokitteh-read-only bot added the api label Sep 30, 2020

Merge remote-tracking branch 'upstream/master' into http-request-head…

256924b

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 assigned alyssawilk Oct 1, 2020

alyssawilk assigned antoniovicente Oct 1, 2020

alyssawilk reviewed Oct 1, 2020

View reviewed changes

antoniovicente mentioned this pull request Oct 1, 2020

[http] Adaptive timeouts for idle HTTP client connections waiting for a request #11427

Closed

antoniovicente reviewed Oct 1, 2020

View reviewed changes

akonradi added 6 commits October 12, 2020 10:30

Update documentation, fix formatting

97de0f2

Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into http-request-head…

a1e78ee

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

Use unique_ptr::reset() instead of disabling

773ded2

Signed-off-by: Alex Konradi <akonradi@google.com>

Address review feedback

f780c24

Signed-off-by: Alex Konradi <akonradi@google.com>

Revert "Use unique_ptr::reset() instead of disabling"

0735254

Call disableTimer() before setting deleting for testing Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into http-request-head…

068dbde

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente reviewed Oct 14, 2020

View reviewed changes

akonradi added 2 commits October 19, 2020 11:54

Address review feedback

bd8b3c8

Signed-off-by: Alex Konradi <akonradi@google.com>

Add validation annotation

06e150f

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente reviewed Oct 19, 2020

View reviewed changes

akonradi added 5 commits October 23, 2020 13:57

Revert to sendLocalReply

99ec46b

This is what was decided on in the PR and I forgot to change it back before now. Signed-off-by: Alex Konradi <akonradi@google.com>

Add integration test

df9c705

Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into http-request-head…

7924ff3

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

Fix protodoc_manifest.yaml

9e65d85

Signed-off-by: Alex Konradi <akonradi@google.com>

Fix formatting

a28a251

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente added the waiting label Oct 23, 2020

akonradi added 2 commits October 27, 2020 10:55

Merge remote-tracking branch 'upstream/master' into http-request-head…

991e437

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

Don't reset request header timer on timeout

39dc1be

Signed-off-by: Alex Konradi <akonradi@google.com>

alyssawilk reviewed Oct 29, 2020

View reviewed changes

alyssawilk assigned snowp Oct 29, 2020

Use RawConnectionSocket utility for test

97e07fd

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed antoniovicente’s stale review via 97e07fd October 30, 2020 17:49

Merge remote-tracking branch 'upstream/master' into http-request-head…

e1411ff

…er-timer Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi requested review from PiotrSikora, asraa and lizan as code owners October 30, 2020 18:52

snowp suggested changes Nov 4, 2020

View reviewed changes

Check message, fix stream info

576f43b

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi requested a review from ggreenway as a code owner November 4, 2020 15:46

snowp previously approved these changes Nov 4, 2020

View reviewed changes

mattklein123 self-assigned this Nov 4, 2020

mattklein123 requested changes Nov 4, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 4, 2020

Add header timeout to timeouts.rst

b0d5025

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed snowp’s stale review via b0d5025 November 6, 2020 16:33

repokitteh-read-only bot removed the waiting label Nov 6, 2020

mattklein123 approved these changes Nov 9, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Nov 9, 2020

mattklein123 merged commit e1c138b into envoyproxy:master Nov 9, 2020

akonradi deleted the http-request-header-timer branch November 9, 2020 19:42

		EXPECT_TRUE(connection_driver->closed());
		EXPECT_THAT(response, HasSubstr("408"));

Conversation

akonradi commented Sep 30, 2020

Uh oh!

repokitteh-read-only bot commented Sep 30, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akonradi commented Oct 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Oct 29, 2020

Uh oh!

akonradi commented Oct 29, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mattklein123 Oct 1, 2020 •

edited

Loading