Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase #18997

syhpoon · 2021-11-12T20:58:47Z

Commit Message: http: extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase
Additional Description: In some cases the upstream can produce a non-default reason phrase when using HTTP1. For example AWS S3 can return 503 Slow Down in certain cases. StatefuleHeaderFormatter can now take an optional flag to enable forwarding non-default HTTP1 reason phrases.
Risk Level: Medium
Testing: unit testing, integration testing
Docs Changes: release notes
Release Notes: http: added support for forwarding HTTP1 reason phrase
Platform Specific Features: N/A

repokitteh-read-only · 2021-11-12T20:58:49Z

Hi @syhpoon, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #18997 was opened by syhpoon.

see: more, trace.

repokitteh-read-only · 2021-11-12T20:58:50Z

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: #18997 was opened by syhpoon.

see: more, trace.

repokitteh-read-only · 2021-11-12T20:58:54Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18997 was opened by syhpoon.

see: more, trace.

mattklein123 · 2021-11-15T16:12:43Z

Thank you for the contribution. I'm not sure if you are the same person that I was discussing this on Slack with (https://envoyproxy.slack.com/archives/C78HA81DH/p1634828677000900), but I think we decided that the best and least intrusive way of supporting this is to add this into the stateful header formatter system that allows remembering the case of H1 headers during proxy. I think adding remembering the reason phrase to this would be pretty simple and avoid polluting the header map with H1 specific concepts. Can we try to implement it that way? Thank you.

/wait

syhpoon · 2021-11-15T17:04:53Z

Yes, that was me discussing this issue on Slack.
There are couple of issues with the stateful formatter approach that didn't seem very elegant to me:

Let's say I augment StatefulHeaderKeyFormatter interface with a pair of new methods: set/getReasonPhrase, then I either need to create a brand-new formatter class which only implements the new methods (and thus leaving the formatter ones hanging), or extend the existing PreserveCaseHeaderFormatter one, in which case the class name becomes confusing since not only can it format headers to preserve case but also (unrelatedly) manipulate reason phrase. Whatever route I go, it seems like current and future implementations of the interface will only care about either formatting headers with some custom logic OR preserving the reason phrase (and I'm having a hard time figuring out different type of implementations for this feature).
In either of the cases above, the configuration becomes non-stackable, since there can be only one stateful_formatter config parameter, and one wouldn't be able to enable both preserving header casing and proxying reason phrase, especially with more stateful formatters implemented in the future. This is another sign for me that these two features do not belong in the same interface.

Current implementation make this an HTTP1-specific option. If the option is not set (default), there's no performance penalty as the parser callback is not even called. The only addition is one new class member field, and yes, I agree, it's not ideal to have a http1-specific concept in generic class, but I was kind hoping that folks much more familiar with the codebase could advise on a better way to achieve this, if this is considered a major concern.

mattklein123 · 2021-11-15T17:11:07Z

This feature is a very niche ask, similar to preserving the case of H1 headers. I personally would not overthink this. I think it would be fine to just add an option to https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.proto about whether to preserve the reason phrase, or, I think it woud probably be fine to just do it always when using this formatter.

Then I think it's fine to extend the interface to set/get the reason phrase.

syhpoon · 2021-11-15T17:25:02Z

Okay, then a follow-up question: would you expect for StatefulHeaderKeyFormatter interface to ever get another implementation (in addition to PreserveCaseFormatter)? If not, then sure, it should be fine to add new methods and implement them in the existing class, but if there's a chance there will be other implementations (and from the way it's currently structured it surely seems like this was the plan), then it all becomes pretty messy, since all future implementations would have to override set/getReasonPhrase methods while it wouldn't make a lot of sense to have many implementations of this logic, plus there won't be an easy way to enable multiple key formatters, but I already mentioned that.

Basically this is my main concern, but if you're saying it's okay to go this way, I can definitely tweak my implementation accordingly.

mattklein123 · 2021-11-15T17:28:22Z

then it all becomes pretty messy, since all future implementations would have to override set/getReasonPhrase methods while it wouldn't make a lot of sense to have many implementations of this logic, plus there won't be an easy way to enable multiple key formatters, but I already mentioned that.

