Skip to content

context: harden empty header cases #9862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 3 commits into from
Jan 29, 2020
Merged

context: harden empty header cases #9862

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0395b2
context: harden empty header cases
kyessenov Jan 28, 2020
623e628
use empty headers by default
kyessenov Jan 29, 2020
af60098
use static member
kyessenov Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/common/access_log/access_log_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ bool GrpcStatusFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::

Grpc::Status::GrpcStatus status = Grpc::Status::WellKnownGrpcStatus::Unknown;
const auto& optional_status =
Grpc::Common::getGrpcStatus(response_trailers, response_headers, info);
Grpc::Common::getGrpcStatus(&response_trailers, &response_headers, info);
if (optional_status.has_value()) {
status = optional_status.value();
}
Expand Down
28 changes: 15 additions & 13 deletions source/common/grpc/common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap&
return {static_cast<Status::GrpcStatus>(grpc_status_code)};
}

absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap& trailers,
const Http::HeaderMap& headers,
absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap* trailers,
const Http::HeaderMap* headers,
const StreamInfo::StreamInfo& info,
bool allow_user_defined) {
// The gRPC specification does not guarantee a gRPC status code will be returned from a gRPC
Expand All @@ -74,19 +74,21 @@ absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap&
// 1. trailers gRPC status, if it exists.
// 2. headers gRPC status, if it exists.
// 3. Inferred from info HTTP status, if it exists.
const std::array<absl::optional<Grpc::Status::GrpcStatus>, 3> optional_statuses = {{
{Grpc::Common::getGrpcStatus(trailers, allow_user_defined)},
{Grpc::Common::getGrpcStatus(headers, allow_user_defined)},
{info.responseCode() ? absl::optional<Grpc::Status::GrpcStatus>(
Grpc::Utility::httpToGrpcStatus(info.responseCode().value()))
: absl::nullopt},
}};

for (const auto& optional_status : optional_statuses) {
if (optional_status.has_value()) {
return optional_status;
if (trailers) {
auto status = Grpc::Common::getGrpcStatus(*trailers, allow_user_defined);

@kyessenov kyessenov Jan 28, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor perf optimizations to avoid unnecessary computation.

@lizan lizan Jan 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any case outside of test passing nullptr to this? Did I miss anything?

@kyessenov kyessenov Jan 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@mattklein123 mattklein123 Jan 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kyessenov kyessenov Jan 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@mattklein123 mattklein123 Jan 29, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kyessenov kyessenov Jan 29, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if (status.has_value()) {
return status;
}
}
if (headers) {
auto status = Grpc::Common::getGrpcStatus(*headers, allow_user_defined);
if (status.has_value()) {
return status;
}
}
if (info.responseCode()) {
return Grpc::Utility::httpToGrpcStatus(info.responseCode().value());
}

return absl::nullopt;
}
Expand Down
4 changes: 2 additions & 2 deletions source/common/grpc/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ class Common {
* @return absl::optional<Status::GrpcStatus> the parsed status code or absl::nullopt if no status
* is found
*/
static absl::optional<Status::GrpcStatus> getGrpcStatus(const Http::HeaderMap& trailers,
const Http::HeaderMap& headers,
static absl::optional<Status::GrpcStatus> getGrpcStatus(const Http::HeaderMap* trailers,
const Http::HeaderMap* headers,
const StreamInfo::StreamInfo& info,
bool allow_user_defined = false);

Expand Down
12 changes: 7 additions & 5 deletions source/extensions/filters/common/expr/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ absl::optional<CelValue> RequestWrapper::operator[](CelValue key) const {
} else {
return CelValue::CreateInt64(info_.bytesReceived());
}
} else if (value == TotalSize) {
return CelValue::CreateInt64(info_.bytesReceived() +
(headers_.value_ ? headers_.value_->byteSize() : 0));
} else if (value == Duration) {
auto duration = info_.requestComplete();
if (duration.has_value()) {
Expand Down Expand Up @@ -115,8 +118,6 @@ absl::optional<CelValue> RequestWrapper::operator[](CelValue key) const {
return convertHeaderEntry(headers_.value_->RequestId());
} else if (value == UserAgent) {
return convertHeaderEntry(headers_.value_->UserAgent());
} else if (value == TotalSize) {
return CelValue::CreateInt64(info_.bytesReceived() + headers_.value_->byteSize());
}
}
return {};
Expand All @@ -142,14 +143,15 @@ absl::optional<CelValue> ResponseWrapper::operator[](CelValue key) const {
return CelValue::CreateInt64(info_.responseFlags());
} else if (value == GrpcStatus) {
auto const& optional_status =
Grpc::Common::getGrpcStatus(*(trailers_.value_), *(headers_.value_), info_);
Grpc::Common::getGrpcStatus(trailers_.value_, headers_.value_, info_);
if (optional_status.has_value()) {
return CelValue::CreateInt64(optional_status.value());
}
return {};
} else if (value == TotalSize) {
return CelValue::CreateInt64(info_.bytesSent() + headers_.value_->byteSize() +
trailers_.value_->byteSize());
return CelValue::CreateInt64(info_.bytesSent() +
(headers_.value_ ? headers_.value_->byteSize() : 0) +
(trailers_.value_ ? trailers_.value_->byteSize() : 0));
}
return {};
}
Expand Down
12 changes: 8 additions & 4 deletions test/common/grpc/common_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,18 @@ TEST(GrpcContextTest, GetGrpcStatusWithFallbacks) {
NiceMock<StreamInfo::MockStreamInfo> info;
EXPECT_CALL(info, responseCode()).WillRepeatedly(testing::Return(404));

EXPECT_EQ(Status::Ok, Common::getGrpcStatus(ok_status, no_status, info).value());
EXPECT_EQ(Status::Ok, Common::getGrpcStatus(&ok_status, &no_status, info).value());
EXPECT_EQ(Status::Ok, Common::getGrpcStatus(&ok_status, nullptr, info).value());

EXPECT_EQ(Status::Ok, Common::getGrpcStatus(no_status, ok_status, info).value());
EXPECT_EQ(Status::Ok, Common::getGrpcStatus(&no_status, &ok_status, info).value());
EXPECT_EQ(Status::Ok, Common::getGrpcStatus(nullptr, &ok_status, info).value());

EXPECT_EQ(Status::Unimplemented, Common::getGrpcStatus(no_status, no_status, info).value());
EXPECT_EQ(Status::Unimplemented, Common::getGrpcStatus(&no_status, &no_status, info).value());
EXPECT_EQ(Status::Unimplemented, Common::getGrpcStatus(nullptr, nullptr, info).value());

NiceMock<StreamInfo::MockStreamInfo> info_without_code;
EXPECT_FALSE(Common::getGrpcStatus(no_status, no_status, info_without_code));
EXPECT_FALSE(Common::getGrpcStatus(&no_status, &no_status, info_without_code));
EXPECT_FALSE(Common::getGrpcStatus(nullptr, nullptr, info_without_code));
}

TEST(GrpcContextTest, GetGrpcMessage) {
Expand Down
43 changes: 43 additions & 0 deletions test/extensions/filters/common/expr/context_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ TEST(Context, EmptyHeadersAttributes) {

TEST(Context, RequestAttributes) {
NiceMock<StreamInfo::MockStreamInfo> info;
NiceMock<StreamInfo::MockStreamInfo> empty_info;
Http::TestHeaderMapImpl header_map{
{":method", "POST"}, {":scheme", "http"}, {":path", "/meow?yes=1"},
{":authority", "kittens.com"}, {"referer", "dogs.com"}, {"user-agent", "envoy-mobile"},
{"content-length", "10"}, {"x-request-id", "blah"},
};
RequestWrapper request(&header_map, info);
RequestWrapper empty_request(nullptr, empty_info);

EXPECT_CALL(info, bytesReceived()).WillRepeatedly(Return(10));
// "2018-04-03T23:06:09.123Z".
Expand Down Expand Up @@ -67,6 +69,12 @@ TEST(Context, RequestAttributes) {
ASSERT_TRUE(value.value().IsString());
EXPECT_EQ("http", value.value().StringOrDie().value());
}

{
auto value = empty_request[CelValue::CreateStringView(Scheme)];
EXPECT_FALSE(value.has_value());
}

{
auto value = request[CelValue::CreateStringView(Host)];
EXPECT_TRUE(value.has_value());
Expand Down Expand Up @@ -131,6 +139,14 @@ TEST(Context, RequestAttributes) {
EXPECT_EQ(138, value.value().Int64OrDie());
}

{
auto value = empty_request[CelValue::CreateStringView(TotalSize)];
EXPECT_TRUE(value.has_value());
ASSERT_TRUE(value.value().IsInt64());
// this includes the headers size
EXPECT_EQ(0, value.value().Int64OrDie());
}

{
auto value = request[CelValue::CreateStringView(Time)];
EXPECT_TRUE(value.has_value());
Expand Down Expand Up @@ -159,12 +175,22 @@ TEST(Context, RequestAttributes) {
EXPECT_EQ("15ms", absl::FormatDuration(value.value().DurationOrDie()));
}

{
auto value = empty_request[CelValue::CreateStringView(Duration)];
EXPECT_FALSE(value.has_value());
}

{
auto value = request[CelValue::CreateStringView(Protocol)];
EXPECT_TRUE(value.has_value());
ASSERT_TRUE(value.value().IsString());
EXPECT_EQ("HTTP/2", value.value().StringOrDie().value());
}

{
auto value = empty_request[CelValue::CreateStringView(Protocol)];
EXPECT_FALSE(value.has_value());
}
}

TEST(Context, RequestFallbackAttributes) {
Expand Down Expand Up @@ -195,12 +221,14 @@ TEST(Context, RequestFallbackAttributes) {

TEST(Context, ResponseAttributes) {
NiceMock<StreamInfo::MockStreamInfo> info;
NiceMock<StreamInfo::MockStreamInfo> empty_info;
const std::string header_name = "test-header";
const std::string trailer_name = "test-trailer";
const std::string grpc_status = "grpc-status";
Http::TestHeaderMapImpl header_map{{header_name, "a"}};
Http::TestHeaderMapImpl trailer_map{{trailer_name, "b"}, {grpc_status, "8"}};
ResponseWrapper response(&header_map, &trailer_map, info);
ResponseWrapper empty_response(nullptr, nullptr, empty_info);

EXPECT_CALL(info, responseCode()).WillRepeatedly(Return(404));
EXPECT_CALL(info, bytesSent()).WillRepeatedly(Return(123));
Expand Down Expand Up @@ -230,6 +258,13 @@ TEST(Context, ResponseAttributes) {
EXPECT_EQ(160, value.value().Int64OrDie());
}

{
auto value = empty_response[CelValue::CreateStringView(TotalSize)];
EXPECT_TRUE(value.has_value());
ASSERT_TRUE(value.value().IsInt64());
EXPECT_EQ(0, value.value().Int64OrDie());
}

{
auto value = response[CelValue::CreateStringView(Code)];
EXPECT_TRUE(value.has_value());
Expand Down Expand Up @@ -267,18 +302,26 @@ TEST(Context, ResponseAttributes) {
ASSERT_TRUE(header.value().IsString());
EXPECT_EQ("b", header.value().StringOrDie().value());
}

{
auto value = response[CelValue::CreateStringView(Flags)];
EXPECT_TRUE(value.has_value());
ASSERT_TRUE(value.value().IsInt64());
EXPECT_EQ(0x1, value.value().Int64OrDie());
}

{
auto value = response[CelValue::CreateStringView(GrpcStatus)];
EXPECT_TRUE(value.has_value());
ASSERT_TRUE(value.value().IsInt64());
EXPECT_EQ(0x8, value.value().Int64OrDie());
}

{
auto value = empty_response[CelValue::CreateStringView(GrpcStatus)];
EXPECT_FALSE(value.has_value());
}

{
Http::TestHeaderMapImpl header_map{{header_name, "a"}, {grpc_status, "7"}};
Http::TestHeaderMapImpl trailer_map{{trailer_name, "b"}};
Expand Down