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

Jump to bottom

router: on shadowing, add "-shadow" to host, not after port #2600

Merged
merged 9 commits into from Feb 20, 2018
Merged

router: on shadowing, add "-shadow" to host, not after port #2600

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
10cbe81
Split port from host then append -shadow
dio Feb 13, 2018
8bb59d3
Use std::forward_as_tuple
dio Feb 13, 2018
04cba1e
Do splitToken only when it has :
dio Feb 13, 2018
0d0ecc9
Move comment
dio Feb 13, 2018
afe8110
Add test for host with port
dio Feb 14, 2018
4003529
Merge remote-tracking branch 'upstream/master' into split-port
dio Feb 15, 2018
eda6c13
Refactor test
dio Feb 15, 2018
7d7e613
Refactor shadow writer implementation
dio Feb 16, 2018
8a7ace3
Remove 'a'
dio Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 10 additions & 7 deletions source/common/router/shadow_writer_impl.cc
View file
Open in desktop
Expand Up @@ -6,18 +6,21 @@
#include "common/common/assert.h"
#include "common/http/headers.h"

#include "absl/strings/str_join.h"

namespace Envoy {
namespace Router {

void ShadowWriterImpl::shadow(const std::string& cluster, Http::MessagePtr&& request,
std::chrono::milliseconds timeout) {
// Switch authority to add a shadow postfix. This allows upstream logging to make a more sense.
// TODO PERF: Avoid copy.
std::string host = request->headers().Host()->value().c_str();
ASSERT(!host.empty());
host += "-shadow";
request->headers().Host()->value(host);

ASSERT(!request->headers().Host()->value().empty());
// Switch authority to add a shadow postfix. This allows upstream logging to make a more
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do you mind fixing the second sentence to not have 'a' in 'to make a more'.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

// sense.
auto parts = StringUtil::splitToken(request->headers().Host()->value().c_str(), ":");
ASSERT(parts.size() > 0 && parts.size() <= 2);
request->headers().Host()->value(
parts.size() == 2 ? absl::StrJoin(parts, "-shadow:")
: absl::StrCat(request->headers().Host()->value().c_str(), "-shadow"));
// Configuration should guarantee that cluster exists before calling here. This is basically
// fire and forget. We don't handle cancelling.
cm_.httpAsyncClientForCluster(cluster).send(std::move(request), *this,
Expand Down
16 changes: 11 additions & 5 deletions test/common/router/shadow_writer_impl_test.cc
View file
Open in desktop
Expand Up @@ -16,13 +16,13 @@ using testing::_;
namespace Envoy {
namespace Router {

TEST(ShadowWriterImplTest, All) {
void expectShadowWriter(absl::string_view host, absl::string_view shadowed_host) {
Upstream::MockClusterManager cm;
ShadowWriterImpl writer(cm);

// Success case
Http::MessagePtr message(new Http::RequestMessageImpl());
message->headers().insertHost().value(std::string("cluster1"));
message->headers().insertHost().value(std::string(host));
EXPECT_CALL(cm, httpAsyncClientForCluster("foo")).WillOnce(ReturnRef(cm.async_client_));
Http::MockAsyncClientRequest request(&cm.async_client_);
Http::AsyncClient::Callbacks* callback;
Expand All @@ -32,7 +32,7 @@ TEST(ShadowWriterImplTest, All) {
Invoke([&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks,
const Optional<std::chrono::milliseconds>&) -> Http::AsyncClient::Request* {
EXPECT_EQ(message, inner_message);
EXPECT_STREQ("cluster1-shadow", message->headers().Host()->value().c_str());
EXPECT_EQ(shadowed_host, message->headers().Host()->value().c_str());
callback = &callbacks;
return &request;
}));
Expand All @@ -43,21 +43,27 @@ TEST(ShadowWriterImplTest, All) {

// Failure case
message.reset(new Http::RequestMessageImpl());
message->headers().insertHost().value(std::string("cluster2"));
message->headers().insertHost().value(std::string(host));
EXPECT_CALL(cm, httpAsyncClientForCluster("bar")).WillOnce(ReturnRef(cm.async_client_));
EXPECT_CALL(cm.async_client_,
send_(_, _, Optional<std::chrono::milliseconds>(std::chrono::milliseconds(10))))
.WillOnce(
Invoke([&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks,
const Optional<std::chrono::milliseconds>&) -> Http::AsyncClient::Request* {
EXPECT_EQ(message, inner_message);
EXPECT_STREQ("cluster2-shadow", message->headers().Host()->value().c_str());
EXPECT_EQ(shadowed_host, message->headers().Host()->value().c_str());
callback = &callbacks;
return &request;
}));
writer.shadow("bar", std::move(message), std::chrono::milliseconds(10));
callback->onFailure(Http::AsyncClient::FailureReason::Reset);
}

TEST(ShadowWriterImplTest, All) {
expectShadowWriter("cluster1", "cluster1-shadow");
expectShadowWriter("cluster1:8000", "cluster1-shadow:8000");
expectShadowWriter("cluster1:80", "cluster1-shadow:80");
}

} // namespace Router
} // namespace Envoy