route config: Allow to configure direct response body size by dio · Pull Request #14778 · envoyproxy/envoy

dio · 2021-01-21T06:03:11Z

Commit Message: This patch adds max_direct_response_body_size_bytes to set the maximum bytes of the direct response body size (in bytes). The config is added as a field in RouteConfiguration.

Additional Description: Reviving #13487 with a slightly different approach (add the config to RouteConfiguration instead of directly per direct response config entry).

Risk Level: Low, since the default behavior is preserved.
Testing: Updated to test the newly introduced config.
Docs Changes: Updated.
Release Notes: Added.
Fixes #13422

Signed-off-by: Dhi Aurrahman dio@rockybars.com

This patch allows configuring direct response body size, by setting route config max_direct_response_body_size_bytes. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

repokitteh-read-only · 2021-01-21T06:03:18Z

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

🐱

Caused by: #14778 was opened by dio.

see: more, trace.

htuch

Thanks for the PR @dio! I'll take this review since @mattklein123 is out.
/wait

htuch · 2021-01-22T16:48:48Z

api/envoy/config/route/v3/route.proto

+  // .. warning::
+  //
+  //   The content of :ref:`direct response body
+  //   <envoy_api_field_config.route.v3.DirectResponseAction.body>` will be held in memory.


Can you be more specific here? I.e. is the concern it's held in configuration memory, or in buffers on the response path, etc.

Tried to add more description on this. Please let me know if I put it wrongly.

api/envoy/config/route/v3/route.proto

source/common/router/config_utility.h

test/common/router/config_impl_test.cc

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch

LGTM modulo comment in route.proto. Thanks!
/wait

htuch · 2021-01-25T02:48:16Z

api/envoy/config/route/v3/route.proto

-  //   <envoy_api_field_config.route.v3.DirectResponseAction.body>` will be held in memory.
+  //   Envoy currently holds the content of :ref:`direct response body
+  //   <envoy_api_field_config.route.v3.DirectResponseAction.body>` in memory. Be careful setting
+  //   this to be larger than the default 4KB. The default is limited to 4KB to keep the proxy's


Is this the only reason? My understanding is we also want to avoid flooding watermarked buffers. E.g. if the buffer limits are 128KB and we dump 10MB in a direct response, that will be queued. @alyssawilk for thoughts.

Oh, this is a gem. Thanks, @htuch! I didn't know that. I will do more digging. And, yes, I'll be very thankful if @alyssawilk can give some thoughts on this.

I don't think it'd be queued, I think we'd disable reading from a nonexistent upstream (and risk OOMs if there were lots of direct responses)
I think it'd be fine to allow larger responses (though I'd be inclined to add a warning note in the API about it not being subject to memory guards so use with care) but suggest an integration test to make sure it all works as I think it should :-)

I have added "standard" cases to the direct response integration test. If you think we need to cover more cases, please let me know. re: Buffer limit, I have a test case that sends 1000 * 4K so I think it's enough.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch

LGTM, but defer to @alyssawilk for a pass/merge.

htuch · 2021-01-27T01:43:13Z

api/envoy/config/route/v3/route.proto

+  //   Envoy currently holds the content of :ref:`direct response body
+  //   <envoy_api_field_config.route.v3.DirectResponseAction.body>` in memory. Be careful setting
+  //   this to be larger than the default 4KB, since the allocated memory for direct response body
+  //   is not being subject to memory guards.


I might phrase this as "data plane buffering controls" rather than "memory guards".

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2021-01-27T09:24:35Z

/retest

repokitteh-read-only · 2021-01-27T09:24:40Z

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

🐱

Caused by: a #14778 (comment) was created by @dio.

see: more, trace.

alyssawilk

Looks good overall - I'm sure this will be really useful to a lot of users - thanks for adding it!

api/envoy/config/route/v3/route.proto

docs/root/version_history/current.rst

alyssawilk · 2021-01-27T19:08:39Z

test/integration/direct_response_integration_test.cc

+
+TEST_P(DirectResponseIntegrationTest, DirectResponseBodySizeLarge) {
+  // Test with a large direct response body size.
+  testDirectResponseBodySize(1000 * 4096);


mind also setting
config_helper_.setBufferLimits(1024, 1024);
for this one?

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

alyssawilk · 2021-02-01T21:03:35Z

docs/root/version_history/current.rst

 * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false.
 * overload: add support for scaling :ref:`transport connection timeouts<envoy_v3_api_enum_value_config.overload.v3.ScaleTimersOverloadActionConfig.TimerType.TRANSPORT_SOCKET_CONNECT>`. This can be used to reduce the TLS handshake timeout in response to overload.
+* route config: added :ref:`max_direct_response_body_size_bytes <envoy_v3_api_field_config.route.v3.RouteConfiguration.max_direct_response_body_size_bytes>` to set maximum :ref:`direct response body <envoy_v3_api_field_config.route.v3.DirectResponseAction.body>` size in bytes. If not specified the default remains 4096 bytes.
+* server: added :ref:`fips_mode <statistics>` statistic.


is this a bad merge?

Yes, sorry. This is updated.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch

LGTM, thanks!

dio added 2 commits January 21, 2021 05:50

route config: Allow to configure direct response body size

4a47266

This patch allows configuring direct response body size, by setting route config max_direct_response_body_size_bytes. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove

246b560

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

repokitteh-read-only bot added the api label Jan 21, 2021

repokitteh-read-only bot assigned mattklein123 Jan 21, 2021

Merge remote-tracking branch 'upstream/main'

b6d6498

htuch suggested changes Jan 22, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 22, 2021

htuch assigned htuch and unassigned mattklein123 Jan 22, 2021

Review comments

a053adc

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

repokitteh-read-only bot removed the waiting label Jan 22, 2021

Missing v3

f6392e7

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch reviewed Jan 25, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 25, 2021

Add integration test

06f6ddf

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

repokitteh-read-only bot removed the waiting label Jan 26, 2021

dio added 6 commits January 26, 2021 02:17

Comment update

047caa5

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/main'

e2114cd

Fix

15c7732

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Format

dad5001

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Order

1455ce6

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/main'

4b85eb3

htuch reviewed Jan 27, 2021

View reviewed changes

Update comment

84ebd6a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

alyssawilk self-assigned this Jan 27, 2021

alyssawilk reviewed Jan 27, 2021

View reviewed changes

Review comments

ce8a6b2

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

alyssawilk previously approved these changes Jan 28, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main'

41caa4e

dio dismissed alyssawilk’s stale review via 41caa4e February 1, 2021 20:32

alyssawilk reviewed Feb 1, 2021

View reviewed changes

dio added 2 commits February 1, 2021 21:36

Merge remote-tracking branch 'upstream/main'

0e1d85d

Fix

6ce6bd6

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio requested a review from htuch February 1, 2021 21:39

alyssawilk approved these changes Feb 1, 2021

View reviewed changes

htuch approved these changes Feb 2, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Feb 2, 2021

htuch merged commit eeb7adc into envoyproxy:main Feb 2, 2021

Conversation

dio commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 21, 2021

Uh oh!

htuch 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!

Uh oh!

Uh oh!

Uh oh!

htuch 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!

htuch 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!

dio commented Jan 27, 2021

Uh oh!

repokitteh-read-only bot commented Jan 27, 2021

Uh oh!

alyssawilk left a comment

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dio commented Jan 21, 2021 •

edited

Loading