diff --git a/include/envoy/upstream/thread_local_cluster.h b/include/envoy/upstream/thread_local_cluster.h index 009ef64821aaa..9e37b95a02c21 100644 --- a/include/envoy/upstream/thread_local_cluster.h +++ b/include/envoy/upstream/thread_local_cluster.h @@ -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; diff --git a/source/common/router/shadow_writer_impl.cc b/source/common/router/shadow_writer_impl.cc index b54266ea6abaa..7129d88a1f248 100644 --- a/source/common/router/shadow_writer_impl.cc +++ b/source/common/router/shadow_writer_impl.cc @@ -13,6 +13,14 @@ 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)) { + ENVOY_LOG(debug, "shadow cluster '{}' does not exist", cluster); + return; + } + 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(), ":"); @@ -20,8 +28,7 @@ void ShadowWriterImpl::shadow(const std::string& cluster, Http::MessagePtr&& req 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(timeout)); } diff --git a/source/common/router/shadow_writer_impl.h b/source/common/router/shadow_writer_impl.h index 3724795b5b3e9..72da4bc0a7c09 100644 --- a/source/common/router/shadow_writer_impl.h +++ b/source/common/router/shadow_writer_impl.h @@ -13,7 +13,9 @@ namespace Router { * Implementation of ShadowWriter that takes incoming requests to shadow and implements "fire and * forget" behavior using an async client. */ -class ShadowWriterImpl : public ShadowWriter, public Http::AsyncClient::Callbacks { +class ShadowWriterImpl : Logger::Loggable, + public ShadowWriter, + public Http::AsyncClient::Callbacks { public: ShadowWriterImpl(Upstream::ClusterManager& cm) : cm_(cm) {} diff --git a/test/common/router/shadow_writer_impl_test.cc b/test/common/router/shadow_writer_impl_test.cc index 53f979e8826db..a3683bc3e3061 100644 --- a/test/common/router/shadow_writer_impl_test.cc +++ b/test/common/router/shadow_writer_impl_test.cc @@ -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(5)))) + .WillOnce(Invoke( + [&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks, + const absl::optional&) -> 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(5)))) - .WillOnce(Invoke( - [&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks, - const absl::optional&) -> 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(10)))) - .WillOnce(Invoke( - [&](Http::MessagePtr& inner_message, Http::AsyncClient::Callbacks& callbacks, - const absl::optional&) -> 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