From 1192041d7bd9fd87ef818b2f9bc7902a524825de Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 16 Aug 2022 13:52:05 +0000 Subject: [PATCH 01/19] implementation add headers add if absent Signed-off-by: wbpcode --- api/envoy/config/core/v3/base.proto | 6 +- envoy/http/header_map.h | 5 +- source/common/router/header_formatter.cc | 3 +- source/common/router/header_formatter.h | 22 +-- source/common/router/header_parser.cc | 99 +++++++--- source/common/router/header_parser.h | 17 +- test/common/router/config_impl_test.cc | 158 +++++++++------ test/common/router/header_formatter_test.cc | 204 ++++++++++++++------ 8 files changed, 342 insertions(+), 172 deletions(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index c9f0a76a5ec0..30fe66d9a839 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -358,8 +358,10 @@ message HeaderValueOption { // existing values. Otherwise it replaces any existing values. google.protobuf.BoolValue append = 2; - // [#not-implemented-hide:] Describes the action taken to append/overwrite the given value for an existing header - // or to only add this header if it's absent. Value defaults to :ref:`APPEND_IF_EXISTS_OR_ADD`. + // Describes the action taken to append/overwrite the given value for an existing header + // or to only add this header if it's absent. + // Value defaults to :ref:`APPEND_IF_EXISTS_OR_ADD + // `. HeaderAppendAction append_action = 3 [(validate.rules).enum = {defined_only: true}]; // Is the header value allowed to be empty? If false (default), custom headers with empty values are dropped, diff --git a/envoy/http/header_map.h b/envoy/http/header_map.h index bc3906be2b27..11739fbbb34f 100644 --- a/envoy/http/header_map.h +++ b/envoy/http/header_map.h @@ -690,8 +690,9 @@ using HeaderMapPtr = std::unique_ptr; * Wraps a set of header modifications. */ struct HeaderTransforms { - std::vector> headers_to_append; - std::vector> headers_to_overwrite; + std::vector> headers_to_append_or_add; + std::vector> headers_to_overwrite_or_add; + std::vector> headers_to_add_if_absent; std::vector headers_to_remove; }; diff --git a/source/common/router/header_formatter.cc b/source/common/router/header_formatter.cc index 2062cdc7956e..67e1bc215aa4 100644 --- a/source/common/router/header_formatter.cc +++ b/source/common/router/header_formatter.cc @@ -236,8 +236,7 @@ StreamInfoHeaderFormatter::FieldExtractor sslConnectionInfoStringHeaderExtractor } // namespace -StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_name, bool append) - : append_(append) { +StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_name) { if (field_name == "PROTOCOL") { field_extractor_ = [](const Envoy::StreamInfo::StreamInfo& stream_info) { return Envoy::Formatter::SubstitutionFormatUtils::protocolToStringOrDefault( diff --git a/source/common/router/header_formatter.h b/source/common/router/header_formatter.h index 0aace90b3653..aaa26358c4a2 100644 --- a/source/common/router/header_formatter.h +++ b/source/common/router/header_formatter.h @@ -20,12 +20,6 @@ class HeaderFormatter { virtual ~HeaderFormatter() = default; virtual const std::string format(const Envoy::StreamInfo::StreamInfo& stream_info) const PURE; - - /** - * @return bool indicating whether the formatted header should be appended to the existing - * headers or replace any existing values for the header - */ - virtual bool append() const PURE; }; using HeaderFormatterPtr = std::unique_ptr; @@ -35,18 +29,16 @@ using HeaderFormatterPtr = std::unique_ptr; */ class StreamInfoHeaderFormatter : public HeaderFormatter { public: - StreamInfoHeaderFormatter(absl::string_view field_name, bool append); + StreamInfoHeaderFormatter(absl::string_view field_name); // HeaderFormatter::format const std::string format(const Envoy::StreamInfo::StreamInfo& stream_info) const override; - bool append() const override { return append_; } using FieldExtractor = std::function; using FormatterPtrMap = absl::node_hash_map; private: FieldExtractor field_extractor_; - const bool append_; // Maps a string format pattern (including field name and any command operators between // parenthesis) to the list of FormatterProviderPtrs that are capable of formatting that pattern. @@ -61,18 +53,16 @@ class StreamInfoHeaderFormatter : public HeaderFormatter { */ class PlainHeaderFormatter : public HeaderFormatter { public: - PlainHeaderFormatter(const std::string& static_header_value, bool append) - : static_value_(static_header_value), append_(append) {} + PlainHeaderFormatter(const std::string& static_header_value) + : static_value_(static_header_value) {} // HeaderFormatter::format const std::string format(const Envoy::StreamInfo::StreamInfo&) const override { return static_value_; }; - bool append() const override { return append_; } private: const std::string static_value_; - const bool append_; }; /** @@ -80,8 +70,8 @@ class PlainHeaderFormatter : public HeaderFormatter { */ class CompoundHeaderFormatter : public HeaderFormatter { public: - CompoundHeaderFormatter(std::vector&& formatters, bool append) - : formatters_(std::move(formatters)), append_(append) {} + CompoundHeaderFormatter(std::vector&& formatters) + : formatters_(std::move(formatters)) {} // HeaderFormatter::format const std::string format(const Envoy::StreamInfo::StreamInfo& stream_info) const override { @@ -91,11 +81,9 @@ class CompoundHeaderFormatter : public HeaderFormatter { } return buf; }; - bool append() const override { return append_; } private: const std::vector formatters_; - const bool append_; }; } // namespace Router diff --git a/source/common/router/header_parser.cc b/source/common/router/header_parser.cc index dc38ff993c1c..0435e6896e6b 100644 --- a/source/common/router/header_parser.cc +++ b/source/common/router/header_parser.cc @@ -37,8 +37,7 @@ std::string unescape(absl::string_view sv) { return absl::StrReplaceAll(sv, {{"% // The statement machine does minimal validation of the arguments (if any) and does not know the // names of valid variables. Interpretation of the variable name and arguments is delegated to // StreamInfoHeaderFormatter. -HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& header_value, - bool append) { +HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& header_value) { const std::string& key = header_value.key(); // PGV constraints provide this guarantee. ASSERT(!key.empty()); @@ -55,7 +54,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea absl::string_view format(header_value.value()); if (format.empty()) { - return std::make_unique("", append); + return std::make_unique(""); } std::vector formatters; @@ -89,7 +88,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea state = ParserState::VariableName; if (pos > start) { absl::string_view literal = format.substr(start, pos - start); - formatters.emplace_back(new PlainHeaderFormatter(unescape(literal), append)); + formatters.emplace_back(new PlainHeaderFormatter(unescape(literal))); } start = pos + 1; break; @@ -98,8 +97,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea // Consume "VAR" from "%VAR%" or "%VAR(...)%" if (ch == '%') { // Found complete variable name, add formatter. - formatters.emplace_back( - new StreamInfoHeaderFormatter(format.substr(start, pos - start), append)); + formatters.emplace_back(new StreamInfoHeaderFormatter(format.substr(start, pos - start))); start = pos + 1; state = ParserState::Literal; break; @@ -179,8 +177,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea case ParserState::ExpectVariableEnd: // Search for closing % of a %VAR(...)% expression if (ch == '%') { - formatters.emplace_back( - new StreamInfoHeaderFormatter(format.substr(start, pos - start), append)); + formatters.emplace_back(new StreamInfoHeaderFormatter(format.substr(start, pos - start))); start = pos + 1; state = ParserState::Literal; break; @@ -205,7 +202,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea if (pos > start) { // Trailing constant data. absl::string_view literal = format.substr(start, pos - start); - formatters.emplace_back(new PlainHeaderFormatter(unescape(literal), append)); + formatters.emplace_back(new PlainHeaderFormatter(unescape(literal))); } ASSERT(!formatters.empty()); @@ -214,22 +211,36 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea return std::move(formatters[0]); } - return std::make_unique(std::move(formatters), append); + return std::make_unique(std::move(formatters)); } } // namespace -HeaderParserPtr HeaderParser::configure( - const Protobuf::RepeatedPtrField& headers_to_add) { +HeaderParserPtr +HeaderParser::configure(const Protobuf::RepeatedPtrField& headers_to_add) { HeaderParserPtr header_parser(new HeaderParser()); for (const auto& header_value_option : headers_to_add) { - const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(header_value_option, append, true); - HeaderFormatterPtr header_formatter = parseInternal(header_value_option.header(), append); + HeaderAppendAction append_action; + + if (header_value_option.has_append()) { + // 'append' is set and ensure the 'append_action' value is equal to the default value. + if (header_value_option.append_action() != HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD) { + throw EnvoyException("Both append and append_action are set and it's not allowed"); + } + + append_action = header_value_option.append().value() + ? HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD + : HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD; + } else { + append_action = header_value_option.append_action(); + } + + HeaderFormatterPtr header_formatter = parseInternal(header_value_option.header()); header_parser->headers_to_add_.emplace_back( Http::LowerCaseString(header_value_option.header().key()), HeadersToAddEntry{std::move(header_formatter), header_value_option.header().value(), - header_value_option.keep_empty_value()}); + append_action, header_value_option.keep_empty_value()}); } return header_parser; @@ -237,22 +248,22 @@ HeaderParserPtr HeaderParser::configure( HeaderParserPtr HeaderParser::configure( const Protobuf::RepeatedPtrField& headers_to_add, - bool append) { + HeaderAppendAction append_action) { HeaderParserPtr header_parser(new HeaderParser()); for (const auto& header_value : headers_to_add) { - HeaderFormatterPtr header_formatter = parseInternal(header_value, append); + HeaderFormatterPtr header_formatter = parseInternal(header_value); header_parser->headers_to_add_.emplace_back( Http::LowerCaseString(header_value.key()), - HeadersToAddEntry{std::move(header_formatter), header_value.value()}); + HeadersToAddEntry{std::move(header_formatter), header_value.value(), append_action}); } return header_parser; } -HeaderParserPtr HeaderParser::configure( - const Protobuf::RepeatedPtrField& headers_to_add, - const Protobuf::RepeatedPtrField& headers_to_remove) { +HeaderParserPtr +HeaderParser::configure(const Protobuf::RepeatedPtrField& headers_to_add, + const Protobuf::RepeatedPtrField& headers_to_remove) { HeaderParserPtr header_parser = configure(headers_to_add); for (const auto& header : headers_to_remove) { @@ -285,10 +296,20 @@ void HeaderParser::evaluateHeaders(Http::HeaderMap& headers, const std::string value = stream_info != nullptr ? entry.formatter_->format(*stream_info) : entry.original_value_; if (!value.empty() || entry.add_if_empty_) { - if (entry.formatter_->append()) { + switch (entry.append_action_) { + case HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD: headers.addReferenceKey(key, value); - } else { + break; + case HeaderAppendOption::ADD_IF_ABSENT: + if (auto header_entry = headers.get(key); header_entry.empty()) { + headers.addReferenceKey(key, value); + } + break; + case HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD: headers.setReferenceKey(key, value); + break; + default: + break; } } } @@ -302,17 +323,33 @@ Http::HeaderTransforms HeaderParser::getHeaderTransforms(const StreamInfo::Strea if (do_formatting) { const std::string value = entry.formatter_->format(stream_info); if (!value.empty() || entry.add_if_empty_) { - if (entry.formatter_->append()) { - transforms.headers_to_append.push_back({key, value}); - } else { - transforms.headers_to_overwrite.push_back({key, value}); + switch (entry.append_action_) { + case HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD: + transforms.headers_to_append_or_add.push_back({key, value}); + break; + case HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD: + transforms.headers_to_overwrite_or_add.push_back({key, value}); + break; + case HeaderAppendOption::ADD_IF_ABSENT: + transforms.headers_to_add_if_absent.push_back({key, value}); + break; + default: + break; } } } else { - if (entry.formatter_->append()) { - transforms.headers_to_append.push_back({key, entry.original_value_}); - } else { - transforms.headers_to_overwrite.push_back({key, entry.original_value_}); + switch (entry.append_action_) { + case HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD: + transforms.headers_to_append_or_add.push_back({key, entry.original_value_}); + break; + case HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD: + transforms.headers_to_overwrite_or_add.push_back({key, entry.original_value_}); + break; + case HeaderAppendOption::ADD_IF_ABSENT: + transforms.headers_to_add_if_absent.push_back({key, entry.original_value_}); + break; + default: + break; } } } diff --git a/source/common/router/header_parser.h b/source/common/router/header_parser.h index 733a894287a6..01d8912519bd 100644 --- a/source/common/router/header_parser.h +++ b/source/common/router/header_parser.h @@ -15,6 +15,8 @@ namespace Envoy { namespace Router { class HeaderParser; +using HeaderAppendAction = envoy::config::core::v3::HeaderValueOption::HeaderAppendAction; +using HeaderAppendOption = envoy::config::core::v3::HeaderValueOption; using HeaderParserPtr = std::unique_ptr; /** @@ -28,8 +30,8 @@ class HeaderParser : public Http::HeaderEvaluator { * @param headers_to_add defines the headers to add during calls to evaluateHeaders * @return HeaderParserPtr a configured HeaderParserPtr */ - static HeaderParserPtr configure( - const Protobuf::RepeatedPtrField& headers_to_add); + static HeaderParserPtr + configure(const Protobuf::RepeatedPtrField& headers_to_add); /* * @param headers_to_add defines headers to add during calls to evaluateHeaders. @@ -38,16 +40,16 @@ class HeaderParser : public Http::HeaderEvaluator { */ static HeaderParserPtr configure(const Protobuf::RepeatedPtrField& headers_to_add, - bool append); + HeaderAppendAction append_action); /* * @param headers_to_add defines headers to add during calls to evaluateHeaders * @param headers_to_remove defines headers to remove during calls to evaluateHeaders * @return HeaderParserPtr a configured HeaderParserPtr */ - static HeaderParserPtr configure( - const Protobuf::RepeatedPtrField& headers_to_add, - const Protobuf::RepeatedPtrField& headers_to_remove); + static HeaderParserPtr + configure(const Protobuf::RepeatedPtrField& headers_to_add, + const Protobuf::RepeatedPtrField& headers_to_remove); void evaluateHeaders(Http::HeaderMap& headers, const StreamInfo::StreamInfo& stream_info) const override; @@ -68,8 +70,9 @@ class HeaderParser : public Http::HeaderEvaluator { private: struct HeadersToAddEntry { - HeaderFormatterPtr formatter_; + const HeaderFormatterPtr formatter_; const std::string original_value_; + const HeaderAppendAction append_action_; const bool add_if_empty_ = false; }; diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 8055c1a80bd1..b9b648aa0d04 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -182,7 +182,7 @@ class ConfigImplTestBase { return factory_context_.scope().symbolTable().toString(name); } - std::string responseHeadersConfig(const bool most_specific_wins, const bool append) { + std::string responseHeadersConfig(const bool most_specific_wins, HeaderAppendAction action) { factory_context_.cluster_manager_.initializeClusters( {"www2", "root_www2", "www2_staging", "instant-server"}, {}); @@ -194,11 +194,11 @@ class ConfigImplTestBase { - header: key: x-global-header1 value: vhost-override - append: {1} + append_action: {1} - header: key: x-vhost-header1 value: vhost1-www2 - append: {1} + append_action: {1} response_headers_to_remove: ["x-vhost-remove"] routes: - match: @@ -210,15 +210,15 @@ class ConfigImplTestBase { - header: key: x-route-header value: route-override - append: {1} + append_action: {1} - header: key: x-global-header1 value: route-override - append: {1} + append_action: {1} - header: key: x-vhost-header1 value: route-override - append: {1} + append_action: {1} - match: path: "/" route: @@ -227,7 +227,7 @@ class ConfigImplTestBase { - header: key: x-route-header value: route-allpath - append: {1} + append_action: {1} response_headers_to_remove: ["x-route-remove"] - match: prefix: "/" @@ -239,7 +239,7 @@ class ConfigImplTestBase { - header: key: x-vhost-header1 value: vhost1-www2_staging - append: {1} + append_action: {1} routes: - match: prefix: "/" @@ -249,7 +249,7 @@ class ConfigImplTestBase { - header: key: x-route-header value: route-allprefix - append: {1} + append_action: {1} - name: default domains: ["*"] routes: @@ -262,12 +262,22 @@ internal_only_headers: ["x-lyft-user-id"] - header: key: x-global-header1 value: global1 - append: {1} + append_action: {1} response_headers_to_remove: ["x-global-remove"] most_specific_header_mutations_wins: {0} )EOF"; - return fmt::format(yaml, most_specific_wins, append); + std::string action_string; + + if (action == HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD) { + action_string = "APPEND_IF_EXISTS_OR_ADD"; + } else if (action == HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD) { + action_string = "OVERWRITE_IF_EXISTS_OR_ADD"; + } else if (action == HeaderAppendOption::ADD_IF_ABSENT) { + action_string = "ADD_IF_ABSENT"; + } + + return fmt::format(yaml, most_specific_wins, action_string); } std::string requestHeadersConfig(const bool most_specific_wins) { @@ -281,11 +291,11 @@ most_specific_header_mutations_wins: {0} - header: key: x-global-header value: vhost-www2 - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: x-vhost-header value: vhost-www2 - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD request_headers_to_remove: ["x-vhost-nope"] routes: - match: @@ -294,15 +304,15 @@ most_specific_header_mutations_wins: {0} - header: key: x-global-header value: route-endpoint - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: x-vhost-header value: route-endpoint - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: x-route-header value: route-endpoint - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD request_headers_to_remove: ["x-route-nope"] route: cluster: www2 @@ -321,7 +331,7 @@ most_specific_header_mutations_wins: {0} - header: key: x-global-header value: global - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD request_headers_to_remove: ["x-global-nope"] most_specific_header_mutations_wins: {0} )EOF"; @@ -1497,14 +1507,14 @@ TEST_F(RouteMatcherTest, TestAddRemoveRequestHeaders) { EXPECT_EQ("route-override", headers.get_("x-vhost-header1")); EXPECT_EQ("route-new_endpoint", headers.get_("x-route-header")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header1"), "route-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "route-override"), Pair(Http::LowerCaseString("x-route-header"), "route-new_endpoint"), Pair(Http::LowerCaseString("x-global-header1"), "vhost-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2"), Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-header-to-remove-at-route-level-1"), Http::LowerCaseString("x-header-to-remove-at-vhost-level-1"), @@ -1520,12 +1530,12 @@ TEST_F(RouteMatcherTest, TestAddRemoveRequestHeaders) { EXPECT_EQ("vhost1-www2", headers.get_("x-vhost-header1")); EXPECT_EQ("route-allpath", headers.get_("x-route-header")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-route-header"), "route-allpath"), Pair(Http::LowerCaseString("x-global-header1"), "vhost-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2"), Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-header-to-remove-at-route-level-2"), Http::LowerCaseString("x-header-to-remove-at-vhost-level-1"), @@ -1541,11 +1551,11 @@ TEST_F(RouteMatcherTest, TestAddRemoveRequestHeaders) { EXPECT_EQ("vhost1-www2_staging", headers.get_("x-vhost-header1")); EXPECT_EQ("route-allprefix", headers.get_("x-route-header")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-route-header"), "route-allprefix"), Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2_staging"), Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-header-to-remove-at-route-level-3"), Http::LowerCaseString("x-header-to-remove-at-vhost-level-2"), @@ -1559,9 +1569,9 @@ TEST_F(RouteMatcherTest, TestAddRemoveRequestHeaders) { route->finalizeRequestHeaders(headers, stream_info, true); EXPECT_EQ("global1", headers.get_("x-global-header1")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-header-to-remove-at-global-level"))); } @@ -1594,8 +1604,8 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalse) { EXPECT_FALSE(headers.has("x-vhost-nope")); EXPECT_FALSE(headers.has("x-route-nope")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, IsEmpty()); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header"), "route-endpoint"), Pair(Http::LowerCaseString("x-vhost-header"), "route-endpoint"), Pair(Http::LowerCaseString("x-route-header"), "route-endpoint"), @@ -1622,8 +1632,8 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalse) { EXPECT_FALSE(headers.has("x-vhost-nope")); EXPECT_TRUE(headers.has("x-route-nope")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, IsEmpty()); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header"), "vhost-www2"), Pair(Http::LowerCaseString("x-vhost-header"), "vhost-www2"), Pair(Http::LowerCaseString("x-global-header"), "global"))); @@ -1646,8 +1656,8 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalse) { EXPECT_TRUE(headers.has("x-vhost-nope")); EXPECT_TRUE(headers.has("x-route-nope")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, IsEmpty()); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header"), "global"))); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-global-nope"))); @@ -1675,8 +1685,8 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalseMostSpecificWins) EXPECT_FALSE(headers.has("x-vhost-nope")); EXPECT_FALSE(headers.has("x-route-nope")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, IsEmpty()); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header"), "global"), Pair(Http::LowerCaseString("x-global-header"), "vhost-www2"), Pair(Http::LowerCaseString("x-vhost-header"), "vhost-www2"), @@ -1702,8 +1712,8 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalseMostSpecificWins) EXPECT_FALSE(headers.has("x-vhost-nope")); EXPECT_TRUE(headers.has("x-route-nope")); auto transforms = route->requestHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, IsEmpty()); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header"), "global"), Pair(Http::LowerCaseString("x-global-header"), "vhost-www2"), Pair(Http::LowerCaseString("x-vhost-header"), "vhost-www2"))); @@ -1715,7 +1725,8 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalseMostSpecificWins) // Validates behavior of response_headers_to_add and response_headers_to_remove at router, vhost, // and route levels. TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { - const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/false, /*append=*/true); + const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/false, + HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD); NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); @@ -1732,14 +1743,14 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { EXPECT_EQ("route-override", headers.get_("x-vhost-header1")); EXPECT_EQ("route-override", headers.get_("x-route-header")); auto transforms = route->responseHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-route-header"), "route-override"), Pair(Http::LowerCaseString("x-global-header1"), "route-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "route-override"), Pair(Http::LowerCaseString("x-global-header1"), "vhost-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2"), Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-vhost-remove"), Http::LowerCaseString("x-global-remove"))); @@ -1755,12 +1766,12 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { EXPECT_EQ("vhost1-www2", headers.get_("x-vhost-header1")); EXPECT_EQ("route-allpath", headers.get_("x-route-header")); auto transforms = route->responseHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-route-header"), "route-allpath"), Pair(Http::LowerCaseString("x-global-header1"), "vhost-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2"), Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-route-remove"), Http::LowerCaseString("x-vhost-remove"), @@ -1778,11 +1789,11 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { EXPECT_EQ("vhost1-www2_staging", headers.get_("x-vhost-header1")); EXPECT_EQ("route-allprefix", headers.get_("x-route-header")); auto transforms = route->responseHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-route-header"), "route-allprefix"), Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2_staging"), Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-global-remove"))); } @@ -1795,9 +1806,9 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { route->finalizeResponseHeaders(headers, stream_info); EXPECT_EQ("global1", headers.get_("x-global-header1")); auto transforms = route->responseHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header1"), "global1"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-global-remove"))); } @@ -1807,8 +1818,9 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { ContainerEq(config.internalOnlyHeaders())); } -TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAppendFalse) { - const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/false, /*append=*/false); +TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersOverwriteIfExistOrAdd) { + const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/false, + HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); @@ -1823,8 +1835,39 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAppendFalse) { EXPECT_EQ("route-override", headers.get_("x-route-header")); auto transforms = route->responseHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, IsEmpty()); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, + ElementsAre(Pair(Http::LowerCaseString("x-route-header"), "route-override"), + Pair(Http::LowerCaseString("x-global-header1"), "route-override"), + Pair(Http::LowerCaseString("x-vhost-header1"), "route-override"), + Pair(Http::LowerCaseString("x-global-header1"), "vhost-override"), + Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2"), + Pair(Http::LowerCaseString("x-global-header1"), "global1"))); + EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-vhost-remove"), + Http::LowerCaseString("x-global-remove"))); +} + +TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAddIfAbsent) { + const std::string yaml = + responseHeadersConfig(/*most_specific_wins=*/false, HeaderAppendOption::ADD_IF_ABSENT); + NiceMock stream_info; + + TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); + + Http::TestRequestHeaderMapImpl req_headers = + genHeaders("www.lyft.com", "/new_endpoint/foo", "GET"); + const RouteEntry* route = config.route(req_headers, 0)->routeEntry(); + Http::TestResponseHeaderMapImpl headers{{":status", "200"}, {"x-route-header", "exist-value"}}; + route->finalizeResponseHeaders(headers, stream_info); + EXPECT_EQ("global1", headers.get_("x-global-header1")); + EXPECT_EQ("vhost1-www2", headers.get_("x-vhost-header1")); + // If related header is exist in the headers then do nothing. + EXPECT_EQ("exist-value", headers.get_("x-route-header")); + + auto transforms = route->responseHeaderTransforms(stream_info); + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_add_if_absent, ElementsAre(Pair(Http::LowerCaseString("x-route-header"), "route-override"), Pair(Http::LowerCaseString("x-global-header1"), "route-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "route-override"), @@ -1836,7 +1879,8 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAppendFalse) { } TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAppendMostSpecificWins) { - const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/true, /*append=*/false); + const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/true, + HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); @@ -1851,8 +1895,8 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAppendMostSpecificWins) { EXPECT_EQ("route-override", headers.get_("x-route-header")); auto transforms = route->responseHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, IsEmpty()); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_append_or_add, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-global-header1"), "global1"), Pair(Http::LowerCaseString("x-global-header1"), "vhost-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "vhost1-www2"), @@ -1880,7 +1924,7 @@ class HeaderTransformsDoFormattingTest : public RouteMatcherTest { - header: key: x-has-variable value: "%PER_REQUEST_STATE(testing)%" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD )EOF"; const std::string yaml = fmt::format(yaml_template, @@ -1907,14 +1951,14 @@ class HeaderTransformsDoFormattingTest : public RouteMatcherTest { auto transforms = run_request_header_test ? route->requestHeaderTransforms(stream_info, /*do_formatting=*/true) : route->responseHeaderTransforms(stream_info, /*do_formatting=*/true); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-has-variable"), "test_value"))); transforms = run_request_header_test ? route->requestHeaderTransforms(stream_info, /*do_formatting=*/false) : route->responseHeaderTransforms(stream_info, /*do_formatting=*/false); EXPECT_THAT( - transforms.headers_to_overwrite, + transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-has-variable"), "%PER_REQUEST_STATE(testing)%"))); } }; @@ -1981,7 +2025,7 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddNoHostOrPseudoHeader) { - header: key: {} value: vhost-www2 - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD )EOF", header); @@ -6021,12 +6065,12 @@ class WeightedClustersHeaderTransformationsTest : public RouteMatcherTest { const RouteEntry* route = config.route(req_headers, 0)->routeEntry(); auto transforms = run_request_header_test ? route->requestHeaderTransforms(stream_info) : route->responseHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-cluster-header"), "cluster1"), Pair(Http::LowerCaseString("x-route-header"), "route-override"), Pair(Http::LowerCaseString("x-global-header1"), "route-override"), Pair(Http::LowerCaseString("x-vhost-header1"), "route-override"))); - EXPECT_THAT(transforms.headers_to_overwrite, IsEmpty()); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, IsEmpty()); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-vhost-remove"))); } }; diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index aabd1c883f10..f97ab769ecaa 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -49,7 +49,7 @@ class StreamInfoHeaderFormatterTest : public testing::Test { void testFormatting(const Envoy::StreamInfo::MockStreamInfo& stream_info, const std::string& variable, const std::string& expected_output) { { - auto f = StreamInfoHeaderFormatter(variable, false); + auto f = StreamInfoHeaderFormatter(variable); const std::string formatted_string = f.format(stream_info); EXPECT_EQ(expected_output, formatted_string); } @@ -61,7 +61,7 @@ class StreamInfoHeaderFormatterTest : public testing::Test { } void testInvalidFormat(const std::string& variable) { - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter(variable, false), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter{variable}, EnvoyException, fmt::format("field '{}' not supported as custom header", variable)); } }; @@ -849,19 +849,17 @@ TEST_F(StreamInfoHeaderFormatterTest, TestFormatWithNonStringPerRequestStateVari TEST_F(StreamInfoHeaderFormatterTest, WrongFormatOnPerRequestStateVariable) { // No parameters - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("PER_REQUEST_STATE()", false), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("PER_REQUEST_STATE()"), EnvoyException, "Invalid header configuration. Expected format " "PER_REQUEST_STATE(), actual format " "PER_REQUEST_STATE()"); // Missing single parens - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("PER_REQUEST_STATE(testing", false), - EnvoyException, + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("PER_REQUEST_STATE(testing"), EnvoyException, "Invalid header configuration. Expected format " "PER_REQUEST_STATE(), actual format " "PER_REQUEST_STATE(testing"); - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("PER_REQUEST_STATE testing)", false), - EnvoyException, + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("PER_REQUEST_STATE testing)"), EnvoyException, "Invalid header configuration. Expected format " "PER_REQUEST_STATE(), actual format " "PER_REQUEST_STATE testing)"); @@ -871,8 +869,7 @@ TEST_F(StreamInfoHeaderFormatterTest, UnknownVariable) { testInvalidFormat("INVA TEST_F(StreamInfoHeaderFormatterTest, WrongFormatOnUpstreamMetadataVariable) { // Invalid JSON. - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA(abcd)", false), - EnvoyException, + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA(abcd)"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " "UPSTREAM_METADATA(abcd), because JSON supplied is not valid. " @@ -880,38 +877,38 @@ TEST_F(StreamInfoHeaderFormatterTest, WrongFormatOnUpstreamMetadataVariable) { "invalid literal; last read: 'a'\n"); // No parameters. - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA", false), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " "UPSTREAM_METADATA"); EXPECT_THROW_WITH_MESSAGE( - StreamInfoHeaderFormatter("UPSTREAM_METADATA()", false), EnvoyException, + StreamInfoHeaderFormatter("UPSTREAM_METADATA()"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format UPSTREAM_METADATA(), " "because JSON supplied is not valid. Error(line 1, column 1, token ): syntax error while " "parsing value - unexpected end of input; expected '[', '{', or a literal\n"); // One parameter. - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"ns\"])", false), + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"ns\"])"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " "UPSTREAM_METADATA([\"ns\"])"); // Missing close paren. - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA(", false), EnvoyException, + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA("), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " "UPSTREAM_METADATA("); - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA([a,b,c,d]", false), + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA([a,b,c,d]"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " "UPSTREAM_METADATA([a,b,c,d]"); - EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"a\",\"b\"]", false), + EXPECT_THROW_WITH_MESSAGE(StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"a\",\"b\"]"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " @@ -919,7 +916,7 @@ TEST_F(StreamInfoHeaderFormatterTest, WrongFormatOnUpstreamMetadataVariable) { // Non-string elements. EXPECT_THROW_WITH_MESSAGE( - StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"a\", 1])", false), EnvoyException, + StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"a\", 1])"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " "UPSTREAM_METADATA([\"a\", 1]), because JSON field from line 1 accessed with type 'String' " @@ -927,7 +924,7 @@ TEST_F(StreamInfoHeaderFormatterTest, WrongFormatOnUpstreamMetadataVariable) { // Invalid string elements. EXPECT_THROW_WITH_MESSAGE( - StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"a\", \"\\unothex\"])", false), EnvoyException, + StreamInfoHeaderFormatter("UPSTREAM_METADATA([\"a\", \"\\unothex\"])"), EnvoyException, "Invalid header configuration. Expected format UPSTREAM_METADATA([\"namespace\", " "\"k\", ...]), actual format UPSTREAM_METADATA([\"a\", \"\\unothex\"]), because JSON " "supplied is not valid. Error(line 1, column 10, token \"\\un): syntax error while parsing " @@ -935,7 +932,7 @@ TEST_F(StreamInfoHeaderFormatterTest, WrongFormatOnUpstreamMetadataVariable) { // Non-array parameters. EXPECT_THROW_WITH_MESSAGE( - StreamInfoHeaderFormatter("UPSTREAM_METADATA({\"a\":1})", false), EnvoyException, + StreamInfoHeaderFormatter("UPSTREAM_METADATA({\"a\":1})"), EnvoyException, "Invalid header configuration. Expected format " "UPSTREAM_METADATA([\"namespace\", \"k\", ...]), actual format " "UPSTREAM_METADATA({\"a\":1}), because JSON field from line 1 accessed with type 'Array' " @@ -948,8 +945,7 @@ TEST_F(StreamInfoHeaderFormatterTest, WrongFormatOnUpstreamMetadataVariable) { "5211111111111111111111111111111111111111111111111111111111111111111111111111111111111111" "1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" "1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" - "1111111111111111111111111111111111111111111111111)", - false), + "1111111111111111111111111111111111111111111111111)"), EnvoyException, "Invalid header configuration. Expected format UPSTREAM_METADATA([\"namespace\", \"k\", " "...]), actual format " @@ -1193,14 +1189,14 @@ match: { prefix: "/new_endpoint" } - header: key: "x-client-ip" value: "%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-client-ip-port" value: "%DOWNSTREAM_REMOTE_ADDRESS%" - header: key: "x-client-port" value: "%DOWNSTREAM_REMOTE_PORT%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD )EOF"; HeaderParserPtr req_header_parser = @@ -1223,12 +1219,12 @@ match: { prefix: "/new_endpoint" } - header: key: "x-upstream-remote-address" value: "%UPSTREAM_REMOTE_ADDRESS%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD keep_empty_value: true - header: key: "x-upstream-local-port" value: "%UPSTREAM_LOCAL_PORT%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD )EOF"; HeaderParserPtr req_header_parser = @@ -1252,14 +1248,14 @@ match: { prefix: "/new_endpoint" } - header: key: "x-client-ip" value: "%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-client-ip-port" value: "%DOWNSTREAM_REMOTE_ADDRESS%" - header: key: "x-client-port" value: "%DOWNSTREAM_REMOTE_PORT%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD )EOF"; HeaderParserPtr req_header_parser = @@ -1284,7 +1280,8 @@ TEST(HeaderParserTest, EvaluateHeaderValuesWithNullStreamInfo) { // This tests when we have "StreamInfoHeaderFormatter", but stream info is null. first_entry.set_value("%DOWNSTREAM_REMOTE_ADDRESS%"); - HeaderParserPtr req_header_parser_add = HeaderParser::configure(headers_values, /*append=*/true); + HeaderParserPtr req_header_parser_add = + HeaderParser::configure(headers_values, HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD); req_header_parser_add->evaluateHeaders(header_map, nullptr); EXPECT_TRUE(header_map.has("key")); EXPECT_EQ("%DOWNSTREAM_REMOTE_ADDRESS%", header_map.get_("key")); @@ -1294,7 +1291,8 @@ TEST(HeaderParserTest, EvaluateHeaderValuesWithNullStreamInfo) { set_entry.set_key("key"); set_entry.set_value("great"); - HeaderParserPtr req_header_parser_set = HeaderParser::configure(headers_values, /*append=*/false); + HeaderParserPtr req_header_parser_set = + HeaderParser::configure(headers_values, HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); req_header_parser_set->evaluateHeaders(header_map, nullptr); EXPECT_TRUE(header_map.has("key")); EXPECT_EQ("great", header_map.get_("key")); @@ -1305,7 +1303,7 @@ TEST(HeaderParserTest, EvaluateHeaderValuesWithNullStreamInfo) { empty_entry.set_value(""); HeaderParserPtr req_header_parser_empty = - HeaderParser::configure(headers_values, /*append=*/false); + HeaderParser::configure(headers_values, HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); req_header_parser_empty->evaluateHeaders(header_map, nullptr); EXPECT_FALSE(header_map.has("empty")); } @@ -1320,7 +1318,7 @@ match: { prefix: "/new_endpoint" } - header: key: "x-key" value: "%UPSTREAM_METADATA([\"namespace\", \"key\"])%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD )EOF"; HeaderParserPtr req_header_parser = @@ -1346,7 +1344,7 @@ match: { prefix: "/new_endpoint" } - header: key: "static-header" value: "static-value" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD )EOF"; HeaderParserPtr req_header_parser = @@ -1469,23 +1467,23 @@ match: { prefix: "/new_endpoint" } - header: key: "static-header" value: "static-value" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-client-ip" value: "%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start" value: "%START_TIME(%s%3f)%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start-default" value: "%START_TIME%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start-range" value: "%START_TIME(%f, %1f, %2f, %3f, %4f, %5f, %6f, %7f, %8f, %9f)%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD )EOF"; // Disable append mode. @@ -1543,38 +1541,38 @@ match: { prefix: "/new_endpoint" } - header: key: "x-client-ip" value: "%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-client-ip-port" value: "%DOWNSTREAM_REMOTE_ADDRESS%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start" value: "%START_TIME(%s.%3f)%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start-multiple" value: "%START_TIME(%s.%3f)% %START_TIME% %START_TIME(%s)%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start-f" value: "%START_TIME(f)%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start-range" value: "%START_TIME(%f, %1f, %2f, %3f, %4f, %5f, %6f, %7f, %8f, %9f)%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-request-start-default" value: "%START_TIME%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "set-cookie" value: "foo" - header: key: "set-cookie" value: "bar" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD response_headers_to_remove: ["x-nope"] )EOF"; @@ -1639,6 +1637,66 @@ request_headers_to_remove: ["x-foo-header"] EXPECT_EQ("bar", header_map.get_("x-foo-header")); } +TEST(HeaderParserTest, EvaluateRequestHeadersAddIfAbsent) { + const std::string yaml = R"EOF( +match: { prefix: "/new_endpoint" } +route: + cluster: www2 +response_headers_to_add: + - header: + key: "x-foo-header" + value: "foo" + append_action: APPEND_IF_EXISTS_OR_ADD + - header: + key: "x-bar-header" + value: "bar" + append_action: OVERWRITE_IF_EXISTS_OR_ADD + - header: + key: "x-per-header" + value: "per" + append_action: ADD_IF_ABSENT +response_headers_to_remove: ["x-baz-header"] +)EOF"; + + const auto route = parseRouteFromV3Yaml(yaml); + HeaderParserPtr req_header_parser = + HeaderParser::configure(route.request_headers_to_add(), route.request_headers_to_remove()); + NiceMock stream_info; + + { + Http::TestResponseHeaderMapImpl header_map; + req_header_parser->evaluateHeaders(header_map, stream_info); + EXPECT_EQ("foo", header_map.get_("x-foo-header")); + EXPECT_EQ("bar", header_map.get_("x-bar-header")); + EXPECT_EQ("per", header_map.get_("x-per-header")); + } + + { + Http::TestResponseHeaderMapImpl header_map{{"x-foo-header", "exist-foo"}}; + req_header_parser->evaluateHeaders(header_map, stream_info); + EXPECT_EQ(2, header_map.get(Http::LowerCaseString("x-foo-header")).size()); + EXPECT_EQ("bar", header_map.get_("x-bar-header")); + EXPECT_EQ("per", header_map.get_("x-per-header")); + } + + { + Http::TestResponseHeaderMapImpl header_map{{"x-bar-header", "exist-bar"}}; + req_header_parser->evaluateHeaders(header_map, stream_info); + EXPECT_EQ("foo", header_map.get_("x-foo-header")); + EXPECT_EQ("bar", header_map.get_("x-bar-header")); + EXPECT_EQ(1, header_map.get(Http::LowerCaseString("x-bar-header")).size()); + EXPECT_EQ("per", header_map.get_("x-per-header")); + } + + { + Http::TestResponseHeaderMapImpl header_map{{"x-per-header", "exist-per"}}; + req_header_parser->evaluateHeaders(header_map, stream_info); + EXPECT_EQ("foo", header_map.get_("x-foo-header")); + EXPECT_EQ("bar", header_map.get_("x-bar-header")); + EXPECT_EQ("exist-per", header_map.get_("x-per-header")); + } +} + TEST(HeaderParserTest, EvaluateResponseHeadersRemoveBeforeAdd) { const std::string yaml = R"EOF( match: { prefix: "/new_endpoint" } @@ -1670,15 +1728,15 @@ match: { prefix: "/new_endpoint" } - header: key: "x-foo-header" value: "foo" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-bar-header" value: "bar" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: "x-per-request-header" value: "%PER_REQUEST_STATE(testing)%" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD response_headers_to_remove: ["x-baz-header"] )EOF"; @@ -1697,9 +1755,9 @@ response_headers_to_remove: ["x-baz-header"] ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); auto transforms = resp_header_parser->getHeaderTransforms(stream_info); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-foo-header"), "foo"))); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-bar-header"), "bar"), Pair(Http::LowerCaseString("x-per-request-header"), "test_value"))); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-baz-header"))); @@ -1714,15 +1772,15 @@ match: { prefix: "/new_endpoint" } - header: key: "x-foo-header" value: "foo" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: "x-bar-header" value: "bar" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: "x-per-request-header" value: "%PER_REQUEST_STATE(testing)%" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD response_headers_to_remove: ["x-baz-header"] )EOF"; @@ -1742,15 +1800,53 @@ response_headers_to_remove: ["x-baz-header"] auto transforms = response_header_parser->getHeaderTransforms(stream_info, /*do_formatting=*/false); - EXPECT_THAT(transforms.headers_to_append, + EXPECT_THAT(transforms.headers_to_append_or_add, ElementsAre(Pair(Http::LowerCaseString("x-foo-header"), "foo"))); - EXPECT_THAT(transforms.headers_to_overwrite, + EXPECT_THAT(transforms.headers_to_overwrite_or_add, ElementsAre(Pair(Http::LowerCaseString("x-bar-header"), "bar"), Pair(Http::LowerCaseString("x-per-request-header"), "%PER_REQUEST_STATE(testing)%"))); EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-baz-header"))); } +TEST(HeaderParserTest, GetHeaderTransformsForAllActions) { + const std::string yaml = R"EOF( +match: { prefix: "/new_endpoint" } +route: + cluster: www2 +response_headers_to_add: + - header: + key: "x-foo-header" + value: "foo" + append_action: APPEND_IF_EXISTS_OR_ADD + - header: + key: "x-bar-header" + value: "bar" + append_action: OVERWRITE_IF_EXISTS_OR_ADD + - header: + key: "x-per-header" + value: "per" + append_action: ADD_IF_ABSENT +response_headers_to_remove: ["x-baz-header"] +)EOF"; + + const auto route = parseRouteFromV3Yaml(yaml); + HeaderParserPtr response_header_parser = + HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); + NiceMock stream_info; + + auto transforms = + response_header_parser->getHeaderTransforms(stream_info, /*do_formatting=*/false); + EXPECT_THAT(transforms.headers_to_append_or_add, + ElementsAre(Pair(Http::LowerCaseString("x-foo-header"), "foo"))); + EXPECT_THAT(transforms.headers_to_overwrite_or_add, + ElementsAre(Pair(Http::LowerCaseString("x-bar-header"), "bar"))); + EXPECT_THAT(transforms.headers_to_add_if_absent, + ElementsAre(Pair(Http::LowerCaseString("x-per-header"), "per"))); + + EXPECT_THAT(transforms.headers_to_remove, ElementsAre(Http::LowerCaseString("x-baz-header"))); +} + } // namespace } // namespace Router } // namespace Envoy From 4ef483fcc8d2441681aa8df2ce987aa883642156 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 23 Aug 2022 08:28:49 +0000 Subject: [PATCH 02/19] mark append to deprecated and add test for deprecated append Signed-off-by: wbpcode --- api/envoy/config/core/v3/base.proto | 4 +- test/common/router/header_formatter_test.cc | 44 +++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 30fe66d9a839..2633d300b9b1 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -356,7 +356,9 @@ message HeaderValueOption { // Should the value be appended? If true (default), the value is appended to // existing values. Otherwise it replaces any existing values. - google.protobuf.BoolValue append = 2; + // This field is deprecated and please use + // :ref:`append_action ` as replacement. + google.protobuf.BoolValue append = 2 [deprecated = true]; // Describes the action taken to append/overwrite the given value for an existing header // or to only add this header if it's absent. diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index f97ab769ecaa..1c4769c11460 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -1697,6 +1697,50 @@ response_headers_to_remove: ["x-baz-header"] } } +TEST(HeaderParserTest, DEPRECATED_FEATURE_TEST(EvaluateRequestHeadersAddWithDeprecatedAppend)) { + const std::string yaml = R"EOF( +match: { prefix: "/new_endpoint" } +route: + cluster: www2 +response_headers_to_add: + - header: + key: "x-foo-header" + value: "foo" + append: true + - header: + key: "x-bar-header" + value: "bar" + append: false +)EOF"; + + const auto route = parseRouteFromV3Yaml(yaml); + HeaderParserPtr req_header_parser = + HeaderParser::configure(route.request_headers_to_add(), route.request_headers_to_remove()); + NiceMock stream_info; + + { + Http::TestResponseHeaderMapImpl header_map; + req_header_parser->evaluateHeaders(header_map, stream_info); + EXPECT_EQ("foo", header_map.get_("x-foo-header")); + EXPECT_EQ("bar", header_map.get_("x-bar-header")); + } + + { + Http::TestResponseHeaderMapImpl header_map{{"x-foo-header", "exist-foo"}}; + req_header_parser->evaluateHeaders(header_map, stream_info); + EXPECT_EQ(2, header_map.get(Http::LowerCaseString("x-foo-header")).size()); + EXPECT_EQ("bar", header_map.get_("x-bar-header")); + } + + { + Http::TestResponseHeaderMapImpl header_map{{"x-bar-header", "exist-bar"}}; + req_header_parser->evaluateHeaders(header_map, stream_info); + EXPECT_EQ("foo", header_map.get_("x-foo-header")); + EXPECT_EQ("bar", header_map.get_("x-bar-header")); + EXPECT_EQ(1, header_map.get(Http::LowerCaseString("x-bar-header")).size()); + } +} + TEST(HeaderParserTest, EvaluateResponseHeadersRemoveBeforeAdd) { const std::string yaml = R"EOF( match: { prefix: "/new_endpoint" } From 2c11cccf84b5b66e127eee344d11ac75bf3f4d12 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 23 Aug 2022 08:31:46 +0000 Subject: [PATCH 03/19] remove not implemented hide Signed-off-by: wbpcode --- 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 2633d300b9b1..5ba74d60a1da 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -334,7 +334,7 @@ message HeaderValueOption { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.HeaderValueOption"; - // [#not-implemented-hide:] Describes the supported actions types for header append action. + // 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 From fc9908eb0356a3664a1501d26885277d999b7ab4 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 23 Aug 2022 11:22:40 +0000 Subject: [PATCH 04/19] fix proto Signed-off-by: wbpcode --- api/envoy/config/core/v3/base.proto | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 5ba74d60a1da..52a3b8a2c54e 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -358,7 +358,8 @@ message HeaderValueOption { // existing values. Otherwise it replaces any existing values. // This field is deprecated and please use // :ref:`append_action ` as replacement. - google.protobuf.BoolValue append = 2 [deprecated = true]; + google.protobuf.BoolValue append = 2 + [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; // Describes the action taken to append/overwrite the given value for an existing header // or to only add this header if it's absent. From 12db5e10be72829d41a3507b952880e553cbacdd Mon Sep 17 00:00:00 2001 From: wbpcode Date: Tue, 23 Aug 2022 14:20:05 +0000 Subject: [PATCH 05/19] fix type error and test Signed-off-by: wbpcode --- source/common/grpc/async_client_impl.cc | 4 +-- source/common/router/config_impl.cc | 14 ++++++---- source/common/router/header_parser.cc | 30 ++++++++++----------- source/common/router/header_parser.h | 17 ++++++------ test/common/router/header_formatter_test.cc | 29 ++++++++++---------- 5 files changed, 49 insertions(+), 45 deletions(-) diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index 2bbabb86fa91..086019beea3b 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -19,8 +19,8 @@ AsyncClientImpl::AsyncClientImpl(Upstream::ClusterManager& cm, TimeSource& time_source) : cm_(cm), remote_cluster_name_(config.envoy_grpc().cluster_name()), host_name_(config.envoy_grpc().authority()), time_source_(time_source), - metadata_parser_( - Router::HeaderParser::configure(config.initial_metadata(), /*append=*/false)) {} + metadata_parser_(Router::HeaderParser::configure( + config.initial_metadata(), Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} AsyncClientImpl::~AsyncClientImpl() { ASSERT(isThreadSafe()); diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 64a4496b849d..9d35db16064c 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -58,11 +58,15 @@ namespace { constexpr uint32_t DEFAULT_MAX_DIRECT_RESPONSE_BODY_SIZE_BYTES = 4096; void mergeTransforms(Http::HeaderTransforms& dest, const Http::HeaderTransforms& src) { - dest.headers_to_append.insert(dest.headers_to_append.end(), src.headers_to_append.begin(), - src.headers_to_append.end()); - dest.headers_to_overwrite.insert(dest.headers_to_overwrite.end(), - src.headers_to_overwrite.begin(), - src.headers_to_overwrite.end()); + dest.headers_to_append_or_add.insert(dest.headers_to_append_or_add.end(), + src.headers_to_append_or_add.begin(), + src.headers_to_append_or_add.end()); + dest.headers_to_overwrite_or_add.insert(dest.headers_to_overwrite_or_add.end(), + src.headers_to_overwrite_or_add.begin(), + src.headers_to_overwrite_or_add.end()); + dest.headers_to_add_if_absent.insert(dest.headers_to_add_if_absent.end(), + src.headers_to_add_if_absent.begin(), + src.headers_to_add_if_absent.end()); dest.headers_to_remove.insert(dest.headers_to_remove.end(), src.headers_to_remove.begin(), src.headers_to_remove.end()); } diff --git a/source/common/router/header_parser.cc b/source/common/router/header_parser.cc index 0435e6896e6b..de2408261883 100644 --- a/source/common/router/header_parser.cc +++ b/source/common/router/header_parser.cc @@ -217,7 +217,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea } // namespace HeaderParserPtr -HeaderParser::configure(const Protobuf::RepeatedPtrField& headers_to_add) { +HeaderParser::configure(const Protobuf::RepeatedPtrField& headers_to_add) { HeaderParserPtr header_parser(new HeaderParser()); for (const auto& header_value_option : headers_to_add) { @@ -225,13 +225,13 @@ HeaderParser::configure(const Protobuf::RepeatedPtrField& he if (header_value_option.has_append()) { // 'append' is set and ensure the 'append_action' value is equal to the default value. - if (header_value_option.append_action() != HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD) { + if (header_value_option.append_action() != HeaderValueOption::APPEND_IF_EXISTS_OR_ADD) { throw EnvoyException("Both append and append_action are set and it's not allowed"); } append_action = header_value_option.append().value() - ? HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD - : HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD; + ? HeaderValueOption::APPEND_IF_EXISTS_OR_ADD + : HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD; } else { append_action = header_value_option.append_action(); } @@ -262,7 +262,7 @@ HeaderParserPtr HeaderParser::configure( } HeaderParserPtr -HeaderParser::configure(const Protobuf::RepeatedPtrField& headers_to_add, +HeaderParser::configure(const Protobuf::RepeatedPtrField& headers_to_add, const Protobuf::RepeatedPtrField& headers_to_remove) { HeaderParserPtr header_parser = configure(headers_to_add); @@ -297,19 +297,19 @@ void HeaderParser::evaluateHeaders(Http::HeaderMap& headers, stream_info != nullptr ? entry.formatter_->format(*stream_info) : entry.original_value_; if (!value.empty() || entry.add_if_empty_) { switch (entry.append_action_) { - case HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD: + case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: headers.addReferenceKey(key, value); break; - case HeaderAppendOption::ADD_IF_ABSENT: + case HeaderValueOption::ADD_IF_ABSENT: if (auto header_entry = headers.get(key); header_entry.empty()) { headers.addReferenceKey(key, value); } break; - case HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD: + case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: headers.setReferenceKey(key, value); break; default: - break; + PANIC_DUE_TO_CORRUPT_ENUM; } } } @@ -324,13 +324,13 @@ Http::HeaderTransforms HeaderParser::getHeaderTransforms(const StreamInfo::Strea const std::string value = entry.formatter_->format(stream_info); if (!value.empty() || entry.add_if_empty_) { switch (entry.append_action_) { - case HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD: + case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: transforms.headers_to_append_or_add.push_back({key, value}); break; - case HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD: + case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: transforms.headers_to_overwrite_or_add.push_back({key, value}); break; - case HeaderAppendOption::ADD_IF_ABSENT: + case HeaderValueOption::ADD_IF_ABSENT: transforms.headers_to_add_if_absent.push_back({key, value}); break; default: @@ -339,13 +339,13 @@ Http::HeaderTransforms HeaderParser::getHeaderTransforms(const StreamInfo::Strea } } else { switch (entry.append_action_) { - case HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD: + case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: transforms.headers_to_append_or_add.push_back({key, entry.original_value_}); break; - case HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD: + case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: transforms.headers_to_overwrite_or_add.push_back({key, entry.original_value_}); break; - case HeaderAppendOption::ADD_IF_ABSENT: + case HeaderValueOption::ADD_IF_ABSENT: transforms.headers_to_add_if_absent.push_back({key, entry.original_value_}); break; default: diff --git a/source/common/router/header_parser.h b/source/common/router/header_parser.h index 01d8912519bd..4006822315ac 100644 --- a/source/common/router/header_parser.h +++ b/source/common/router/header_parser.h @@ -15,10 +15,11 @@ namespace Envoy { namespace Router { class HeaderParser; -using HeaderAppendAction = envoy::config::core::v3::HeaderValueOption::HeaderAppendAction; -using HeaderAppendOption = envoy::config::core::v3::HeaderValueOption; using HeaderParserPtr = std::unique_ptr; +using HeaderAppendAction = envoy::config::core::v3::HeaderValueOption::HeaderAppendAction; +using HeaderValueOption = envoy::config::core::v3::HeaderValueOption; + /** * HeaderParser manipulates Http::HeaderMap instances. Headers to be added are pre-parsed to select * between a constant value implementation and a dynamic value implementation based on @@ -31,7 +32,7 @@ class HeaderParser : public Http::HeaderEvaluator { * @return HeaderParserPtr a configured HeaderParserPtr */ static HeaderParserPtr - configure(const Protobuf::RepeatedPtrField& headers_to_add); + configure(const Protobuf::RepeatedPtrField& headers_to_add); /* * @param headers_to_add defines headers to add during calls to evaluateHeaders. @@ -48,7 +49,7 @@ class HeaderParser : public Http::HeaderEvaluator { * @return HeaderParserPtr a configured HeaderParserPtr */ static HeaderParserPtr - configure(const Protobuf::RepeatedPtrField& headers_to_add, + configure(const Protobuf::RepeatedPtrField& headers_to_add, const Protobuf::RepeatedPtrField& headers_to_remove); void evaluateHeaders(Http::HeaderMap& headers, @@ -70,10 +71,10 @@ class HeaderParser : public Http::HeaderEvaluator { private: struct HeadersToAddEntry { - const HeaderFormatterPtr formatter_; - const std::string original_value_; - const HeaderAppendAction append_action_; - const bool add_if_empty_ = false; + HeaderFormatterPtr formatter_; + std::string original_value_; + HeaderAppendAction append_action_; + bool add_if_empty_ = false; }; std::vector> headers_to_add_; diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 1c4769c11460..78bf165eafc2 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -1281,7 +1281,7 @@ TEST(HeaderParserTest, EvaluateHeaderValuesWithNullStreamInfo) { first_entry.set_value("%DOWNSTREAM_REMOTE_ADDRESS%"); HeaderParserPtr req_header_parser_add = - HeaderParser::configure(headers_values, HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD); + HeaderParser::configure(headers_values, HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); req_header_parser_add->evaluateHeaders(header_map, nullptr); EXPECT_TRUE(header_map.has("key")); EXPECT_EQ("%DOWNSTREAM_REMOTE_ADDRESS%", header_map.get_("key")); @@ -1292,7 +1292,7 @@ TEST(HeaderParserTest, EvaluateHeaderValuesWithNullStreamInfo) { set_entry.set_value("great"); HeaderParserPtr req_header_parser_set = - HeaderParser::configure(headers_values, HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); + HeaderParser::configure(headers_values, HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); req_header_parser_set->evaluateHeaders(header_map, nullptr); EXPECT_TRUE(header_map.has("key")); EXPECT_EQ("great", header_map.get_("key")); @@ -1303,7 +1303,7 @@ TEST(HeaderParserTest, EvaluateHeaderValuesWithNullStreamInfo) { empty_entry.set_value(""); HeaderParserPtr req_header_parser_empty = - HeaderParser::configure(headers_values, HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); + HeaderParser::configure(headers_values, HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); req_header_parser_empty->evaluateHeaders(header_map, nullptr); EXPECT_FALSE(header_map.has("empty")); } @@ -1655,17 +1655,16 @@ match: { prefix: "/new_endpoint" } key: "x-per-header" value: "per" append_action: ADD_IF_ABSENT -response_headers_to_remove: ["x-baz-header"] )EOF"; const auto route = parseRouteFromV3Yaml(yaml); - HeaderParserPtr req_header_parser = - HeaderParser::configure(route.request_headers_to_add(), route.request_headers_to_remove()); + HeaderParserPtr resp_header_parser = + HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); NiceMock stream_info; { Http::TestResponseHeaderMapImpl header_map; - req_header_parser->evaluateHeaders(header_map, stream_info); + resp_header_parser->evaluateHeaders(header_map, stream_info); EXPECT_EQ("foo", header_map.get_("x-foo-header")); EXPECT_EQ("bar", header_map.get_("x-bar-header")); EXPECT_EQ("per", header_map.get_("x-per-header")); @@ -1673,7 +1672,7 @@ response_headers_to_remove: ["x-baz-header"] { Http::TestResponseHeaderMapImpl header_map{{"x-foo-header", "exist-foo"}}; - req_header_parser->evaluateHeaders(header_map, stream_info); + resp_header_parser->evaluateHeaders(header_map, stream_info); EXPECT_EQ(2, header_map.get(Http::LowerCaseString("x-foo-header")).size()); EXPECT_EQ("bar", header_map.get_("x-bar-header")); EXPECT_EQ("per", header_map.get_("x-per-header")); @@ -1681,7 +1680,7 @@ response_headers_to_remove: ["x-baz-header"] { Http::TestResponseHeaderMapImpl header_map{{"x-bar-header", "exist-bar"}}; - req_header_parser->evaluateHeaders(header_map, stream_info); + resp_header_parser->evaluateHeaders(header_map, stream_info); EXPECT_EQ("foo", header_map.get_("x-foo-header")); EXPECT_EQ("bar", header_map.get_("x-bar-header")); EXPECT_EQ(1, header_map.get(Http::LowerCaseString("x-bar-header")).size()); @@ -1690,7 +1689,7 @@ response_headers_to_remove: ["x-baz-header"] { Http::TestResponseHeaderMapImpl header_map{{"x-per-header", "exist-per"}}; - req_header_parser->evaluateHeaders(header_map, stream_info); + resp_header_parser->evaluateHeaders(header_map, stream_info); EXPECT_EQ("foo", header_map.get_("x-foo-header")); EXPECT_EQ("bar", header_map.get_("x-bar-header")); EXPECT_EQ("exist-per", header_map.get_("x-per-header")); @@ -1714,27 +1713,27 @@ match: { prefix: "/new_endpoint" } )EOF"; const auto route = parseRouteFromV3Yaml(yaml); - HeaderParserPtr req_header_parser = - HeaderParser::configure(route.request_headers_to_add(), route.request_headers_to_remove()); + HeaderParserPtr resp_header_parser = + HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); NiceMock stream_info; { Http::TestResponseHeaderMapImpl header_map; - req_header_parser->evaluateHeaders(header_map, stream_info); + resp_header_parser->evaluateHeaders(header_map, stream_info); EXPECT_EQ("foo", header_map.get_("x-foo-header")); EXPECT_EQ("bar", header_map.get_("x-bar-header")); } { Http::TestResponseHeaderMapImpl header_map{{"x-foo-header", "exist-foo"}}; - req_header_parser->evaluateHeaders(header_map, stream_info); + resp_header_parser->evaluateHeaders(header_map, stream_info); EXPECT_EQ(2, header_map.get(Http::LowerCaseString("x-foo-header")).size()); EXPECT_EQ("bar", header_map.get_("x-bar-header")); } { Http::TestResponseHeaderMapImpl header_map{{"x-bar-header", "exist-bar"}}; - req_header_parser->evaluateHeaders(header_map, stream_info); + resp_header_parser->evaluateHeaders(header_map, stream_info); EXPECT_EQ("foo", header_map.get_("x-foo-header")); EXPECT_EQ("bar", header_map.get_("x-bar-header")); EXPECT_EQ(1, header_map.get(Http::LowerCaseString("x-bar-header")).size()); From b5aa2bd22762aa9beed874fe913df2ddec8ea4df Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 24 Aug 2022 02:26:40 +0000 Subject: [PATCH 06/19] fix test Signed-off-by: wbpcode --- source/common/grpc/google_async_client_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index 1a59b220b0b3..7722eca4af80 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -84,8 +84,8 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher, target_uri_(config.google_grpc().target_uri()), scope_(scope), per_stream_buffer_limit_bytes_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( config.google_grpc(), per_stream_buffer_limit_bytes, DefaultBufferLimitBytes)), - metadata_parser_( - Router::HeaderParser::configure(config.initial_metadata(), /*append=*/false)) { + metadata_parser_(Router::HeaderParser::configure( + config.initial_metadata(), Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) { // We rebuild the channel each time we construct the channel. It appears that the gRPC library is // smart enough to do connection pooling and reuse with identical channel args, so this should // have comparable overhead to what we are doing in Grpc::AsyncClientImpl, i.e. no expensive From b1e37d0e4a9b119d35e1f77fb34df10676526f8d Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 24 Aug 2022 02:57:15 +0000 Subject: [PATCH 07/19] fix test Signed-off-by: wbpcode --- .../extensions/filters/common/ext_authz/ext_authz_http_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index 203bc3e826b7..fb62e8a6b1f1 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -144,7 +144,8 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3 path_prefix_(path_prefix), tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())), request_headers_parser_(Router::HeaderParser::configure( - config.http_service().authorization_request().headers_to_add(), false)) {} + config.http_service().authorization_request().headers_to_add(), + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} MatcherSharedPtr ClientConfig::toRequestMatchers(const envoy::type::matcher::v3::ListStringMatcher& list) { From 8a14119b9f86b50717c0003bae4cd340da6733a4 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 24 Aug 2022 06:45:53 +0000 Subject: [PATCH 08/19] fix test again and again Signed-off-by: wbpcode --- test/common/router/config_impl_test.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index b9b648aa0d04..d169c84f9f5b 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -269,11 +269,11 @@ most_specific_header_mutations_wins: {0} std::string action_string; - if (action == HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD) { + if (action == HeaderValueOption::APPEND_IF_EXISTS_OR_ADD) { action_string = "APPEND_IF_EXISTS_OR_ADD"; - } else if (action == HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD) { + } else if (action == HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD) { action_string = "OVERWRITE_IF_EXISTS_OR_ADD"; - } else if (action == HeaderAppendOption::ADD_IF_ABSENT) { + } else if (action == HeaderValueOption::ADD_IF_ABSENT) { action_string = "ADD_IF_ABSENT"; } @@ -1726,7 +1726,7 @@ TEST_F(RouteMatcherTest, TestRequestHeadersToAddWithAppendFalseMostSpecificWins) // and route levels. TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/false, - HeaderAppendOption::APPEND_IF_EXISTS_OR_ADD); + HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); @@ -1820,7 +1820,7 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeaders) { TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersOverwriteIfExistOrAdd) { const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/false, - HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); @@ -1849,7 +1849,7 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersOverwriteIfExistOrAdd) { TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAddIfAbsent) { const std::string yaml = - responseHeadersConfig(/*most_specific_wins=*/false, HeaderAppendOption::ADD_IF_ABSENT); + responseHeadersConfig(/*most_specific_wins=*/false, HeaderValueOption::ADD_IF_ABSENT); NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); @@ -1859,8 +1859,8 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAddIfAbsent) { const RouteEntry* route = config.route(req_headers, 0)->routeEntry(); Http::TestResponseHeaderMapImpl headers{{":status", "200"}, {"x-route-header", "exist-value"}}; route->finalizeResponseHeaders(headers, stream_info); - EXPECT_EQ("global1", headers.get_("x-global-header1")); - EXPECT_EQ("vhost1-www2", headers.get_("x-vhost-header1")); + EXPECT_EQ("route-override", headers.get_("x-global-header1")); + EXPECT_EQ("route-override", headers.get_("x-vhost-header1")); // If related header is exist in the headers then do nothing. EXPECT_EQ("exist-value", headers.get_("x-route-header")); @@ -1880,7 +1880,7 @@ TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAddIfAbsent) { TEST_F(RouteMatcherTest, TestAddRemoveResponseHeadersAppendMostSpecificWins) { const std::string yaml = responseHeadersConfig(/*most_specific_wins=*/true, - HeaderAppendOption::OVERWRITE_IF_EXISTS_OR_ADD); + HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); From 497b8c18375ef2d0e2d5603b7626e8e9d5b7f166 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 24 Aug 2022 11:25:13 +0000 Subject: [PATCH 09/19] fix route check tool Signed-off-by: wbpcode --- test/tools/router_check/test/config/DirectResponse.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/tools/router_check/test/config/DirectResponse.yaml b/test/tools/router_check/test/config/DirectResponse.yaml index bf624eff3d6c..4b2af7d37002 100644 --- a/test/tools/router_check/test/config/DirectResponse.yaml +++ b/test/tools/router_check/test/config/DirectResponse.yaml @@ -19,7 +19,7 @@ virtual_hosts: - header: key: "content-type" value: "application/json" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - match: path: /ping-json2 direct_response: @@ -27,7 +27,7 @@ virtual_hosts: body: inline_string: "{ 'message': 'Success!' }" response_headers_to_add: - # This is bad, don't do it! Use append: false! + # This is bad, don't do it! Use append_action: OVERWRITE_IF_EXISTS_OR_ADD! - header: key: "content-type" value: "application/json" From d81d23bf37296defcf50322200f2abfe998a7949 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 24 Aug 2022 14:13:58 +0000 Subject: [PATCH 10/19] address comments Signed-off-by: wbpcode --- source/common/router/header_parser.h | 3 +- test/common/router/header_formatter_test.cc | 31 ++++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/source/common/router/header_parser.h b/source/common/router/header_parser.h index 4006822315ac..9071c351585f 100644 --- a/source/common/router/header_parser.h +++ b/source/common/router/header_parser.h @@ -36,7 +36,8 @@ class HeaderParser : public Http::HeaderEvaluator { /* * @param headers_to_add defines headers to add during calls to evaluateHeaders. - * @param append defines whether headers will be appended or replaced. + * @param append_action defines action taken to append/overwrite the given value for an existing + * header or to only add this header if it's absent. * @return HeaderParserPtr a configured HeaderParserPtr. */ static HeaderParserPtr diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 78bf165eafc2..e4b8925b6007 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -1658,8 +1658,7 @@ match: { prefix: "/new_endpoint" } )EOF"; const auto route = parseRouteFromV3Yaml(yaml); - HeaderParserPtr resp_header_parser = - HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); + HeaderParserPtr resp_header_parser = HeaderParser::configure(route.response_headers_to_add()); NiceMock stream_info; { @@ -1713,8 +1712,7 @@ match: { prefix: "/new_endpoint" } )EOF"; const auto route = parseRouteFromV3Yaml(yaml); - HeaderParserPtr resp_header_parser = - HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove()); + HeaderParserPtr resp_header_parser = HeaderParser::configure(route.response_headers_to_add()); NiceMock stream_info; { @@ -1740,6 +1738,31 @@ match: { prefix: "/new_endpoint" } } } +TEST(HeaderParserTest, + DEPRECATED_FEATURE_TEST(EvaluateRequestHeadersAddWithDeprecatedAppendAndAction)) { + const std::string yaml = R"EOF( +match: { prefix: "/new_endpoint" } +route: + cluster: www2 +response_headers_to_add: + - header: + key: "x-foo-header" + value: "foo" + append: true + append_action: OVERWRITE_IF_EXISTS_OR_ADD + - header: + key: "x-bar-header" + value: "bar" + append: false +)EOF"; + + const auto route = parseRouteFromV3Yaml(yaml); + + EXPECT_THROW_WITH_MESSAGE(HeaderParser::configure(route.response_headers_to_add()), + EnvoyException, + "Both append and append_action are set and it's not allowed"); +} + TEST(HeaderParserTest, EvaluateResponseHeadersRemoveBeforeAdd) { const std::string yaml = R"EOF( match: { prefix: "/new_endpoint" } From 9fb3315bd32b9c0ef454cffd0e6e43bbd29a4a81 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 02:15:46 +0000 Subject: [PATCH 11/19] update all append to append_action except deprecated test Signed-off-by: wbpcode --- .../http/http_conn_man/headers.rst | 2 +- .../http/http_conn_man/local_reply.rst | 2 +- ...local-rate-limit-global-configuration.yaml | 2 +- ...te-limit-route-specific-configuration.yaml | 2 +- .../local-rate-limit-with-descriptors.yaml | 2 +- examples/local_ratelimit/ratelimit-envoy.yaml | 4 +-- test/common/local_reply/local_reply_test.cc | 6 ++-- test/common/router/header_formatter_test.cc | 11 ++++--- test/common/tcp_proxy/upstream_test.cc | 10 +++---- .../upstream/health_checker_impl_test.cc | 5 ++-- .../ext_authz/ext_authz_integration_test.cc | 28 +++++++++-------- .../ext_proc/ext_proc_integration_test.cc | 3 +- .../filters/http/ext_proc/filter_test.cc | 30 +++++++++---------- .../http/ext_proc/mutation_utils_test.cc | 12 ++++---- .../http/local_ratelimit/config_test.cc | 8 ++--- .../http/local_ratelimit/filter_test.cc | 6 ++-- .../local_ratelimit_integration_test.cc | 2 +- test/integration/header_integration_test.cc | 2 +- test/integration/integration_test.cc | 18 +++++++---- .../local_reply_integration_test.cc | 6 ++-- 20 files changed, 87 insertions(+), 74 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/headers.rst b/docs/root/configuration/http/http_conn_man/headers.rst index 2976e093348a..a6c8089b4a04 100644 --- a/docs/root/configuration/http/http_conn_man/headers.rst +++ b/docs/root/configuration/http/http_conn_man/headers.rst @@ -830,7 +830,7 @@ Supported variable names are: - header: key: "x-request-start" value: "%START_TIME(%s.%3f)%" - append: true + append_action: APPEND_IF_EXISTS_OR_ADD %RESPONSE_FLAGS% Additional details about the response or connection, if any. Possible values and their meanings diff --git a/docs/root/configuration/http/http_conn_man/local_reply.rst b/docs/root/configuration/http/http_conn_man/local_reply.rst index 8811aa5ba241..6b6ea1efe0ac 100644 --- a/docs/root/configuration/http/http_conn_man/local_reply.rst +++ b/docs/root/configuration/http/http_conn_man/local_reply.rst @@ -33,7 +33,7 @@ Example of a LocalReplyConfig - header: key: "foo" value: "bar" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD status_code: 401 body: inline_string: "not allowed" diff --git a/docs/root/configuration/http/http_filters/_include/local-rate-limit-global-configuration.yaml b/docs/root/configuration/http/http_filters/_include/local-rate-limit-global-configuration.yaml index cc1ee642374e..69370957efac 100644 --- a/docs/root/configuration/http/http_filters/_include/local-rate-limit-global-configuration.yaml +++ b/docs/root/configuration/http/http_filters/_include/local-rate-limit-global-configuration.yaml @@ -30,7 +30,7 @@ static_resources: numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-local-rate-limit value: 'true' diff --git a/docs/root/configuration/http/http_filters/_include/local-rate-limit-route-specific-configuration.yaml b/docs/root/configuration/http/http_filters/_include/local-rate-limit-route-specific-configuration.yaml index f017c4f25d05..3f1293ebba51 100644 --- a/docs/root/configuration/http/http_filters/_include/local-rate-limit-route-specific-configuration.yaml +++ b/docs/root/configuration/http/http_filters/_include/local-rate-limit-route-specific-configuration.yaml @@ -45,7 +45,7 @@ static_resources: numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-local-rate-limit value: 'true' diff --git a/docs/root/configuration/http/http_filters/_include/local-rate-limit-with-descriptors.yaml b/docs/root/configuration/http/http_filters/_include/local-rate-limit-with-descriptors.yaml index 80a3cb734434..0bb198019cbc 100644 --- a/docs/root/configuration/http/http_filters/_include/local-rate-limit-with-descriptors.yaml +++ b/docs/root/configuration/http/http_filters/_include/local-rate-limit-with-descriptors.yaml @@ -54,7 +54,7 @@ static_resources: numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-test-rate-limit value: 'true' diff --git a/examples/local_ratelimit/ratelimit-envoy.yaml b/examples/local_ratelimit/ratelimit-envoy.yaml index 806fcdcf79f7..92cdde6ea62c 100644 --- a/examples/local_ratelimit/ratelimit-envoy.yaml +++ b/examples/local_ratelimit/ratelimit-envoy.yaml @@ -41,7 +41,7 @@ static_resources: numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-local-rate-limit value: 'true' @@ -90,7 +90,7 @@ static_resources: numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-local-rate-limit value: 'true' diff --git a/test/common/local_reply/local_reply_test.cc b/test/common/local_reply/local_reply_test.cc index 854eb24bcb98..34fa030a98af 100644 --- a/test/common/local_reply/local_reply_test.cc +++ b/test/common/local_reply/local_reply_test.cc @@ -359,15 +359,15 @@ TEST_F(LocalReplyTest, TestHeaderAddition) { - header: key: foo-1 value: bar1 - append: true + append_action: APPEND_IF_EXISTS_OR_ADD - header: key: foo-2 value: override-bar2 - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: foo-3 value: append-bar3 - append: true + append_action: APPEND_IF_EXISTS_OR_ADD )"; TestUtility::loadFromYaml(yaml, config_); auto local = Factory::create(config_, context_); diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index e4b8925b6007..b33efb96906c 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -762,7 +762,7 @@ TEST_F(StreamInfoHeaderFormatterTest, ValidateLimitsOnUserDefinedHeaders) { std::string long_string(16385, 'a'); header->mutable_header()->set_key("header_name"); header->mutable_header()->set_value(long_string); - header->mutable_append()->set_value(true); + header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); EXPECT_THROW_WITH_REGEX(TestUtility::validate(route), ProtoValidationException, "Proto constraint validation failed.*"); } @@ -1488,9 +1488,12 @@ match: { prefix: "/new_endpoint" } // Disable append mode. envoy::config::route::v3::Route route = parseRouteFromV3Yaml(yaml); - route.mutable_request_headers_to_add(0)->mutable_append()->set_value(false); - route.mutable_request_headers_to_add(1)->mutable_append()->set_value(false); - route.mutable_request_headers_to_add(2)->mutable_append()->set_value(false); + route.mutable_request_headers_to_add(0)->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + route.mutable_request_headers_to_add(1)->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + route.mutable_request_headers_to_add(2)->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); HeaderParserPtr req_header_parser = Router::HeaderParser::configure(route.request_headers_to_add()); diff --git a/test/common/tcp_proxy/upstream_test.cc b/test/common/tcp_proxy/upstream_test.cc index fa3e5f4a956a..5bc1af6d6f41 100644 --- a/test/common/tcp_proxy/upstream_test.cc +++ b/test/common/tcp_proxy/upstream_test.cc @@ -300,13 +300,13 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeaders) { hdr = header->mutable_header(); hdr->set_key("header1"); hdr->set_value("value1"); - header->mutable_append()->set_value(true); + header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); header = this->config_message_.add_headers_to_add(); hdr = header->mutable_header(); hdr->set_key("header1"); hdr->set_value("value2"); - header->mutable_append()->set_value(true); + header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; @@ -328,13 +328,13 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, ConfigReuse) { auto* hdr = header->mutable_header(); hdr->set_key("key"); hdr->set_value("value1"); - header->mutable_append()->set_value(true); + header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); header = this->config_message_.add_headers_to_add(); hdr = header->mutable_header(); hdr->set_key("key"); hdr->set_value("value2"); - header->mutable_append()->set_value(true); + header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; @@ -377,7 +377,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeadersWithDownstreamIn hdr = header->mutable_header(); hdr->set_key("downstream_local_port"); hdr->set_value("%DOWNSTREAM_LOCAL_PORT%"); - header->mutable_append()->set_value(true); + header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index aed6aad899d7..466ee81209b2 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -594,7 +594,7 @@ class HttpHealthCheckerImplTest : public Event::TestUsingSimulatedTime, - header: key: user-agent value: CoolEnvoy/HC - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: x-protocol value: "%PROTOCOL%" @@ -4577,7 +4577,8 @@ class GrpcHealthCheckerImplTestBase : public Event::TestUsingSimulatedTime, config.mutable_grpc_health_check()->set_service_name("service"); for (const auto& pair : headers_to_add) { auto header_value_option = config.mutable_grpc_health_check()->add_initial_metadata(); - header_value_option->mutable_append()->set_value(false); + header_value_option->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto header = header_value_option->mutable_header(); header->set_key(pair.first); header->set_value(pair.second); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 65752924572b..4cdddf5e1b09 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -273,14 +273,15 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, for (const auto& header_to_add : headers_to_add) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->mutable_append()->set_value(false); + entry->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(header_to_add.first); entry->mutable_header()->set_value(header_to_add.second); } for (const auto& header_to_append : headers_to_append) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->mutable_append()->set_value(true); + entry->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(header_to_append.first); entry->mutable_header()->set_value(header_to_append.second); } @@ -291,15 +292,15 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, } // Entries in this headers are not present in the original request headers. - new_headers_from_upstream.iterate( - [&check_response](const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { - auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - // Try to append to a non-existent field. - entry->mutable_append()->set_value(true); - entry->mutable_header()->set_key(std::string(h.key().getStringView())); - entry->mutable_header()->set_value(std::string(h.value().getStringView())); - return Http::HeaderMap::Iterate::Continue; - }); + new_headers_from_upstream.iterate([&check_response]( + const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); + // Try to append to a non-existent field. + entry->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->mutable_header()->set_key(std::string(h.key().getStringView())); + entry->mutable_header()->set_value(std::string(h.value().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); // Entries in this headers are not present in the original request headers. But we set append = // true and append = false. @@ -321,7 +322,7 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto key = std::string(response_header_to_add.first); const auto value = std::string(response_header_to_add.second); - entry->mutable_append()->set_value(true); + entry->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_add {}={}", key, value); @@ -333,7 +334,8 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto value = std::string(response_header_to_set.second); // Replaces the one sent by the upstream. - entry->mutable_append()->set_value(false); + entry->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_set {}={}", key, value); diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 11be1f54a626..ffea5216ac2d 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -603,7 +603,8 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseTwoStatuses) { auto* add3 = response_mutation->add_set_headers(); add3->mutable_header()->set_key(":status"); add3->mutable_header()->set_value("202"); - add3->mutable_append()->set_value(true); + add3->mutable_append_action()->set_value( + Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); return true; }); diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 9d3fc0c9866b..e79b47fc58e6 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -365,19 +365,19 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) { EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); - processRequestHeaders( - false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) { - auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); - auto add1 = headers_mut->add_set_headers(); - add1->mutable_header()->set_key("x-new-header"); - add1->mutable_header()->set_value("new"); - add1->mutable_append()->set_value(false); - auto add2 = headers_mut->add_set_headers(); - add2->mutable_header()->set_key("x-some-other-header"); - add2->mutable_header()->set_value("no"); - add2->mutable_append()->set_value(true); - *headers_mut->add_remove_headers() = "x-do-we-want-this"; - }); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, + HeadersResponse& header_resp) { + auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); + auto add1 = headers_mut->add_set_headers(); + add1->mutable_header()->set_key("x-new-header"); + add1->mutable_header()->set_value("new"); + add1->mutable_append_action()->set_value(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + auto add2 = headers_mut->add_set_headers(); + add2->mutable_header()->set_key("x-some-other-header"); + add2->mutable_header()->set_value("no"); + add2->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + *headers_mut->add_remove_headers() = "x-do-we-want-this"; + }); // We should now have changed the original header a bit Http::TestRequestHeaderMapImpl expected{{":path", "/"}, @@ -462,11 +462,11 @@ TEST_F(HttpFilterTest, PostAndRespondImmediately) { hdr1->mutable_header()->set_key("content-type"); hdr1->mutable_header()->set_value("text/plain"); auto* hdr2 = immediate_headers->add_set_headers(); - hdr2->mutable_append()->set_value(true); + hdr2->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); hdr2->mutable_header()->set_key("x-another-thing"); hdr2->mutable_header()->set_value("1"); auto* hdr3 = immediate_headers->add_set_headers(); - hdr3->mutable_append()->set_value(true); + hdr3->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); hdr3->mutable_header()->set_key("x-another-thing"); hdr3->mutable_header()->set_value("2"); stream_callbacks_->onReceiveMessage(std::move(resp1)); 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 26a63c91869a..fcacf8a5e0b8 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -61,15 +61,15 @@ TEST(MutationUtils, TestApplyMutations) { envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->mutable_append()->set_value(true); + s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("2"); s = mutation.add_set_headers(); - s->mutable_append()->set_value(true); + s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("3"); s = mutation.add_set_headers(); - s->mutable_append()->set_value(false); + s->mutable_append_action()->set_value(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-replace-this"); s->mutable_header()->set_value("no"); s = mutation.add_set_headers(); @@ -149,7 +149,7 @@ TEST(MutationUtils, TestNonAppendableHeaders) { Http::TestRequestHeaderMapImpl headers; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->mutable_append()->set_value(true); + s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/foo"); s = mutation.add_set_headers(); @@ -158,11 +158,11 @@ TEST(MutationUtils, TestNonAppendableHeaders) { // These two should be ignored since we ignore attempts // to set multiple values for system headers. s = mutation.add_set_headers(); - s->mutable_append()->set_value(true); + s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/baz"); s = mutation.add_set_headers(); - s->mutable_append()->set_value(true); + s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":status"); s->mutable_header()->set_value("401"); diff --git a/test/extensions/filters/http/local_ratelimit/config_test.cc b/test/extensions/filters/http/local_ratelimit/config_test.cc index d10e3963d998..e865566c0c77 100644 --- a/test/extensions/filters/http/local_ratelimit/config_test.cc +++ b/test/extensions/filters/http/local_ratelimit/config_test.cc @@ -47,7 +47,7 @@ stat_prefix: test numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-test-rate-limit value: 'true' @@ -143,7 +143,7 @@ stat_prefix: test numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-test-rate-limit value: 'true' @@ -188,7 +188,7 @@ stat_prefix: test numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-test-rate-limit value: 'true' @@ -242,7 +242,7 @@ stat_prefix: test numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-test-rate-limit value: 'true' diff --git a/test/extensions/filters/http/local_ratelimit/filter_test.cc b/test/extensions/filters/http/local_ratelimit/filter_test.cc index 28bd72c904f9..816bf45b4d10 100644 --- a/test/extensions/filters/http/local_ratelimit/filter_test.cc +++ b/test/extensions/filters/http/local_ratelimit/filter_test.cc @@ -31,12 +31,12 @@ stat_prefix: test numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-test-rate-limit value: 'true' request_headers_to_add_when_not_enforced: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-local-ratelimited value: 'true' @@ -273,7 +273,7 @@ stat_prefix: test numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-test-rate-limit value: 'true' diff --git a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc index 57ab031d3837..bb15ab5c7b74 100644 --- a/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc +++ b/test/extensions/filters/http/local_ratelimit/local_ratelimit_integration_test.cc @@ -41,7 +41,7 @@ name: envoy.filters.http.local_ratelimit numerator: 100 denominator: HUNDRED response_headers_to_add: - - append: false + - append_action: OVERWRITE_IF_EXISTS_OR_ADD header: key: x-local-rate-limit value: 'true' diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index 5a93eb3fb32a..516e3a74f514 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -35,7 +35,7 @@ std::string ipSuppressEnvoyHeadersTestParamsToString( void disableHeaderValueOptionAppend( Protobuf::RepeatedPtrField& header_value_options) { for (auto& i : header_value_options) { - i.mutable_append()->set_value(false); + i.mutable_append_action()->set_value(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); } } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 0b5ea3b99a69..58e409e13625 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -293,16 +293,19 @@ TEST_P(IntegrationTest, RouterDirectResponseWithBody) { auto* header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("x-additional-header"); header_value_option->mutable_header()->set_value("example-value"); - header_value_option->mutable_append()->set_value(false); + header_value_option->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-type"); header_value_option->mutable_header()->set_value("text/html"); - header_value_option->mutable_append()->set_value(false); + header_value_option->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); // Add a wrong content-length. header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-length"); header_value_option->mutable_header()->set_value("2000"); - header_value_option->mutable_append()->set_value(false); + header_value_option->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto* virtual_host = route_config->add_virtual_hosts(); virtual_host->set_name(domain); virtual_host->add_domains(domain); @@ -341,16 +344,19 @@ TEST_P(IntegrationTest, RouterDirectResponseEmptyBody) { auto* header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("x-additional-header"); header_value_option->mutable_header()->set_value("example-value"); - header_value_option->mutable_append()->set_value(false); + header_value_option->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-type"); header_value_option->mutable_header()->set_value("text/html"); - header_value_option->mutable_append()->set_value(false); + header_value_option->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); // Add a wrong content-length. header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-length"); header_value_option->mutable_header()->set_value("2000"); - header_value_option->mutable_append()->set_value(false); + header_value_option->mutable_append_action()->set_value( + Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto* virtual_host = route_config->add_virtual_hosts(); virtual_host->set_name(domain); virtual_host->add_domains(domain); diff --git a/test/integration/local_reply_integration_test.cc b/test/integration/local_reply_integration_test.cc index 04838bbf311b..1cd11fc863c9 100644 --- a/test/integration/local_reply_integration_test.cc +++ b/test/integration/local_reply_integration_test.cc @@ -33,11 +33,11 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJson) { - header: key: foo value: bar - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD - header: key: content-type value: "application/json-custom" - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD body_format: json_format: level: TRACE @@ -278,7 +278,7 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJsonForFirstMatchingFi - header: key: foo value: bar - append: false + append_action: OVERWRITE_IF_EXISTS_OR_ADD body: inline_string: "customized body text" body_format_override: From c31c504a64312335f91aeb09460524ba79e7bf55 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 04:12:43 +0000 Subject: [PATCH 12/19] fix fix fix set append action Signed-off-by: wbpcode --- test/common/router/header_formatter_test.cc | 8 ++--- test/common/tcp_proxy/upstream_test.cc | 10 +++---- .../upstream/health_checker_impl_test.cc | 3 +- .../ext_authz/ext_authz_integration_test.cc | 28 ++++++++--------- .../ext_proc/ext_proc_integration_test.cc | 3 +- .../filters/http/ext_proc/filter_test.cc | 30 +++++++++---------- .../http/ext_proc/mutation_utils_test.cc | 12 ++++---- test/integration/header_integration_test.cc | 2 +- test/integration/integration_test.cc | 12 ++++---- 9 files changed, 52 insertions(+), 56 deletions(-) diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index b33efb96906c..87656be69532 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -762,7 +762,7 @@ TEST_F(StreamInfoHeaderFormatterTest, ValidateLimitsOnUserDefinedHeaders) { std::string long_string(16385, 'a'); header->mutable_header()->set_key("header_name"); header->mutable_header()->set_value(long_string); - header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); EXPECT_THROW_WITH_REGEX(TestUtility::validate(route), ProtoValidationException, "Proto constraint validation failed.*"); } @@ -1488,11 +1488,11 @@ match: { prefix: "/new_endpoint" } // Disable append mode. envoy::config::route::v3::Route route = parseRouteFromV3Yaml(yaml); - route.mutable_request_headers_to_add(0)->mutable_append_action()->set_value( + route.mutable_request_headers_to_add(0)->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); - route.mutable_request_headers_to_add(1)->mutable_append_action()->set_value( + route.mutable_request_headers_to_add(1)->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); - route.mutable_request_headers_to_add(2)->mutable_append_action()->set_value( + route.mutable_request_headers_to_add(2)->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); HeaderParserPtr req_header_parser = diff --git a/test/common/tcp_proxy/upstream_test.cc b/test/common/tcp_proxy/upstream_test.cc index 5bc1af6d6f41..0bb4b7dea737 100644 --- a/test/common/tcp_proxy/upstream_test.cc +++ b/test/common/tcp_proxy/upstream_test.cc @@ -300,13 +300,13 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeaders) { hdr = header->mutable_header(); hdr->set_key("header1"); hdr->set_value("value1"); - header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); header = this->config_message_.add_headers_to_add(); hdr = header->mutable_header(); hdr->set_key("header1"); hdr->set_value("value2"); - header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; @@ -328,13 +328,13 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, ConfigReuse) { auto* hdr = header->mutable_header(); hdr->set_key("key"); hdr->set_value("value1"); - header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); header = this->config_message_.add_headers_to_add(); hdr = header->mutable_header(); hdr->set_key("key"); hdr->set_value("value2"); - header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; @@ -377,7 +377,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeadersWithDownstreamIn hdr = header->mutable_header(); hdr->set_key("downstream_local_port"); hdr->set_value("%DOWNSTREAM_LOCAL_PORT%"); - header->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 466ee81209b2..bfd90d4008ec 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -4577,8 +4577,7 @@ class GrpcHealthCheckerImplTestBase : public Event::TestUsingSimulatedTime, config.mutable_grpc_health_check()->set_service_name("service"); for (const auto& pair : headers_to_add) { auto header_value_option = config.mutable_grpc_health_check()->add_initial_metadata(); - header_value_option->mutable_append_action()->set_value( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + header_value_option->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto header = header_value_option->mutable_header(); header->set_key(pair.first); header->set_value(pair.second); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 4cdddf5e1b09..5da807f9b99d 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -273,15 +273,14 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, for (const auto& header_to_add : headers_to_add) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->mutable_append_action()->set_value( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + entry->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(header_to_add.first); entry->mutable_header()->set_value(header_to_add.second); } for (const auto& header_to_append : headers_to_append) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(header_to_append.first); entry->mutable_header()->set_value(header_to_append.second); } @@ -292,15 +291,15 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, } // Entries in this headers are not present in the original request headers. - new_headers_from_upstream.iterate([&check_response]( - const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { - auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - // Try to append to a non-existent field. - entry->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); - entry->mutable_header()->set_key(std::string(h.key().getStringView())); - entry->mutable_header()->set_value(std::string(h.value().getStringView())); - return Http::HeaderMap::Iterate::Continue; - }); + new_headers_from_upstream.iterate( + [&check_response](const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); + // Try to append to a non-existent field. + entry->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->mutable_header()->set_key(std::string(h.key().getStringView())); + entry->mutable_header()->set_value(std::string(h.value().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); // Entries in this headers are not present in the original request headers. But we set append = // true and append = false. @@ -322,7 +321,7 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto key = std::string(response_header_to_add.first); const auto value = std::string(response_header_to_add.second); - entry->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_add {}={}", key, value); @@ -334,8 +333,7 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto value = std::string(response_header_to_set.second); // Replaces the one sent by the upstream. - entry->mutable_append_action()->set_value( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + entry->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_set {}={}", key, value); diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index ffea5216ac2d..972fb26cabb5 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -603,8 +603,7 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseTwoStatuses) { auto* add3 = response_mutation->add_set_headers(); add3->mutable_header()->set_key(":status"); add3->mutable_header()->set_value("202"); - add3->mutable_append_action()->set_value( - Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + add3->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); return true; }); diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index e79b47fc58e6..9080e905969f 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -365,19 +365,19 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) { EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); - processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, - HeadersResponse& header_resp) { - auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); - auto add1 = headers_mut->add_set_headers(); - add1->mutable_header()->set_key("x-new-header"); - add1->mutable_header()->set_value("new"); - add1->mutable_append_action()->set_value(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); - auto add2 = headers_mut->add_set_headers(); - add2->mutable_header()->set_key("x-some-other-header"); - add2->mutable_header()->set_value("no"); - add2->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); - *headers_mut->add_remove_headers() = "x-do-we-want-this"; - }); + processRequestHeaders( + false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) { + auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); + auto add1 = headers_mut->add_set_headers(); + add1->mutable_header()->set_key("x-new-header"); + add1->mutable_header()->set_value("new"); + add1->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + auto add2 = headers_mut->add_set_headers(); + add2->mutable_header()->set_key("x-some-other-header"); + add2->mutable_header()->set_value("no"); + add2->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + *headers_mut->add_remove_headers() = "x-do-we-want-this"; + }); // We should now have changed the original header a bit Http::TestRequestHeaderMapImpl expected{{":path", "/"}, @@ -462,11 +462,11 @@ TEST_F(HttpFilterTest, PostAndRespondImmediately) { hdr1->mutable_header()->set_key("content-type"); hdr1->mutable_header()->set_value("text/plain"); auto* hdr2 = immediate_headers->add_set_headers(); - hdr2->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + hdr2->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); hdr2->mutable_header()->set_key("x-another-thing"); hdr2->mutable_header()->set_value("1"); auto* hdr3 = immediate_headers->add_set_headers(); - hdr3->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + hdr3->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); hdr3->mutable_header()->set_key("x-another-thing"); hdr3->mutable_header()->set_value("2"); stream_callbacks_->onReceiveMessage(std::move(resp1)); 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 fcacf8a5e0b8..23e2f95e4f2f 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -61,15 +61,15 @@ TEST(MutationUtils, TestApplyMutations) { envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("2"); s = mutation.add_set_headers(); - s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("3"); s = mutation.add_set_headers(); - s->mutable_append_action()->set_value(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + s->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-replace-this"); s->mutable_header()->set_value("no"); s = mutation.add_set_headers(); @@ -149,7 +149,7 @@ TEST(MutationUtils, TestNonAppendableHeaders) { Http::TestRequestHeaderMapImpl headers; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/foo"); s = mutation.add_set_headers(); @@ -158,11 +158,11 @@ TEST(MutationUtils, TestNonAppendableHeaders) { // These two should be ignored since we ignore attempts // to set multiple values for system headers. s = mutation.add_set_headers(); - s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/baz"); s = mutation.add_set_headers(); - s->mutable_append_action()->set_value(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":status"); s->mutable_header()->set_value("401"); diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index 516e3a74f514..16ae41d8f57d 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -35,7 +35,7 @@ std::string ipSuppressEnvoyHeadersTestParamsToString( void disableHeaderValueOptionAppend( Protobuf::RepeatedPtrField& header_value_options) { for (auto& i : header_value_options) { - i.mutable_append_action()->set_value(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + i.set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); } } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 58e409e13625..e2358caf5b91 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -293,18 +293,18 @@ TEST_P(IntegrationTest, RouterDirectResponseWithBody) { auto* header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("x-additional-header"); header_value_option->mutable_header()->set_value("example-value"); - header_value_option->mutable_append_action()->set_value( + header_value_option->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-type"); header_value_option->mutable_header()->set_value("text/html"); - header_value_option->mutable_append_action()->set_value( + header_value_option->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); // Add a wrong content-length. header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-length"); header_value_option->mutable_header()->set_value("2000"); - header_value_option->mutable_append_action()->set_value( + header_value_option->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto* virtual_host = route_config->add_virtual_hosts(); virtual_host->set_name(domain); @@ -344,18 +344,18 @@ TEST_P(IntegrationTest, RouterDirectResponseEmptyBody) { auto* header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("x-additional-header"); header_value_option->mutable_header()->set_value("example-value"); - header_value_option->mutable_append_action()->set_value( + header_value_option->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-type"); header_value_option->mutable_header()->set_value("text/html"); - header_value_option->mutable_append_action()->set_value( + header_value_option->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); // Add a wrong content-length. header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-length"); header_value_option->mutable_header()->set_value("2000"); - header_value_option->mutable_append_action()->set_value( + header_value_option->set_append_action( Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto* virtual_host = route_config->add_virtual_hosts(); virtual_host->set_name(domain); From 6e7c975722c145a18534b30886be5b5678553733 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 06:52:04 +0000 Subject: [PATCH 13/19] update some name Signed-off-by: wbpcode --- source/common/grpc/async_client_impl.cc | 3 +- .../common/grpc/google_async_client_impl.cc | 3 +- .../common/ext_authz/ext_authz_http_impl.cc | 2 +- test/common/router/header_formatter_test.cc | 8 ++--- test/common/tcp_proxy/upstream_test.cc | 10 +++---- .../upstream/health_checker_impl_test.cc | 3 +- .../ext_authz/ext_authz_integration_test.cc | 28 +++++++++-------- .../ext_proc/ext_proc_integration_test.cc | 3 +- .../filters/http/ext_proc/filter_test.cc | 30 +++++++++---------- .../http/ext_proc/mutation_utils_test.cc | 12 ++++---- test/integration/header_integration_test.cc | 2 +- test/integration/integration_test.cc | 12 ++++---- 12 files changed, 61 insertions(+), 55 deletions(-) diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index 086019beea3b..ed1fe61e8f1d 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -20,7 +20,8 @@ AsyncClientImpl::AsyncClientImpl(Upstream::ClusterManager& cm, : cm_(cm), remote_cluster_name_(config.envoy_grpc().cluster_name()), host_name_(config.envoy_grpc().authority()), time_source_(time_source), metadata_parser_(Router::HeaderParser::configure( - config.initial_metadata(), Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} + config.initial_metadata(), + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} AsyncClientImpl::~AsyncClientImpl() { ASSERT(isThreadSafe()); diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index 7722eca4af80..180b786608a8 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -85,7 +85,8 @@ GoogleAsyncClientImpl::GoogleAsyncClientImpl(Event::Dispatcher& dispatcher, per_stream_buffer_limit_bytes_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( config.google_grpc(), per_stream_buffer_limit_bytes, DefaultBufferLimitBytes)), metadata_parser_(Router::HeaderParser::configure( - config.initial_metadata(), Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) { + config.initial_metadata(), + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) { // We rebuild the channel each time we construct the channel. It appears that the gRPC library is // smart enough to do connection pooling and reuse with identical channel args, so this should // have comparable overhead to what we are doing in Grpc::AsyncClientImpl, i.e. no expensive diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index fb62e8a6b1f1..3157136b433e 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -145,7 +145,7 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3 tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())), request_headers_parser_(Router::HeaderParser::configure( config.http_service().authorization_request().headers_to_add(), - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} MatcherSharedPtr ClientConfig::toRequestMatchers(const envoy::type::matcher::v3::ListStringMatcher& list) { diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 87656be69532..72b00c9b0e94 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -762,7 +762,7 @@ TEST_F(StreamInfoHeaderFormatterTest, ValidateLimitsOnUserDefinedHeaders) { std::string long_string(16385, 'a'); header->mutable_header()->set_key("header_name"); header->mutable_header()->set_value(long_string); - header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); EXPECT_THROW_WITH_REGEX(TestUtility::validate(route), ProtoValidationException, "Proto constraint validation failed.*"); } @@ -1489,11 +1489,11 @@ match: { prefix: "/new_endpoint" } // Disable append mode. envoy::config::route::v3::Route route = parseRouteFromV3Yaml(yaml); route.mutable_request_headers_to_add(0)->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); route.mutable_request_headers_to_add(1)->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); route.mutable_request_headers_to_add(2)->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); HeaderParserPtr req_header_parser = Router::HeaderParser::configure(route.request_headers_to_add()); diff --git a/test/common/tcp_proxy/upstream_test.cc b/test/common/tcp_proxy/upstream_test.cc index 0bb4b7dea737..4190ccc2f57f 100644 --- a/test/common/tcp_proxy/upstream_test.cc +++ b/test/common/tcp_proxy/upstream_test.cc @@ -300,13 +300,13 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeaders) { hdr = header->mutable_header(); hdr->set_key("header1"); hdr->set_value("value1"); - header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); header = this->config_message_.add_headers_to_add(); hdr = header->mutable_header(); hdr->set_key("header1"); hdr->set_value("value2"); - header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; @@ -328,13 +328,13 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, ConfigReuse) { auto* hdr = header->mutable_header(); hdr->set_key("key"); hdr->set_value("value1"); - header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); header = this->config_message_.add_headers_to_add(); hdr = header->mutable_header(); hdr->set_key("key"); hdr->set_value("value2"); - header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; @@ -377,7 +377,7 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeadersWithDownstreamIn hdr = header->mutable_header(); hdr->set_key("downstream_local_port"); hdr->set_value("%DOWNSTREAM_LOCAL_PORT%"); - header->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + header->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); this->setupUpstream(); std::unique_ptr expected_headers; diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index bfd90d4008ec..8d4cdd20103b 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -4577,7 +4577,8 @@ class GrpcHealthCheckerImplTestBase : public Event::TestUsingSimulatedTime, config.mutable_grpc_health_check()->set_service_name("service"); for (const auto& pair : headers_to_add) { auto header_value_option = config.mutable_grpc_health_check()->add_initial_metadata(); - header_value_option->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + header_value_option->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto header = header_value_option->mutable_header(); header->set_key(pair.first); header->set_value(pair.second); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 5da807f9b99d..42024da7b234 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -273,14 +273,15 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, for (const auto& header_to_add : headers_to_add) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + entry->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(header_to_add.first); entry->mutable_header()->set_value(header_to_add.second); } for (const auto& header_to_append : headers_to_append) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(header_to_append.first); entry->mutable_header()->set_value(header_to_append.second); } @@ -291,15 +292,15 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, } // Entries in this headers are not present in the original request headers. - new_headers_from_upstream.iterate( - [&check_response](const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { - auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - // Try to append to a non-existent field. - entry->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); - entry->mutable_header()->set_key(std::string(h.key().getStringView())); - entry->mutable_header()->set_value(std::string(h.value().getStringView())); - return Http::HeaderMap::Iterate::Continue; - }); + new_headers_from_upstream.iterate([&check_response]( + const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); + // Try to append to a non-existent field. + entry->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->mutable_header()->set_key(std::string(h.key().getStringView())); + entry->mutable_header()->set_value(std::string(h.value().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); // Entries in this headers are not present in the original request headers. But we set append = // true and append = false. @@ -321,7 +322,7 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto key = std::string(response_header_to_add.first); const auto value = std::string(response_header_to_add.second); - entry->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_add {}={}", key, value); @@ -333,7 +334,8 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto value = std::string(response_header_to_set.second); // Replaces the one sent by the upstream. - entry->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + entry->set_append_action( + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_set {}={}", key, value); diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 972fb26cabb5..c0bc3d5e9619 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -603,7 +603,8 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseTwoStatuses) { auto* add3 = response_mutation->add_set_headers(); add3->mutable_header()->set_key(":status"); add3->mutable_header()->set_value("202"); - add3->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + add3->set_append_action( + envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); return true; }); diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 9080e905969f..be41b3112e94 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -365,19 +365,19 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) { EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); - processRequestHeaders( - false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) { - auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); - auto add1 = headers_mut->add_set_headers(); - add1->mutable_header()->set_key("x-new-header"); - add1->mutable_header()->set_value("new"); - add1->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); - auto add2 = headers_mut->add_set_headers(); - add2->mutable_header()->set_key("x-some-other-header"); - add2->mutable_header()->set_value("no"); - add2->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); - *headers_mut->add_remove_headers() = "x-do-we-want-this"; - }); + processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, + HeadersResponse& header_resp) { + auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); + auto add1 = headers_mut->add_set_headers(); + add1->mutable_header()->set_key("x-new-header"); + add1->mutable_header()->set_value("new"); + add1->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + auto add2 = headers_mut->add_set_headers(); + add2->mutable_header()->set_key("x-some-other-header"); + add2->mutable_header()->set_value("no"); + add2->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + *headers_mut->add_remove_headers() = "x-do-we-want-this"; + }); // We should now have changed the original header a bit Http::TestRequestHeaderMapImpl expected{{":path", "/"}, @@ -462,11 +462,11 @@ TEST_F(HttpFilterTest, PostAndRespondImmediately) { hdr1->mutable_header()->set_key("content-type"); hdr1->mutable_header()->set_value("text/plain"); auto* hdr2 = immediate_headers->add_set_headers(); - hdr2->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + hdr2->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); hdr2->mutable_header()->set_key("x-another-thing"); hdr2->mutable_header()->set_value("1"); auto* hdr3 = immediate_headers->add_set_headers(); - hdr3->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + hdr3->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); hdr3->mutable_header()->set_key("x-another-thing"); hdr3->mutable_header()->set_value("2"); stream_callbacks_->onReceiveMessage(std::move(resp1)); 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 23e2f95e4f2f..c0fe6ae76d80 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -61,15 +61,15 @@ TEST(MutationUtils, TestApplyMutations) { envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("2"); s = mutation.add_set_headers(); - s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("3"); s = mutation.add_set_headers(); - s->set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); s->mutable_header()->set_key("x-replace-this"); s->mutable_header()->set_value("no"); s = mutation.add_set_headers(); @@ -149,7 +149,7 @@ TEST(MutationUtils, TestNonAppendableHeaders) { Http::TestRequestHeaderMapImpl headers; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/foo"); s = mutation.add_set_headers(); @@ -158,11 +158,11 @@ TEST(MutationUtils, TestNonAppendableHeaders) { // These two should be ignored since we ignore attempts // to set multiple values for system headers. s = mutation.add_set_headers(); - s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/baz"); s = mutation.add_set_headers(); - s->set_append_action(Router::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); s->mutable_header()->set_key(":status"); s->mutable_header()->set_value("401"); diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index 16ae41d8f57d..d6d351a42a88 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -35,7 +35,7 @@ std::string ipSuppressEnvoyHeadersTestParamsToString( void disableHeaderValueOptionAppend( Protobuf::RepeatedPtrField& header_value_options) { for (auto& i : header_value_options) { - i.set_append_action(Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + i.set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); } } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index e2358caf5b91..f187f381d6f1 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -294,18 +294,18 @@ TEST_P(IntegrationTest, RouterDirectResponseWithBody) { header_value_option->mutable_header()->set_key("x-additional-header"); header_value_option->mutable_header()->set_value("example-value"); header_value_option->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-type"); header_value_option->mutable_header()->set_value("text/html"); header_value_option->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); // Add a wrong content-length. header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-length"); header_value_option->mutable_header()->set_value("2000"); header_value_option->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto* virtual_host = route_config->add_virtual_hosts(); virtual_host->set_name(domain); virtual_host->add_domains(domain); @@ -345,18 +345,18 @@ TEST_P(IntegrationTest, RouterDirectResponseEmptyBody) { header_value_option->mutable_header()->set_key("x-additional-header"); header_value_option->mutable_header()->set_value("example-value"); header_value_option->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-type"); header_value_option->mutable_header()->set_value("text/html"); header_value_option->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); // Add a wrong content-length. header_value_option = route_config->mutable_response_headers_to_add()->Add(); header_value_option->mutable_header()->set_key("content-length"); header_value_option->mutable_header()->set_value("2000"); header_value_option->set_append_action( - Router::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); auto* virtual_host = route_config->add_virtual_hosts(); virtual_host->set_name(domain); virtual_host->add_domains(domain); From 466173eac37405d0e6068df510df36c8307a5b84 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 06:54:20 +0000 Subject: [PATCH 14/19] update enum Signed-off-by: wbpcode --- source/common/router/header_parser.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/router/header_parser.cc b/source/common/router/header_parser.cc index de2408261883..cc96c94ff35a 100644 --- a/source/common/router/header_parser.cc +++ b/source/common/router/header_parser.cc @@ -297,6 +297,7 @@ void HeaderParser::evaluateHeaders(Http::HeaderMap& headers, stream_info != nullptr ? entry.formatter_->format(*stream_info) : entry.original_value_; if (!value.empty() || entry.add_if_empty_) { switch (entry.append_action_) { + PANIC_ON_PROTO_ENUM_SENTINEL_VALUES; case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD: headers.addReferenceKey(key, value); break; @@ -308,8 +309,6 @@ void HeaderParser::evaluateHeaders(Http::HeaderMap& headers, case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD: headers.setReferenceKey(key, value); break; - default: - PANIC_DUE_TO_CORRUPT_ENUM; } } } From 7f71262ec98c38739688d2a7a8577df390194fac Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 12:37:23 +0000 Subject: [PATCH 15/19] revert change of ext authz and ext proc Signed-off-by: wbpcode --- .../ext_authz/ext_authz_integration_test.cc | 28 ++++++++--------- .../ext_proc/ext_proc_integration_test.cc | 3 +- .../filters/http/ext_proc/filter_test.cc | 30 +++++++++---------- .../http/ext_proc/mutation_utils_test.cc | 12 ++++---- 4 files changed, 35 insertions(+), 38 deletions(-) diff --git a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc index 42024da7b234..65752924572b 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_integration_test.cc @@ -273,15 +273,14 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, for (const auto& header_to_add : headers_to_add) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->set_append_action( - envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + entry->mutable_append()->set_value(false); entry->mutable_header()->set_key(header_to_add.first); entry->mutable_header()->set_value(header_to_add.second); } for (const auto& header_to_append : headers_to_append) { auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - entry->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->mutable_append()->set_value(true); entry->mutable_header()->set_key(header_to_append.first); entry->mutable_header()->set_value(header_to_append.second); } @@ -292,15 +291,15 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, } // Entries in this headers are not present in the original request headers. - new_headers_from_upstream.iterate([&check_response]( - const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { - auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); - // Try to append to a non-existent field. - entry->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); - entry->mutable_header()->set_key(std::string(h.key().getStringView())); - entry->mutable_header()->set_value(std::string(h.value().getStringView())); - return Http::HeaderMap::Iterate::Continue; - }); + new_headers_from_upstream.iterate( + [&check_response](const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); + // Try to append to a non-existent field. + entry->mutable_append()->set_value(true); + entry->mutable_header()->set_key(std::string(h.key().getStringView())); + entry->mutable_header()->set_value(std::string(h.value().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); // Entries in this headers are not present in the original request headers. But we set append = // true and append = false. @@ -322,7 +321,7 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto key = std::string(response_header_to_add.first); const auto value = std::string(response_header_to_add.second); - entry->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + entry->mutable_append()->set_value(true); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_add {}={}", key, value); @@ -334,8 +333,7 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, const auto value = std::string(response_header_to_set.second); // Replaces the one sent by the upstream. - entry->set_append_action( - envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + entry->mutable_append()->set_value(false); entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); ENVOY_LOG_MISC(trace, "sendExtAuthzResponse: set response_header_to_set {}={}", key, value); diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index c0bc3d5e9619..11be1f54a626 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -603,8 +603,7 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseTwoStatuses) { auto* add3 = response_mutation->add_set_headers(); add3->mutable_header()->set_key(":status"); add3->mutable_header()->set_value("202"); - add3->set_append_action( - envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + add3->mutable_append()->set_value(true); return true; }); diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index be41b3112e94..9d3fc0c9866b 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -365,19 +365,19 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) { EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); - processRequestHeaders(false, [](const HttpHeaders&, ProcessingResponse&, - HeadersResponse& header_resp) { - auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); - auto add1 = headers_mut->add_set_headers(); - add1->mutable_header()->set_key("x-new-header"); - add1->mutable_header()->set_value("new"); - add1->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); - auto add2 = headers_mut->add_set_headers(); - add2->mutable_header()->set_key("x-some-other-header"); - add2->mutable_header()->set_value("no"); - add2->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); - *headers_mut->add_remove_headers() = "x-do-we-want-this"; - }); + processRequestHeaders( + false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) { + auto headers_mut = header_resp.mutable_response()->mutable_header_mutation(); + auto add1 = headers_mut->add_set_headers(); + add1->mutable_header()->set_key("x-new-header"); + add1->mutable_header()->set_value("new"); + add1->mutable_append()->set_value(false); + auto add2 = headers_mut->add_set_headers(); + add2->mutable_header()->set_key("x-some-other-header"); + add2->mutable_header()->set_value("no"); + add2->mutable_append()->set_value(true); + *headers_mut->add_remove_headers() = "x-do-we-want-this"; + }); // We should now have changed the original header a bit Http::TestRequestHeaderMapImpl expected{{":path", "/"}, @@ -462,11 +462,11 @@ TEST_F(HttpFilterTest, PostAndRespondImmediately) { hdr1->mutable_header()->set_key("content-type"); hdr1->mutable_header()->set_value("text/plain"); auto* hdr2 = immediate_headers->add_set_headers(); - hdr2->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + hdr2->mutable_append()->set_value(true); hdr2->mutable_header()->set_key("x-another-thing"); hdr2->mutable_header()->set_value("1"); auto* hdr3 = immediate_headers->add_set_headers(); - hdr3->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + hdr3->mutable_append()->set_value(true); hdr3->mutable_header()->set_key("x-another-thing"); hdr3->mutable_header()->set_value("2"); stream_callbacks_->onReceiveMessage(std::move(resp1)); 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 c0fe6ae76d80..26a63c91869a 100644 --- a/test/extensions/filters/http/ext_proc/mutation_utils_test.cc +++ b/test/extensions/filters/http/ext_proc/mutation_utils_test.cc @@ -61,15 +61,15 @@ TEST(MutationUtils, TestApplyMutations) { envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_append()->set_value(true); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("2"); s = mutation.add_set_headers(); - s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_append()->set_value(true); s->mutable_header()->set_key("x-append-this"); s->mutable_header()->set_value("3"); s = mutation.add_set_headers(); - s->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); + s->mutable_append()->set_value(false); s->mutable_header()->set_key("x-replace-this"); s->mutable_header()->set_value("no"); s = mutation.add_set_headers(); @@ -149,7 +149,7 @@ TEST(MutationUtils, TestNonAppendableHeaders) { Http::TestRequestHeaderMapImpl headers; envoy::service::ext_proc::v3::HeaderMutation mutation; auto* s = mutation.add_set_headers(); - s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_append()->set_value(true); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/foo"); s = mutation.add_set_headers(); @@ -158,11 +158,11 @@ TEST(MutationUtils, TestNonAppendableHeaders) { // These two should be ignored since we ignore attempts // to set multiple values for system headers. s = mutation.add_set_headers(); - s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_append()->set_value(true); s->mutable_header()->set_key(":path"); s->mutable_header()->set_value("/baz"); s = mutation.add_set_headers(); - s->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD); + s->mutable_append()->set_value(true); s->mutable_header()->set_key(":status"); s->mutable_header()->set_value("401"); From b8e6967c1cc66863ab7829cda16b1a742d8e6beb Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 12:38:46 +0000 Subject: [PATCH 16/19] revert change of ext authz and ext proc Signed-off-by: wbpcode --- .../extensions/filters/common/ext_authz/ext_authz_http_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index 3157136b433e..203bc3e826b7 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -144,8 +144,7 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3 path_prefix_(path_prefix), tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())), request_headers_parser_(Router::HeaderParser::configure( - config.http_service().authorization_request().headers_to_add(), - envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} + config.http_service().authorization_request().headers_to_add(), false)) {} MatcherSharedPtr ClientConfig::toRequestMatchers(const envoy::type::matcher::v3::ListStringMatcher& list) { From e9dc557af61dbd6c38bc2d91441acd6e0648310b Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 13:37:05 +0000 Subject: [PATCH 17/19] Revert "revert change of ext authz and ext proc" This reverts commit b8e6967c1cc66863ab7829cda16b1a742d8e6beb. --- .../extensions/filters/common/ext_authz/ext_authz_http_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc index 203bc3e826b7..3157136b433e 100644 --- a/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc +++ b/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc @@ -144,7 +144,8 @@ ClientConfig::ClientConfig(const envoy::extensions::filters::http::ext_authz::v3 path_prefix_(path_prefix), tracing_name_(fmt::format("async {} egress", config.http_service().server_uri().cluster())), request_headers_parser_(Router::HeaderParser::configure( - config.http_service().authorization_request().headers_to_add(), false)) {} + config.http_service().authorization_request().headers_to_add(), + envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD)) {} MatcherSharedPtr ClientConfig::toRequestMatchers(const envoy::type::matcher::v3::ListStringMatcher& list) { From 6ba47a32bec9c804d66d5e8b25233f8f96fe0f2a Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 25 Aug 2022 15:11:01 +0000 Subject: [PATCH 18/19] fix header integration test Signed-off-by: wbpcode --- test/integration/header_integration_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index d6d351a42a88..9a31a706a8ed 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -206,7 +206,9 @@ class HeaderIntegrationTest auto* mutable_header = header_value_option->mutable_header(); mutable_header->set_key(key); mutable_header->set_value(value); - header_value_option->mutable_append()->set_value(append); + header_value_option->set_append_action( + append ? envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD + : envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD); } void prepareEDS() { From 41a69e1fdd7324f5a9beb4e57a7c52fc1e9e8cc5 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Fri, 26 Aug 2022 12:39:51 +0000 Subject: [PATCH 19/19] add release note Signed-off-by: wbpcode --- changelogs/current.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index f4cf64c98bdd..2549181bc4d0 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -124,3 +124,7 @@ new_features: allow network filters other than HTTP Connection Manager to be created for QUIC listeners. deprecated: +- area: http + change: | + deprecated :ref:`append ` and please use + :ref:`append_action ` first.