From 01acc4d686912f256b92f9100ae990fda0c02f07 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Fri, 22 Dec 2023 17:38:24 +0000 Subject: [PATCH 01/11] ext_proc: allow override metadata without grpc_service allows `overrides.metadata` in `ExtProcPerRoute` that appends and overrides parent metadata: - match: ... route: ... typed_per_filter_config: envoy.filters.http.ext_proc: "@type": type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExtProcPerRoute overrides: metadata: - key: "x-ext-proc-override" value: "route3-override" - key: "x-router3-metadata-append" value: "route3-metadata-append" http_filters: - name: envoy.filters.http.ext_proc typed_config: "@type": type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExternalProcessor grpc_service: envoy_grpc: cluster_name: ext-proc-proxy timeout: 60s initial_metadata: - key: "x-ext-proc-override" value: "root" - key: "x-ext-proc-original" value: "root" that allows to have: x-ext-proc-override: route3-override x-router3-metadata-append: route3-metadata-append x-ext-proc-original: root Signed-off-by: Ivan Prisyazhnyy --- .../filters/http/ext_proc/v3/ext_proc.proto | 9 ++- changelogs/current.yaml | 7 ++ .../filters/http/ext_proc/ext_proc.cc | 66 ++++++++++++++++++- .../filters/http/ext_proc/ext_proc.h | 2 + 4 files changed, 81 insertions(+), 3 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..31fa4f07aa21 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 @@ -3,6 +3,7 @@ syntax = "proto3"; package envoy.extensions.filters.http.ext_proc.v3; import "envoy/config/common/mutation_rules/v3/mutation_rules.proto"; +import "envoy/config/core/v3/base.proto"; import "envoy/config/core/v3/grpc_service.proto"; import "envoy/extensions/filters/http/ext_proc/v3/processing_mode.proto"; import "envoy/type/matcher/v3/string.proto"; @@ -248,7 +249,7 @@ message ExtProcPerRoute { } // Overrides that may be set on a per-route basis -// [#next-free-field: 6] +// [#next-free-field: 7] message ExtProcOverrides { // Set a different processing mode for this route than the default. ProcessingMode processing_mode = 1; @@ -269,4 +270,10 @@ message ExtProcOverrides { // Set a different gRPC service for this route than the default. config.core.v3.GrpcService grpc_service = 5; + + // Additional metadata to include into streams initiated to the ext_proc gRPC + // service. This can be used for scenarios in which additional ad hoc + // authorization headers (e.g. ``x-foo-bar: baz-key``) are to be injected or + // when a route needs to partially override inherited metadata. + repeated config.core.v3.HeaderValue metadata = 6; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 3087384716ce..f5891cdb9019 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -368,6 +368,13 @@ new_features: change: | Added support for emitting per opcode decoder error metrics via :ref:`enable_per_opcode_decoder_error_metrics `. +- area: ext_proc + change: | + Added + :ref:`metadata ` + config API to allow extending inherited metadata from ExternalProcessor + :ref:`grpc_service ` + with the new or updated values. deprecated: - area: wasm diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 82b6848f9d3f..8bec96dc2390 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -129,6 +129,14 @@ FilterConfigPerRoute::initGrpcService(const ExtProcPerRoute& config) { } return absl::nullopt; } +static std::vector +initMetadata(const ExtProcPerRoute& config) { + std::vector metadata; + for (const auto& header : config.overrides().metadata()) { + metadata.emplace_back(header); + } + return metadata; +} absl::optional FilterConfigPerRoute::mergeProcessingMode(const FilterConfigPerRoute& less_specific, @@ -140,16 +148,44 @@ FilterConfigPerRoute::mergeProcessingMode(const FilterConfigPerRoute& less_speci : less_specific.processingMode(); } +// replace all entries with the same name or append one. +static void mergeHeaderValue(std::vector& metadata, + const envoy::config::core::v3::HeaderValue& header) { + size_t count = 0; + for (auto& dest : metadata) { + if (dest.key() == header.key()) { + dest.CopyFrom(header); + count++; + } + } + if (!count) { + metadata.emplace_back(header); + } +} + +static std::vector +mergeMetadata(const FilterConfigPerRoute& less_specific, + const FilterConfigPerRoute& more_specific) { + std::vector metadata(less_specific.metadata()); + + for (const auto& header : more_specific.metadata()) { + mergeHeaderValue(metadata, header); + } + + return metadata; +} + FilterConfigPerRoute::FilterConfigPerRoute(const ExtProcPerRoute& config) : disabled_(config.disabled()), processing_mode_(initProcessingMode(config)), - grpc_service_(initGrpcService(config)) {} + grpc_service_(initGrpcService(config)), metadata_(initMetadata(config)) {} FilterConfigPerRoute::FilterConfigPerRoute(const FilterConfigPerRoute& less_specific, const FilterConfigPerRoute& more_specific) : disabled_(more_specific.disabled()), processing_mode_(mergeProcessingMode(less_specific, more_specific)), grpc_service_(more_specific.grpcService().has_value() ? more_specific.grpcService() - : less_specific.grpcService()) {} + : less_specific.grpcService()), + metadata_(mergeMetadata(less_specific, more_specific)) {} void Filter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { Http::PassThroughFilter::setDecoderFilterCallbacks(callbacks); @@ -866,6 +902,22 @@ static ProcessingMode allDisabledMode() { return pm; } +// replace all entries with the same name or append one. +static void +mergeHeaderValue(Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, + const envoy::config::core::v3::HeaderValue& header) { + size_t count = 0; + for (auto& dest : metadata) { + if (dest.key() == header.key()) { + dest.CopyFrom(header); + count++; + } + } + if (!count) { + metadata.Add()->CopyFrom(header); + } +} + void Filter::mergePerRouteConfig() { if (route_config_merged_) { return; @@ -912,6 +964,16 @@ void Filter::mergePerRouteConfig() { grpc_service_ = *merged_config->grpcService(); config_with_hash_key_.setConfig(*merged_config->grpcService()); } + if (!merged_config->metadata().empty()) { + ENVOY_LOG(trace, "Overriding metadata from per-route configuration"); + envoy::config::core::v3::GrpcService config = config_with_hash_key_.config(); + auto ptr = config.mutable_initial_metadata(); + for (const auto& header : merged_config->metadata()) { + ENVOY_LOG(trace, "Setting metadata {} = {}", header.key(), header.value()); + mergeHeaderValue(*ptr, header); + } + config_with_hash_key_.setConfig(config); + } } std::string responseCaseToString(const ProcessingResponse::ResponseCase response_case) { diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 20a2e676719f..00de3b049ad5 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -224,6 +224,7 @@ class FilterConfigPerRoute : public Router::RouteSpecificFilterConfig { const absl::optional& grpcService() const { return grpc_service_; } + const std::vector& metadata() const { return metadata_; } private: absl::optional @@ -240,6 +241,7 @@ class FilterConfigPerRoute : public Router::RouteSpecificFilterConfig { const absl::optional processing_mode_; const absl::optional grpc_service_; + std::vector metadata_; }; class Filter : public Logger::Loggable, From 143ef4a82d26a75ccd460563f5e48044607883c5 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Tue, 26 Dec 2023 16:37:05 +0000 Subject: [PATCH 02/11] test: add ext_proc per route overrides.metadata test - TEST(OverrideTest, MetadataOverride) - TEST_F(HttpFilterTest, GrpcServiceMetadataOverride) - TEST_P(ExtProcIntegrationTest, PerRouteMetadata) $ bazel test //test/extensions/filters/http/ext_proc:config_test --config=clang --verbose_failures --test_arg="-l trace" $ bazel test //test/extensions/filters/http/ext_proc:filter_test --config=clang --verbose_failures --test_arg="-l trace" $ bazel test //test/extensions/filters/http/ext_proc:ext_proc_integration_test --config=clang --verbose_failures --test_arg=--gtest_filter='*/ExtProcIntegrationTest.PerRouteMetadata/*' --test_arg="-l trace" Signed-off-by: Ivan Prisyazhnyy --- .../filters/http/ext_proc/ext_proc.h | 3 + .../filters/http/ext_proc/config_test.cc | 17 +++++ .../ext_proc/ext_proc_integration_test.cc | 30 ++++++++ .../filters/http/ext_proc/filter_test.cc | 72 +++++++++++++++++++ .../extensions/filters/http/ext_proc/utils.cc | 8 +++ test/extensions/filters/http/ext_proc/utils.h | 2 + 6 files changed, 132 insertions(+) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 00de3b049ad5..39a2ac9e53c8 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -268,6 +268,9 @@ class Filter : public Logger::Loggable, encoding_state_(*this, config->processingMode()) {} const FilterConfig& config() const { return *config_; } + const envoy::config::core::v3::GrpcService& grpc_service_config() const { + return config_with_hash_key_.config(); + } ExtProcFilterStats& stats() { return stats_; } ExtProcLoggingInfo* loggingInfo() { return logging_info_; } diff --git a/test/extensions/filters/http/ext_proc/config_test.cc b/test/extensions/filters/http/ext_proc/config_test.cc index 6cfa1d9196a9..c8045fef0a9e 100644 --- a/test/extensions/filters/http/ext_proc/config_test.cc +++ b/test/extensions/filters/http/ext_proc/config_test.cc @@ -80,6 +80,23 @@ TEST(HttpExtProcConfigTest, CorrectConfigServerContext) { cb(filter_callback); } +TEST(HttpExtProcConfigTest, CorrectRouteMetadataOnlyConfig) { + std::string yaml = R"EOF( + overrides: + metadata: + - key: "a" + value: "a" + )EOF"; + + ExternalProcessingFilterConfig factory; + ProtobufTypes::MessagePtr proto_config = factory.createEmptyRouteConfigProto(); + TestUtility::loadFromYaml(yaml, *proto_config); + + testing::NiceMock context; + Router::RouteSpecificFilterConfigConstSharedPtr cb = factory.createRouteSpecificFilterConfig( + *proto_config, context, context.messageValidationVisitor()); +} + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters 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 b75f2eb4e142..92e7cefacd6a 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 @@ -38,6 +38,7 @@ using envoy::service::ext_proc::v3::ProcessingResponse; using envoy::service::ext_proc::v3::TrailersResponse; using Extensions::HttpFilters::ExternalProcessing::HasNoHeader; using Extensions::HttpFilters::ExternalProcessing::HeaderProtosEqual; +using Extensions::HttpFilters::ExternalProcessing::makeHeaderValue; using Extensions::HttpFilters::ExternalProcessing::SingleHeaderValueIs; using Http::LowerCaseString; @@ -2503,6 +2504,35 @@ TEST_P(ExtProcIntegrationTest, PerRouteGrpcService) { EXPECT_THAT(response->headers(), SingleHeaderValueIs("x-response-processed", "1")); } +// Set up per-route configuration that extends original metadata. +TEST_P(ExtProcIntegrationTest, PerRouteMetadata) { + initializeConfig(); + + // Override metadata from route config. + config_helper_.addConfigModifier([this](HttpConnectionManager& cm) { + // Set up "/foo" so that it will use a different GrpcService + auto* vh = cm.mutable_route_config()->mutable_virtual_hosts()->Mutable(0); + auto* route = vh->mutable_routes()->Mutable(0); + route->mutable_match()->set_path("/foo"); + ExtProcPerRoute per_route; + *per_route.mutable_overrides()->mutable_metadata()->Add() = makeHeaderValue("b", "c"); + *per_route.mutable_overrides()->mutable_metadata()->Add() = makeHeaderValue("c", "c"); + setPerRouteConfig(route, per_route); + }); + + HttpIntegrationTest::initialize(); + + // Request that matches route directed to ext_proc_server_0 + auto response = + sendDownstreamRequest([](Http::RequestHeaderMap& headers) { headers.setPath("/foo"); }); + + processRequestHeadersMessage(*grpc_upstreams_[0], true, absl::nullopt); + handleUpstreamRequest(); + + processResponseHeadersMessage(*grpc_upstreams_[0], false, absl::nullopt); + verifyDownstreamResponse(*response, 200); +} + // Sending new timeout API in both downstream request and upstream response // handling path with header mutation. TEST_P(ExtProcIntegrationTest, RequestAndResponseMessageNewTimeoutWithHeaderMutation) { diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 47865bdbf334..b36942135c13 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -2979,6 +2979,23 @@ TEST(OverrideTest, GrpcServiceNonOverride) { EXPECT_THAT(*merged_route.grpcService(), ProtoEq(cfg1.overrides().grpc_service())); } +// When merging two configurations, second metadata override only extends the first's one. +TEST(OverrideTest, MetadataOverride) { + ExtProcPerRoute cfg1; + cfg1.mutable_overrides()->mutable_metadata()->Add()->CopyFrom(makeHeaderValue("a", "a")); + cfg1.mutable_overrides()->mutable_metadata()->Add()->CopyFrom(makeHeaderValue("b", "b")); + ExtProcPerRoute cfg2; + cfg2.mutable_overrides()->mutable_metadata()->Add()->CopyFrom(makeHeaderValue("b", "c")); + cfg2.mutable_overrides()->mutable_metadata()->Add()->CopyFrom(makeHeaderValue("c", "c")); + FilterConfigPerRoute route1(cfg1); + FilterConfigPerRoute route2(cfg2); + FilterConfigPerRoute merged_route(route1, route2); + ASSERT_TRUE(merged_route.metadata().size() == 3); + EXPECT_THAT(merged_route.metadata()[0], ProtoEq(cfg1.overrides().metadata()[0])); + EXPECT_THAT(merged_route.metadata()[1], ProtoEq(cfg2.overrides().metadata()[0])); + EXPECT_THAT(merged_route.metadata()[2], ProtoEq(cfg2.overrides().metadata()[1])); +} + // Verify that attempts to change headers that are not allowed to be changed // are ignored and a counter is incremented. TEST_F(HttpFilterTest, IgnoreInvalidHeaderMutations) { @@ -3302,6 +3319,61 @@ TEST_F(HttpFilter2Test, LastEncodeDataCallExceedsStreamBufferLimitWouldJustRaise conn_manager_->onData(fake_input, false); } +// Test that per route metadata override does override inherited grpc_service configuration. +TEST_F(HttpFilterTest, GrpcServiceMetadataOverride) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + initial_metadata: + - key: "a" + value: "a" + - key: "b" + value: "b" + )EOF"); + + // Route configuration overrides the grpc_service metadata. + ExtProcPerRoute route_proto; + *route_proto.mutable_overrides()->mutable_metadata()->Add() = makeHeaderValue("b", "c"); + *route_proto.mutable_overrides()->mutable_metadata()->Add() = makeHeaderValue("c", "c"); + FilterConfigPerRoute route_config(route_proto); + EXPECT_CALL(decoder_callbacks_, traversePerFilterConfig(_)) + .WillOnce( + testing::Invoke([&](std::function cb) { + cb(route_config); + })); + + // Build expected merged grpc_service configuration. + { + std::string expected_config = (R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + initial_metadata: + - key: "a" + value: "a" + - key: "b" + value: "c" + - key: "c" + value: "c" + )EOF"); + envoy::extensions::filters::http::ext_proc::v3::ExternalProcessor expected_proto{}; + TestUtility::loadFromYaml(expected_config, expected_proto); + final_expected_grpc_service_.emplace(expected_proto.grpc_service()); + config_with_hash_key_.setConfig(expected_proto.grpc_service()); + } + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); + processRequestHeaders(false, absl::nullopt); + + const auto& meta = filter_->grpc_service_config().initial_metadata(); + EXPECT_EQ(meta[0].value(), "a"); // a = a inherited + EXPECT_EQ(meta[1].value(), "c"); // b = c overridden + EXPECT_EQ(meta[2].value(), "c"); // c = c added + + filter_->onDestroy(); +} + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters diff --git a/test/extensions/filters/http/ext_proc/utils.cc b/test/extensions/filters/http/ext_proc/utils.cc index 0e322535c08a..e3723b8b728b 100644 --- a/test/extensions/filters/http/ext_proc/utils.cc +++ b/test/extensions/filters/http/ext_proc/utils.cc @@ -29,6 +29,14 @@ bool ExtProcTestUtility::headerProtosEqualIgnoreOrder( return TestUtility::headerMapEqualIgnoreOrder(expected, actual_headers); } +envoy::config::core::v3::HeaderValue makeHeaderValue(const std::string& key, + const std::string& value) { + envoy::config::core::v3::HeaderValue v; + v.set_key(key); + v.set_value(value); + return v; +} + } // namespace ExternalProcessing } // namespace HttpFilters } // namespace Extensions diff --git a/test/extensions/filters/http/ext_proc/utils.h b/test/extensions/filters/http/ext_proc/utils.h index ba88559b9c74..858e8e7cb01d 100644 --- a/test/extensions/filters/http/ext_proc/utils.h +++ b/test/extensions/filters/http/ext_proc/utils.h @@ -50,6 +50,8 @@ MATCHER_P2(SingleProtoHeaderValueIs, key, value, return false; } +envoy::config::core::v3::HeaderValue makeHeaderValue(const std::string& key, + const std::string& value); } // namespace ExternalProcessing } // namespace HttpFilters } // namespace Extensions From 4b34b95d24cfd8478806f67d66635aba0baa7afd Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Thu, 18 Jan 2024 15:08:21 +0000 Subject: [PATCH 03/11] fix: move metadata ext_proc static fns to anon namespace Signed-off-by: Ivan Prisyazhnyy --- .../filters/http/ext_proc/ext_proc.cc | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 8bec96dc2390..ce8b2fcf5764 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -129,14 +129,15 @@ FilterConfigPerRoute::initGrpcService(const ExtProcPerRoute& config) { } return absl::nullopt; } -static std::vector -initMetadata(const ExtProcPerRoute& config) { +namespace { +std::vector initMetadata(const ExtProcPerRoute& config) { std::vector metadata; for (const auto& header : config.overrides().metadata()) { metadata.emplace_back(header); } return metadata; } +} // namespace absl::optional FilterConfigPerRoute::mergeProcessingMode(const FilterConfigPerRoute& less_specific, @@ -148,9 +149,10 @@ FilterConfigPerRoute::mergeProcessingMode(const FilterConfigPerRoute& less_speci : less_specific.processingMode(); } +namespace { // replace all entries with the same name or append one. -static void mergeHeaderValue(std::vector& metadata, - const envoy::config::core::v3::HeaderValue& header) { +void mergeHeaderValue(std::vector& metadata, + const envoy::config::core::v3::HeaderValue& header) { size_t count = 0; for (auto& dest : metadata) { if (dest.key() == header.key()) { @@ -163,7 +165,7 @@ static void mergeHeaderValue(std::vector& } } -static std::vector +std::vector mergeMetadata(const FilterConfigPerRoute& less_specific, const FilterConfigPerRoute& more_specific) { std::vector metadata(less_specific.metadata()); @@ -174,6 +176,7 @@ mergeMetadata(const FilterConfigPerRoute& less_specific, return metadata; } +} // namespace FilterConfigPerRoute::FilterConfigPerRoute(const ExtProcPerRoute& config) : disabled_(config.disabled()), processing_mode_(initProcessingMode(config)), @@ -902,10 +905,10 @@ static ProcessingMode allDisabledMode() { return pm; } +namespace { // replace all entries with the same name or append one. -static void -mergeHeaderValue(Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, - const envoy::config::core::v3::HeaderValue& header) { +void mergeHeaderValue(Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, + const envoy::config::core::v3::HeaderValue& header) { size_t count = 0; for (auto& dest : metadata) { if (dest.key() == header.key()) { @@ -917,6 +920,7 @@ mergeHeaderValue(Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderVal metadata.Add()->CopyFrom(header); } } +} // namespace void Filter::mergePerRouteConfig() { if (route_config_merged_) { From a8f22c874b33bd6315200631f70f0ce7dba867d3 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Thu, 25 Jan 2024 12:13:11 +0000 Subject: [PATCH 04/11] move anon namespace to the top with all statics Signed-off-by: Ivan Prisyazhnyy --- .../filters/http/ext_proc/ext_proc.cc | 176 ++++++++---------- .../filters/http/ext_proc/ext_proc.h | 10 - 2 files changed, 81 insertions(+), 105 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index ce8b2fcf5764..3016b99fe16e 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -14,6 +14,11 @@ namespace Envoy { namespace Extensions { namespace HttpFilters { namespace ExternalProcessing { +namespace { + +const std::string ErrorPrefix = "ext_proc_error"; +const int DefaultImmediateStatus = 200; +const std::string FilterName = "envoy.filters.http.ext_proc"; using envoy::config::common::mutation_rules::v3::HeaderMutationRules; using envoy::extensions::filters::http::ext_proc::v3::ExtProcPerRoute; @@ -33,9 +38,67 @@ using Http::RequestTrailerMap; using Http::ResponseHeaderMap; using Http::ResponseTrailerMap; -static const std::string ErrorPrefix = "ext_proc_error"; -static const int DefaultImmediateStatus = 200; -static const std::string FilterName = "envoy.filters.http.ext_proc"; +absl::optional initProcessingMode(const ExtProcPerRoute& config) { + if (!config.disabled() && config.has_overrides() && config.overrides().has_processing_mode()) { + return config.overrides().processing_mode(); + } + return absl::nullopt; +} +absl::optional +initGrpcService(const ExtProcPerRoute& config) { + if (config.has_overrides() && config.overrides().has_grpc_service()) { + return config.overrides().grpc_service(); + } + return absl::nullopt; +} + +absl::optional mergeProcessingMode(const FilterConfigPerRoute& less_specific, + const FilterConfigPerRoute& more_specific) { + if (more_specific.disabled()) { + return absl::nullopt; + } + return more_specific.processingMode().has_value() ? more_specific.processingMode() + : less_specific.processingMode(); +} +// replace all entries with the same name or append one. +void mergeHeaderValue(std::vector& metadata, + const envoy::config::core::v3::HeaderValue& header) { + size_t count = 0; + for (auto& dest : metadata) { + if (dest.key() == header.key()) { + dest.CopyFrom(header); + count++; + } + } + if (!count) { + metadata.emplace_back(header); + } +} +std::vector +mergeMetadata(const FilterConfigPerRoute& less_specific, + const FilterConfigPerRoute& more_specific) { + std::vector metadata(less_specific.metadata()); + + for (const auto& header : more_specific.metadata()) { + mergeHeaderValue(metadata, header); + } + + return metadata; +} +// replace all entries with the same name or append one. +void mergeHeaderValue(Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, + const envoy::config::core::v3::HeaderValue& header) { + bool count = false; + for (auto& dest : metadata) { + if (dest.key() == header.key()) { + dest.CopyFrom(header); + count = true; + } + } + if (!count) { + metadata.Add()->CopyFrom(header); + } +} // Changes to headers are normally tested against the MutationRules supplied // with configuration. When writing an immediate response message, however, @@ -58,6 +121,19 @@ class ImmediateMutationChecker { std::unique_ptr rule_checker_; }; +const ImmediateMutationChecker& immediateResponseChecker() { + CONSTRUCT_ON_FIRST_USE(ImmediateMutationChecker); +} + +ProcessingMode allDisabledMode() { + ProcessingMode pm; + pm.set_request_header_mode(ProcessingMode::SKIP); + pm.set_response_header_mode(ProcessingMode::SKIP); + return pm; +} + +} // namespace + void ExtProcLoggingInfo::recordGrpcCall( std::chrono::microseconds latency, Grpc::Status::GrpcStatus call_status, ProcessorState::CallbackState callback_state, @@ -115,72 +191,10 @@ ExtProcLoggingInfo::grpcCalls(envoy::config::core::v3::TrafficDirection traffic_ : encoding_processor_grpc_calls_; } -absl::optional -FilterConfigPerRoute::initProcessingMode(const ExtProcPerRoute& config) { - if (!config.disabled() && config.has_overrides() && config.overrides().has_processing_mode()) { - return config.overrides().processing_mode(); - } - return absl::nullopt; -} -absl::optional -FilterConfigPerRoute::initGrpcService(const ExtProcPerRoute& config) { - if (config.has_overrides() && config.overrides().has_grpc_service()) { - return config.overrides().grpc_service(); - } - return absl::nullopt; -} -namespace { -std::vector initMetadata(const ExtProcPerRoute& config) { - std::vector metadata; - for (const auto& header : config.overrides().metadata()) { - metadata.emplace_back(header); - } - return metadata; -} -} // namespace - -absl::optional -FilterConfigPerRoute::mergeProcessingMode(const FilterConfigPerRoute& less_specific, - const FilterConfigPerRoute& more_specific) { - if (more_specific.disabled()) { - return absl::nullopt; - } - return more_specific.processingMode().has_value() ? more_specific.processingMode() - : less_specific.processingMode(); -} - -namespace { -// replace all entries with the same name or append one. -void mergeHeaderValue(std::vector& metadata, - const envoy::config::core::v3::HeaderValue& header) { - size_t count = 0; - for (auto& dest : metadata) { - if (dest.key() == header.key()) { - dest.CopyFrom(header); - count++; - } - } - if (!count) { - metadata.emplace_back(header); - } -} - -std::vector -mergeMetadata(const FilterConfigPerRoute& less_specific, - const FilterConfigPerRoute& more_specific) { - std::vector metadata(less_specific.metadata()); - - for (const auto& header : more_specific.metadata()) { - mergeHeaderValue(metadata, header); - } - - return metadata; -} -} // namespace - FilterConfigPerRoute::FilterConfigPerRoute(const ExtProcPerRoute& config) : disabled_(config.disabled()), processing_mode_(initProcessingMode(config)), - grpc_service_(initGrpcService(config)), metadata_(initMetadata(config)) {} + grpc_service_(initGrpcService(config)), + metadata_(config.overrides().metadata().begin(), config.overrides().metadata().end()) {} FilterConfigPerRoute::FilterConfigPerRoute(const FilterConfigPerRoute& less_specific, const FilterConfigPerRoute& more_specific) @@ -857,10 +871,6 @@ void Filter::onFinishProcessorCalls(Grpc::Status::GrpcStatus call_status) { encoding_state_.onFinishProcessorCall(call_status); } -static const ImmediateMutationChecker& immediateResponseChecker() { - CONSTRUCT_ON_FIRST_USE(ImmediateMutationChecker); -} - void Filter::sendImmediateResponse(const ImmediateResponse& response) { auto status_code = response.has_status() ? response.status().code() : DefaultImmediateStatus; if (!MutationUtils::isValidHttpStatus(status_code)) { @@ -898,30 +908,6 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { mutate_headers, grpc_status, details); } -static ProcessingMode allDisabledMode() { - ProcessingMode pm; - pm.set_request_header_mode(ProcessingMode::SKIP); - pm.set_response_header_mode(ProcessingMode::SKIP); - return pm; -} - -namespace { -// replace all entries with the same name or append one. -void mergeHeaderValue(Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, - const envoy::config::core::v3::HeaderValue& header) { - size_t count = 0; - for (auto& dest : metadata) { - if (dest.key() == header.key()) { - dest.CopyFrom(header); - count++; - } - } - if (!count) { - metadata.Add()->CopyFrom(header); - } -} -} // namespace - void Filter::mergePerRouteConfig() { if (route_config_merged_) { return; diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 39a2ac9e53c8..0459ec33db77 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -227,16 +227,6 @@ class FilterConfigPerRoute : public Router::RouteSpecificFilterConfig { const std::vector& metadata() const { return metadata_; } private: - absl::optional - initProcessingMode(const envoy::extensions::filters::http::ext_proc::v3::ExtProcPerRoute& config); - - absl::optional - initGrpcService(const envoy::extensions::filters::http::ext_proc::v3::ExtProcPerRoute& config); - - absl::optional - mergeProcessingMode(const FilterConfigPerRoute& less_specific, - const FilterConfigPerRoute& more_specific); - const bool disabled_; const absl::optional processing_mode_; From a8510131b0dbf8ea105b26e69123f72964873b1f Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Thu, 25 Jan 2024 14:50:06 +0000 Subject: [PATCH 05/11] make global static vars constexpr absl string views Signed-off-by: Ivan Prisyazhnyy --- source/extensions/filters/http/ext_proc/BUILD | 2 ++ .../filters/http/ext_proc/ext_proc.cc | 35 +++++++++++-------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index 484395f170d3..19cf20c83ea1 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -31,8 +31,10 @@ envoy_cc_library( "//source/common/runtime:runtime_features_lib", "//source/extensions/filters/common/mutation_rules:mutation_rules_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", + "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/status", "@com_google_absl//absl/strings:str_format", + "@com_google_absl//absl/strings:string_view", "@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", diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 3016b99fe16e..9a26dab46b21 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -8,7 +8,9 @@ #include "source/common/runtime/runtime_features.h" #include "source/extensions/filters/http/ext_proc/mutation_utils.h" +#include "absl/base/attributes.h" #include "absl/strings/str_format.h" +#include "absl/strings/string_view.h" namespace Envoy { namespace Extensions { @@ -16,10 +18,6 @@ namespace HttpFilters { namespace ExternalProcessing { namespace { -const std::string ErrorPrefix = "ext_proc_error"; -const int DefaultImmediateStatus = 200; -const std::string FilterName = "envoy.filters.http.ext_proc"; - using envoy::config::common::mutation_rules::v3::HeaderMutationRules; using envoy::extensions::filters::http::ext_proc::v3::ExtProcPerRoute; using envoy::extensions::filters::http::ext_proc::v3::ProcessingMode; @@ -38,12 +36,17 @@ using Http::RequestTrailerMap; using Http::ResponseHeaderMap; using Http::ResponseTrailerMap; +constexpr absl::string_view ErrorPrefix = "ext_proc_error"; +constexpr int DefaultImmediateStatus = 200; +ABSL_ATTRIBUTE_UNUSED constexpr absl::string_view FilterName = "envoy.filters.http.ext_proc"; + absl::optional initProcessingMode(const ExtProcPerRoute& config) { if (!config.disabled() && config.has_overrides() && config.overrides().has_processing_mode()) { return config.overrides().processing_mode(); } return absl::nullopt; } + absl::optional initGrpcService(const ExtProcPerRoute& config) { if (config.has_overrides() && config.overrides().has_grpc_service()) { @@ -60,34 +63,38 @@ absl::optional mergeProcessingMode(const FilterConfigPerRoute& l return more_specific.processingMode().has_value() ? more_specific.processingMode() : less_specific.processingMode(); } -// replace all entries with the same name or append one. -void mergeHeaderValue(std::vector& metadata, - const envoy::config::core::v3::HeaderValue& header) { - size_t count = 0; + +// Replaces all entries with the same name or append one. +void mergeHeaderValues(std::vector& metadata, + const envoy::config::core::v3::HeaderValue& header) { + bool count = false; for (auto& dest : metadata) { if (dest.key() == header.key()) { dest.CopyFrom(header); - count++; + count = true; } } if (!count) { metadata.emplace_back(header); } } + std::vector mergeMetadata(const FilterConfigPerRoute& less_specific, const FilterConfigPerRoute& more_specific) { std::vector metadata(less_specific.metadata()); for (const auto& header : more_specific.metadata()) { - mergeHeaderValue(metadata, header); + mergeHeaderValues(metadata, header); } return metadata; } -// replace all entries with the same name or append one. -void mergeHeaderValue(Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, - const envoy::config::core::v3::HeaderValue& header) { + +// Replaces all entries with the same name or append one. +void mergeHeaderValuesField( + Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, + const envoy::config::core::v3::HeaderValue& header) { bool count = false; for (auto& dest : metadata) { if (dest.key() == header.key()) { @@ -960,7 +967,7 @@ void Filter::mergePerRouteConfig() { auto ptr = config.mutable_initial_metadata(); for (const auto& header : merged_config->metadata()) { ENVOY_LOG(trace, "Setting metadata {} = {}", header.key(), header.value()); - mergeHeaderValue(*ptr, header); + mergeHeaderValuesField(*ptr, header); } config_with_hash_key_.setConfig(config); } From d2fa3fc6bb37e4a4eafed387a0874dfd0fb1fbd3 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Thu, 25 Jan 2024 15:19:16 +0000 Subject: [PATCH 06/11] fix changelog cur to have ext proc quoted Signed-off-by: Ivan Prisyazhnyy --- changelogs/current.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 8bd43a85482b..4d3bbc6e80ee 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -23,7 +23,7 @@ new_features: change: | Added :ref:`metadata ` - config API to allow extending inherited metadata from ExternalProcessor + config API to allow extending inherited metadata from ``ExternalProcessor`` :ref:`grpc_service ` with the new or updated values. - area: grpc reverse bridge From 4864ce59369522be2dda1db8260fdb26e4953a77 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Thu, 25 Jan 2024 15:53:28 +0000 Subject: [PATCH 07/11] fix/changelog: mention both grpc_service-s to override in refs Signed-off-by: Ivan Prisyazhnyy --- changelogs/current.yaml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 4d3bbc6e80ee..e46e3318dcb5 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -23,8 +23,10 @@ new_features: change: | Added :ref:`metadata ` - config API to allow extending inherited metadata from ``ExternalProcessor`` - :ref:`grpc_service ` + config API to allow extending inherited metadata from + :ref:`ExternalProcessor.grpc_service ` + and + :ref:`ExtProcOverrides.grpc_service ` with the new or updated values. - area: grpc reverse bridge change: | From 96ab5192cabe4b3b5264038f2984ff177638abc9 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Thu, 25 Jan 2024 15:59:49 +0000 Subject: [PATCH 08/11] change: remove unused FilterName global variable Signed-off-by: Ivan Prisyazhnyy --- source/extensions/filters/http/ext_proc/BUILD | 1 - source/extensions/filters/http/ext_proc/ext_proc.cc | 2 -- 2 files changed, 3 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/BUILD b/source/extensions/filters/http/ext_proc/BUILD index 19cf20c83ea1..b7e6607f6a8b 100644 --- a/source/extensions/filters/http/ext_proc/BUILD +++ b/source/extensions/filters/http/ext_proc/BUILD @@ -31,7 +31,6 @@ envoy_cc_library( "//source/common/runtime:runtime_features_lib", "//source/extensions/filters/common/mutation_rules:mutation_rules_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", - "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/status", "@com_google_absl//absl/strings:str_format", "@com_google_absl//absl/strings:string_view", diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 9a26dab46b21..50cc539334a4 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -8,7 +8,6 @@ #include "source/common/runtime/runtime_features.h" #include "source/extensions/filters/http/ext_proc/mutation_utils.h" -#include "absl/base/attributes.h" #include "absl/strings/str_format.h" #include "absl/strings/string_view.h" @@ -38,7 +37,6 @@ using Http::ResponseTrailerMap; constexpr absl::string_view ErrorPrefix = "ext_proc_error"; constexpr int DefaultImmediateStatus = 200; -ABSL_ATTRIBUTE_UNUSED constexpr absl::string_view FilterName = "envoy.filters.http.ext_proc"; absl::optional initProcessingMode(const ExtProcPerRoute& config) { if (!config.disabled() && config.has_overrides() && config.overrides().has_processing_mode()) { From 299911b82cd67af55b0db53bd4daaf345121c851 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Wed, 31 Jan 2024 21:03:54 +0000 Subject: [PATCH 09/11] fix: changelog ext_proc perroute override grpc metadata ref Signed-off-by: Ivan Prisyazhnyy --- changelogs/current.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 12b7a761b615..14446cdb3dcb 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -34,7 +34,7 @@ new_features: - area: ext_proc change: | Added - :ref:`metadata ` + :ref:`metadata ` config API to allow extending inherited metadata from :ref:`ExternalProcessor.grpc_service ` and From d768963f91bcbd2f7c0742f92711cc513f6680a4 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Sat, 10 Feb 2024 12:01:11 +0000 Subject: [PATCH 10/11] nit/ext_proc: rename to grpc_initial_metadata and count to has_key Signed-off-by: Ivan Prisyazhnyy --- .../filters/http/ext_proc/v3/ext_proc.proto | 2 +- changelogs/current.yaml | 2 +- .../filters/http/ext_proc/ext_proc.cc | 34 +++++++++---------- .../filters/http/ext_proc/ext_proc.h | 6 ++-- .../filters/http/ext_proc/config_test.cc | 2 +- .../ext_proc/ext_proc_integration_test.cc | 6 ++-- .../filters/http/ext_proc/filter_test.cc | 32 +++++++++++------ 7 files changed, 49 insertions(+), 35 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 7e4d2ff70ddd..5ef01fd8a290 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 @@ -307,5 +307,5 @@ message ExtProcOverrides { // service. This can be used for scenarios in which additional ad hoc // authorization headers (e.g. ``x-foo-bar: baz-key``) are to be injected or // when a route needs to partially override inherited metadata. - repeated config.core.v3.HeaderValue grpc_metadata = 7; + repeated config.core.v3.HeaderValue grpc_initial_metadata = 7; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 14446cdb3dcb..cd0767926b3e 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -34,7 +34,7 @@ new_features: - area: ext_proc change: | Added - :ref:`metadata ` + :ref:`grpc_initial_metadata ` config API to allow extending inherited metadata from :ref:`ExternalProcessor.grpc_service ` and diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 5520973b957b..88ac3e5a0d11 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -104,24 +104,24 @@ absl::optional mergeProcessingMode(const FilterConfigPerRoute& l // Replaces all entries with the same name or append one. void mergeHeaderValues(std::vector& metadata, const envoy::config::core::v3::HeaderValue& header) { - bool count = false; + bool has_key = false; for (auto& dest : metadata) { if (dest.key() == header.key()) { dest.CopyFrom(header); - count = true; + has_key = true; } } - if (!count) { + if (!has_key) { metadata.emplace_back(header); } } std::vector -mergeMetadata(const FilterConfigPerRoute& less_specific, - const FilterConfigPerRoute& more_specific) { - std::vector metadata(less_specific.grpcMetadata()); +mergeGrpcInitialMetadata(const FilterConfigPerRoute& less_specific, + const FilterConfigPerRoute& more_specific) { + std::vector metadata(less_specific.grpcInitialMetadata()); - for (const auto& header : more_specific.grpcMetadata()) { + for (const auto& header : more_specific.grpcInitialMetadata()) { mergeHeaderValues(metadata, header); } @@ -132,14 +132,14 @@ mergeMetadata(const FilterConfigPerRoute& less_specific, void mergeHeaderValuesField( Protobuf::RepeatedPtrField<::envoy::config::core::v3::HeaderValue>& metadata, const envoy::config::core::v3::HeaderValue& header) { - bool count = false; + bool has_key = false; for (auto& dest : metadata) { if (dest.key() == header.key()) { dest.CopyFrom(header); - count = true; + has_key = true; } } - if (!count) { + if (!has_key) { metadata.Add()->CopyFrom(header); } } @@ -238,8 +238,8 @@ ExtProcLoggingInfo::grpcCalls(envoy::config::core::v3::TrafficDirection traffic_ FilterConfigPerRoute::FilterConfigPerRoute(const ExtProcPerRoute& config) : disabled_(config.disabled()), processing_mode_(initProcessingMode(config)), grpc_service_(initGrpcService(config)), - grpc_metadata_(config.overrides().grpc_metadata().begin(), - config.overrides().grpc_metadata().end()), + grpc_initial_metadata_(config.overrides().grpc_initial_metadata().begin(), + config.overrides().grpc_initial_metadata().end()), untyped_forwarding_namespaces_(initUntypedForwardingNamespaces(config)), typed_forwarding_namespaces_(initTypedForwardingNamespaces(config)), untyped_receiving_namespaces_(initUntypedReceivingNamespaces(config)) {} @@ -250,7 +250,7 @@ FilterConfigPerRoute::FilterConfigPerRoute(const FilterConfigPerRoute& less_spec processing_mode_(mergeProcessingMode(less_specific, more_specific)), grpc_service_(more_specific.grpcService().has_value() ? more_specific.grpcService() : less_specific.grpcService()), - grpc_metadata_(mergeMetadata(less_specific, more_specific)), + grpc_initial_metadata_(mergeGrpcInitialMetadata(less_specific, more_specific)), untyped_forwarding_namespaces_(more_specific.untypedForwardingMetadataNamespaces().has_value() ? more_specific.untypedForwardingMetadataNamespaces() : less_specific.untypedForwardingMetadataNamespaces()), @@ -1132,12 +1132,12 @@ void Filter::mergePerRouteConfig() { grpc_service_ = *merged_config->grpcService(); config_with_hash_key_.setConfig(*merged_config->grpcService()); } - if (!merged_config->grpcMetadata().empty()) { - ENVOY_LOG(trace, "Overriding metadata from per-route configuration"); + if (!merged_config->grpcInitialMetadata().empty()) { + ENVOY_LOG(trace, "Overriding grpc initial metadata from per-route configuration"); envoy::config::core::v3::GrpcService config = config_with_hash_key_.config(); auto ptr = config.mutable_initial_metadata(); - for (const auto& header : merged_config->grpcMetadata()) { - ENVOY_LOG(trace, "Setting metadata {} = {}", header.key(), header.value()); + for (const auto& header : merged_config->grpcInitialMetadata()) { + ENVOY_LOG(trace, "Setting grpc initial metadata {} = {}", header.key(), header.value()); mergeHeaderValuesField(*ptr, header); } config_with_hash_key_.setConfig(config); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 192a87858352..ba90d5113cb2 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -247,8 +247,8 @@ class FilterConfigPerRoute : public Router::RouteSpecificFilterConfig { const absl::optional& grpcService() const { return grpc_service_; } - const std::vector& grpcMetadata() const { - return grpc_metadata_; + const std::vector& grpcInitialMetadata() const { + return grpc_initial_metadata_; } const absl::optional>& @@ -267,7 +267,7 @@ class FilterConfigPerRoute : public Router::RouteSpecificFilterConfig { const absl::optional processing_mode_; const absl::optional grpc_service_; - std::vector grpc_metadata_; + std::vector grpc_initial_metadata_; const absl::optional> untyped_forwarding_namespaces_; const absl::optional> typed_forwarding_namespaces_; diff --git a/test/extensions/filters/http/ext_proc/config_test.cc b/test/extensions/filters/http/ext_proc/config_test.cc index b73f60858ede..5c5196af6551 100644 --- a/test/extensions/filters/http/ext_proc/config_test.cc +++ b/test/extensions/filters/http/ext_proc/config_test.cc @@ -109,7 +109,7 @@ TEST(HttpExtProcConfigTest, CorrectConfigServerContext) { TEST(HttpExtProcConfigTest, CorrectRouteMetadataOnlyConfig) { std::string yaml = R"EOF( overrides: - grpc_metadata: + grpc_initial_metadata: - key: "a" value: "a" )EOF"; 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 c9a08539d0da..745147376ebc 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 @@ -2592,8 +2592,10 @@ TEST_P(ExtProcIntegrationTest, PerRouteGrpcMetadata) { auto* route = vh->mutable_routes()->Mutable(0); route->mutable_match()->set_path("/foo"); ExtProcPerRoute per_route; - *per_route.mutable_overrides()->mutable_grpc_metadata()->Add() = makeHeaderValue("b", "c"); - *per_route.mutable_overrides()->mutable_grpc_metadata()->Add() = makeHeaderValue("c", "c"); + *per_route.mutable_overrides()->mutable_grpc_initial_metadata()->Add() = + makeHeaderValue("b", "c"); + *per_route.mutable_overrides()->mutable_grpc_initial_metadata()->Add() = + makeHeaderValue("c", "c"); setPerRouteConfig(route, per_route); }); diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 3b91b2f86479..83d469b60331 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -3001,18 +3001,28 @@ TEST(OverrideTest, GrpcServiceNonOverride) { // When merging two configurations, second metadata override only extends the first's one. TEST(OverrideTest, GrpcMetadataOverride) { ExtProcPerRoute cfg1; - cfg1.mutable_overrides()->mutable_grpc_metadata()->Add()->CopyFrom(makeHeaderValue("a", "a")); - cfg1.mutable_overrides()->mutable_grpc_metadata()->Add()->CopyFrom(makeHeaderValue("b", "b")); + cfg1.mutable_overrides()->mutable_grpc_initial_metadata()->Add()->CopyFrom( + makeHeaderValue("a", "a")); + cfg1.mutable_overrides()->mutable_grpc_initial_metadata()->Add()->CopyFrom( + makeHeaderValue("b", "b")); + ExtProcPerRoute cfg2; - cfg2.mutable_overrides()->mutable_grpc_metadata()->Add()->CopyFrom(makeHeaderValue("b", "c")); - cfg2.mutable_overrides()->mutable_grpc_metadata()->Add()->CopyFrom(makeHeaderValue("c", "c")); + cfg2.mutable_overrides()->mutable_grpc_initial_metadata()->Add()->CopyFrom( + makeHeaderValue("b", "c")); + cfg2.mutable_overrides()->mutable_grpc_initial_metadata()->Add()->CopyFrom( + makeHeaderValue("c", "c")); + FilterConfigPerRoute route1(cfg1); FilterConfigPerRoute route2(cfg2); FilterConfigPerRoute merged_route(route1, route2); - ASSERT_TRUE(merged_route.grpcMetadata().size() == 3); - EXPECT_THAT(merged_route.grpcMetadata()[0], ProtoEq(cfg1.overrides().grpc_metadata()[0])); - EXPECT_THAT(merged_route.grpcMetadata()[1], ProtoEq(cfg2.overrides().grpc_metadata()[0])); - EXPECT_THAT(merged_route.grpcMetadata()[2], ProtoEq(cfg2.overrides().grpc_metadata()[1])); + + ASSERT_TRUE(merged_route.grpcInitialMetadata().size() == 3); + EXPECT_THAT(merged_route.grpcInitialMetadata()[0], + ProtoEq(cfg1.overrides().grpc_initial_metadata()[0])); + EXPECT_THAT(merged_route.grpcInitialMetadata()[1], + ProtoEq(cfg2.overrides().grpc_initial_metadata()[0])); + EXPECT_THAT(merged_route.grpcInitialMetadata()[2], + ProtoEq(cfg2.overrides().grpc_initial_metadata()[1])); } // Verify that attempts to change headers that are not allowed to be changed @@ -3885,8 +3895,10 @@ TEST_F(HttpFilterTest, GrpcServiceMetadataOverride) { // Route configuration overrides the grpc_service metadata. ExtProcPerRoute route_proto; - *route_proto.mutable_overrides()->mutable_grpc_metadata()->Add() = makeHeaderValue("b", "c"); - *route_proto.mutable_overrides()->mutable_grpc_metadata()->Add() = makeHeaderValue("c", "c"); + *route_proto.mutable_overrides()->mutable_grpc_initial_metadata()->Add() = + makeHeaderValue("b", "c"); + *route_proto.mutable_overrides()->mutable_grpc_initial_metadata()->Add() = + makeHeaderValue("c", "c"); FilterConfigPerRoute route_config(route_proto); EXPECT_CALL(decoder_callbacks_, traversePerFilterConfig(_)) .WillOnce( From 6a28914225c3b790d2d3448ae131841e6c3f77b6 Mon Sep 17 00:00:00 2001 From: Ivan Prisyazhnyy Date: Sat, 17 Feb 2024 10:22:39 +0000 Subject: [PATCH 11/11] test/ext_proc: integ test improvement to check upstream hdrs Signed-off-by: Ivan Prisyazhnyy --- .../filters/http/ext_proc/ext_proc_integration_test.cc | 6 ++++++ 1 file changed, 6 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 745147376ebc..b2c8eb1728ad 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 @@ -2606,6 +2606,12 @@ TEST_P(ExtProcIntegrationTest, PerRouteGrpcMetadata) { sendDownstreamRequest([](Http::RequestHeaderMap& headers) { headers.setPath("/foo"); }); processRequestHeadersMessage(*grpc_upstreams_[0], true, absl::nullopt); + EXPECT_EQ( + "c", + processor_stream_->headers().get(Http::LowerCaseString("b"))[0]->value().getStringView()); + EXPECT_EQ( + "c", + processor_stream_->headers().get(Http::LowerCaseString("c"))[0]->value().getStringView()); handleUpstreamRequest(); processResponseHeadersMessage(*grpc_upstreams_[0], false, absl::nullopt);