http: remove getAll() header map API and switch all usages to get() #13363
Conversation
This forces all callers to think about multiple header values. Signed-off-by: Matt Klein <[email protected]>
mattklein123
requested review from
alyssawilk,
dio,
jmarantz,
lizan and
zuercher
as code owners
|
@alyssawilk lmk if you want a comment for every converted prod code site. I think I had that in setec at some point and can bring it back. |
|
yeah, I think any point we're automatically pulling [0] we should comment why that's OK. |
|
OK I will update. |
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
|
@alyssawilk updated |
source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc
Show resolved
Hide resolved
| @@ -200,7 +200,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { | |||
| // Currently it is not possible to add {{"append": "1"}, {"append": "2"}} (the intended | |||
| // combined headers: {{"original": "true"}, {"append": "1"}, {"append": "2"}}) to the request | |||
| // to upstream server by only sets `headers_to_append`. | |||
| if (header_to_modify != nullptr) { | |||
| if (!header_to_modify.empty()) { | |||
There was a problem hiding this comment.
In general I wonder if it's worth a docs comment somewhere that header to append appends to first value.
If had a tracing header in a non-envoy server which did multi-value,
Trace: ServerA
Trace: ServerB
I think Envoy as server C would generally end up
Trace: ServerA, ServerC
Trace: ServerB.
We offer no header order guarantees so really anyone who cares about order is ideally doing inline appends anyway, so optional.
There was a problem hiding this comment.
Honestly I find this code mind boggingly confusing every time I look at this, between gRPC/HTTP/etc. @dio do you mind taking a look and seeing if we can improve the docs here for extauth about this behavior?
Signed-off-by: Matt Klein <[email protected]>
|
Once we agree on this PR I'm going to need to update envoy-filter-example to get this to pass FYI. |
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
@alyssawilk I think I covered most of the comments. PTAL. When you are satisfied I will fix envoy-filter-example and we can merge in lockstep. |
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
To go with envoyproxy/envoy#13363 Signed-off-by: Matt Klein <[email protected]>
|
See envoyproxy/envoy-filter-example#135 for required filter example fix. |
|
Hm, looks like CI needs a pass and it needs a merge though. I'll do another pass once you have that sorted. |
To go with envoyproxy/envoy#13363 Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
| // TODO(kyessenov, PiotrSikora): This needs to either return a concatenated list of values, or | ||
| // the ABI needs to be changed to return multiple values. This is a potential security issue. |
There was a problem hiding this comment.
@kyessenov @PiotrSikora this needs to be fixed ASAP
There was a problem hiding this comment.
(Talked to @kyessenov offline about this and it will be fixed soon)
There was a problem hiding this comment.
FWIW, the ABI in proxy-wasm/spec#1 already returns multiple values.
There was a problem hiding this comment.
Perfect. Let's do that then.
|
@alyssawilk I need another approve if you have a sec. |
This is a follow up to 2c60632.
This forces all callers to think about multiple header values. There may be places that we want
to support multiple values, but none of them are security critical and this change should be
functionally equivalent to what exists today.
Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A