thrift: add support for case-sensitive header keys

[rgs1]

Based off #20596, this preserves the current behavior by default.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com
Co-authored-by: fishy

[repokitteh-read-only]

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

🐱

Caused by: #20893 was opened by rgs1.

see: more, trace.

[rgs1]

cc: @fishcakez @JuniorHsu

[rgs1]

cc: @davinci26 @fishy

[rgs1]

@htuch mind taking a look at API please?

@zuercher this is ready for general review too.

Thanks!

[htuch]

/lgtm api

[wbpcode]

Altough, by this way we can insert some case insensitive header key to the Http::HeaderMap, here are some other problems.

The headers will be used in the header matching or getting cluster with cluster_header or some other scenes.
And the keys in these config will always be convert to lower case. Because the get API of Http::HeaderMap only accept key of type Http::LowerCaseString. This will result in some weird results.
For example, if we set cluster_header to "Key", and there is related "Key" and "Value" in the headers. If we set header_keys_case_sensitive to true, we can never get the valid header value.

I think the key point is that we shouldn't use the Http::HeaderMap as the header map type of thrift. The Http::HeaderMap is designed for HTTP (lower case key, multiple value for same key), and headers of thrift should be a normal string map (ref https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/THeaderTransport.h).

(Dubbo has the similar problem.)

[wbpcode]

/assign

[wbpcode]

/wait-any

[rgs1]

Altough, by this way we can insert some case insensitive header key to the Http::HeaderMap, here are some other problems.

The headers will be used in the header matching or getting cluster with cluster_header or some other scenes. And the keys in these config will always be convert to lower case. Because the get API of Http::HeaderMap only accept key of type Http::LowerCaseString. This will result in some weird results. For example, if we set cluster_header to "Key", and there is related "Key" and "Value" in the headers. If we set header_keys_case_sensitive to true, we can never get the valid header value.

I think the key point is that we shouldn't use the Http::HeaderMap as the header map type of thrift. The Http::HeaderMap is designed for HTTP (lower case key, multiple value for same key), and headers of thrift should be a normal string map (ref https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/THeaderTransport.h).

(Dubbo has the similar problem.)

I think we can document all of these limitations and still move forward with this. Speaking of HTTP, we do support case preservation there (see #15619).

cc: @fishy

[wbpcode]

I think we can document all of these limitations and still move forward with this. Speaking of HTTP, we do support case preservation there (see #15619).

Yeah, but the seaching of header key will no be influenced by this preservation (#15619). And the approach in this PR does.

IMO, I think it would be better to replace Http::HeaderMap in the thrift proxy with absl::flat_hash_map<std::strinig, std::string>.
Or we can use the approach in the #15619 to preserve the case-sensitive key.

[rgs1]

I think we can document all of these limitations and still move forward with this. Speaking of HTTP, we do support case preservation there (see #15619).

Yeah, but the seaching of header key will no be influenced by this preservation (#15619). And the approach in this PR does.

IMO, I think it would be better to replace Http::HeaderMap in the thrift proxy with absl::flat_hash_map<std::strinig, std::string>. Or we can use the approach in the #15619 to preserve the case-sensitive key.

Replacing Http::HeaderMap might sound tempting, but we rely on it for some things:

https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/network/thrift_proxy/router/router_impl.cc#L117

So it sounds like the proper way of doing this would be the approach in #15619 (at the cost of using more memory).

[wbpcode]

Yeah, the approach in the #15619 is best way for now.

(FWIW, may be we can try to make the http header match utility a template, then it can be easily reused by other type headers (e.g. absl::flat_hash_map<std::strinig, std::string>. I will have a try. Because the dubbo also need this.)

[rgs1]

@wbpcode ok I pushed something similar to #15619 but less complex (e.g.: the formatter is hard coded because, at least right now, we don't need a whole new plugging system for headers formatting in Thrift). Let me know what you think, thanks!

[rgs1]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20893 (comment) was created by @rgs1.

see: more, trace.

[rgs1]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20893 (comment) was created by @rgs1.

see: more, trace.

[wbpcode]

LGTM. Thanks for your great work. cc @zuercher for final pass or merge.

[wbpcode]

cc @htuch could your give another lgtm for the API? Thanks.

[htuch]

/lgtm api

[rgs1]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20893 (comment) was created by @rgs1.

see: more, trace.

[rgs1]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20893 (comment) was created by @rgs1.

see: more, trace.

[rgs1]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20893 (comment) was created by @rgs1.

see: more, trace.

[rgs1]

@zuercher this is ready for another pass (the macos failure was something else, possibly transient).

[zuercher]

Looks good!