Move EDS Clusters initialization after ADS (#2760)

Fixes #2759 Signed-off-by: Rama <[email protected]>
envoyproxy · Mar 11, 2018 · e6ff690 · e6ff690
1 parent b2610c8
commit e6ff690
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 16 deletions.
diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc
@@ -205,6 +205,21 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
     }
   }
 
+  // Now setup ADS if needed, this might rely on a primary cluster.
+  if (bootstrap.dynamic_resources().has_ads_config()) {
+    ads_mux_.reset(new Config::GrpcMuxImpl(
+        bootstrap.node(),
+        Config::Utility::factoryForApiConfigSource(
+            *async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats)
+            ->create(),
+        primary_dispatcher,
+        *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(
+            "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources")));
+  } else {
+    ads_mux_.reset(new Config::NullGrpcMuxImpl());
+  }
+
+  // After ADS is initialized, load EDS static clusters as EDS config may potentially need ADS.
   for (const auto& cluster : bootstrap.static_resources().clusters()) {
     // Now load all the secondary clusters.
     if (cluster.type() == envoy::api::v2::Cluster::EDS) {
@@ -255,21 +270,6 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
     return std::make_shared<ThreadLocalClusterManagerImpl>(*this, dispatcher, local_cluster_name);
   });
 
-  // Now setup ADS if needed, this might rely on a primary cluster and the
-  // thread local cluster manager.
-  if (bootstrap.dynamic_resources().has_ads_config()) {
-    ads_mux_.reset(new Config::GrpcMuxImpl(
-        bootstrap.node(),
-        Config::Utility::factoryForApiConfigSource(
-            *async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats)
-            ->create(),
-        primary_dispatcher,
-        *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(
-            "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources")));
-  } else {
-    ads_mux_.reset(new Config::NullGrpcMuxImpl());
-  }
-
   // We can now potentially create the CDS API once the backing cluster exists.
   if (bootstrap.dynamic_resources().has_cds_config()) {
     cds_api_ = factory_.createCds(bootstrap.dynamic_resources().cds_config(), eds_config_, *this);
@@ -820,4 +820,4 @@ ProdClusterManagerFactory::createCds(const envoy::api::v2::core::ConfigSource& c
 }
 
 } // namespace Upstream
-} // namespace Envoy
+} // namespace Envoy
diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc
@@ -434,5 +434,58 @@ TEST_P(AdsFailIntegrationTest, ConnectDisconnect) {
   ads_stream_->finishGrpcStream(Grpc::Status::Internal);
 }
 
+class AdsConfigIntegrationTest : public HttpIntegrationTest,
+                                 public Grpc::GrpcClientIntegrationParamTest {
+public:
+  AdsConfigIntegrationTest()
+      : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, ipVersion(), config) {}
+
+  void TearDown() override {
+    test_server_.reset();
+    fake_upstreams_.clear();
+  }
+
+  void createUpstreams() override {
+    HttpIntegrationTest::createUpstreams();
+    fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_));
+  }
+
+  void initialize() override {
+    config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
+      auto* grpc_service =
+          bootstrap.mutable_dynamic_resources()->mutable_ads_config()->add_grpc_services();
+      setGrpcService(*grpc_service, "ads_cluster", fake_upstreams_.back()->localAddress());
+      auto* ads_cluster = bootstrap.mutable_static_resources()->add_clusters();
+      ads_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]);
+      ads_cluster->set_name("ads_cluster");
+
+      // Add EDS static Cluster that uses ADS as config Source.
+      auto* ads_eds_cluster = bootstrap.mutable_static_resources()->add_clusters();
+      ads_eds_cluster->set_name("ads_eds_cluster");
+      ads_eds_cluster->set_type(envoy::api::v2::Cluster::EDS);
+      auto* eds_cluster_config = ads_eds_cluster->mutable_eds_cluster_config();
+      auto* eds_config = eds_cluster_config->mutable_eds_config();
+      eds_config->mutable_ads();
+    });
+    setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
+    HttpIntegrationTest::initialize();
+  }
+
+  FakeHttpConnectionPtr ads_connection_;
+  FakeStreamPtr ads_stream_;
+};
+
+INSTANTIATE_TEST_CASE_P(IpVersionsClientType, AdsConfigIntegrationTest,
+                        GRPC_CLIENT_INTEGRATION_PARAMS);
+
+// This is s regression validating that we don't crash on EDS static Cluster that uses ADS.
+TEST_P(AdsConfigIntegrationTest, EdsClusterWithAdsConfigSource) {
+  initialize();
+  ads_connection_ = fake_upstreams_[1]->waitForHttpConnection(*dispatcher_);
+  ads_stream_ = ads_connection_->waitForNewStream(*dispatcher_);
+  ads_stream_->startGrpcStream();
+  ads_stream_->finishGrpcStream(Grpc::Status::Ok);
+}
+
 } // namespace
 } // namespace Envoy