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
Changes from 3 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
@@ -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",
],
)

18 changes: 14 additions & 4 deletions api/request_source/service.proto
Original file line number Diff line number Diff line change
@@ -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 {
@@ -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 {
// 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;
}
1 change: 1 addition & 0 deletions source/client/BUILD
Original file line number Diff line number Diff line change
@@ -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",
],
)

8 changes: 5 additions & 3 deletions source/client/service_impl.cc
Original file line number Diff line number Diff line change
@@ -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"
@@ -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));
@@ -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());
@@ -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
@@ -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",
],
)

26 changes: 22 additions & 4 deletions source/common/request_stream_grpc_client_impl.cc
Original file line number Diff line number Diff line change
@@ -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";

@@ -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);
3 changes: 3 additions & 0 deletions test/BUILD
Original file line number Diff line number Diff line change
@@ -325,8 +325,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",
],
)

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"

@@ -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.
@@ -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";
};
}

@@ -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, GenericHeaderFieldsUsingDeprecatedEnvoyV3Api) {
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
@@ -74,4 +104,5 @@ TEST_F(ProtoRequestHelperTest, AmbiguousHost) {
translateExpectingEqual();
}

} // namespace
} // namespace Nighthawk