http: adding an interface to inform filters of local replies by alyssawilk · Pull Request #15172 · envoyproxy/envoy

alyssawilk · 2021-02-24T15:48:30Z

Risk Level: low (no-op for most filters)
Testing: new integration test. NEEDS UNIT TESTS
Docs Changes: n/a
Release Notes: TBD
Fixes #14791

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-02-24T15:49:27Z

Hey @goaway WDYT of this as a start?

Looking for high level suggestions (do we need other return options? more data in the struct? better naming?) and once I have your thumbs up I'll add unit tests and send it to the maintainers for review.

goaway

Wow, thank you for putting something together so quickly! This looks great, and I do think it gives us everything we need.

I don't immediately see a need for any extra data; we surface errors with an error code and a message, which has a direct corollary to the HTTP status code and body. The only thing I wonder is if introducing a different raiseStreamError interface might allow for more nuanced status codes than what HTTP affords in some cases.

But that also seems like something we could consider separately - today we map HTTP code to an error code anyways so this would still be a big improvement for us.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-03-03T14:03:36Z

ping!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp

Thanks this looks pretty good and not too complicated.

Curious what you think about @goaway's suggestion of an error API that would allow us to semantically differentiate a local reply used as a helper to respond (e.g. https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/health_check/health_check.cc#L179-L186) vs stream errors (https://github.com/envoyproxy/envoy/blob/main/source/common/router/router.cc#L659-L661).

I think this could probably be solved by just having a bool streamError() const on StreamInfo and having a wrapper around sendLocalReply that sets this value for the error cases, but curious if you think it's worth integrating this into the onLocalReply API more tightly.

snowp · 2021-03-03T15:58:41Z

include/envoy/http/filter.h

+  // Continue sending onLocalReply to all filters, but reset the stream once all filters have been
+  // informed rather than sending the local reply.


Should we have some way of indicating to filters that the local reply is going to be reset? I can imagine filters wanting to do some mutation of the local reply in some cases, but it doesn't make sense for them to do this if a previous filter has triggered a reset

they can't mutate the local reply here, only from the actual sendlocalReply, but can't hurt to pass on that info

include/envoy/http/filter.h

snowp · 2021-03-03T16:01:41Z

source/common/http/filter_manager.cc

+    ENVOY_STREAM_LOG(debug, "Resetting stream due to {}. onLocalReply requested reset.", *this,
+                     details);


Any way we can capture which filter is triggering the reset? Or let the filter that triggers a reset include information?

I mean we don't really for sendLocalReply logs either, right?

At least with local replies we can infer it from the response code details, but yea I don't think this is necessary, just a nice to have

snowp · 2021-03-03T16:08:52Z

source/common/http/filter_manager.h

  // Http::FilterChainFactoryCallbacks
  void addStreamDecoderFilter(StreamDecoderFilterSharedPtr filter) override {
    addStreamDecoderFilterWorker(filter, nullptr, false);
+    filters_.push_back(filter.get());


Should we document the order in which filters are given the callback? Seems like the order is first added to last added, regardless of whether it was a decoder or encoder filter. I think people might intuitively expect this to follow the decoder or encoder filter chains, but currently this can interleave between the two.

I think with the current impl the order doesn't matter at all, but I can imagine iterations of this feature (like my suggestion of capturing information about which filter is doing the reset) where it starts mattering.

sure, added to include.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk

I think this is what @goaway described at the mobile meeting. We could always make the hcm sendLocalReply be an overridable function and have envoy mobile override it, but I think there was some desire to have filters be informed that a reply is imminent, without actually sending the reply. There's also the complexities of sendLocalReply usually generating a reply but sometimes doing direct-encode or rst where I think this results in a simpler and harder-to-screw-up interface. I'm up for whatever though :-)

alyssawilk · 2021-03-03T18:22:36Z

source/common/http/filter_manager.cc

