http: add HTTP/1.1 case preservation #15619

mattklein123 · 2021-03-23T04:55:28Z

Add new stateful header formatter extension point
Add preserve case formatter extension

Risk Level: Low
Testing: New integration test
Docs Changes: Added
Release Notes: Added
Platform Specific Features: N/A

1) Add new stateful header formatter extension point 2) Add preserve case formatter extension Fixes #14363 Signed-off-by: Matt Klein <mklein@lyft.com>

repokitteh-read-only · 2021-03-23T04:55:34Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15619 was opened by mattklein123.

see: more, trace.

source/common/http/http1/settings.h

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2021-03-23T05:15:28Z

@asraa @alyssawilk @jmarantz @snowp PTAL. I'm pretty happy with how this turned out. It's quite clean and the perf overhead when not in use should be negligible.

jmarantz

Epic! Looks great overall. A few minor comments so far.

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

api/envoy/config/core/v3/protocol.proto

include/envoy/http/codec.h

include/envoy/http/header_map.h

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

test/extensions/http/header_formatters/preserve_case/BUILD

docs/root/configuration/http/http_conn_man/header_casing.rst

api/envoy/config/core/v3/protocol.proto

include/envoy/http/codec.h

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2021-03-24T00:09:02Z

@asraa @jmarantz @snowp I think this is ready for another pass.

snowp

This looks great, just a couple small comments

docs/root/configuration/http/http_conn_man/header_casing.rst

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

include/envoy/http/header_map.h

jmarantz

looks great; just a few nits.

docs/root/configuration/http/http_conn_man/header_casing.rst

.../extensions/http/header_formatters/preserve_case/preserve_case_formatter_integration_test.cc

include/envoy/http/header_formatter.h

source/common/http/http1/codec_impl.cc

jtway

This seems great overall. Couple minor comments.

source/common/http/http1/codec_impl.cc

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

.../extensions/http/header_formatters/preserve_case/preserve_case_formatter_integration_test.cc

include/envoy/http/header_map.h

source/common/http/header_map_impl.h

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

alyssawilk

Awesome, I'm sure loads of users are going to love this one.

Two drive by comment from the "what could possibly go wrong" department.

My first thought was "What about T-E header which we add and remove?" Looks like you handle that case, but mind adding it to the integration test?

From a user experience perspective I suspect folks will probably want snow's proper casing along with your preserve casing, so if there's a header addition (say we gunzip and go from content length to transfer encoding) or header creation (send local reply with content length) we "create" the right one. Do we expect them to play well together? If so I'd suggest having the canonical config use both, update docs explaining why users who want preserve probably want create, and integration test the transformation example.

mattklein123 · 2021-03-24T17:29:11Z

From a user experience perspective I suspect folks will probably want snow's proper casing along with your preserve casing, so if there's a header addition (say we gunzip and go from content length to transfer encoding) or header creation (send local reply with content length) we "create" the right one. Do we expect them to play well together? If so I'd suggest having the canonical config use both, update docs explaining why users who want preserve probably want create, and integration test the transformation example.

This is interesting. I didn't think about them being stacked like this. This is not how it works today. What does everyone think about this? Right now it's actually possible in the proto to configure this. I can potentially deprecate and make it repeated? That would be a pretty big change and I'm not sure it's worth it? Thoughts?

alyssawilk · 2021-03-24T17:32:47Z

We're adding this case preservation system to handle clients/servers who do case sensitive compares.
If regular Envoy tweaking of headers results in lower case content-length etc. I think they're still going to have a problem, so I think we have to make sure we have a way to do preservation and proper casing to really solve that problem.

mattklein123 · 2021-03-24T22:04:54Z

OK this should be ready for another pass, thanks.

jmarantz · 2021-03-25T03:13:29Z

include/envoy/http/codec.h

@@ -439,11 +440,16 @@ struct Http1Settings {
    // Performs proper casing of header keys: the first and all alpha characters following a
    // non-alphanumeric character is capitalized.
    ProperCase,
+    // A stateful formatter extension has been configured.
+    StatefulFormatter,


I think given @alyssawilk insight that we might want to generate mixed-case results for synthesized headers while preserving the case of others, maybe we want to avoid checking in the API that has these choices as exclusive?

I think from a data structure perspective we'd need to actually eliminate StatefulFormatter as a choice here and just allow users to configure both the enum and a stateful formatter (or repeated stateful formatters if we think that might be useful in the future). Does it make sense to do that now before the initial checkin?

I'm less concerned about the code and more about the API. My understanding from the thread above is that we decided if we need this in the future we can implement a nested formatter inside the preserve case formatter for unknown headers? Because of that I think we decide we can punt for now until someone asks for it?

Personally, I would think that any other than nested or another extension (one that may not even be in the core repo) would need to handle prioritization. For instance if you defined a stateful (preserve case), and stateless (proper case), in the current form the stateless would just end up proper casing the header keys that the stateful formatter was looking to preserve. Am I missing something @jmarantz, as it stands I'd agree 💯 with @mattklein123.

Signed-off-by: Matt Klein <mklein@lyft.com>

snowp

Looks good besides the one comment.

I think we might not be propagating the casing in situation where we copy the downstream headers (e.g. request shadowing), but it might not be worth trying to solve this now.

snowp · 2021-03-26T16:42:09Z

tools/code_format/check_format.py

@@ -65,7 +65,9 @@
 # perform temporary registrations.
 REGISTER_FACTORY_TEST_ALLOWLIST = (
    "./test/common/config/registry_test.cc", "./test/integration/clusters/",
-    "./test/integration/filters/")
+    "./test/integration/filters/",
+    "./test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_integration_test.cc"


The comment above suggests you should be using InjectFactory instead of of REGISTER_FACTORY, thoughts?

This isn't possible for integration tests. This is here for the same reason test/integration/filters is here.

Huh I thought I'd seen InjectFactory used in integration tests, but I could be wrong here

Ah OK it looks like it can. Sure I can fix that.

mattklein123 · 2021-03-26T16:46:26Z

I think we might not be propagating the casing in situation where we copy the downstream headers (e.g. request shadowing), but it might not be worth trying to solve this now.

Yes, it won't work in that case. I think it's not very hard to fix. We could add a clone() method to the formatter, and then clone it during header map copy. I didn't do this because I doubt anyone will notice and I didn't want to use shared_ptr. wdyt?

snowp · 2021-03-26T16:59:34Z

I didn't do this because I doubt anyone will notice and I didn't want to use shared_ptr. wdyt?

I think if it's limited to request shadowing it's probably fine to leave for now, I imagine the main use case will be to format response headers, so the shadowing case is probably not important. I wasn't sure if there were other header copying cases that we needed to cover

mattklein123 · 2021-03-26T17:02:30Z

Any copying case can be fixed relatively easily by allowing formatter cloning, but like I said, I would rather wait until someone complains, because I doubt they will. It won't be hard to fix at that point.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2021-03-26T22:13:49Z

@snowp updated per comments

snowp

Thanks!

dalegaspi · 2021-03-27T12:27:22Z

is this going to be available on the latest release only or is it going to be back-ported to some older releases?

antoniovicente · 2021-03-29T21:35:47Z

source/extensions/extensions_build_config.bzl

+    # HTTP header formatters
+    #
+
+    "envoy.http.stateful_header_formatters.preserve_case":       "//source/extensions/http/header_formatters/preserve_case:preserve_case_formatter"


missing , at the end of the line.

Will fix this in my current PR

noah8713 · 2021-11-30T18:52:35Z

@mattklein123 Is there an optimal option/improvement/suggestion for skipping this for http2 clusters when in case like istio, ingress gateway needs to allow both http1/2 traffic. We ran into istio/istio#36299 when using auto_config for case preserve leveraging envoy to auto detect http1.1/2 depending on upstream config but it didn't pick up as expected.

mattklein123 · 2021-11-30T20:58:07Z

@mattklein123 Is there an optimal option/improvement/suggestion for skipping this for http2 clusters when in case like istio, ingress gateway needs to allow both http1/2 traffic. We ran into istio/istio#36299 when using auto_config for case preserve leveraging envoy to auto detect http1.1/2 depending on upstream config but it didn't pick up as expected.

This only takes effect for H1. If something else is happening I would open a new issue with repro instructions, etc.

noah8713 · 2021-11-30T21:24:45Z

Thanks @mattklein123 : I have mentioned details of reproducing in istio/istio#36299 as it happens while using auto_config for case preserve in httpprotocol options vs explicit_http_config as mentioned in the doc. I am not sure if I should open a new issue here too or continue on the one on istio side.

noah8713 · 2021-12-01T02:17:36Z

@mattklein123 Is there an optimal option/improvement/suggestion for skipping this for http2 clusters when in case like istio, ingress gateway needs to allow both http1/2 traffic. We ran into istio/istio#36299 when using auto_config for case preserve leveraging envoy to auto detect http1.1/2 depending on upstream config but it didn't pick up as expected.

This only takes effect for H1. If something else is happening I would open a new issue with repro instructions, etc.

Filed new issue for the same #19149

1) Add new stateful header formatter extension point; 2) Add preserve case formatter extension; Fixes envoyproxy#14363 Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com>

* feat: http: add HTTP/1.1 case preservation (envoyproxy#15619) 1) Add new stateful header formatter extension point; 2) Add preserve case formatter extension; Fixes envoyproxy#14363 Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com> * fix: fix Error envoy_cc_library() got unexpected keyword argument: category Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com> * fix: source/common/http/http1/settings.cc:39:39: error: unused parameter 'validate_scheme' Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com> * feat: preserve case with proper case config Signed-off-by: jiangshantao <jiangshantao-dbg@qq.com> Co-authored-by: jiangshantao <jiangshantao-dbg@qq.com>

http: add HTTP/1.1 case preservation

f8b938c

1) Add new stateful header formatter extension point 2) Add preserve case formatter extension Fixes #14363 Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 requested a review from alyssawilk as a code owner March 23, 2021 04:55

repokitteh-read-only bot added the api label Mar 23, 2021

repokitteh-read-only bot assigned lizan Mar 23, 2021

mattklein123 assigned jmarantz, snowp and alyssawilk and unassigned lizan Mar 23, 2021

mattklein123 commented Mar 23, 2021

View reviewed changes

source/common/http/http1/settings.h Show resolved Hide resolved

fix

7a47e28

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 assigned asraa Mar 23, 2021

jmarantz reviewed Mar 23, 2021

View reviewed changes

asraa reviewed Mar 23, 2021

View reviewed changes

test/extensions/http/header_formatters/preserve_case/BUILD Outdated Show resolved Hide resolved

asraa reviewed Mar 23, 2021

View reviewed changes

docs/root/configuration/http/http_conn_man/header_casing.rst Outdated Show resolved Hide resolved

snowp reviewed Mar 23, 2021

View reviewed changes

api/envoy/config/core/v3/protocol.proto Outdated Show resolved Hide resolved

include/envoy/http/codec.h Outdated Show resolved Hide resolved

mattklein123 added 5 commits March 23, 2021 16:48

Merge remote-tracking branch 'origin/main' into header_casing_take_2

8579832

Signed-off-by: Matt Klein <mklein@lyft.com>

comments

7743a61

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge remote-tracking branch 'origin/main' into header_casing_take_2

251cda7

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

b25b17e

Signed-off-by: Matt Klein <mklein@lyft.com>

more optref tests

29b8e59

Signed-off-by: Matt Klein <mklein@lyft.com>

snowp suggested changes Mar 24, 2021

View reviewed changes

cretz reviewed Mar 24, 2021

View reviewed changes

include/envoy/http/header_map.h Show resolved Hide resolved

jmarantz reviewed Mar 24, 2021

View reviewed changes

jtway previously approved these changes Mar 24, 2021

View reviewed changes

alyssawilk reviewed Mar 24, 2021

View reviewed changes

mattklein123 dismissed jtway’s stale review via a443664 March 24, 2021 22:04

repokitteh-read-only bot removed the waiting label Mar 24, 2021

jmarantz reviewed Mar 25, 2021

View reviewed changes

Merge branch 'main' into header_casing_take_2

163dbda

Signed-off-by: Matt Klein <mklein@lyft.com>

snowp suggested changes Mar 26, 2021

View reviewed changes

mattklein123 added 2 commits March 26, 2021 22:02

Merge remote-tracking branch 'origin/main' into header_casing_take_2

8f95245

Signed-off-by: Matt Klein <mklein@lyft.com>

comments

9f4aae4

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 mentioned this pull request Mar 26, 2021

HRW Hash Load Balancing #15596

Open

snowp approved these changes Mar 27, 2021

View reviewed changes

jtway approved these changes Mar 27, 2021

View reviewed changes

mattklein123 merged commit 2a4d97c into main Mar 28, 2021

mattklein123 deleted the header_casing_take_2 branch March 28, 2021 16:51

esmet mentioned this pull request Mar 29, 2021

Ambassador Megapatch on release/v1.17 datawire/envoy#20

Closed

antoniovicente reviewed Mar 29, 2021

View reviewed changes

jtway mentioned this pull request Aug 10, 2021

Allow stateful and stateless header formatters to both be configured #17661

Open

rgs1 mentioned this pull request Apr 25, 2022

thrift: add support for case-sensitive header keys #20893

Merged

Shiva2199 mentioned this pull request Feb 2, 2023

Feature Request: Envoy should be able to preserve header casing in HTTP/1.1 aws/aws-app-mesh-roadmap#454

Open

slonka mentioned this pull request Aug 12, 2024

HTTP/1.1 Header casing kumahq/kuma#11093

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: add HTTP/1.1 case preservation #15619

http: add HTTP/1.1 case preservation #15619

mattklein123 commented Mar 23, 2021

repokitteh-read-only bot commented Mar 23, 2021

mattklein123 commented Mar 23, 2021

jmarantz left a comment

mattklein123 commented Mar 24, 2021

snowp left a comment

jmarantz left a comment

jtway left a comment

alyssawilk left a comment

mattklein123 commented Mar 24, 2021

alyssawilk commented Mar 24, 2021

mattklein123 commented Mar 24, 2021

jmarantz Mar 25, 2021

mattklein123 Mar 25, 2021

jtway Mar 25, 2021

snowp left a comment

snowp Mar 26, 2021

mattklein123 Mar 26, 2021

snowp Mar 26, 2021

mattklein123 Mar 26, 2021

mattklein123 commented Mar 26, 2021

snowp commented Mar 26, 2021

mattklein123 commented Mar 26, 2021

mattklein123 commented Mar 26, 2021

snowp left a comment

dalegaspi commented Mar 27, 2021

antoniovicente Mar 29, 2021

mattklein123 Mar 30, 2021

noah8713 commented Nov 30, 2021

mattklein123 commented Nov 30, 2021

noah8713 commented Nov 30, 2021

noah8713 commented Dec 1, 2021

http: add HTTP/1.1 case preservation #15619

http: add HTTP/1.1 case preservation #15619

Conversation

mattklein123 commented Mar 23, 2021

repokitteh-read-only bot commented Mar 23, 2021

mattklein123 commented Mar 23, 2021

jmarantz left a comment

Choose a reason for hiding this comment

mattklein123 commented Mar 24, 2021

snowp left a comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

jtway left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

mattklein123 commented Mar 24, 2021

alyssawilk commented Mar 24, 2021

mattklein123 commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowp 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 commented Mar 26, 2021

snowp commented Mar 26, 2021

mattklein123 commented Mar 26, 2021

mattklein123 commented Mar 26, 2021

snowp left a comment

Choose a reason for hiding this comment

dalegaspi commented Mar 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noah8713 commented Nov 30, 2021

mattklein123 commented Nov 30, 2021

noah8713 commented Nov 30, 2021

noah8713 commented Dec 1, 2021