upstream: clean up feature parsing code (#14629)

Fixing a perfectly safe and fairly terrible version merge in the ALPN pr the "refactor all upstream config" PRs. the original code created the new options for new config, and parseFeatures handled parsing features from either the new options, or the old config. I decided that was too complicated, changed the code to always create the new options struct and forgot to clean up parseFeatures to assume the presence of the new options struct and remove handling things the old style way. Risk Level: low (clean up inaccessible code) Testing: added one extra unit test just because Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Jan 12, 2021 · 0e1b388 · 0e1b388
1 parent a836c8e
commit 0e1b388
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 23 deletions.
diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc
@@ -728,7 +728,7 @@ ClusterInfoImpl::ClusterInfoImpl(
                                         config, *stats_scope_, factory_context.clusterManager())
                                   : nullptr),
       features_(ClusterInfoImpl::HttpProtocolOptionsConfigImpl::parseFeatures(
-          config, http_protocol_options_)),
+          config, *http_protocol_options_)),
       resource_managers_(config, runtime, name_, *stats_scope_,
                          factory_context.clusterManager().clusterCircuitBreakersStatNames()),
       maintenance_mode_runtime_key_(absl::StrCat("upstream.maintenance_mode.", name_)),

diff --git a/source/extensions/upstreams/http/config.cc b/source/extensions/upstreams/http/config.cc
@@ -43,33 +43,23 @@ getHttp2Options(const envoy::extensions::upstreams::http::v3::HttpProtocolOption
 
 } // namespace
 
-uint64_t
-ProtocolOptionsConfigImpl::parseFeatures(const envoy::config::cluster::v3::Cluster& config,
-                                         std::shared_ptr<const ProtocolOptionsConfigImpl> options) {
+uint64_t ProtocolOptionsConfigImpl::parseFeatures(const envoy::config::cluster::v3::Cluster& config,
+                                                  const ProtocolOptionsConfigImpl& options) {
   uint64_t features = 0;
 
-  if (options) {
-    if (options->use_http2_) {
-      features |= Upstream::ClusterInfo::Features::HTTP2;
-    }
-    if (options->use_downstream_protocol_) {
-      features |= Upstream::ClusterInfo::Features::USE_DOWNSTREAM_PROTOCOL;
-    }
-  } else {
-    if (config.has_http2_protocol_options()) {
-      features |= Upstream::ClusterInfo::Features::HTTP2;
-    }
-    if (config.protocol_selection() ==
-        envoy::config::cluster::v3::Cluster::USE_DOWNSTREAM_PROTOCOL) {
-      features |= Upstream::ClusterInfo::Features::USE_DOWNSTREAM_PROTOCOL;
-    }
+  if (options.use_http2_) {
+    features |= Upstream::ClusterInfo::Features::HTTP2;
   }
-  if (config.close_connections_on_host_health_failure()) {
-    features |= Upstream::ClusterInfo::Features::CLOSE_CONNECTIONS_ON_HOST_HEALTH_FAILURE;
+  if (options.use_downstream_protocol_) {
+    features |= Upstream::ClusterInfo::Features::USE_DOWNSTREAM_PROTOCOL;
   }
-  if (options->use_alpn_) {
+  if (options.use_alpn_) {
     features |= Upstream::ClusterInfo::Features::USE_ALPN;
   }
+
+  if (config.close_connections_on_host_health_failure()) {
+    features |= Upstream::ClusterInfo::Features::CLOSE_CONNECTIONS_ON_HOST_HEALTH_FAILURE;
+  }
   return features;
 }
 

diff --git a/source/extensions/upstreams/http/config.h b/source/extensions/upstreams/http/config.h
@@ -37,7 +37,7 @@ class ProtocolOptionsConfigImpl : public Upstream::ProtocolOptionsConfig {
   // Given the supplied cluster config, and protocol options configuration,
   // returns a unit64_t representing the enabled Upstream::ClusterInfo::Features.
   static uint64_t parseFeatures(const envoy::config::cluster::v3::Cluster& config,
-                                std::shared_ptr<const ProtocolOptionsConfigImpl> options);
+                                const ProtocolOptionsConfigImpl& options);
 
   const Envoy::Http::Http1Settings http1_settings_;
   const envoy::config::core::v3::Http2ProtocolOptions http2_options_;

diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc
@@ -237,6 +237,10 @@ TEST_F(ClusterManagerImplTest, MultipleProtocolCluster) {
       protocol_selection: USE_DOWNSTREAM_PROTOCOL
   )EOF";
   create(parseBootstrapFromV3Yaml(yaml));
+  auto info =
+      cluster_manager_->clusters().active_clusters_.find("http12_cluster")->second.get().info();
+  EXPECT_NE(0, info->features() & Upstream::ClusterInfo::Features::USE_DOWNSTREAM_PROTOCOL);
+
   checkConfigDump(R"EOF(
 static_clusters:
   - cluster: