hcm: fix sending local reply during encode + grpc request classification by snowp · Pull Request #13256 · envoyproxy/envoy

snowp · 2020-09-24T15:47:11Z

Fixes two issues that surfaced when adding an integration test for the
gRPC reverse bridge:

when sending a local reply during the encoding path, local_complete_
is set to true which results in endStream being called twice. We fix
this by not calling endStream during the header/body callback when
local_complete_ is true
the direct reply function was not using the state variable to detect
is_grpc_request, which meant that for the reverse bridge is was using
the modified headers which no longer have a gRPC content type.

Signed-off-by: Snow Pettersen snowp@lyft.com

Risk Level: Medium, HCM changes
Testing: New integration test
Docs Changes: n/a
Release Notes: n/a

Fixes two issues that surfaced when adding an integration test for the gRPC reverse bridge: 1) when sending a local reply during the encoding path, local_complete_ is set to true which results in endStream being called twice. We fix this by not calling endStream during the header/body callback when local_complete_ is true 2) the direct reply function was not using the state variable to detect is_grpc_request, which meant that for the reverse bridge is was using the modified headers which no longer have a gRPC content type. Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp · 2020-09-24T15:47:55Z

Locally this seems to work and cause no issues with integration tests, lmk if you want to proceed with this or revert the other PR @mattklein123

snowp · 2020-09-24T15:50:36Z

@zuercher Turns out there was a bug around text/plain as well, because during integration tests we end up taking a different flow and hit code that wasn't checking the state variable but instead checked the request headers that we'd already modified to not be grpc anymore.

Lesson to be learnt: integration test it!

mattklein123

At a high level this makes sense to me. But I would like to see:

conn_manager_impl unit tests
non-extension specific integration tests

If it's easier to revert the other PR for now and re-apply with ^ that's fine with me. Thank you!

/wait

mattklein123 · 2020-09-24T21:09:15Z

source/common/http/filter_manager.cc

            filter_manager_callbacks_.encodeHeaders(*filter_manager_callbacks_.responseHeaders(),
                                                    end_stream);
-            maybeEndEncode(end_stream);
+            maybeEndEncode(end_stream && !state_.local_complete_);


Sorry where is local_complete_ set to true? Before the filter chain starts iterating? This is not super intuitive. Can you add more comments?

but why do we need to sometimes end encode after encode headers / encode data, and sometimes outside of the EncodeFunctions? Can't we just remove the one at the end, or remove these two calls?

I looked through the code and I think it was there because the call to encode headers could trigger a reset, in which case encodeData wouldn't be called, so in that case endStream wouldn't be called. Calling reset would call local_complete_, and so endStream would be called at the end of the function.

That said, I tried removing it and no integration tests seem to fail. Seems like doEndStream is called as part of reset anyways, so it might not be necessary? Pushed this change, so let's see if CI passes

snowp · 2020-09-24T21:15:46Z

Yeah that's reasonable, let's revert the other PR now to keep HEAD healthy.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

yanavlasov · 2020-09-29T15:18:10Z

test/integration/filters/local_reply_during_encoding_filter.cc

+  constexpr static char name[] = "local-reply-during-encode";
+
+  Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap&, bool) override {
+    decoder_callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr, absl::nullopt,


Would it make sense to add sendLocalReply to the StreamEncoderFilterCallbacks interface?

+1 per slack convo also

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123

Thanks LGTM other than remaining comments.

/wait

mattklein123 · 2020-09-30T16:27:11Z

test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc

  ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
 }

-TEST_P(ReverseBridgeIntegrationTest, EnabledRouteBadContentType) {


FWIW I don't see the harm of leaving this test.

I was thinking that it wouldn't pass without the other PR but I'm not sure if that's true, I'll try adding it back

Oh sorry I forgot about that. OK if you want to add it back there that's fine.

I was able to get it working, seems like it just tested an existing feature so it passes now and should nicely serve as a regression test to make sure that we're not breaking this feature with the change to use sendLocalReply in the future

mattklein123 · 2020-09-30T16:27:26Z

test/integration/filters/local_reply_during_encoding_filter.cc

+  constexpr static char name[] = "local-reply-during-encode";
+
+  Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap&, bool) override {
+    decoder_callbacks_->sendLocalReply(Http::Code::InternalServerError, "", nullptr, absl::nullopt,


+1 per slack convo also

This reverts commit 2f4d2f6. Signed-off-by: Snow Pettersen <snowp@lyft.com>

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123

Awesome, thanks!

snowp · 2020-10-01T11:46:14Z

/retest

repokitteh-read-only · 2020-10-01T11:46:19Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13256 (comment) was created by @snowp.

see: more, trace.

snowp · 2020-10-01T11:54:11Z

@alyssawilk do you wanna take a look at this as well?

alyssawilk

Looks good overall! Really only the one question and the test nits.

alyssawilk · 2020-10-01T12:45:37Z

source/common/http/filter_manager.cc

            filter_manager_callbacks_.encodeHeaders(*filter_manager_callbacks_.responseHeaders(),
                                                    end_stream);
-            maybeEndEncode(end_stream);
+            maybeEndEncode(end_stream && !state_.local_complete_);


but why do we need to sometimes end encode after encode headers / encode data, and sometimes outside of the EncodeFunctions? Can't we just remove the one at the end, or remove these two calls?

alyssawilk · 2020-10-01T12:50:33Z

test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc

+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+
+  // Ensure that we stripped the length prefix and set the appropriate headers.
+  EXPECT_EQ("f", upstream_request_->body().toString());


I think a bunch of these are tested in another test.

WDYT of just using sendRequestAndWaitForReponse() and cutting out all the duplicate stuff?

alyssawilk · 2020-10-01T12:51:49Z

test/integration/protocol_integration_test.cc

+
+  // Wait for the upstream request and begin sending a response with end_stream = false.
+  waitForNextUpstreamRequest();
+  upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "503"}}, true);


