diff --git a/source/extensions/path/match/uri_template/config.h b/source/extensions/path/match/uri_template/config.h index 1097531e6fe1..3c78455f4bb6 100644 --- 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(path_match_config); diff --git a/source/extensions/path/uri_template_lib/BUILD b/source/extensions/path/uri_template_lib/BUILD index dfea1c17ec2c..a7c407c72859 100644 --- 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 index 7cec7fed99c0..fd57184ba941 100644 --- 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 #include +#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> parseLiteral(absl::string_view pattern) { std::vector(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, unparsed_pattern); } @@ -155,24 +157,25 @@ absl::StatusOr> parseOperator(absl::string_view pattern) if (absl::StartsWith(pattern, "*")) { return ParsedResult(Operator::PathGlob, pattern.substr(1)); } - return absl::InvalidArgumentError("Invalid Operator"); + return absl::InvalidArgumentError(fmt::format("Invalid Operator: \"{}\"", pattern)); } absl::StatusOr> 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 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 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> 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(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 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 index 67a78aa8fd4b..c99108c566ae 100644 --- 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(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 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) {