Skip to content

shadow: fix crash when shadow cluster does not exist #4582

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
ggreenway merged 3 commits into from
Oct 2, 2018
Merged

shadow: fix crash when shadow cluster does not exist #4582

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
80348d8
shadow: fix crash when shadow cluster does not exist
mattklein123 Oct 1, 2018
17c5f7b
Merge remote-tracking branch 'origin/master' into shadow_no_cluster
mattklein123 Oct 2, 2018
e7fe19e
comments
mattklein123 Oct 2, 2018
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
3 changes: 1 addition & 2 deletions include/envoy/upstream/thread_local_cluster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ class ThreadLocalCluster {

/**
* @return ClusterInfoConstSharedPtr the info for this cluster. The info is safe to store beyond
* the
* lifetime of the ThreadLocalCluster instance itself.
* the lifetime of the ThreadLocalCluster instance itself.
*/
virtual ClusterInfoConstSharedPtr info() PURE;

Expand Down
10 changes: 8 additions & 2 deletions source/common/router/shadow_writer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,21 @@ namespace Router {

void ShadowWriterImpl::shadow(const std::string& cluster, Http::MessagePtr&& request,
std::chrono::milliseconds timeout) {
// It's possible that the cluster specified in the route configuration no longer exists due
// to a CDS removal. Check that it still exists before shadowing.
// TODO(mattklein123): Optimally we would have a stat but for now just fix the crashing issue.
if (!cm_.get(cluster)) {
return;

@junr03 junr03 Oct 2, 2018

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 wonder if we should at least leave a warn log behind so people are not at a loss for why they're not seeing shadow traffic.

@mattklein123 mattklein123 Oct 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think a warn log is not a great idea since this is in the data path and could spam quite a bit. Though I had considered at least adding a debug log so I will do that.

@ggreenway ggreenway Oct 2, 2018

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.

As you mention in the comment, I think a stat is most appropriate in this path. But I'm fine with merging this crash-fix as-is. @junr03 you ok with that?

}

ASSERT(!request->headers().Host()->value().empty());
// Switch authority to add a shadow postfix. This allows upstream logging to make 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.
// This is basically fire and forget. We don't handle cancelling.
cm_.httpAsyncClientForCluster(cluster).send(std::move(request), *this,
absl::optional<std::chrono::milliseconds>(timeout));
}
Expand Down
86 changes: 45 additions & 41 deletions test/common/router/shadow_writer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,58 +11,62 @@
#include "gtest/gtest.h"

using testing::_;
using testing::InSequence;
using testing::Invoke;
using testing::Return;

namespace Envoy {
namespace Router {

void expectShadowWriter(absl::string_view host, absl::string_view shadowed_host) {
Upstream::MockClusterManager cm;
ShadowWriterImpl writer(cm);
class ShadowWriterImplTest : public testing::Test {
public:
void expectShadowWriter(absl::string_view host, absl::string_view shadowed_host) {
Http::MessagePtr message(new Http::RequestMessageImpl());
message->headers().insertHost().value(std::string(host));
EXPECT_CALL(cm_, get("foo"));
EXPECT_CALL(cm_, httpAsyncClientForCluster("foo")).WillOnce(ReturnRef(cm_.async_client_));
Http::MockAsyncClientRequest request(&cm_.async_client_);
EXPECT_CALL(
cm_.async_client_,
send_(_, _, absl::optional<std::chrono::milliseconds>(std::chrono::milliseconds(5))))
.WillOnce(Invoke(
[&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks,
const absl::optional<std::chrono::milliseconds>&) -> Http::AsyncClient::Request* {
EXPECT_EQ(message, inner_message);
EXPECT_EQ(shadowed_host, message->headers().Host()->value().c_str());
callback_ = &callbacks;
return &request;
}));
writer_.shadow("foo", std::move(message), std::chrono::milliseconds(5));
}

// Success case
Http::MessagePtr message(new Http::RequestMessageImpl());
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;
EXPECT_CALL(cm.async_client_,
send_(_, _, absl::optional<std::chrono::milliseconds>(std::chrono::milliseconds(5))))
.WillOnce(Invoke(
[&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks,
const absl::optional<std::chrono::milliseconds>&) -> Http::AsyncClient::Request* {
EXPECT_EQ(message, inner_message);
EXPECT_EQ(shadowed_host, message->headers().Host()->value().c_str());
callback = &callbacks;
return &request;
}));
writer.shadow("foo", std::move(message), std::chrono::milliseconds(5));
Upstream::MockClusterManager cm_;
ShadowWriterImpl writer_{cm_};
Http::AsyncClient::Callbacks* callback_{};
};

Http::MessagePtr response(new Http::RequestMessageImpl());
callback->onSuccess(std::move(response));
TEST_F(ShadowWriterImplTest, Success) {
InSequence s;

// Failure case
message.reset(new Http::RequestMessageImpl());
message->headers().insertHost().value(std::string(host));
EXPECT_CALL(cm, httpAsyncClientForCluster("bar")).WillOnce(ReturnRef(cm.async_client_));
EXPECT_CALL(cm.async_client_,
send_(_, _, absl::optional<std::chrono::milliseconds>(std::chrono::milliseconds(10))))
.WillOnce(Invoke(
[&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks,
const absl::optional<std::chrono::milliseconds>&) -> Http::AsyncClient::Request* {
EXPECT_EQ(message, inner_message);
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);
expectShadowWriter("cluster1", "cluster1-shadow");
Http::MessagePtr response(new Http::RequestMessageImpl());
callback_->onSuccess(std::move(response));
}

TEST(ShadowWriterImplTest, All) {
expectShadowWriter("cluster1", "cluster1-shadow");
TEST_F(ShadowWriterImplTest, Failure) {
InSequence s;

expectShadowWriter("cluster1:8000", "cluster1-shadow:8000");
expectShadowWriter("cluster1:80", "cluster1-shadow:80");
callback_->onFailure(Http::AsyncClient::FailureReason::Reset);
}

TEST_F(ShadowWriterImplTest, NoCluster) {
InSequence s;

Http::MessagePtr message(new Http::RequestMessageImpl());
EXPECT_CALL(cm_, get("foo")).WillOnce(Return(nullptr));
EXPECT_CALL(cm_, httpAsyncClientForCluster("foo")).Times(0);
writer_.shadow("foo", std::move(message), std::chrono::milliseconds(5));
}

} // namespace Router
Expand Down