http: moving exceptions out of http_server_properties_cache_manager_i…

…mpl (#27829) Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
envoyproxy · Jun 7, 2023 · 548d477 · 548d477
1 parent 009d739
commit 548d477
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 22 deletions.
diff --git a/source/common/http/http_server_properties_cache_manager_impl.cc b/source/common/http/http_server_properties_cache_manager_impl.cc
@@ -24,16 +24,10 @@ HttpServerPropertiesCacheManagerImpl::HttpServerPropertiesCacheManagerImpl(
 HttpServerPropertiesCacheSharedPtr HttpServerPropertiesCacheManagerImpl::getCache(
     const envoy::config::core::v3::AlternateProtocolsCacheOptions& options,
     Event::Dispatcher& dispatcher) {
-  if (options.has_key_value_store_config() && data_.concurrency_ != 1) {
-    throw EnvoyException(
-        fmt::format("options has key value store but Envoy has concurrency = {} : {}",
-                    data_.concurrency_, options.DebugString()));
-  }
-
   const auto& existing_cache = (*slot_).caches_.find(options.name());
   if (existing_cache != (*slot_).caches_.end()) {
     if (!Protobuf::util::MessageDifferencer::Equivalent(options, existing_cache->second.options_)) {
-      throw EnvoyException(fmt::format(
+      IS_ENVOY_BUG(fmt::format(
           "options specified alternate protocols cache '{}' with different settings"
           " first '{}' second '{}'",
           options.name(), existing_cache->second.options_.DebugString(), options.DebugString()));

diff --git a/source/extensions/filters/http/alternate_protocols_cache/filter.cc b/source/extensions/filters/http/alternate_protocols_cache/filter.cc
@@ -38,6 +38,7 @@ Filter::Filter(const FilterConfigSharedPtr& config, Event::Dispatcher& dispatche
     : cache_(config->getAlternateProtocolCache(dispatcher)), time_source_(config->timeSource()) {}
 
 Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers, bool) {
+
   if (!cache_) {
     return Http::FilterHeadersStatus::Continue;
   }

diff --git a/source/extensions/upstreams/http/config.cc b/source/extensions/upstreams/http/config.cc
@@ -84,13 +84,21 @@ bool useHttp3(const envoy::extensions::upstreams::http::v3::HttpProtocolOptions&
 
 absl::optional<const envoy::config::core::v3::AlternateProtocolsCacheOptions>
 getAlternateProtocolsCacheOptions(
-    const envoy::extensions::upstreams::http::v3::HttpProtocolOptions& options) {
+    const envoy::extensions::upstreams::http::v3::HttpProtocolOptions& options,
+    Server::Configuration::ServerFactoryContext& server_context) {
   if (options.has_auto_config() && options.auto_config().has_http3_protocol_options()) {
     if (!options.auto_config().has_alternate_protocols_cache_options()) {
       throw EnvoyException(fmt::format("alternate protocols cache must be configured when HTTP/3 "
                                        "is enabled with auto_config"));
     }
-    return options.auto_config().alternate_protocols_cache_options();
+    auto cache_options = options.auto_config().alternate_protocols_cache_options();
+    if (cache_options.has_key_value_store_config() && server_context.options().concurrency() != 1) {
+      throw EnvoyException(
+          fmt::format("options has key value store but Envoy has concurrency = {} : {}",
+                      server_context.options().concurrency(), cache_options.DebugString()));
+    }
+
+    return cache_options;
   }
   return absl::nullopt;
 }
@@ -178,7 +186,7 @@ ProtocolOptionsConfigImpl::ProtocolOptionsConfigImpl(
                     options.upstream_http_protocol_options())
               : absl::nullopt),
       http_filters_(options.http_filters()),
-      alternate_protocol_cache_options_(getAlternateProtocolsCacheOptions(options)),
+      alternate_protocol_cache_options_(getAlternateProtocolsCacheOptions(options, server_context)),
       header_validator_factory_(createHeaderValidatorFactory(options, server_context)),
       use_downstream_protocol_(options.has_use_downstream_protocol_config()),
       use_http2_(useHttp2(options)), use_http3_(useHttp3(options)),

diff --git a/test/common/http/http_server_properties_cache_manager_test.cc b/test/common/http/http_server_properties_cache_manager_test.cc
@@ -100,14 +100,6 @@ TEST_F(HttpServerPropertiesCacheManagerTest, GetCacheWithCanonicalEntry) {
   EXPECT_TRUE(cache->findAlternatives(origin).has_value());
 }
 
-TEST_F(HttpServerPropertiesCacheManagerTest, GetCacheWithFlushingAndConcurrency) {
-  EXPECT_CALL(context_.options_, concurrency()).WillOnce(Return(5));
-  options1_.mutable_key_value_store_config();
-  initialize();
-  EXPECT_THROW_WITH_REGEX(manager_->getCache(options1_, dispatcher_), EnvoyException,
-                          "options has key value store but Envoy has concurrency = 5");
-}
-
 TEST_F(HttpServerPropertiesCacheManagerTest, GetCacheForDifferentOptions) {
   initialize();
   HttpServerPropertiesCacheSharedPtr cache1 = manager_->getCache(options1_, dispatcher_);
@@ -120,9 +112,9 @@ TEST_F(HttpServerPropertiesCacheManagerTest, GetCacheForConflictingOptions) {
   initialize();
   HttpServerPropertiesCacheSharedPtr cache1 = manager_->getCache(options1_, dispatcher_);
   options2_.set_name(options1_.name());
-  EXPECT_THROW_WITH_REGEX(
-      manager_->getCache(options2_, dispatcher_), EnvoyException,
-      "options specified alternate protocols cache 'name1' with different settings.*");
+  EXPECT_ENVOY_BUG(manager_->getCache(options2_, dispatcher_),
+                   "options specified alternate protocols cache 'name1' with different settings "
+                   "first 'name: \"name1\"");
 }
 
 } // namespace

diff --git a/test/extensions/upstreams/http/config_test.cc b/test/extensions/upstreams/http/config_test.cc
@@ -84,6 +84,18 @@ TEST_F(ConfigTest, AutoHttp3NoCache) {
       "alternate protocols cache must be configured when HTTP/3 is enabled with auto_config");
 }
 
+TEST_F(ConfigTest, KvStoreConcurrencyFail) {
+  options_.mutable_auto_config();
+  options_.mutable_auto_config()->mutable_http3_protocol_options();
+  options_.mutable_auto_config()
+      ->mutable_alternate_protocols_cache_options()
+      ->mutable_key_value_store_config();
+  server_context_.options_.concurrency_ = 2;
+  EXPECT_THROW_WITH_MESSAGE(
+      ProtocolOptionsConfigImpl config(options_, server_context_), EnvoyException,
+      "options has key value store but Envoy has concurrency = 2 : key_value_store_config {\n}\n");
+}
+
 namespace {
 
 class TestHeaderValidatorFactoryConfig : public ::Envoy::Http::HeaderValidatorFactoryConfig {

diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml
@@ -130,7 +130,6 @@ paths:
     - source/common/http/filter_chain_helper.h
     - source/common/http/conn_manager_utility.cc
     - source/common/http/match_delegate/config.cc
-    - source/common/http/http_server_properties_cache_manager_impl.cc
     - source/common/protobuf/yaml_utility.cc
     - source/common/protobuf/visitor_helper.h
     - source/common/protobuf/visitor.cc