cel: add support for re-evaluating CEL expression on response path

changelogs/current.yaml

@@ -298,6 +298,10 @@ new_features:
 - area: composite
   change: |
     Allow composite filter to be configured to insert a filter into filter chain outside of the decode headers lifecycle phase.
+- area: cel
+  change: |
+    Added functionality to reevaluate CEL expressions that attempt to read response path data on
+    the request path once the data is available. Allows CEL matching based on both request and response headers.
 - area: rbac
   change: |
     Switch the IP matcher to use LC-Trie for performance improvements.

envoy/matcher/matcher.h

@@ -135,7 +135,7 @@ struct MatchResult {
   static MatchResult noMatch() { return MatchResult(NoMatch{}); }
   static MatchResult insufficientData() { return MatchResult(InsufficientData{}); }
   bool isInsufficientData() const { return absl::holds_alternative<InsufficientData>(result_); }
-  bool isComplete() const { return !isInsufficientData(); }
+  bool isComplete() const { return !isInsufficientData() || isMatch(); }
   bool isNoMatch() const { return absl::holds_alternative<NoMatch>(result_); }
   bool isMatch() const { return absl::holds_alternative<ActionConstSharedPtr>(result_); }
   const ActionConstSharedPtr& action() const {

source/extensions/filters/common/expr/evaluator.cc

@@ -57,6 +57,7 @@ absl::optional<CelValue> StreamActivation::FindValue(absl::string_view name,
     return CelValue::CreateMap(
         Protobuf::Arena::Create<RequestWrapper>(arena, *arena, activation_request_headers_, info));
   case ActivationToken::Response:
+    response_path_data_needed_ = true;
     return CelValue::CreateMap(Protobuf::Arena::Create<ResponseWrapper>(
         arena, *arena, activation_response_headers_, activation_response_trailers_, info));
   case ActivationToken::Connection:

source/extensions/filters/common/expr/evaluator.h

@@ -30,8 +30,6 @@ namespace Filters {
 namespace Common {
 namespace Expr {
 
-using Activation = google::api::expr::runtime::BaseActivation;
-using ActivationPtr = std::unique_ptr<Activation>;
 using Builder = google::api::expr::runtime::CelExpressionBuilder;
 using BuilderConstPtr = std::unique_ptr<const Builder>;
 using Expression = google::api::expr::runtime::CelExpression;
@@ -57,6 +55,7 @@ class StreamActivation : public google::api::expr::runtime::BaseActivation {
   FindFunctionOverloads(absl::string_view) const override {
     return {};
   }
+  bool response_path_data_needed() const { return response_path_data_needed_; }
 
 protected:
   void resetActivation() const;
@@ -65,8 +64,12 @@ class StreamActivation : public google::api::expr::runtime::BaseActivation {
   mutable const ::Envoy::Http::RequestHeaderMap* activation_request_headers_{nullptr};
   mutable const ::Envoy::Http::ResponseHeaderMap* activation_response_headers_{nullptr};
   mutable const ::Envoy::Http::ResponseTrailerMap* activation_response_trailers_{nullptr};
+  mutable bool response_path_data_needed_{false};
 };
 
+using Activation = StreamActivation;
+using ActivationPtr = std::unique_ptr<Activation>;
+
 // Creates an activation providing the common context attributes.
 // The activation lazily creates wrappers during an evaluation using the evaluation arena.
 ActivationPtr createActivation(const ::Envoy::LocalInfo::LocalInfo* local_info,

source/extensions/filters/http/match_delegate/config.cc

@@ -119,7 +119,7 @@ void DelegatingStreamFilter::FilterMatchState::evaluateMatchTree(
 
   match_tree_evaluated_ = match_result.isComplete();
 
-  if (match_tree_evaluated_ && match_result.isMatch()) {
+  if (match_result.isMatch()) {
     const auto& result = match_result.action();
     if (result == nullptr || SkipAction().typeUrl() == result->typeUrl()) {
       skip_filter_ = true;

source/extensions/matching/http/cel_input/cel_input.h

@@ -22,13 +22,18 @@ using ::Envoy::Http::RequestHeaderMapOptConstRef;
 using ::Envoy::Http::ResponseHeaderMapOptConstRef;
 using ::Envoy::Http::ResponseTrailerMapOptConstRef;
 
-using BaseActivationPtr = std::unique_ptr<google::api::expr::runtime::BaseActivation>;
+using StreamActivationPtr = std::unique_ptr<Filters::Common::Expr::StreamActivation>;
 
 // CEL matcher specific matching data
 class CelMatchData : public ::Envoy::Matcher::CustomMatchData {
 public:
-  explicit CelMatchData(BaseActivationPtr activation) : activation_(std::move(activation)) {}
-  BaseActivationPtr activation_;
+  explicit CelMatchData(StreamActivationPtr activation) : activation_(std::move(activation)) {}
+  bool needs_reinvoked_on_response() const {
+    // Allows us to skip re-evaluating the CEL expression if it did not rely on
+    // response path data during its first evaluation.
+    return activation_->response_path_data_needed();
+  }
+  StreamActivationPtr activation_;
 };
 
 class HttpCelDataInput : public Matcher::DataInput<Envoy::Http::HttpMatchingData> {
@@ -41,14 +46,17 @@ class HttpCelDataInput : public Matcher::DataInput<Envoy::Http::HttpMatchingData
 
     // CEL library supports mixed matching of request/response attributes(e.g., headers, trailers)
     // and attributes from stream info.
-    std::unique_ptr<google::api::expr::runtime::BaseActivation> activation =
-        Extensions::Filters::Common::Expr::createActivation(
-            nullptr, // TODO: pass local_info to CEL activation.
-            data.streamInfo(), maybe_request_headers.ptr(), maybe_response_headers.ptr(),
-            maybe_response_trailers.ptr());
-
-    return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable,
-            std::make_unique<CelMatchData>(std::move(activation))};
+    StreamActivationPtr activation = Extensions::Filters::Common::Expr::createActivation(
+        nullptr, // TODO: pass local_info to CEL activation.
+        data.streamInfo(), maybe_request_headers.ptr(), maybe_response_headers.ptr(),
+        maybe_response_trailers.ptr());
+    Matcher::DataInputGetResult::DataAvailability availability =
+        !data.requestHeaders().has_value() || !data.responseHeaders().has_value() ||
+                !data.responseTrailers().has_value()
+            ? Matcher::DataInputGetResult::DataAvailability::MoreDataMightBeAvailable
+            : Matcher::DataInputGetResult::DataAvailability::AllDataAvailable;
+
+    return {availability, std::make_unique<CelMatchData>(std::move(activation))};
   }
 
   absl::string_view dataInputType() const override { return "cel_data_input"; }

source/extensions/matching/input_matchers/cel_matcher/matcher.cc

@@ -22,6 +22,9 @@ CelInputMatcher::CelInputMatcher(CelMatcherSharedPtr cel_matcher,
       }()) {}
 
 bool CelInputMatcher::match(const MatchingDataType& input) {
+  if (cached_result_.has_value()) {
+    return *cached_result_;
+  }
   Protobuf::Arena arena;
   if (auto* ptr = absl::get_if<std::shared_ptr<::Envoy::Matcher::CustomMatchData>>(&input);
       ptr != nullptr) {
@@ -32,7 +35,11 @@ bool CelInputMatcher::match(const MatchingDataType& input) {
 
     auto eval_result = compiled_expr_.evaluate(*cel_data->activation_, &arena);
     if (eval_result.ok() && eval_result.value().IsBool()) {
-      return eval_result.value().BoolOrDie();
+      bool result = eval_result.value().BoolOrDie();
+      if (!cel_data->needs_reinvoked_on_response()) {
+        cached_result_ = result;
+      }
+      return result;
     }
   }
 

source/extensions/matching/input_matchers/cel_matcher/matcher.h

@@ -10,6 +10,7 @@
 #include "source/extensions/filters/common/expr/evaluator.h"
 #include "source/extensions/matching/http/cel_input/cel_input.h"
 
+#include "absl/types/optional.h"
 #include "absl/types/variant.h"
 #include "xds/type/matcher/v3/cel.pb.h"
 
@@ -41,6 +42,10 @@ class CelInputMatcher : public InputMatcher, public Logger::Loggable<Logger::Id:
 
 private:
   const Filters::Common::Expr::CompiledExpression compiled_expr_;
+  // Result may be cached if it does not require fields which are unavailable;
+  // e.g. response path headers or trailers. Allows us to skip re-evaluation in
+  // cases where it would not change.
+  absl::optional<bool> cached_result_;
 };
 
 } // namespace CelMatcher

test/extensions/matching/input_matchers/cel_matcher/cel_matcher_test.cc

@@ -34,6 +34,7 @@ using ::Envoy::Http::LowerCaseString;
 using ::Envoy::Http::TestRequestHeaderMapImpl;
 using ::Envoy::Http::TestResponseHeaderMapImpl;
 using ::Envoy::Http::TestResponseTrailerMapImpl;
+using ::Envoy::Matcher::HasInsufficientData;
 using ::Envoy::Matcher::HasNoMatch;
 using ::Envoy::Matcher::HasStringAction;
 
@@ -191,15 +192,115 @@ TEST_F(CelMatcherTest, CelMatcherRequestHeaderNotMatched) {
   buildCustomHeader({{"authenticated_user", "NOT_MATCHED"}}, request_headers);
   data_.onRequestHeaders(request_headers);
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 
   // Build header with request header key field mismatched case.
   TestRequestHeaderMapImpl request_headers_2 = default_headers_;
   buildCustomHeader({{"NOT_MATCHED", "staging"}}, request_headers_2);
   Envoy::Http::Matching::HttpMatchingDataImpl data_2 =
       Envoy::Http::Matching::HttpMatchingDataImpl(stream_info_);
   data_2.onRequestHeaders(request_headers_2);
-  EXPECT_THAT(matcher_tree->match(data_2), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_2), HasInsufficientData());
+}
+
+// Tests CEL expression that must be invoked twice: request headers + response headers.
+TEST_F(CelMatcherTest, CelMatcherRequestInsufficientDataResponseHeaderMatched) {
+  auto matcher_tree = buildMatcherTree(ResponseHeaderAndPathCelExprString);
+
+  TestRequestHeaderMapImpl request_headers;
+  buildCustomHeader({{":path", "/foo"}}, request_headers);
+  data_.onRequestHeaders(request_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+
+  TestResponseHeaderMapImpl response_headers;
+  response_headers.addCopy(LowerCaseString(":status"), "200");
+  response_headers.addCopy(LowerCaseString("content-type"), "text/plain");
+  response_headers.addCopy(LowerCaseString("content-length"), "3");
+  data_.onResponseHeaders(response_headers);
+
+  auto result = matcher_tree->match(data_);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasStringAction("match!!"));
+}
+
+// Tests CEL expression that must be invoked twice, but does not match on the response path.
+TEST_F(CelMatcherTest, CelMatcherRequestInsufficientDataResponseHeaderNotMatched) {
+  auto matcher_tree = buildMatcherTree(ResponseHeaderAndPathCelExprString);
+
+  TestRequestHeaderMapImpl request_headers;
+  buildCustomHeader({{":path", "/foo"}}, request_headers);
+  data_.onRequestHeaders(request_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+
+  TestResponseHeaderMapImpl response_headers;
+  response_headers.addCopy(LowerCaseString(":status"), "200");
+  response_headers.addCopy(LowerCaseString("content-type"), "text/html");
+  response_headers.addCopy(LowerCaseString("content-length"), "3");
+  data_.onResponseHeaders(response_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+}
+
+// Tests CEL expression that matches on both request + response, but does not match on the request
+// path.
+TEST_F(CelMatcherTest, CelMatcherRequestInsufficientDataPathNotMatched) {
+  auto matcher_tree = buildMatcherTree(ResponseHeaderAndPathCelExprString);
+
+  TestRequestHeaderMapImpl request_headers;
+  buildCustomHeader({{":path", "/bar"}}, request_headers);
+  data_.onRequestHeaders(request_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+
+  TestResponseHeaderMapImpl response_headers;
+  response_headers.addCopy(LowerCaseString(":status"), "200");
+  response_headers.addCopy(LowerCaseString("content-type"), "text/plain");
+  response_headers.addCopy(LowerCaseString("content-length"), "3");
+  data_.onResponseHeaders(response_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+}
+
+TEST_F(CelMatcherTest, CelMatcherCachesMatchesNotReliantOnResponseData) {
+  auto matcher_tree = buildMatcherTree(RequestHeaderCelExprString);
+
+  TestRequestHeaderMapImpl request_headers;
+  buildCustomHeader({{"authenticated_user", "not-staging"}}, request_headers);
+  data_.onRequestHeaders(request_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+
+  // Call onRequestHeaders again, but this time change the data so that it should've matched. If the
+  // CEL evaluation was cached, it will remain InsufficientData.
+  buildCustomHeader({{"authenticated_user", "staging"}}, request_headers);
+  data_.onRequestHeaders(request_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+}
+
+// Tests CEL matcher returns HasNoMatch after seeing trailers.
+TEST_F(CelMatcherTest, CelMatcherRequestResponseMoreDataAvailNotMatched) {
+  auto matcher_tree = buildMatcherTree(ResponseHeaderAndPathCelExprString);
+
+  TestRequestHeaderMapImpl request_headers;
+  buildCustomHeader({{":path", "/bar"}}, request_headers);
+  data_.onRequestHeaders(request_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+
+  TestResponseHeaderMapImpl response_headers;
+  response_headers.addCopy(LowerCaseString(":status"), "200");
+  response_headers.addCopy(LowerCaseString("content-type"), "text/plain");
+  response_headers.addCopy(LowerCaseString("content-length"), "3");
+  data_.onResponseHeaders(response_headers);
+
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
+
+  TestResponseTrailerMapImpl response_trailers = {{"transfer-encoding", "chunked"}};
+  data_.onResponseTrailers(response_trailers);
+  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
 }
 
 TEST_F(CelMatcherTest, CelMatcherClusterMetadataMatched) {
@@ -220,7 +321,7 @@ TEST_F(CelMatcherTest, CelMatcherClusterMetadataNotMatched) {
   auto matcher_tree = buildMatcherTree(absl::StrFormat(
       UpstreamClusterMetadataCelString, kFilterNamespace, kMetadataKey, kMetadataValue));
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherRouteMetadataMatched) {
@@ -241,7 +342,7 @@ TEST_F(CelMatcherTest, CelMatcherRouteMetadataNotMatched) {
   auto matcher_tree = buildMatcherTree(absl::StrFormat(
       UpstreamClusterMetadataCelString, kFilterNamespace, kMetadataKey, kMetadataValue));
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherDynamicMetadataMatched) {
@@ -261,7 +362,7 @@ TEST_F(CelMatcherTest, CelMatcherDynamicMetadataNotMatched) {
   auto matcher_tree = buildMatcherTree(
       absl::StrFormat(DynamicMetadataCelString, kFilterNamespace, kMetadataKey, kMetadataValue));
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherTypedDynamicMetadataMatched) {
@@ -307,7 +408,7 @@ TEST_F(CelMatcherTest, CelMatcherRequestHeaderPathNotMatched) {
   buildCustomHeader({{":path", "/bar"}}, request_headers);
   data_.onRequestHeaders(request_headers);
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherResponseHeaderMatched) {
@@ -328,7 +429,7 @@ TEST_F(CelMatcherTest, CelMatcherResponseHeaderNotMatched) {
   TestResponseHeaderMapImpl response_headers = {{"content-type", "text/html"}};
   data_.onResponseHeaders(response_headers);
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherResponseTrailerMatched) {
@@ -346,7 +447,7 @@ TEST_F(CelMatcherTest, CelMatcherResponseTrailerNotMatched) {
   TestResponseTrailerMapImpl response_trailers = {{"transfer-encoding", "chunked_not_matched"}};
   data_.onResponseTrailers(response_trailers);
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherRequestHeaderAndPathMatched) {
@@ -366,7 +467,7 @@ TEST_F(CelMatcherTest, CelMatcherRequestHeaderAndPathNotMatched) {
   buildCustomHeader({{"user", "prod"}, {":path", "/foo"}}, request_headers);
   data_.onRequestHeaders(request_headers);
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherRequestHeaderOrPathMatched) {
@@ -386,7 +487,7 @@ TEST_F(CelMatcherTest, CelMatcherRequestHeaderOrPathNotMatched) {
   buildCustomHeader({{"user", "prod"}, {":path", "/bar"}}, request_headers);
   data_.onRequestHeaders(request_headers);
 
-  EXPECT_THAT(matcher_tree->match(data_), HasNoMatch());
+  EXPECT_THAT(matcher_tree->match(data_), HasInsufficientData());
 }
 
 TEST_F(CelMatcherTest, CelMatcherRequestResponseMatched) {