Make route prefix map matchers able to retry (#38986)

ravenblackx · web-flow · commit e17a126c3e72 · 2025-05-29T11:25:05.000-07:00
Commit Message: Make route prefix map matchers able to retry
Additional Description: The concept of this change is that the current
behavior is quite surprising; I believe what happens after this change
is what a user would typically expect the prefix matcher behavior to be,
but unfortunately changing it in-place would be a behavior change now
that the existing behavior is established, so this needs a runtime
guard.

The difference with this version of the prefix match map is that the
`on_no_match` field still runs if there was a subtree matcher that
didn't match, whereas the old one requires each subtree to have its own
`on_no_match`, resulting in a huge amount of duplicate config code for
our use-case.

Also, before falling back to the on_no_match, this variant tries any
also-matching shorter prefixes, e.g. a prefix match map with retries
like
```
"/banana": subtree_a,
"/ban": subtree_b
"/": subtree_c
on_no_match
```

With a request path `/banana/is/a/fruit` would first try subtree_a, and
if conditions in there don't return a match, would try subtree_b, then
again to subtree_c, then finally the on_no_match action.

Risk Level: A little; runtime guard allows reverting.
Testing: Increased testing for existing config to ensure current [maybe
unwanted] behavior, added tests for new behavior.
Docs Changes: relnotes only.
Release Notes: Yes.
Platform Specific Features: n/a

---------

Signed-off-by: Raven Black &lt;ravenblack@dropbox.com&gt;
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -7,6 +7,15 @@ behavior_changes:
     As announced in November 2024 (see https://github.com/envoyproxy/envoy/issues/37621), the
     grpc_credentials/aws_iam extension is being deleted. Any configuration referencing this extension
     will fail to load.
+- area: prefix_match_map
+  change: |
+    :ref:`prefix_match_map <envoy_v3_api_field_config.common.matcher.v3.Matcher.MatcherTree.prefix_match_map>`
+    now continues to search for a match with shorter prefix if a longer match
+    does not find an action. This brings it in line with the behavior of ``matcher_list``.
+    This change can temporarily be reverted by setting the runtime guard
+    ``envoy.reloadable_features.prefix_map_matcher_resume_after_subtree_miss`` to false.
+    If the old behavior is desired more permanently, this can be achieved in config by setting
+    an ``on_no_match`` action that responds with 404 for each subtree.
 
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*
diff --git a/source/common/matcher/BUILD b/source/common/matcher/BUILD
@@ -30,6 +30,7 @@ envoy_cc_library(
     deps = [
         ":map_matcher_lib",
         "//source/common/common:trie_lookup_table_lib",
+        "//source/common/runtime:runtime_features_lib",
     ],
 )
 
diff --git a/source/common/matcher/exact_map_matcher.h b/source/common/matcher/exact_map_matcher.h
@@ -32,13 +32,14 @@ template <class DataType> class ExactMapMatcher : public MapMatcher<DataType> {
                   absl::optional<OnMatch<DataType>> on_no_match, absl::Status& creation_status)
       : MapMatcher<DataType>(std::move(data_input), std::move(on_no_match), creation_status) {}
 
-  absl::optional<OnMatch<DataType>> doMatch(const std::string& data) override {
-    const auto itr = children_.find(data);
+  typename MatchTree<DataType>::MatchResult doMatch(const DataType&, absl::string_view key,
+                                                    SkippedMatchCb<DataType>) override {
+    const auto itr = children_.find(key);
     if (itr != children_.end()) {
-      return itr->second;
+      return {MatchState::MatchComplete, itr->second};
     }
 
-    return absl::nullopt;
+    return {MatchState::MatchComplete, absl::nullopt};
   }
 
 private:
diff --git a/source/common/matcher/map_matcher.h b/source/common/matcher/map_matcher.h
@@ -32,9 +32,13 @@ class MapMatcher : public MatchTree<DataType>, Logger::Loggable<Logger::Id::matc
       return MatchTree<DataType>::handleRecursionAndSkips(on_no_match_, data, skipped_match_cb);
     }
 
-    const absl::optional<OnMatch<DataType>> result = doMatch(absl::get<std::string>(input.data_));
+    const typename MatchTree<DataType>::MatchResult result =
+        doMatch(data, absl::get<std::string>(input.data_), skipped_match_cb);
+    if (result.match_state_ != MatchState::MatchComplete) {
+      return result;
+    }
     // No match.
-    if (!result.has_value()) {
+    if (!result.on_match_.has_value()) {
       // Match failed.
       if (input.data_availability_ ==
           DataInputGetResult::DataAvailability::MoreDataMightBeAvailable) {
@@ -46,7 +50,7 @@ class MapMatcher : public MatchTree<DataType>, Logger::Loggable<Logger::Id::matc
 
     // Handle recursion and keep_matching.
     auto processed_result =
-        MatchTree<DataType>::handleRecursionAndSkips(result, data, skipped_match_cb);
+        MatchTree<DataType>::handleRecursionAndSkips(result.on_match_, data, skipped_match_cb);
     // Matched or failed nested matching.
     if (processed_result.match_state_ != MatchState::MatchComplete ||
         processed_result.on_match_.has_value()) {
@@ -74,7 +78,9 @@ class MapMatcher : public MatchTree<DataType>, Logger::Loggable<Logger::Id::matc
   // The inner match method. Attempts to match against the resulting data string. If the match
   // result was determined, the OnMatch will be returned. If a match result was determined to be no
   // match, {} will be returned.
-  virtual absl::optional<OnMatch<DataType>> doMatch(const std::string& data) PURE;
+  virtual typename MatchTree<DataType>::MatchResult
+  doMatch(const DataType& data, absl::string_view key,
+          SkippedMatchCb<DataType> skipped_match_cb) PURE;
 };
 
 } // namespace Matcher
diff --git a/source/common/matcher/prefix_map_matcher.h b/source/common/matcher/prefix_map_matcher.h
@@ -2,6 +2,7 @@
 
 #include "source/common/common/trie_lookup_table.h"
 #include "source/common/matcher/map_matcher.h"
+#include "source/common/runtime/runtime_features.h"
 
 namespace Envoy {
 namespace Matcher {
@@ -30,15 +31,26 @@ template <class DataType> class PrefixMapMatcher : public MapMatcher<DataType> {
                    absl::optional<OnMatch<DataType>> on_no_match, absl::Status& creation_status)
       : MapMatcher<DataType>(std::move(data_input), std::move(on_no_match), creation_status) {}
 
-  // TODO(ravenblackx): Implement a Reentrant. Re-entry gives access to the
-  // shorter, matching prefixes for callers.
-  absl::optional<OnMatch<DataType>> doMatch(const std::string& data) override {
-    const auto result = children_.findLongestPrefix(data);
-    if (result) {
-      return *result;
+  typename MatchTree<DataType>::MatchResult
+  doMatch(const DataType& data, absl::string_view key,
+          SkippedMatchCb<DataType> skipped_match_cb) override {
+    const absl::InlinedVector<std::shared_ptr<OnMatch<DataType>>, 4> results =
+        children_.findMatchingPrefixes(key);
+    bool retry_shorter = Runtime::runtimeFeatureEnabled(
+        "envoy.reloadable_features.prefix_map_matcher_resume_after_subtree_miss");
+    for (auto it = results.rbegin(); it != results.rend(); ++it) {
+      const std::shared_ptr<OnMatch<DataType>>& on_match = *it;
+      typename MatchTree<DataType>::MatchResult result =
+          MatchTree<DataType>::handleRecursionAndSkips(*on_match, data, skipped_match_cb);
+      if (result.match_state_ != MatchState::MatchComplete || result.on_match_.has_value() ||
+          !retry_shorter) {
+        // If the match failed to complete, or if it matched, or
+        // if we're doing the legacy "don't try additional matchers"
+        // behavior, return whatever the first match's result was.
+        return result;
+      }
     }
-
-    return absl::nullopt;
+    return {MatchState::MatchComplete, absl::nullopt};
   }
 
 private:
diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -70,6 +70,7 @@ RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout);
 RUNTIME_GUARD(envoy_reloadable_features_original_src_fix_port_exhaustion);
 RUNTIME_GUARD(envoy_reloadable_features_prefer_ipv6_dns_on_macos);
 RUNTIME_GUARD(envoy_reloadable_features_prefer_quic_client_udp_gro);
+RUNTIME_GUARD(envoy_reloadable_features_prefix_map_matcher_resume_after_subtree_miss);
 RUNTIME_GUARD(envoy_reloadable_features_proxy_104);
 RUNTIME_GUARD(envoy_reloadable_features_proxy_ssl_port);
 RUNTIME_GUARD(envoy_reloadable_features_proxy_status_mapping_more_core_response_flags);
diff --git a/test/common/matcher/BUILD b/test/common/matcher/BUILD
@@ -87,6 +87,7 @@ envoy_cc_test(
         "//test/mocks/server:factory_context_mocks",
         "//test/mocks/stream_info:stream_info_mocks",
         "//test/test_common:registry_lib",
+        "//test/test_common:test_runtime_lib",
         "@com_github_cncf_xds//xds/type/matcher/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/common/matcher/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
diff --git a/test/common/matcher/matcher_test.cc b/test/common/matcher/matcher_test.cc
@@ -14,6 +14,7 @@
 #include "test/mocks/matcher/mocks.h"
 #include "test/mocks/server/factory_context.h"
 #include "test/test_common/registry.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "gtest/gtest.h"
@@ -133,6 +134,213 @@ TEST_F(MatcherTest, TestPrefixMatcher) {
   EXPECT_THAT(result, HasResult(IsStringAction("expected!")));
 }
 
+TEST_F(MatcherTest, TestPrefixMatcherWithRetryInnerMissPerformsOuterOnNoMatch) {
+  const std::string yaml = R"EOF(
+on_no_match:
+  action:
+    name: test_action
+    typed_config:
+      "@type": type.googleapis.com/google.protobuf.StringValue
+      value: expected!
+matcher_tree:
+  input:
+    name: outer_input
+    typed_config:
+      "@type": type.googleapis.com/google.protobuf.StringValue
+  prefix_match_map:
+    map:
+      "":
+        matcher:
+          matcher_list:
+            matchers:
+            - predicate:
+                single_predicate:
+                  input:
+                    name: inner_input
+                    typed_config:
+                      "@type": type.googleapis.com/google.protobuf.BoolValue
+                  value_match:
+                    exact: foo
+              on_match:
+                action:
+                  name: test_action
+                  typed_config:
+                    "@type": type.googleapis.com/google.protobuf.StringValue
+                    value: unexpected
+      val:
+        matcher:
+          matcher_list:
+            matchers:
+            - predicate:
+                single_predicate:
+                  input:
+                    name: inner_input
+                    typed_config:
+                      "@type": type.googleapis.com/google.protobuf.BoolValue
+                  value_match:
+                    exact: foo
+              on_match:
+                action:
+                  name: test_action
+                  typed_config:
+                    "@type": type.googleapis.com/google.protobuf.StringValue
+                    value: unexpected
+  )EOF";
+
+  envoy::config::common::matcher::v3::Matcher matcher;
+  MessageUtil::loadFromYaml(yaml, matcher, ProtobufMessage::getStrictValidationVisitor());
+
+  TestUtility::validate(matcher);
+
+  auto outer_factory = TestDataInputStringFactory("value");
+  auto inner_factory = TestDataInputBoolFactory("bar");
+
+  EXPECT_CALL(validation_visitor_,
+              performDataInputValidation(_, "type.googleapis.com/google.protobuf.StringValue"));
+  EXPECT_CALL(validation_visitor_,
+              performDataInputValidation(_, "type.googleapis.com/google.protobuf.BoolValue"))
+      .Times(2);
+  auto match_tree = factory_.create(matcher);
+
+  const auto result = match_tree()->match(TestData());
+  EXPECT_THAT(result, HasStringAction("expected!"));
+}
+
+TEST_F(MatcherTest, TestPrefixMatcherWithRetryInnerMissRetriesShorterPrefix) {
+  const std::string yaml = R"EOF(
+matcher_tree:
+  input:
+    name: outer_input
+    typed_config:
+      "@type": type.googleapis.com/google.protobuf.StringValue
+  prefix_match_map:
+    map:
+      val:
+        matcher:
+          matcher_list:
+            matchers:
+            - predicate:
+                single_predicate:
+                  input:
+                    name: inner_input
+                    typed_config:
+                      "@type": type.googleapis.com/google.protobuf.BoolValue
+                  value_match:
+                    exact: bar
+              on_match:
+                action:
+                  name: test_action
+                  typed_config:
+                    "@type": type.googleapis.com/google.protobuf.StringValue
+                    value: expected!
+      valu:
+        matcher:
+          matcher_list:
+            matchers:
+            - predicate:
+                single_predicate:
+                  input:
+                    name: inner_input
+                    typed_config:
+                      "@type": type.googleapis.com/google.protobuf.BoolValue
+                  value_match:
+                    exact: foo
+              on_match:
+                action:
+                  name: test_action
+                  typed_config:
+                    "@type": type.googleapis.com/google.protobuf.StringValue
+                    value: not expected
+  )EOF";
+
+  envoy::config::common::matcher::v3::Matcher matcher;
+  MessageUtil::loadFromYaml(yaml, matcher, ProtobufMessage::getStrictValidationVisitor());
+
+  TestUtility::validate(matcher);
+
+  auto outer_factory = TestDataInputStringFactory("value");
+  auto inner_factory = TestDataInputBoolFactory("bar");
+
+  EXPECT_CALL(validation_visitor_,
+              performDataInputValidation(_, "type.googleapis.com/google.protobuf.StringValue"));
+  EXPECT_CALL(validation_visitor_,
+              performDataInputValidation(_, "type.googleapis.com/google.protobuf.BoolValue"))
+      .Times(2);
+  auto match_tree = factory_.create(matcher);
+
+  const auto result = match_tree()->match(TestData());
+  EXPECT_THAT(result, HasStringAction("expected!"));
+}
+
+TEST_F(MatcherTest, TestPrefixMatcherWithoutRetryInnerMissDoesNotRetryShorterPrefix) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues(
+      {{"envoy.reloadable_features.prefix_map_matcher_resume_after_subtree_miss", "false"}});
+  const std::string yaml = R"EOF(
+matcher_tree:
+  input:
+    name: outer_input
+    typed_config:
+      "@type": type.googleapis.com/google.protobuf.StringValue
+  prefix_match_map:
+    map:
+      val:
+        matcher:
+          matcher_list:
+            matchers:
+            - predicate:
+                single_predicate:
+                  input:
+                    name: inner_input
+                    typed_config:
+                      "@type": type.googleapis.com/google.protobuf.BoolValue
+                  value_match:
+                    exact: bar
+              on_match:
+                action:
+                  name: test_action
+                  typed_config:
+                    "@type": type.googleapis.com/google.protobuf.StringValue
+                    value: expected!
+      valu:
+        matcher:
+          matcher_list:
+            matchers:
+            - predicate:
+                single_predicate:
+                  input:
+                    name: inner_input
+                    typed_config:
+                      "@type": type.googleapis.com/google.protobuf.BoolValue
+                  value_match:
+                    exact: foo
+              on_match:
+                action:
+                  name: test_action
+                  typed_config:
+                    "@type": type.googleapis.com/google.protobuf.StringValue
+                    value: not expected
+  )EOF";
+
+  envoy::config::common::matcher::v3::Matcher matcher;
+  MessageUtil::loadFromYaml(yaml, matcher, ProtobufMessage::getStrictValidationVisitor());
+
+  TestUtility::validate(matcher);
+
+  auto outer_factory = TestDataInputStringFactory("value");
+  auto inner_factory = TestDataInputBoolFactory("bar");
+
+  EXPECT_CALL(validation_visitor_,
+              performDataInputValidation(_, "type.googleapis.com/google.protobuf.StringValue"));
+  EXPECT_CALL(validation_visitor_,
+              performDataInputValidation(_, "type.googleapis.com/google.protobuf.BoolValue"))
+      .Times(2);
+  auto match_tree = factory_.create(matcher);
+
+  const auto result = match_tree()->match(TestData());
+  EXPECT_THAT(result, HasNoMatch());
+}
+
 TEST_F(MatcherTest, TestInvalidFloatPrefixMapMatcher) {
   const std::string yaml = R"EOF(
 matcher_tree:
diff --git a/test/common/matcher/prefix_map_matcher_test.cc b/test/common/matcher/prefix_map_matcher_test.cc
@@ -124,5 +124,23 @@ TEST(PrefixMapMatcherTest, MoreDataMightBeAvailableMatch) {
   verifyImmediateMatch(result, "match");
 }
 
+TEST(PrefixMapMatcherTest, MoreDataMightBeAvailableNoMatchThenMatchDoesNotPerformSecondMatch) {
+  std::unique_ptr<PrefixMapMatcher<TestData>> matcher = *PrefixMapMatcher<TestData>::create(
+      std::make_unique<TestInput>(DataInputGetResult{
+          DataInputGetResult::DataAvailability::MoreDataMightBeAvailable, "match"}),
+      stringOnMatch<TestData>("no_match"));
+  std::unique_ptr<PrefixMapMatcher<TestData>> child_matcher = *PrefixMapMatcher<TestData>::create(
+      std::make_unique<TestInput>(DataInputGetResult{
+          DataInputGetResult::DataAvailability::MoreDataMightBeAvailable, "match"}),
+      absl::nullopt);
+
+  matcher->addChild("match", {nullptr, std::move(child_matcher)});
+  matcher->addChild("mat", stringOnMatch<TestData>("second_match"));
+
+  TestData data;
+  const auto result = matcher->match(data);
+  verifyNotEnoughDataForMatch(result);
+}
+
 } // namespace Matcher
 } // namespace Envoy

Original file line number	Diff line number	Diff line change
`@@ -30,6 +30,7 @@ envoy_cc_library(`
`30`	`30`	`deps = [`
`31`	`31`	`":map_matcher_lib",`
`32`	`32`	`"//source/common/common:trie_lookup_table_lib",`
	`33`	`+ "//source/common/runtime:runtime_features_lib",`
`33`	`34`	`],`
`34`	`35`	`)`
`35`	`36`