http: Internal redirects with request body by derekargueta · Pull Request #15634 · envoyproxy/envoy

derekargueta · 2021-03-23T21:22:57Z

Commit Message: http: Internal redirects with request body
Additional Description: Adds support for handling internal redirects for requests that have a body.
Risk Level: medium - HCM/FM stream recreation
Testing: Added unit and integration tests
Docs Changes: Done
Release Notes: Added
Runtime guard: envoy.reloadable_features.internal_redirects_with_body
Fixes: #14736

Signed-off-by: Derek Argueta darguetap@gmail.com

Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta · 2021-03-23T21:26:43Z

all existing tests pass. I assume this needs a runtime guard since it modifies behavior without a configuration change (hmm, should it be a config option? exclude_requests_with_body or something?) and users may need to adjust buffer limits since any route that has this enabled will need to buffer the request body - don't think there's a way around that since you can't know if it needs a redirect ahead of time.

Still some cleanup to do as well, but wanted to see if I can start getting feedback on the direction cc @alyssawilk @mattklein123

Also thinking that trailers should probably be handled as well?

docs/root/intro/arch_overview/http/http_connection_management.rst

alyssawilk

Looks great overall - thanks for tackling this one! I know we'll be wanting to use it for small bodied requests as well.

docs/root/version_history/current.rst

source/common/http/conn_manager_impl.cc

alyssawilk · 2021-03-24T13:01:33Z

test/integration/redirect_integration_test.cc

+
+  default_request_headers_.setHost("handle.internal.redirect");
+  IntegrationStreamDecoderPtr response =
+      codec_client_->makeRequestWithBody(default_request_headers_, "foobarbizbaz");


Let's also make sure we have a test where we encode headers and partial body, do an early redirect, and downstream sends the rest of the body (or make sure we don't redirect given partial body if you don't want to deal with that yet)

I don't think I want to tackle partial bodies quite yet (happy to do further iterations, but hoping to get the complete-request-body functionality in very soon 🙂).

We still have the condition The request must have been fully processed by Envoy., which is gated by checking that downstream_end_stream_ is true in order to do an internal redirect.

source/common/http/conn_manager_impl.cc

docs/root/intro/arch_overview/http/http_connection_management.rst

test/integration/redirect_integration_test.cc

Signed-off-by: Derek Argueta <darguetap@gmail.com>

source/common/router/router.cc

Signed-off-by: Derek Argueta <darguetap@gmail.com>

alyssawilk

Looking good! Here's a few more thoughts.

docs/root/intro/arch_overview/http/http_connection_management.rst

docs/root/version_history/current.rst

source/common/http/conn_manager_impl.cc

source/common/router/router.cc

source/common/runtime/runtime_features.cc

Signed-off-by: Derek Argueta <darguetap@gmail.com>

alyssawilk · 2021-03-31T13:17:10Z

Hm, looks like that asan failure may be real. Want to check it out?

https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/70794/logs/169

Signed-off-by: Derek Argueta <darguetap@gmail.com>

alyssawilk

Looks great overall! Only minor nits left so @snowp up for second pass?

Also cc @penguingao in case he wants to check it out.

alyssawilk · 2021-03-31T17:01:56Z

source/common/http/filter_manager.cc

  // the stream is complete may differ, re-check bytesReceived() to make sure
  // there was no body from the HCM's point of view.
