Skip to content

Commit

Permalink
runtime: deprecate validate_grpc_header
Browse files Browse the repository at this point in the history 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
  • Loading branch information
@alyssawilk
alyssawilk committed Oct 22, 2024
1 parent 66febdf commit 2e20cc5
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 198 deletions.
5 changes: 4 additions & 1 deletion changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
- area: router
change: |
Removed runtime guard ``envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request``.
Removed runtime guard ``envoy.reloadable_features.send_local_reply_when_no_buffer_and_upstream_request``.
- area: grpc
change: |
Removed runtime guard ``envoy.reloadable_features.validate_grpc_header_before_log_grpc_status``.
- area: http
change: |
Removed runtime flag ``envoy.reloadable_features.http_route_connect_proxy_by_default`` and legacy code paths.
Expand Down
14 changes: 4 additions & 10 deletions source/common/formatter/http_specific_formatter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,8 @@ GrpcStatusFormatter::GrpcStatusFormatter(const std::string& main_header,
absl::optional<std::string>
GrpcStatusFormatter::formatWithContext(const HttpFormatterContext& context,
const StreamInfo::StreamInfo& info) const {
if (Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.validate_grpc_header_before_log_grpc_status")) {
if (!Grpc::Common::isGrpcRequestHeaders(context.requestHeaders())) {
return absl::nullopt;
}
if (!Grpc::Common::isGrpcRequestHeaders(context.requestHeaders())) {
return absl::nullopt;
}
const auto grpc_status = Grpc::Common::getGrpcStatus(context.responseTrailers(),
context.responseHeaders(), info, true);
Expand Down Expand Up @@ -242,11 +239,8 @@ GrpcStatusFormatter::formatWithContext(const HttpFormatterContext& context,
ProtobufWkt::Value
GrpcStatusFormatter::formatValueWithContext(const HttpFormatterContext& context,
const StreamInfo::StreamInfo& info) const {
if (Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.validate_grpc_header_before_log_grpc_status")) {
if (!Grpc::Common::isGrpcRequestHeaders(context.requestHeaders())) {
return SubstitutionFormatUtils::unspecifiedValue();
}
if (!Grpc::Common::isGrpcRequestHeaders(context.requestHeaders())) {
return SubstitutionFormatUtils::unspecifiedValue();
}
const auto grpc_status = Grpc::Common::getGrpcStatus(context.responseTrailers(),
context.responseHeaders(), info, true);
Expand Down
1 change: 0 additions & 1 deletion source/common/runtime/runtime_features.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ RUNTIME_GUARD(envoy_reloadable_features_use_http_client_to_fetch_aws_credentials
RUNTIME_GUARD(envoy_reloadable_features_use_route_host_mutation_for_auto_sni_san);
RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_listener);
RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status);
RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers);
RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled);
RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding);
Expand Down
186 changes: 0 additions & 186 deletions test/common/formatter/substitution_formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3294,65 +3294,6 @@ TEST(SubstitutionFormatterTest, GrpcStatusFormatterCamelStringTest) {
}
}

TEST(SubstitutionFormatterTest,
GrpcStatusFormatterCamelStringTest_validate_grpc_header_before_log_grpc_status) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues({
{"envoy.reloadable_features.validate_grpc_header_before_log_grpc_status", "false"},
});

GrpcStatusFormatter formatter("grpc-status", "", absl::optional<size_t>(),
GrpcStatusFormatter::Format::CamelString);
NiceMock<StreamInfo::MockStreamInfo> stream_info;
Http::TestRequestHeaderMapImpl request_header;
Http::TestResponseHeaderMapImpl response_header;
Http::TestResponseTrailerMapImpl response_trailer;
std::string body;

HttpFormatterContext formatter_context(&request_header, &response_header, &response_trailer,
body);

std::vector<std::string> grpc_statuses{
"OK", "Canceled", "Unknown", "InvalidArgument", "DeadlineExceeded",
"NotFound", "AlreadyExists", "PermissionDenied", "ResourceExhausted", "FailedPrecondition",
"Aborted", "OutOfRange", "Unimplemented", "Internal", "Unavailable",
"DataLoss", "Unauthenticated"};
for (size_t i = 0; i < grpc_statuses.size(); ++i) {
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", std::to_string(i)}};
EXPECT_EQ(grpc_statuses[i], formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue(grpc_statuses[i])));
}
{
response_trailer = Http::TestResponseTrailerMapImpl{{"not-a-grpc-status", "13"}};
EXPECT_EQ(absl::nullopt, formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::nullValue()));
}
{
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "-1"}};
EXPECT_EQ("-1", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("-1")));
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "42738"}};
EXPECT_EQ("42738", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("42738")));
response_trailer.clear();
}
{
response_header = Http::TestResponseHeaderMapImpl{{"grpc-status", "-1"}};
EXPECT_EQ("-1", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("-1")));
response_header = Http::TestResponseHeaderMapImpl{{"grpc-status", "42738"}};
EXPECT_EQ("42738", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("42738")));
response_header.clear();
}
}

TEST(SubstitutionFormatterTest, GrpcStatusFormatterSnakeStringTest) {
GrpcStatusFormatter formatter("grpc-status", "", absl::optional<size_t>(),
GrpcStatusFormatter::Format::SnakeString);
Expand Down Expand Up @@ -3439,77 +3380,6 @@ TEST(SubstitutionFormatterTest, GrpcStatusFormatterSnakeStringTest) {
}
}

TEST(SubstitutionFormatterTest,
GrpcStatusFormatterSnakeStringTest_validate_grpc_header_before_log_grpc_status) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues({
{"envoy.reloadable_features.validate_grpc_header_before_log_grpc_status", "false"},
});

GrpcStatusFormatter formatter("grpc-status", "", absl::optional<size_t>(),
GrpcStatusFormatter::Format::SnakeString);
NiceMock<StreamInfo::MockStreamInfo> stream_info;
Http::TestRequestHeaderMapImpl request_header;
Http::TestResponseHeaderMapImpl response_header;
Http::TestResponseTrailerMapImpl response_trailer;
std::string body;

HttpFormatterContext formatter_context(&request_header, &response_header, &response_trailer,
body);

std::vector<std::string> grpc_statuses{"OK",
"CANCELLED",
"UNKNOWN",
"INVALID_ARGUMENT",
"DEADLINE_EXCEEDED",
"NOT_FOUND",
"ALREADY_EXISTS",
"PERMISSION_DENIED",
"RESOURCE_EXHAUSTED",
"FAILED_PRECONDITION",
"ABORTED",
"OUT_OF_RANGE",
"UNIMPLEMENTED",
"INTERNAL",
"UNAVAILABLE",
"DATA_LOSS",
"UNAUTHENTICATED"};
for (size_t i = 0; i < grpc_statuses.size(); ++i) {
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", std::to_string(i)}};
EXPECT_EQ(grpc_statuses[i], formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue(grpc_statuses[i])));
}
{
response_trailer = Http::TestResponseTrailerMapImpl{{"not-a-grpc-status", "13"}};
EXPECT_EQ(absl::nullopt, formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::nullValue()));
}
{
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "-1"}};
EXPECT_EQ("-1", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("-1")));
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "42738"}};
EXPECT_EQ("42738", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("42738")));
response_trailer.clear();
}
{
response_header = Http::TestResponseHeaderMapImpl{{"grpc-status", "-1"}};
EXPECT_EQ("-1", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("-1")));
response_header = Http::TestResponseHeaderMapImpl{{"grpc-status", "42738"}};
EXPECT_EQ("42738", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::stringValue("42738")));
response_header.clear();
}
}

TEST(SubstitutionFormatterTest, GrpcStatusFormatterNumberTest) {
GrpcStatusFormatter formatter("grpc-status", "", absl::optional<size_t>(),
GrpcStatusFormatter::Format::Number);
Expand Down Expand Up @@ -3581,62 +3451,6 @@ TEST(SubstitutionFormatterTest, GrpcStatusFormatterNumberTest) {
}
}

TEST(SubstitutionFormatterTest,
GrpcStatusFormatterNumberTest_validate_grpc_header_before_log_grpc_status) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues({
{"envoy.reloadable_features.validate_grpc_header_before_log_grpc_status", "false"},
});

GrpcStatusFormatter formatter("grpc-status", "", absl::optional<size_t>(),
GrpcStatusFormatter::Format::Number);
NiceMock<StreamInfo::MockStreamInfo> stream_info;
Http::TestRequestHeaderMapImpl request_header;
Http::TestResponseHeaderMapImpl response_header;
Http::TestResponseTrailerMapImpl response_trailer;
std::string body;

HttpFormatterContext formatter_context(&request_header, &response_header, &response_trailer,
body);

const int grpcStatuses = static_cast<int>(Grpc::Status::WellKnownGrpcStatus::MaximumKnown) + 1;

for (size_t i = 0; i < grpcStatuses; ++i) {
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", std::to_string(i)}};
EXPECT_EQ(std::to_string(i), formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::numberValue(i)));
}
{
response_trailer = Http::TestResponseTrailerMapImpl{{"not-a-grpc-status", "13"}};
EXPECT_EQ(absl::nullopt, formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::nullValue()));
}
{
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "-1"}};
EXPECT_EQ("-1", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::numberValue(-1)));
response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "42738"}};
EXPECT_EQ("42738", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::numberValue(42738)));
response_trailer.clear();
}
{
response_header = Http::TestResponseHeaderMapImpl{{"grpc-status", "-1"}};
EXPECT_EQ("-1", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::numberValue(-1)));
response_header = Http::TestResponseHeaderMapImpl{{"grpc-status", "42738"}};
EXPECT_EQ("42738", formatter.formatWithContext(formatter_context, stream_info));
EXPECT_THAT(formatter.formatValueWithContext(formatter_context, stream_info),
ProtoEq(ValueUtil::numberValue(42738)));
response_header.clear();
}
}

void verifyStructOutput(ProtobufWkt::Struct output,
absl::node_hash_map<std::string, std::string> expected_map) {
for (const auto& pair : expected_map) {
Expand Down

0 comments on commit 2e20cc5

Please sign in to comment.