context: harden empty header cases #9862
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
source/common/grpc/common.cc
Outdated
| if (optional_status.has_value()) { | ||
| return optional_status; | ||
| if (trailers) { | ||
| auto status = Grpc::Common::getGrpcStatus(*trailers, allow_user_defined); |
There was a problem hiding this comment.
Minor perf optimizations to avoid unnecessary computation.
There was a problem hiding this comment.
I don't see any case outside of test passing nullptr to this? Did I miss anything?
There was a problem hiding this comment.
It's passed from context.cc which itself is used by Wasm context.cc. Wasm sets trailers to nullptr (a separate issue which we are debugging).
There was a problem hiding this comment.
IIRC right now we assume that headers are always passed to these functions, and static empty headers are passed in cases where there are none, which keeps the logic more simple. I would recommend you do the same from the WASM code unless there is a compelling reason to change this. If we do change it we should be consistent everywhere.
There was a problem hiding this comment.
Wasm captures local variables into object fields here, so it's natural to use nullptr as a default value (for performance reasons I assume to avoid de-allocating an optional). Generally speaking, I am not sure it's always right to replace missing headers with empty headers.
If there is a strong desire to use HeaderMap&, I can adapt the code to do that. WDYT?
There was a problem hiding this comment.
Originally I think the code was written this way to avoid having the conditional checks in a bunch of places. My point mainly is that I think we should be consistent here so I would take a look at all the callers.
There was a problem hiding this comment.
I reverted the changes outside of the context to keep the nullptr business confined to that unit. Using header map singleton consistent with other instances (e.g. access_log_base.cc).
Signed-off-by: Kuat Yessenov <kuat@google.com>
| @@ -141,15 +143,18 @@ absl::optional<CelValue> ResponseWrapper::operator[](CelValue key) const { | |||
| } else if (value == Flags) { | |||
| return CelValue::CreateInt64(info_.responseFlags()); | |||
| } else if (value == GrpcStatus) { | |||
| auto const& optional_status = | |||
| Grpc::Common::getGrpcStatus(*(trailers_.value_), *(headers_.value_), info_); | |||
| ConstSingleton<Http::HeaderMapImpl> empty_headers; | |||
There was a problem hiding this comment.
You don't need even to instantiate ConstSingleton here. Just use ConstSingleton<Http::HeaderMapImpl>::get().
There was a problem hiding this comment.
Ah yeah, it's a static member. Done.
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Cherry-pick of envoyproxy#9862
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov kuat@google.com
Description: envoy passes nullptr whenever either headers or trailers are not available. This change hardens the context to handle those cases.
Risk Level: low
Testing: added unit tests
Docs Changes: none
Release Notes: none
CC: @douglas-reid