uri_template: verbose errors

Signed-off-by: Adi Suissa-Peleg <[email protected]>
adisuissa · Jun 12, 2024 · 7bf102c · 7bf102c
1 parent b37d9d2
commit 7bf102c
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 14 deletions.
diff --git a/source/extensions/path/match/uri_template/config.h b/source/extensions/path/match/uri_template/config.h
@@ -22,9 +22,11 @@ class UriTemplateMatcherFactory : public Router::PathMatcherFactory {
         const envoy::extensions::path::match::uri_template::v3::UriTemplateMatchConfig&>(
         config, ProtobufMessage::getStrictValidationVisitor());
 
-    if (!UriTemplate::isValidMatchPattern(path_match_config.path_template()).ok()) {
-      return absl::InvalidArgumentError(fmt::format("path_match_policy.path_template {} is invalid",
-                                                    path_match_config.path_template()));
+    const absl::Status valid = UriTemplate::isValidMatchPattern(path_match_config.path_template());
+    if (!valid.ok()) {
+      return absl::InvalidArgumentError(
+          fmt::format("path_match_policy.path_template {} is invalid: {}",
+                      path_match_config.path_template(), valid.message()));
     }
 
     return std::make_shared<UriTemplateMatcher>(path_match_config);

diff --git a/source/extensions/path/uri_template_lib/BUILD b/source/extensions/path/uri_template_lib/BUILD
@@ -34,6 +34,7 @@ envoy_cc_library(
     srcs = ["uri_template_internal.cc"],
     hdrs = ["uri_template_internal.h"],
     deps = [
+        "//source/common/common:fmt_lib",
         "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/flags:flag",
         "@com_google_absl//absl/functional:function_ref",

diff --git a/source/extensions/path/uri_template_lib/uri_template_internal.cc b/source/extensions/path/uri_template_lib/uri_template_internal.cc
@@ -6,6 +6,8 @@
 #include <variant>
 #include <vector>
 
+#include "source/common/common/fmt.h"
+
 #include "absl/container/flat_hash_set.h"
 #include "absl/flags/flag.h"
 #include "absl/functional/function_ref.h"
@@ -143,7 +145,7 @@ absl::StatusOr<ParsedResult<Literal>> parseLiteral(absl::string_view pattern) {
       std::vector<absl::string_view>(absl::StrSplit(pattern, absl::MaxSplits('/', 1)))[0];
   absl::string_view unparsed_pattern = pattern.substr(literal.size());
   if (!isValidLiteral(literal)) {
-    return absl::InvalidArgumentError("Invalid literal");
+    return absl::InvalidArgumentError(fmt::format("Invalid literal: \"{}\"", literal));
   }
   return ParsedResult<Literal>(literal, unparsed_pattern);
 }
@@ -155,24 +157,25 @@ absl::StatusOr<ParsedResult<Operator>> parseOperator(absl::string_view pattern)
   if (absl::StartsWith(pattern, "*")) {
     return ParsedResult<Operator>(Operator::PathGlob, pattern.substr(1));
   }
-  return absl::InvalidArgumentError("Invalid Operator");
+  return absl::InvalidArgumentError(fmt::format("Invalid Operator: \"{}\"", pattern));
 }
 
 absl::StatusOr<ParsedResult<Variable>> parseVariable(absl::string_view pattern) {
   // Locate the variable pattern to parse.
   if (pattern.size() < 2 || (pattern)[0] != '{') {
-    return absl::InvalidArgumentError("Invalid variable");
+    return absl::InvalidArgumentError(fmt::format("Invalid variable: \"{}\"", pattern));
   }
   std::vector<absl::string_view> parts = absl::StrSplit(pattern.substr(1), absl::MaxSplits('}', 1));
   if (parts.size() != 2) {
-    return absl::InvalidArgumentError("Unmatched variable bracket");
+    return absl::InvalidArgumentError(fmt::format("Unmatched variable bracket in \"{}\"", pattern));
   }
   absl::string_view unparsed_pattern = parts[1];
 
   // Parse the actual variable pattern, starting with the variable name.
   std::vector<absl::string_view> variable_parts = absl::StrSplit(parts[0], absl::MaxSplits('=', 1));
   if (!isValidVariableName(variable_parts[0])) {
-    return absl::InvalidArgumentError("Invalid variable name");
+    return absl::InvalidArgumentError(
+        fmt::format("Invalid variable name: \"{}\"", variable_parts[0]));
   }
   Variable var = Variable(variable_parts[0], {});
 
@@ -204,7 +207,8 @@ absl::StatusOr<ParsedResult<Variable>> parseVariable(absl::string_view pattern)
     var.match_.push_back(match);
     if (!pattern_item.empty()) {
       if (pattern_item[0] != '/' || pattern_item.size() == 1) {
-        return absl::InvalidArgumentError("Invalid variable match");
+        return absl::InvalidArgumentError(
+            fmt::format("Invalid variable match: \"{}\"", pattern_item));
       }
       pattern_item = pattern_item.substr(1);
     }
@@ -222,16 +226,21 @@ gatherCaptureNames(const struct ParsedPathPattern& pattern) {
       continue;
     }
     if (captured_variables.size() >= kPatternMatchingMaxVariablesPerPath) {
-      return absl::InvalidArgumentError("Exceeded variable count limit");
+      return absl::InvalidArgumentError(fmt::format("Exceeded variable count limit ({} > {})",
+                                                    captured_variables.size(),
+                                                    kPatternMatchingMaxVariablesPerPath));
     }
     absl::string_view name = absl::get<Variable>(segment).name_;
 
     if (name.size() < kPatternMatchingMinVariableNameLen ||
         name.size() > kPatternMatchingMaxVariableNameLen) {
-      return absl::InvalidArgumentError("Invalid variable name length");
+      return absl::InvalidArgumentError(fmt::format(
+          "Invalid variable name length (length of \"{}\" should be at least {} and no more than "
+          "{})",
+          name, kPatternMatchingMinVariableNameLen, kPatternMatchingMaxVariableNameLen));
     }
     if (captured_variables.contains(name)) {
-      return absl::InvalidArgumentError("Repeated variable name");
+      return absl::InvalidArgumentError(fmt::format("Repeated variable name: \"{}\"", name));
     }
     captured_variables.emplace(name);
   }
@@ -275,7 +284,7 @@ absl::StatusOr<ParsedPathPattern> parsePathPatternSyntax(absl::string_view path)
 
   static const LazyRE2 printable_regex = {"^/[[:graph:]]*$"};
   if (!RE2::FullMatch(path, *printable_regex)) {
-    return absl::InvalidArgumentError("Invalid pattern");
+    return absl::InvalidArgumentError(fmt::format("Invalid pattern: \"{}\"", path));
   }
 
   // Parse the leading '/'

diff --git a/test/extensions/path/match/uri_template/config_test.cc b/test/extensions/path/match/uri_template/config_test.cc
@@ -65,7 +65,41 @@ TEST(ConfigTest, InvalidConfigSetup) {
 
   EXPECT_FALSE(config_or_error.ok());
   EXPECT_EQ(config_or_error.status().message(),
-            "path_match_policy.path_template /bar/{lang}/{country is invalid");
+            "path_match_policy.path_template /bar/{lang}/{country is invalid: Unmatched variable "
+            "bracket in \"{country\"");
+}
+
+// Followup on issue https://github.com/envoyproxy/envoy/issues/34507 -
+// providing more details on errors.
+TEST(ConfigTest, InvalidConfigSetupMoreInfo) {
+  const std::string yaml_string = R"EOF(
+      name: envoy.path.match.uri_template.uri_template_matcher
+      typed_config:
+        "@type": type.googleapis.com/envoy.extensions.path.match.uri_template.v3.UriTemplateMatchConfig
+        path_template: "/api/MyFunction('{id}')"
+)EOF";
+
+  envoy::config::core::v3::TypedExtensionConfig config;
+  TestUtility::loadFromYaml(yaml_string, config);
+
+  const auto& factory =
+      &Envoy::Config::Utility::getAndCheckFactory<Router::PathMatcherFactory>(config);
+
+  EXPECT_NE(nullptr, factory);
+  EXPECT_EQ(factory->name(), "envoy.path.match.uri_template.uri_template_matcher");
+
+  auto message = Envoy::Config::Utility::translateAnyToFactoryConfig(
+      config.typed_config(), ProtobufMessage::getStrictValidationVisitor(), *factory);
+
+  EXPECT_NE(nullptr, message);
+
+  absl::StatusOr<Router::PathMatcherSharedPtr> config_or_error =
+      factory->createPathMatcher(*message);
+
+  EXPECT_FALSE(config_or_error.ok());
+  EXPECT_EQ(config_or_error.status().message(),
+            "path_match_policy.path_template /api/MyFunction('{id}') is invalid: Invalid literal: "
+            "\"MyFunction('{id}')\"");
 }
 
 TEST(ConfigTest, TestConfigSetup) {