diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce5..f62d9b812f640 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -13,5 +13,12 @@ removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` new_features: +- area: ext_proc + change: | + Added runtime guard ``envoy.reloadable_features.ext_proc_modified_append_default_value``. + The default value of the runtime is kept as false to keep the legacy behavior. Stats counters and error logs are added if + :ref:`append ` in + :ref:`external processor service ` is not explicitly encoded. + These data will give us information whether it is safe to change the default value of ``append`` to be true. deprecated: diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index c30e1e632f7b1..8c61adb1a2a27 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -179,6 +179,7 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_use_network_type_socket_option); FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_disable_client_early_data); FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_graceful_grpc_close); +FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_modified_append_default_value); // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index f23a3f09914fa..7e569a1eb6221 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -1540,7 +1540,7 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) { if (response.has_headers()) { const absl::Status mut_status = MutationUtils::applyHeaderMutations( response.headers(), headers, false, config().mutationChecker(), - stats_.rejected_header_mutations_); + stats_.rejected_header_mutations_, stats_.invalid_header_append_encoding_); if (!mut_status.ok()) { ENVOY_LOG_EVERY_POW_2(error, "Immediate response mutations failed with {}", mut_status.message()); diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 88e9ff6d7df45..9eb47d425f522 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -44,6 +44,7 @@ namespace ExternalProcessing { COUNTER(failure_mode_allowed) \ COUNTER(message_timeouts) \ COUNTER(rejected_header_mutations) \ + COUNTER(invalid_header_append_encoding) \ COUNTER(override_message_timeout_received) \ COUNTER(override_message_timeout_ignored) \ COUNTER(clear_route_cache_ignored) \ diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index eaaec2ad7f9aa..ba58648ef8b0f 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -106,10 +106,36 @@ absl::Status MutationUtils::headerMutationResultCheck(const Http::HeaderMap& hea return absl::OkStatus(); } +// TODO(yanjunxiang-google): Remove the invalid_header_append_encoding counter when removing the +// runtime. +bool MutationUtils::getAppendFromHeaderMutation( + const envoy::config::core::v3::HeaderValueOption& set_header, + Stats::Counter& invalid_append_encoding) { + bool append; + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.ext_proc_modified_append_default_value")) { + // This is legacy case. i.e, only "append" is supported, and default "append"" is false. + append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(set_header, append, false); + + // As "append" is WKT, it needs to be explicitly encoded based on the + // API guide. Log an error and increment the counter in case it is not set. + if (!set_header.has_append()) { + ENVOY_LOG_EVERY_POW_2(error, "set_headers append value is not explicitly set, this is wrong"); + invalid_append_encoding.inc(); + } + } else { + // This is new behavior with default append as true. And "append_action" support + // can be added in this case. + append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(set_header, append, true); + } + return append; +} + absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, Http::HeaderMap& headers, bool replacing_message, const Checker& checker, Counter& rejected_mutations, + Counter& invalid_append_encoding, bool remove_content_length) { // Check whether the remove_headers or set_headers size exceed the HTTP connection manager limit. // Reject the mutation and return error status if either one does. @@ -162,7 +188,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, return absl::InvalidArgumentError("Invalid character in set_headers mutation."); } const LowerCaseString header_name(sh.header().key()); - const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); + const bool append = getAppendFromHeaderMutation(sh, invalid_append_encoding); 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); diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.h b/source/extensions/filters/http/ext_proc/mutation_utils.h index 7e177befea746..c42e775297ede 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.h +++ b/source/extensions/filters/http/ext_proc/mutation_utils.h @@ -33,7 +33,8 @@ class MutationUtils : public Logger::Loggable { applyHeaderMutations(const envoy::service::ext_proc::v3::HeaderMutation& mutation, Http::HeaderMap& headers, bool replacing_message, const Filters::Common::MutationRules::Checker& rule_checker, - Stats::Counter& rejected_mutations, bool remove_content_length = false); + Stats::Counter& rejected_mutations, Stats::Counter& invalid_append_encoding, + bool remove_content_length = false); // Modify a buffer based on a set of mutations from a protobuf static void applyBodyMutations(const envoy::service::ext_proc::v3::BodyMutation& mutation, @@ -59,6 +60,9 @@ class MutationUtils : public Logger::Loggable { // Check whether the header size after mutation is over the HCM size config. static absl::Status headerMutationResultCheck(const Http::HeaderMap& headers, Stats::Counter& rejected_mutations); + static bool + getAppendFromHeaderMutation(const envoy::config::core::v3::HeaderValueOption& set_header, + Stats::Counter& invalid_append_encoding); }; } // namespace ExternalProcessing diff --git a/source/extensions/filters/http/ext_proc/processor_state.cc b/source/extensions/filters/http/ext_proc/processor_state.cc index c0485e1918143..c765bf2b706b4 100644 --- a/source/extensions/filters/http/ext_proc/processor_state.cc +++ b/source/extensions/filters/http/ext_proc/processor_state.cc @@ -114,7 +114,7 @@ absl::Status ProcessorState::processHeaderMutation(const CommonResponse& common_ common_response.header_mutation(), *headers_, common_response.status() == CommonResponse::CONTINUE_AND_REPLACE, filter_.config().mutationChecker(), filter_.stats().rejected_header_mutations_, - shouldRemoveContentLength()); + filter_.stats().invalid_header_append_encoding_, shouldRemoveContentLength()); return mut_status; } @@ -511,7 +511,8 @@ absl::Status ProcessorState::handleTrailersResponse(const TrailersResponse& resp if (response.has_header_mutation() && trailers_ != nullptr) { auto mut_status = MutationUtils::applyHeaderMutations( response.header_mutation(), *trailers_, false, filter_.config().mutationChecker(), - filter_.stats().rejected_header_mutations_); + filter_.stats().rejected_header_mutations_, + filter_.stats().invalid_header_append_encoding_); if (!mut_status.ok()) { filter_.onProcessTrailersResponse(response, mut_status, trafficDirection()); return mut_status; diff --git a/test/extensions/filters/http/ext_proc/BUILD b/test/extensions/filters/http/ext_proc/BUILD index 13e9944bf11c4..e4a5d6de6e5cd 100644 --- a/test/extensions/filters/http/ext_proc/BUILD +++ b/test/extensions/filters/http/ext_proc/BUILD @@ -65,6 +65,7 @@ envoy_extension_cc_test( "//test/mocks/server:server_factory_context_mocks", "//test/proto:helloworld_proto_cc_proto", "//test/test_common:registry_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/http/ext_proc/response_processors/save_processing_response/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 8739e864e9712..e7177bc940d5f 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 @@ -122,6 +122,8 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, void initializeConfig(ConfigOptions config_option = {}, const std::vector>& cluster_endpoints = {{0, 1}, {1, 1}}) { + scoped_runtime_.mergeValues( + {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "false"}}); int total_cluster_endpoints = 0; std::for_each( cluster_endpoints.begin(), cluster_endpoints.end(), diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 17b06dded283c..c1c6af3199772 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -36,6 +36,7 @@ #include "test/mocks/upstream/cluster_manager.h" #include "test/test_common/printers.h" #include "test/test_common/registry.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -97,6 +98,9 @@ static const std::string filter_config_name = "scooby.dooby.doo"; class HttpFilterTest : public testing::Test { protected: void initialize(std::string&& yaml, bool is_upstream_filter = false) { + scoped_runtime_.mergeValues( + {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "false"}}); + client_ = std::make_unique(); route_ = std::make_shared>(); EXPECT_CALL(*client_, start(_, _, _, _)).WillOnce(Invoke(this, &HttpFilterTest::doStart)); @@ -663,6 +667,7 @@ class HttpFilterTest : public testing::Test { envoy::config::core::v3::Metadata dynamic_metadata_; testing::NiceMock connection_; NiceMock factory_context_; + TestScopedRuntime scoped_runtime_; }; // Using the default configuration, test the filter with a processor that @@ -821,6 +826,69 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) { EXPECT_EQ(1, config_->stats().streams_closed_.value()); } +TEST_F(HttpFilterTest, PostAndChangeHeadersAppendDefaulFalse) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + )EOF"); + + // Set the runtime to false to force append default to be false, which is legacy behavior. + scoped_runtime_.mergeValues( + {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "false"}}); + + request_headers_.addCopy(LowerCaseString("x-some-other-header"), "yes"); + 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(); + add1->mutable_header()->set_key("x-some-other-header"); + add1->mutable_header()->set_raw_value("no"); + }); + + // We should now have changed the original header a bit + TestRequestHeaderMapImpl expected{{":path", "/"}, + {":method", "POST"}, + {":scheme", "http"}, + {"host", "host"}, + {"x-some-other-header", "no"}}; + EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected)); + filter_->onDestroy(); +} + +TEST_F(HttpFilterTest, PostAndChangeHeadersAppendDefaulTrue) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + )EOF"); + + // Set the runtime to true to force append default to be true. + scoped_runtime_.mergeValues( + {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "true"}}); + + request_headers_.addCopy(LowerCaseString("x-some-other-header"), "yes"); + 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(); + add1->mutable_header()->set_key("x-some-other-header"); + add1->mutable_header()->set_raw_value("no"); + }); + + // We should now have changed the original header a bit + TestRequestHeaderMapImpl expected{{":path", "/"}, + {":method", "POST"}, + {":scheme", "http"}, + {"host", "host"}, + {"x-some-other-header", "yes"}, + {"x-some-other-header", "no"}}; + EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected)); + filter_->onDestroy(); +} + // Using the default configuration, test the filter with a processor that // replies to the request_headers message with an "immediate response" message // that should result in a response being directly sent downstream with 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 210836d0288be..449c2ff43d5d1 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -136,11 +136,13 @@ TEST_F(MutationUtilsTest, TestApplyMutations) { // Use the default mutation rules Checker checker(HeaderMutationRules::default_instance(), regex_engine_); - Envoy::Stats::MockCounter rejections; + Envoy::Stats::MockCounter rejections, invalid_append; EXPECT_CALL(rejections, inc()).Times(10); + EXPECT_CALL(invalid_append, inc()).Times(10); // There were 10 attempts to change un-changeable headers above. - EXPECT_TRUE( - MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections, + invalid_append) + .ok()); Http::TestRequestHeaderMapImpl expected_headers{ {":scheme", "https"}, @@ -185,10 +187,12 @@ TEST_F(MutationUtilsTest, TestNonAppendableHeaders) { // Use the default mutation rules Checker checker(HeaderMutationRules::default_instance(), regex_engine_); // There were two invalid attempts above. - Envoy::Stats::MockCounter rejections; + Envoy::Stats::MockCounter rejections, invalid_append; EXPECT_CALL(rejections, inc()).Times(2); - EXPECT_TRUE( - MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + EXPECT_CALL(invalid_append, inc()); + EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections, + invalid_append) + .ok()); Http::TestRequestHeaderMapImpl expected_headers{ {":path", "/foo"}, @@ -203,15 +207,16 @@ TEST_F(MutationUtilsTest, TestSetHeaderWithInvalidCharacter) { {"host", "localhost:1000"}, }; Checker checker(HeaderMutationRules::default_instance(), regex_engine_); - Envoy::Stats::MockCounter rejections; + Envoy::Stats::MockCounter rejections, invalid_append; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); // Test header key contains invalid character. s->mutable_header()->set_key("x-append-this\n"); s->mutable_header()->set_raw_value("value"); EXPECT_CALL(rejections, inc()); - EXPECT_FALSE( - MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + EXPECT_FALSE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections, + invalid_append) + .ok()); mutation.Clear(); s = mutation.add_set_headers(); @@ -219,8 +224,9 @@ TEST_F(MutationUtilsTest, TestSetHeaderWithInvalidCharacter) { s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_raw_value("value\r"); EXPECT_CALL(rejections, inc()); - EXPECT_FALSE( - MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + EXPECT_FALSE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections, + invalid_append) + .ok()); } TEST_F(MutationUtilsTest, TestSetHeaderWithContentLength) { @@ -232,7 +238,7 @@ TEST_F(MutationUtilsTest, TestSetHeaderWithContentLength) { }; // Use the default mutation rules Checker checker(HeaderMutationRules::default_instance(), regex_engine_); - Envoy::Stats::MockCounter rejections; + Envoy::Stats::MockCounter rejections, invalid_append; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); // Test header key contains content_length. @@ -240,12 +246,15 @@ TEST_F(MutationUtilsTest, TestSetHeaderWithContentLength) { s->mutable_header()->set_raw_value("10"); EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections, + invalid_append, /*remove_content_length=*/true) .ok()); // When `remove_content_length` is true, content_length headers is not added. EXPECT_EQ(headers.ContentLength(), nullptr); + EXPECT_CALL(invalid_append, inc()); EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections, + invalid_append, /*remove_content_length=*/false) .ok()); // When `remove_content_length` is false, content_length headers is added. @@ -260,10 +269,11 @@ TEST_F(MutationUtilsTest, TestRemoveHeaderWithInvalidCharacter) { envoy::service::ext_proc::v3::HeaderMutation mutation; mutation.add_remove_headers("host\n"); Checker checker(HeaderMutationRules::default_instance(), regex_engine_); - Envoy::Stats::MockCounter rejections; + Envoy::Stats::MockCounter rejections, invalid_append; EXPECT_CALL(rejections, inc()); - EXPECT_FALSE( - MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + EXPECT_FALSE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections, + invalid_append) + .ok()); } // Ensure that we actually replace the body