+    ENVOY_STREAM_LOG(debug, "Resetting stream due to {}. onLocalReply requested reset.", *this,
+                     details);


I mean we don't really for sendLocalReply logs either, right?

alyssawilk · 2021-03-03T18:22:49Z

source/common/http/filter_manager.h

  // Http::FilterChainFactoryCallbacks
  void addStreamDecoderFilter(StreamDecoderFilterSharedPtr filter) override {
    addStreamDecoderFilterWorker(filter, nullptr, false);
+    filters_.push_back(filter.get());


sure, added to include.

alyssawilk · 2021-03-03T18:23:14Z

include/envoy/http/filter.h

+  // Continue sending onLocalReply to all filters, but reset the stream once all filters have been
+  // informed rather than sending the local reply.


they can't mutate the local reply here, only from the actual sendlocalReply, but can't hurt to pass on that info

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

goaway · 2021-03-03T20:26:40Z

Just to clarify - the ability to override sendLocalReply or some antecedent call was mostly just a suggestion about one possible approach. I think the approach here works great though - the main requirement is to have an unambiguous signal that filters get to see regardless of whether a local reply ends up traversing the filter chain.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-03-08T14:35:38Z

@snowp the mac failure is the known timeout, and other comments addressed. PTAL when you get a chance!

snowp

LGTM besides the one comment comment

snowp · 2021-03-08T14:39:46Z

include/envoy/http/filter.h

+   * This will be called on both encoder and decoder filters starting at the
+   * router filter and working towards the first filter configured.


In theory someone could be using another terminal filter that isn't the router filter and still make use of local replies right? Maybe make this "at the terminal filter"

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp

Thanks!

draft onLocalError

ecbba2f

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned goaway Feb 24, 2021

goaway previously approved these changes Feb 24, 2021

View reviewed changes

alyssawilk added 2 commits March 1, 2021 13:59

Merge branch 'main' into stream_error

d059f32

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

unit tests

0b11bfe

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed goaway’s stale review via 0b11bfe March 1, 2021 21:26

alyssawilk marked this pull request as ready for review March 1, 2021 21:46

alyssawilk assigned snowp Mar 1, 2021

alyssawilk added 2 commits March 2, 2021 09:56

typo

6084a39

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

tidy

b374b19

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into stream_error

bd74e50

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp suggested changes Mar 3, 2021

View reviewed changes

alyssawilk added 2 commits March 3, 2021 14:06

comments

c077e50

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into stream_error

81f63f5

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk commented Mar 3, 2021

View reviewed changes

spelling

eee1580

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into stream_error

9209fa7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp suggested changes Mar 8, 2021

View reviewed changes

comment

3f0e7dd

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp approved these changes Mar 8, 2021

View reviewed changes

alyssawilk merged commit 62bd82d into envoyproxy:main Mar 8, 2021

alyssawilk deleted the stream_error branch August 18, 2021 13:59

JuniorHsu mentioned this pull request May 19, 2022

thrift: support onLocalReply to inform filters of local replies #21387

Merged

KBaichoo mentioned this pull request Nov 10, 2022

Cleanup: Update comment. #23929

Merged

wbpcode mentioned this pull request Sep 9, 2024

http: minor code clean up to the http filter manager #36027

Merged

		// Continue sending onLocalReply to all filters, but reset the stream once all filters have been
		// informed rather than sending the local reply.

		ENVOY_STREAM_LOG(debug, "Resetting stream due to {}. onLocalReply requested reset.", *this,
		details);

		* This will be called on both encoder and decoder filters starting at the
		* router filter and working towards the first filter configured.

Conversation

alyssawilk commented Feb 24, 2021

Uh oh!

alyssawilk commented Feb 24, 2021

Uh oh!

goaway left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Mar 3, 2021

Uh oh!

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

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!

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!

goaway commented Mar 3, 2021

Uh oh!

alyssawilk commented Mar 8, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants