http1: allow configuring format of encoded http1 response headers by snowp · Pull Request #8499 · envoyproxy/envoy

snowp · 2019-10-04T23:58:45Z

Adds a configuration option that will convert all header keys into
Train-Case. This is useful to allow Envoy to respond with headers
that match the casing of other systems, to ensure that the wire format
of responses is unchanged when migrating to Envoy.

Signed-off-by: Snow Pettersen snowp@squareup.com

Risk Level: Mediun
Testing: UTs
Docs Changes: Proto comments
Release Notes: n/a
Fixes #8463

Adds a configuration option that will convert all header keys into Train-Case. This is useful to allow Envoy to respond with headers that match the casing of other systems, to ensure that the wire format of responses is unchanged when migrating to Envoy. Fixes envoyproxy#8463 Signed-off-by: Snow Pettersen <snowp@squareup.com>

repokitteh-read-only · 2019-10-04T23:58:48Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8499 was opened by snowp.

see: more, trace.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2019-10-05T00:09:44Z

Mainly looking for API review at this point: is this too specific? Should we be aiming for something more flexible like arbitrary regex transformations with the ability to uppercase matches?

For example, this could be represented by a series of regex matches: { match: /^[a-z]/, uppercase: true } + { match: /[^0-9a-zA-Z][a-z]/, uppercase: true }. This would come at some perf cost and code complexity but would make this much more flexible.

Open to ideas for how to best express this so that it can be useful for as many people as possible.

@htuch @alyssawilk

mattklein123

Just flushing out a drive by API comment. Nice to see this being worked on!

mattklein123 · 2019-10-06T02:39:09Z

api/envoy/api/v2/core/protocol.proto

  string default_host_for_http_10 = 3;
+
+  message HeaderKeyFormat {
+    enum HeaderFormat {


enums are pretty fragile for stuff like this. I would recommend just making this a oneof with a required value and a single TrainCase message that is empty. In the future we can easily add configuration to the TrainCase message or add more complicated message types.

That's probably a good idea.

htuch · 2019-10-07T16:16:38Z

@snowp would pass-thru of client casing work to support this use case? @jmarantz has done some research on this and it seems doable.

snowp · 2019-10-07T16:41:14Z

Pass-through would work in certain scenarios (h/1.1 all the way), but I had some concerns about supporting intermediate h2 hops, as we tend to use h2 wherever possible. Seems like @alyssawilk shared some of those concerns based on the conversation in #8463.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

htuch · 2019-10-08T04:51:22Z

api/envoy/api/v2/core/protocol.proto

  string default_host_for_http_10 = 3;
+
+  message HeaderKeyFormat {
+    enum HeaderFormat {


That's probably a good idea.

htuch · 2019-10-08T04:52:06Z

api/envoy/api/v2/core/protocol.proto

+      // Formats the header using Train-Case: the first character and any character following a
+      // special character will be capitalized if it's an alpha character. For example,
+      // "content-type" becomes "Content-Type", and "foo$b#$ar" becomes "Foo$B#$Ar".
+      TRAIN_CASE = 1;


Is "train case" a standard term? I haven't heard it before.

i hadn't heard about it either before it was used in the issue that was filed. Googling around I can find some references to it (https://docs.rs/Inflector/0.7.0/inflector/cases/traincase/index.html, https://medium.com/infancyit/letter-case-special-styles-7fa369bf173d), though other sources have it mean something else (https://en.wikipedia.org/wiki/Letter_case#Special_case_styles says TRAIN-CASE, https://www.fluentcpp.com/2019/01/08/restmycase-a-c-library-for-formatting-string-cases/ says train_Case).

Open to naming suggestions here, given that this doesn't seem to be a very common term

One option might be rfc7230_casing since we're basically trying to present the headers the way they are cased in the HTTP/1.1 RFCs.

that sounds good to me. @PiotrSikora any concerns about taking the name of the RFC in vain?

"TrainCase" is called PascalCase (aka Upper Camel Case).

We should also have the DONT_MODIFY case (probably a good default, and in theory it's backwards compatible, since HTTP headers are case-insensitive), because not all standard headers use PascalCase (e.g. Content-MD5 and TE), and use transcoding to PascalCase or lowercase only when transforming headers between HTTP/1.1 and HTTP/2.

Nevermind, PascalCase is without hyphens.

@htuch using RFC number here isn't appropriate, IMHO, because that would imply that the casing in defined in that RFC, whereas that's not the case (as far as I can tell).

How about propercase_words? then we define a word as any sequence of alpha characters

htuch · 2019-10-08T04:52:37Z

api/envoy/api/v2/core/protocol.proto

+
+  // Describes how the keys for response headers should be formatted. By default, all header keys
+  // are lower cased.
+  HeaderKeyFormat response_header_key_format = 4;


Is this only for response? What about request headers forwarded to the upstream?

i can add it for the request path too, but that seems to be a bit more involved since the http1 codec settings aren't currently being passed through to the connection pool/upstream http/1.1 codec (since all the current options apply only to the HCM).

How about i add the proto def /w not-implemented and leave that as a TODO?

I think it makes sense to have it just be header_format, and keep it hidden until we have it implemented on both sides, because being allowd to set response header format where the settings are used for requests is weird.
WDYT?

yeah i like that, will go with that

Arguably though clients and backends might have different case senstivity concerns. E.g. one wants Rfc7230-Casing, the other wants 1337Spe4k-k451nG.

Sure, but we have codec options in both places.
AFIK when there's protocol options on an HCM, this will affect how we serialize the response headers and when we set the protocol options in CDS it'll affect how we serialize request headers. Since we're not doing enforcement of request headers at the listener or response headers in the connection pool, I don't know what it would mean to have response headers in the connection pool Http1 options config.

I suggest we just have one name, and note it affects how we serialize the headers out at the codec.

Yeah, I think the fact that the configuration for request and response headers wouldn't occur in the same location (HCM vs Cluster) makes having it be a single field make more sense than two fields with only one ever being relevant nicer

Good point, a single name makes sense given the multiplicity of codec options on downstream/upstream.

htuch · 2019-10-08T04:53:25Z

@snowp ack, let's continue with explicit capitalization support then.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2019-10-11T18:07:10Z

This should be ready for review for whoever wants to pick this up

alyssawilk

Very cool! First pass thoughts

api/envoy/api/v2/core/protocol.proto

include/envoy/http/codec.h

alyssawilk · 2019-10-15T13:54:38Z

source/common/http/http1/codec_impl.cc


-        static_cast<StreamEncoderImpl*>(context)->encodeHeader(key_to_use,
-                                                               header.value().getStringView());
+        if (header_key_formatter_ != nullptr) {


I'd lean towards an Encode helper function which took the context, cormatter (which could be null) and string view header key

api/envoy/api/v2/core/protocol.proto

alyssawilk · 2019-10-15T14:07:18Z

source/common/http/http1/header_formatter.cc

+namespace Envoy {
+namespace Http {
+namespace Http1 {
+std::string ProperCaseHeaderKeyFormatter::format(absl::string_view key) const {


Looking at https://en.wikipedia.org/wiki/List_of_HTTP_header_fields

This won't do A-IM, TE Content-MD5, HTTP2-Setting, and about 10 others the way an HTTP/1 user might expect.

I'd lean towards saying we do standard capitalization of standard HTTP 1.1 headers that Envoy knows about (e.g. TE) with a link to headers Envoy knows about. ProperCaseExceptForRegisteredStandardizedHeaderExceptions is too wordy, but I think that's what we want?

This way if someone later on desperately needs P3P cased they awy they want, they can just add it to the list of Envoy known headers. And when we eventually add the Envoy extension mechanism for arbitrary headers we keep talking about, folks can do their own custom builds with TheirWhackyCAPs if they want.

Alternately if we want to go for the simple case, I think we should document there are known standard headers which will still look unusual to HTTP/1 consumers, and someone can implement ProperCaseByDefaultButWithOverrides later now we have an API for things.

@mattklein123 WDYT?

Maybe StandardOrProperCase? I can go either way on whether we want to handle well known headers, it sounds a tad bit more in line with what someone might expect but it might be overkill.

Looking at Go's net/http (https://golang.org/src/net/http/header.go#L214) they do the same kind of formatting this PR does when canonicalizing headers except that they only uppercase letters after a hyphen.

I don't have a strong opinion here as long as it is well documented. The way that we have things set up at the API layer we can always make things configurable later if needed.

A soft suggestion of having StandardOrProperCase behavior; and perhaps condensing the config name down to StandardCase (which would follow well-known casings for well-known headers, and standard casing patterns for H/1 otherwise).

alyssawilk · 2019-10-15T14:08:53Z

tools/spelling_dictionary.txt

 Welford's
 Werror
 XDS
+ar


can we change foo$b#$ar to foo$b#$are for all the various explantions of that transform to avoid this addition? Feels like it'd be a common typo of "are" which we should catch.

mattklein123

Cool stuff, a few drive by comments.

api/envoy/api/v2/core/protocol.proto

mattklein123 · 2019-10-15T19:25:46Z

source/common/http/http1/codec_impl.cc

  bool saw_content_length = false;
  headers.iterate(
-      [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
+      [this](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {


FWIW, this function used a C-style callback originally for performance reasons when I was optimizing for the horse race case. I would leave a comment here about looking at perf, or potentially just pre-create the lambda at construction time so that in the keep alive case we don't need any extra allocations.

Do you think it's worth having multiple versions of iterate, one with C-style callback and one with std::function? afaik std::function adds some overhead compared to C-style callbacks due to the additional indirection

Turns out I was able to do this without changing the signature, since I could just use the encoder that was already being passed as the context to resolve the formatter.

mattklein123 · 2019-10-15T19:27:22Z

source/common/http/http1/header_formatter.cc

+namespace Envoy {
+namespace Http {
+namespace Http1 {
+std::string ProperCaseHeaderKeyFormatter::format(absl::string_view key) const {


I don't have a strong opinion here as long as it is well documented. The way that we have things set up at the API layer we can always make things configurable later if needed.

htuch · 2019-10-16T17:58:46Z

api/envoy/api/v2/core/protocol.proto

+    oneof header_format {
+      // Formats the header by proper casing words: the first character and any character following
+      // a special character will be capitalized if it's an alpha character. For example,
+      // "content-type" becomes "Content-Type", and "foo$b#$ar" becomes "Foo$B#$Ar".


@envoyproxy/api-shepherds API LGTM. I think if there are nuances around details with how this is done, e.g. whether it's - only or any punctuation, we can sort them out with ProperCaseWords fields later.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk

Looking good! I'd be inclined to bump risk to medium as there are now some extra refactors, but your call.

alyssawilk · 2019-10-22T13:46:09Z

api/envoy/api/v2/core/protocol.proto

+      // Formats the header by proper casing words: the first character and any character following
+      // a special character will be capitalized if it's an alpha character. For example,
+      // "content-type" becomes "Content-Type", and "foo$b#$are" becomes "Foo$B#$Are".
+      ProperCaseWords proper_case_words = 1;


Optional: Do you think it's worth calling out both that this is an effort to normalize HTTP/1 headers to what users expect, and that this will not behave "as expected" for all headers, e.g. TE? I can imagine that folks who have HTTP/1 casing issues will think this is the solution to their problem and I think documenting the simplicity vs the non-handled corner cases will probably save someone some trouble.

alyssawilk · 2019-10-22T13:52:19Z

source/common/http/http1/codec_impl.cc

-                   Headers::get().TransferEncoding.get().size(),
-                   Headers::get().TransferEncodingValues.Chunked.c_str(),
-                   Headers::get().TransferEncodingValues.Chunked.size());
+      encodeFormattedHeader(Headers::get().TransferEncoding.get().c_str(),


can we skip the c_str() here and in other places?

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2019-10-22T17:39:02Z

/azp run

azure-pipelines · 2019-10-22T17:39:20Z

Azure Pipelines successfully started running 3 pipeline(s).

mattklein123 · 2019-10-23T17:23:32Z

Please merge master to fix coverage.

/wait

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Nice, LGTM with some small comments.

/wait

mattklein123 · 2019-10-23T21:01:04Z

source/common/http/http1/codec_impl.cc

    runHighWatermarkCallbacks();
  }
+
+  encode_header_cb_ = [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {


Sorry I think we can kill this member variable and revert this part of the change now, right?

mattklein123 · 2019-10-23T21:02:03Z

source/common/http/http1/codec_impl.h

  bool is_response_to_head_request_{false};
  bool is_content_length_allowed_{true};
+  HeaderMap::ConstIterateCb encode_header_cb_;
+  HeaderKeyFormatter* header_key_formatter_;


nit: can this be const? (Both the pointer and the object)

mattklein123 · 2019-10-23T21:02:19Z

source/common/http/http1/header_formatter.cc

+std::string ProperCaseHeaderKeyFormatter::format(absl::string_view key) const {
+  auto copy = std::string(key);
+
+  bool shouldCapitalize = true;


Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Nice!

…voyproxy#8499) Adds a configuration option that will convert all header keys into Proper-Case. This is useful to allow Envoy to respond with headers that match the casing of other systems, to ensure that the wire format of responses is unchanged when migrating to Envoy. Fixes envoyproxy#8463 Signed-off-by: Snow Pettersen <snowp@squareup.com>

sunnoy · 2021-03-15T03:32:04Z

So how to configure the custom header format, for example, I want to get this format header：

Sec-WebSocket-Accept

not 

Sec-Websocket-Accept

@snowp

snowp changed the title ~~WIP http1: allow configuring format of encoded response headers~~ WIP http1: allow configuring format of encoded http1 response headers Oct 5, 2019

add missing file

2cf2291

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123 reviewed Oct 6, 2019

View reviewed changes

use oneof instead of enum

9312c48

Signed-off-by: Snow Pettersen <snowp@squareup.com>

htuch reviewed Oct 8, 2019

View reviewed changes

Snow Pettersen added 6 commits October 10, 2019 17:25

rename flag

42939ae

Signed-off-by: Snow Pettersen <snowp@squareup.com>

proto format

dc23103

Signed-off-by: Snow Pettersen <snowp@squareup.com>

add missing files

7d316f7

Signed-off-by: Snow Pettersen <snowp@squareup.com>

Merge remote-tracking branch 'envoy/master' into h1-casing

78a9236

Signed-off-by: Snow Pettersen <snowp@squareup.com>

format

ca60948

Signed-off-by: Snow Pettersen <snowp@squareup.com>

spelling

9efd2ff

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp changed the title ~~WIP http1: allow configuring format of encoded http1 response headers~~ http1: allow configuring format of encoded http1 response headers Oct 11, 2019

snowp marked this pull request as ready for review October 11, 2019 15:40

snowp assigned alyssawilk Oct 11, 2019

alyssawilk reviewed Oct 15, 2019

View reviewed changes

alyssawilk added the waiting label Oct 15, 2019

mattklein123 reviewed Oct 15, 2019

View reviewed changes

htuch reviewed Oct 16, 2019

View reviewed changes

repokitteh-read-only bot added the api label Oct 18, 2019

Snow Pettersen added 4 commits October 18, 2019 13:15

reuse lambda, fix spelling, use helper method

3cbeeb6

Signed-off-by: Snow Pettersen <snowp@squareup.com>

simplify signature for helper

0b6ecdd

Signed-off-by: Snow Pettersen <snowp@squareup.com>

formatting

16dccd7

Signed-off-by: Snow Pettersen <snowp@squareup.com>

add comments

af608d8

Signed-off-by: Snow Pettersen <snowp@squareup.com>

repokitteh-read-only bot added the waiting label Oct 21, 2019

Snow Pettersen added 3 commits October 21, 2019 14:55

more test cases

6805001

Signed-off-by: Snow Pettersen <snowp@squareup.com>

reformat codec_impl

7e965a1

Signed-off-by: Snow Pettersen <snowp@squareup.com>

reflow comment

8b555ce

Signed-off-by: Snow Pettersen <snowp@squareup.com>

repokitteh-read-only bot removed the waiting label Oct 21, 2019

Snow Pettersen added 3 commits October 21, 2019 15:29

Merge remote-tracking branch 'envoy/master' into h1-casing

e8af8b9

Signed-off-by: Snow Pettersen <snowp@squareup.com>

spelling

b201b16

Signed-off-by: Snow Pettersen <snowp@squareup.com>

formatting

f11ea28

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk reviewed Oct 22, 2019

View reviewed changes

Snow Pettersen added 2 commits October 22, 2019 09:27

remove redundant c_str() call

553adae

Signed-off-by: Snow Pettersen <snowp@squareup.com>

add comment about TE header

0cd5e84

Signed-off-by: Snow Pettersen <snowp@squareup.com>

alyssawilk previously approved these changes Oct 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 23, 2019

Merge remote-tracking branch 'envoy/master' into h1-casing

6e708cb

Signed-off-by: Snow Pettersen <snowp@squareup.com>

repokitteh-read-only bot removed the waiting label Oct 23, 2019

mattklein123 requested changes Oct 23, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 23, 2019

remove cb field, make pointer const and snake case local var

26b3b3f

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp dismissed alyssawilk’s stale review via 26b3b3f October 23, 2019 21:22

repokitteh-read-only bot removed the waiting label Oct 23, 2019

mattklein123 approved these changes Oct 23, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Oct 23, 2019

snowp merged commit 7846427 into envoyproxy:master Oct 24, 2019

igcherkaev mentioned this pull request Nov 5, 2019

Allow configuring header_key_format for envoy-proxy istio/istio#18672

Closed

Conversation

snowp commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 4, 2019

Uh oh!

snowp commented Oct 5, 2019

Uh oh!

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

htuch commented Oct 7, 2019

Uh oh!

snowp commented Oct 7, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

htuch commented Oct 8, 2019

Uh oh!

snowp commented Oct 11, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented Oct 4, 2019 •

edited

Loading

PiotrSikora Oct 8, 2019 •

edited

Loading