From 0543abde3372524b92aba060f166d4fd6f2f3797 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 2 Feb 2024 17:43:38 -0500 Subject: [PATCH 01/12] revise service api for attributes Signed-off-by: Jacob Bohanon --- .../service/ext_proc/v3/external_processor.proto | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index aa62ef74226da..bf1895fc18000 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -56,7 +56,7 @@ service ExternalProcessor { // This represents the different types of messages that Envoy can send // to an external processing server. -// [#next-free-field: 9] +// [#next-free-field: 10] message ProcessingRequest { // Specify whether the filter that sent this request is running in synchronous // or asynchronous mode. The choice of synchronous or asynchronous mode @@ -110,8 +110,19 @@ message ProcessingRequest { HttpTrailers response_trailers = 7; } + // [#not-implemented-hide:] + // TODO(jbohanon) reserve field as part of rework detailed in https://github.com/envoyproxy/envoy/issues/32125 // Dynamic metadata associated with the request. config.core.v3.Metadata metadata_context = 8; + + // [#not-implemented-hide:] + // The values of properties selected by the ``request_attributes`` + // or ``response_attributes`` list in the configuration. Each entry + // in the list is populated from the standard + // :ref:`attributes ` supported across Envoy. + // This field also contains dynamic metadata from forwarding namespaces + // specified in configuration. + map attributes = 9; } // For every ProcessingRequest received by the server with the ``async_mode`` field @@ -204,6 +215,7 @@ message HttpHeaders { config.core.v3.HeaderMap headers = 1; // [#not-implemented-hide:] + // TODO(jbohanon) reserve field as part of rework detailed in https://github.com/envoyproxy/envoy/issues/32125 // The values of properties selected by the ``request_attributes`` // or ``response_attributes`` list in the configuration. Each entry // in the list is populated From 0baa98a8ead058a43dd6340586559c6a58fc0aaa Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Mon, 5 Feb 2024 11:00:41 -0500 Subject: [PATCH 02/12] remove metadata-related API changes Signed-off-by: Jacob Bohanon --- api/envoy/service/ext_proc/v3/external_processor.proto | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index bf1895fc18000..13ef49a54326a 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -110,8 +110,6 @@ message ProcessingRequest { HttpTrailers response_trailers = 7; } - // [#not-implemented-hide:] - // TODO(jbohanon) reserve field as part of rework detailed in https://github.com/envoyproxy/envoy/issues/32125 // Dynamic metadata associated with the request. config.core.v3.Metadata metadata_context = 8; @@ -120,8 +118,6 @@ message ProcessingRequest { // or ``response_attributes`` list in the configuration. Each entry // in the list is populated from the standard // :ref:`attributes ` supported across Envoy. - // This field also contains dynamic metadata from forwarding namespaces - // specified in configuration. map attributes = 9; } From 71d001dc59c724a93aea889fea86b97e992d5820 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Mon, 5 Feb 2024 17:00:19 -0500 Subject: [PATCH 03/12] kick CI Signed-off-by: Jacob Bohanon From 62ba44f3aea5c90181d879cf4374bfc401a2969f Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 6 Feb 2024 08:14:47 -0500 Subject: [PATCH 04/12] send attributes with first processing message on request & response path, not just with headers messages Signed-off-by: Jacob Bohanon --- .../ext_proc/v3/external_processor.proto | 12 +---- .../filters/http/ext_proc/ext_proc.cc | 49 ++++++++----------- .../filters/http/ext_proc/ext_proc.h | 5 +- .../filters/http/ext_proc/processor_state.h | 25 ++++++++++ 4 files changed, 51 insertions(+), 40 deletions(-) diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index 13ef49a54326a..15d0688588659 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -113,7 +113,6 @@ message ProcessingRequest { // Dynamic metadata associated with the request. config.core.v3.Metadata metadata_context = 8; - // [#not-implemented-hide:] // The values of properties selected by the ``request_attributes`` // or ``response_attributes`` list in the configuration. Each entry // in the list is populated from the standard @@ -200,6 +199,8 @@ message ProcessingResponse { // This message is sent to the external server when the HTTP request and responses // are first received. message HttpHeaders { + reserved 2; + // The HTTP request headers. All header keys will be // lower-cased, because HTTP header keys are case-insensitive. // The ``headers`` encoding is based on the runtime guard @@ -210,15 +211,6 @@ message HttpHeaders { // :ref:`value ` field. config.core.v3.HeaderMap headers = 1; - // [#not-implemented-hide:] - // TODO(jbohanon) reserve field as part of rework detailed in https://github.com/envoyproxy/envoy/issues/32125 - // The values of properties selected by the ``request_attributes`` - // or ``response_attributes`` list in the configuration. Each entry - // in the list is populated - // from the standard :ref:`attributes ` - // supported across Envoy. - map attributes = 2; - // If true, then there is no message body associated with this // request or response. bool end_of_stream = 3; diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 4b41dbdc57a24..f06f5c66621ca 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -276,8 +276,7 @@ void Filter::onDestroy() { } FilterHeadersStatus Filter::onHeaders(ProcessorState& state, - Http::RequestOrResponseHeaderMap& headers, bool end_stream, - ProtobufWkt::Struct* proto) { + Http::RequestOrResponseHeaderMap& headers, bool end_stream) { switch (openStream()) { case StreamOpenState::Error: return FilterHeadersStatus::StopIteration; @@ -291,14 +290,12 @@ FilterHeadersStatus Filter::onHeaders(ProcessorState& state, state.setHeaders(&headers); state.setHasNoBody(end_stream); ProcessingRequest req; + addAttributes(state, req); addDynamicMetadata(state, req); auto* headers_req = state.mutableHeaders(req); MutationUtils::headersToProto(headers, config_->allowedHeaders(), config_->disallowedHeaders(), *headers_req->mutable_headers()); headers_req->set_end_of_stream(end_stream); - if (proto != nullptr) { - (*headers_req->mutable_attributes())[FilterName] = *proto; - } state.onStartProcessorCall(std::bind(&Filter::onMessageTimeout, this), config_->messageTimeout(), ProcessorState::CallbackState::HeadersCallback); ENVOY_LOG(debug, "Sending headers message"); @@ -317,17 +314,7 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { - ProtobufWkt::Struct proto; - - if (config_->expressionManager().hasRequestExpr()) { - auto activation_ptr = Filters::Common::Expr::createActivation( - &config_->expressionManager().localInfo(), decoding_state_.callbacks()->streamInfo(), - &headers, nullptr, nullptr); - proto = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); - } - - status = onHeaders(decoding_state_, headers, end_stream, - config_->expressionManager().hasRequestExpr() ? &proto : nullptr); + status = onHeaders(decoding_state_, headers, end_stream); ENVOY_LOG(trace, "onHeaders returning {}", static_cast(status)); } else { ENVOY_LOG(trace, "decodeHeaders: Skipped header processing"); @@ -590,7 +577,7 @@ FilterTrailersStatus Filter::onTrailers(ProcessorState& state, Http::HeaderMap& FilterTrailersStatus Filter::decodeTrailers(RequestTrailerMap& trailers) { ENVOY_LOG(trace, "decodeTrailers"); const auto status = onTrailers(decoding_state_, trailers); - ENVOY_LOG(trace, "encodeTrailers returning {}", static_cast(status)); + ENVOY_LOG(trace, "decodeTrailers returning {}", static_cast(status)); return status; } @@ -605,17 +592,7 @@ FilterHeadersStatus Filter::encodeHeaders(ResponseHeaderMap& headers, bool end_s FilterHeadersStatus status = FilterHeadersStatus::Continue; if (!processing_complete_ && encoding_state_.sendHeaders()) { - ProtobufWkt::Struct proto; - - if (config_->expressionManager().hasResponseExpr()) { - auto activation_ptr = Filters::Common::Expr::createActivation( - &config_->expressionManager().localInfo(), encoding_state_.callbacks()->streamInfo(), - nullptr, &headers, nullptr); - proto = config_->expressionManager().evaluateResponseAttributes(*activation_ptr); - } - - status = onHeaders(encoding_state_, headers, end_stream, - config_->expressionManager().hasResponseExpr() ? &proto : nullptr); + status = onHeaders(encoding_state_, headers, end_stream); ENVOY_LOG(trace, "onHeaders returns {}", static_cast(status)); } else { ENVOY_LOG(trace, "encodeHeaders: Skipped header processing"); @@ -650,6 +627,7 @@ ProcessingRequest Filter::setupBodyChunk(ProcessorState& state, const Buffer::In bool end_stream) { ENVOY_LOG(debug, "Sending a body chunk of {} bytes, end_stream {}", data.length(), end_stream); ProcessingRequest req; + addAttributes(state, req); addDynamicMetadata(state, req); auto* body_req = state.mutableBody(req); body_req->set_end_of_stream(end_stream); @@ -667,6 +645,7 @@ void Filter::sendBodyChunk(ProcessorState& state, ProcessorState::CallbackState void Filter::sendTrailers(ProcessorState& state, const Http::HeaderMap& trailers) { ProcessingRequest req; + addAttributes(state, req); addDynamicMetadata(state, req); auto* trailers_req = state.mutableTrailers(req); MutationUtils::headersToProto(trailers, config_->allowedHeaders(), config_->disallowedHeaders(), @@ -771,6 +750,20 @@ void Filter::addDynamicMetadata(const ProcessorState& state, ProcessingRequest& *req.mutable_metadata_context() = forwarding_metadata; } +void Filter::addAttributes(const ProcessorState& state, ProcessingRequest& req) { + if (!state.sendAttributes(config_->expressionManager())) { + return; + } + + auto activation_ptr = Filters::Common::Expr::createActivation( + &config_->expressionManager().localInfo(), state.callbacks()->streamInfo(), + state.requestHeaders(), state.responseHeaders(), state.trailers()); + attributes = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); + + state.sentAttributes(true); + (*req.mutable_attributes())[FilterName] = *attributes; +} + void Filter::setDynamicMetadata(Http::StreamFilterCallbacks* cb, const ProcessorState& state, const ProcessingResponse& response) { if (state.untypedReceivingMetadataNamespaces().empty() || !response.has_dynamic_metadata()) { diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 78d97cbe9bab0..707b4842e3a70 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -373,8 +373,7 @@ 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, - ProtobufWkt::Struct* proto); + Http::RequestOrResponseHeaderMap& headers, bool end_stream); // Return a pair of whether to terminate returning the current result. std::pair sendStreamChunk(ProcessorState& state); @@ -386,6 +385,8 @@ class Filter : public Logger::Loggable, void setDecoderDynamicMetadata(const envoy::service::ext_proc::v3::ProcessingResponse& response); void addDynamicMetadata(const ProcessorState& state, envoy::service::ext_proc::v3::ProcessingRequest& req); + void addAttributes(const ProcessorState& state, + envoy::service::ext_proc::v3::ProcessingRequest& req); const FilterConfigSharedPtr config_; const ExternalProcessorClientPtr client_; diff --git a/source/extensions/filters/http/ext_proc/processor_state.h b/source/extensions/filters/http/ext_proc/processor_state.h index a3ebe27617f0b..9feda7dad84d3 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.h +++ b/source/extensions/filters/http/ext_proc/processor_state.h @@ -15,6 +15,7 @@ #include "source/common/common/logger.h" #include "absl/status/status.h" +#include "matching_utils.h" namespace Envoy { namespace Extensions { @@ -136,6 +137,9 @@ class ProcessorState : public Logger::Loggable { void setHeaders(Http::RequestOrResponseHeaderMap* headers) { headers_ = headers; } void setTrailers(Http::HeaderMap* trailers) { trailers_ = trailers; } + virtual const Http::RequestOrResponseHeaderMap* requestHeaders() const PURE; + virtual const Http::RequestOrResponseHeaderMap* responseHeaders() const PURE; + const Http::HeaderMap* responseTrailers() const { return trailers_; } void onStartProcessorCall(Event::TimerCb cb, std::chrono::milliseconds timeout, CallbackState callback_state); @@ -202,6 +206,10 @@ class ProcessorState : public Logger::Loggable { virtual Http::StreamFilterCallbacks* callbacks() const PURE; + virtual bool sendAttributes(const ExpressionManager& mgr) const PURE; + + void sentAttributes(bool sent) { attributes_sent_ = sent; } + protected: void setBodyMode( envoy::extensions::filters::http::ext_proc::v3::ProcessingMode_BodySendMode body_mode); @@ -250,6 +258,9 @@ class ProcessorState : public Logger::Loggable { const std::vector* typed_forwarding_namespaces_{}; const std::vector* untyped_receiving_namespaces_{}; + // If true, the attributes for this processing state have already been sent. + bool attributes_sent_{}; + private: virtual void clearRouteCache(const envoy::service::ext_proc::v3::CommonResponse&) {} }; @@ -324,6 +335,13 @@ class DecodingProcessorState : public ProcessorState { Http::StreamFilterCallbacks* callbacks() const override { return decoder_callbacks_; } + bool sendAttributes(const ExpressionManager& mgr) const override { + return !attributes_sent_ && mgr.hasRequestExpr(); + } + + const Http::RequestOrResponseHeaderMap* requestHeaders() const override { return headers_; }; + const Http::RequestOrResponseHeaderMap* responseHeaders() const override { return nullptr; } + private: void setProcessingModeInternal( const envoy::extensions::filters::http::ext_proc::v3::ProcessingMode& mode); @@ -404,6 +422,13 @@ class EncodingProcessorState : public ProcessorState { Http::StreamFilterCallbacks* callbacks() const override { return encoder_callbacks_; } + bool sendAttributes(const ExpressionManager& mgr) const override { + return !attributes_sent_ && mgr.hasResponseExpr(); + } + + const Http::RequestOrResponseHeaderMap* requestHeaders() const override { return nullptr; }; + const Http::RequestOrResponseHeaderMap* responseHeaders() const override { return headers_; } + private: void setProcessingModeInternal( const envoy::extensions::filters::http::ext_proc::v3::ProcessingMode& mode); From 212a876ce1f8fc65c4c0f622c0474860c08179d7 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Tue, 6 Feb 2024 12:26:11 -0500 Subject: [PATCH 05/12] re-add HttpHeaders attribbutes field as not-implemented and deprecated Signed-off-by: Jacob Bohanon --- api/envoy/service/ext_proc/v3/external_processor.proto | 8 ++++++-- .../filters/http/ext_proc/ext_proc_integration_test.cc | 5 +---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index 15d0688588659..466666cff58c2 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -199,8 +199,6 @@ message ProcessingResponse { // This message is sent to the external server when the HTTP request and responses // are first received. message HttpHeaders { - reserved 2; - // The HTTP request headers. All header keys will be // lower-cased, because HTTP header keys are case-insensitive. // The ``headers`` encoding is based on the runtime guard @@ -211,6 +209,12 @@ message HttpHeaders { // :ref:`value ` field. config.core.v3.HeaderMap headers = 1; + // [#not-implemented-hide:] + // This field is deprecated and not implemented. Attributes will be sent in + // the top-level :ref:`attributes attributes = 2; + // If true, then there is no message body associated with this // request or response. bool end_of_stream = 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 d0becb1f2bab0..bf9d9d025b1b0 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 @@ -3429,10 +3429,7 @@ TEST_P(ExtProcIntegrationTest, SendAndReceiveDynamicMetadata) { } #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) { +TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { 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"); From d4e3e3fc212ad57c4ab55144fa30ac3fb4c4a6bf Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 7 Feb 2024 08:00:54 -0500 Subject: [PATCH 06/12] PR feedback and fixing integration test Signed-off-by: Jacob Bohanon --- api/envoy/service/ext_proc/v3/BUILD | 1 + .../ext_proc/v3/external_processor.proto | 4 +- .../filters/http/ext_proc/ext_proc.cc | 7 +++- .../filters/http/ext_proc/processor_state.h | 11 +++-- .../ext_proc/ext_proc_integration_test.cc | 41 +++++++++++++++++-- 5 files changed, 54 insertions(+), 10 deletions(-) diff --git a/api/envoy/service/ext_proc/v3/BUILD b/api/envoy/service/ext_proc/v3/BUILD index 0e337d5c3ed11..37704a3249557 100644 --- a/api/envoy/service/ext_proc/v3/BUILD +++ b/api/envoy/service/ext_proc/v3/BUILD @@ -7,6 +7,7 @@ licenses(["notice"]) # Apache 2 api_proto_package( has_services = True, deps = [ + "//envoy/annotations:pkg", "//envoy/config/core/v3:pkg", "//envoy/extensions/filters/http/ext_proc/v3:pkg", "//envoy/type/v3:pkg", diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index 466666cff58c2..638ffbab19696 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -2,6 +2,7 @@ syntax = "proto3"; package envoy.service.ext_proc.v3; +import "envoy/annotations/deprecation.proto"; import "envoy/config/core/v3/base.proto"; import "envoy/extensions/filters/http/ext_proc/v3/processing_mode.proto"; import "envoy/type/v3/http_status.proto"; @@ -213,7 +214,8 @@ message HttpHeaders { // This field is deprecated and not implemented. Attributes will be sent in // the top-level :ref:`attributes attributes = 2; + map attributes = 2 + [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; // If true, then there is no message body associated with this // request or response. diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index f06f5c66621ca..807f9c59c097b 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -312,6 +312,11 @@ FilterHeadersStatus Filter::decodeHeaders(RequestHeaderMap& headers, bool end_st decoding_state_.setCompleteBodyAvailable(true); } + // Set the request headers on decoding and encoding state in case they are + // needed later. + decoding_state_.setRequestHeaders(&headers); + encoding_state_.setRequestHeaders(&headers); + FilterHeadersStatus status = FilterHeadersStatus::Continue; if (decoding_state_.sendHeaders()) { status = onHeaders(decoding_state_, headers, end_stream); @@ -760,7 +765,7 @@ void Filter::addAttributes(const ProcessorState& state, ProcessingRequest& req) state.requestHeaders(), state.responseHeaders(), state.trailers()); attributes = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); - state.sentAttributes(true); + state.setSentAttributes(true); (*req.mutable_attributes())[FilterName] = *attributes; } diff --git a/source/extensions/filters/http/ext_proc/processor_state.h b/source/extensions/filters/http/ext_proc/processor_state.h index 9feda7dad84d3..da6faec33f613 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.h +++ b/source/extensions/filters/http/ext_proc/processor_state.h @@ -135,9 +135,10 @@ class ProcessorState : public Logger::Loggable { return body_mode_; } + void setRequestHeaders(Http::RequestOrResponseHeaderMap* headers) { request_headers_ = headers; } void setHeaders(Http::RequestOrResponseHeaderMap* headers) { headers_ = headers; } void setTrailers(Http::HeaderMap* trailers) { trailers_ = trailers; } - virtual const Http::RequestOrResponseHeaderMap* requestHeaders() const PURE; + const Http::RequestOrResponseHeaderMap* requestHeaders() const { return request_headers_; }; virtual const Http::RequestOrResponseHeaderMap* responseHeaders() const PURE; const Http::HeaderMap* responseTrailers() const { return trailers_; } @@ -208,7 +209,7 @@ class ProcessorState : public Logger::Loggable { virtual bool sendAttributes(const ExpressionManager& mgr) const PURE; - void sentAttributes(bool sent) { attributes_sent_ = sent; } + void setSentAttributes(bool sent) { attributes_sent_ = sent; } protected: void setBodyMode( @@ -244,6 +245,10 @@ class ProcessorState : public Logger::Loggable { // The specific mode for body handling envoy::extensions::filters::http::ext_proc::v3::ProcessingMode_BodySendMode body_mode_; + // The request_headers_ field is guaranteed to hold the pointer to the request + // headers as set in decodeHeaders. This allows both decoding and encoding states + // to have access to the request headers map. + Http::RequestOrResponseHeaderMap* request_headers_ = nullptr; Http::RequestOrResponseHeaderMap* headers_ = nullptr; Http::HeaderMap* trailers_ = nullptr; Event::TimerPtr message_timer_; @@ -339,7 +344,6 @@ class DecodingProcessorState : public ProcessorState { return !attributes_sent_ && mgr.hasRequestExpr(); } - const Http::RequestOrResponseHeaderMap* requestHeaders() const override { return headers_; }; const Http::RequestOrResponseHeaderMap* responseHeaders() const override { return nullptr; } private: @@ -426,7 +430,6 @@ class EncodingProcessorState : public ProcessorState { return !attributes_sent_ && mgr.hasResponseExpr(); } - const Http::RequestOrResponseHeaderMap* requestHeaders() const override { return nullptr; }; const Http::RequestOrResponseHeaderMap* responseHeaders() const override { return headers_; } private: 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 bf9d9d025b1b0..c93e56aa48fb4 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 @@ -3431,7 +3431,9 @@ TEST_P(ExtProcIntegrationTest, SendAndReceiveDynamicMetadata) { #if defined(USE_CEL_PARSER) TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { proto_config_.mutable_processing_mode()->set_request_header_mode(ProcessingMode::SEND); + proto_config_.mutable_processing_mode()->set_request_trailer_mode(ProcessingMode::SEND); proto_config_.mutable_processing_mode()->set_response_header_mode(ProcessingMode::SEND); + proto_config_.mutable_processing_mode()->set_response_trailer_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"); @@ -3442,29 +3444,60 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { initializeConfig(); HttpIntegrationTest::initialize(); auto response = sendDownstreamRequest(absl::nullopt); - processRequestHeadersMessage( - *grpc_upstreams_[0], true, [](const HttpHeaders& req, HeadersResponse&) { + + // Handle request headers message. + processGenericMessage( + *grpc_upstreams_[0], true, [](const ProcessingRequest& req, ProcessingResponse&) { + EXPECT_TRUE(req.has_request_headers()); 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); + + // Make sure we are not including any data in the deprecated HttpHeaders.attributes. + EXPECT_TRUE(req.request_headers().attributes().empty()); return true; }); + // Handle request trailers message, making sure we did not send request attributes again. + processGenericMessage(*grpc_upstreams_[0], false, + [](const ProcessingRequest& req, ProcessingResponse&) { + EXPECT_TRUE(req.has_request_trailers()); + EXPECT_TRUE(req.attributes().empty()); + return true; + }); + handleUpstreamRequest(); - processResponseHeadersMessage( - *grpc_upstreams_[0], false, [](const HttpHeaders& req, HeadersResponse&) { + // Handle response headers message. + processGenericMessage( + *grpc_upstreams_[0], false, [](const ProcessingRequest& req, ProcessingResponse&) { + EXPECT_TRUE(req.has_response_headers()); 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(), StreamInfo::ResponseCodeDetails::get().ViaUpstream); + + // Make sure we didn't include request attributes in the response-path processing request. + EXPECT_FALSE(proto_struct.fields().contains("request.method")); + + // Make sure we are not including any data in the deprecated HttpHeaders.attributes. + EXPECT_TRUE(req.response_headers().attributes().empty()); return true; }); + // Handle response trailers message, making sure we did not send request or response attributes + // again. + processGenericMessage(*grpc_upstreams_[0], false, + [](const ProcessingRequest& req, ProcessingResponse&) { + EXPECT_TRUE(req.has_response_trailers()); + EXPECT_TRUE(req.attributes().empty()); + return true; + }); + verifyDownstreamResponse(*response, 200); } #endif From b8b087f7e86120dbef07f4062e088e2df6463c4c Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 7 Feb 2024 08:20:10 -0500 Subject: [PATCH 07/12] fix proto Signed-off-by: Jacob Bohanon --- api/envoy/service/ext_proc/v3/external_processor.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/service/ext_proc/v3/external_processor.proto b/api/envoy/service/ext_proc/v3/external_processor.proto index 638ffbab19696..21ce87a0cf509 100644 --- a/api/envoy/service/ext_proc/v3/external_processor.proto +++ b/api/envoy/service/ext_proc/v3/external_processor.proto @@ -2,7 +2,6 @@ syntax = "proto3"; package envoy.service.ext_proc.v3; -import "envoy/annotations/deprecation.proto"; import "envoy/config/core/v3/base.proto"; import "envoy/extensions/filters/http/ext_proc/v3/processing_mode.proto"; import "envoy/type/v3/http_status.proto"; @@ -10,6 +9,7 @@ import "envoy/type/v3/http_status.proto"; import "google/protobuf/duration.proto"; import "google/protobuf/struct.proto"; +import "envoy/annotations/deprecation.proto"; import "udpa/annotations/status.proto"; import "validate/validate.proto"; From 1f4df2d36b02594bdb3882cc625df111d8b94e3f Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 7 Feb 2024 08:55:00 -0500 Subject: [PATCH 08/12] build fixes Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/ext_proc.cc | 8 ++++---- source/extensions/filters/http/ext_proc/ext_proc.h | 3 +-- 2 files changed, 5 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 807f9c59c097b..60e9e3e4516ca 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -755,18 +755,18 @@ void Filter::addDynamicMetadata(const ProcessorState& state, ProcessingRequest& *req.mutable_metadata_context() = forwarding_metadata; } -void Filter::addAttributes(const ProcessorState& state, ProcessingRequest& req) { +void Filter::addAttributes(ProcessorState& state, ProcessingRequest& req) { if (!state.sendAttributes(config_->expressionManager())) { return; } auto activation_ptr = Filters::Common::Expr::createActivation( &config_->expressionManager().localInfo(), state.callbacks()->streamInfo(), - state.requestHeaders(), state.responseHeaders(), state.trailers()); - attributes = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); + state.requestHeaders(), state.responseHeaders(), state.responseTrailers()); + auto attributes = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); state.setSentAttributes(true); - (*req.mutable_attributes())[FilterName] = *attributes; + (*req.mutable_attributes())[FilterName] = attributes; } void Filter::setDynamicMetadata(Http::StreamFilterCallbacks* cb, const ProcessorState& state, diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 707b4842e3a70..1ef14a7c2e810 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -385,8 +385,7 @@ class Filter : public Logger::Loggable, void setDecoderDynamicMetadata(const envoy::service::ext_proc::v3::ProcessingResponse& response); void addDynamicMetadata(const ProcessorState& state, envoy::service::ext_proc::v3::ProcessingRequest& req); - void addAttributes(const ProcessorState& state, - envoy::service::ext_proc::v3::ProcessingRequest& req); + void addAttributes(ProcessorState& state, envoy::service::ext_proc::v3::ProcessingRequest& req); const FilterConfigSharedPtr config_; const ExternalProcessorClientPtr client_; From dc9b5a2b413d7c826fc8f92b4c1cf89b7104dffa Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 7 Feb 2024 09:20:46 -0500 Subject: [PATCH 09/12] build fixes Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/processor_state.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/processor_state.h b/source/extensions/filters/http/ext_proc/processor_state.h index da6faec33f613..844474f697107 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.h +++ b/source/extensions/filters/http/ext_proc/processor_state.h @@ -135,10 +135,10 @@ class ProcessorState : public Logger::Loggable { return body_mode_; } - void setRequestHeaders(Http::RequestOrResponseHeaderMap* headers) { request_headers_ = headers; } + void setRequestHeaders(Http::RequestHeaderMap* headers) { request_headers_ = headers; } void setHeaders(Http::RequestOrResponseHeaderMap* headers) { headers_ = headers; } void setTrailers(Http::HeaderMap* trailers) { trailers_ = trailers; } - const Http::RequestOrResponseHeaderMap* requestHeaders() const { return request_headers_; }; + const Http::RequestHeaderMap* requestHeaders() const { return request_headers_; }; virtual const Http::RequestOrResponseHeaderMap* responseHeaders() const PURE; const Http::HeaderMap* responseTrailers() const { return trailers_; } @@ -248,7 +248,7 @@ class ProcessorState : public Logger::Loggable { // The request_headers_ field is guaranteed to hold the pointer to the request // headers as set in decodeHeaders. This allows both decoding and encoding states // to have access to the request headers map. - Http::RequestOrResponseHeaderMap* request_headers_ = nullptr; + Http::RequestHeaderMap* request_headers_ = nullptr; Http::RequestOrResponseHeaderMap* headers_ = nullptr; Http::HeaderMap* trailers_ = nullptr; Event::TimerPtr message_timer_; From f90f7b2023ef41740425d3c50388b32eefbb8582 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Wed, 7 Feb 2024 12:23:16 -0500 Subject: [PATCH 10/12] build fixes Signed-off-by: Jacob Bohanon --- source/extensions/filters/http/ext_proc/ext_proc.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 60e9e3e4516ca..3503821d59d64 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 "envoy/config/core/v3/grpc_service.pb.h" #include "envoy/extensions/filters/http/ext_proc/v3/processing_mode.pb.h" @@ -762,7 +764,8 @@ void Filter::addAttributes(ProcessorState& state, ProcessingRequest& req) { auto activation_ptr = Filters::Common::Expr::createActivation( &config_->expressionManager().localInfo(), state.callbacks()->streamInfo(), - state.requestHeaders(), state.responseHeaders(), state.responseTrailers()); + state.requestHeaders(), dynamic_cast(state.responseHeaders()), + dynamic_cast(state.responseTrailers())); auto attributes = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); state.setSentAttributes(true); From 56e76b654fa40a871d91ae8702fdad8b0da58da7 Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Thu, 8 Feb 2024 07:27:35 -0500 Subject: [PATCH 11/12] fix integration test Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc.cc | 2 +- .../filters/http/ext_proc/processor_state.h | 15 +++++++++ test/extensions/filters/http/ext_proc/BUILD | 1 + .../ext_proc/ext_proc_integration_test.cc | 31 ++++++++++--------- 4 files changed, 34 insertions(+), 15 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 3503821d59d64..3e68fcae75688 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -766,7 +766,7 @@ void Filter::addAttributes(ProcessorState& state, ProcessingRequest& req) { &config_->expressionManager().localInfo(), state.callbacks()->streamInfo(), state.requestHeaders(), dynamic_cast(state.responseHeaders()), dynamic_cast(state.responseTrailers())); - auto attributes = config_->expressionManager().evaluateRequestAttributes(*activation_ptr); + auto attributes = state.evaluateAttributes(config_->expressionManager(), *activation_ptr); state.setSentAttributes(true); (*req.mutable_attributes())[FilterName] = attributes; diff --git a/source/extensions/filters/http/ext_proc/processor_state.h b/source/extensions/filters/http/ext_proc/processor_state.h index 844474f697107..01e9bc57ae59f 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.h +++ b/source/extensions/filters/http/ext_proc/processor_state.h @@ -211,6 +211,10 @@ class ProcessorState : public Logger::Loggable { void setSentAttributes(bool sent) { attributes_sent_ = sent; } + virtual ProtobufWkt::Struct + evaluateAttributes(const ExpressionManager& mgr, + const Filters::Common::Expr::Activation& activation) const PURE; + protected: void setBodyMode( envoy::extensions::filters::http::ext_proc::v3::ProcessingMode_BodySendMode body_mode); @@ -345,6 +349,11 @@ class DecodingProcessorState : public ProcessorState { } const Http::RequestOrResponseHeaderMap* responseHeaders() const override { return nullptr; } + ProtobufWkt::Struct + evaluateAttributes(const ExpressionManager& mgr, + const Filters::Common::Expr::Activation& activation) const override { + return mgr.evaluateRequestAttributes(activation); + } private: void setProcessingModeInternal( @@ -432,6 +441,12 @@ class EncodingProcessorState : public ProcessorState { const Http::RequestOrResponseHeaderMap* responseHeaders() const override { return headers_; } + ProtobufWkt::Struct + evaluateAttributes(const ExpressionManager& mgr, + const Filters::Common::Expr::Activation& activation) const override { + return mgr.evaluateResponseAttributes(activation); + } + 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 9d511fbd9d8a9..13971f8250e43 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -158,6 +158,7 @@ envoy_extension_cc_test( "//test/proto:helloworld_proto_cc_proto", "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/ext_proc/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/set_metadata/v3:pkg_cc_proto", "@envoy_api//envoy/service/ext_proc/v3:pkg_cc_proto", 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 c93e56aa48fb4..0177bd4d65b62 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 @@ -1,6 +1,7 @@ #include #include +#include "envoy/config/core/v3/base.pb.h" #include "envoy/extensions/filters/http/ext_proc/v3/ext_proc.pb.h" #include "envoy/extensions/filters/http/set_metadata/v3/set_metadata.pb.h" #include "envoy/network/address.h" @@ -293,7 +294,6 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, ASSERT_TRUE(processor_connection_->waitForNewStream(*dispatcher_, processor_stream_)); } ASSERT_TRUE(processor_stream_->waitForGrpcMessage(*dispatcher_, request)); - ASSERT_TRUE(request.has_request_headers()); if (first_message) { processor_stream_->startGrpcStream(); } @@ -3431,7 +3431,6 @@ TEST_P(ExtProcIntegrationTest, SendAndReceiveDynamicMetadata) { #if defined(USE_CEL_PARSER) TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { proto_config_.mutable_processing_mode()->set_request_header_mode(ProcessingMode::SEND); - proto_config_.mutable_processing_mode()->set_request_trailer_mode(ProcessingMode::SEND); proto_config_.mutable_processing_mode()->set_response_header_mode(ProcessingMode::SEND); proto_config_.mutable_processing_mode()->set_response_trailer_mode(ProcessingMode::SEND); proto_config_.mutable_request_attributes()->Add("request.path"); @@ -3447,7 +3446,11 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { // Handle request headers message. processGenericMessage( - *grpc_upstreams_[0], true, [](const ProcessingRequest& req, ProcessingResponse&) { + *grpc_upstreams_[0], true, [](const ProcessingRequest& req, ProcessingResponse& resp) { + // Add something to the response so the message isn't seen as spurious + envoy::service::ext_proc::v3::HeadersResponse headers_resp; + *(resp.mutable_request_headers()) = headers_resp; + EXPECT_TRUE(req.has_request_headers()); EXPECT_EQ(req.attributes().size(), 1); auto proto_struct = req.attributes().at("envoy.filters.http.ext_proc"); @@ -3461,19 +3464,15 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { return true; }); - // Handle request trailers message, making sure we did not send request attributes again. - processGenericMessage(*grpc_upstreams_[0], false, - [](const ProcessingRequest& req, ProcessingResponse&) { - EXPECT_TRUE(req.has_request_trailers()); - EXPECT_TRUE(req.attributes().empty()); - return true; - }); - - handleUpstreamRequest(); + handleUpstreamRequestWithTrailer(); // Handle response headers message. processGenericMessage( - *grpc_upstreams_[0], false, [](const ProcessingRequest& req, ProcessingResponse&) { + *grpc_upstreams_[0], false, [](const ProcessingRequest& req, ProcessingResponse& resp) { + // Add something to the response so the message isn't seen as spurious + envoy::service::ext_proc::v3::HeadersResponse headers_resp; + *(resp.mutable_response_headers()) = headers_resp; + EXPECT_TRUE(req.has_response_headers()); EXPECT_EQ(req.attributes().size(), 1); auto proto_struct = req.attributes().at("envoy.filters.http.ext_proc"); @@ -3492,7 +3491,11 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { // Handle response trailers message, making sure we did not send request or response attributes // again. processGenericMessage(*grpc_upstreams_[0], false, - [](const ProcessingRequest& req, ProcessingResponse&) { + [](const ProcessingRequest& req, ProcessingResponse& resp) { + // Add something to the response so the message isn't seen as spurious + envoy::service::ext_proc::v3::TrailersResponse trailer_resp; + *(resp.mutable_response_trailers()) = trailer_resp; + EXPECT_TRUE(req.has_response_trailers()); EXPECT_TRUE(req.attributes().empty()); return true; From fe0d21f4ed49c42047c080ee94c230cedd352a8f Mon Sep 17 00:00:00 2001 From: Jacob Bohanon Date: Fri, 9 Feb 2024 11:59:47 -0500 Subject: [PATCH 12/12] check behavior when trying to send unavailable attribute Signed-off-by: Jacob Bohanon --- .../filters/http/ext_proc/ext_proc_integration_test.cc | 4 ++++ 1 file changed, 4 insertions(+) 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 0177bd4d65b62..fb936314551db 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 @@ -3437,6 +3437,7 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { 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_request_attributes()->Add("response.code"); proto_config_.mutable_response_attributes()->Add("response.code"); proto_config_.mutable_response_attributes()->Add("response.code_details"); @@ -3458,6 +3459,9 @@ TEST_P(ExtProcIntegrationTest, RequestResponseAttributes) { 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); + // Make sure we did not include the attribute which was not yet available. + EXPECT_EQ(proto_struct.fields().size(), 4); + EXPECT_FALSE(proto_struct.fields().contains("response.code")); // Make sure we are not including any data in the deprecated HttpHeaders.attributes. EXPECT_TRUE(req.request_headers().attributes().empty());