I don't think it's really that messy. The interface methods could be a nop in a different implementation. As long as we clearly document that the PreserveCaseFormatter does this, I think it's fine. I agree the name is now not great, but so it goes. I just don't think it's worth leaking the implementation details of this very niche feature into the rest of the codebase.

Basically this is my main concern, but if you're saying it's okay to go this way, I can definitely tweak my implementation accordingly.

Let's get another opinion from @alyssawilk on this. @alyssawilk for preserving the H1 reason phrase, do you think we should bolt it on to the stateful header formatter code or build it more directly in?

alyssawilk · 2021-11-17T15:16:14Z

I'm less familiar with the stateful header formatters so largely inclined to trust you on this, but naming aside the formatters largely exist to handle "metadata" for how we want to serialize HTTP/1.1 requests to the wire, so I'm totally fine jamming the reason phrase in there too.

mattklein123

Looks generally on the right track, some comments to get you started, thanks.

/wait

mattklein123 · 2021-11-19T23:03:04Z

api/envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.proto

@@ -16,4 +16,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // See the :ref:`header casing <config_http_conn_man_header_casing>` configuration guide for more
 // information.
 message PreserveCaseFormatterConfig {
+  // Allows forwarding reason phrase text.


Add a release note for this please

mattklein123 · 2021-11-19T23:03:19Z

envoy/http/header_formatter.h

+  /**
+   * Called to save received reason phrase
+   */
+  virtual void setReasonPhrase(const char* data, uint32_t length) PURE;


Pass absl::string_view

mattklein123 · 2021-11-19T23:06:26Z

source/common/http/http1/legacy_parser_impl.cc

@@ -38,7 +38,7 @@ class LegacyHttpParserImpl::Impl {
    parser_.allow_chunked_length = 1;
  }

-  Impl(http_parser_type type, void* data) : Impl(type) {
+  Impl(http_parser_type type, void* data, bool parse_status) : Impl(type) {


I would just always parse status, the cost is low.

Removed the flag.

mattklein123 · 2021-11-19T23:07:33Z

source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.h


 private:
  StringUtil::CaseUnorderedSet original_header_keys_;
+  bool forward_reason_phrase_{false};
+  absl::string_view reason_phrase_;


This needs to be std::string or you will be referencing data that is gone.

jmarantz · 2021-11-22T16:59:42Z

@syhpoon please avoid force-pushing in Envoy Github as it disrupts the comment-flow. Just do normal merges and pushes.

syhpoon · 2021-11-23T03:20:25Z

@mattklein123 all the comments should have been addressed, would appreciate another 👀

alyssawilk · 2021-11-23T13:26:28Z

Apologies for delay but Matt's out this week and where normally we'd reassign, over half the maintainers are out as well so thanks for your patience on unusual review lag :-)

syhpoon · 2021-11-23T15:04:26Z

No worries! Thanks, Alyssa for letting me know!

Signed-off-by: Max Kuznetsov <[email protected]>

mattklein123 · 2021-11-29T18:09:19Z

@syhpoon friendly request to please not force push. It makes reviews much harder. Thank you!

syhpoon · 2021-11-29T18:13:10Z

Sure, but there were no new comments and previous CI run failed, it didn't look the force-push would mess anything.

mattklein123

Thanks, looks awesome. Just 2 small test comments then we can ship!

/wait

mattklein123 · 2021-11-29T18:14:33Z

test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_test.cc

@@ -9,7 +9,7 @@ namespace HeaderFormatters {
 namespace PreserveCase {

 TEST(PreserveCaseFormatterTest, All) {
-  PreserveCaseHeaderFormatter formatter;
+  PreserveCaseHeaderFormatter formatter(false);


Can you verify that when this is false, and you set a reason phrase, you don't get a reason phrase back?

Added another test case for this.

mattklein123 · 2021-11-29T18:14:54Z

...tp/header_formatters/preserve_case/preserve_case_formatter_reason_phrase_integration_test.cc

+                         testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
+                         TestUtility::ipTestParamsToString);
+
+TEST_P(PreserveCaseFormatterReasonPhraseIntegrationTest, VerifyReasonPhrase) {


Can you add an off test also where when explicitly disabled you get back the stock phrase?

Signed-off-by: Max Kuznetsov <[email protected]>

mattklein123

LGTM but needs format fix, thanks.

/wait

Signed-off-by: Max Kuznetsov <[email protected]>

syhpoon · 2021-11-30T03:36:18Z

Thanks @mattklein123, should be good to go!

mattklein123

Thanks, nice work!

syhpoon · 2021-11-30T17:31:03Z

Thanks, Matt!

repokitteh-read-only bot added the api label Nov 12, 2021

repokitteh-read-only bot assigned markdroth Nov 12, 2021

mattklein123 self-assigned this Nov 15, 2021

repokitteh-read-only bot added the waiting label Nov 15, 2021

syhpoon marked this pull request as ready for review November 16, 2021 22:02

syhpoon requested review from jmarantz and mattklein123 as code owners November 19, 2021 21:56

repokitteh-read-only bot removed the waiting label Nov 19, 2021

mattklein123 requested changes Nov 19, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 19, 2021

syhpoon changed the title ~~Add Http1 option to allow proxying response reason phrase~~ Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase Nov 22, 2021

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

syhpoon requested a review from mattklein123 November 23, 2021 03:18

Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase

06b45cd

Signed-off-by: Max Kuznetsov <[email protected]>

mattklein123 requested changes Nov 29, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 29, 2021

expand tests

c89ca0e

Signed-off-by: Max Kuznetsov <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 29, 2021

fix linter warnings

8d6fe2c

Signed-off-by: Max Kuznetsov <[email protected]>

mattklein123 reviewed Nov 29, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 29, 2021

fix another formatting warning

20aff9a

Signed-off-by: Max Kuznetsov <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 29, 2021

mattklein123 approved these changes Nov 30, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Nov 30, 2021

mattklein123 merged commit 76a70b4 into envoyproxy:main Nov 30, 2021

syhpoon deleted the proxy-reason-phrase branch November 30, 2021 16:26

jpsim mentioned this pull request Dec 6, 2021

Update Envoy to 70a5f29 envoyproxy/envoy-mobile#1962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase #18997

Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase #18997

syhpoon commented Nov 12, 2021 •

edited

Loading

repokitteh-read-only bot commented Nov 12, 2021

repokitteh-read-only bot commented Nov 12, 2021

repokitteh-read-only bot commented Nov 12, 2021

mattklein123 commented Nov 15, 2021

syhpoon commented Nov 15, 2021

mattklein123 commented Nov 15, 2021

syhpoon commented Nov 15, 2021

mattklein123 commented Nov 15, 2021 •

edited

Loading

alyssawilk commented Nov 17, 2021

mattklein123 left a comment

mattklein123 Nov 19, 2021

syhpoon Nov 23, 2021

mattklein123 Nov 19, 2021

syhpoon Nov 23, 2021

mattklein123 Nov 19, 2021

syhpoon Nov 23, 2021

mattklein123 Nov 19, 2021

syhpoon Nov 23, 2021

jmarantz commented Nov 22, 2021

syhpoon commented Nov 23, 2021

alyssawilk commented Nov 23, 2021

syhpoon commented Nov 23, 2021

mattklein123 commented Nov 29, 2021

syhpoon commented Nov 29, 2021

mattklein123 left a comment

mattklein123 Nov 29, 2021

syhpoon Nov 29, 2021

mattklein123 Nov 29, 2021

syhpoon Nov 29, 2021

mattklein123 left a comment

syhpoon commented Nov 30, 2021

mattklein123 left a comment

syhpoon commented Nov 30, 2021

Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase #18997

Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase #18997

Conversation

syhpoon commented Nov 12, 2021 • edited Loading

repokitteh-read-only bot commented Nov 12, 2021

repokitteh-read-only bot commented Nov 12, 2021

repokitteh-read-only bot commented Nov 12, 2021

mattklein123 commented Nov 15, 2021

syhpoon commented Nov 15, 2021

mattklein123 commented Nov 15, 2021

syhpoon commented Nov 15, 2021

mattklein123 commented Nov 15, 2021 • edited Loading

alyssawilk commented Nov 17, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Nov 22, 2021

syhpoon commented Nov 23, 2021

alyssawilk commented Nov 23, 2021

syhpoon commented Nov 23, 2021

mattklein123 commented Nov 29, 2021

syhpoon commented Nov 29, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

syhpoon commented Nov 30, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

syhpoon commented Nov 30, 2021

syhpoon commented Nov 12, 2021 •

edited

Loading

mattklein123 commented Nov 15, 2021 •

edited

Loading