diff --git a/changelogs/current.yaml b/changelogs/current.yaml index c864088fa37fb..46d45f42fe1fd 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -24,6 +24,14 @@ behavior_changes: rather than waiting for the original request to complete. This allows shadowing requests larger than the buffer limit, but also means shadowing may take place for requests which are canceled mid-stream. This behavior change can be temporarily reverted by flipping ``envoy.reloadable_features.streaming_shadow`` to false. +- area: ext_proc + change: | + The default behavior for header mutations in ext_proc changed. Previously, header values would be replaced by default, + and setting append to true was required for appending. Now, header values will be appended by default if append is + not specified. The old behavior can be temporarily restored by setting runtime guard + ``envoy.reloadable_features.ext_proc_legacy_append`` to ``true``. For more control over header mutation behavior, + use the :ref:`append_action ` field which will + be the only supported option in the future. minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* @@ -265,6 +273,11 @@ new_features: change: | Added support in SNI dynamic forward proxy for saving the resolved upstream address in the filter state. The state is saved with the key ``envoy.stream.upstream_address``. +- area: ext_proc + change: | + Added support for ``HeaderAppendAction`` in ext_proc mutation utils, allowing more granular control over header + modifications using ``APPEND_IF_EXISTS_OR_ADD``, ``ADD_IF_ABSENT``, ``OVERWRITE_IF_EXISTS``, and + ``OVERWRITE_IF_EXISTS_OR_ADD`` actions. The existing append behavior continues to work as before. deprecated: - area: rbac diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 7667f36e66d4c..d6d1a4b1a78f5 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -113,6 +113,9 @@ RUNTIME_GUARD(envoy_restart_features_use_eds_cache_for_ads); // Begin false flags. Most of them should come with a TODO to flip true. +// Used to temporarily allow the legacy append behavior on ext_proc header mutations +FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_legacy_append); + // Sentinel and test flag. FALSE_RUNTIME_GUARD(envoy_reloadable_features_test_feature_false); // TODO(adisuissa) reset to true to enable unified mux by default diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index eaaec2ad7f9aa..c99e3d2658a36 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -161,20 +161,66 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, rejected_mutations.inc(); return absl::InvalidArgumentError("Invalid character in set_headers mutation."); } + + auto check_op = CheckOperation::SET; + auto should_append = false; const LowerCaseString header_name(sh.header().key()); - const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); - const auto check_op = (append && !headers.get(header_name).empty()) ? CheckOperation::APPEND - : CheckOperation::SET; - auto check_result = checker.check(check_op, header_name, header_value); - if (replacing_message && header_name == Http::Headers::get().Method) { - // Special handling to allow changing ":method" when the - // CONTINUE_AND_REPLACE option is selected, to stay compatible. - check_result = CheckResult::OK; + const auto header_exists = !headers.get(header_name).empty(); + + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.ext_proc_legacy_append")) { + // Legacy append behavior + should_append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); + check_op = (should_append && header_exists) ? CheckOperation::APPEND : CheckOperation::SET; + } else { + if (sh.has_append()) { + // 'append' is set and ensure the 'append_action' value is equal to the default value. + if (sh.append_action() != + envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD) { + return absl::InvalidArgumentError( + "Both append and append_action are set and it's not allowed"); + } + + // Legacy append behavior + should_append = sh.append().value(); + check_op = (should_append && header_exists) ? CheckOperation::APPEND : CheckOperation::SET; + } else { + switch (sh.append_action()) { + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; + case envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: + should_append = true; + check_op = CheckOperation::APPEND; + break; + case envoy::config::core::v3::HeaderValueOption::ADD_IF_ABSENT: + if (header_exists) { + continue; // Skip if header exists + } + should_append = false; + check_op = CheckOperation::SET; + break; + case envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS: + if (!header_exists) { + continue; // Skip if header doesn't exist + } + should_append = false; + check_op = CheckOperation::SET; + break; + case envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: + should_append = false; + check_op = CheckOperation::SET; + break; + } + } } - switch (check_result) { + + const auto check_result = checker.check(check_op, header_name, header_value); + const auto final_result = replacing_message && header_name == Http::Headers::get().Method + ? CheckResult::OK + : check_result; + + switch (final_result) { case CheckResult::OK: - ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); - if (append) { + ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), should_append); + if (should_append) { headers.addCopy(header_name, header_value); } else { headers.setCopy(header_name, header_value); diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index cf172ae5d2a1d..f326ebb8d6946 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -151,6 +151,7 @@ envoy_extension_cc_test( "//source/extensions/filters/http/ext_proc:mutation_utils_lib", "//test/mocks/server:server_factory_context_mocks", "//test/mocks/stats:stats_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/common/mutation_rules/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 9449599c41f51..696ac25c6c5e6 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 @@ -1131,6 +1131,8 @@ TEST_P(ExtProcIntegrationTest, SetHostHeaderRoutingSucceeded) { // Set host header to match the domain of virtual host in routing configuration. auto* mut = response_header_mutation->add_set_headers(); + mut->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); mut->mutable_header()->set_key(":authority"); mut->mutable_header()->set_raw_value(vhost_domain); @@ -1222,18 +1224,26 @@ TEST_P(ExtProcIntegrationTest, GetAndSetPathHeader) { auto response_header_mutation = headers_resp.mutable_response()->mutable_header_mutation(); auto* mut1 = response_header_mutation->add_set_headers(); + mut1->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); mut1->mutable_header()->set_key(":path"); mut1->mutable_header()->set_raw_value("/mutated_path/bluh"); auto* mut2 = response_header_mutation->add_set_headers(); + mut2->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); mut2->mutable_header()->set_key(":scheme"); mut2->mutable_header()->set_raw_value("https"); auto* mut3 = response_header_mutation->add_set_headers(); + mut3->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); mut3->mutable_header()->set_key(":authority"); mut3->mutable_header()->set_raw_value("new_host"); auto* mut4 = response_header_mutation->add_set_headers(); + mut4->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); mut4->mutable_header()->set_key(":method"); mut4->mutable_header()->set_raw_value("POST"); return true; @@ -1606,9 +1616,13 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponse) { *grpc_upstreams_[0], false, [](const HttpHeaders&, HeadersResponse& headers_resp) { auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation(); auto* add1 = response_mutation->add_set_headers(); + add1->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add1->mutable_header()->set_key("x-response-processed"); add1->mutable_header()->set_raw_value("1"); auto* add2 = response_mutation->add_set_headers(); + add2->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add2->mutable_header()->set_key(":status"); add2->mutable_header()->set_raw_value("201"); return true; @@ -1661,9 +1675,13 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseTwoStatuses) { *grpc_upstreams_[0], false, [](const HttpHeaders&, HeadersResponse& headers_resp) { auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation(); auto* add1 = response_mutation->add_set_headers(); + add1->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add1->mutable_header()->set_key("x-response-processed"); add1->mutable_header()->set_raw_value("1"); auto* add2 = response_mutation->add_set_headers(); + add2->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add2->mutable_header()->set_key(":status"); add2->mutable_header()->set_raw_value("201"); auto* add3 = response_mutation->add_set_headers(); @@ -2362,6 +2380,10 @@ TEST_P(ExtProcIntegrationTest, GetAndIncorrectlyModifyHeaderOnBodyPartialBuffer) // Test the ability of the filter to turn a GET into a POST by adding a body // and changing the method. TEST_P(ExtProcIntegrationTest, ConvertGetToPost) { + // NOTE: The use of `append` has been deprecated in favor of `append_action`. This is a temporary + // provision for users who have enabled legacy behavior, which will be phased out entirely in the + // future. + scoped_runtime_.mergeValues({{"envoy.reloadable_features.ext_proc_legacy_append", "true"}}); initializeConfig(); HttpIntegrationTest::initialize(); @@ -3033,9 +3055,13 @@ TEST_P(ExtProcIntegrationTest, PerRouteGrpcService) { *grpc_upstreams_[1], false, [](const HttpHeaders&, HeadersResponse& headers_resp) { auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation(); auto* add1 = response_mutation->add_set_headers(); + add1->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add1->mutable_header()->set_key("x-response-processed"); add1->mutable_header()->set_raw_value("1"); auto* add2 = response_mutation->add_set_headers(); + add2->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add2->mutable_header()->set_key(":status"); add2->mutable_header()->set_raw_value("201"); return true; @@ -5244,4 +5270,161 @@ TEST_P(ExtProcIntegrationTest, ModeOverrideDisallowed) { verifyDownstreamResponse(*response, 200); } +TEST_P(ExtProcIntegrationTest, HeaderAppendAction) { + initializeConfig(); + HttpIntegrationTest::initialize(); + + auto response = sendDownstreamRequest([](Http::HeaderMap& headers) { + headers.addCopy(LowerCaseString("x-append-test"), "original"); + headers.addCopy(LowerCaseString("x-overwrite-test"), "original"); + }); + + // Test different append actions + processRequestHeadersMessage( + *grpc_upstreams_[0], true, [](const HttpHeaders& /*headers*/, HeadersResponse& headers_resp) { + auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation(); + + // Append to existing header + auto* mut1 = response_mutation->add_set_headers(); + mut1->set_append_action( + envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + mut1->mutable_header()->set_key("x-append-test"); + mut1->mutable_header()->set_raw_value("appended"); + + // Add new header + auto* mut2 = response_mutation->add_set_headers(); + mut2->set_append_action(envoy::config::core::v3::HeaderValueOption::ADD_IF_ABSENT); + mut2->mutable_header()->set_key("x-new-absent"); + mut2->mutable_header()->set_raw_value("new"); + + // Overwrite existing header + auto* mut3 = response_mutation->add_set_headers(); + mut3->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS); + mut3->mutable_header()->set_key("x-overwrite-test"); + mut3->mutable_header()->set_raw_value("overwritten"); + + return true; + }); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + // Get a copy of upstream headers for comparison + Http::TestRequestHeaderMapImpl expected_request_headers{ + {":scheme", "http"}, + {":method", "GET"}, + {":path", "/"}, + {":authority", "host"}, + {"x-append-test", "original"}, + {"x-append-test", "appended"}, + {"x-new-absent", "new"}, + {"x-overwrite-test", "overwritten"}, + {"x-envoy-expected-rq-timeout-ms", "15000"}, + {"x-forwarded-proto", "http"}}; + + Http::TestRequestHeaderMapImpl upstream_headers; + const auto& headers = upstream_request_->headers(); + headers.iterate([&upstream_headers](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + // Skip x-request-id header + if (header.key().getStringView() != "x-request-id") { + upstream_headers.addCopy(Http::LowerCaseString(std::string(header.key().getStringView())), + std::string(header.value().getStringView())); + } + return Http::HeaderMap::Iterate::Continue; + }); + + EXPECT_THAT(&upstream_headers, HeaderMapEqualIgnoreOrder(&expected_request_headers)); + + // Complete the request + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + verifyDownstreamResponse(*response, 504); +} + +TEST_P(ExtProcIntegrationTest, HeaderAppendActionWithResponseHeaders) { + initializeConfig(); + HttpIntegrationTest::initialize(); + + auto response = sendDownstreamRequest(absl::nullopt); + processRequestHeadersMessage(*grpc_upstreams_[0], true, absl::nullopt); + handleUpstreamRequest(); + + processResponseHeadersMessage( + *grpc_upstreams_[0], false, + [](const HttpHeaders& /*headers*/, HeadersResponse& headers_resp) { + auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation(); + + auto* mut1 = response_mutation->add_set_headers(); + mut1->set_append_action( + envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + mut1->mutable_header()->set_key("x-multiple-test"); + mut1->mutable_header()->set_raw_value("first"); + + auto* mut2 = response_mutation->add_set_headers(); + mut2->set_append_action( + envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + mut2->mutable_header()->set_key("x-multiple-test"); + mut2->mutable_header()->set_raw_value("second"); + + return true; + }); + + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + + const Http::ResponseHeaderMap& response_headers = response->headers(); + EXPECT_EQ( + response_headers.get(Http::LowerCaseString("x-multiple-test"))[0]->value().getStringView(), + "first"); + EXPECT_EQ( + response_headers.get(Http::LowerCaseString("x-multiple-test"))[1]->value().getStringView(), + "second"); + EXPECT_EQ(response->headers().getStatusValue(), "200"); +} + +TEST_P(ExtProcIntegrationTest, HeaderAppendActionWithTrailers) { + proto_config_.mutable_processing_mode()->set_request_header_mode(ProcessingMode::SEND); + proto_config_.mutable_processing_mode()->set_request_trailer_mode(ProcessingMode::SEND); + initializeConfig(); + HttpIntegrationTest::initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + auto encoder_decoder = codec_client_->startRequest(headers); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + Http::TestRequestTrailerMapImpl request_trailers{ + {"x-trailer-test", "original"}, + }; + codec_client_->sendTrailers(*request_encoder_, request_trailers); + + processRequestHeadersMessage(*grpc_upstreams_[0], true, absl::nullopt); + + processRequestTrailersMessage( + *grpc_upstreams_[0], false, [](const HttpTrailers& /*trailers*/, TrailersResponse& resp) { + auto* trailer_mut = resp.mutable_header_mutation(); + auto* mut = trailer_mut->add_set_headers(); + mut->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + mut->mutable_header()->set_key("x-trailer-test"); + mut->mutable_header()->set_raw_value("appended"); + return true; + }); + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + ASSERT_TRUE(upstream_request_->trailers()); + const auto& trailer_values = + upstream_request_->trailers()->get(Http::LowerCaseString("x-trailer-test")); + ASSERT_EQ(trailer_values.size(), 2); + EXPECT_EQ(trailer_values[0]->value().getStringView(), "original"); + EXPECT_EQ(trailer_values[1]->value().getStringView(), "appended"); + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + verifyDownstreamResponse(*response, 504); +} + } // namespace Envoy diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 2fcedff16489b..898ba8d2fafa5 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -796,6 +796,8 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) { auto* resp_headers_mut = header_resp.mutable_response()->mutable_header_mutation(); auto* resp_add1 = resp_headers_mut->add_set_headers(); + resp_add1->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add1->mutable_header()->set_key("x-new-header"); resp_add1->mutable_header()->set_raw_value("new"); }); @@ -847,6 +849,7 @@ TEST_F(HttpFilterTest, PostAndRespondImmediately) { immediate_response->set_details("Got a bad request"); auto* immediate_headers = immediate_response->mutable_headers(); auto* hdr1 = immediate_headers->add_set_headers(); + hdr1->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); hdr1->mutable_header()->set_key("content-type"); hdr1->mutable_header()->set_raw_value("text/plain"); auto* hdr2 = immediate_headers->add_set_headers(); @@ -902,6 +905,7 @@ TEST_F(HttpFilterTest, PostAndRespondImmediatelyWithDisabledConfig) { immediate_response->set_details("Got a bad request"); auto* immediate_headers = immediate_response->mutable_headers(); auto* hdr1 = immediate_headers->add_set_headers(); + hdr1->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); hdr1->mutable_header()->set_key("content-type"); hdr1->mutable_header()->set_raw_value("text/plain"); stream_callbacks_->onReceiveMessage(std::move(resp1)); @@ -2859,6 +2863,8 @@ TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutation) { processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); resp.mutable_response()->set_clear_route_cache(true); @@ -2876,6 +2882,8 @@ TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutation) { processResponseBody([](const HttpBody&, ProcessingResponse&, BodyResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); resp.mutable_response()->set_clear_route_cache(true); @@ -2909,6 +2917,8 @@ TEST_F(HttpFilterTest, ClearRouteCacheDisabledHeaderMutation) { processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); resp.mutable_response()->set_clear_route_cache(true); @@ -2926,6 +2936,8 @@ TEST_F(HttpFilterTest, ClearRouteCacheDisabledHeaderMutation) { processResponseBody([](const HttpBody&, ProcessingResponse&, BodyResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); resp.mutable_response()->set_clear_route_cache(true); @@ -3031,6 +3043,8 @@ TEST_F(HttpFilterTest, FilterRouteCacheActionSetToClearHeaderMutation) { processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); }); @@ -3110,6 +3124,8 @@ TEST_F(HttpFilterTest, FilterRouteCacheActionSetToRetainWithHeaderMutation) { processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); resp.mutable_response()->set_clear_route_cache(true); @@ -3138,6 +3154,8 @@ TEST_F(HttpFilterTest, FilterRouteCacheActionSetToRetainResponseNotWithHeaderMut processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); }); @@ -3165,14 +3183,15 @@ TEST_F(HttpFilterTest, ReplaceRequest) { Buffer::OwnedImpl req_buffer; setUpDecodingBuffering(req_buffer, true); - processRequestHeaders( - false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& hdrs_resp) { - hdrs_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); - auto* hdr = hdrs_resp.mutable_response()->mutable_header_mutation()->add_set_headers(); - hdr->mutable_header()->set_key(":method"); - hdr->mutable_header()->set_raw_value("POST"); - hdrs_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, World!"); - }); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, + HeadersResponse& hdrs_resp) { + hdrs_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); + auto* hdr = hdrs_resp.mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + hdr->mutable_header()->set_key(":method"); + hdr->mutable_header()->set_raw_value("POST"); + hdrs_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, World!"); + }); TestRequestHeaderMapImpl expected_request{ {":scheme", "http"}, {":authority", "host"}, {":path", "/"}, {":method", "POST"}}; @@ -3226,14 +3245,15 @@ TEST_F(HttpFilterTest, ReplaceCompleteResponseBuffered) { Buffer::OwnedImpl buffered_resp_data; setUpEncodingBuffering(buffered_resp_data, true); - processResponseHeaders( - false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& hdrs_resp) { - hdrs_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); - auto* hdr = hdrs_resp.mutable_response()->mutable_header_mutation()->add_set_headers(); - hdr->mutable_header()->set_key("x-test-header"); - hdr->mutable_header()->set_raw_value("true"); - hdrs_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, World!"); - }); + processResponseHeaders(false, [](const HttpHeaders&, ProcessingResponse&, + HeadersResponse& hdrs_resp) { + hdrs_resp.mutable_response()->set_status(CommonResponse::CONTINUE_AND_REPLACE); + auto* hdr = hdrs_resp.mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + hdr->mutable_header()->set_key("x-test-header"); + hdr->mutable_header()->set_raw_value("true"); + hdrs_resp.mutable_response()->mutable_body_mutation()->set_body("Hello, World!"); + }); // Ensure buffered data was updated EXPECT_EQ(buffered_resp_data.toString(), "Hello, World!"); @@ -3428,16 +3448,17 @@ TEST_F(HttpFilterTest, IgnoreInvalidHeaderMutations) { EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); - processRequestHeaders( - false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) { - auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); - auto add1 = headers_mut->add_set_headers(); - // Not allowed to change the "host" header by default - add1->mutable_header()->set_key("Host"); - add1->mutable_header()->set_raw_value("wrong:1234"); - // Not allowed to remove x-envoy headers by default. - headers_mut->add_remove_headers("x-envoy-special-thing"); - }); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, + HeadersResponse& header_resp) { + auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); + auto add1 = headers_mut->add_set_headers(); + // Not allowed to change the "host" header by default + add1->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + add1->mutable_header()->set_key("Host"); + add1->mutable_header()->set_raw_value("wrong:1234"); + // Not allowed to remove x-envoy headers by default. + headers_mut->add_remove_headers("x-envoy-special-thing"); + }); // The original headers should not have been modified now. TestRequestHeaderMapImpl expected{ @@ -3482,6 +3503,7 @@ TEST_F(HttpFilterTest, FailOnInvalidHeaderMutations) { resp1->mutable_request_headers()->mutable_response()->mutable_header_mutation(); auto add1 = headers_mut->add_set_headers(); // Not allowed to change the "host" header by default + add1->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add1->mutable_header()->set_key("Host"); add1->mutable_header()->set_raw_value("wrong:1234"); // Not allowed to remove x-envoy headers by default. @@ -3525,9 +3547,13 @@ TEST_F(HttpFilterTest, ResponseTrailerMutationExceedSizeLimit) { // The trailer mutation in the response does not exceed the count limit 100 or the // size limit 2kb. But the result header map size exceeds the count limit 2kb. auto add1 = headers_mut->add_set_headers(); + add1->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add1->mutable_header()->set_key("x-new-header-0123456789"); add1->mutable_header()->set_raw_value("new-header-0123456789"); auto add2 = headers_mut->add_set_headers(); + add2->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); add2->mutable_header()->set_key("x-some-other-header-0123456789"); add2->mutable_header()->set_raw_value("some-new-header-0123456789"); }, @@ -4680,6 +4706,8 @@ TEST_F(HttpFilterTest, ClearRouteCacheHeaderMutationUpstreamIgnored) { processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& resp) { auto* resp_headers_mut = resp.mutable_response()->mutable_header_mutation(); auto* resp_add = resp_headers_mut->add_set_headers(); + resp_add->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); resp_add->mutable_header()->set_key("x-new-header"); resp_add->mutable_header()->set_raw_value("new"); resp.mutable_response()->set_clear_route_cache(true); @@ -4723,6 +4751,7 @@ TEST_F(HttpFilterTest, PostAndRespondImmediatelyUpstream) { immediate_response->set_details("Got a bad request"); auto* immediate_headers = immediate_response->mutable_headers(); auto* hdr1 = immediate_headers->add_set_headers(); + hdr1->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); hdr1->mutable_header()->set_key("content-type"); hdr1->mutable_header()->set_raw_value("text/plain"); stream_callbacks_->onReceiveMessage(std::move(resp1)); @@ -5018,9 +5047,13 @@ TEST_F(HttpFilter2Test, LastDecodeDataCallExceedsStreamBufferLimitWouldJustRaise auto* headers_response = response->mutable_request_headers(); auto* hdr = headers_response->mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); hdr->mutable_header()->set_key("foo"); hdr->mutable_header()->set_raw_value("gift-from-external-server"); hdr = headers_response->mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); hdr->mutable_header()->set_key(":path"); hdr->mutable_header()->set_raw_value("/mutated_path/bluh"); HttpFilterTest::stream_callbacks_->onReceiveMessage(std::move(response)); @@ -5123,9 +5156,13 @@ TEST_F(HttpFilter2Test, LastEncodeDataCallExceedsStreamBufferLimitWouldJustRaise auto* headers_response = response->mutable_response_headers(); auto* hdr = headers_response->mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); hdr->mutable_header()->set_key("foo"); hdr->mutable_header()->set_raw_value("gift-from-external-server"); hdr = headers_response->mutable_response()->mutable_header_mutation()->add_set_headers(); + hdr->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); hdr->mutable_header()->set_key("new_response_header"); hdr->mutable_header()->set_raw_value("bluh"); HttpFilterTest::stream_callbacks_->onReceiveMessage(std::move(response)); diff --git a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc index ea0efe3b6cf2d..be080ed22fcd0 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -6,6 +6,7 @@ #include "test/extensions/filters/http/ext_proc/utils.h" #include "test/mocks/server/server_factory_context.h" #include "test/mocks/stats/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -55,6 +56,12 @@ TEST_F(MutationUtilsTest, TestBuildHeaders) { } TEST_F(MutationUtilsTest, TestApplyMutations) { + // NOTE: The use of `append` has been deprecated in favor of `append_action`. This is a temporary + // provision for users who have enabled legacy behavior, which will be phased out entirely in the + // future. + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.ext_proc_legacy_append", "true"}}); + Http::TestRequestHeaderMapImpl headers{ {":scheme", "https"}, {":method", "GET"}, @@ -162,6 +169,12 @@ TEST_F(MutationUtilsTest, TestApplyMutations) { } TEST_F(MutationUtilsTest, TestNonAppendableHeaders) { + // NOTE: The use of `append` has been deprecated in favor of `append_action`. This is a temporary + // provision for users who have enabled legacy behavior, which will be phased out entirely in the + // future. + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.ext_proc_legacy_append", "true"}}); + Http::TestRequestHeaderMapImpl headers; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); @@ -444,6 +457,148 @@ TEST_F(MutationUtilsTest, TestDisallowHeaderSetNotAllowHeader) { EXPECT_THAT(proto_headers, HeaderProtosEqual(expected)); } +TEST_F(MutationUtilsTest, TestAppendActionBehaviors) { + Http::TestRequestHeaderMapImpl headers{ + {":method", "GET"}, {"existing-header", "original"}, + {"append-target", "first"}, {"overwrite-target", "original"}, + {"multiple-append", "1"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + + // Test APPEND_IF_EXISTS_OR_ADD + auto* s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_header()->set_key("append-target"); + s->mutable_header()->set_raw_value("second"); + + // Should be added since header doesn't exist + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_header()->set_key("new-appended"); + s->mutable_header()->set_raw_value("value"); + + // Test ADD_IF_ABSENT + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::ADD_IF_ABSENT); + s->mutable_header()->set_key("existing-header"); + s->mutable_header()->set_raw_value("should-not-be-added"); + + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::ADD_IF_ABSENT); + s->mutable_header()->set_key("new-header"); + s->mutable_header()->set_raw_value("new-value"); + + // Test OVERWRITE_IF_EXISTS + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS); + s->mutable_header()->set_key("overwrite-target"); + s->mutable_header()->set_raw_value("new-value"); + + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS); + s->mutable_header()->set_key("non-existent"); + s->mutable_header()->set_raw_value("should-not-be-added"); + + // Test OVERWRITE_IF_EXISTS_OR_ADD + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + s->mutable_header()->set_key("existing-header"); + s->mutable_header()->set_raw_value("overwritten"); + + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + s->mutable_header()->set_key("new-overwritten"); + s->mutable_header()->set_raw_value("added"); + + // Test multiple appends to same header + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_header()->set_key("multiple-append"); + s->mutable_header()->set_raw_value("2"); + + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_header()->set_key("multiple-append"); + s->mutable_header()->set_raw_value("3"); + + // Use the default mutation rules + Checker checker(HeaderMutationRules::default_instance(), regex_engine_); + Stats::MockCounter rejections; + EXPECT_TRUE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + + Http::TestRequestHeaderMapImpl expected_headers{ + {":method", "GET"}, + {"append-target", "first"}, + {"append-target", "second"}, + {"new-appended", "value"}, + {"new-header", "new-value"}, + {"overwrite-target", "new-value"}, + {"existing-header", "overwritten"}, + {"new-overwritten", "added"}, + {"multiple-append", "1"}, + {"multiple-append", "2"}, + {"multiple-append", "3"}, + }; + + EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); +} + +// Test interaction between legacy append and new append_action +TEST_F(MutationUtilsTest, TestAppendActionWithLegacyAppend) { + Http::TestRequestHeaderMapImpl headers{ + {"target", "original"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + + // Set with legacy append first + auto* s = mutation.add_set_headers(); + s->mutable_append()->set_value(true); + s->mutable_header()->set_key("target"); + s->mutable_header()->set_raw_value("legacy-append"); + + // Then with new append_action + s = mutation.add_set_headers(); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_header()->set_key("target"); + s->mutable_header()->set_raw_value("new-append"); + + Checker checker(HeaderMutationRules::default_instance(), regex_engine_); + Stats::MockCounter rejections; + EXPECT_TRUE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + + Http::TestRequestHeaderMapImpl expected_headers{ + {"target", "original"}, + {"target", "legacy-append"}, + {"target", "new-append"}, + }; + + EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); +} + +TEST_F(MutationUtilsTest, TestInvalidAppendConfig) { + Http::TestRequestHeaderMapImpl headers{ + {"original-header", "value"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + auto* s = mutation.add_set_headers(); + s->mutable_header()->set_key("test-header"); + s->mutable_header()->set_raw_value("test-value"); + s->mutable_append()->set_value(true); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + + Checker checker(HeaderMutationRules::default_instance(), regex_engine_); + Stats::MockCounter rejections; + + auto result = MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections); + EXPECT_FALSE(result.ok()); + EXPECT_EQ(result.message(), "Both append and append_action are set and it's not allowed"); +} + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters