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.

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!

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/).

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

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.

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.

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.

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?

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.

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

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

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 :-)

No worries! Thanks, Alyssa for letting me know!

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

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

Thanks @mattklein123, should be good to go!

Thanks, Matt!