again I think you can just sendRequestAndWaitForResponse with default request and response headers, since all you want to do is make sure the status code is set correctly.

Signed-off-by: Snow Pettersen <snowp@lyft.com>

alyssawilk

LGTM modulo ci investigation

snowp · 2020-10-06T17:12:21Z

/retests

snowp · 2020-10-06T17:12:41Z

/retest

repokitteh-read-only · 2020-10-06T17:12:46Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13256 (comment) was created by @snowp.

see: more, trace.

snowp requested a review from zuercher as a code owner September 24, 2020 15:47

snowp assigned mattklein123 Sep 24, 2020

mattklein123 requested changes Sep 24, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 24, 2020

Snow Pettersen added 7 commits September 28, 2020 16:46

Merge remote-tracking branch 'envoy/master' into fix-local-reply-grpc

bf7d9a4

Signed-off-by: Snow Pettersen <snowp@lyft.com>

add unit tests for filter manager

a83f841

Signed-off-by: Snow Pettersen <snowp@lyft.com>

format

7075903

Signed-off-by: Snow Pettersen <snowp@lyft.com>

add protocol test

c25fbf4

Signed-off-by: Snow Pettersen <snowp@lyft.com>

Merge remote-tracking branch 'envoy/master' into fix-local-reply-grpc

b6e60bd

Signed-off-by: Snow Pettersen <snowp@lyft.com>

revert grpc bridge test

2f4d2f6

Signed-off-by: Snow Pettersen <snowp@lyft.com>

add test filter

2965e02

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only bot removed the waiting label Sep 29, 2020

add comments

701c360

Signed-off-by: Snow Pettersen <snowp@lyft.com>

yanavlasov requested changes Sep 29, 2020

View reviewed changes

Snow Pettersen added 2 commits September 29, 2020 21:34

ci

7cad387

Signed-off-by: Snow Pettersen <snowp@lyft.com>

make tests more lenient

21d3cae

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123 requested changes Sep 30, 2020

View reviewed changes

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

Snow Pettersen added 5 commits September 30, 2020 19:34

Revert "revert grpc bridge test"

03c9af7

This reverts commit 2f4d2f6. Signed-off-by: Snow Pettersen <snowp@lyft.com>

fix test

46a362b

Signed-off-by: Snow Pettersen <snowp@lyft.com>

add encoding filter callback

8e65501

Signed-off-by: Snow Pettersen <snowp@lyft.com>

clang tidy

d223fa3

Signed-off-by: Snow Pettersen <snowp@lyft.com>

format

ad5563e

Signed-off-by: Snow Pettersen <snowp@lyft.com>

repokitteh-read-only bot removed the waiting label Sep 30, 2020

clang-tidy

3992803

Signed-off-by: Snow Pettersen <snowp@lyft.com>

mattklein123 previously approved these changes Oct 1, 2020

View reviewed changes

alyssawilk self-assigned this Oct 1, 2020

alyssawilk reviewed Oct 1, 2020

View reviewed changes

feedback

8db3c3f

Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp dismissed mattklein123’s stale review via 8db3c3f October 2, 2020 22:38

Merge remote-tracking branch 'envoy/master' into fix-local-reply-grpc

74f9787

Signed-off-by: Snow Pettersen <snowp@lyft.com>

alyssawilk approved these changes Oct 5, 2020

View reviewed changes

yanavlasov approved these changes Oct 6, 2020

View reviewed changes

snowp merged commit fb7bdbe into envoyproxy:master Oct 6, 2020

nareddyt mentioned this pull request Feb 17, 2021

hcm: Handle stream destroy during encodeData due to sendLocalReply #15075

Merged

Conversation

snowp commented Sep 24, 2020

Uh oh!

snowp commented Sep 24, 2020

Uh oh!

snowp commented Sep 24, 2020

Uh oh!

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

snowp commented Sep 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented Oct 1, 2020

Uh oh!

repokitteh-read-only bot commented Oct 1, 2020

Uh oh!

snowp commented Oct 1, 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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented Oct 6, 2020

Uh oh!

snowp commented Oct 6, 2020

Uh oh!

repokitteh-read-only bot commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants