ext_proc: send attributes

source/extensions/filters/http/ext_proc/ext_proc.cc

@@ -1,7 +1,5 @@
 #include "source/extensions/filters/http/ext_proc/ext_proc.h"
 
-#include <typeinfo>
-
 #include "envoy/config/common/mutation_rules/v3/mutation_rules.pb.h"
 
 #include "source/common/http/utility.h"
@@ -244,12 +242,10 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st
   FilterHeadersStatus status = FilterHeadersStatus::Continue;
   if (decoding_state_.sendHeaders()) {
     absl::optional<Envoy::ProtobufWkt::Struct> proto;
-    if (config().hasRequestExpr()) {
+    if (config_->expressionManager().hasRequestExpr()) {
       auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(),
                                                                     &headers, nullptr, nullptr);
-      ExpressionManager::printExprPtrAndType(config().getExprPtr("request.path"),
-                                             "in decodeHeaders");
-      proto = config().evaluateRequestAttributes(std::move(activation_ptr));
+      proto = config_->expressionManager().evaluateRequestAttributes(std::move(activation_ptr));
     }
 
     status = onHeaders(decoding_state_, headers, end_stream, proto);
@@ -531,10 +527,10 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s
   FilterHeadersStatus status = FilterHeadersStatus::Continue;
   if (!processing_complete_ && encoding_state_.sendHeaders()) {
     absl::optional<Envoy::ProtobufWkt::Struct> proto;
-    if (config_->hasResponseExpr()) {
+    if (config_->expressionManager().hasResponseExpr()) {
       auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(),
                                                                     nullptr, &headers, nullptr);
-      proto = config_->evaluateResponseAttributes(std::move(activation_ptr));
+      proto = config_->expressionManager().evaluateResponseAttributes(std::move(activation_ptr));
     }
 
     status = onHeaders(encoding_state_, headers, end_stream, proto);

source/extensions/filters/http/ext_proc/ext_proc.h

@@ -141,15 +141,7 @@ class FilterConfig {
             MatchingUtils::initHeaderMatchers(config.forward_rules().allowed_headers())),
         disallowed_headers_(
             MatchingUtils::initHeaderMatchers(config.forward_rules().disallowed_headers())),
-        expression_manager_(builder, config.request_attributes(), config.response_attributes()) {
-    printExprPtrAndType("in FilterConfig constructor");
-  }
-
-  // TODO delete
-  void printExprPtrAndType(std::string location) {
-    expression_manager_.printExprPtrAndType(expression_manager_.getExprPtr("request.path"),
-                                            location);
-  }
+        expression_manager_(builder, config.request_attributes(), config.response_attributes()) {}
 
   bool failureModeAllow() const { return failure_mode_allow_; }
 
@@ -179,24 +171,7 @@ class FilterConfig {
 
   const Envoy::ProtobufWkt::Struct& filterMetadata() const { return filter_metadata_; }
 
-  const absl::optional<ProtobufWkt::Struct>
-  evaluateRequestAttributes(const Filters::Common::Expr::ActivationPtr& activation) const {
-    return expression_manager_.evaluateRequestAttributes(std::move(activation));
-  }
-
-  const absl::optional<ProtobufWkt::Struct>
-  evaluateResponseAttributes(const Filters::Common::Expr::ActivationPtr& activation) const {
-    return expression_manager_.evaluateResponseAttributes(std::move(activation));
-  }
-
-  bool hasRequestExpr() const { return expression_manager_.hasRequestExpr(); }
-
-  bool hasResponseExpr() const { return expression_manager_.hasResponseExpr(); }
-
-  // TODO: delete
-  const ExpressionManager::ExpressionPtrWithExpr& getExprPtr(std::string matcher) const {
-    return expression_manager_.getExprPtr(matcher);
-  }
+  const ExpressionManager& expressionManager() const { return expression_manager_; }
 
 private:
   ExtProcFilterStats generateStats(const std::string& prefix,
@@ -271,9 +246,7 @@ class Filter : public Logger::Loggable<Logger::Id::ext_proc>,
       : config_(config), client_(std::move(client)), stats_(config->stats()),
         grpc_service_(grpc_service), config_with_hash_key_(grpc_service),
         decoding_state_(*this, config->processingMode()),
-        encoding_state_(*this, config->processingMode()) {
-    config_->printExprPtrAndType("in Filter constructor");
-  }
+        encoding_state_(*this, config->processingMode()) {}
 
   const FilterConfig& config() const { return *config_; }
 

source/extensions/filters/http/ext_proc/matching_utils.cc

@@ -1,37 +1,33 @@
 #include "source/extensions/filters/http/ext_proc/matching_utils.h"
 
 #include <memory>
-#include <typeinfo>
+
+#if defined(USE_CEL_PARSER)
+#include "parser/parser.h"
+#endif
 
 namespace Envoy {
 namespace Extensions {
 namespace HttpFilters {
 namespace ExternalProcessing {
 
-absl::flat_hash_map<std::string, std::unique_ptr<ExpressionManager::ExpressionPtrWithExpr>>
-ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers) const {
-  absl::flat_hash_map<std::string, std::unique_ptr<ExpressionManager::ExpressionPtrWithExpr>>
-      expressions;
+absl::flat_hash_map<std::string, Filters::Common::Expr::ExpressionPtr>
+ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers) {
+  absl::flat_hash_map<std::string, Filters::Common::Expr::ExpressionPtr> expressions;
 #if defined(USE_CEL_PARSER)
   for (const auto& matcher : matchers) {
     auto parse_status = google::api::expr::parser::Parse(matcher);
     if (!parse_status.ok()) {
       throw EnvoyException("Unable to parse descriptor expression: " +
                            parse_status.status().ToString());
     }
-    const google::api::expr::v1alpha1::Expr& parse_status_expr = parse_status.value().expr();
-    const Filters::Common::Expr::ExpressionPtr& expression =
-        Extensions::Filters::Common::Expr::createExpression(builder_->builder(), parse_status_expr);
-    std::unique_ptr<ExpressionPtrWithExpr> expr =
-        std::make_unique<ExpressionPtrWithExpr>(parse_status_expr, std::move(expression.get()));
-    if (matcher == "request.path") {
-      ExpressionManager::printExprPtrAndType(*expr, "after construction");
-    }
-    expressions.try_emplace(matcher, std::move(expr));
-    if (matcher == "request.path") {
-      ExpressionManager::printExprPtrAndType(*expressions.at(matcher),
-                                             "after placing in container");
-    }
+
+    auto iter = expr_list_.emplace_back(parse_status.value());
+
+    Filters::Common::Expr::ExpressionPtr expression =
+        Extensions::Filters::Common::Expr::createExpression(builder_->builder(), iter.expr());
+
+    expressions.try_emplace(matcher, std::move(expression));
   }
 #else
   ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment."
@@ -41,6 +37,47 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField<std::string>
   return expressions;
 }
 
+const absl::optional<ProtobufWkt::Struct> ExpressionManager::evaluateAttributes(
+    const Filters::Common::Expr::ActivationPtr& activation,
+    const absl::flat_hash_map<std::string, Filters::Common::Expr::ExpressionPtr>& expr) {
+  absl::optional<ProtobufWkt::Struct> proto;
+  if (!expr.empty()) {
+    proto.emplace(ProtobufWkt::Struct{});
+    for (const auto& hash_entry : expr) {
+      ProtobufWkt::Arena arena;
+      const auto result = hash_entry.second->Evaluate(*activation, &arena);
+      if (!result.ok()) {
+        // TODO: Stats?
+        continue;
+      }
+
+      if (result.value().IsError()) {
+        ENVOY_LOG(trace, "error parsing cel expression {}", hash_entry.first);
+        continue;
+      }
+
+      ProtobufWkt::Value value;
+      switch (result.value().type()) {
+      case google::api::expr::runtime::CelValue::Type::kBool:
+        value.set_bool_value(result.value().BoolOrDie());
+        break;
+      case google::api::expr::runtime::CelValue::Type::kNullType:
+        value.set_null_value(ProtobufWkt::NullValue{});
+        break;
+      case google::api::expr::runtime::CelValue::Type::kDouble:
+        value.set_number_value(result.value().DoubleOrDie());
+        break;
+      default:
+        value.set_string_value(Filters::Common::Expr::print(result.value()));
+      }
+
+      (*(proto.value()).mutable_fields())[hash_entry.first] = value;
+    }
+  }
+
+  return proto;
+}
+
 } // namespace ExternalProcessing
 } // namespace HttpFilters
 } // namespace Extensions

source/extensions/filters/http/ext_proc/matching_utils.h

@@ -1,16 +1,10 @@
 #pragma once
 
-#include <typeinfo>
-
 #include "source/common/common/logger.h"
 #include "source/common/common/matchers.h"
 #include "source/common/protobuf/protobuf.h"
 #include "source/extensions/filters/common/expr/evaluator.h"
 
-#if defined(USE_CEL_PARSER)
-#include "parser/parser.h"
-#endif
-
 namespace Envoy {
 namespace Extensions {
 namespace HttpFilters {
@@ -22,32 +16,7 @@ class ExpressionManager : public Logger::Loggable<Logger::Id::ext_proc> {
                     const Protobuf::RepeatedPtrField<std::string>& request_matchers,
                     const Protobuf::RepeatedPtrField<std::string>& response_matchers)
       : builder_(builder), request_expr_(initExpressions(request_matchers)),
-        response_expr_(initExpressions(response_matchers)) {
-    printExprPtrAndType(*request_expr_.at("request.path"), "in ExpressionManager constructor");
-  };
-
-  // This struct exists because the lifetime of the api expr must exceed expressions
-  // parsed from it.
-  struct ExpressionPtrWithExpr {
-    ExpressionPtrWithExpr(const google::api::expr::v1alpha1::Expr& expr,
-                          Filters::Common::Expr::Expression* expr_ptr)
-        : expr_(expr), expression_ptr_(std::move(expr_ptr)){};
-    ~ExpressionPtrWithExpr() {
-      std::cout << "!!!!!! ExpressionPtrWithExpr is being destructed !!!!!!" << std::endl;
-    };
-    const google::api::expr::v1alpha1::Expr& expr_;
-    Filters::Common::Expr::ExpressionPtr expression_ptr_;
-  };
-
-  static void printExprPtrAndType(const ExpressionPtrWithExpr& expr, std::string location) {
-    std::cout << "expression_ptr_ address " << location << ": " << expr.expression_ptr_.get()
-              << std::endl;
-    auto& val = *expr.expression_ptr_.get();
-    std::cout << "expression_ptr_ type " << location << ": " << typeid(val).name() << std::endl;
-    std::cout << "expr_ address " << location << ": " << &expr.expr_ << std::endl;
-    std::cout << "expr_ type " << location << ": " << typeid(expr.expr_).name() << std::endl;
-    std::cout << "----------------------------------------------------------" << std::endl;
-  }
+        response_expr_(initExpressions(response_matchers)){};
 
   bool hasRequestExpr() const { return !request_expr_.empty(); };
 
@@ -65,59 +34,19 @@ class ExpressionManager : public Logger::Loggable<Logger::Id::ext_proc> {
 
   static const absl::optional<ProtobufWkt::Struct> evaluateAttributes(
       const Filters::Common::Expr::ActivationPtr& activation,
-      const absl::flat_hash_map<std::string, std::unique_ptr<ExpressionPtrWithExpr>>& expr) {
-    absl::optional<ProtobufWkt::Struct> proto;
-    if (!expr.empty()) {
-      proto.emplace(ProtobufWkt::Struct{});
-      for (const auto& hash_entry : expr) {
-        ProtobufWkt::Arena arena;
-        printExprPtrAndType(*hash_entry.second, "in evaluateAttributes");
-        const auto result = hash_entry.second->expression_ptr_.get()->Evaluate(*activation, &arena);
-        if (!result.ok()) {
-          // TODO: Stats?
-          continue;
-        }
-
-        if (result.value().IsError()) {
-          ENVOY_LOG(trace, "error parsing cel expression {}", hash_entry.first);
-          continue;
-        }
-
-        ProtobufWkt::Value value;
-        switch (result.value().type()) {
-        case google::api::expr::runtime::CelValue::Type::kBool:
-          value.set_bool_value(result.value().BoolOrDie());
-          break;
-        case google::api::expr::runtime::CelValue::Type::kNullType:
-          value.set_null_value(ProtobufWkt::NullValue{});
-          break;
-        case google::api::expr::runtime::CelValue::Type::kDouble:
-          value.set_number_value(result.value().DoubleOrDie());
-          break;
-        default:
-          value.set_string_value(Filters::Common::Expr::print(result.value()));
-        }
-
-        (*(proto.value()).mutable_fields())[hash_entry.first] = value;
-      }
-    }
-
-    return proto;
-  }
-
-  // TODO: delete
-  const ExpressionPtrWithExpr& getExprPtr(std::string matcher) const {
-    return *request_expr_.at(matcher);
-  }
+      const absl::flat_hash_map<std::string, Filters::Common::Expr::ExpressionPtr>& expr);
 
 private:
-  absl::flat_hash_map<std::string, std::unique_ptr<ExpressionPtrWithExpr>>
-  initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers) const;
+  // This list is required to maintain the lifetimes of expr objects on which compiled expressions
+  // depend
+  std::list<google::api::expr::v1alpha1::ParsedExpr> expr_list_;
+  absl::flat_hash_map<std::string, Filters::Common::Expr::ExpressionPtr>
+  initExpressions(const Protobuf::RepeatedPtrField<std::string>& matchers);
 
   Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_;
 
-  const absl::flat_hash_map<std::string, std::unique_ptr<ExpressionPtrWithExpr>> request_expr_;
-  const absl::flat_hash_map<std::string, std::unique_ptr<ExpressionPtrWithExpr>> response_expr_;
+  const absl::flat_hash_map<std::string, Filters::Common::Expr::ExpressionPtr> request_expr_;
+  const absl::flat_hash_map<std::string, Filters::Common::Expr::ExpressionPtr> response_expr_;
 };
 
 class MatchingUtils : public Logger::Loggable<Logger::Id::ext_proc> {

test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc

@@ -3315,11 +3315,11 @@ TEST_P(ExtProcIntegrationTest, GetAndSetRequestResponseAttributes) {
   proto_config_.mutable_processing_mode()->set_request_header_mode(ProcessingMode::SEND);
   proto_config_.mutable_processing_mode()->set_response_header_mode(ProcessingMode::SEND);
   proto_config_.mutable_request_attributes()->Add("request.path");
-  /* proto_config_.mutable_request_attributes()->Add("request.method"); */
-  /* proto_config_.mutable_request_attributes()->Add("request.scheme"); */
-  /* proto_config_.mutable_request_attributes()->Add("connection.mtls"); */
-  /* proto_config_.mutable_response_attributes()->Add("response.code"); */
-  /* proto_config_.mutable_response_attributes()->Add("response.code_details"); */
+  proto_config_.mutable_request_attributes()->Add("request.method");
+  proto_config_.mutable_request_attributes()->Add("request.scheme");
+  proto_config_.mutable_request_attributes()->Add("connection.mtls");
+  proto_config_.mutable_response_attributes()->Add("response.code");
+  proto_config_.mutable_response_attributes()->Add("response.code_details");
 
   initializeConfig();
   HttpIntegrationTest::initialize();
@@ -3329,25 +3329,22 @@ TEST_P(ExtProcIntegrationTest, GetAndSetRequestResponseAttributes) {
         EXPECT_EQ(req.attributes().size(), 1);
         auto proto_struct = req.attributes().at("envoy.filters.http.ext_proc");
         EXPECT_EQ(proto_struct.fields().at("request.path").string_value(), "/");
-        /* EXPECT_EQ(proto_struct.fields().at("request.method").string_value(), "GET"); */
-        /* EXPECT_EQ(proto_struct.fields().at("request.scheme").string_value(), "http"); */
-        /* EXPECT_EQ(proto_struct.fields().at("connection.mtls").bool_value(), false); */
+        EXPECT_EQ(proto_struct.fields().at("request.method").string_value(), "GET");
+        EXPECT_EQ(proto_struct.fields().at("request.scheme").string_value(), "http");
+        EXPECT_EQ(proto_struct.fields().at("connection.mtls").bool_value(), false);
         return true;
       });
 
   handleUpstreamRequest();
 
-  processResponseHeadersMessage(*grpc_upstreams_[0], false,
-                                [](const HttpHeaders& /* req*/, HeadersResponse&) {
-                                  /* EXPECT_EQ(req.attributes().size(), 1); */
-                                  /* auto proto_struct =
-                                   * req.attributes().at("envoy.filters.http.ext_proc"); */
-                                  /* EXPECT_EQ(proto_struct.fields().at("response.code").string_value(),
-                                   * "200"); */
-                                  /* EXPECT_EQ(proto_struct.fields().at("response.code_details").string_value(),
-                                   * "via_upstream"); */
-                                  return true;
-                                });
+  processResponseHeadersMessage(
+      *grpc_upstreams_[0], false, [](const HttpHeaders& req, HeadersResponse&) {
+        EXPECT_EQ(req.attributes().size(), 1);
+        auto proto_struct = req.attributes().at("envoy.filters.http.ext_proc");
+        EXPECT_EQ(proto_struct.fields().at("response.code").string_value(), "200");
+        EXPECT_EQ(proto_struct.fields().at("response.code_details").string_value(), "via_upstream");
+        return true;
+      });
 
   verifyDownstreamResponse(*response, 200);
 }