Make cdn_loop exception free (envoyproxy#38107)

Commit Message: Make cdn_loop exception free Additional Description: Risk Level: low Testing: unit tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Takeshi Yoneda <[email protected]>
antoniovleonti · Jan 21, 2025 · 9dd80f2 · 9dd80f2
1 parent 2b41333
commit 9dd80f2
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 14 deletions.
diff --git a/source/extensions/filters/http/cdn_loop/config.cc b/source/extensions/filters/http/cdn_loop/config.cc
@@ -21,13 +21,14 @@ using ::Envoy::Extensions::HttpFilters::CdnLoop::Parser::parseCdnId;
 using ::Envoy::Extensions::HttpFilters::CdnLoop::Parser::ParseContext;
 using ::Envoy::Extensions::HttpFilters::CdnLoop::Parser::ParsedCdnId;
 
-Http::FilterFactoryCb CdnLoopFilterFactory::createFilterFactoryFromProtoTyped(
+absl::StatusOr<Http::FilterFactoryCb> CdnLoopFilterFactory::createFilterFactoryFromProtoTyped(
     const envoy::extensions::filters::http::cdn_loop::v3::CdnLoopConfig& config,
     const std::string& /*stats_prefix*/, Server::Configuration::FactoryContext& /*context*/) {
   StatusOr<ParsedCdnId> context = parseCdnId(ParseContext(config.cdn_id()));
   if (!context.ok() || !context->context().atEnd()) {
-    throw EnvoyException(fmt::format("Provided cdn_id \"{}\" is not a valid CDN identifier: {}",
-                                     config.cdn_id(), context.status()));
+    return absl::InvalidArgumentError(
+        fmt::format("Provided cdn_id \"{}\" is not a valid CDN identifier: {}", config.cdn_id(),
+                    context.status().message()));
   }
   return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
     callbacks.addStreamDecoderFilter(

diff --git a/source/extensions/filters/http/cdn_loop/config.h b/source/extensions/filters/http/cdn_loop/config.h
@@ -14,13 +14,13 @@ namespace Extensions {
 namespace HttpFilters {
 namespace CdnLoop {
 
-class CdnLoopFilterFactory
-    : public Common::FactoryBase<envoy::extensions::filters::http::cdn_loop::v3::CdnLoopConfig> {
+class CdnLoopFilterFactory : public Common::ExceptionFreeFactoryBase<
+                                 envoy::extensions::filters::http::cdn_loop::v3::CdnLoopConfig> {
 public:
-  CdnLoopFilterFactory() : FactoryBase("envoy.filters.http.cdn_loop") {}
+  CdnLoopFilterFactory() : ExceptionFreeFactoryBase("envoy.filters.http.cdn_loop") {}
 
 private:
-  Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
+  absl::StatusOr<Http::FilterFactoryCb> createFilterFactoryFromProtoTyped(
       const envoy::extensions::filters::http::cdn_loop::v3::CdnLoopConfig& config,
       const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;
 };

diff --git a/test/extensions/filters/http/cdn_loop/config_test.cc b/test/extensions/filters/http/cdn_loop/config_test.cc
@@ -50,8 +50,9 @@ TEST(CdnLoopFilterFactoryTest, InvalidCdnId) {
   config.set_cdn_id("[not-token-or-ip");
   CdnLoopFilterFactory factory;
 
-  EXPECT_THAT_THROWS_MESSAGE(factory.createFilterFactoryFromProto(config, "stats", context).value(),
-                             EnvoyException, HasSubstr("is not a valid CDN identifier"));
+  auto status_or = factory.createFilterFactoryFromProto(config, "stats", context);
+  EXPECT_FALSE(status_or.ok());
+  EXPECT_THAT(status_or.status().message(), HasSubstr("is not a valid CDN identifier"));
 }
 
 TEST(CdnLoopFilterFactoryTest, InvalidCdnIdNonHeaderWhitespace) {
@@ -61,8 +62,9 @@ TEST(CdnLoopFilterFactoryTest, InvalidCdnIdNonHeaderWhitespace) {
   config.set_cdn_id("\r\n");
   CdnLoopFilterFactory factory;
 
-  EXPECT_THAT_THROWS_MESSAGE(factory.createFilterFactoryFromProto(config, "stats", context).value(),
-                             EnvoyException, HasSubstr("is not a valid CDN identifier"));
+  auto status_or = factory.createFilterFactoryFromProto(config, "stats", context);
+  EXPECT_FALSE(status_or.ok());
+  EXPECT_THAT(status_or.status().message(), HasSubstr("is not a valid CDN identifier"));
 }
 
 TEST(CdnLoopFilterFactoryTest, InvalidParsedCdnIdNotInput) {
@@ -72,8 +74,9 @@ TEST(CdnLoopFilterFactoryTest, InvalidParsedCdnIdNotInput) {
   config.set_cdn_id("cdn,cdn");
   CdnLoopFilterFactory factory;
 
-  EXPECT_THAT_THROWS_MESSAGE(factory.createFilterFactoryFromProto(config, "stats", context).value(),
-                             EnvoyException, HasSubstr("is not a valid CDN identifier"));
+  auto status_or = factory.createFilterFactoryFromProto(config, "stats", context);
+  EXPECT_FALSE(status_or.ok());
+  EXPECT_THAT(status_or.status().message(), HasSubstr("is not a valid CDN identifier"));
 }
 
 } // namespace CdnLoop

diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml
@@ -170,7 +170,6 @@ paths:
     - source/extensions/filters/http/aws_lambda
     - source/extensions/filters/http/basic_auth
     - source/extensions/filters/http/cache
-    - source/extensions/filters/http/cdn_loop
     - source/extensions/filters/http/common
     - source/extensions/filters/http/composite
     - source/extensions/filters/http/ext_authz