From 5b8758f2e5ab2904ffef4b4ab2223fd3e6ca3f7f Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Fri, 29 Sep 2023 02:59:12 +0000 Subject: [PATCH 01/36] add test case Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/filter_test.cc | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index fcf4b4421c1b5..3bf42c48ca217 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -3204,6 +3204,43 @@ TEST_F(HttpFilter2Test, LastEncodeDataCallExceedsStreamBufferLimitWouldJustRaise conn_manager_->onData(fake_input, false); } +TEST_F(HttpFilterTest, MultipleSetCookieHeaders) { + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + failure_mode_allow: true + )EOF"); + + // Create synthetic HTTP request + request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); + + // Simulate multiple Set-Cookie headers in the response + response_headers_.addCopy(LowerCaseString(":status"), "200"); + response_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); + response_headers_.addCopy(LowerCaseString("set-cookie"), "cookie1=value1"); + response_headers_.addCopy(LowerCaseString("set-cookie"), "cookie2=value2"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + + // Validate that multiple Set-Cookie headers are preserved + processResponseHeaders(false, + [](const HttpHeaders& header_resp, ProcessingResponse&, HeadersResponse&) { + EXPECT_FALSE(header_resp.end_of_stream()); + TestRequestHeaderMapImpl expected_response{ + {":status", "200"}, + {"content-type", "text/plain"}, + {"set-cookie", "cookie1=value1"}, + {"set-cookie", "cookie2=value2"} + }; + EXPECT_THAT(header_resp.headers(), HeaderProtosEqual(expected_response)); + }); + + filter_->onDestroy(); +} + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters From 16e39bd16ef9cbb95fc629890e1e6f9e7b00e6a0 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Fri, 29 Sep 2023 03:35:28 +0000 Subject: [PATCH 02/36] fix format issue Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/filter_test.cc | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 3bf42c48ca217..c3ee878d0aa54 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -3226,17 +3226,15 @@ TEST_F(HttpFilterTest, MultipleSetCookieHeaders) { EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); // Validate that multiple Set-Cookie headers are preserved - processResponseHeaders(false, - [](const HttpHeaders& header_resp, ProcessingResponse&, HeadersResponse&) { - EXPECT_FALSE(header_resp.end_of_stream()); - TestRequestHeaderMapImpl expected_response{ - {":status", "200"}, - {"content-type", "text/plain"}, - {"set-cookie", "cookie1=value1"}, - {"set-cookie", "cookie2=value2"} - }; - EXPECT_THAT(header_resp.headers(), HeaderProtosEqual(expected_response)); - }); + processResponseHeaders( + false, [](const HttpHeaders& header_resp, ProcessingResponse&, HeadersResponse&) { + EXPECT_FALSE(header_resp.end_of_stream()); + TestRequestHeaderMapImpl expected_response{{":status", "200"}, + {"content-type", "text/plain"}, + {"set-cookie", "cookie1=value1"}, + {"set-cookie", "cookie2=value2"}}; + EXPECT_THAT(header_resp.headers(), HeaderProtosEqual(expected_response)); + }); filter_->onDestroy(); } From cce7b8944a5449dacbd24d4b627a8fe0a93df647 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Tue, 3 Oct 2023 21:50:58 +0000 Subject: [PATCH 03/36] honor HeaderValueOption.append_action, if append_action does not exist, honor append option to avoid breaking change Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 80 +++++++++++++------ .../filters/http/ext_proc/filter_test.cc | 35 -------- 2 files changed, 54 insertions(+), 61 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index d56cd20c2fb78..d2b5e0379d9fe 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -24,6 +24,9 @@ using Stats::Counter; using envoy::service::ext_proc::v3::BodyMutation; using envoy::service::ext_proc::v3::HeaderMutation; +using HeaderAppendAction = envoy::config::core::v3::HeaderValueOption::HeaderAppendAction; +using HeaderValueOption = envoy::config::core::v3::HeaderValueOption; + bool MutationUtils::headerInMatcher( absl::string_view key, const std::vector& header_matchers) { return std::any_of(header_matchers.begin(), header_matchers.end(), @@ -174,33 +177,58 @@ 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 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; - } - switch (check_result) { - case CheckResult::OK: - ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); - if (append) { - headers.addCopy(header_name, header_value); - } else { - headers.setCopy(header_name, header_value); + + if (sh.append_action()) { + switch (sh.append_action()) { + case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: + headers.addReferenceKey(header_name, header_value); + break; + case HeaderValueOption::ADD_IF_ABSENT: + if (headers.get(header_name).empty()) { + headers.addReferenceKey(header_name, header_value); + } + break; + case HeaderValueOption::OVERWRITE_IF_EXISTS: + if (!headers.get(header_name).empty()) { + headers.setReferenceKey(header_name, header_value); + } + break; + case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: + headers.setReferenceKey(header_name, header_value); + break; + default: + // Handle unknown/invalid append_action value + return absl::InvalidArgumentError("Invalid append_action value"); + } + } else { + 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; + } + switch (check_result) { + case CheckResult::OK: + ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); + if (append) { + headers.addCopy(header_name, header_value); + } else { + headers.setCopy(header_name, header_value); + } + break; + case CheckResult::IGNORE: + ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name); + rejected_mutations.inc(); + break; + case CheckResult::FAIL: + ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); + rejected_mutations.inc(); + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); } - break; - case CheckResult::IGNORE: - ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name); - rejected_mutations.inc(); - break; - case CheckResult::FAIL: - ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); - rejected_mutations.inc(); - return absl::InvalidArgumentError( - absl::StrCat("Invalid attempt to modify ", static_cast(header_name))); } } diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index c3ee878d0aa54..fcf4b4421c1b5 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -3204,41 +3204,6 @@ TEST_F(HttpFilter2Test, LastEncodeDataCallExceedsStreamBufferLimitWouldJustRaise conn_manager_->onData(fake_input, false); } -TEST_F(HttpFilterTest, MultipleSetCookieHeaders) { - initialize(R"EOF( - grpc_service: - envoy_grpc: - cluster_name: "ext_proc_server" - failure_mode_allow: true - )EOF"); - - // Create synthetic HTTP request - request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); - - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); - - // Simulate multiple Set-Cookie headers in the response - response_headers_.addCopy(LowerCaseString(":status"), "200"); - response_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); - response_headers_.addCopy(LowerCaseString("set-cookie"), "cookie1=value1"); - response_headers_.addCopy(LowerCaseString("set-cookie"), "cookie2=value2"); - - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); - - // Validate that multiple Set-Cookie headers are preserved - processResponseHeaders( - false, [](const HttpHeaders& header_resp, ProcessingResponse&, HeadersResponse&) { - EXPECT_FALSE(header_resp.end_of_stream()); - TestRequestHeaderMapImpl expected_response{{":status", "200"}, - {"content-type", "text/plain"}, - {"set-cookie", "cookie1=value1"}, - {"set-cookie", "cookie2=value2"}}; - EXPECT_THAT(header_resp.headers(), HeaderProtosEqual(expected_response)); - }); - - filter_->onDestroy(); -} - } // namespace } // namespace ExternalProcessing } // namespace HttpFilters From 37185a7f3d0b382d543e819dd807d3b4576ef02b Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Wed, 4 Oct 2023 01:41:50 +0000 Subject: [PATCH 04/36] add test cases Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 6 +- .../http/ext_proc/mutation_utils_test.cc | 101 ++++++++++++++++++ 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index d2b5e0379d9fe..0ec95bdb56253 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -188,14 +188,14 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, headers.addReferenceKey(header_name, header_value); } break; + case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: + headers.setReferenceKey(header_name, header_value); + break; case HeaderValueOption::OVERWRITE_IF_EXISTS: if (!headers.get(header_name).empty()) { headers.setReferenceKey(header_name, header_value); } break; - case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: - headers.setReferenceKey(header_name, header_value); - break; default: // Handle unknown/invalid append_action value return absl::InvalidArgumentError("Invalid append_action value"); 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 b193e10e84c36..6cde52d75d4f0 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -412,6 +412,107 @@ TEST(MutationUtils, TestDisallowHeaderSetNotAllowHeader) { EXPECT_THAT(proto_headers, HeaderProtosEqual(expected)); } +TEST(MutationUtils, TestAppendActionAppendIfExists) { + Http::TestRequestHeaderMapImpl headers{ + {"set-cookie", "Value123"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + auto* set_header = mutation.add_set_headers(); + set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_value("Value234"); + set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + + Checker checker(HeaderMutationRules::default_instance()); + Envoy::Stats::MockCounter rejections; + + EXPECT_TRUE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + + Http::TestRequestHeaderMapImpl expected_headers{ + {"set-cookie", "Value123, Value234"}, + }; + + EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); +} + +TEST(MutationUtils, TestAppendActionAddIfAbsent) { + Http::TestRequestHeaderMapImpl headers{ + {"set-cookie", "Value123"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + auto* set_header = mutation.add_set_headers(); + set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_value("Value234"); + set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + + Checker checker(HeaderMutationRules::default_instance()); + Envoy::Stats::MockCounter rejections; + + EXPECT_TRUE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + + Http::TestRequestHeaderMapImpl expected_headers{ + {"set-cookie", "Value123"}, + }; + + EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); +} + +TEST(MutationUtils, TestAppendActionOverwriteIfExists) { + Http::TestRequestHeaderMapImpl headers{ + {"set-cookie", "Value123"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + auto* set_header = mutation.add_set_headers(); + set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_value("Value234"); + set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + + Checker checker(HeaderMutationRules::default_instance()); + Envoy::Stats::MockCounter rejections; + + EXPECT_TRUE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + + Http::TestRequestHeaderMapImpl expected_headers{ + {"set-cookie", "Value234"}, + }; + + EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); +} + +TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { + Http::TestRequestHeaderMapImpl headers{ + {"set-cookie", "Value123"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + auto* set_header = mutation.add_set_headers(); + set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_value("Value234"); + set_header->set_append_action( + envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + + Checker checker(HeaderMutationRules::default_instance()); + Envoy::Stats::MockCounter rejections; + + EXPECT_TRUE( + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); + + Http::TestRequestHeaderMapImpl expected_headers{ + {"set-cookie", "Value234"}, + }; + + EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); +} + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters From 44b52977f0898bd992109a24089977207e6c35c2 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Thu, 5 Oct 2023 23:16:01 +0000 Subject: [PATCH 05/36] fix tests Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- source/extensions/filters/http/ext_proc/mutation_utils.cc | 3 +++ test/extensions/filters/http/ext_proc/mutation_utils_test.cc | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 0ec95bdb56253..71de78badbff5 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -178,6 +178,9 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } const LowerCaseString header_name(sh.header().key()); + ENVOY_LOG(debug, "append_action value: {}", sh.append_action()); + ENVOY_LOG(info, "append_action value: {}", sh.append_action()); + if (sh.append_action()) { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: 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 6cde52d75d4f0..45df49eb9771b 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -412,7 +412,7 @@ TEST(MutationUtils, TestDisallowHeaderSetNotAllowHeader) { EXPECT_THAT(proto_headers, HeaderProtosEqual(expected)); } -TEST(MutationUtils, TestAppendActionAppendIfExists) { +TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { Http::TestRequestHeaderMapImpl headers{ {"set-cookie", "Value123"}, }; From cf742c27c3de7ae791dfb33f63c44cd2e3e4aa00 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Fri, 6 Oct 2023 17:13:15 +0000 Subject: [PATCH 06/36] fix test Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- source/extensions/filters/http/ext_proc/mutation_utils.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 71de78badbff5..a573cbe45265a 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -178,8 +178,8 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } const LowerCaseString header_name(sh.header().key()); - ENVOY_LOG(debug, "append_action value: {}", sh.append_action()); - ENVOY_LOG(info, "append_action value: {}", sh.append_action()); + ENVOY_LOG(warn, "append_action value: {}", sh.append_action()); + ENVOY_LOG(error, "append_action value: {}", sh.append_action()); if (sh.append_action()) { switch (sh.append_action()) { From 98c852fe7cb1e05affc548f620f8fa8ca6918c7a Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 03:02:51 +0000 Subject: [PATCH 07/36] fix tests Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 12 +++++++----- .../filters/http/ext_proc/mutation_utils_test.cc | 8 ++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index a573cbe45265a..d7286d57602d6 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -178,25 +178,27 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } const LowerCaseString header_name(sh.header().key()); - ENVOY_LOG(warn, "append_action value: {}", sh.append_action()); ENVOY_LOG(error, "append_action value: {}", sh.append_action()); + ENVOY_LOG(error, "header name: {}", header_name); + ENVOY_LOG(error, "header value: {}, header raw value: {}", sh.header().value(), + sh.header().raw_value()); if (sh.append_action()) { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: - headers.addReferenceKey(header_name, header_value); + headers.addCopy(header_name, header_value); break; case HeaderValueOption::ADD_IF_ABSENT: if (headers.get(header_name).empty()) { - headers.addReferenceKey(header_name, header_value); + headers.addCopy(header_name, header_value); } break; case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: - headers.setReferenceKey(header_name, header_value); + headers.setCopy(header_name, header_value); break; case HeaderValueOption::OVERWRITE_IF_EXISTS: if (!headers.get(header_name).empty()) { - headers.setReferenceKey(header_name, header_value); + headers.setCopy(header_name, header_value); } break; default: 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 45df49eb9771b..9d67cdccdb02e 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -413,6 +413,8 @@ TEST(MutationUtils, TestDisallowHeaderSetNotAllowHeader) { } TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ {"set-cookie", "Value123"}, }; @@ -438,6 +440,8 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { } TEST(MutationUtils, TestAppendActionAddIfAbsent) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ {"set-cookie", "Value123"}, }; @@ -463,6 +467,8 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { } TEST(MutationUtils, TestAppendActionOverwriteIfExists) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ {"set-cookie", "Value123"}, }; @@ -488,6 +494,8 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { } TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ {"set-cookie", "Value123"}, }; From 00acddf3a6fb095d9a686caeb3b31db459c97d12 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 15:58:52 +0000 Subject: [PATCH 08/36] add checker to append_action Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 139 ++++++++++++++---- .../filters/http/ext_proc/mutation_utils.h | 6 + .../http/ext_proc/mutation_utils_test.cc | 3 +- 3 files changed, 117 insertions(+), 31 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index d7286d57602d6..8146b3b610ab2 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -184,56 +184,97 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, sh.header().raw_value()); if (sh.append_action()) { + bool append_mode; + CheckOperation check_op; + CheckResult checkResult; switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: - headers.addCopy(header_name, header_value); + append_mode = true; + check_op = + (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; + checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, + check_op, header_name, header_value, sh, append_mode); + if (checkResult == CheckResult::FAIL) { + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); + } break; case HeaderValueOption::ADD_IF_ABSENT: if (headers.get(header_name).empty()) { - headers.addCopy(header_name, header_value); + append_mode = true; + check_op = CheckOperation::SET; + checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, + check_op, header_name, header_value, sh, append_mode); + if (checkResult == CheckResult::FAIL) { + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); + } } break; case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: - headers.setCopy(header_name, header_value); + append_mode = false; + check_op =CheckOperation::SET; + checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, + check_op, header_name, header_value, sh, append_mode); + if (checkResult == CheckResult::FAIL) { + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); + } break; case HeaderValueOption::OVERWRITE_IF_EXISTS: if (!headers.get(header_name).empty()) { - headers.setCopy(header_name, header_value); + append_mode = false; + check_op = CheckOperation::SET; + checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, + check_op, header_name, header_value, sh, append_mode); + if (checkResult == CheckResult::FAIL) { + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); + } } break; default: // Handle unknown/invalid append_action value - return absl::InvalidArgumentError("Invalid append_action value"); + return absl::InvalidArgumentError(absl::StrCat( + "Invalid append_action value ", static_cast(header_name))); } } else { - 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; - } - switch (check_result) { - case CheckResult::OK: - ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); - if (append) { - headers.addCopy(header_name, header_value); - } else { - headers.setCopy(header_name, header_value); - } - break; - case CheckResult::IGNORE: - ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name); - rejected_mutations.inc(); - break; - case CheckResult::FAIL: - ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); - rejected_mutations.inc(); + const bool append_mode = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); + const auto check_op = (append_mode && !headers.get(header_name).empty()) + ? CheckOperation::APPEND + : CheckOperation::SET; + auto checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, + check_op, header_name, header_value, sh, append_mode); + if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); } + /* + 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; + } + switch (check_result) { + case CheckResult::OK: + ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); + if (append) { + headers.addCopy(header_name, header_value); + } else { + headers.setCopy(header_name, header_value); + } + break; + case CheckResult::IGNORE: + ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name); + rejected_mutations.inc(); + break; + case CheckResult::FAIL: + ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); + rejected_mutations.inc(); + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); + } */ } } @@ -241,6 +282,44 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, return headerMutationResultCheck(headers, rejected_mutations); } +// Define a common function to handle the check result logic +Filters::Common::MutationRules::CheckResult +MutationUtils::handleCheckResult(Http::HeaderMap& headers, bool replacing_message, + const Filters::Common::MutationRules::Checker& checker, + Stats::Counter& rejected_mutations, + Filters::Common::MutationRules::CheckOperation check_op, + Http::LowerCaseString header_name, absl::string_view header_value, + envoy::config::core::v3::HeaderValueOption sh, bool append_mode) { + 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; + } + + switch (check_result) { + case CheckResult::OK: + ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append_mode); + if (append_mode) { + headers.addCopy(header_name, header_value); + } else { + headers.setCopy(header_name, header_value); + } + break; + case CheckResult::IGNORE: + ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name); + rejected_mutations.inc(); + break; + case CheckResult::FAIL: + ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); + rejected_mutations.inc(); + // You can add additional handling for the FAIL case here if needed. + break; + } + + return check_result; +} + void MutationUtils::applyBodyMutations(const BodyMutation& mutation, Buffer::Instance& buffer) { switch (mutation.mutation_case()) { case BodyMutation::MutationCase::kClearBody: diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.h b/source/extensions/filters/http/ext_proc/mutation_utils.h index bc5c1e7c00d74..49e5a1c3e419c 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.h +++ b/source/extensions/filters/http/ext_proc/mutation_utils.h @@ -58,6 +58,12 @@ 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 Filters::Common::MutationRules::CheckResult + handleCheckResult(Http::HeaderMap& headers, bool replacing_message, + const Filters::Common::MutationRules::Checker& checker, Stats::Counter& rejected_mutations, + Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name, + absl::string_view header_value, envoy::config::core::v3::HeaderValueOption sh, bool append_mode); }; } // namespace ExternalProcessing 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 9d67cdccdb02e..9cbc2ac45ec27 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -433,7 +433,8 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); Http::TestRequestHeaderMapImpl expected_headers{ - {"set-cookie", "Value123, Value234"}, + {"set-cookie", "Value123"}, + {"set-cookie", "Value234"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); From cc4a88fc9ce8ac03248dd90a6658d054a7afb19a Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 16:07:03 +0000 Subject: [PATCH 09/36] add checker to append_action Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../extensions/filters/http/ext_proc/mutation_utils.cc | 4 ++-- .../extensions/filters/http/ext_proc/mutation_utils.h | 10 ++++++---- .../filters/http/ext_proc/mutation_utils_test.cc | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 8146b3b610ab2..f4abd2c46de8f 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -213,13 +213,13 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, break; case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: append_mode = false; - check_op =CheckOperation::SET; + check_op = CheckOperation::SET; checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, check_op, header_name, header_value, sh, append_mode); if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); - } + } break; case HeaderValueOption::OVERWRITE_IF_EXISTS: if (!headers.get(header_name).empty()) { diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.h b/source/extensions/filters/http/ext_proc/mutation_utils.h index 49e5a1c3e419c..2f34816153583 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.h +++ b/source/extensions/filters/http/ext_proc/mutation_utils.h @@ -60,10 +60,12 @@ class MutationUtils : public Logger::Loggable { Stats::Counter& rejected_mutations); static Filters::Common::MutationRules::CheckResult - handleCheckResult(Http::HeaderMap& headers, bool replacing_message, - const Filters::Common::MutationRules::Checker& checker, Stats::Counter& rejected_mutations, - Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name, - absl::string_view header_value, envoy::config::core::v3::HeaderValueOption sh, bool append_mode); + handleCheckResult(Http::HeaderMap& headers, bool replacing_message, + const Filters::Common::MutationRules::Checker& checker, + Stats::Counter& rejected_mutations, + Filters::Common::MutationRules::CheckOperation check_op, + Http::LowerCaseString header_name, absl::string_view header_value, + envoy::config::core::v3::HeaderValueOption sh, bool append_mode); }; } // namespace ExternalProcessing 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 9cbc2ac45ec27..bc9246e63a165 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -433,8 +433,8 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); Http::TestRequestHeaderMapImpl expected_headers{ - {"set-cookie", "Value123"}, - {"set-cookie", "Value234"}, + {"set-cookie", "Value123"}, + {"set-cookie", "Value234"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); From c6372f37276171b52d976663484889d1482a9460 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 16:55:06 +0000 Subject: [PATCH 10/36] test case append or add Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 4 +-- .../filters/http/ext_proc/mutation_utils.h | 2 +- .../http/ext_proc/mutation_utils_test.cc | 26 +++++++++---------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index f4abd2c46de8f..2bab40f0308ce 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -289,7 +289,7 @@ MutationUtils::handleCheckResult(Http::HeaderMap& headers, bool replacing_messag Stats::Counter& rejected_mutations, Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name, absl::string_view header_value, - envoy::config::core::v3::HeaderValueOption sh, bool append_mode) { + bool append_mode) { 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 @@ -299,7 +299,7 @@ MutationUtils::handleCheckResult(Http::HeaderMap& headers, bool replacing_messag switch (check_result) { case CheckResult::OK: - ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append_mode); + ENVOY_LOG(error, "Setting header {} append = {}", header_name, append_mode); if (append_mode) { headers.addCopy(header_name, header_value); } else { diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.h b/source/extensions/filters/http/ext_proc/mutation_utils.h index 2f34816153583..5e8e7d37f0a2a 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.h +++ b/source/extensions/filters/http/ext_proc/mutation_utils.h @@ -65,7 +65,7 @@ class MutationUtils : public Logger::Loggable { Stats::Counter& rejected_mutations, Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name, absl::string_view header_value, - envoy::config::core::v3::HeaderValueOption sh, bool append_mode); + bool append_mode); }; } // namespace ExternalProcessing 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 bc9246e63a165..8ed7042732fcc 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -152,7 +152,7 @@ TEST(MutationUtils, TestApplyMutations) { {"x-append-this", "3"}, {"x-remove-and-append-this", "4"}, {"x-replace-this", "nope"}, - {"x-envoy-strange-thing", "No"}, + /* {"x-envoy-strange-thing", "No"}, */ }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); @@ -416,12 +416,12 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ - {"set-cookie", "Value123"}, + {"Set-Cookie", "Value123"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_key("Set-Cookie"); set_header->mutable_header()->set_value("Value234"); set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); @@ -433,8 +433,8 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); Http::TestRequestHeaderMapImpl expected_headers{ - {"set-cookie", "Value123"}, - {"set-cookie", "Value234"}, + {"Set-Cookie", "Value123"}, + {"Set-Cookie", "Value234"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); @@ -444,12 +444,12 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ - {"set-cookie", "Value123"}, + {"Set-Cookie", "Value123"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_key("Set-Cookie"); set_header->mutable_header()->set_value("Value234"); set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); @@ -461,7 +461,7 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); Http::TestRequestHeaderMapImpl expected_headers{ - {"set-cookie", "Value123"}, + {"Set-cookie", "Value123"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); @@ -471,12 +471,12 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ - {"set-cookie", "Value123"}, + {"Set-Cookie", "Value123"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_key("Set-Cookie"); set_header->mutable_header()->set_value("Value234"); set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); @@ -498,12 +498,12 @@ TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ - {"set-cookie", "Value123"}, + {"Set-Cookie", "Value123"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("set-cookie"); + set_header->mutable_header()->set_key("Set-Cookie"); set_header->mutable_header()->set_value("Value234"); set_header->set_append_action( envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: @@ -516,7 +516,7 @@ TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); Http::TestRequestHeaderMapImpl expected_headers{ - {"set-cookie", "Value234"}, + {"Set-Cookie", "Value234"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); From ba085b3f983184ecb0b1c83a287bf343c840f91a Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 17:09:20 +0000 Subject: [PATCH 11/36] test case append or add Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 12 +++++------- .../filters/http/ext_proc/mutation_utils.h | 12 +++++------- .../filters/http/ext_proc/mutation_utils_test.cc | 2 +- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 2bab40f0308ce..fa269fd12b935 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -283,13 +283,11 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } // Define a common function to handle the check result logic -Filters::Common::MutationRules::CheckResult -MutationUtils::handleCheckResult(Http::HeaderMap& headers, bool replacing_message, - const Filters::Common::MutationRules::Checker& checker, - Stats::Counter& rejected_mutations, - Filters::Common::MutationRules::CheckOperation check_op, - Http::LowerCaseString header_name, absl::string_view header_value, - bool append_mode) { +Filters::Common::MutationRules::CheckResult MutationUtils::handleCheckResult( + Http::HeaderMap& headers, bool replacing_message, + const Filters::Common::MutationRules::Checker& checker, Stats::Counter& rejected_mutations, + Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name, + absl::string_view header_value, bool append_mode) { 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 diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.h b/source/extensions/filters/http/ext_proc/mutation_utils.h index 5e8e7d37f0a2a..795cca95d95cf 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.h +++ b/source/extensions/filters/http/ext_proc/mutation_utils.h @@ -59,13 +59,11 @@ class MutationUtils : public Logger::Loggable { static absl::Status headerMutationResultCheck(const Http::HeaderMap& headers, Stats::Counter& rejected_mutations); - static Filters::Common::MutationRules::CheckResult - handleCheckResult(Http::HeaderMap& headers, bool replacing_message, - const Filters::Common::MutationRules::Checker& checker, - Stats::Counter& rejected_mutations, - Filters::Common::MutationRules::CheckOperation check_op, - Http::LowerCaseString header_name, absl::string_view header_value, - bool append_mode); + static Filters::Common::MutationRules::CheckResult handleCheckResult( + Http::HeaderMap& headers, bool replacing_message, + const Filters::Common::MutationRules::Checker& checker, Stats::Counter& rejected_mutations, + Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name, + absl::string_view header_value, bool append_mode); }; } // namespace ExternalProcessing 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 8ed7042732fcc..2d417b470c0a6 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -152,7 +152,7 @@ TEST(MutationUtils, TestApplyMutations) { {"x-append-this", "3"}, {"x-remove-and-append-this", "4"}, {"x-replace-this", "nope"}, - /* {"x-envoy-strange-thing", "No"}, */ + /* {"x-envoy-strange-thing", "No"}, */ }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); From 32d658975741889fd8a70e9093beafb583fbae85 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 17:30:19 +0000 Subject: [PATCH 12/36] test case append or add Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../extensions/filters/http/ext_proc/mutation_utils.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index fa269fd12b935..f751b1055dd1b 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -193,7 +193,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, check_op = (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, sh, append_mode); + check_op, header_name, header_value, append_mode); if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); @@ -204,7 +204,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, append_mode = true; check_op = CheckOperation::SET; checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, sh, append_mode); + check_op, header_name, header_value, append_mode); if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); @@ -215,7 +215,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, append_mode = false; check_op = CheckOperation::SET; checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, sh, append_mode); + check_op, header_name, header_value, append_mode); if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); @@ -226,7 +226,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, append_mode = false; check_op = CheckOperation::SET; checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, sh, append_mode); + check_op, header_name, header_value, append_mode); if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); @@ -244,7 +244,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, ? CheckOperation::APPEND : CheckOperation::SET; auto checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, sh, append_mode); + check_op, header_name, header_value, append_mode); if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); From f70f49510dd85f9a51c78fdec07a7631f1c0ef00 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 19:25:43 +0000 Subject: [PATCH 13/36] update test Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils_test.cc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) 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 2d417b470c0a6..c4b3e4f88984a 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -63,6 +63,7 @@ TEST(MutationUtils, TestApplyMutations) { {"x-replace-this", "Yes"}, {"x-remove-this", "Yes"}, {"x-envoy-strange-thing", "No"}, + {"Set-Cookie", "xyz"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; @@ -130,6 +131,10 @@ TEST(MutationUtils, TestApplyMutations) { s = mutation.add_set_headers(); s->mutable_header()->set_key(":status"); s->mutable_header()->set_value("100"); + s = mutation.add_set_headers(); +s->mutable_append()->set_value(true); +s->mutable_header()->set_key("Set-Cookie"); +s->mutable_header()->set_value("3"); // Use the default mutation rules Checker checker(HeaderMutationRules::default_instance()); @@ -152,7 +157,9 @@ TEST(MutationUtils, TestApplyMutations) { {"x-append-this", "3"}, {"x-remove-and-append-this", "4"}, {"x-replace-this", "nope"}, - /* {"x-envoy-strange-thing", "No"}, */ + {"x-envoy-strange-thing", "No"}, + {"Set-Cookie", "xyz"}, + {"Set-Cookie", "3"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); @@ -415,7 +422,7 @@ TEST(MutationUtils, TestDisallowHeaderSetNotAllowHeader) { TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); - Http::TestRequestHeaderMapImpl headers{ + Http::TestResponseHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, }; @@ -432,7 +439,7 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { EXPECT_TRUE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); - Http::TestRequestHeaderMapImpl expected_headers{ + Http::TestResponseHeaderMapImpl expected_headers{ {"Set-Cookie", "Value123"}, {"Set-Cookie", "Value234"}, }; From 872aaa3b30272569f651928d776fff0cb32ac772 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 7 Oct 2023 19:36:32 +0000 Subject: [PATCH 14/36] fix format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 c4b3e4f88984a..c1f55397bd5c2 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -63,7 +63,7 @@ TEST(MutationUtils, TestApplyMutations) { {"x-replace-this", "Yes"}, {"x-remove-this", "Yes"}, {"x-envoy-strange-thing", "No"}, - {"Set-Cookie", "xyz"}, + {"Set-Cookie", "xyz"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; @@ -132,9 +132,9 @@ TEST(MutationUtils, TestApplyMutations) { s->mutable_header()->set_key(":status"); s->mutable_header()->set_value("100"); s = mutation.add_set_headers(); -s->mutable_append()->set_value(true); -s->mutable_header()->set_key("Set-Cookie"); -s->mutable_header()->set_value("3"); + s->mutable_append()->set_value(true); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("3"); // Use the default mutation rules Checker checker(HeaderMutationRules::default_instance()); From fbd6de87eece76c463b457319761336ca382bfd9 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sun, 8 Oct 2023 00:26:47 +0000 Subject: [PATCH 15/36] debug test Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 60 +++++++++++-------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index f751b1055dd1b..e9a2fbc327983 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -149,6 +149,10 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } } + bool append_mode_for_append_action; + Filters::Common::MutationRules::CheckOperation check_op_for_append_action; + Filters::Common::MutationRules::CheckResult checkResult_for_append_action; + for (const auto& sh : mutation.set_headers()) { if (!sh.has_header()) { continue; @@ -183,51 +187,59 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, ENVOY_LOG(error, "header value: {}, header raw value: {}", sh.header().value(), sh.header().raw_value()); - if (sh.append_action()) { - bool append_mode; - CheckOperation check_op; - CheckResult checkResult; + if (!sh.has_append()) { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: - append_mode = true; - check_op = + ENVOY_LOG(error, "Inside append action APPEND_IF_EXISTS_OR_ADD {} ", + HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + append_mode_for_append_action = true; + check_op_for_append_action = (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; - checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, append_mode); - if (checkResult == CheckResult::FAIL) { + checkResult_for_append_action = handleCheckResult( + headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, + header_name, header_value, append_mode_for_append_action); + if (checkResult_for_append_action == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); } break; case HeaderValueOption::ADD_IF_ABSENT: + ENVOY_LOG(error, "Inside append action ADD_IF_ABSENT {}", HeaderValueOption::ADD_IF_ABSENT); if (headers.get(header_name).empty()) { - append_mode = true; - check_op = CheckOperation::SET; - checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, append_mode); - if (checkResult == CheckResult::FAIL) { + append_mode_for_append_action = true; + check_op_for_append_action = CheckOperation::SET; + checkResult_for_append_action = handleCheckResult( + headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, + header_name, header_value, append_mode_for_append_action); + if (checkResult_for_append_action == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); } } break; case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: - append_mode = false; - check_op = CheckOperation::SET; - checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, append_mode); - if (checkResult == CheckResult::FAIL) { + ENVOY_LOG(error, "Inside append action OVERWRITE_IF_EXISTS_OR_ADD {}", + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + append_mode_for_append_action = false; + check_op_for_append_action = CheckOperation::SET; + checkResult_for_append_action = handleCheckResult( + headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, + header_name, header_value, append_mode_for_append_action); + if (checkResult_for_append_action == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); } break; case HeaderValueOption::OVERWRITE_IF_EXISTS: + ENVOY_LOG(error, "Inside append action OVERWRITE_IF_EXISTS{}", + HeaderValueOption::OVERWRITE_IF_EXISTS); if (!headers.get(header_name).empty()) { - append_mode = false; - check_op = CheckOperation::SET; - checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, append_mode); - if (checkResult == CheckResult::FAIL) { + append_mode_for_append_action = false; + check_op_for_append_action = CheckOperation::SET; + checkResult_for_append_action = handleCheckResult( + headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, + header_name, header_value, append_mode_for_append_action); + if (checkResult_for_append_action == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); } From ad337c298a56325b66d4d42680c08d372a3ad782 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sun, 8 Oct 2023 02:58:33 +0000 Subject: [PATCH 16/36] add false runtime guard Co-authored-by: tony-2023 <138949958+tony-2023@users.noreply.github.com> Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- changelogs/current.yaml | 4 ++++ source/common/runtime/runtime_features.cc | 2 ++ .../filters/http/ext_proc/mutation_utils.cc | 3 ++- .../http/ext_proc/mutation_utils_test.cc | 20 +++++++++++++++---- 4 files changed, 24 insertions(+), 5 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e97077ca0a277..e07ac03475e61 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -352,3 +352,7 @@ deprecated: change: | deprecated runtime key ``overload.global_downstream_max_connections`` in favor of :ref:`downstream connections monitor `. +- area: http ext_proc filter + change: | + Change to hornor HeaderValueOption.change_action. This change can be reverted temporarily by + setting the runtime guard ``envoy.reloadable_features.header_value_option_change_action`` to false. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index de2fc4c2a2986..977da91a59d6c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -127,6 +127,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_universal_header_validator) FALSE_RUNTIME_GUARD(envoy_reloadable_features_no_downgrade_to_canonical_name); // TODO(pksohn): enable after fixing https://github.com/envoyproxy/envoy/issues/29930 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_defer_logging_to_ack_listener); +// TODO(ronnie-personal): enable by default after a complete deprecation period. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_header_value_option_change_action) // 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/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index e9a2fbc327983..6772b30f94ee7 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -187,7 +187,8 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, ENVOY_LOG(error, "header value: {}, header raw value: {}", sh.header().value(), sh.header().raw_value()); - if (!sh.has_append()) { + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_value_option_change_action")) { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: ENVOY_LOG(error, "Inside append action APPEND_IF_EXISTS_OR_ADD {} ", 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 c1f55397bd5c2..1b18348113433 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -421,7 +421,10 @@ TEST(MutationUtils, TestDisallowHeaderSetNotAllowHeader) { TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "true"}, + }); Http::TestResponseHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, }; @@ -449,7 +452,10 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { TEST(MutationUtils, TestAppendActionAddIfAbsent) { TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "true"}, + }); Http::TestRequestHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, }; @@ -476,7 +482,10 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { TEST(MutationUtils, TestAppendActionOverwriteIfExists) { TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "true"}, + }); Http::TestRequestHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, }; @@ -503,7 +512,10 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "true"}, + }); Http::TestRequestHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, }; From f8ce8571dc771d5cffd781a63655ff6b40de0621 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sun, 8 Oct 2023 03:44:01 +0000 Subject: [PATCH 17/36] fix format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- changelogs/current.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e07ac03475e61..df5bfdffee33a 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -355,4 +355,4 @@ deprecated: - area: http ext_proc filter change: | Change to hornor HeaderValueOption.change_action. This change can be reverted temporarily by - setting the runtime guard ``envoy.reloadable_features.header_value_option_change_action`` to false. + setting the runtime guard ``envoy.reloadable_features.header_value_option_change_action`` to false. From a54590f4a643f324214b5c046f518c74ca9d68e4 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sun, 8 Oct 2023 20:11:25 +0000 Subject: [PATCH 18/36] update test cases Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 62 +++++------ .../http/ext_proc/mutation_utils_test.cc | 103 ++++++++++++------ 2 files changed, 95 insertions(+), 70 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 6772b30f94ee7..c283e15de848e 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -152,6 +152,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, bool append_mode_for_append_action; Filters::Common::MutationRules::CheckOperation check_op_for_append_action; Filters::Common::MutationRules::CheckResult checkResult_for_append_action; + auto is_duplicate = false; for (const auto& sh : mutation.set_headers()) { if (!sh.has_header()) { @@ -193,15 +194,32 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: ENVOY_LOG(error, "Inside append action APPEND_IF_EXISTS_OR_ADD {} ", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); - append_mode_for_append_action = true; - check_op_for_append_action = - (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; - checkResult_for_append_action = handleCheckResult( - headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, - header_name, header_value, append_mode_for_append_action); - if (checkResult_for_append_action == CheckResult::FAIL) { - return absl::InvalidArgumentError(absl::StrCat( - "Invalid attempt to modify ", static_cast(header_name))); + // Check if the header already exists with the same name and value. + if (!headers.get(header_name).empty()) { + is_duplicate = false; + Http::HeaderMap::GetResult result = headers.get(header_name); + for (size_t i = 0; i < result.size(); ++i) { + const Http::HeaderEntry* entry = result[i]; + const absl::string_view& existing_value = entry->value().getStringView(); + + // Compare the existing value with your desired header_value + if (existing_value == header_value) { + is_duplicate = true; + break; + } + } + } + if (!is_duplicate) { + append_mode_for_append_action = true; + check_op_for_append_action = + (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; + checkResult_for_append_action = handleCheckResult( + headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, + header_name, header_value, append_mode_for_append_action); + if (checkResult_for_append_action == CheckResult::FAIL) { + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); + } } break; case HeaderValueOption::ADD_IF_ABSENT: @@ -262,32 +280,6 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); } - /* - 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; - } - switch (check_result) { - case CheckResult::OK: - ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append); - if (append) { - headers.addCopy(header_name, header_value); - } else { - headers.setCopy(header_name, header_value); - } - break; - case CheckResult::IGNORE: - ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name); - rejected_mutations.inc(); - break; - case CheckResult::FAIL: - ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); - rejected_mutations.inc(); - return absl::InvalidArgumentError(absl::StrCat( - "Invalid attempt to modify ", static_cast(header_name))); - } */ } } 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 1b18348113433..1040bf2b75e0c 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -63,7 +63,6 @@ TEST(MutationUtils, TestApplyMutations) { {"x-replace-this", "Yes"}, {"x-remove-this", "Yes"}, {"x-envoy-strange-thing", "No"}, - {"Set-Cookie", "xyz"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; @@ -133,8 +132,8 @@ TEST(MutationUtils, TestApplyMutations) { s->mutable_header()->set_value("100"); s = mutation.add_set_headers(); s->mutable_append()->set_value(true); - s->mutable_header()->set_key("Set-Cookie"); - s->mutable_header()->set_value("3"); + s->mutable_header()->set_key("x-append-this"); + s->mutable_header()->set_value("1"); // Use the default mutation rules Checker checker(HeaderMutationRules::default_instance()); @@ -158,8 +157,7 @@ TEST(MutationUtils, TestApplyMutations) { {"x-remove-and-append-this", "4"}, {"x-replace-this", "nope"}, {"x-envoy-strange-thing", "No"}, - {"Set-Cookie", "xyz"}, - {"Set-Cookie", "3"}, + {"x-append-this", "1"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); @@ -430,11 +428,21 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { }; envoy::service::ext_proc::v3::HeaderMutation mutation; - auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("Set-Cookie"); - set_header->mutable_header()->set_value("Value234"); - set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + auto* s = mutation.add_set_headers(); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("Value234"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("Value123"); + s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("other-header"); + s->mutable_header()->set_value("xyz"); + s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; @@ -445,6 +453,7 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { Http::TestResponseHeaderMapImpl expected_headers{ {"Set-Cookie", "Value123"}, {"Set-Cookie", "Value234"}, + {"other-header", "xyz"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); @@ -461,11 +470,21 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { }; envoy::service::ext_proc::v3::HeaderMutation mutation; - auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("Set-Cookie"); - set_header->mutable_header()->set_value("Value234"); - set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + auto* s = mutation.add_set_headers(); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("Value234"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("Value123"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("other-header"); + s->mutable_header()->set_value("Value123"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; @@ -473,14 +492,13 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { EXPECT_TRUE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); - Http::TestRequestHeaderMapImpl expected_headers{ - {"Set-cookie", "Value123"}, - }; + Http::TestRequestHeaderMapImpl expected_headers{{"Set-cookie", "Value123"}, + {"other-header", "Value123"}}; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } -TEST(MutationUtils, TestAppendActionOverwriteIfExists) { +TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.send_header_raw_value", "false"}, @@ -491,11 +509,22 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { }; envoy::service::ext_proc::v3::HeaderMutation mutation; - auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("Set-Cookie"); - set_header->mutable_header()->set_value("Value234"); - set_header->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + auto* s = mutation.add_set_headers(); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("Value234"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("Value123"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + + s = mutation.add_set_headers(); + s->mutable_header()->set_key("other-header"); + s->mutable_header()->set_value("new value"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; @@ -503,14 +532,13 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { EXPECT_TRUE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); - Http::TestRequestHeaderMapImpl expected_headers{ - {"set-cookie", "Value234"}, - }; + Http::TestRequestHeaderMapImpl expected_headers{{"Set-Cookie", "Value123"}, + {"other-header", "new value"}}; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } -TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { +TEST(MutationUtils, TestAppendActionOverwriteIfExists) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.send_header_raw_value", "false"}, @@ -521,12 +549,17 @@ TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { }; envoy::service::ext_proc::v3::HeaderMutation mutation; - auto* set_header = mutation.add_set_headers(); - set_header->mutable_header()->set_key("Set-Cookie"); - set_header->mutable_header()->set_value("Value234"); - set_header->set_append_action( - envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + auto* s = mutation.add_set_headers(); + s->mutable_header()->set_key("Set-Cookie"); + s->mutable_header()->set_value("Value234"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + + s = mutation.add_set_headers(); + s->mutable_header()->set_key("other-header"); + s->mutable_header()->set_value("new value"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; @@ -535,7 +568,7 @@ TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); Http::TestRequestHeaderMapImpl expected_headers{ - {"Set-Cookie", "Value234"}, + {"set-cookie", "Value234"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); From b8bc861693d6e56c6d6928a5d682af72215e68fa Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Mon, 9 Oct 2023 02:01:34 +0000 Subject: [PATCH 19/36] add tests to filter_test Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 2 +- .../filters/http/ext_proc/filter_test.cc | 84 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index c283e15de848e..1638f13b8e8cd 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -195,8 +195,8 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, ENVOY_LOG(error, "Inside append action APPEND_IF_EXISTS_OR_ADD {} ", HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); // Check if the header already exists with the same name and value. + is_duplicate = false; if (!headers.get(header_name).empty()) { - is_duplicate = false; Http::HeaderMap::GetResult result = headers.get(header_name); for (size_t i = 0; i < result.size(); ++i) { const Http::HeaderEntry* entry = result[i]; diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 77d0479c3dd82..d8068ef2c1985 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -3293,6 +3293,90 @@ TEST_F(HttpFilter2Test, LastEncodeDataCallExceedsStreamBufferLimitWouldJustRaise conn_manager_->onData(fake_input, false); } +TEST_F(HttpFilterTest, AppendActionTest) { + scoped_runtime_.mergeValues( + {{"envoy.reloadable_features.header_value_option_change_action", "ture"}}); + initialize(R"EOF( + grpc_service: + envoy_grpc: + cluster_name: "ext_proc_server" + )EOF"); + + // Initialize request headers. + request_headers_.addCopy(LowerCaseString("x-original-header"), "original_value"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); + + // Apply the "append_action" to add a value to an existing header. + processRequestHeaders( + false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) { + auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); + auto s = headers_mut->add_set_headers(); + s->mutable_header()->set_key("x-original-header"); + s->mutable_header()->set_value(" appended_value"); + s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + }); + + // Check that the header was appended correctly. + TestRequestHeaderMapImpl expected{{":path", "/"}, + {":method", "POST"}, + {":scheme", "http"}, + {"host", "host"}, + {"x-original-header", "original_value"}, + {"x-original-header", "appended_value"}}; + EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected)); + + // Continue processing data and trailers. + Buffer::OwnedImpl req_data("foo"); + EXPECT_EQ(FilterDataStatus::Continue, filter_->decodeData(req_data, true)); + EXPECT_EQ(FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_)); + + // Initialize response headers. + response_headers_.addCopy(LowerCaseString(":status"), "200"); + response_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); + response_headers_.addCopy(LowerCaseString("content-length"), "3"); + + EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false)); + + // Apply the "append_action" to response headers. + processResponseHeaders(false, [](const HttpHeaders& response_headers, ProcessingResponse&, + HeadersResponse& header_resp) { + EXPECT_FALSE(response_headers.end_of_stream()); + TestRequestHeaderMapImpl expected_response{ + {":status", "200"}, {"content-type", "text/plain"}, {"content-length", "3"}}; + EXPECT_THAT(response_headers.headers(), HeaderProtosEqual(expected_response)); + + auto* resp_headers_mut = header_resp.mutable_response()->mutable_header_mutation(); + auto* s = resp_headers_mut->add_set_headers(); + s->mutable_header()->set_key("x-original-response-header"); + s->mutable_header()->set_value("appended_response_value"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + }); + + // Check that the response header was appended correctly. + TestRequestHeaderMapImpl final_expected_response{ + {":status", "200"}, + {"content-type", "text/plain"}, + {"content-length", "3"}, + {"x-original-response-header", "appended_response_value"}}; + EXPECT_THAT(&response_headers_, HeaderMapEqualIgnoreOrder(&final_expected_response)); + + // Continue processing data, encode trailers, and destroy the filter. + Buffer::OwnedImpl resp_data("bar"); + EXPECT_EQ(FilterDataStatus::Continue, filter_->encodeData(resp_data, false)); + Buffer::OwnedImpl empty_data; + EXPECT_EQ(FilterDataStatus::Continue, filter_->encodeData(empty_data, true)); + EXPECT_EQ(FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers_)); + filter_->onDestroy(); + + EXPECT_EQ(1, config_->stats().streams_started_.value()); + EXPECT_EQ(2, config_->stats().stream_msgs_sent_.value()); + EXPECT_EQ(2, config_->stats().stream_msgs_received_.value()); + EXPECT_EQ(1, config_->stats().streams_closed_.value()); +} + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters From 98cd440c24d79d1b0255d73cf036d7fc31e0b4ab Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Mon, 9 Oct 2023 20:57:49 +0000 Subject: [PATCH 20/36] debug test cases Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- source/extensions/filters/http/ext_proc/mutation_utils.cc | 5 ++++- test/extensions/filters/http/ext_proc/filter_test.cc | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 1638f13b8e8cd..339ca82646b48 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -196,11 +196,13 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); // Check if the header already exists with the same name and value. is_duplicate = false; + ENVOY_LOG(error, "1value of is_duplciate {}", is_duplicate); if (!headers.get(header_name).empty()) { Http::HeaderMap::GetResult result = headers.get(header_name); for (size_t i = 0; i < result.size(); ++i) { const Http::HeaderEntry* entry = result[i]; const absl::string_view& existing_value = entry->value().getStringView(); + ENVOY_LOG(error, "existing header {}; value: {}", header_name, existing_value); // Compare the existing value with your desired header_value if (existing_value == header_value) { @@ -209,6 +211,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } } } + ENVOY_LOG(error, "2value of is_duplciate {}", is_duplicate); if (!is_duplicate) { append_mode_for_append_action = true; check_op_for_append_action = @@ -299,7 +302,7 @@ Filters::Common::MutationRules::CheckResult MutationUtils::handleCheckResult( // CONTINUE_AND_REPLACE option is selected, to stay compatible. check_result = CheckResult::OK; } - + ENVOY_LOG(error, "before switch check_result {}", check_result); switch (check_result) { case CheckResult::OK: ENVOY_LOG(error, "Setting header {} append = {}", header_name, append_mode); diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index d8068ef2c1985..5ef10d5d80616 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -3294,8 +3294,11 @@ TEST_F(HttpFilter2Test, LastEncodeDataCallExceedsStreamBufferLimitWouldJustRaise } TEST_F(HttpFilterTest, AppendActionTest) { - scoped_runtime_.mergeValues( - {{"envoy.reloadable_features.header_value_option_change_action", "ture"}}); + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "true"}, + }); initialize(R"EOF( grpc_service: envoy_grpc: From 77ac18f7074ca314df56e5d1526dae31a3ba0e83 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Tue, 10 Oct 2023 01:17:07 +0000 Subject: [PATCH 21/36] cleanup tests Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 24 +++---------------- .../filters/http/ext_proc/filter_test.cc | 24 +++++++++++++------ 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 339ca82646b48..c4c2b733885c6 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -183,26 +183,17 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } const LowerCaseString header_name(sh.header().key()); - ENVOY_LOG(error, "append_action value: {}", sh.append_action()); - ENVOY_LOG(error, "header name: {}", header_name); - ENVOY_LOG(error, "header value: {}, header raw value: {}", sh.header().value(), - sh.header().raw_value()); - if (Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.header_value_option_change_action")) { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: - ENVOY_LOG(error, "Inside append action APPEND_IF_EXISTS_OR_ADD {} ", - HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); // Check if the header already exists with the same name and value. is_duplicate = false; - ENVOY_LOG(error, "1value of is_duplciate {}", is_duplicate); if (!headers.get(header_name).empty()) { Http::HeaderMap::GetResult result = headers.get(header_name); + absl::string_view existing_value; for (size_t i = 0; i < result.size(); ++i) { - const Http::HeaderEntry* entry = result[i]; - const absl::string_view& existing_value = entry->value().getStringView(); - ENVOY_LOG(error, "existing header {}; value: {}", header_name, existing_value); + existing_value = result[i]->value().getStringView(); // Compare the existing value with your desired header_value if (existing_value == header_value) { @@ -211,7 +202,6 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } } } - ENVOY_LOG(error, "2value of is_duplciate {}", is_duplicate); if (!is_duplicate) { append_mode_for_append_action = true; check_op_for_append_action = @@ -226,7 +216,6 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } break; case HeaderValueOption::ADD_IF_ABSENT: - ENVOY_LOG(error, "Inside append action ADD_IF_ABSENT {}", HeaderValueOption::ADD_IF_ABSENT); if (headers.get(header_name).empty()) { append_mode_for_append_action = true; check_op_for_append_action = CheckOperation::SET; @@ -240,8 +229,6 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } break; case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: - ENVOY_LOG(error, "Inside append action OVERWRITE_IF_EXISTS_OR_ADD {}", - HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); append_mode_for_append_action = false; check_op_for_append_action = CheckOperation::SET; checkResult_for_append_action = handleCheckResult( @@ -253,8 +240,6 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } break; case HeaderValueOption::OVERWRITE_IF_EXISTS: - ENVOY_LOG(error, "Inside append action OVERWRITE_IF_EXISTS{}", - HeaderValueOption::OVERWRITE_IF_EXISTS); if (!headers.get(header_name).empty()) { append_mode_for_append_action = false; check_op_for_append_action = CheckOperation::SET; @@ -302,10 +287,9 @@ Filters::Common::MutationRules::CheckResult MutationUtils::handleCheckResult( // CONTINUE_AND_REPLACE option is selected, to stay compatible. check_result = CheckResult::OK; } - ENVOY_LOG(error, "before switch check_result {}", check_result); + // Print the value as an integer switch (check_result) { case CheckResult::OK: - ENVOY_LOG(error, "Setting header {} append = {}", header_name, append_mode); if (append_mode) { headers.addCopy(header_name, header_value); } else { @@ -313,11 +297,9 @@ Filters::Common::MutationRules::CheckResult MutationUtils::handleCheckResult( } break; case CheckResult::IGNORE: - ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name); rejected_mutations.inc(); break; case CheckResult::FAIL: - ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name); rejected_mutations.inc(); // You can add additional handling for the FAIL case here if needed. break; diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 5ef10d5d80616..67553a3c035fd 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -3316,9 +3316,14 @@ TEST_F(HttpFilterTest, AppendActionTest) { auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); auto s = headers_mut->add_set_headers(); s->mutable_header()->set_key("x-original-header"); - s->mutable_header()->set_value(" appended_value"); + s->mutable_header()->set_value("appended_value"); s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + s = headers_mut->add_set_headers(); + s->mutable_header()->set_key("another-request"); + s->mutable_header()->set_value("request"); + s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); }); // Check that the header was appended correctly. @@ -3327,7 +3332,8 @@ TEST_F(HttpFilterTest, AppendActionTest) { {":scheme", "http"}, {"host", "host"}, {"x-original-header", "original_value"}, - {"x-original-header", "appended_value"}}; + {"x-original-header", "appended_value"}, + {"another-request", "request"}}; EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected)); // Continue processing data and trailers. @@ -3356,14 +3362,18 @@ TEST_F(HttpFilterTest, AppendActionTest) { s->mutable_header()->set_value("appended_response_value"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + s = resp_headers_mut->add_set_headers(); + s->mutable_header()->set_key("x-original-response-header"); + s->mutable_header()->set_value("second value"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); }); // Check that the response header was appended correctly. - TestRequestHeaderMapImpl final_expected_response{ - {":status", "200"}, - {"content-type", "text/plain"}, - {"content-length", "3"}, - {"x-original-response-header", "appended_response_value"}}; + TestRequestHeaderMapImpl final_expected_response{{":status", "200"}, + {"content-type", "text/plain"}, + {"content-length", "3"}, + {"x-original-response-header", "second value"}}; EXPECT_THAT(&response_headers_, HeaderMapEqualIgnoreOrder(&final_expected_response)); // Continue processing data, encode trailers, and destroy the filter. From c19cc3dd436d4f653067df18b54ac145fabb4018 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Tue, 10 Oct 2023 02:48:33 +0000 Subject: [PATCH 22/36] cleanup tests Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../http/ext_proc/mutation_utils_test.cc | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) 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 1040bf2b75e0c..f0aab9bdd71b3 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -443,9 +443,18 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { s->mutable_header()->set_value("xyz"); s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + // Attempts to set the status header out of range should + // also be ignored. + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("This is not even an integer"); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("100"); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; + EXPECT_CALL(rejections, inc()).Times(2); EXPECT_TRUE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); @@ -483,11 +492,24 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { s = mutation.add_set_headers(); s->mutable_header()->set_key("other-header"); s->mutable_header()->set_value("Value123"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + // Attempts to set the status header out of range should + // also be ignored. + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("This is not even an integer"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("100"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; + EXPECT_CALL(rejections, inc()).Times(2); EXPECT_TRUE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); @@ -523,11 +545,24 @@ TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { s = mutation.add_set_headers(); s->mutable_header()->set_key("other-header"); s->mutable_header()->set_value("new value"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + // Attempts to set the status header out of range should + // also be ignored. + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("This is not even an integer"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("100"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; + EXPECT_CALL(rejections, inc()).Times(2); EXPECT_TRUE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); @@ -558,11 +593,22 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { s = mutation.add_set_headers(); s->mutable_header()->set_key("other-header"); s->mutable_header()->set_value("new value"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("This is not even an integer"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + s = mutation.add_set_headers(); + s->mutable_header()->set_key(":status"); + s->mutable_header()->set_value("100"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; + EXPECT_CALL(rejections, inc()).Times(2); EXPECT_TRUE( MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok()); From aa1cb9b8b9ef8096b3b317da0120d3329fdd3e36 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Tue, 10 Oct 2023 03:48:13 +0000 Subject: [PATCH 23/36] fix test Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- test/extensions/filters/http/ext_proc/mutation_utils_test.cc | 2 ++ 1 file changed, 2 insertions(+) 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 f0aab9bdd71b3..fb34dbbc2c85e 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -581,6 +581,7 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { }); Http::TestRequestHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, + {":status", "500"}, }; envoy::service::ext_proc::v3::HeaderMutation mutation; @@ -615,6 +616,7 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { Http::TestRequestHeaderMapImpl expected_headers{ {"set-cookie", "Value234"}, + {":status", "500"}, }; EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); From aca53c27cd8cc79d55f2bb9c78cf846c3d87624b Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Wed, 11 Oct 2023 00:59:22 +0000 Subject: [PATCH 24/36] add test for more code coverage Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../http/ext_proc/mutation_utils_test.cc | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) 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 fb34dbbc2c85e..2e1bb7212e36b 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -606,7 +606,6 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { s->mutable_header()->set_value("100"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); - Checker checker(HeaderMutationRules::default_instance()); Envoy::Stats::MockCounter rejections; EXPECT_CALL(rejections, inc()).Times(2); @@ -622,6 +621,55 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } +TEST(MutationUtils, TestApplyMutationsWithCheckFailure) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "true"}, + }); + + Http::TestRequestHeaderMapImpl headers{ + {"orignial-header", "any value"}, + }; + + envoy::service::ext_proc::v3::HeaderMutation mutation; + auto* s = mutation.add_set_headers(); + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action( + ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action( + ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action( + ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + s = mutation.add_set_headers(); + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action( + ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + + HeaderMutationRules rules; + rules.mutable_disallow_all()->set_value(true); + rules.mutable_disallow_is_error()->set_value(true); + Checker checker(rules); + + Envoy::Stats::MockCounter rejections; + EXPECT_CALL(rejections, inc()).Times(4); // Expect 1 rejection + + const auto result = + MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections); + + // Assert that the result is an InvalidArgumentError with the specified error message + EXPECT_EQ(result.code(), absl::StatusCode::kInvalidArgument); + EXPECT_EQ(result.message(), "Invalid attempt to modify x-check-this-header"); +} // namespace + } // namespace } // namespace ExternalProcessing } // namespace HttpFilters From 962dd86126b19b4da6df331c446e5f8b14e7749a Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Wed, 11 Oct 2023 01:27:02 +0000 Subject: [PATCH 25/36] fix format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../http/ext_proc/mutation_utils_test.cc | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) 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 2e1bb7212e36b..4439310f78224 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -636,23 +636,25 @@ TEST(MutationUtils, TestApplyMutationsWithCheckFailure) { auto* s = mutation.add_set_headers(); s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); - s->set_append_action( - ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); s = mutation.add_set_headers(); s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); - s->set_append_action( - ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + s = mutation.add_set_headers(); s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); - s->set_append_action( - ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + s = mutation.add_set_headers(); s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); - s->set_append_action( - ::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); HeaderMutationRules rules; rules.mutable_disallow_all()->set_value(true); From 43b39820a427e201eb500ef08a21effaa9fc78d1 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Wed, 11 Oct 2023 21:45:44 +0000 Subject: [PATCH 26/36] fix test Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../http/ext_proc/mutation_utils_test.cc | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) 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 4439310f78224..91631ee3d3dee 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -621,7 +621,12 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } -TEST(MutationUtils, TestApplyMutationsWithCheckFailure) { +class CheckFailureTest : public testing::TestWithParam { +}; + +INSTANTIATE_TEST_SUITE_P(Params, CheckFailureTest, testing::Values(4,3,2,1,0)); + +TEST_P(CheckFailureTest, TestApplyMutationsWithCheckFailure) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.send_header_raw_value", "false"}, @@ -634,35 +639,48 @@ TEST(MutationUtils, TestApplyMutationsWithCheckFailure) { envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); + switch (GetParam()) { + case 0: s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); - s = mutation.add_set_headers(); + break; + case 1: s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); - - s = mutation.add_set_headers(); + break; + case 2: s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); - - s = mutation.add_set_headers(); + break; + case 3: + headers.addCopy("x-check-this-header", "to be updated"); s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); - + break; + case 4: + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "false"} }); + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->mutable_append()->set_value(true); + } + HeaderMutationRules rules; rules.mutable_disallow_all()->set_value(true); rules.mutable_disallow_is_error()->set_value(true); Checker checker(rules); Envoy::Stats::MockCounter rejections; - EXPECT_CALL(rejections, inc()).Times(4); // Expect 1 rejection + EXPECT_CALL(rejections, inc()).Times(1); // Expect 1 rejection const auto result = MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections); From 23e2fb85853b51a1a978ccffbab8884a17f55efc Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Thu, 12 Oct 2023 00:45:57 +0000 Subject: [PATCH 27/36] fix format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../http/ext_proc/mutation_utils_test.cc | 63 +++++++++---------- 1 file changed, 31 insertions(+), 32 deletions(-) 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 91631ee3d3dee..7bdb274d84342 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -621,10 +621,9 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { EXPECT_THAT(&headers, HeaderMapEqualIgnoreOrder(&expected_headers)); } -class CheckFailureTest : public testing::TestWithParam { -}; +class CheckFailureTest : public testing::TestWithParam {}; -INSTANTIATE_TEST_SUITE_P(Params, CheckFailureTest, testing::Values(4,3,2,1,0)); +INSTANTIATE_TEST_SUITE_P(Params, CheckFailureTest, testing::Values(4, 3, 2, 1, 0)); TEST_P(CheckFailureTest, TestApplyMutationsWithCheckFailure) { TestScopedRuntime scoped_runtime; @@ -641,46 +640,46 @@ TEST_P(CheckFailureTest, TestApplyMutationsWithCheckFailure) { auto* s = mutation.add_set_headers(); switch (GetParam()) { case 0: - s->mutable_header()->set_key("x-check-this-header"); - s->mutable_header()->set_value("value-to-check"); - s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); - break; + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD); + break; case 1: - s->mutable_header()->set_key("x-check-this-header"); - s->mutable_header()->set_value("value-to-check"); - s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); - break; + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT); + break; case 2: - s->mutable_header()->set_key("x-check-this-header"); - s->mutable_header()->set_value("value-to-check"); - s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); - break; + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD); + break; case 3: - headers.addCopy("x-check-this-header", "to be updated"); - s->mutable_header()->set_key("x-check-this-header"); - s->mutable_header()->set_value("value-to-check"); - s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: - HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); - break; + headers.addCopy("x-check-this-header", "to be updated"); + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction:: + HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); + break; case 4: - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.send_header_raw_value", "false"}, - {"envoy.reloadable_features.header_value_option_change_action", "false"} }); - s->mutable_header()->set_key("x-check-this-header"); - s->mutable_header()->set_value("value-to-check"); - s->mutable_append()->set_value(true); + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.send_header_raw_value", "false"}, + {"envoy.reloadable_features.header_value_option_change_action", "false"}}); + s->mutable_header()->set_key("x-check-this-header"); + s->mutable_header()->set_value("value-to-check"); + s->mutable_append()->set_value(true); } - + HeaderMutationRules rules; rules.mutable_disallow_all()->set_value(true); rules.mutable_disallow_is_error()->set_value(true); Checker checker(rules); Envoy::Stats::MockCounter rejections; - EXPECT_CALL(rejections, inc()).Times(1); // Expect 1 rejection + EXPECT_CALL(rejections, inc()); // Expect 1 rejection const auto result = MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections); From 291817c33ae2ffafb4c4ee886cc9d8d3be4cc370 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Thu, 12 Oct 2023 19:06:03 +0000 Subject: [PATCH 28/36] check append_action has a value Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- source/extensions/filters/http/ext_proc/mutation_utils.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index c4c2b733885c6..ad7f643e26978 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -183,8 +183,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } const LowerCaseString header_name(sh.header().key()); - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.header_value_option_change_action")) { + if (sh.append_action().has_value()) { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: // Check if the header already exists with the same name and value. From ad19ed92c387d3bdbf212ee09557f730c551b2d5 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Fri, 13 Oct 2023 18:28:01 +0000 Subject: [PATCH 29/36] address review comment Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- api/envoy/config/core/v3/base.proto | 10 ++++++++-- changelogs/current.yaml | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index f84e168b39b27..09d7ddfe830a0 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -347,8 +347,14 @@ message HeaderValueOption { // Describes the supported actions types for header append action. enum HeaderAppendAction { // This action will append the specified value to the existing values if the header - // already exists. If the header doesn't exist then this will add the header with - // specified key and value. + // already exists and the header is inline header. + // set-cookie is special predefined inline header that allows to be multi headers present + // rather than concatenation. + // Reject append request on single-value header like host, content-length. + // You can find more detailed information at RFC 7230 section 3.2.2. + // + // If the header doesn't exist then this will add the header with + // specified key and value. APPEND_IF_EXISTS_OR_ADD = 0; // This action will add the header if it doesn't already exist. If the header diff --git a/changelogs/current.yaml b/changelogs/current.yaml index df5bfdffee33a..fe90880762bd4 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -354,5 +354,5 @@ deprecated: `. - area: http ext_proc filter change: | - Change to hornor HeaderValueOption.change_action. This change can be reverted temporarily by + Change to hornor append_action . This change can be reverted temporarily by setting the runtime guard ``envoy.reloadable_features.header_value_option_change_action`` to false. From fec6ae3170a5c0712ad5612f0e967f878891ce97 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Fri, 13 Oct 2023 18:57:01 +0000 Subject: [PATCH 30/36] fix yaml format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- changelogs/current.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index fe90880762bd4..e983af901f191 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -354,5 +354,6 @@ deprecated: `. - area: http ext_proc filter change: | - Change to hornor append_action . This change can be reverted temporarily by - setting the runtime guard ``envoy.reloadable_features.header_value_option_change_action`` to false. + Change to hornor append_action . + This change can be reverted temporarily by setting the runtime guard + ``envoy.reloadable_features.header_value_option_change_action`` to false. \ No newline at end of file From 65d38d460701c10418bcc064deda3efbed115fb9 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Fri, 13 Oct 2023 18:58:33 +0000 Subject: [PATCH 31/36] fix yaml format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- changelogs/current.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e983af901f191..aa3db3614eabc 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -356,4 +356,4 @@ deprecated: change: | Change to hornor append_action . This change can be reverted temporarily by setting the runtime guard - ``envoy.reloadable_features.header_value_option_change_action`` to false. \ No newline at end of file + ``envoy.reloadable_features.header_value_option_change_action`` to false. From e994879f7766c8ec5d85179b3124ae8812a05d56 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Fri, 13 Oct 2023 19:08:05 +0000 Subject: [PATCH 32/36] fix yaml format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- api/envoy/config/core/v3/base.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 09d7ddfe830a0..1eb2161d914c8 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -354,7 +354,7 @@ message HeaderValueOption { // You can find more detailed information at RFC 7230 section 3.2.2. // // If the header doesn't exist then this will add the header with - // specified key and value. + // specified key and value. APPEND_IF_EXISTS_OR_ADD = 0; // This action will add the header if it doesn't already exist. If the header From 121a42b83b8491e2a855a86f70a707b751b43cac Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Wed, 18 Oct 2023 02:08:09 +0000 Subject: [PATCH 33/36] fix format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- api/envoy/config/core/v3/base.proto | 8 +---- changelogs/current.yaml | 5 --- source/common/runtime/runtime_features.cc | 2 -- .../filters/http/ext_proc/mutation_utils.cc | 36 +++++-------------- .../http/ext_proc/mutation_utils_test.cc | 19 +++------- 5 files changed, 15 insertions(+), 55 deletions(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 1eb2161d914c8..f84e168b39b27 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -347,13 +347,7 @@ message HeaderValueOption { // Describes the supported actions types for header append action. enum HeaderAppendAction { // This action will append the specified value to the existing values if the header - // already exists and the header is inline header. - // set-cookie is special predefined inline header that allows to be multi headers present - // rather than concatenation. - // Reject append request on single-value header like host, content-length. - // You can find more detailed information at RFC 7230 section 3.2.2. - // - // If the header doesn't exist then this will add the header with + // already exists. If the header doesn't exist then this will add the header with // specified key and value. APPEND_IF_EXISTS_OR_ADD = 0; diff --git a/changelogs/current.yaml b/changelogs/current.yaml index b61f46fec369f..9f9ca7db88569 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -425,8 +425,3 @@ deprecated: change: | deprecated runtime key ``overload.global_downstream_max_connections`` in favor of :ref:`downstream connections monitor `. -- area: http ext_proc filter - change: | - Change to hornor append_action . - This change can be reverted temporarily by setting the runtime guard - ``envoy.reloadable_features.header_value_option_change_action`` to false. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 92e6fc9bbc35c..65e733051d22c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -129,8 +129,6 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_universal_header_validator) FALSE_RUNTIME_GUARD(envoy_reloadable_features_no_downgrade_to_canonical_name); // TODO(pksohn): enable after fixing https://github.com/envoyproxy/envoy/issues/29930 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_defer_logging_to_ack_listener); -// TODO(ronnie-personal): enable by default after a complete deprecation period. -FALSE_RUNTIME_GUARD(envoy_reloadable_features_header_value_option_change_action) // 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/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index ad7f643e26978..2a9ffde01a83a 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -152,7 +152,6 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, bool append_mode_for_append_action; Filters::Common::MutationRules::CheckOperation check_op_for_append_action; Filters::Common::MutationRules::CheckResult checkResult_for_append_action; - auto is_duplicate = false; for (const auto& sh : mutation.set_headers()) { if (!sh.has_header()) { @@ -186,32 +185,15 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, if (sh.append_action().has_value()) { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: - // Check if the header already exists with the same name and value. - is_duplicate = false; - if (!headers.get(header_name).empty()) { - Http::HeaderMap::GetResult result = headers.get(header_name); - absl::string_view existing_value; - for (size_t i = 0; i < result.size(); ++i) { - existing_value = result[i]->value().getStringView(); - - // Compare the existing value with your desired header_value - if (existing_value == header_value) { - is_duplicate = true; - break; - } - } - } - if (!is_duplicate) { - append_mode_for_append_action = true; - check_op_for_append_action = - (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; - checkResult_for_append_action = handleCheckResult( - headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, - header_name, header_value, append_mode_for_append_action); - if (checkResult_for_append_action == CheckResult::FAIL) { - return absl::InvalidArgumentError(absl::StrCat( - "Invalid attempt to modify ", static_cast(header_name))); - } + append_mode_for_append_action = true; + check_op_for_append_action = + (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; + checkResult_for_append_action = handleCheckResult( + headers, replacing_message, checker, rejected_mutations, check_op_for_append_action, + header_name, header_value, append_mode_for_append_action); + if (checkResult_for_append_action == CheckResult::FAIL) { + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); } break; case HeaderValueOption::ADD_IF_ABSENT: 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 7bdb274d84342..f47a697c4827b 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -421,7 +421,6 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.send_header_raw_value", "false"}, - {"envoy.reloadable_features.header_value_option_change_action", "true"}, }); Http::TestResponseHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, @@ -470,10 +469,7 @@ TEST(MutationUtils, TestAppendActionAppendIfExistsOrAdd) { TEST(MutationUtils, TestAppendActionAddIfAbsent) { TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.send_header_raw_value", "false"}, - {"envoy.reloadable_features.header_value_option_change_action", "true"}, - }); + scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, }; @@ -522,10 +518,7 @@ TEST(MutationUtils, TestAppendActionAddIfAbsent) { TEST(MutationUtils, TestAppendActionOverwriteOrAdd) { TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues({ - {"envoy.reloadable_features.send_header_raw_value", "false"}, - {"envoy.reloadable_features.header_value_option_change_action", "true"}, - }); + scoped_runtime.mergeValues({{"envoy.reloadable_features.send_header_raw_value", "false"}}); Http::TestRequestHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, }; @@ -577,7 +570,6 @@ TEST(MutationUtils, TestAppendActionOverwriteIfExists) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.send_header_raw_value", "false"}, - {"envoy.reloadable_features.header_value_option_change_action", "true"}, }); Http::TestRequestHeaderMapImpl headers{ {"Set-Cookie", "Value123"}, @@ -629,7 +621,6 @@ TEST_P(CheckFailureTest, TestApplyMutationsWithCheckFailure) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues({ {"envoy.reloadable_features.send_header_raw_value", "false"}, - {"envoy.reloadable_features.header_value_option_change_action", "true"}, }); Http::TestRequestHeaderMapImpl headers{ @@ -665,9 +656,9 @@ TEST_P(CheckFailureTest, TestApplyMutationsWithCheckFailure) { HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS); break; case 4: - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.send_header_raw_value", "false"}, - {"envoy.reloadable_features.header_value_option_change_action", "false"}}); + scoped_runtime.mergeValues({ + {"envoy.reloadable_features.send_header_raw_value", "false"}, + }); s->mutable_header()->set_key("x-check-this-header"); s->mutable_header()->set_value("value-to-check"); s->mutable_append()->set_value(true); From 7ae3f78f99c98c9a58c33cb3c35940b8957e66c6 Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 21 Oct 2023 03:10:10 +0000 Subject: [PATCH 34/36] modify code to check if append has a value Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 2a9ffde01a83a..c592c77fb2ab5 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -182,7 +182,19 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } const LowerCaseString header_name(sh.header().key()); - if (sh.append_action().has_value()) { + bool append = false; + if (sh.has_append()) { + append = sh.append().value; + const auto check_op = (append && !headers.get(header_name).empty()) + ? CheckOperation::APPEND + : CheckOperation::SET; + auto checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, + check_op, header_name, header_value, append); + if (checkResult == CheckResult::FAIL) { + return absl::InvalidArgumentError(absl::StrCat( + "Invalid attempt to modify ", static_cast(header_name))); + } + } else { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: append_mode_for_append_action = true; @@ -238,18 +250,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, return absl::InvalidArgumentError(absl::StrCat( "Invalid append_action value ", static_cast(header_name))); } - } else { - const bool append_mode = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false); - const auto check_op = (append_mode && !headers.get(header_name).empty()) - ? CheckOperation::APPEND - : CheckOperation::SET; - auto checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, - check_op, header_name, header_value, append_mode); - if (checkResult == CheckResult::FAIL) { - return absl::InvalidArgumentError(absl::StrCat( - "Invalid attempt to modify ", static_cast(header_name))); - } - } + } } // After header mutation, check the ending headers are not exceeding the HCM limit. From 3cc6e3f315d61caa9552f756b248d2f11b8df33d Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 21 Oct 2023 03:35:15 +0000 Subject: [PATCH 35/36] fix format Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- .../filters/http/ext_proc/mutation_utils.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index c592c77fb2ab5..2dff50ad1943c 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -182,19 +182,18 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, } const LowerCaseString header_name(sh.header().key()); - bool append = false; + bool append = false; if (sh.has_append()) { append = sh.append().value; - const auto check_op = (append && !headers.get(header_name).empty()) - ? CheckOperation::APPEND - : CheckOperation::SET; + const auto check_op = (append && !headers.get(header_name).empty()) ? CheckOperation::APPEND + : CheckOperation::SET; auto checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations, check_op, header_name, header_value, append); if (checkResult == CheckResult::FAIL) { return absl::InvalidArgumentError(absl::StrCat( "Invalid attempt to modify ", static_cast(header_name))); } - } else { + } else { switch (sh.append_action()) { case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: append_mode_for_append_action = true; @@ -250,7 +249,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, return absl::InvalidArgumentError(absl::StrCat( "Invalid append_action value ", static_cast(header_name))); } - } + } } // After header mutation, check the ending headers are not exceeding the HCM limit. From f7c4aac6269fbc1f3d725920610bdebd76276d3e Mon Sep 17 00:00:00 2001 From: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> Date: Sat, 21 Oct 2023 20:58:08 +0000 Subject: [PATCH 36/36] fix syntax error Signed-off-by: Ronnie-personal <76408835+Ronnie-personal@users.noreply.github.com> --- source/extensions/filters/http/ext_proc/mutation_utils.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/http/ext_proc/mutation_utils.cc b/source/extensions/filters/http/ext_proc/mutation_utils.cc index 2dff50ad1943c..b0a9787df8b76 100644 --- a/source/extensions/filters/http/ext_proc/mutation_utils.cc +++ b/source/extensions/filters/http/ext_proc/mutation_utils.cc @@ -184,7 +184,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation, bool append = false; if (sh.has_append()) { - append = sh.append().value; + append = sh.append().value(); const auto check_op = (append && !headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET; auto checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations,