shadow: fix crash when shadow cluster does not exist

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;
 

source/common/router/shadow_writer_impl.cc

@@ -13,15 +13,22 @@ 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(), ":");
   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));
 }

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<Logger::Id::router>,
+                         public ShadowWriter,
+                         public Http::AsyncClient::Callbacks {
 public:
   ShadowWriterImpl(Upstream::ClusterManager& cm) : cm_(cm) {}
 

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>(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