Skip to content
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.

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

Deprecate Envoy API v2 primitives in the service proto for Request Source. #589

Merged
merged 5 commits into from
Dec 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions api/request_source/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ api_cc_py_proto_library(
"@envoy_api//envoy/api/v2/auth:pkg",
"@envoy_api//envoy/api/v2/cluster:pkg",
"@envoy_api//envoy/api/v2/core:pkg",
"@envoy_api//envoy/config/core/v3:pkg",
],
)

Expand Down
18 changes: 14 additions & 4 deletions api/request_source/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ syntax = "proto3";

package nighthawk.request_source;

import "google/protobuf/wrappers.proto";
import "envoy/api/v2/core/base.proto";
import "envoy/config/core/v3/base.proto";
import "google/protobuf/wrappers.proto";

// Used to request a RequestStreamResponse.
message RequestStreamRequest {
Expand Down Expand Up @@ -37,9 +38,18 @@ message RequestSpecifier {
// Request content length. The client will transfer the number of bytes specified here for the
// request body.
google.protobuf.UInt32Value content_length = 4;
// Request header replacements. Any existing header(s) with the same name will be removed
// before setting.
envoy.api.v2.core.HeaderMap headers = 5;

oneof oneof_headers {
mum4k marked this conversation as resolved.
Show resolved Hide resolved
// Request header replacements. Any existing header(s) with the same name will be removed
// before setting.
//
// Envoy deprecated its v2 API, prefer to use v3_headers instead.
envoy.api.v2.core.HeaderMap headers = 5 [deprecated = true];

// Request header replacements. Any existing header(s) with the same name will be removed
// before setting.
envoy.config.core.v3.HeaderMap v3_headers = 6;
}
// TODO(oschaaf): nice to have
// google.protobuf.StringValue sni_hostname = 10;
}
Expand Down
1 change: 1 addition & 0 deletions source/client/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ envoy_cc_library(
"//api/client:grpc_service_lib",
"//api/request_source:grpc_request_source_service_lib",
"@envoy//source/common/common:thread_lib_with_external_headers",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)

Expand Down
8 changes: 5 additions & 3 deletions source/client/service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include <grpc++/grpc++.h>

#include "envoy/config/core/v3/base.pb.h"

#include "common/request_source_impl.h"

#include "client/client.h"
Expand Down Expand Up @@ -106,7 +108,7 @@ ::grpc::Status ServiceImpl::ExecutionStream(
}

namespace {
void addHeader(envoy::api::v2::core::HeaderMap* map, absl::string_view key,
void addHeader(envoy::config::core::v3::HeaderMap* map, absl::string_view key,
absl::string_view value) {
auto* request_header = map->add_headers();
request_header->set_key(std::string(key));
Expand Down Expand Up @@ -144,7 +146,7 @@ ::grpc::Status RequestSourceServiceImpl::RequestStream(
HeaderMapPtr headers = request->header();
nighthawk::request_source::RequestStreamResponse response;
auto* request_specifier = response.mutable_request_specifier();
auto* request_headers = request_specifier->mutable_headers();
auto* request_headers = request_specifier->mutable_v3_headers();
headers->iterate([&request_headers](const Envoy::Http::HeaderEntry& header)
-> Envoy::Http::HeaderMap::Iterate {
addHeader(request_headers, header.key().getStringView(), header.value().getStringView());
Expand All @@ -163,4 +165,4 @@ ::grpc::Status RequestSourceServiceImpl::RequestStream(
}

} // namespace Client
} // namespace Nighthawk
} // namespace Nighthawk
2 changes: 2 additions & 0 deletions source/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ envoy_cc_library(
"@envoy//source/common/grpc:typed_async_client_lib_with_external_headers",
"@envoy//source/common/http:header_map_lib_with_external_headers",
"@envoy//source/common/http:headers_lib_with_external_headers",
"@envoy_api//envoy/api/v2/core:pkg_cc_proto",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)

Expand Down
26 changes: 22 additions & 4 deletions source/common/request_stream_grpc_client_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,22 @@

#include <string>

#include "envoy/api/v2/core/base.pb.h"
#include "envoy/config/core/v3/base.pb.h"
#include "envoy/stats/scope.h"

#include "external/envoy/source/common/common/assert.h"
#include "external/envoy/source/common/http/header_map_impl.h"
#include "external/envoy/source/common/http/headers.h"

#include "api/request_source/service.pb.h"

#include "common/request_impl.h"

namespace Nighthawk {

using ::nighthawk::request_source::RequestSpecifier;

const std::string RequestStreamGrpcClientImpl::METHOD_NAME =
"nighthawk.request_source.NighthawkRequestSourceService.RequestStream";

Expand Down Expand Up @@ -75,15 +81,27 @@ RequestPtr ProtoRequestHelper::messageToRequest(
RequestPtr request = std::make_unique<RequestImpl>(header);

if (message.has_request_specifier()) {
const auto& request_specifier = message.request_specifier();
if (request_specifier.has_headers()) {
const auto& message_request_headers = request_specifier.headers();
for (const auto& message_header : message_request_headers.headers()) {
const RequestSpecifier& request_specifier = message.request_specifier();

if (request_specifier.has_v3_headers()) {
const envoy::config::core::v3::HeaderMap& message_request_headers =
request_specifier.v3_headers();
for (const envoy::config::core::v3::HeaderValue& message_header :
message_request_headers.headers()) {
Envoy::Http::LowerCaseString header_name(message_header.key());
header->remove(header_name);
header->addCopy(header_name, message_header.value());
}
} else if (request_specifier.has_headers()) {
const envoy::api::v2::core::HeaderMap& message_request_headers = request_specifier.headers();
for (const envoy::api::v2::core::HeaderValue& message_header :
message_request_headers.headers()) {
Envoy::Http::LowerCaseString header_name(message_header.key());
header->remove(header_name);
header->addCopy(header_name, message_header.value());
}
}

if (request_specifier.has_content_length()) {
std::string s_content_length = absl::StrCat("", request_specifier.content_length().value());
header->remove(Envoy::Http::Headers::get().ContentLength);
Expand Down
3 changes: 3 additions & 0 deletions test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,11 @@ envoy_cc_test(
srcs = ["request_stream_grpc_client_test.cc"],
repository = "@envoy",
deps = [
"//api/request_source:grpc_request_source_service_lib",
"//source/common:request_stream_grpc_client_lib",
"@envoy//test/test_common:utility_lib",
"@envoy_api//envoy/api/v2/core:pkg_cc_proto",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)

Expand Down
45 changes: 38 additions & 7 deletions test/request_stream_grpc_client_test.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#include "envoy/api/v2/core/base.pb.h"
#include "envoy/config/core/v3/base.pb.h"

#include "external/envoy/test/test_common/utility.h"

#include "api/request_source/service.pb.h"

#include "common/request_impl.h"
#include "common/request_stream_grpc_client_impl.h"

Expand All @@ -8,6 +13,9 @@
using namespace testing;

namespace Nighthawk {
namespace {

using ::nighthawk::request_source::RequestSpecifier;

// The grpc client itself is tested via the python based integration tests.
// It is convenient to test message translation here.
Expand All @@ -18,7 +26,9 @@ class ProtoRequestHelperTest : public Test {
// We test for equality. If we observe mismatch, we use EXPECT_EQ which is guaranteed
// to fail -- but will provide much more helpful output.
if (!Envoy::TestUtility::headerMapEqualIgnoreOrder(expected_header_, *request->header())) {
EXPECT_EQ(expected_header_, *request->header());
EXPECT_EQ(expected_header_, *request->header()) << "expected headers:\n"
<< expected_header_ << "\nactual headers:\n"
<< *request->header() << "\n";
};
}

Expand All @@ -42,14 +52,34 @@ TEST_F(ProtoRequestHelperTest, ExplicitFields) {
translateExpectingEqual();
}

// Test the generic header api we offer in the proto api.
TEST_F(ProtoRequestHelperTest, GenericHeaderFields) {
auto* request_specifier = response_.mutable_request_specifier();
auto* headers = request_specifier->mutable_headers();
auto* header_1 = headers->add_headers();
// Test the generic header api we offer in the proto api using Envoy API v2
// primitives.
TEST_F(ProtoRequestHelperTest, GenericHeaderFieldsUsingDeprecatedEnvoyV2Api) {
RequestSpecifier* request_specifier = response_.mutable_request_specifier();
envoy::api::v2::core::HeaderMap* headers = request_specifier->mutable_headers();
envoy::api::v2::core::HeaderValue* header_1 = headers->add_headers();
header_1->set_key("header1");
header_1->set_value("value1");
envoy::api::v2::core::HeaderValue* header_2 = headers->add_headers();
header_2->set_key("header2");
header_2->set_value("value2");
// We re-add the same header, but do not expect that to show up in the translation because we
// always replace.
headers->add_headers()->MergeFrom(*header_2);
expected_header_ =
Envoy::Http::TestRequestHeaderMapImpl{{"header1", "value1"}, {"header2", "value2"}};
translateExpectingEqual();
}

// Test the generic header api we offer in the proto api using Envoy API v3
// primitives.
TEST_F(ProtoRequestHelperTest, GenericHeaderFieldsUsingEnvoyV3Api) {
RequestSpecifier* request_specifier = response_.mutable_request_specifier();
envoy::config::core::v3::HeaderMap* headers = request_specifier->mutable_v3_headers();
envoy::config::core::v3::HeaderValue* header_1 = headers->add_headers();
header_1->set_key("header1");
header_1->set_value("value1");
auto* header_2 = headers->add_headers();
envoy::config::core::v3::HeaderValue* header_2 = headers->add_headers();
header_2->set_key("header2");
header_2->set_value("value2");
// We re-add the same header, but do not expect that to show up in the translation because we
Expand All @@ -74,4 +104,5 @@ TEST_F(ProtoRequestHelperTest, AmbiguousHost) {
translateExpectingEqual();
}

} // namespace
} // namespace Nighthawk