From 87cf9ba6180bc0586459c872cfc7b03c8fb001b7 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 8 Nov 2023 11:33:05 -0500 Subject: [PATCH 01/35] extract attributes changes from #29069 Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/v3/ext_proc.proto | 4 - changelogs/current.yaml | 7 ++ source/extensions/filters/http/ext_proc/BUILD | 25 +++++- .../filters/http/ext_proc/config.cc | 13 +-- .../extensions/filters/http/ext_proc/config.h | 3 +- .../filters/http/ext_proc/ext_proc.cc | 85 ++++++++++++++++++- .../filters/http/ext_proc/ext_proc.h | 38 ++++++++- .../filters/http/ext_proc/processor_state.h | 6 ++ test/extensions/filters/http/ext_proc/BUILD | 18 ++++ .../filters/http/ext_proc/config_test.cc | 16 +++- .../ext_proc/ext_proc_integration_test.cc | 68 ++++++++++++++- .../filters/http/ext_proc/filter_test.cc | 6 +- .../filters/http/ext_proc/ordering_test.cc | 4 + .../unit_test_fuzz/ext_proc_unit_test_fuzz.cc | 4 +- 14 files changed, 270 insertions(+), 27 deletions(-) diff --git a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto index c9500486a3a5..37d289ec6a6f 100644 --- a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto +++ b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto @@ -125,7 +125,6 @@ message ExternalProcessor { // for a reply. bool async_mode = 4; - // [#not-implemented-hide:] // Envoy provides a number of :ref:`attributes ` // for expressive policies. Each attribute name provided in this field will be // matched against that list and populated in the request_headers message. @@ -133,7 +132,6 @@ message ExternalProcessor { // for the list of supported attributes and their types. repeated string request_attributes = 5; - // [#not-implemented-hide:] // Envoy provides a number of :ref:`attributes ` // for expressive policies. Each attribute name provided in this field will be // matched against that list and populated in the response_headers message. @@ -257,12 +255,10 @@ message ExtProcOverrides { // Set a different asynchronous processing option than the default. bool async_mode = 2; - // [#not-implemented-hide:] // Set different optional attributes than the default setting of the // ``request_attributes`` field. repeated string request_attributes = 3; - // [#not-implemented-hide:] // Set different optional properties than the default setting of the // ``response_attributes`` field. repeated string response_attributes = 4; diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 2e952dea71eb..717c8f014fff 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -161,5 +161,12 @@ new_features: Ratelimit supports optional additional prefix to use when emitting statistics with :ref:`stat_prefix ` configuration flag. +- area: ext_proc + change: | + implemented + :ref:`request_attributes ` + and + :ref:`response_attributes ` + config APIs to enable sending and receiving attributes from/to the external processing server. deprecated: diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index 484395f170d3..ef2c32690f8e 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -19,6 +19,12 @@ envoy_cc_library( "ext_proc.h", "processor_state.h", ], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), deps = [ ":client_interface", ":mutation_utils_lib", @@ -29,24 +35,41 @@ envoy_cc_library( "//source/common/buffer:buffer_lib", "//source/common/protobuf", "//source/common/runtime:runtime_features_lib", + "//source/extensions/filters/common/expr:evaluator_lib", "//source/extensions/filters/common/mutation_rules:mutation_rules_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "@com_google_absl//absl/status", "@com_google_absl//absl/strings:str_format", + "@com_google_cel_cpp//eval/public:builtin_func_registrar", + "@com_google_cel_cpp//eval/public:cel_expr_builder_factory", "@envoy_api//envoy/config/common/mutation_rules/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/ext_proc/v3:pkg_cc_proto", "@envoy_api//envoy/service/ext_proc/v3:pkg_cc_proto", - ], + ] + select( + { + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "@com_google_cel_cpp//parser", + ], + }, + ), ) envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), deps = [ ":client_lib", ":ext_proc", + "//source/extensions/filters/common/expr:evaluator_lib", "//source/extensions/filters/http/common:factory_base_lib", "@envoy_api//envoy/extensions/filters/http/ext_proc/v3:pkg_cc_proto", ], diff --git a/source/extensions/filters/http/ext_proc/config.cc b/source/extensions/filters/http/ext_proc/config.cc index 7a0240e84e03..88c171a9f721 100644 --- a/source/extensions/filters/http/ext_proc/config.cc +++ b/source/extensions/filters/http/ext_proc/config.cc @@ -15,9 +15,9 @@ Http::FilterFactoryCb ExternalProcessingFilterConfig::createFilterFactoryFromPro PROTOBUF_GET_MS_OR_DEFAULT(proto_config, message_timeout, DefaultMessageTimeoutMs); const uint32_t max_message_timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs); - const auto filter_config = - std::make_shared(proto_config, std::chrono::milliseconds(message_timeout_ms), - max_message_timeout_ms, context.scope(), stats_prefix); + const auto filter_config = std::make_shared( + proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, + context.scope(), stats_prefix, Envoy::Extensions::Filters::Common::Expr::getBuilder(context)); return [filter_config, grpc_service = proto_config.grpc_service(), &context](Http::FilterChainFactoryCallbacks& callbacks) { @@ -44,9 +44,10 @@ ExternalProcessingFilterConfig::createFilterFactoryFromProtoWithServerContextTyp PROTOBUF_GET_MS_OR_DEFAULT(proto_config, message_timeout, DefaultMessageTimeoutMs); const uint32_t max_message_timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs); - const auto filter_config = - std::make_shared(proto_config, std::chrono::milliseconds(message_timeout_ms), - max_message_timeout_ms, server_context.scope(), stats_prefix); + const auto filter_config = std::make_shared( + proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, + server_context.scope(), stats_prefix, + Envoy::Extensions::Filters::Common::Expr::getBuilder(server_context)); return [filter_config, grpc_service = proto_config.grpc_service(), &server_context](Http::FilterChainFactoryCallbacks& callbacks) { diff --git a/source/extensions/filters/http/ext_proc/config.h b/source/extensions/filters/http/ext_proc/config.h index a2912466eb6b..e18452e262d0 100644 --- a/source/extensions/filters/http/ext_proc/config.h +++ b/source/extensions/filters/http/ext_proc/config.h @@ -5,6 +5,7 @@ #include "envoy/extensions/filters/http/ext_proc/v3/ext_proc.pb.h" #include "envoy/extensions/filters/http/ext_proc/v3/ext_proc.pb.validate.h" +#include "source/extensions/filters/common/expr/evaluator.h" #include "source/extensions/filters/http/common/factory_base.h" namespace Envoy { @@ -29,7 +30,7 @@ class ExternalProcessingFilterConfig Router::RouteSpecificFilterConfigConstSharedPtr createRouteSpecificFilterConfigTyped( const envoy::extensions::filters::http::ext_proc::v3::ExtProcPerRoute& proto_config, - Server::Configuration::ServerFactoryContext& context, + Server::Configuration::ServerFactoryContext&, ProtobufMessage::ValidationVisitor& validator) override; Http::FilterFactoryCb createFilterFactoryFromProtoWithServerContextTyped( diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 754cc6d17d5a..478cca05cc13 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -8,6 +8,10 @@ #include "absl/strings/str_format.h" +#if defined(USE_CEL_PARSER) +#include "parser/parser.h" +#endif + namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -113,6 +117,27 @@ ExtProcLoggingInfo::grpcCalls(envoy::config::core::v3::TrafficDirection traffic_ : encoding_processor_grpc_calls_; } +absl::flat_hash_map +FilterConfig::initExpressions(const Protobuf::RepeatedPtrField& matchers) const { + absl::flat_hash_map 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()); + } + expressions.emplace(matcher, Extensions::Filters::Common::Expr::createExpression( + builder_->builder(), parse_status.value().expr())); + } +#else + ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." + " Attempted to parse " + + std::to_string(matchers.size()) + " expressions"); +#endif + return expressions; +} + FilterConfigPerRoute::FilterConfigPerRoute(const ExtProcPerRoute& config) : disabled_(config.disabled()) { if (config.has_overrides()) { @@ -201,7 +226,8 @@ void Filter::onDestroy() { } FilterHeadersStatus Filter::onHeaders(ProcessorState& state, - Http::RequestOrResponseHeaderMap& headers, bool end_stream) { + Http::RequestOrResponseHeaderMap& headers, bool end_stream, + absl::optional proto) { switch (openStream()) { case StreamOpenState::Error: return FilterHeadersStatus::StopIteration; @@ -219,6 +245,9 @@ FilterHeadersStatus Filter::onHeaders(ProcessorState& state, MutationUtils::headersToProto(headers, config_->allowedHeaders(), config_->disallowedHeaders(), *headers_req->mutable_headers()); headers_req->set_end_of_stream(end_stream); + if (proto.has_value()) { + (*headers_req->mutable_attributes())[FilterName] = proto.value(); + } state.onStartProcessorCall(std::bind(&Filter::onMessageTimeout, this), config_->messageTimeout(), ProcessorState::CallbackState::HeadersCallback); ENVOY_LOG(debug, "Sending headers message"); @@ -228,6 +257,48 @@ FilterHeadersStatus Filter::onHeaders(ProcessorState& state, return FilterHeadersStatus::StopIteration; } +const absl::optional Filter::evaluateAttributes( + Filters::Common::Expr::ActivationPtr activation, + const absl::flat_hash_map& + expr) { + absl::optional proto; + if (expr.size() > 0) { + proto.emplace(ProtobufWkt::Struct{}); + for (const auto& hash_entry : expr) { + ProtobufWkt::Arena arena; + const auto result = hash_entry.second.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; +} + FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_stream) { ENVOY_LOG(trace, "decodeHeaders: end_stream = {}", end_stream); mergePerRouteConfig(); @@ -237,7 +308,11 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { - status = onHeaders(decoding_state_, headers, end_stream); + auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), + &headers, nullptr, nullptr); + auto proto = evaluateAttributes(std::move(activation_ptr), config_->requestExpr()); + + status = onHeaders(decoding_state_, headers, end_stream, proto); ENVOY_LOG(trace, "onHeaders returning {}", static_cast(status)); } else { ENVOY_LOG(trace, "decodeHeaders: Skipped header processing"); @@ -515,7 +590,11 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s FilterHeadersStatus status = FilterHeadersStatus::Continue; if (!processing_complete_ && encoding_state_.sendHeaders()) { - status = onHeaders(encoding_state_, headers, end_stream); + auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), + nullptr, &headers, nullptr); + auto proto = evaluateAttributes(std::move(activation_ptr), config_->responseExpr()); + + status = onHeaders(encoding_state_, headers, end_stream, proto); ENVOY_LOG(trace, "onHeaders returns {}", static_cast(status)); } else { ENVOY_LOG(trace, "encodeHeaders: Skipped header processing"); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 6338cc4e2aae..5500cee6c7af 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -21,6 +21,7 @@ #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" #include "source/extensions/filters/common/mutation_rules/mutation_rules.h" #include "source/extensions/filters/http/common/pass_through_filter.h" #include "source/extensions/filters/http/ext_proc/client.h" @@ -121,12 +122,13 @@ class ExtProcLoggingInfo : public Envoy::StreamInfo::FilterState::Object { Upstream::HostDescriptionConstSharedPtr upstream_host_; }; -class FilterConfig { +class FilterConfig : public Logger::Loggable { public: FilterConfig(const envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor& config, const std::chrono::milliseconds message_timeout, const uint32_t max_message_timeout_ms, Stats::Scope& scope, - const std::string& stats_prefix) + const std::string& stats_prefix, + Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder) : failure_mode_allow_(config.failure_mode_allow()), disable_clear_route_cache_(config.disable_clear_route_cache()), message_timeout_(message_timeout), max_message_timeout_ms_(max_message_timeout_ms), @@ -136,7 +138,9 @@ class FilterConfig { allow_mode_override_(config.allow_mode_override()), disable_immediate_response_(config.disable_immediate_response()), allowed_headers_(initHeaderMatchers(config.forward_rules().allowed_headers())), - disallowed_headers_(initHeaderMatchers(config.forward_rules().disallowed_headers())) {} + disallowed_headers_(initHeaderMatchers(config.forward_rules().disallowed_headers())), + builder_(builder), request_expr_(initExpressions(config.request_attributes())), + response_expr_(initExpressions(config.response_attributes())) {} bool failureModeAllow() const { return failure_mode_allow_; } @@ -166,6 +170,16 @@ class FilterConfig { const Envoy::ProtobufWkt::Struct& filterMetadata() const { return filter_metadata_; } + const absl::flat_hash_map& + requestExpr() const { + return request_expr_; + } + + const absl::flat_hash_map& + responseExpr() const { + return response_expr_; + } + private: ExtProcFilterStats generateStats(const std::string& prefix, const std::string& filter_stats_prefix, Stats::Scope& scope) { @@ -183,6 +197,9 @@ class FilterConfig { return header_matchers; } + absl::flat_hash_map + initExpressions(const Protobuf::RepeatedPtrField& matchers) const; + const bool failure_mode_allow_; const bool disable_clear_route_cache_; const std::chrono::milliseconds message_timeout_; @@ -201,6 +218,13 @@ class FilterConfig { const std::vector allowed_headers_; // Empty disallowed_header_ means disallow nothing, i.e, allow all. const std::vector disallowed_headers_; + + Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; + + const absl::flat_hash_map + request_expr_; + const absl::flat_hash_map + response_expr_; }; using FilterConfigSharedPtr = std::shared_ptr; @@ -300,7 +324,13 @@ class Filter : public Logger::Loggable, void sendImmediateResponse(const envoy::service::ext_proc::v3::ImmediateResponse& response); Http::FilterHeadersStatus onHeaders(ProcessorState& state, - Http::RequestOrResponseHeaderMap& headers, bool end_stream); + Http::RequestOrResponseHeaderMap& headers, bool end_stream, + absl::optional proto); + + const absl::optional evaluateAttributes( + Filters::Common::Expr::ActivationPtr activation, + const absl::flat_hash_map& + expr); // Return a pair of whether to terminate returning the current result. std::pair sendStreamChunk(ProcessorState& state); Http::FilterDataStatus onData(ProcessorState& state, Buffer::Instance& data, bool end_stream); diff --git a/source/extensions/filters/http/ext_proc/processor_state.h b/source/extensions/filters/http/ext_proc/processor_state.h index de4628864941..c6c50fdecbbb 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.h +++ b/source/extensions/filters/http/ext_proc/processor_state.h @@ -172,6 +172,8 @@ class ProcessorState : public Logger::Loggable { virtual envoy::service::ext_proc::v3::HttpTrailers* mutableTrailers(envoy::service::ext_proc::v3::ProcessingRequest& request) const PURE; + virtual StreamInfo::StreamInfo& streamInfo() PURE; + protected: void setBodyMode( envoy::extensions::filters::http::ext_proc::v3::ProcessingMode_BodySendMode body_mode); @@ -283,6 +285,8 @@ class DecodingProcessorState : public ProcessorState { void requestWatermark() override; void clearWatermark() override; + StreamInfo::StreamInfo& streamInfo() override { return decoder_callbacks_->streamInfo(); } + private: void setProcessingModeInternal( const envoy::extensions::filters::http::ext_proc::v3::ProcessingMode& mode); @@ -356,6 +360,8 @@ class EncodingProcessorState : public ProcessorState { void requestWatermark() override; void clearWatermark() override; + StreamInfo::StreamInfo& streamInfo() override { return encoder_callbacks_->streamInfo(); } + private: void setProcessingModeInternal( const envoy::extensions::filters::http::ext_proc::v3::ProcessingMode& mode); diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index a99fde2191ee..86489d81c259 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -30,6 +30,12 @@ envoy_extension_cc_test( name = "filter_test", size = "small", srcs = ["filter_test.cc"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), extension_names = ["envoy.filters.http.ext_proc"], deps = [ ":mock_server_lib", @@ -117,6 +123,12 @@ envoy_extension_cc_test( name = "ext_proc_integration_test", size = "large", # This test can take a while under tsan. srcs = ["ext_proc_integration_test.cc"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), extension_names = ["envoy.filters.http.ext_proc"], shard_count = 4, tags = [ @@ -139,6 +151,12 @@ envoy_extension_cc_test( name = "streaming_integration_test", size = "large", srcs = ["streaming_integration_test.cc"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), extension_names = ["envoy.filters.http.ext_proc"], tags = [ "cpu:3", diff --git a/test/extensions/filters/http/ext_proc/config_test.cc b/test/extensions/filters/http/ext_proc/config_test.cc index 914e454ebd5b..a5ce03772405 100644 --- a/test/extensions/filters/http/ext_proc/config_test.cc +++ b/test/extensions/filters/http/ext_proc/config_test.cc @@ -21,8 +21,12 @@ TEST(HttpExtProcConfigTest, CorrectConfig) { target_uri: ext_proc_server stat_prefix: google failure_mode_allow: true - request_attributes: 'Foo, Bar, Baz' - response_attributes: More + request_attributes: + - 'Foo' + - 'Bar' + - 'Baz' + response_attributes: + - 'More' processing_mode: request_header_mode: send response_header_mode: skip @@ -53,8 +57,12 @@ TEST(HttpExtProcConfigTest, CorrectConfigServerContext) { target_uri: ext_proc_server stat_prefix: google failure_mode_allow: true - request_attributes: 'Foo, Bar, Baz' - response_attributes: More + request_attributes: + - 'Foo' + - 'Bar' + - 'Baz' + response_attributes: + - 'More' processing_mode: request_header_mode: send response_header_mode: skip diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index a7e111a13ffd..f15cff4fab68 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -79,7 +79,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, scoped_runtime_.mergeValues( {{"envoy.reloadable_features.send_header_raw_value", header_raw_value_}}); scoped_runtime_.mergeValues( - {{"envoy_reloadable_features_immediate_response_use_filter_mutation_rule", + {{"envoy.reloadable_features.immediate_response_use_filter_mutation_rule", filter_mutation_rule_}}); config_helper_.addConfigModifier([this, config_option]( @@ -1570,6 +1570,7 @@ TEST_P(ExtProcIntegrationTest, GetAndRespondImmediately) { EXPECT_THAT(response->headers(), SingleHeaderValueIs("content-type", "application/json")); EXPECT_EQ("{\"reason\": \"Not authorized\"}", response->body()); } + TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithLogging) { ConfigOptions config_option = {}; config_option.add_logging_filter = true; @@ -1836,6 +1837,28 @@ TEST_P(ExtProcIntegrationTest, GetAndImmediateRespondMutationAllowEnvoy) { EXPECT_THAT(response->headers(), SingleHeaderValueIs("x-envoy-foo", "bar")); } +// Test the filter using an ext_proc server that responds to the request_header message +// by sending back an immediate_response message with x-envoy header mutation. +// The deprecated default checker allows x-envoy headers to be mutated and should +// override config-level checkers if the runtime guard is disabled. +TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithDefaultHeaderMutationChecker) { + // this is default, but setting explicitly for test clarity + filter_mutation_rule_ = "false"; + proto_config_.mutable_mutation_rules()->mutable_allow_envoy()->set_value(false); + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequest(absl::nullopt); + processAndRespondImmediately(*grpc_upstreams_[0], true, [](ImmediateResponse& immediate) { + immediate.mutable_status()->set_code(envoy::type::v3::StatusCode::Unauthorized); + auto* hdr = immediate.mutable_headers()->add_set_headers(); + // Adding x-envoy header is allowed since default overrides config. + hdr->mutable_header()->set_key("x-envoy-foo"); + hdr->mutable_header()->set_value("bar"); + }); + verifyDownstreamResponse(*response, 401); + EXPECT_FALSE(response->headers().get(LowerCaseString("x-envoy-foo")).empty()); +} + // Test the filter with request body buffering enabled using // an ext_proc server that responds to the request_body message // by modifying a header that should cause an error. @@ -3285,4 +3308,47 @@ TEST_P(ExtProcIntegrationTest, SendBodyBufferedPartialWithTrailer) { verifyDownstreamResponse(*response, 200); } +#if defined(USE_CEL_PARSER) +// Test the filter using the default configuration by connecting to +// an ext_proc server that responds to the request_headers message +// by requesting to modify the request headers. +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"); + + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequest(absl::nullopt); + processRequestHeadersMessage( + *grpc_upstreams_[0], true, [](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("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); + 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; + }); + + verifyDownstreamResponse(*response, 200); +} +#endif + } // namespace Envoy diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 7e68bd4f1ac5..ad79eaadbe5a 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -121,8 +121,10 @@ class HttpFilterTest : public testing::Test { if (!yaml.empty()) { TestUtility::loadFromYaml(yaml, proto_config); } - config_ = - std::make_shared(proto_config, 200ms, 10000, *stats_store_.rootScope(), ""); + config_ = std::make_shared( + proto_config, 200ms, 10000, *stats_store_.rootScope(), "", + std::make_shared( + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); filter_ = std::make_unique(config_, std::move(client_), proto_config.grpc_service()); filter_->setEncoderFilterCallbacks(encoder_callbacks_); EXPECT_CALL(encoder_callbacks_, encoderBufferLimit()).WillRepeatedly(Return(BufferSize)); diff --git a/test/extensions/filters/http/ext_proc/ordering_test.cc b/test/extensions/filters/http/ext_proc/ordering_test.cc index 744692ef22ff..65aba2e2bfc5 100644 --- a/test/extensions/filters/http/ext_proc/ordering_test.cc +++ b/test/extensions/filters/http/ext_proc/ordering_test.cc @@ -70,6 +70,10 @@ class OrderingTest : public testing::Test { if (cb) { (*cb)(proto_config); } + config_.reset(new FilterConfig( + proto_config, kMessageTimeout, kMaxMessageTimeoutMs, *stats_store_.rootScope(), "", + std::make_shared( + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr)))); config_ = std::make_shared(proto_config, kMessageTimeout, kMaxMessageTimeoutMs, *stats_store_.rootScope(), ""); filter_ = std::make_unique(config_, std::move(client_), proto_config.grpc_service()); diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index 984b828e0e02..d8aac45b885b 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -72,7 +72,9 @@ DEFINE_PROTO_FUZZER( try { config = std::make_shared( proto_config, std::chrono::milliseconds(200), 200, *stats_store.rootScope(), - "ext_proc_prefix"); + "ext_proc_prefix", + std::make_shared( + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); } catch (const EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException during ext_proc filter config validation: {}", e.what()); return; From 45ec23dede02804312f456b8c46a578e85771011 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 8 Nov 2023 14:42:14 -0500 Subject: [PATCH 02/35] fix ordering test constructor Signed-off-by: Jacob Bohanon --- test/extensions/filters/http/ext_proc/ordering_test.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/extensions/filters/http/ext_proc/ordering_test.cc b/test/extensions/filters/http/ext_proc/ordering_test.cc index 65aba2e2bfc5..79fbd7580042 100644 --- a/test/extensions/filters/http/ext_proc/ordering_test.cc +++ b/test/extensions/filters/http/ext_proc/ordering_test.cc @@ -70,12 +70,10 @@ class OrderingTest : public testing::Test { if (cb) { (*cb)(proto_config); } - config_.reset(new FilterConfig( + config_ = std::make_shared( proto_config, kMessageTimeout, kMaxMessageTimeoutMs, *stats_store_.rootScope(), "", std::make_shared( - Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr)))); - config_ = std::make_shared(proto_config, kMessageTimeout, kMaxMessageTimeoutMs, - *stats_store_.rootScope(), ""); + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); filter_ = std::make_unique(config_, std::move(client_), proto_config.grpc_service()); filter_->setEncoderFilterCallbacks(encoder_callbacks_); filter_->setDecoderFilterCallbacks(decoder_callbacks_); From 0a9f2282fecb965ae4b61a7a0e3175653bfda38c Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 16 Nov 2023 15:04:15 -0500 Subject: [PATCH 03/35] don't do CEL stuff if no attributes are configured Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc.cc | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 478cca05cc13..855406190202 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -308,9 +308,12 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { - auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), - &headers, nullptr, nullptr); - auto proto = evaluateAttributes(std::move(activation_ptr), config_->requestExpr()); + absl::optional proto; + if (!config_->requestExpr().empty()) { + auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), + &headers, nullptr, nullptr); + auto proto = evaluateAttributes(std::move(activation_ptr), config_->requestExpr()); + } status = onHeaders(decoding_state_, headers, end_stream, proto); ENVOY_LOG(trace, "onHeaders returning {}", static_cast(status)); @@ -590,9 +593,12 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s FilterHeadersStatus status = FilterHeadersStatus::Continue; if (!processing_complete_ && encoding_state_.sendHeaders()) { - auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), - nullptr, &headers, nullptr); - auto proto = evaluateAttributes(std::move(activation_ptr), config_->responseExpr()); + absl::optional proto; + if (!config_->responseExpr().empty()) { + auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), + nullptr, &headers, nullptr); + auto proto = evaluateAttributes(std::move(activation_ptr), config_->responseExpr()); + } status = onHeaders(encoding_state_, headers, end_stream, proto); ENVOY_LOG(trace, "onHeaders returns {}", static_cast(status)); From 9857119af5eac244698deff4c1c1c8769b647529 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 17 Nov 2023 12:18:16 -0500 Subject: [PATCH 04/35] fix variable shadowing Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/ext_proc.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 855406190202..8f921a7659e8 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -312,7 +312,7 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st if (!config_->requestExpr().empty()) { auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), &headers, nullptr, nullptr); - auto proto = evaluateAttributes(std::move(activation_ptr), config_->requestExpr()); + proto = evaluateAttributes(std::move(activation_ptr), config_->requestExpr()); } status = onHeaders(decoding_state_, headers, end_stream, proto); @@ -597,7 +597,7 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s if (!config_->responseExpr().empty()) { auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), nullptr, &headers, nullptr); - auto proto = evaluateAttributes(std::move(activation_ptr), config_->responseExpr()); + proto = evaluateAttributes(std::move(activation_ptr), config_->responseExpr()); } status = onHeaders(encoding_state_, headers, end_stream, proto); From a4454beba5b45f2014055597dc8e19916794b61c Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 28 Nov 2023 13:41:25 -0500 Subject: [PATCH 05/35] refactor matching utils out Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/BUILD | 23 +++ .../filters/http/ext_proc/ext_proc.cc | 79 +--------- .../filters/http/ext_proc/ext_proc.h | 51 ++---- .../filters/http/ext_proc/matching_utils.h | 145 ++++++++++++++++++ 4 files changed, 193 insertions(+), 105 deletions(-) create mode 100644 source/extensions/filters/http/ext_proc/matching_utils.h diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index ef2c32690f8e..b03d9e26aa57 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -27,6 +27,7 @@ envoy_cc_library( }), deps = [ ":client_interface", + ":matching_utils_lib", ":mutation_utils_lib", "//envoy/event:timer_interface", "//envoy/http:filter_interface", @@ -103,6 +104,28 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "matching_utils_lib", + hdrs = ["matching_utils.h"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), + deps = [ + "//source/common/protobuf", + "@com_google_cel_cpp//eval/public:cel_expr_builder_factory", + ] + select( + { + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "@com_google_cel_cpp//parser", + ], + }, + ), +) + envoy_cc_library( name = "client_lib", srcs = ["client_impl.cc"], diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 8f921a7659e8..a59e3dcced66 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -8,10 +8,6 @@ #include "absl/strings/str_format.h" -#if defined(USE_CEL_PARSER) -#include "parser/parser.h" -#endif - namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -117,27 +113,6 @@ ExtProcLoggingInfo::grpcCalls(envoy::config::core::v3::TrafficDirection traffic_ : encoding_processor_grpc_calls_; } -absl::flat_hash_map -FilterConfig::initExpressions(const Protobuf::RepeatedPtrField& matchers) const { - absl::flat_hash_map 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()); - } - expressions.emplace(matcher, Extensions::Filters::Common::Expr::createExpression( - builder_->builder(), parse_status.value().expr())); - } -#else - ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." - " Attempted to parse " + - std::to_string(matchers.size()) + " expressions"); -#endif - return expressions; -} - FilterConfigPerRoute::FilterConfigPerRoute(const ExtProcPerRoute& config) : disabled_(config.disabled()) { if (config.has_overrides()) { @@ -257,48 +232,6 @@ FilterHeadersStatus Filter::onHeaders(ProcessorState& state, return FilterHeadersStatus::StopIteration; } -const absl::optional Filter::evaluateAttributes( - Filters::Common::Expr::ActivationPtr activation, - const absl::flat_hash_map& - expr) { - absl::optional proto; - if (expr.size() > 0) { - proto.emplace(ProtobufWkt::Struct{}); - for (const auto& hash_entry : expr) { - ProtobufWkt::Arena arena; - const auto result = hash_entry.second.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; -} - FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_stream) { ENVOY_LOG(trace, "decodeHeaders: end_stream = {}", end_stream); mergePerRouteConfig(); @@ -309,12 +242,16 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { absl::optional proto; - if (!config_->requestExpr().empty()) { + std::cout << "checking if requestExpr empty" << std::endl; + if (config_->hasRequestExpr()) { + std::cout << "requestExpr not empty" << std::endl; auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), &headers, nullptr, nullptr); - proto = evaluateAttributes(std::move(activation_ptr), config_->requestExpr()); + std::cout << "evaluating attributes" << std::endl; + proto = config_->evaluateRequestAttributes(std::move(activation_ptr)); } + std::cout << "entering onHeaders" << std::endl; status = onHeaders(decoding_state_, headers, end_stream, proto); ENVOY_LOG(trace, "onHeaders returning {}", static_cast(status)); } else { @@ -594,10 +531,10 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s FilterHeadersStatus status = FilterHeadersStatus::Continue; if (!processing_complete_ && encoding_state_.sendHeaders()) { absl::optional proto; - if (!config_->responseExpr().empty()) { + if (config_->hasResponseExpr()) { auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), nullptr, &headers, nullptr); - proto = evaluateAttributes(std::move(activation_ptr), config_->responseExpr()); + proto = config_->evaluateResponseAttributes(std::move(activation_ptr)); } status = onHeaders(encoding_state_, headers, end_stream, proto); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 5500cee6c7af..e72dbc2b3117 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -25,6 +25,7 @@ #include "source/extensions/filters/common/mutation_rules/mutation_rules.h" #include "source/extensions/filters/http/common/pass_through_filter.h" #include "source/extensions/filters/http/ext_proc/client.h" +#include "source/extensions/filters/http/ext_proc/matching_utils.h" #include "source/extensions/filters/http/ext_proc/processor_state.h" namespace Envoy { @@ -137,10 +138,11 @@ class FilterConfig : public Logger::Loggable { filter_metadata_(config.filter_metadata()), allow_mode_override_(config.allow_mode_override()), disable_immediate_response_(config.disable_immediate_response()), - allowed_headers_(initHeaderMatchers(config.forward_rules().allowed_headers())), - disallowed_headers_(initHeaderMatchers(config.forward_rules().disallowed_headers())), - builder_(builder), request_expr_(initExpressions(config.request_attributes())), - response_expr_(initExpressions(config.response_attributes())) {} + allowed_headers_( + 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()) {} bool failureModeAllow() const { return failure_mode_allow_; } @@ -170,36 +172,26 @@ class FilterConfig : public Logger::Loggable { const Envoy::ProtobufWkt::Struct& filterMetadata() const { return filter_metadata_; } - const absl::flat_hash_map& - requestExpr() const { - return request_expr_; + const absl::optional + evaluateRequestAttributes(const Filters::Common::Expr::ActivationPtr& activation) const { + return expression_manager_.evaluateRequestAttributes(std::move(activation)); } - const absl::flat_hash_map& - responseExpr() const { - return response_expr_; + const absl::optional + 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(); } + private: ExtProcFilterStats generateStats(const std::string& prefix, const std::string& filter_stats_prefix, Stats::Scope& scope) { const std::string final_prefix = absl::StrCat(prefix, "ext_proc.", filter_stats_prefix); return {ALL_EXT_PROC_FILTER_STATS(POOL_COUNTER_PREFIX(scope, final_prefix))}; } - const std::vector - initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_list) { - std::vector header_matchers; - for (const auto& matcher : header_list.patterns()) { - header_matchers.push_back( - std::make_unique>( - matcher)); - } - return header_matchers; - } - - absl::flat_hash_map - initExpressions(const Protobuf::RepeatedPtrField& matchers) const; - const bool failure_mode_allow_; const bool disable_clear_route_cache_; const std::chrono::milliseconds message_timeout_; @@ -219,12 +211,7 @@ class FilterConfig : public Logger::Loggable { // Empty disallowed_header_ means disallow nothing, i.e, allow all. const std::vector disallowed_headers_; - Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; - - const absl::flat_hash_map - request_expr_; - const absl::flat_hash_map - response_expr_; + const ExpressionManager expression_manager_; }; using FilterConfigSharedPtr = std::shared_ptr; @@ -327,10 +314,6 @@ class Filter : public Logger::Loggable, Http::RequestOrResponseHeaderMap& headers, bool end_stream, absl::optional proto); - const absl::optional evaluateAttributes( - Filters::Common::Expr::ActivationPtr activation, - const absl::flat_hash_map& - expr); // Return a pair of whether to terminate returning the current result. std::pair sendStreamChunk(ProcessorState& state); Http::FilterDataStatus onData(ProcessorState& state, Buffer::Instance& data, bool end_stream); diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h new file mode 100644 index 000000000000..7bdc5a35703b --- /dev/null +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -0,0 +1,145 @@ +#pragma once + +#include "source/common/common/logger.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 { +namespace ExternalProcessing { + +class ExpressionManager : public Logger::Loggable { +public: + ExpressionManager(Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder, + const Protobuf::RepeatedPtrField& request_matchers, + const Protobuf::RepeatedPtrField& response_matchers) + : builder_(builder), request_expr_(initExpressions(request_matchers)), + response_expr_(initExpressions(response_matchers)){}; + + struct ExpressionPtrWithExpr { + ExpressionPtrWithExpr(const google::api::expr::v1alpha1::Expr& expr, + const Filters::Common::Expr::ExpressionPtr& expr_ptr) + : expression_ptr_(std::move(expr_ptr)), expr_(std::move(expr)){}; + const Filters::Common::Expr::ExpressionPtr& expression_ptr_; + const google::api::expr::v1alpha1::Expr& expr_; + }; + + bool hasRequestExpr() const { return !request_expr_.empty(); }; + + bool hasResponseExpr() const { return !response_expr_.empty(); }; + + const absl::optional + evaluateRequestAttributes(const Filters::Common::Expr::ActivationPtr& activation) const { + return evaluateAttributes(activation, request_expr_); + } + + const absl::optional + evaluateResponseAttributes(const Filters::Common::Expr::ActivationPtr& activation) const { + return evaluateAttributes(activation, response_expr_); + } + + const absl::optional + evaluateAttributes(const Filters::Common::Expr::ActivationPtr& activation, + const absl::flat_hash_map& expr) const { + std::cout << "entered evaluateAttributes" << std::endl; + absl::optional proto; + if (expr.size() > 0) { + proto.emplace(ProtobufWkt::Struct{}); + for (const auto& hash_entry : expr) { + std::cout << "evaluating attr" << std::endl; + std::cout << "evaluating " << hash_entry.first << std::endl; + ProtobufWkt::Arena arena; + std::cout << "expression_ptr_: "; + std::cout << hash_entry.second.expression_ptr_.get() << std::endl; + std::cout << "activation: "; + std::cout << activation.get() << std::endl; + const auto result = hash_entry.second.expression_ptr_.get()->Evaluate(*activation, &arena); + if (!result.ok()) { + std::cout << "!result.ok()" << std::endl; + // TODO: Stats? + continue; + } + std::cout << "result.ok()" << std::endl; + + 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; + } + +private: + absl::flat_hash_map + initExpressions(const Protobuf::RepeatedPtrField& matchers) const { + absl::flat_hash_map 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()); + } + auto parse_status_expr = parse_status.value().expr(); + auto expression = Extensions::Filters::Common::Expr::createExpression(builder_->builder(), + parse_status_expr); + const ExpressionPtrWithExpr expr(parse_status_expr, expression); + std::cout << "expression_ptr_: "; + std::cout << expr.expression_ptr_.get() << std::endl; + expressions.try_emplace(matcher, std::move(expr)); + } +#else + ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." + " Attempted to parse " + + std::to_string(matchers.size()) + " expressions"); +#endif + return expressions; + } + Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; + + const absl::flat_hash_map request_expr_; + const absl::flat_hash_map response_expr_; +}; + +class MatchingUtils : public Logger::Loggable { +public: + static const std::vector + initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_list) { + std::vector header_matchers; + for (const auto& matcher : header_list.patterns()) { + header_matchers.push_back( + std::make_unique>( + matcher)); + } + return header_matchers; + } +}; + +} // namespace ExternalProcessing +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy From 34b8fbc603a62740d63805f5a9b37403a238ebc4 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 28 Nov 2023 15:42:04 -0500 Subject: [PATCH 06/35] refactor matching utils out Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/BUILD | 1 + .../filters/http/ext_proc/ext_proc.cc | 4 -- .../filters/http/ext_proc/matching_utils.cc | 37 +++++++++++++++++ .../filters/http/ext_proc/matching_utils.h | 41 ++++--------------- 4 files changed, 45 insertions(+), 38 deletions(-) create mode 100644 source/extensions/filters/http/ext_proc/matching_utils.cc diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index b03d9e26aa57..16584555b900 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -106,6 +106,7 @@ envoy_cc_library( envoy_cc_library( name = "matching_utils_lib", + srcs = ["matching_utils.cc"], hdrs = ["matching_utils.h"], copts = select({ "//bazel:windows_x86_64": [], diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index a59e3dcced66..c8883a604ae9 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -242,16 +242,12 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { absl::optional proto; - std::cout << "checking if requestExpr empty" << std::endl; if (config_->hasRequestExpr()) { - std::cout << "requestExpr not empty" << std::endl; auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), &headers, nullptr, nullptr); - std::cout << "evaluating attributes" << std::endl; proto = config_->evaluateRequestAttributes(std::move(activation_ptr)); } - std::cout << "entering onHeaders" << std::endl; status = onHeaders(decoding_state_, headers, end_stream, proto); ENVOY_LOG(trace, "onHeaders returning {}", static_cast(status)); } else { diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc new file mode 100644 index 000000000000..03ccf465a1ce --- /dev/null +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -0,0 +1,37 @@ +#include "source/extensions/filters/http/ext_proc/matching_utils.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace ExternalProcessing { + +absl::flat_hash_map +ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers) const { + absl::flat_hash_map 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 auto parse_status_expr = parse_status.value().expr(); + const auto expression = + Extensions::Filters::Common::Expr::createExpression(builder_->builder(), parse_status_expr); + const ExpressionPtrWithExpr expr(parse_status_expr, std::move(expression)); + std::cout << "expression_ptr_: "; + std::cout << expr.expression_ptr_.get() << std::endl; + expressions.try_emplace(matcher, std::move(expr)); + } +#else + ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." + " Attempted to parse " + + std::to_string(matchers.size()) + " expressions"); +#endif + return expressions; +} + +} // namespace ExternalProcessing +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index 7bdc5a35703b..39f79fe04d3b 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -21,12 +21,14 @@ class ExpressionManager : public Logger::Loggable { : builder_(builder), request_expr_(initExpressions(request_matchers)), response_expr_(initExpressions(response_matchers)){}; + // 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, - const Filters::Common::Expr::ExpressionPtr& expr_ptr) - : expression_ptr_(std::move(expr_ptr)), expr_(std::move(expr)){}; - const Filters::Common::Expr::ExpressionPtr& expression_ptr_; + const Filters::Common::Expr::ExpressionPtr&& expr_ptr) + : expr_(std::move(expr)), expression_ptr_(std::move(expr_ptr)){}; const google::api::expr::v1alpha1::Expr& expr_; + const Filters::Common::Expr::ExpressionPtr& expression_ptr_; }; bool hasRequestExpr() const { return !request_expr_.empty(); }; @@ -46,25 +48,18 @@ class ExpressionManager : public Logger::Loggable { const absl::optional evaluateAttributes(const Filters::Common::Expr::ActivationPtr& activation, const absl::flat_hash_map& expr) const { - std::cout << "entered evaluateAttributes" << std::endl; absl::optional proto; if (expr.size() > 0) { proto.emplace(ProtobufWkt::Struct{}); for (const auto& hash_entry : expr) { - std::cout << "evaluating attr" << std::endl; - std::cout << "evaluating " << hash_entry.first << std::endl; ProtobufWkt::Arena arena; std::cout << "expression_ptr_: "; std::cout << hash_entry.second.expression_ptr_.get() << std::endl; - std::cout << "activation: "; - std::cout << activation.get() << std::endl; const auto result = hash_entry.second.expression_ptr_.get()->Evaluate(*activation, &arena); if (!result.ok()) { - std::cout << "!result.ok()" << std::endl; // TODO: Stats? continue; } - std::cout << "result.ok()" << std::endl; if (result.value().IsError()) { ENVOY_LOG(trace, "error parsing cel expression {}", hash_entry.first); @@ -95,30 +90,8 @@ class ExpressionManager : public Logger::Loggable { private: absl::flat_hash_map - initExpressions(const Protobuf::RepeatedPtrField& matchers) const { - absl::flat_hash_map 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()); - } - auto parse_status_expr = parse_status.value().expr(); - auto expression = Extensions::Filters::Common::Expr::createExpression(builder_->builder(), - parse_status_expr); - const ExpressionPtrWithExpr expr(parse_status_expr, expression); - std::cout << "expression_ptr_: "; - std::cout << expr.expression_ptr_.get() << std::endl; - expressions.try_emplace(matcher, std::move(expr)); - } -#else - ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." - " Attempted to parse " + - std::to_string(matchers.size()) + " expressions"); -#endif - return expressions; - } + initExpressions(const Protobuf::RepeatedPtrField& matchers) const; + Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; const absl::flat_hash_map request_expr_; From 3631683d892d006c0783629c7f77d6f1fe775ffc Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 29 Nov 2023 10:58:21 -0500 Subject: [PATCH 07/35] fix pointer/references Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/BUILD | 2 + .../filters/http/ext_proc/config.cc | 14 ++-- .../filters/http/ext_proc/ext_proc.cc | 8 ++- .../filters/http/ext_proc/ext_proc.h | 8 ++- .../filters/http/ext_proc/matching_utils.cc | 12 ++-- .../filters/http/ext_proc/matching_utils.h | 16 +++-- .../filters/http/ext_proc/processor_state.h | 6 ++ test/extensions/filters/http/ext_proc/BUILD | 18 +++++ .../ext_proc/ext_proc_integration_test.cc | 65 +++++++++++++++++++ .../unit_test_fuzz/ext_proc_unit_test_fuzz.cc | 4 +- 10 files changed, 134 insertions(+), 19 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index f9a3c1a0bbff..36315067cad5 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -92,7 +92,9 @@ envoy_cc_library( ], }), deps = [ + "//envoy/http:header_map_interface", "//source/common/protobuf", + "//source/extensions/filters/common/expr:evaluator_lib", "@com_google_cel_cpp//eval/public:cel_expr_builder_factory", ] + select( { diff --git a/source/extensions/filters/http/ext_proc/config.cc b/source/extensions/filters/http/ext_proc/config.cc index c38dae46b50f..02c494e04863 100644 --- a/source/extensions/filters/http/ext_proc/config.cc +++ b/source/extensions/filters/http/ext_proc/config.cc @@ -1,5 +1,6 @@ #include "source/extensions/filters/http/ext_proc/config.h" +#include "source/extensions/filters/common/expr/evaluator.h" #include "source/extensions/filters/http/ext_proc/client_impl.h" #include "source/extensions/filters/http/ext_proc/ext_proc.h" @@ -15,9 +16,9 @@ Http::FilterFactoryCb ExternalProcessingFilterConfig::createFilterFactoryFromPro PROTOBUF_GET_MS_OR_DEFAULT(proto_config, message_timeout, DefaultMessageTimeoutMs); const uint32_t max_message_timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs); - const auto filter_config = - std::make_shared(proto_config, std::chrono::milliseconds(message_timeout_ms), - max_message_timeout_ms, context.scope(), stats_prefix); + const auto filter_config = std::make_shared( + proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, + context.scope(), stats_prefix, Envoy::Extensions::Filters::Common::Expr::getBuilder(context)); return [filter_config, grpc_service = proto_config.grpc_service(), &context](Http::FilterChainFactoryCallbacks& callbacks) { @@ -45,9 +46,10 @@ ExternalProcessingFilterConfig::createFilterFactoryFromProtoWithServerContextTyp PROTOBUF_GET_MS_OR_DEFAULT(proto_config, message_timeout, DefaultMessageTimeoutMs); const uint32_t max_message_timeout_ms = PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs); - const auto filter_config = - std::make_shared(proto_config, std::chrono::milliseconds(message_timeout_ms), - max_message_timeout_ms, server_context.scope(), stats_prefix); + const auto filter_config = std::make_shared( + proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, + server_context.scope(), stats_prefix, + Envoy::Extensions::Filters::Common::Expr::getBuilder(server_context)); return [filter_config, grpc_service = proto_config.grpc_service(), &server_context](Http::FilterChainFactoryCallbacks& callbacks) { diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 4d0203345aaf..6f9ea2dcd3b1 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -201,7 +201,8 @@ void Filter::onDestroy() { } FilterHeadersStatus Filter::onHeaders(ProcessorState& state, - Http::RequestOrResponseHeaderMap& headers, bool end_stream) { + Http::RequestOrResponseHeaderMap& headers, bool end_stream, + absl::optional proto) { switch (openStream()) { case StreamOpenState::Error: return FilterHeadersStatus::StopIteration; @@ -219,6 +220,9 @@ FilterHeadersStatus Filter::onHeaders(ProcessorState& state, MutationUtils::headersToProto(headers, config_->allowedHeaders(), config_->disallowedHeaders(), *headers_req->mutable_headers()); headers_req->set_end_of_stream(end_stream); + if (proto.has_value()) { + (*headers_req->mutable_attributes())[FilterName] = proto.value(); + } state.onStartProcessorCall(std::bind(&Filter::onMessageTimeout, this), config_->messageTimeout(), ProcessorState::CallbackState::HeadersCallback); ENVOY_LOG(debug, "Sending headers message"); @@ -241,6 +245,8 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st if (config_->hasRequestExpr()) { auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), &headers, nullptr, nullptr); + std::cout << "expression_ptr_ in decodeHeaders: "; + std::cout << config_->getExprPtr("request.path").expression_ptr_.get() << std::endl; proto = config_->evaluateRequestAttributes(std::move(activation_ptr)); } diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 9be00f944bfb..000396b545dc 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -127,7 +127,8 @@ class FilterConfig { FilterConfig(const envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor& config, const std::chrono::milliseconds message_timeout, const uint32_t max_message_timeout_ms, Stats::Scope& scope, - const std::string& stats_prefix) + const std::string& stats_prefix, + Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder) : failure_mode_allow_(config.failure_mode_allow()), disable_clear_route_cache_(config.disable_clear_route_cache()), message_timeout_(message_timeout), max_message_timeout_ms_(max_message_timeout_ms), @@ -184,6 +185,11 @@ class FilterConfig { bool hasResponseExpr() const { return expression_manager_.hasResponseExpr(); } + // TODO: delete + const ExpressionManager::ExpressionPtrWithExpr& getExprPtr(std::string matcher) { + return expression_manager_.getExprPtr(matcher); + } + private: ExtProcFilterStats generateStats(const std::string& prefix, const std::string& filter_stats_prefix, Stats::Scope& scope) { diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 03ccf465a1ce..8f5ca2d0fd0e 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -5,9 +5,9 @@ namespace Extensions { namespace HttpFilters { namespace ExternalProcessing { -absl::flat_hash_map +absl::flat_hash_map ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers) const { - absl::flat_hash_map expressions; + absl::flat_hash_map expressions; #if defined(USE_CEL_PARSER) for (const auto& matcher : matchers) { auto parse_status = google::api::expr::parser::Parse(matcher); @@ -16,12 +16,14 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField parse_status.status().ToString()); } const auto parse_status_expr = parse_status.value().expr(); - const auto expression = + auto expression = Extensions::Filters::Common::Expr::createExpression(builder_->builder(), parse_status_expr); - const ExpressionPtrWithExpr expr(parse_status_expr, std::move(expression)); - std::cout << "expression_ptr_: "; + ExpressionPtrWithExpr expr(parse_status_expr, expression.get()); + std::cout << "expression_ptr_ after construction: "; std::cout << expr.expression_ptr_.get() << std::endl; expressions.try_emplace(matcher, std::move(expr)); + std::cout << "expression_ptr_ after placing in container: "; + std::cout << expressions.at(matcher).expression_ptr_.get() << std::endl; } #else ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index 39f79fe04d3b..32dd1466b280 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -1,6 +1,7 @@ #pragma once #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" @@ -25,10 +26,10 @@ class ExpressionManager : public Logger::Loggable { // parsed from it. struct ExpressionPtrWithExpr { ExpressionPtrWithExpr(const google::api::expr::v1alpha1::Expr& expr, - const Filters::Common::Expr::ExpressionPtr&& expr_ptr) - : expr_(std::move(expr)), expression_ptr_(std::move(expr_ptr)){}; + Filters::Common::Expr::Expression* expr_ptr) + : expr_(expr), expression_ptr_(std::move(expr_ptr)){}; const google::api::expr::v1alpha1::Expr& expr_; - const Filters::Common::Expr::ExpressionPtr& expression_ptr_; + Filters::Common::Expr::ExpressionPtr expression_ptr_; }; bool hasRequestExpr() const { return !request_expr_.empty(); }; @@ -49,11 +50,11 @@ class ExpressionManager : public Logger::Loggable { evaluateAttributes(const Filters::Common::Expr::ActivationPtr& activation, const absl::flat_hash_map& expr) const { absl::optional proto; - if (expr.size() > 0) { + if (!expr.empty()) { proto.emplace(ProtobufWkt::Struct{}); for (const auto& hash_entry : expr) { ProtobufWkt::Arena arena; - std::cout << "expression_ptr_: "; + std::cout << "expression_ptr_ in evaluateAttributes: "; std::cout << hash_entry.second.expression_ptr_.get() << std::endl; const auto result = hash_entry.second.expression_ptr_.get()->Evaluate(*activation, &arena); if (!result.ok()) { @@ -88,6 +89,11 @@ class ExpressionManager : public Logger::Loggable { return proto; } + // TODO: delete + const ExpressionPtrWithExpr& getExprPtr(std::string matcher) const { + return request_expr_.at(matcher); + } + private: absl::flat_hash_map initExpressions(const Protobuf::RepeatedPtrField& matchers) const; diff --git a/source/extensions/filters/http/ext_proc/processor_state.h b/source/extensions/filters/http/ext_proc/processor_state.h index de4628864941..c6c50fdecbbb 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.h +++ b/source/extensions/filters/http/ext_proc/processor_state.h @@ -172,6 +172,8 @@ class ProcessorState : public Logger::Loggable { virtual envoy::service::ext_proc::v3::HttpTrailers* mutableTrailers(envoy::service::ext_proc::v3::ProcessingRequest& request) const PURE; + virtual StreamInfo::StreamInfo& streamInfo() PURE; + protected: void setBodyMode( envoy::extensions::filters::http::ext_proc::v3::ProcessingMode_BodySendMode body_mode); @@ -283,6 +285,8 @@ class DecodingProcessorState : public ProcessorState { void requestWatermark() override; void clearWatermark() override; + StreamInfo::StreamInfo& streamInfo() override { return decoder_callbacks_->streamInfo(); } + private: void setProcessingModeInternal( const envoy::extensions::filters::http::ext_proc::v3::ProcessingMode& mode); @@ -356,6 +360,8 @@ class EncodingProcessorState : public ProcessorState { void requestWatermark() override; void clearWatermark() override; + StreamInfo::StreamInfo& streamInfo() override { return encoder_callbacks_->streamInfo(); } + private: void setProcessingModeInternal( const envoy::extensions::filters::http::ext_proc::v3::ProcessingMode& mode); diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index a99fde2191ee..86489d81c259 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -30,6 +30,12 @@ envoy_extension_cc_test( name = "filter_test", size = "small", srcs = ["filter_test.cc"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), extension_names = ["envoy.filters.http.ext_proc"], deps = [ ":mock_server_lib", @@ -117,6 +123,12 @@ envoy_extension_cc_test( name = "ext_proc_integration_test", size = "large", # This test can take a while under tsan. srcs = ["ext_proc_integration_test.cc"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), extension_names = ["envoy.filters.http.ext_proc"], shard_count = 4, tags = [ @@ -139,6 +151,12 @@ envoy_extension_cc_test( name = "streaming_integration_test", size = "large", srcs = ["streaming_integration_test.cc"], + copts = select({ + "//bazel:windows_x86_64": [], + "//conditions:default": [ + "-DUSE_CEL_PARSER", + ], + }), extension_names = ["envoy.filters.http.ext_proc"], tags = [ "cpu:3", diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index a7e111a13ffd..d2b06dbb41c5 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -1836,6 +1836,28 @@ TEST_P(ExtProcIntegrationTest, GetAndImmediateRespondMutationAllowEnvoy) { EXPECT_THAT(response->headers(), SingleHeaderValueIs("x-envoy-foo", "bar")); } +// Test the filter using an ext_proc server that responds to the request_header message +// by sending back an immediate_response message with x-envoy header mutation. +// The deprecated default checker allows x-envoy headers to be mutated and should +// override config-level checkers if the runtime guard is disabled. +TEST_P(ExtProcIntegrationTest, GetAndRespondImmediatelyWithDefaultHeaderMutationChecker) { + // this is default, but setting explicitly for test clarity + filter_mutation_rule_ = "false"; + proto_config_.mutable_mutation_rules()->mutable_allow_envoy()->set_value(false); + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequest(absl::nullopt); + processAndRespondImmediately(*grpc_upstreams_[0], true, [](ImmediateResponse& immediate) { + immediate.mutable_status()->set_code(envoy::type::v3::StatusCode::Unauthorized); + auto* hdr = immediate.mutable_headers()->add_set_headers(); + // Adding x-envoy header is allowed since default overrides config. + hdr->mutable_header()->set_key("x-envoy-foo"); + hdr->mutable_header()->set_value("bar"); + }); + verifyDownstreamResponse(*response, 401); + EXPECT_FALSE(response->headers().get(LowerCaseString("x-envoy-foo")).empty()); +} + // Test the filter with request body buffering enabled using // an ext_proc server that responds to the request_body message // by modifying a header that should cause an error. @@ -3285,4 +3307,47 @@ TEST_P(ExtProcIntegrationTest, SendBodyBufferedPartialWithTrailer) { verifyDownstreamResponse(*response, 200); } +#if defined(USE_CEL_PARSER) +// Test the filter using the default configuration by connecting to +// an ext_proc server that responds to the request_headers message +// by requesting to modify the request headers. +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"); + + initializeConfig(); + HttpIntegrationTest::initialize(); + auto response = sendDownstreamRequest(absl::nullopt); + processRequestHeadersMessage( + *grpc_upstreams_[0], true, [](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("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); + 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; + }); + + verifyDownstreamResponse(*response, 200); +} +#endif + } // namespace Envoy diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index 984b828e0e02..d8aac45b885b 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -72,7 +72,9 @@ DEFINE_PROTO_FUZZER( try { config = std::make_shared( proto_config, std::chrono::milliseconds(200), 200, *stats_store.rootScope(), - "ext_proc_prefix"); + "ext_proc_prefix", + std::make_shared( + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); } catch (const EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException during ext_proc filter config validation: {}", e.what()); return; From 47c2b4601da77343387e0647631fda22e888083f Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 30 Nov 2023 09:08:50 -0500 Subject: [PATCH 08/35] where are my CEL objects going Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc.cc | 10 +++-- .../filters/http/ext_proc/ext_proc.h | 16 ++++++-- .../filters/http/ext_proc/matching_utils.cc | 26 ++++++++----- .../filters/http/ext_proc/matching_utils.h | 38 +++++++++++++------ .../ext_proc/ext_proc_integration_test.cc | 35 +++++++++-------- 5 files changed, 82 insertions(+), 43 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 6f9ea2dcd3b1..17d2ca9c25fb 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -1,5 +1,7 @@ #include "source/extensions/filters/http/ext_proc/ext_proc.h" +#include + #include "envoy/config/common/mutation_rules/v3/mutation_rules.pb.h" #include "source/common/http/utility.h" @@ -242,12 +244,12 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { absl::optional proto; - if (config_->hasRequestExpr()) { + if (config().hasRequestExpr()) { auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), &headers, nullptr, nullptr); - std::cout << "expression_ptr_ in decodeHeaders: "; - std::cout << config_->getExprPtr("request.path").expression_ptr_.get() << std::endl; - proto = config_->evaluateRequestAttributes(std::move(activation_ptr)); + ExpressionManager::printExprPtrAndType(config().getExprPtr("request.path"), + "in decodeHeaders"); + proto = config().evaluateRequestAttributes(std::move(activation_ptr)); } status = onHeaders(decoding_state_, headers, end_stream, proto); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 000396b545dc..e018f6edcaa4 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -141,7 +141,15 @@ 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()) {} + 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); + } bool failureModeAllow() const { return failure_mode_allow_; } @@ -186,7 +194,7 @@ class FilterConfig { bool hasResponseExpr() const { return expression_manager_.hasResponseExpr(); } // TODO: delete - const ExpressionManager::ExpressionPtrWithExpr& getExprPtr(std::string matcher) { + const ExpressionManager::ExpressionPtrWithExpr& getExprPtr(std::string matcher) const { return expression_manager_.getExprPtr(matcher); } @@ -263,7 +271,9 @@ class Filter : public Logger::Loggable, : 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()) {} + encoding_state_(*this, config->processingMode()) { + config_->printExprPtrAndType("in Filter constructor"); + } const FilterConfig& config() const { return *config_; } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 8f5ca2d0fd0e..2caf156456dc 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -1,13 +1,17 @@ #include "source/extensions/filters/http/ext_proc/matching_utils.h" +#include +#include + namespace Envoy { namespace Extensions { namespace HttpFilters { namespace ExternalProcessing { -absl::flat_hash_map +absl::flat_hash_map> ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers) const { - absl::flat_hash_map expressions; + absl::flat_hash_map> + expressions; #if defined(USE_CEL_PARSER) for (const auto& matcher : matchers) { auto parse_status = google::api::expr::parser::Parse(matcher); @@ -15,15 +19,19 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField throw EnvoyException("Unable to parse descriptor expression: " + parse_status.status().ToString()); } - const auto parse_status_expr = parse_status.value().expr(); - auto expression = + 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); - ExpressionPtrWithExpr expr(parse_status_expr, expression.get()); - std::cout << "expression_ptr_ after construction: "; - std::cout << expr.expression_ptr_.get() << std::endl; + std::unique_ptr expr = + std::make_unique(parse_status_expr, std::move(expression.get())); + if (matcher == "request.path") { + ExpressionManager::printExprPtrAndType(*expr, "after construction"); + } expressions.try_emplace(matcher, std::move(expr)); - std::cout << "expression_ptr_ after placing in container: "; - std::cout << expressions.at(matcher).expression_ptr_.get() << std::endl; + if (matcher == "request.path") { + ExpressionManager::printExprPtrAndType(*expressions.at(matcher), + "after placing in container"); + } } #else ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index 32dd1466b280..7ccd4d7d5b24 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "source/common/common/logger.h" #include "source/common/common/matchers.h" #include "source/common/protobuf/protobuf.h" @@ -20,7 +22,9 @@ class ExpressionManager : public Logger::Loggable { const Protobuf::RepeatedPtrField& request_matchers, const Protobuf::RepeatedPtrField& response_matchers) : builder_(builder), request_expr_(initExpressions(request_matchers)), - response_expr_(initExpressions(response_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. @@ -28,10 +32,23 @@ class ExpressionManager : public Logger::Loggable { 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; + } + bool hasRequestExpr() const { return !request_expr_.empty(); }; bool hasResponseExpr() const { return !response_expr_.empty(); }; @@ -46,17 +63,16 @@ class ExpressionManager : public Logger::Loggable { return evaluateAttributes(activation, response_expr_); } - const absl::optional - evaluateAttributes(const Filters::Common::Expr::ActivationPtr& activation, - const absl::flat_hash_map& expr) const { + static const absl::optional evaluateAttributes( + const Filters::Common::Expr::ActivationPtr& activation, + const absl::flat_hash_map>& expr) { absl::optional proto; if (!expr.empty()) { proto.emplace(ProtobufWkt::Struct{}); for (const auto& hash_entry : expr) { ProtobufWkt::Arena arena; - std::cout << "expression_ptr_ in evaluateAttributes: "; - std::cout << hash_entry.second.expression_ptr_.get() << std::endl; - const auto result = hash_entry.second.expression_ptr_.get()->Evaluate(*activation, &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; @@ -91,17 +107,17 @@ class ExpressionManager : public Logger::Loggable { // TODO: delete const ExpressionPtrWithExpr& getExprPtr(std::string matcher) const { - return request_expr_.at(matcher); + return *request_expr_.at(matcher); } private: - absl::flat_hash_map + absl::flat_hash_map> initExpressions(const Protobuf::RepeatedPtrField& matchers) const; Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; - const absl::flat_hash_map request_expr_; - const absl::flat_hash_map response_expr_; + const absl::flat_hash_map> request_expr_; + const absl::flat_hash_map> response_expr_; }; class MatchingUtils : public Logger::Loggable { diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index d2b06dbb41c5..28d1ea705460 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/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,22 +3329,25 @@ 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); } From 86ae01fa40043a122d48f1ac68d3f1e040b153fd Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 30 Nov 2023 11:16:49 -0500 Subject: [PATCH 09/35] fix lifetime issue and clean up Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc.cc | 12 +-- .../filters/http/ext_proc/ext_proc.h | 33 +------ .../filters/http/ext_proc/matching_utils.cc | 73 +++++++++++---- .../filters/http/ext_proc/matching_utils.h | 89 ++----------------- .../ext_proc/ext_proc_integration_test.cc | 35 ++++---- 5 files changed, 87 insertions(+), 155 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 17d2ca9c25fb..8b6dd2e743ec 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -1,7 +1,5 @@ #include "source/extensions/filters/http/ext_proc/ext_proc.h" -#include - #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 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 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); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index e018f6edcaa4..c00d659fee37 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/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 - evaluateRequestAttributes(const Filters::Common::Expr::ActivationPtr& activation) const { - return expression_manager_.evaluateRequestAttributes(std::move(activation)); - } - - const absl::optional - 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, : 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_; } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 2caf156456dc..46801055105f 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -1,17 +1,19 @@ #include "source/extensions/filters/http/ext_proc/matching_utils.h" #include -#include + +#if defined(USE_CEL_PARSER) +#include "parser/parser.h" +#endif namespace Envoy { namespace Extensions { namespace HttpFilters { namespace ExternalProcessing { -absl::flat_hash_map> -ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers) const { - absl::flat_hash_map> - expressions; +absl::flat_hash_map +ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers) { + absl::flat_hash_map expressions; #if defined(USE_CEL_PARSER) for (const auto& matcher : matchers) { auto parse_status = google::api::expr::parser::Parse(matcher); @@ -19,19 +21,13 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField 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 expr = - std::make_unique(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 return expressions; } +const absl::optional ExpressionManager::evaluateAttributes( + const Filters::Common::Expr::ActivationPtr& activation, + const absl::flat_hash_map& expr) { + absl::optional 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 diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index 7ccd4d7d5b24..305d40c84535 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -1,16 +1,10 @@ #pragma once -#include - #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 { const Protobuf::RepeatedPtrField& request_matchers, const Protobuf::RepeatedPtrField& 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 { static const absl::optional evaluateAttributes( const Filters::Common::Expr::ActivationPtr& activation, - const absl::flat_hash_map>& expr) { - absl::optional 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& expr); private: - absl::flat_hash_map> - initExpressions(const Protobuf::RepeatedPtrField& matchers) const; + // This list is required to maintain the lifetimes of expr objects on which compiled expressions + // depend + std::list expr_list_; + absl::flat_hash_map + initExpressions(const Protobuf::RepeatedPtrField& matchers); Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; - const absl::flat_hash_map> request_expr_; - const absl::flat_hash_map> response_expr_; + const absl::flat_hash_map request_expr_; + const absl::flat_hash_map response_expr_; }; class MatchingUtils : public Logger::Loggable { diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 28d1ea705460..d2b06dbb41c5 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/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); } From 0d8f5d4ed6d210d5764080b60f78dc657edc654d Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 30 Nov 2023 11:26:58 -0500 Subject: [PATCH 10/35] fix runtime feature format Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index d2b06dbb41c5..7c20bfc968eb 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -79,7 +79,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, scoped_runtime_.mergeValues( {{"envoy.reloadable_features.send_header_raw_value", header_raw_value_}}); scoped_runtime_.mergeValues( - {{"envoy_reloadable_features_immediate_response_use_filter_mutation_rule", + {{"envoy.reloadable_features.immediate_response_use_filter_mutation_rule", filter_mutation_rule_}}); config_helper_.addConfigModifier([this, config_option]( From dc692e115ca08e243e20aafb64d0b5eb8b3b4199 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 30 Nov 2023 14:22:03 -0500 Subject: [PATCH 11/35] declare iter as reference Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/matching_utils.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 46801055105f..a806e83a9481 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -22,7 +22,7 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField parse_status.status().ToString()); } - auto iter = expr_list_.emplace_back(parse_status.value()); + auto& iter = expr_list_.emplace_back(parse_status.value()); Filters::Common::Expr::ExpressionPtr expression = Extensions::Filters::Common::Expr::createExpression(builder_->builder(), iter.expr()); From 8d34e78c4ed0c5a21f2ec05bf139edc9e30481b9 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 30 Nov 2023 16:11:54 -0500 Subject: [PATCH 12/35] fix tests and clean up Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/v3/ext_proc.proto | 3 --- changelogs/current.yaml | 7 +++++++ .../filters/http/ext_proc/config_test.cc | 16 ++++++++++++---- .../filters/http/ext_proc/filter_test.cc | 6 ++++-- .../filters/http/ext_proc/ordering_test.cc | 6 ++++-- .../unit_test_fuzz/ext_proc_unit_test_fuzz.cc | 3 +-- 6 files changed, 28 insertions(+), 13 deletions(-) diff --git a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto index c9500486a3a5..9874208aabdf 100644 --- a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto +++ b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto @@ -28,7 +28,6 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // **Current Implementation Status:** // All options and processing modes are implemented except for the following: // -// * Request and response attributes are not sent and not processed. // * Dynamic metadata in responses from the external processor is ignored. // * "async mode" is not implemented. @@ -125,7 +124,6 @@ message ExternalProcessor { // for a reply. bool async_mode = 4; - // [#not-implemented-hide:] // Envoy provides a number of :ref:`attributes ` // for expressive policies. Each attribute name provided in this field will be // matched against that list and populated in the request_headers message. @@ -133,7 +131,6 @@ message ExternalProcessor { // for the list of supported attributes and their types. repeated string request_attributes = 5; - // [#not-implemented-hide:] // Envoy provides a number of :ref:`attributes ` // for expressive policies. Each attribute name provided in this field will be // matched against that list and populated in the response_headers message. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 2075df8a3393..e4ab0614915e 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -216,5 +216,12 @@ new_features: ` to allow recording an access log entry periodically for the UDP session, and allow recording an access log entry on the connection tunnel created successfully to upstream when UDP tunneling is configured. +- area: ext_proc + change: | + implemented + :ref:`request_attributes ` + and + :ref:`response_attributes ` + config APIs to enable sending and receiving attributes from/to the external processing server. deprecated: diff --git a/test/extensions/filters/http/ext_proc/config_test.cc b/test/extensions/filters/http/ext_proc/config_test.cc index 6cfa1d9196a9..47e36558a260 100644 --- a/test/extensions/filters/http/ext_proc/config_test.cc +++ b/test/extensions/filters/http/ext_proc/config_test.cc @@ -21,8 +21,12 @@ TEST(HttpExtProcConfigTest, CorrectConfig) { target_uri: ext_proc_server stat_prefix: google failure_mode_allow: true - request_attributes: 'Foo, Bar, Baz' - response_attributes: More + request_attributes: + - 'Foo' + - 'Bar' + - 'Baz' + response_attributes: + - 'More' processing_mode: request_header_mode: send response_header_mode: skip @@ -54,8 +58,12 @@ TEST(HttpExtProcConfigTest, CorrectConfigServerContext) { target_uri: ext_proc_server stat_prefix: google failure_mode_allow: true - request_attributes: 'Foo, Bar, Baz' - response_attributes: More + request_attributes: + - 'Foo' + - 'Bar' + - 'Baz' + response_attributes: + - 'More' processing_mode: request_header_mode: send response_header_mode: skip diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 7e68bd4f1ac5..ad79eaadbe5a 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -121,8 +121,10 @@ class HttpFilterTest : public testing::Test { if (!yaml.empty()) { TestUtility::loadFromYaml(yaml, proto_config); } - config_ = - std::make_shared(proto_config, 200ms, 10000, *stats_store_.rootScope(), ""); + config_ = std::make_shared( + proto_config, 200ms, 10000, *stats_store_.rootScope(), "", + std::make_shared( + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); filter_ = std::make_unique(config_, std::move(client_), proto_config.grpc_service()); filter_->setEncoderFilterCallbacks(encoder_callbacks_); EXPECT_CALL(encoder_callbacks_, encoderBufferLimit()).WillRepeatedly(Return(BufferSize)); diff --git a/test/extensions/filters/http/ext_proc/ordering_test.cc b/test/extensions/filters/http/ext_proc/ordering_test.cc index 744692ef22ff..79fbd7580042 100644 --- a/test/extensions/filters/http/ext_proc/ordering_test.cc +++ b/test/extensions/filters/http/ext_proc/ordering_test.cc @@ -70,8 +70,10 @@ class OrderingTest : public testing::Test { if (cb) { (*cb)(proto_config); } - config_ = std::make_shared(proto_config, kMessageTimeout, kMaxMessageTimeoutMs, - *stats_store_.rootScope(), ""); + config_ = std::make_shared( + proto_config, kMessageTimeout, kMaxMessageTimeoutMs, *stats_store_.rootScope(), "", + std::make_shared( + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); filter_ = std::make_unique(config_, std::move(client_), proto_config.grpc_service()); filter_->setEncoderFilterCallbacks(encoder_callbacks_); filter_->setDecoderFilterCallbacks(decoder_callbacks_); diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index d8aac45b885b..60bc34c4a1c9 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -71,8 +71,7 @@ DEFINE_PROTO_FUZZER( try { config = std::make_shared( - proto_config, std::chrono::milliseconds(200), 200, *stats_store.rootScope(), - "ext_proc_prefix", + proto_config, std::chrono::milliseconds(200), 200, *stats_store.rootScope(), "", std::make_shared( Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); } catch (const EnvoyException& e) { From fd598b92889c0e848510f38d14d1906eb3bcd532 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 1 Dec 2023 09:18:42 -0500 Subject: [PATCH 13/35] skip fuzzing request and response attributes on ASAN_FUZZER due to unrelated issue with ASAN in upstream CEL parsing Signed-off-by: Jacob Bohanon --- .bazelrc | 1 + .../unit_test_fuzz/ext_proc_unit_test_fuzz.cc | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/.bazelrc b/.bazelrc index a5b1ab886dba..f9e24de2e5b8 100644 --- a/.bazelrc +++ b/.bazelrc @@ -417,6 +417,7 @@ build:asan-fuzzer --config=clang-asan build:asan-fuzzer --copt=-fno-omit-frame-pointer # Remove UBSAN halt_on_error to avoid crashing on protobuf errors. build:asan-fuzzer --test_env=UBSAN_OPTIONS=print_stacktrace=1 +build:asan-fuzzer --copt=-DASAN_FUZZER build:oss-fuzz --config=fuzzing build:oss-fuzz --define=FUZZING_ENGINE=oss-fuzz diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index 60bc34c4a1c9..24beb027ceef 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -56,6 +56,19 @@ DEFINE_PROTO_FUZZER( ENVOY_LOG_MISC(debug, "EnvoyException during validation: {}", e.what()); return; } + +// ASAN fuzz testing is producing an error on CEL parsing that is believed to be a false positive. +// It is determined that the error is unrelated to the implementation of request and response +// attributes as demonstrated here +// https://github.com/envoyproxy/envoy/compare/main...jbohanon:envoy:bug/cel-parser-antlr-exception-use-after-free. +// Discussion on this is located at https://github.com/envoyproxy/envoy/pull/31017 +#ifdef ASAN_FUZZER + if (!input.config().request_attributes().empty() || + !input.config().response_attributes().empty()) { + return; + } +#endif + static FuzzerMocks mocks; NiceMock stats_store; From 2954fc7870cb0a7e00d6fd6f006d8c1d131a2476 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Mon, 4 Dec 2023 14:39:36 -0500 Subject: [PATCH 14/35] use unique_ptr instead of absl::optional for struct, reference for activation Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc.cc | 18 ++-- .../filters/http/ext_proc/ext_proc.h | 10 +-- .../filters/http/ext_proc/matching_utils.cc | 82 +++++++++++-------- .../filters/http/ext_proc/matching_utils.h | 27 ++---- 4 files changed, 70 insertions(+), 67 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 8b6dd2e743ec..d5c4ebbeeef4 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -202,7 +202,7 @@ void Filter::onDestroy() { FilterHeadersStatus Filter::onHeaders(ProcessorState& state, Http::RequestOrResponseHeaderMap& headers, bool end_stream, - absl::optional proto) { + std::unique_ptr proto) { switch (openStream()) { case StreamOpenState::Error: return FilterHeadersStatus::StopIteration; @@ -220,8 +220,8 @@ FilterHeadersStatus Filter::onHeaders(ProcessorState& state, MutationUtils::headersToProto(headers, config_->allowedHeaders(), config_->disallowedHeaders(), *headers_req->mutable_headers()); headers_req->set_end_of_stream(end_stream); - if (proto.has_value()) { - (*headers_req->mutable_attributes())[FilterName] = proto.value(); + if (proto != nullptr) { + (*headers_req->mutable_attributes())[FilterName] = *proto; } state.onStartProcessorCall(std::bind(&Filter::onMessageTimeout, this), config_->messageTimeout(), ProcessorState::CallbackState::HeadersCallback); @@ -241,14 +241,14 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { - absl::optional proto; + std::unique_ptr proto; if (config_->expressionManager().hasRequestExpr()) { auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), &headers, nullptr, nullptr); - proto = config_->expressionManager().evaluateRequestAttributes(std::move(activation_ptr)); + proto = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); } - status = onHeaders(decoding_state_, headers, end_stream, proto); + status = onHeaders(decoding_state_, headers, end_stream, std::move(proto)); ENVOY_LOG(trace, "onHeaders returning {}", static_cast(status)); } else { ENVOY_LOG(trace, "decodeHeaders: Skipped header processing"); @@ -526,14 +526,14 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s FilterHeadersStatus status = FilterHeadersStatus::Continue; if (!processing_complete_ && encoding_state_.sendHeaders()) { - absl::optional proto; + std::unique_ptr proto; if (config_->expressionManager().hasResponseExpr()) { auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), nullptr, &headers, nullptr); - proto = config_->expressionManager().evaluateResponseAttributes(std::move(activation_ptr)); + proto = config_->expressionManager().evaluateResponseAttributes(*activation_ptr); } - status = onHeaders(encoding_state_, headers, end_stream, proto); + status = onHeaders(encoding_state_, headers, end_stream, std::move(proto)); ENVOY_LOG(trace, "onHeaders returns {}", static_cast(status)); } else { ENVOY_LOG(trace, "encodeHeaders: Skipped header processing"); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index c00d659fee37..d8a439d52989 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -138,9 +138,9 @@ class FilterConfig { allow_mode_override_(config.allow_mode_override()), disable_immediate_response_(config.disable_immediate_response()), allowed_headers_( - MatchingUtils::initHeaderMatchers(config.forward_rules().allowed_headers())), + initHeaderMatchers(config.forward_rules().allowed_headers())), disallowed_headers_( - MatchingUtils::initHeaderMatchers(config.forward_rules().disallowed_headers())), + initHeaderMatchers(config.forward_rules().disallowed_headers())), expression_manager_(builder, config.request_attributes(), config.response_attributes()) {} bool failureModeAllow() const { return failure_mode_allow_; } @@ -169,7 +169,7 @@ class FilterConfig { return disallowed_headers_; } - const Envoy::ProtobufWkt::Struct& filterMetadata() const { return filter_metadata_; } + const ProtobufWkt::Struct& filterMetadata() const { return filter_metadata_; } const ExpressionManager& expressionManager() const { return expression_manager_; } @@ -187,7 +187,7 @@ class FilterConfig { ExtProcFilterStats stats_; const envoy::extensions::filters::http::ext_proc::v3::ProcessingMode processing_mode_; const Filters::Common::MutationRules::Checker mutation_checker_; - const Envoy::ProtobufWkt::Struct filter_metadata_; + const ProtobufWkt::Struct filter_metadata_; // If set to true, allow the processing mode to be modified by the ext_proc response. const bool allow_mode_override_; // If set to true, disable the immediate response from the ext_proc server, which means @@ -299,7 +299,7 @@ class Filter : public Logger::Loggable, Http::FilterHeadersStatus onHeaders(ProcessorState& state, Http::RequestOrResponseHeaderMap& headers, bool end_stream, - absl::optional proto); + std::unique_ptr proto); // Return a pair of whether to terminate returning the current result. std::pair sendStreamChunk(ProcessorState& state); diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index a806e83a9481..fb47cc85b490 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -37,47 +37,61 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField return expressions; } -const absl::optional ExpressionManager::evaluateAttributes( - const Filters::Common::Expr::ActivationPtr& activation, +std::unique_ptr ExpressionManager::evaluateAttributes( + const Filters::Common::Expr::Activation& activation, const absl::flat_hash_map& expr) { - absl::optional 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; + + if (expr.empty()) { + return nullptr; + } + + auto proto = std::make_unique(); + 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())); + } + + auto proto_mut_fields = proto->mutable_fields(); + (*proto_mut_fields)[hash_entry.first] = value; } return proto; } +const std::vector +initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_list) { + std::vector header_matchers; + for (const auto& matcher : header_list.patterns()) { + header_matchers.push_back( + std::make_unique>( + matcher)); + } + return header_matchers; +} + } // namespace ExternalProcessing } // namespace HttpFilters } // namespace Extensions diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index 305d40c84535..c27b089d1db4 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -22,18 +22,18 @@ class ExpressionManager : public Logger::Loggable { bool hasResponseExpr() const { return !response_expr_.empty(); }; - const absl::optional - evaluateRequestAttributes(const Filters::Common::Expr::ActivationPtr& activation) const { + std::unique_ptr + evaluateRequestAttributes(const Filters::Common::Expr::Activation& activation) const { return evaluateAttributes(activation, request_expr_); } - const absl::optional - evaluateResponseAttributes(const Filters::Common::Expr::ActivationPtr& activation) const { + std::unique_ptr + evaluateResponseAttributes(const Filters::Common::Expr::Activation& activation) const { return evaluateAttributes(activation, response_expr_); } - static const absl::optional evaluateAttributes( - const Filters::Common::Expr::ActivationPtr& activation, + static std::unique_ptr evaluateAttributes( + const Filters::Common::Expr::Activation& activation, const absl::flat_hash_map& expr); private: @@ -49,19 +49,8 @@ class ExpressionManager : public Logger::Loggable { const absl::flat_hash_map response_expr_; }; -class MatchingUtils : public Logger::Loggable { -public: - static const std::vector - initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_list) { - std::vector header_matchers; - for (const auto& matcher : header_list.patterns()) { - header_matchers.push_back( - std::make_unique>( - matcher)); - } - return header_matchers; - } -}; +const std::vector +initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_list); } // namespace ExternalProcessing } // namespace HttpFilters From 520d511d383cf48b59002b8d2ed95c5c1e532ae6 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 5 Dec 2023 10:53:20 -0500 Subject: [PATCH 15/35] use object to store pair of parsed, compiled expression Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc.h | 6 ++--- .../filters/http/ext_proc/matching_utils.cc | 23 +++++++++++-------- .../filters/http/ext_proc/matching_utils.h | 20 ++++++++-------- .../ext_proc/ext_proc_integration_test.cc | 3 ++- 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index d8a439d52989..fc4591d5ba70 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -137,10 +137,8 @@ class FilterConfig { filter_metadata_(config.filter_metadata()), allow_mode_override_(config.allow_mode_override()), disable_immediate_response_(config.disable_immediate_response()), - allowed_headers_( - initHeaderMatchers(config.forward_rules().allowed_headers())), - disallowed_headers_( - initHeaderMatchers(config.forward_rules().disallowed_headers())), + allowed_headers_(initHeaderMatchers(config.forward_rules().allowed_headers())), + disallowed_headers_(initHeaderMatchers(config.forward_rules().disallowed_headers())), expression_manager_(builder, config.request_attributes(), config.response_attributes()) {} bool failureModeAllow() const { return failure_mode_allow_; } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index fb47cc85b490..a1f61085f2ec 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -11,23 +11,26 @@ namespace Extensions { namespace HttpFilters { namespace ExternalProcessing { -absl::flat_hash_map +absl::flat_hash_map ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField& matchers) { - absl::flat_hash_map expressions; + absl::flat_hash_map expressions; #if defined(USE_CEL_PARSER) for (const auto& matcher : matchers) { + if (expressions.contains(matcher)) { + continue; + } auto parse_status = google::api::expr::parser::Parse(matcher); if (!parse_status.ok()) { throw EnvoyException("Unable to parse descriptor expression: " + parse_status.status().ToString()); } - auto& iter = expr_list_.emplace_back(parse_status.value()); - Filters::Common::Expr::ExpressionPtr expression = - Extensions::Filters::Common::Expr::createExpression(builder_->builder(), iter.expr()); + Extensions::Filters::Common::Expr::createExpression(builder_->builder(), + parse_status.value().expr()); - expressions.try_emplace(matcher, std::move(expression)); + CelExpression cel_expr{parse_status.value(), std::move(expression)}; + expressions.emplace(matcher, std::move(cel_expr)); } #else ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." @@ -37,9 +40,9 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField return expressions; } -std::unique_ptr ExpressionManager::evaluateAttributes( - const Filters::Common::Expr::Activation& activation, - const absl::flat_hash_map& expr) { +std::unique_ptr +ExpressionManager::evaluateAttributes(const Filters::Common::Expr::Activation& activation, + const absl::flat_hash_map& expr) { if (expr.empty()) { return nullptr; @@ -48,7 +51,7 @@ std::unique_ptr ExpressionManager::evaluateAttributes( auto proto = std::make_unique(); for (const auto& hash_entry : expr) { ProtobufWkt::Arena arena; - const auto result = hash_entry.second->Evaluate(activation, &arena); + const auto result = hash_entry.second.compiled_expr_->Evaluate(activation, &arena); if (!result.ok()) { // TODO: Stats? continue; diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index c27b089d1db4..8b83866eb4e1 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -12,6 +12,11 @@ namespace ExternalProcessing { class ExpressionManager : public Logger::Loggable { public: + struct CelExpression { + google::api::expr::v1alpha1::ParsedExpr parsed_expr_; + Filters::Common::Expr::ExpressionPtr compiled_expr_; + }; + ExpressionManager(Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder, const Protobuf::RepeatedPtrField& request_matchers, const Protobuf::RepeatedPtrField& response_matchers) @@ -32,21 +37,18 @@ class ExpressionManager : public Logger::Loggable { return evaluateAttributes(activation, response_expr_); } - static std::unique_ptr evaluateAttributes( - const Filters::Common::Expr::Activation& activation, - const absl::flat_hash_map& expr); + static std::unique_ptr + evaluateAttributes(const Filters::Common::Expr::Activation& activation, + const absl::flat_hash_map& expr); private: - // This list is required to maintain the lifetimes of expr objects on which compiled expressions - // depend - std::list expr_list_; - absl::flat_hash_map + absl::flat_hash_map initExpressions(const Protobuf::RepeatedPtrField& matchers); Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; - const absl::flat_hash_map request_expr_; - const absl::flat_hash_map response_expr_; + const absl::flat_hash_map request_expr_; + const absl::flat_hash_map response_expr_; }; const std::vector diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 7c20bfc968eb..2b34b4c1e89c 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -3342,7 +3342,8 @@ 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("response.code").string_value(), "200"); - EXPECT_EQ(proto_struct.fields().at("response.code_details").string_value(), "via_upstream"); + EXPECT_EQ(proto_struct.fields().at("response.code_details").string_value(), + StreamInfo::ResponseCodeDetails::get().ViaUpstream); return true; }); From f6bbcf793e621f0fb0d3fd107acc2bbd950d4518 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 15 Dec 2023 08:10:51 -0500 Subject: [PATCH 16/35] inline CelExpression initialization into emplace call Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/matching_utils.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index a1f61085f2ec..681fa0f095d9 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -29,8 +29,7 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField Extensions::Filters::Common::Expr::createExpression(builder_->builder(), parse_status.value().expr()); - CelExpression cel_expr{parse_status.value(), std::move(expression)}; - expressions.emplace(matcher, std::move(cel_expr)); + expressions.emplace(matcher, ExpressionManager::CelExpression{parse_status.value(), std::move(expression)}); } #else ENVOY_LOG(warn, "CEL expression parsing is not available for use in this environment." From fbdbfb494c069ddd1b1c8be4083d893a4db5fd58 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 15 Dec 2023 09:02:53 -0500 Subject: [PATCH 17/35] raw ptr working Signed-off-by: Jacob Bohanon --- .../extensions/filters/http/ext_proc/ext_proc.cc | 14 +++++++++----- source/extensions/filters/http/ext_proc/ext_proc.h | 2 +- .../filters/http/ext_proc/matching_utils.cc | 9 +++++---- .../filters/http/ext_proc/matching_utils.h | 6 +++--- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index d5c4ebbeeef4..126ce0fd826d 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -202,7 +202,7 @@ void Filter::onDestroy() { FilterHeadersStatus Filter::onHeaders(ProcessorState& state, Http::RequestOrResponseHeaderMap& headers, bool end_stream, - std::unique_ptr proto) { + ProtobufWkt::Struct* proto) { switch (openStream()) { case StreamOpenState::Error: return FilterHeadersStatus::StopIteration; @@ -241,14 +241,16 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { - std::unique_ptr proto; + ProtobufWkt::Struct proto; + if (config_->expressionManager().hasRequestExpr()) { auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), &headers, nullptr, nullptr); proto = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); } - status = onHeaders(decoding_state_, headers, end_stream, std::move(proto)); + status = onHeaders(decoding_state_, headers, end_stream, + config_->expressionManager().hasRequestExpr() ? &proto : nullptr); ENVOY_LOG(trace, "onHeaders returning {}", static_cast(status)); } else { ENVOY_LOG(trace, "decodeHeaders: Skipped header processing"); @@ -526,14 +528,16 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s FilterHeadersStatus status = FilterHeadersStatus::Continue; if (!processing_complete_ && encoding_state_.sendHeaders()) { - std::unique_ptr proto; + ProtobufWkt::Struct proto; + if (config_->expressionManager().hasResponseExpr()) { auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), nullptr, &headers, nullptr); proto = config_->expressionManager().evaluateResponseAttributes(*activation_ptr); } - status = onHeaders(encoding_state_, headers, end_stream, std::move(proto)); + status = onHeaders(encoding_state_, headers, end_stream, + config_->expressionManager().hasResponseExpr() ? &proto : nullptr); ENVOY_LOG(trace, "onHeaders returns {}", static_cast(status)); } else { ENVOY_LOG(trace, "encodeHeaders: Skipped header processing"); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index fc4591d5ba70..b4f096afb79b 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -297,7 +297,7 @@ class Filter : public Logger::Loggable, Http::FilterHeadersStatus onHeaders(ProcessorState& state, Http::RequestOrResponseHeaderMap& headers, bool end_stream, - std::unique_ptr proto); + ProtobufWkt::Struct* proto); // Return a pair of whether to terminate returning the current result. std::pair sendStreamChunk(ProcessorState& state); diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 681fa0f095d9..4e2c891e2387 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -39,15 +39,16 @@ ExpressionManager::initExpressions(const Protobuf::RepeatedPtrField return expressions; } -std::unique_ptr +ProtobufWkt::Struct ExpressionManager::evaluateAttributes(const Filters::Common::Expr::Activation& activation, const absl::flat_hash_map& expr) { + ProtobufWkt::Struct proto; + if (expr.empty()) { - return nullptr; + return proto; } - auto proto = std::make_unique(); for (const auto& hash_entry : expr) { ProtobufWkt::Arena arena; const auto result = hash_entry.second.compiled_expr_->Evaluate(activation, &arena); @@ -76,7 +77,7 @@ ExpressionManager::evaluateAttributes(const Filters::Common::Expr::Activation& a value.set_string_value(Filters::Common::Expr::print(result.value())); } - auto proto_mut_fields = proto->mutable_fields(); + auto proto_mut_fields = proto.mutable_fields(); (*proto_mut_fields)[hash_entry.first] = value; } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index 8b83866eb4e1..e033e75e8908 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -27,17 +27,17 @@ class ExpressionManager : public Logger::Loggable { bool hasResponseExpr() const { return !response_expr_.empty(); }; - std::unique_ptr + ProtobufWkt::Struct evaluateRequestAttributes(const Filters::Common::Expr::Activation& activation) const { return evaluateAttributes(activation, request_expr_); } - std::unique_ptr + ProtobufWkt::Struct evaluateResponseAttributes(const Filters::Common::Expr::Activation& activation) const { return evaluateAttributes(activation, response_expr_); } - static std::unique_ptr + static ProtobufWkt::Struct evaluateAttributes(const Filters::Common::Expr::Activation& activation, const absl::flat_hash_map& expr); From 58faac176dddcdc357dda146df347d847eae1c96 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 15 Dec 2023 09:43:07 -0500 Subject: [PATCH 18/35] update dep allowlist Signed-off-by: Jacob Bohanon --- bazel/repository_locations.bzl | 1 + 1 file changed, 1 insertion(+) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 4d8f68f5f28f..231c08106aa1 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1240,6 +1240,7 @@ REPOSITORY_LOCATIONS_SPEC = dict( "envoy.formatter.cel", "envoy.bootstrap.wasm", "envoy.rate_limit_descriptors.expr", + "envoy.filters.http.ext_proc", "envoy.filters.http.rate_limit_quota", "envoy.filters.http.rbac", "envoy.filters.http.wasm", From 19fdb00a4529b9506304dc7b81c6628d871f62bb Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 15 Dec 2023 11:06:04 -0500 Subject: [PATCH 19/35] use serverFactoryContext to get builder singleton Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/config.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/config.cc b/source/extensions/filters/http/ext_proc/config.cc index 654778569113..e8eaf4c80b9b 100644 --- a/source/extensions/filters/http/ext_proc/config.cc +++ b/source/extensions/filters/http/ext_proc/config.cc @@ -18,7 +18,8 @@ Http::FilterFactoryCb ExternalProcessingFilterConfig::createFilterFactoryFromPro PROTOBUF_GET_MS_OR_DEFAULT(proto_config, max_message_timeout, DefaultMaxMessageTimeoutMs); const auto filter_config = std::make_shared( proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, - context.scope(), stats_prefix, Envoy::Extensions::Filters::Common::Expr::getBuilder(context)); + context.scope(), stats_prefix, + Envoy::Extensions::Filters::Common::Expr::getBuilder(context.serverFactoryContext())); return [filter_config, grpc_service = proto_config.grpc_service(), &context](Http::FilterChainFactoryCallbacks& callbacks) { From 8948d0f7e7cf833479d26c82dd6fcccaaf5732ab Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 15 Dec 2023 11:27:24 -0500 Subject: [PATCH 20/35] update dep allowlist Signed-off-by: Jacob Bohanon --- bazel/repository_locations.bzl | 1 + 1 file changed, 1 insertion(+) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 231c08106aa1..2af8e21c833a 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -1211,6 +1211,7 @@ REPOSITORY_LOCATIONS_SPEC = dict( "envoy.access_loggers.wasm", "envoy.bootstrap.wasm", "envoy.rate_limit_descriptors.expr", + "envoy.filters.http.ext_proc", "envoy.filters.http.rate_limit_quota", "envoy.filters.http.rbac", "envoy.filters.http.wasm", From fe68f15ce99a14ba68d99116e95b290a306e5bbe Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 15 Dec 2023 16:01:28 -0500 Subject: [PATCH 21/35] PR comments Signed-off-by: Jacob Bohanon --- .bazelrc | 1 - .../filters/http/ext_proc/matching_utils.cc | 2 +- .../filters/http/ext_proc/matching_utils.h | 2 +- .../unit_test_fuzz/ext_proc_unit_test_fuzz.cc | 12 +++++------- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/.bazelrc b/.bazelrc index f7423df60570..df66d82946b7 100644 --- a/.bazelrc +++ b/.bazelrc @@ -417,7 +417,6 @@ build:asan-fuzzer --config=clang-asan build:asan-fuzzer --copt=-fno-omit-frame-pointer # Remove UBSAN halt_on_error to avoid crashing on protobuf errors. build:asan-fuzzer --test_env=UBSAN_OPTIONS=print_stacktrace=1 -build:asan-fuzzer --copt=-DASAN_FUZZER build:oss-fuzz --config=fuzzing build:oss-fuzz --define=FUZZING_ENGINE=oss-fuzz diff --git a/source/extensions/filters/http/ext_proc/matching_utils.cc b/source/extensions/filters/http/ext_proc/matching_utils.cc index 3a9045399dce..e778be5ab945 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.cc +++ b/source/extensions/filters/http/ext_proc/matching_utils.cc @@ -85,7 +85,7 @@ ExpressionManager::evaluateAttributes(const Filters::Common::Expr::Activation& a return proto; } -const std::vector +std::vector initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_list) { std::vector header_matchers; for (const auto& matcher : header_list.patterns()) { diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index e033e75e8908..96a07caf34bb 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -51,7 +51,7 @@ class ExpressionManager : public Logger::Loggable { const absl::flat_hash_map response_expr_; }; -const std::vector +std::vector initHeaderMatchers(const envoy::type::matcher::v3::ListStringMatcher& header_list); } // namespace ExternalProcessing diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index 24beb027ceef..0f2167e9929e 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -57,17 +57,15 @@ DEFINE_PROTO_FUZZER( return; } -// ASAN fuzz testing is producing an error on CEL parsing that is believed to be a false positive. -// It is determined that the error is unrelated to the implementation of request and response -// attributes as demonstrated here -// https://github.com/envoyproxy/envoy/compare/main...jbohanon:envoy:bug/cel-parser-antlr-exception-use-after-free. -// Discussion on this is located at https://github.com/envoyproxy/envoy/pull/31017 -#ifdef ASAN_FUZZER + // ASAN fuzz testing is producing an error on CEL parsing that is believed to be a false positive. + // It is determined that the error is unrelated to the implementation of request and response + // attributes as demonstrated here + // https://github.com/envoyproxy/envoy/compare/main...jbohanon:envoy:bug/cel-parser-antlr-exception-use-after-free. + // Discussion on this is located at https://github.com/envoyproxy/envoy/pull/31017 if (!input.config().request_attributes().empty() || !input.config().response_attributes().empty()) { return; } -#endif static FuzzerMocks mocks; NiceMock stats_store; From d6cb9321eaeec2cecabe10b387b357affa2bf410 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 15 Dec 2023 18:17:24 -0500 Subject: [PATCH 22/35] kick CI Signed-off-by: Jacob Bohanon From ae4258ddc8716770945822b0b584af0fc561f07b Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 19 Dec 2023 10:05:20 -0500 Subject: [PATCH 23/35] plumb through local info Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/config.cc | 6 ++++-- source/extensions/filters/http/ext_proc/ext_proc.cc | 10 ++++++---- source/extensions/filters/http/ext_proc/ext_proc.h | 6 ++++-- .../extensions/filters/http/ext_proc/matching_utils.h | 7 ++++++- test/extensions/filters/http/ext_proc/BUILD | 1 + test/extensions/filters/http/ext_proc/filter_test.cc | 4 +++- test/extensions/filters/http/ext_proc/ordering_test.cc | 4 +++- .../filters/http/ext_proc/unit_test_fuzz/BUILD | 1 + .../ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc | 6 ++++-- 9 files changed, 32 insertions(+), 13 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/config.cc b/source/extensions/filters/http/ext_proc/config.cc index e8eaf4c80b9b..ee45fc6a73c4 100644 --- a/source/extensions/filters/http/ext_proc/config.cc +++ b/source/extensions/filters/http/ext_proc/config.cc @@ -19,7 +19,8 @@ Http::FilterFactoryCb ExternalProcessingFilterConfig::createFilterFactoryFromPro const auto filter_config = std::make_shared( proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, context.scope(), stats_prefix, - Envoy::Extensions::Filters::Common::Expr::getBuilder(context.serverFactoryContext())); + Envoy::Extensions::Filters::Common::Expr::getBuilder(context.serverFactoryContext()), + context.serverFactoryContext().localInfo()); return [filter_config, grpc_service = proto_config.grpc_service(), &context](Http::FilterChainFactoryCallbacks& callbacks) { @@ -49,7 +50,8 @@ ExternalProcessingFilterConfig::createFilterFactoryFromProtoWithServerContextTyp const auto filter_config = std::make_shared( proto_config, std::chrono::milliseconds(message_timeout_ms), max_message_timeout_ms, server_context.scope(), stats_prefix, - Envoy::Extensions::Filters::Common::Expr::getBuilder(server_context)); + Envoy::Extensions::Filters::Common::Expr::getBuilder(server_context), + server_context.localInfo()); return [filter_config, grpc_service = proto_config.grpc_service(), &server_context](Http::FilterChainFactoryCallbacks& callbacks) { diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 25b702468365..e2759fc35107 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -244,8 +244,9 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st ProtobufWkt::Struct proto; if (config_->expressionManager().hasRequestExpr()) { - auto activation_ptr = Filters::Common::Expr::createActivation(decoding_state_.streamInfo(), - &headers, nullptr, nullptr); + auto activation_ptr = Filters::Common::Expr::createActivation( + &config_->expressionManager().localInfo(), decoding_state_.streamInfo(), &headers, + nullptr, nullptr); proto = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); } @@ -531,8 +532,9 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s ProtobufWkt::Struct proto; if (config_->expressionManager().hasResponseExpr()) { - auto activation_ptr = Filters::Common::Expr::createActivation(encoding_state_.streamInfo(), - nullptr, &headers, nullptr); + auto activation_ptr = Filters::Common::Expr::createActivation( + &config_->expressionManager().localInfo(), encoding_state_.streamInfo(), nullptr, + &headers, nullptr); proto = config_->expressionManager().evaluateResponseAttributes(*activation_ptr); } diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index b4f096afb79b..5e8cbec5f5c6 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -128,7 +128,8 @@ class FilterConfig { const std::chrono::milliseconds message_timeout, const uint32_t max_message_timeout_ms, Stats::Scope& scope, const std::string& stats_prefix, - Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder) + Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder, + const LocalInfo::LocalInfo& local_info) : failure_mode_allow_(config.failure_mode_allow()), disable_clear_route_cache_(config.disable_clear_route_cache()), message_timeout_(message_timeout), max_message_timeout_ms_(max_message_timeout_ms), @@ -139,7 +140,8 @@ class FilterConfig { disable_immediate_response_(config.disable_immediate_response()), allowed_headers_(initHeaderMatchers(config.forward_rules().allowed_headers())), disallowed_headers_(initHeaderMatchers(config.forward_rules().disallowed_headers())), - expression_manager_(builder, config.request_attributes(), config.response_attributes()) {} + expression_manager_(builder, local_info, config.request_attributes(), + config.response_attributes()) {} bool failureModeAllow() const { return failure_mode_allow_; } diff --git a/source/extensions/filters/http/ext_proc/matching_utils.h b/source/extensions/filters/http/ext_proc/matching_utils.h index 96a07caf34bb..f02c51ac2728 100644 --- a/source/extensions/filters/http/ext_proc/matching_utils.h +++ b/source/extensions/filters/http/ext_proc/matching_utils.h @@ -18,9 +18,11 @@ class ExpressionManager : public Logger::Loggable { }; ExpressionManager(Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder, + const LocalInfo::LocalInfo& local_info, const Protobuf::RepeatedPtrField& request_matchers, const Protobuf::RepeatedPtrField& response_matchers) - : builder_(builder), request_expr_(initExpressions(request_matchers)), + : builder_(builder), local_info_(local_info), + request_expr_(initExpressions(request_matchers)), response_expr_(initExpressions(response_matchers)){}; bool hasRequestExpr() const { return !request_expr_.empty(); }; @@ -41,11 +43,14 @@ class ExpressionManager : public Logger::Loggable { evaluateAttributes(const Filters::Common::Expr::Activation& activation, const absl::flat_hash_map& expr); + const LocalInfo::LocalInfo& localInfo() const { return local_info_; }; + private: absl::flat_hash_map initExpressions(const Protobuf::RepeatedPtrField& matchers); Extensions::Filters::Common::Expr::BuilderInstanceSharedPtr builder_; + const LocalInfo::LocalInfo& local_info_; const absl::flat_hash_map request_expr_; const absl::flat_hash_map response_expr_; diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index 86489d81c259..431286d586dd 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -82,6 +82,7 @@ envoy_extension_cc_test( "//source/extensions/filters/http/ext_proc", "//test/common/http:common_lib", "//test/mocks/event:event_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/server:factory_context_mocks", "//test/test_common:test_runtime_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index ad79eaadbe5a..c52d9aae6023 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -124,7 +124,8 @@ class HttpFilterTest : public testing::Test { config_ = std::make_shared( proto_config, 200ms, 10000, *stats_store_.rootScope(), "", std::make_shared( - Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr)), + local_info_); filter_ = std::make_unique(config_, std::move(client_), proto_config.grpc_service()); filter_->setEncoderFilterCallbacks(encoder_callbacks_); EXPECT_CALL(encoder_callbacks_, encoderBufferLimit()).WillRepeatedly(Return(BufferSize)); @@ -563,6 +564,7 @@ class HttpFilterTest : public testing::Test { std::vector timers_; TestScopedRuntime scoped_runtime_; Envoy::Event::SimulatedTimeSystem* test_time_; + testing::NiceMock local_info_; }; // Using the default configuration, test the filter with a processor that diff --git a/test/extensions/filters/http/ext_proc/ordering_test.cc b/test/extensions/filters/http/ext_proc/ordering_test.cc index 79fbd7580042..d0fedeb64e3e 100644 --- a/test/extensions/filters/http/ext_proc/ordering_test.cc +++ b/test/extensions/filters/http/ext_proc/ordering_test.cc @@ -73,7 +73,8 @@ class OrderingTest : public testing::Test { config_ = std::make_shared( proto_config, kMessageTimeout, kMaxMessageTimeoutMs, *stats_store_.rootScope(), "", std::make_shared( - Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr)), + local_info_); filter_ = std::make_unique(config_, std::move(client_), proto_config.grpc_service()); filter_->setEncoderFilterCallbacks(encoder_callbacks_); filter_->setDecoderFilterCallbacks(decoder_callbacks_); @@ -214,6 +215,7 @@ class OrderingTest : public testing::Test { Http::TestResponseHeaderMapImpl response_headers_; Http::TestRequestTrailerMapImpl request_trailers_; Http::TestResponseTrailerMapImpl response_trailers_; + NiceMock local_info_; }; // A base class for tests that will check that gRPC streams fail while being created diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD b/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD index 2a7265e6a63b..d39d753ed928 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD @@ -39,6 +39,7 @@ envoy_cc_fuzz_test( "//source/extensions/filters/http/ext_proc:config", "//test/extensions/filters/http/common/fuzz:http_filter_fuzzer_lib", "//test/mocks/http:http_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", ], ) diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index 0f2167e9929e..b0a8c794bd6c 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -45,7 +45,8 @@ class FuzzerMocks { NiceMock request_trailers_; NiceMock response_trailers_; NiceMock buffer_; - testing::NiceMock async_client_stream_info_; + NiceMock async_client_stream_info_; + NiceMock local_info_; }; DEFINE_PROTO_FUZZER( @@ -84,7 +85,8 @@ DEFINE_PROTO_FUZZER( config = std::make_shared( proto_config, std::chrono::milliseconds(200), 200, *stats_store.rootScope(), "", std::make_shared( - Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr))); + Envoy::Extensions::Filters::Common::Expr::createBuilder(nullptr)), + mocks.local_info_); } catch (const EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException during ext_proc filter config validation: {}", e.what()); return; From 8e020363d389b7d161bb509e9d477f21cb754cd0 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 19 Dec 2023 10:39:09 -0500 Subject: [PATCH 24/35] kick CI -- RBE cache error in iOS Signed-off-by: Jacob Bohanon From d4d94e61d7c133d91ad791003524a45d7a52ce3a Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 19 Dec 2023 11:07:49 -0500 Subject: [PATCH 25/35] add inludes for MockLocalInfo Signed-off-by: Jacob Bohanon --- test/extensions/filters/http/ext_proc/filter_test.cc | 1 + test/extensions/filters/http/ext_proc/ordering_test.cc | 1 + .../http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc | 1 + 3 files changed, 3 insertions(+) diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index c52d9aae6023..f58c857c3b8e 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -21,6 +21,7 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/http/mocks.h" #include "test/mocks/http/stream_encoder.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/router/mocks.h" #include "test/mocks/runtime/mocks.h" diff --git a/test/extensions/filters/http/ext_proc/ordering_test.cc b/test/extensions/filters/http/ext_proc/ordering_test.cc index d0fedeb64e3e..06d1e89e4aba 100644 --- a/test/extensions/filters/http/ext_proc/ordering_test.cc +++ b/test/extensions/filters/http/ext_proc/ordering_test.cc @@ -7,6 +7,7 @@ #include "test/extensions/filters/http/ext_proc/mock_server.h" #include "test/mocks/event/mocks.h" #include "test/mocks/http/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/router/mocks.h" #include "test/mocks/stream_info/mocks.h" diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc index b0a8c794bd6c..184c4be32365 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_unit_test_fuzz.cc @@ -5,6 +5,7 @@ #include "test/extensions/filters/http/ext_proc/unit_test_fuzz/mocks.h" #include "test/fuzz/fuzz_runner.h" #include "test/mocks/http/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" using testing::Return; From 7019dd1c4057259d6c10b420c2e5e7a114509831 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Mon, 8 Jan 2024 12:21:05 -0500 Subject: [PATCH 26/35] kick CI Signed-off-by: Jacob Bohanon From defdd9468ffd33c585b92a89d048ad4e483d1b77 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 9 Jan 2024 16:33:48 -0500 Subject: [PATCH 27/35] kick CI Signed-off-by: Jacob Bohanon From c86a7314a076ba129a7caf9d71f891fa3de716ce Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 9 Jan 2024 18:11:53 -0500 Subject: [PATCH 28/35] kick CI Signed-off-by: Jacob Bohanon From 0b3e71500c4b6eead1fee0407fb903774bfaae96 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Mon, 15 Jan 2024 09:37:54 -0500 Subject: [PATCH 29/35] kick CI Signed-off-by: Jacob Bohanon From a3e689fc185c8e499729887466df57dfac5d7188 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Mon, 15 Jan 2024 09:43:50 -0500 Subject: [PATCH 30/35] revert unwanted local build change Signed-off-by: Jacob Bohanon --- bazel/foreign_cc/BUILD | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bazel/foreign_cc/BUILD b/bazel/foreign_cc/BUILD index ce66e8f3e0bd..eb9c9e81d2c4 100644 --- a/bazel/foreign_cc/BUILD +++ b/bazel/foreign_cc/BUILD @@ -140,8 +140,7 @@ configure_make( # Workaround for the issue with statically linked libstdc++ # using -l:libstdc++.a. env = { - # "CXXFLAGS": "--static -lstdc++ -Wno-unused-command-line-argument", - "CXXFLAGS": "-Wno-unused-command-line-argument", + "CXXFLAGS": "--static -lstdc++ -Wno-unused-command-line-argument", }, lib_source = "@net_colm_open_source_colm//:all", out_binaries = ["colm"], @@ -163,8 +162,7 @@ configure_make( # Workaround for the issue with statically linked libstdc++ # using -l:libstdc++.a. env = { - # "CXXFLAGS": "--static -lstdc++ -Wno-unused-command-line-argument", - "CXXFLAGS": "-Wno-unused-command-line-argument", + "CXXFLAGS": "--static -lstdc++ -Wno-unused-command-line-argument", }, lib_source = "@net_colm_open_source_ragel//:all", out_binaries = ["ragel"], From 9948c57eaea37bfea59adeb5a7cb86879ffb1303 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 16 Jan 2024 13:14:54 -0500 Subject: [PATCH 31/35] add skip_on_windows due to recent CEL bump Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/BUILD | 3 +++ test/extensions/filters/http/ext_proc/BUILD | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index 36315067cad5..f3a4cac5186a 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "ext_proc.h", "processor_state.h", ], + tags = ["skip_on_windows"], deps = [ ":client_interface", ":matching_utils_lib", @@ -45,6 +46,7 @@ envoy_cc_extension( name = "config", srcs = ["config.cc"], hdrs = ["config.h"], + tags = ["skip_on_windows"], deps = [ ":client_lib", ":ext_proc", @@ -91,6 +93,7 @@ envoy_cc_library( "-DUSE_CEL_PARSER", ], }), + tags = ["skip_on_windows"], deps = [ "//envoy/http:header_map_interface", "//source/common/protobuf", diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index 431286d586dd..c59d34f28e75 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -19,6 +19,7 @@ envoy_extension_cc_test( size = "small", srcs = ["config_test.cc"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ "//source/extensions/filters/http/ext_proc:config", "//test/mocks/server:factory_context_mocks", @@ -37,6 +38,7 @@ envoy_extension_cc_test( ], }), extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ ":mock_server_lib", ":utils_lib", @@ -67,6 +69,7 @@ envoy_extension_cc_test( size = "small", srcs = ["state_test.cc"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ "//source/extensions/filters/http/ext_proc", ], @@ -77,6 +80,7 @@ envoy_extension_cc_test( size = "small", srcs = ["ordering_test.cc"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ ":mock_server_lib", "//source/extensions/filters/http/ext_proc", @@ -94,6 +98,7 @@ envoy_extension_cc_test( size = "small", srcs = ["client_test.cc"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ "//source/common/http:header_map_lib", "//source/extensions/filters/http/ext_proc:client_lib", @@ -109,6 +114,7 @@ envoy_extension_cc_test( size = "small", srcs = ["mutation_utils_test.cc"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ ":utils_lib", "//source/extensions/filters/common/mutation_rules:mutation_rules_lib", @@ -135,6 +141,7 @@ envoy_extension_cc_test( tags = [ "cpu:3", ], + tags = ["skip_on_windows"], deps = [ ":logging_test_filter_lib", ":utils_lib", @@ -162,6 +169,7 @@ envoy_extension_cc_test( tags = [ "cpu:3", ], + tags = ["skip_on_windows"], deps = [ ":test_processor_lib", ":utils_lib", @@ -182,6 +190,7 @@ envoy_extension_cc_test_library( srcs = ["test_processor.cc"], hdrs = ["test_processor.h"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ "//envoy/network:address_interface", "//test/test_common:network_utility_lib", @@ -197,6 +206,7 @@ envoy_extension_cc_test_library( srcs = ["mock_server.cc"], hdrs = ["mock_server.h"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ "//source/extensions/filters/http/ext_proc:client_interface", ], @@ -207,6 +217,7 @@ envoy_extension_cc_test_library( srcs = ["utils.cc"], hdrs = ["utils.h"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ "//envoy/http:header_map_interface", "//test/test_common:utility_lib", @@ -220,6 +231,7 @@ envoy_extension_cc_test_library( srcs = ["ext_proc_grpc_fuzz_helper.cc"], hdrs = ["ext_proc_grpc_fuzz_helper.h"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ "//source/common/common:thread_lib", "//source/common/grpc:common_lib", @@ -262,6 +274,7 @@ envoy_cc_fuzz_test( srcs = ["ext_proc_grpc_fuzz.cc"], hdrs = ["ext_proc_grpc_fuzz.h"], corpus = "ext_proc_grpc_corpus", + tags = ["skip_on_windows"], deps = EXT_PROC_GRPC_FUZZ_TEST_DEPS, ) @@ -270,6 +283,7 @@ envoy_cc_fuzz_test( srcs = ["ext_proc_grpc_fuzz_persistent.cc"], hdrs = ["ext_proc_grpc_fuzz.h"], corpus = "ext_proc_grpc_corpus", + tags = ["skip_on_windows"], deps = EXT_PROC_GRPC_FUZZ_TEST_DEPS, ) @@ -277,6 +291,7 @@ envoy_extension_cc_test( name = "ext_proc_benchmark_test", srcs = ["ext_proc_benchmark_test.cc"], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ ":test_processor_lib", "//envoy/http:header_map_interface", @@ -304,6 +319,7 @@ envoy_extension_cc_test_library( "logging_test_filter.cc", ], extension_names = ["envoy.filters.http.ext_proc"], + tags = ["skip_on_windows"], deps = [ ":logging_test_filter_proto_cc_proto", "//envoy/http:filter_interface", From fcaa5af36f44f3c3a2b593e67dec0a45e8296feb Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 16 Jan 2024 13:30:37 -0500 Subject: [PATCH 32/35] fix BUILD Signed-off-by: Jacob Bohanon --- test/extensions/filters/http/ext_proc/BUILD | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index c59d34f28e75..0f00ed80ee5b 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -140,8 +140,8 @@ envoy_extension_cc_test( shard_count = 4, tags = [ "cpu:3", + "skip_on_windows", ], - tags = ["skip_on_windows"], deps = [ ":logging_test_filter_lib", ":utils_lib", @@ -168,8 +168,8 @@ envoy_extension_cc_test( extension_names = ["envoy.filters.http.ext_proc"], tags = [ "cpu:3", + "skip_on_windows", ], - tags = ["skip_on_windows"], deps = [ ":test_processor_lib", ":utils_lib", From 5fac8a5eaf5dd84eecd034a4e28340075c94a67f Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 17 Jan 2024 09:14:05 -0500 Subject: [PATCH 33/35] skip client build on windows Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/BUILD | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index f3a4cac5186a..976de7b294b8 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -58,6 +58,7 @@ envoy_cc_extension( envoy_cc_library( name = "client_interface", hdrs = ["client.h"], + tags = ["skip_on_windows"], deps = [ "//envoy/grpc:async_client_manager_interface", "//envoy/grpc:status", @@ -113,6 +114,7 @@ envoy_cc_library( name = "client_lib", srcs = ["client_impl.cc"], hdrs = ["client_impl.h"], + tags = ["skip_on_windows"], deps = [ ":client_interface", "//envoy/grpc:async_client_interface", From 970fb4dfe7c0feb4a8d8f1be6a42b717e80ea1f9 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 17 Jan 2024 16:56:21 -0500 Subject: [PATCH 34/35] MORE SKIP ON WINDOWS Signed-off-by: Jacob Bohanon --- test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD b/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD index d39d753ed928..5b12d7ce9390 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD @@ -13,6 +13,7 @@ envoy_package() envoy_cc_mock( name = "ext_proc_mocks", hdrs = ["mocks.h"], + tags = ["skip_on_windows"], deps = [ "//source/extensions/filters/http/ext_proc:client_interface", "@envoy_api//envoy/service/ext_proc/v3:pkg_cc_proto", @@ -22,6 +23,7 @@ envoy_cc_mock( envoy_proto_library( name = "ext_proc_unit_test_fuzz_proto", srcs = ["ext_proc_unit_test_fuzz.proto"], + tags = ["skip_on_windows"], deps = [ "//test/fuzz:common_proto", "@envoy_api//envoy/extensions/filters/http/ext_proc/v3:pkg", @@ -33,6 +35,7 @@ envoy_cc_fuzz_test( name = "ext_proc_unit_test_fuzz", srcs = ["ext_proc_unit_test_fuzz.cc"], corpus = "ext_proc_corpus", + tags = ["skip_on_windows"], deps = [ ":ext_proc_mocks", ":ext_proc_unit_test_fuzz_proto_cc_proto", From a43136e21198ee5900b4b478760b1fa7df2cbf18 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 18 Jan 2024 11:44:25 -0500 Subject: [PATCH 35/35] add filter to windows skip in bazel/repositories.bzl Signed-off-by: Jacob Bohanon --- bazel/repositories.bzl | 1 + test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index aa93c9c838d8..65d6de41043d 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -18,6 +18,7 @@ WINDOWS_SKIP_TARGETS = [ "envoy.access_loggers.extension_filters.cel", "envoy.rate_limit_descriptors.expr", "envoy.filters.http.rate_limit_quota", + "envoy.filters.http.ext_proc", "envoy.formatter.cel", "envoy.matching.inputs.cel_data_input", "envoy.matching.matchers.cel_matcher", diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD b/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD index 5b12d7ce9390..630a47ea12df 100644 --- a/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/BUILD @@ -23,7 +23,6 @@ envoy_cc_mock( envoy_proto_library( name = "ext_proc_unit_test_fuzz_proto", srcs = ["ext_proc_unit_test_fuzz.proto"], - tags = ["skip_on_windows"], deps = [ "//test/fuzz:common_proto", "@envoy_api//envoy/extensions/filters/http/ext_proc/v3:pkg",