Add setRequestDecoder to ResponseEncoder interface by pksohn · Pull Request #24368 · envoyproxy/envoy

pksohn · 2022-12-05T21:26:01Z

Commit Message: Add setRequestDecoder to ResponseEncoder interface
Additional Description: We also set the new request decoder in recreateStream. This change was introduced in #23648 but was moved into a separate PR given its more general scope.
Risk Level: Low
Testing: No tests added.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Paul Sohn <paulsohn@google.com>

repokitteh-read-only · 2022-12-05T21:26:05Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24368 was opened by pksohn.

see: more, trace.

pksohn · 2022-12-05T21:26:46Z

/assign @danzh2010

pksohn · 2022-12-05T21:26:55Z

/assign @alyssawilk

alyssawilk · 2022-12-06T14:09:42Z

/retest

repokitteh-read-only · 2022-12-06T14:09:51Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #24368 (comment) was created by @alyssawilk.

see: more, trace.

alyssawilk

LGTM modulo one of two explanatory comments :-)

alyssawilk · 2022-12-06T14:12:06Z

envoy/http/codec.h

+
+  /**
+   * Set a new request decoder for this ResponseEncoder.
+   * @param decoder new request decoder.


think it's worth a comment on why this is a useful association?

added, ptal!

alyssawilk · 2022-12-06T14:12:18Z

source/common/http/http1/codec_impl.h

    return stream_error_on_invalid_http_message_;
  }

+  void setRequestDecoder(Http::RequestDecoder& /*decoder*/) override {}


or maybe why HTTP/1.1 doesn't need this

Added. H/1 will need more work to actually reset the decoder on internal redirect, but it seems like we don't need to tackle that now?

We will need to make it work for H1 to defer logging I guess? Can you add a TODO?

danzh2010

Thanks for taking this part out! One more request, could you move the recreateStream() change to this PR as well?

Signed-off-by: Paul Sohn <paulsohn@google.com>

pksohn · 2022-12-06T17:05:39Z

Thanks for taking this part out! One more request, could you move the recreateStream() change to this PR as well?

Done.

Signed-off-by: Paul Sohn <paulsohn@google.com>

repokitteh-read-only · 2022-12-06T18:24:47Z

CC @envoyproxy/mobile-maintainers: FYI only for changes made to (mobile/).
envoyproxy/mobile-maintainers assignee is @alyssawilk

🐱

Caused by: #24368 was synchronize by pksohn.

see: more, trace.

Signed-off-by: Paul Sohn <paulsohn@google.com>

danzh2010 · 2022-12-06T19:51:19Z

source/common/http/conn_manager_impl.cc

+  // This doesn't currently work for HTTP/1 as the H/1 ResponseEncoder doesn't hold the active
+  // stream's pointer to the RequestDecoder.


For my own knowledge, how does H1 codec decode request headers in that case?

H1 has an ActiveStream that has a handle on both a ResponseEncoder and RequestDecoder. So e.g. on receiving headers the connection goes through the active request to get to the decoder, like here. It seems like the H/2 and H/3 codecs have the decoding path go through the response encoder -> request decoder partly to simplify multiplexing, so it makes sense that H/1 (which doesn't have streams) is organized a bit differently. I could be wrong about that last part.

Get it! Thanks for the link!

danzh2010 · 2022-12-06T19:54:05Z

source/common/http/http1/codec_impl.h

    return stream_error_on_invalid_http_message_;
  }

+  void setRequestDecoder(Http::RequestDecoder& /*decoder*/) override {}


We will need to make it work for H1 to defer logging I guess? Can you add a TODO?

Signed-off-by: Paul Sohn <paulsohn@google.com>

alyssawilk · 2022-12-07T14:05:59Z

cc @mattklein123 for @google pass (circleci failure can be ignored)

danzh2010

LGTM. Please update the PR description to reflect the recreateStream() change.

danzh2010 · 2022-12-07T16:13:08Z

source/common/http/conn_manager_impl.cc

+  // This doesn't currently work for HTTP/1 as the H/1 ResponseEncoder doesn't hold the active
+  // stream's pointer to the RequestDecoder.


Get it! Thanks for the link!

….h-into-multiple-header-files * origin/main: Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) Signed-off-by: JP Simard <jp@jpsim.com>

…-cpp-to-latest-version * origin/main: (23 commits) Reduce Route memory utilization by avoiding RuntimeData instances when not needed (#24327) build: fix compile error for mac (#24429) postgres: support for upstream SSL (#23990) iOS: split `EnvoyEngine.h` into multiple header files (#24397) mobile: check for pending exceptions after JNI call (#24361) Remove uneccessary `this->` from mobile engine builder (#24389) Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) ... Signed-off-by: JP Simard <jp@jpsim.com>

Add setRequestDecoder to ResponseEncoder interface

30a70cc

Signed-off-by: Paul Sohn <paulsohn@google.com>

pksohn marked this pull request as ready for review December 5, 2022 21:26

repokitteh-read-only bot assigned danzh2010 Dec 5, 2022

repokitteh-read-only bot assigned alyssawilk Dec 5, 2022

alyssawilk reviewed Dec 6, 2022

View reviewed changes

danzh2010 reviewed Dec 6, 2022

View reviewed changes

pksohn added 2 commits December 6, 2022 16:31

In recreateStream, set request decoder for new response encoder

351ae73

Signed-off-by: Paul Sohn <paulsohn@google.com>

add comments

fed169f

Signed-off-by: Paul Sohn <paulsohn@google.com>

pksohn added 3 commits December 6, 2022 17:53

rm outdated comment

41876ee

Signed-off-by: Paul Sohn <paulsohn@google.com>

add explanatory comment

e897a9d

Signed-off-by: Paul Sohn <paulsohn@google.com>

mobile client: implement setRequestDecoder

7b6e5d6

Signed-off-by: Paul Sohn <paulsohn@google.com>

pksohn requested review from Augustyniak, RyanTheOptimist, abeyad and jpsim as code owners December 6, 2022 18:24

repokitteh-read-only bot added the mobile label Dec 6, 2022

fix formatting

6af9d70

Signed-off-by: Paul Sohn <paulsohn@google.com>

danzh2010 reviewed Dec 6, 2022

View reviewed changes

add todo for H/1

e643111

Signed-off-by: Paul Sohn <paulsohn@google.com>

alyssawilk approved these changes Dec 7, 2022

View reviewed changes

alyssawilk assigned mattklein123 and unassigned alyssawilk and danzh2010 Dec 7, 2022

alyssawilk assigned alyssawilk and danzh2010 Dec 7, 2022

danzh2010 approved these changes Dec 7, 2022

View reviewed changes

mattklein123 approved these changes Dec 7, 2022

View reviewed changes

mattklein123 merged commit 39ca99f into envoyproxy:main Dec 7, 2022

pksohn deleted the requestdecoder branch December 7, 2022 17:37

		// This doesn't currently work for HTTP/1 as the H/1 ResponseEncoder doesn't hold the active
		// stream's pointer to the RequestDecoder.

Conversation

pksohn commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 5, 2022

Uh oh!

pksohn commented Dec 5, 2022

Uh oh!

pksohn commented Dec 5, 2022

Uh oh!

alyssawilk commented Dec 6, 2022

Uh oh!

repokitteh-read-only bot commented Dec 6, 2022

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!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

pksohn commented Dec 6, 2022

Uh oh!

repokitteh-read-only bot commented Dec 6, 2022

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!

alyssawilk commented Dec 7, 2022

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pksohn commented Dec 5, 2022 •

edited

Loading