-  if (!complete() || parent_.stream_info_.bytesReceived() != 0) {
+  if (!parent_.enableInternalRedirectsWithBody() &&


I think we want
if (!complete() || (!parent_.enableInternalRedirectsWithBody() parent_.stream_info_.bytesReceived() != 0)
no?

Ah you're right - the integration test still passed since the router has its own end-of-stream/request-body-present checks that generally prevent us from reaching this point but (as the above comment mentions) this seems to be an extra protection layer against incomplete requests.

I don't see any existing tests for this branch but I can try to put one together - this could occur when the router's downstream_end_stream_ is true but the FM's state_.local_complete_ is false, and if the router's bufferedBody() is false (router doesn't think there's a body) but FM's receivedBytes() is non-zero. Hmmm, tricky cases to test.

test/common/router/router_test.cc

derekargueta · 2021-04-05T22:17:33Z

/retest

repokitteh-read-only · 2021-04-05T22:17:36Z

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

🐱

Caused by: a #15634 (comment) was created by @derekargueta.

see: more, trace.

derekargueta · 2021-04-05T22:39:34Z

hmm, //test/integration:protocol_integration_test failed coverage twice in a row, seemingly for this error

FAIL: //test/integration:protocol_integration_test (shard 2 of 5) (see /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-fastbuild/testlogs/test/integration/protocol_integration_test/shard_2_of_5/test.log)
INFO: From Testing //test/integration:protocol_integration_test (shard 2 of 5):
stdout (/build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/_tmp/actions/stdout-13151) exceeds maximum size of --experimental_ui_max_stdouterr_bytes=1048576 bytes; skipping

https://dev.azure.com/cncf/envoy/_build/results?buildId=71238&view=logs&j=5035ce29-be3c-5fcf-dc50-abb6a265f8b1&t=548c3e59-0417-5971-4f1b-f8545005c291&l=10704

gonna give it one more kick while I try to repro locally

derekargueta · 2021-04-05T22:39:44Z

/retest

repokitteh-read-only · 2021-04-05T22:39:48Z

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

🐱

Caused by: a #15634 (comment) was created by @derekargueta.

see: more, trace.

alyssawilk · 2021-04-06T12:39:48Z

@snowp any other requests as second pass reviewer (once you're back tomorrow)?

snowp

Thanks this looks good in general, just a few questions

source/common/http/conn_manager_impl.cc

source/common/router/router.cc

Signed-off-by: Derek Argueta <darguetap@gmail.com>

…er_.bufferedRequestData() a second time Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta · 2021-04-08T01:16:21Z

/retest

repokitteh-read-only · 2021-04-08T01:16:25Z

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

🐱

Caused by: a #15634 (comment) was created by @derekargueta.

see: more, trace.

derekargueta · 2021-04-08T02:32:44Z

/retest

repokitteh-read-only · 2021-04-08T02:32:48Z

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

🐱

Caused by: a #15634 (comment) was created by @derekargueta.

see: more, trace.

alyssawilk · 2021-04-08T12:53:30Z

verify examples got broken last night by a g-c-p change - main merge should now fix!

Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta · 2021-04-08T18:38:13Z

/retest

repokitteh-read-only · 2021-04-08T18:38:18Z

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

🐱

Caused by: a #15634 (comment) was created by @derekargueta.

see: more, trace.

snowp

LGTM, thanks!

derekargueta added 2 commits March 23, 2021 14:15

inital working implementation of internal redirects with request body

062ba89

Signed-off-by: Derek Argueta <darguetap@gmail.com>

docs updates

9c78a33

Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta commented Mar 23, 2021

View reviewed changes

docs/root/intro/arch_overview/http/http_connection_management.rst Show resolved Hide resolved

alyssawilk self-assigned this Mar 24, 2021

alyssawilk reviewed Mar 24, 2021

View reviewed changes

alyssawilk added the waiting label Mar 25, 2021

derekargueta added 4 commits March 26, 2021 12:08

add runtime guard for internal redirects with request body

121897e

Signed-off-by: Derek Argueta <darguetap@gmail.com>

ensure that full body was read in integration test

7ff59b2

Signed-off-by: Derek Argueta <darguetap@gmail.com>

handle request buffer overflows for internal redirects

be591ab

Signed-off-by: Derek Argueta <darguetap@gmail.com>

Merge branch 'main' into internal-redirects-with-request-body

cfcf021

Signed-off-by: Derek Argueta <darguetap@gmail.com>

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

update docs with reuqirements for internal redirects

faac7a8

Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta commented Mar 26, 2021

View reviewed changes

source/common/router/router.cc Outdated Show resolved Hide resolved

derekargueta commented Mar 26, 2021

View reviewed changes

source/common/router/router.cc Outdated Show resolved Hide resolved

derekargueta added 4 commits March 26, 2021 16:19

update router unit tests

fca3fb1

Signed-off-by: Derek Argueta <darguetap@gmail.com>

fix format

954b790

Signed-off-by: Derek Argueta <darguetap@gmail.com>

learned that current.rst is alphabetically sorted

30aad9e

Signed-off-by: Derek Argueta <darguetap@gmail.com>

add missing mock

25c766a

Signed-off-by: Derek Argueta <darguetap@gmail.com>

alyssawilk reviewed Mar 29, 2021

View reviewed changes

derekargueta added 4 commits March 29, 2021 19:32

review feedback

877f90e

Signed-off-by: Derek Argueta <darguetap@gmail.com>

Merge branch 'main' into internal-redirects-with-request-body

22523c1

Signed-off-by: Derek Argueta <darguetap@gmail.com>

fix tests

9a99cef

Signed-off-by: Derek Argueta <darguetap@gmail.com>

docs note about trailers

d35327c

Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta marked this pull request as ready for review March 31, 2021 01:10

fix formatting

c8bfad1

Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta added 2 commits March 31, 2021 09:33

attempt ASAN fix

920b32c

Signed-off-by: Derek Argueta <darguetap@gmail.com>

more sure fix for ASAN

47ff9bf

Signed-off-by: Derek Argueta <darguetap@gmail.com>

alyssawilk reviewed Mar 31, 2021

View reviewed changes

derekargueta requested a review from snowp April 5, 2021 23:25

snowp reviewed Apr 7, 2021

View reviewed changes

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

source/common/router/router.cc Outdated Show resolved Hide resolved

derekargueta added 2 commits April 7, 2021 08:27

review feedback: collapse if-statements and snap runtime value to filter

014fbc5

Signed-off-by: Derek Argueta <darguetap@gmail.com>

Merge branch 'main' into internal-redirects-with-request-body

0aade8d

Signed-off-by: Derek Argueta <darguetap@gmail.com>

derekargueta dismissed alyssawilk’s stale review via 0aade8d April 7, 2021 15:29

derekargueta requested review from alyssawilk and snowp April 7, 2021 15:33

derekargueta added 3 commits April 7, 2021 11:54

Merge branch 'main' into internal-redirects-with-request-body

a4e68e2

Signed-off-by: Derek Argueta <darguetap@gmail.com>

add ASSERT_TRUE for waitForEndStream() as done in d6acb14

13cdac0

Signed-off-by: Derek Argueta <darguetap@gmail.com>

re-use buffered_request_data variable instead of calling filter_manag…

721938f

…er_.bufferedRequestData() a second time Signed-off-by: Derek Argueta <darguetap@gmail.com>

Merge branch 'main' into internal-redirects-with-request-body

44c8594

Signed-off-by: Derek Argueta <darguetap@gmail.com>

snowp approved these changes Apr 8, 2021

View reviewed changes

snowp merged commit 9c6a44c into envoyproxy:main Apr 8, 2021

derekargueta deleted the internal-redirects-with-request-body branch April 8, 2021 23:23

alyssawilk mentioned this pull request Aug 31, 2021

OnDemand VHDS makes the first request returns 404. #17891

Closed

alyssawilk mentioned this pull request Jan 12, 2022

envoy.reloadable_features.internal_redirects_with_body deprecation #19506

Closed

adisuissa mentioned this pull request Sep 2, 2025

POST/PUT requests with body data got 404 responses during VHDS route discovery #40883

Closed

Conversation

derekargueta commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekargueta commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alyssawilk Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

derekargueta Mar 26, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk commented Mar 31, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

derekargueta Mar 31, 2021 • 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!

Uh oh!

derekargueta commented Apr 5, 2021

Uh oh!

repokitteh-read-only bot commented Apr 5, 2021

Uh oh!

derekargueta commented Apr 5, 2021

Uh oh!

derekargueta commented Apr 5, 2021

Uh oh!

repokitteh-read-only bot commented Apr 5, 2021

Uh oh!

alyssawilk commented Apr 6, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

derekargueta commented Apr 8, 2021

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

derekargueta commented Apr 8, 2021

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

alyssawilk commented Apr 8, 2021

Uh oh!

derekargueta commented Apr 8, 2021

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

snowp left a comment

derekargueta commented Mar 23, 2021 •

edited

Loading

derekargueta commented Mar 23, 2021 •

edited

Loading

derekargueta Mar 26, 2021 •

edited

Loading

derekargueta Mar 31, 2021 •

edited

Loading