From 3e63d74b7e3b0ece88622d16b18135002ec7289b Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Tue, 14 Jul 2020 17:36:44 -0400 Subject: [PATCH 1/7] Make HeaderMap::ConstIterateCb a std::function The existing cast-to-void*-then-back method for passing in a context is type-unsafe and can make for hard-to-locate errors. By using a std::function instead of a function pointer, the caller can get compilation error instead of runtime errors, and doesn't have to do any sort of bundling dance with std::pair or a custom struct to pass multiple context items in. Signed-off-by: Alex Konradi --- include/envoy/http/header_map.h | 11 +- .../common/grpc/google_async_client_impl.cc | 13 +- source/common/http/header_list_view.cc | 14 +- source/common/http/header_map_impl.cc | 60 +++---- source/common/http/header_map_impl.h | 12 +- source/common/http/header_utility.cc | 37 ++-- source/common/http/http1/codec_impl.cc | 44 +++-- source/common/http/http2/codec_impl.cc | 11 +- source/common/http/utility.cc | 73 ++++---- source/extensions/common/aws/utility.cc | 59 +++---- .../common/ext_authz/check_request_utils.cc | 21 +-- .../common/ext_authz/ext_authz_http_impl.cc | 41 +++-- .../http/aws_lambda/aws_lambda_filter.cc | 33 ++-- .../filters/http/grpc_web/grpc_web_filter.cc | 17 +- .../extensions/filters/http/lua/lua_filter.cc | 20 +-- .../extensions/filters/http/lua/wrappers.cc | 11 +- .../filters/http/tap/tap_config_impl.cc | 45 +++-- .../thrift_proxy/header_transport_impl.cc | 13 +- .../thrift_proxy/twitter_protocol_impl.cc | 57 +++---- .../grpc_credentials/aws_iam/config.cc | 21 +-- .../quic_listeners/quiche/envoy_quic_utils.cc | 13 +- .../common/ot/opentracing_driver_impl.cc | 27 +-- test/common/http/header_map_impl_fuzz_test.cc | 24 ++- .../common/http/header_map_impl_speed_test.cc | 6 +- test/common/http/header_map_impl_test.cc | 160 +++++------------- test/common/http/header_utility_test.cc | 13 +- test/common/router/header_formatter_test.cc | 23 ++- test/common/router/router_test.cc | 15 +- .../aws_lambda_filter_integration_test.cc | 24 +-- .../http/aws_lambda/aws_lambda_filter_test.cc | 32 ++-- .../ext_authz/ext_authz_integration_test.cc | 20 +-- .../grpc_json_transcoder_integration_test.cc | 6 +- .../ratelimit/ratelimit_integration_test.cc | 44 ++--- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 7 +- test/fuzz/utility.h | 14 +- test/integration/http_integration.cc | 18 +- test/mocks/http/mocks.h | 40 ++--- test/test_common/printers.cc | 12 +- test/test_common/utility.cc | 32 ++-- test/test_common/utility.h | 8 +- 40 files changed, 459 insertions(+), 692 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 82a79d4fa6e55..90466744ce426 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -519,24 +519,21 @@ class HeaderMap { /** * Callback when calling iterate() over a const header map. * @param header supplies the header entry. - * @param context supplies the context passed to iterate(). - * @return Iterate::Continue to continue iteration. + * @return Iterate::Continue to continue iteration, or Iterate::Break to stop; */ - using ConstIterateCb = Iterate (*)(const HeaderEntry&, void*); + using ConstIterateCb = std::function; /** * Iterate over a constant header map. * @param cb supplies the iteration callback. - * @param context supplies the context that will be passed to the callback. */ - virtual void iterate(ConstIterateCb cb, void* context) const PURE; + virtual void iterate(ConstIterateCb cb) const PURE; /** * Iterate over a constant header map in reverse order. * @param cb supplies the iteration callback. - * @param context supplies the context that will be passed to the callback. */ - virtual void iterateReverse(ConstIterateCb cb, void* context) const PURE; + virtual void iterateReverse(ConstIterateCb cb) const PURE; /** * Clears the headers in the map. diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index d22903da723e1..37f5e858a495b 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -173,14 +173,11 @@ void GoogleAsyncStreamImpl::initialize(bool /*buffer_body_for_retry*/) { // copy headers here. auto initial_metadata = Http::RequestHeaderMapImpl::create(); callbacks_.onCreateInitialMetadata(*initial_metadata); - initial_metadata->iterate( - [](const Http::HeaderEntry& header, void* ctxt) { - auto* client_context = static_cast(ctxt); - client_context->AddMetadata(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return Http::HeaderMap::Iterate::Continue; - }, - &ctxt_); + initial_metadata->iterate([this](const Http::HeaderEntry& header) { + ctxt_.AddMetadata(std::string(header.key().getStringView()), + std::string(header.value().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); // Invoke stub call. rw_ = parent_.stub_->PrepareCall(&ctxt_, "/" + service_full_name_ + "/" + method_name_, &parent_.tls_.completionQueue()); diff --git a/source/common/http/header_list_view.cc b/source/common/http/header_list_view.cc index a29bc84bf86fd..adfb3f0657fa7 100644 --- a/source/common/http/header_list_view.cc +++ b/source/common/http/header_list_view.cc @@ -4,15 +4,11 @@ namespace Envoy { namespace Http { HeaderListView::HeaderListView(const HeaderMap& header_map) { - header_map.iterate( - [](const Http::HeaderEntry& header, void* context) -> HeaderMap::Iterate { - auto* context_ptr = static_cast(context); - context_ptr->keys_.emplace_back(std::reference_wrapper(header.key())); - context_ptr->values_.emplace_back( - std::reference_wrapper(header.value())); - return HeaderMap::Iterate::Continue; - }, - this); + header_map.iterate([this](const Http::HeaderEntry& header) -> HeaderMap::Iterate { + keys_.emplace_back(std::reference_wrapper(header.key())); + values_.emplace_back(std::reference_wrapper(header.value())); + return HeaderMap::Iterate::Continue; + }); } } // namespace Http diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 8803997b706d7..35572390820fd 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -264,28 +264,27 @@ void HeaderMapImpl::subtractSize(uint64_t size) { } void HeaderMapImpl::copyFrom(HeaderMap& lhs, const HeaderMap& header_map) { - header_map.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - // TODO(mattklein123) PERF: Avoid copying here if not necessary. - HeaderString key_string; - key_string.setCopy(header.key().getStringView()); - HeaderString value_string; - value_string.setCopy(header.value().getStringView()); - - static_cast(context)->addViaMove(std::move(key_string), - std::move(value_string)); - return HeaderMap::Iterate::Continue; - }, - &lhs); + header_map.iterate([&lhs](const HeaderEntry& header) -> HeaderMap::Iterate { + // TODO(mattklein123) PERF: Avoid copying here if not necessary. + HeaderString key_string; + key_string.setCopy(header.key().getStringView()); + HeaderString value_string; + value_string.setCopy(header.value().getStringView()); + + lhs.addViaMove(std::move(key_string), std::move(value_string)); + return HeaderMap::Iterate::Continue; + }); } namespace { // This is currently only used in tests and is not optimized for performance. -HeaderMap::Iterate collectAllHeaders(const HeaderEntry& header, void* headers) { - static_cast>*>(headers)->push_back( - std::make_pair(header.key().getStringView(), header.value().getStringView())); - return HeaderMap::Iterate::Continue; +HeaderMap::ConstIterateCb +collectAllHeaders(std::vector>* dest) { + return [dest](const HeaderEntry& header) -> HeaderMap::Iterate { + dest->push_back(std::make_pair(header.key().getStringView(), header.value().getStringView())); + return HeaderMap::Iterate::Continue; + }; }; } // namespace @@ -298,7 +297,7 @@ bool HeaderMapImpl::operator==(const HeaderMap& rhs) const { std::vector> rhs_headers; rhs_headers.reserve(rhs.size()); - rhs.iterate(collectAllHeaders, &rhs_headers); + rhs.iterate(collectAllHeaders(&rhs_headers)); auto i = headers_.begin(); auto j = rhs_headers.begin(); @@ -462,17 +461,17 @@ HeaderEntry* HeaderMapImpl::getExisting(const LowerCaseString& key) { return nullptr; } -void HeaderMapImpl::iterate(HeaderMap::ConstIterateCb cb, void* context) const { +void HeaderMapImpl::iterate(HeaderMap::ConstIterateCb cb) const { for (const HeaderEntryImpl& header : headers_) { - if (cb(header, context) == HeaderMap::Iterate::Break) { + if (cb(header) == HeaderMap::Iterate::Break) { break; } } } -void HeaderMapImpl::iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const { +void HeaderMapImpl::iterateReverse(HeaderMap::ConstIterateCb cb) const { for (auto it = headers_.rbegin(); it != headers_.rend(); it++) { - if (cb(*it, context) == HeaderMap::Iterate::Break) { + if (cb(*it) == HeaderMap::Iterate::Break) { break; } } @@ -527,17 +526,12 @@ size_t HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { } void HeaderMapImpl::dumpState(std::ostream& os, int indent_level) const { - using IterateData = std::pair; - const char* spaces = spacesForLevel(indent_level); - IterateData iterate_data = std::make_pair(&os, spaces); - iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - auto* data = static_cast(context); - *data->first << data->second << "'" << header.key().getStringView() << "', '" - << header.value().getStringView() << "'\n"; - return HeaderMap::Iterate::Continue; - }, - &iterate_data); + iterate([&os, + spaces = spacesForLevel(indent_level)](const HeaderEntry& header) -> HeaderMap::Iterate { + os << spaces << "'" << header.key().getStringView() << "', '" << header.value().getStringView() + << "'\n"; + return HeaderMap::Iterate::Continue; + }); } HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl** entry, diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 693762a21aebd..f9dba6b56ab51 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -86,8 +86,8 @@ class HeaderMapImpl : NonCopyable { void setCopy(const LowerCaseString& key, absl::string_view value); uint64_t byteSize() const; const HeaderEntry* get(const LowerCaseString& key) const; - void iterate(HeaderMap::ConstIterateCb cb, void* context) const; - void iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const; + void iterate(HeaderMap::ConstIterateCb cb) const; + void iterateReverse(HeaderMap::ConstIterateCb cb) const; void clear(); size_t remove(const LowerCaseString& key); size_t removePrefix(const LowerCaseString& key); @@ -298,11 +298,9 @@ template class TypedHeaderMapImpl : public HeaderMapImpl, publ const HeaderEntry* get(const LowerCaseString& key) const override { return HeaderMapImpl::get(key); } - void iterate(HeaderMap::ConstIterateCb cb, void* context) const override { - HeaderMapImpl::iterate(cb, context); - } - void iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const override { - HeaderMapImpl::iterateReverse(cb, context); + void iterate(HeaderMap::ConstIterateCb cb) const override { HeaderMapImpl::iterate(cb); } + void iterateReverse(HeaderMap::ConstIterateCb cb) const override { + HeaderMapImpl::iterateReverse(cb); } void clear() override { HeaderMapImpl::clear(); } size_t remove(const LowerCaseString& key) override { return HeaderMapImpl::remove(key); } diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index b180ad3ead3bc..d56ab6657fc55 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -78,18 +78,13 @@ HeaderUtility::HeaderData::HeaderData(const envoy::config::route::v3::HeaderMatc void HeaderUtility::getAllOfHeader(const HeaderMap& headers, absl::string_view key, std::vector& out) { - auto args = std::make_pair(LowerCaseString(std::string(key)), &out); - - headers.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - auto key_ret = - static_cast*>*>(context); - if (header.key() == key_ret->first.get().c_str()) { - key_ret->second->emplace_back(header.value().getStringView()); - } - return HeaderMap::Iterate::Continue; - }, - &args); + headers.iterate([key = LowerCaseString(std::string(key)), + &out](const HeaderEntry& header) -> HeaderMap::Iterate { + if (header.key() == key.get().c_str()) { + out.emplace_back(header.value().getStringView()); + } + return HeaderMap::Iterate::Continue; + }); } bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, @@ -170,16 +165,14 @@ bool HeaderUtility::isConnectResponse(const RequestHeaderMapPtr& request_headers } void HeaderUtility::addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add) { - headers_to_add.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - HeaderString k; - k.setCopy(header.key().getStringView()); - HeaderString v; - v.setCopy(header.value().getStringView()); - static_cast(context)->addViaMove(std::move(k), std::move(v)); - return HeaderMap::Iterate::Continue; - }, - &headers); + headers_to_add.iterate([&headers](const HeaderEntry& header) -> HeaderMap::Iterate { + HeaderString k; + k.setCopy(header.key().getStringView()); + HeaderString v; + v.setCopy(header.value().getStringView()); + headers.addViaMove(std::move(k), std::move(v)); + return HeaderMap::Iterate::Continue; + }); } bool HeaderUtility::isEnvoyInternalRequest(const RequestHeaderMap& headers) { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index ad47f9fe564ed..eb71d3fa70c1f 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -110,27 +110,24 @@ void ResponseEncoderImpl::encode100ContinueHeaders(const ResponseHeaderMap& head void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& headers, absl::optional status, bool end_stream) { bool saw_content_length = false; - headers.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - absl::string_view key_to_use = header.key().getStringView(); - uint32_t key_size_to_use = header.key().size(); - // Translate :authority -> host so that upper layers do not need to deal with this. - if (key_size_to_use > 1 && key_to_use[0] == ':' && key_to_use[1] == 'a') { - key_to_use = absl::string_view(Headers::get().HostLegacy.get()); - key_size_to_use = Headers::get().HostLegacy.get().size(); - } + headers.iterate([this](const HeaderEntry& header) -> HeaderMap::Iterate { + absl::string_view key_to_use = header.key().getStringView(); + uint32_t key_size_to_use = header.key().size(); + // Translate :authority -> host so that upper layers do not need to deal with this. + if (key_size_to_use > 1 && key_to_use[0] == ':' && key_to_use[1] == 'a') { + key_to_use = absl::string_view(Headers::get().HostLegacy.get()); + key_size_to_use = Headers::get().HostLegacy.get().size(); + } - // Skip all headers starting with ':' that make it here. - if (key_to_use[0] == ':') { - return HeaderMap::Iterate::Continue; - } + // Skip all headers starting with ':' that make it here. + if (key_to_use[0] == ':') { + return HeaderMap::Iterate::Continue; + } - static_cast(context)->encodeFormattedHeader( - key_to_use, header.value().getStringView()); + encodeFormattedHeader(key_to_use, header.value().getStringView()); - return HeaderMap::Iterate::Continue; - }, - this); + return HeaderMap::Iterate::Continue; + }); if (headers.ContentLength()) { saw_content_length = true; @@ -234,13 +231,10 @@ void StreamEncoderImpl::encodeTrailersBase(const HeaderMap& trailers) { // Finalize the body connection_.buffer().add(LAST_CHUNK); - trailers.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - static_cast(context)->encodeFormattedHeader( - header.key().getStringView(), header.value().getStringView()); - return HeaderMap::Iterate::Continue; - }, - this); + trailers.iterate([this](const HeaderEntry& header) -> HeaderMap::Iterate { + encodeFormattedHeader(header.key().getStringView(), header.value().getStringView()); + return HeaderMap::Iterate::Continue; + }); connection_.flushOutput(); connection_.buffer().add(CRLF); diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 4a94bb3aafdfb..1a4c7e6cae448 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -128,13 +128,10 @@ static void insertHeader(std::vector& headers, const HeaderEntry& he void ConnectionImpl::StreamImpl::buildHeaders(std::vector& final_headers, const HeaderMap& headers) { final_headers.reserve(headers.size()); - headers.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - std::vector* final_headers = static_cast*>(context); - insertHeader(*final_headers, header); - return HeaderMap::Iterate::Continue; - }, - &final_headers); + headers.iterate([&final_headers](const HeaderEntry& header) -> HeaderMap::Iterate { + insertHeader(final_headers, header); + return HeaderMap::Iterate::Continue; + }); } void ConnectionImpl::ServerStreamImpl::encode100ContinueHeaders(const ResponseHeaderMap& headers) { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index bee04a98cabe0..0fbddf80c467b 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -290,50 +290,41 @@ absl::string_view Utility::findQueryStringStart(const HeaderString& path) { std::string Utility::parseCookieValue(const HeaderMap& headers, const std::string& key) { - struct State { - std::string key_; - std::string ret_; - }; - - State state; - state.key_ = key; - - headers.iterateReverse( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - // Find the cookie headers in the request (typically, there's only one). - if (header.key() == Http::Headers::get().Cookie.get()) { - - // Split the cookie header into individual cookies. - for (const auto s : StringUtil::splitToken(header.value().getStringView(), ";")) { - // Find the key part of the cookie (i.e. the name of the cookie). - size_t first_non_space = s.find_first_not_of(" "); - size_t equals_index = s.find('='); - if (equals_index == absl::string_view::npos) { - // The cookie is malformed if it does not have an `=`. Continue - // checking other cookies in this header. - continue; - } - const absl::string_view k = s.substr(first_non_space, equals_index - first_non_space); - State* state = static_cast(context); - // If the key matches, parse the value from the rest of the cookie string. - if (k == state->key_) { - absl::string_view v = s.substr(equals_index + 1, s.size() - 1); - - // Cookie values may be wrapped in double quotes. - // https://tools.ietf.org/html/rfc6265#section-4.1.1 - if (v.size() >= 2 && v.back() == '"' && v[0] == '"') { - v = v.substr(1, v.size() - 2); - } - state->ret_ = std::string{v}; - return HeaderMap::Iterate::Break; - } + std::string ret; + + headers.iterateReverse([&key, &ret](const HeaderEntry& header) -> HeaderMap::Iterate { + // Find the cookie headers in the request (typically, there's only one). + if (header.key() == Http::Headers::get().Cookie.get()) { + + // Split the cookie header into individual cookies. + for (const auto s : StringUtil::splitToken(header.value().getStringView(), ";")) { + // Find the key part of the cookie (i.e. the name of the cookie). + size_t first_non_space = s.find_first_not_of(" "); + size_t equals_index = s.find('='); + if (equals_index == absl::string_view::npos) { + // The cookie is malformed if it does not have an `=`. Continue + // checking other cookies in this header. + continue; + } + const absl::string_view k = s.substr(first_non_space, equals_index - first_non_space); + // If the key matches, parse the value from the rest of the cookie string. + if (k == key) { + absl::string_view v = s.substr(equals_index + 1, s.size() - 1); + + // Cookie values may be wrapped in double quotes. + // https://tools.ietf.org/html/rfc6265#section-4.1.1 + if (v.size() >= 2 && v.back() == '"' && v[0] == '"') { + v = v.substr(1, v.size() - 2); } + ret = std::string{v}; + return HeaderMap::Iterate::Break; } - return HeaderMap::Iterate::Continue; - }, - &state); + } + } + return HeaderMap::Iterate::Continue; + }); - return state.ret_; + return ret; } std::string Utility::makeSetCookieValue(const std::string& key, const std::string& value, diff --git a/source/extensions/common/aws/utility.cc b/source/extensions/common/aws/utility.cc index f13012860e706..794051e0d67a5 100644 --- a/source/extensions/common/aws/utility.cc +++ b/source/extensions/common/aws/utility.cc @@ -14,37 +14,34 @@ namespace Aws { std::map Utility::canonicalizeHeaders(const Http::RequestHeaderMap& headers) { std::map out; - headers.iterate( - [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { - auto* map = static_cast*>(context); - // Skip empty headers - if (entry.key().empty() || entry.value().empty()) { - return Http::HeaderMap::Iterate::Continue; - } - // Pseudo-headers should not be canonicalized - if (!entry.key().getStringView().empty() && entry.key().getStringView()[0] == ':') { - return Http::HeaderMap::Iterate::Continue; - } - // Skip headers that are likely to mutate, when crossing proxies - const auto key = entry.key().getStringView(); - if (key == Http::Headers::get().ForwardedFor.get() || - key == Http::Headers::get().ForwardedProto.get() || key == "x-amzn-trace-id") { - return Http::HeaderMap::Iterate::Continue; - } - - std::string value(entry.value().getStringView()); - // Remove leading, trailing, and deduplicate repeated ascii spaces - absl::RemoveExtraAsciiWhitespace(&value); - const auto iter = map->find(std::string(entry.key().getStringView())); - // If the entry already exists, append the new value to the end - if (iter != map->end()) { - iter->second += fmt::format(",{}", value); - } else { - map->emplace(std::string(entry.key().getStringView()), value); - } - return Http::HeaderMap::Iterate::Continue; - }, - &out); + headers.iterate([&out](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { + // Skip empty headers + if (entry.key().empty() || entry.value().empty()) { + return Http::HeaderMap::Iterate::Continue; + } + // Pseudo-headers should not be canonicalized + if (!entry.key().getStringView().empty() && entry.key().getStringView()[0] == ':') { + return Http::HeaderMap::Iterate::Continue; + } + // Skip headers that are likely to mutate, when crossing proxies + const auto key = entry.key().getStringView(); + if (key == Http::Headers::get().ForwardedFor.get() || + key == Http::Headers::get().ForwardedProto.get() || key == "x-amzn-trace-id") { + return Http::HeaderMap::Iterate::Continue; + } + + std::string value(entry.value().getStringView()); + // Remove leading, trailing, and deduplicate repeated ascii spaces + absl::RemoveExtraAsciiWhitespace(&value); + const auto iter = out.find(std::string(entry.key().getStringView())); + // If the entry already exists, append the new value to the end + if (iter != out.end()) { + iter->second += fmt::format(",{}", value); + } else { + out.emplace(std::string(entry.key().getStringView()), value); + } + return Http::HeaderMap::Iterate::Continue; + }); // The AWS SDK has a quirk where it removes "default ports" (80, 443) from the host headers // Additionally, we canonicalize the :authority header as "host" // TODO(lavignes): This may need to be tweaked to canonicalize :authority for HTTP/2 requests diff --git a/source/extensions/filters/common/ext_authz/check_request_utils.cc b/source/extensions/filters/common/ext_authz/check_request_utils.cc index 7906ad3607f6a..55847df1a946f 100644 --- a/source/extensions/filters/common/ext_authz/check_request_utils.cc +++ b/source/extensions/filters/common/ext_authz/check_request_utils.cc @@ -115,18 +115,15 @@ void CheckRequestUtils::setHttpRequest( } // Fill in the headers. - auto mutable_headers = httpreq.mutable_headers(); - headers.iterate( - [](const Envoy::Http::HeaderEntry& e, void* ctx) { - // Skip any client EnvoyAuthPartialBody header, which could interfere with internal use. - if (e.key().getStringView() != Http::Headers::get().EnvoyAuthPartialBody.get()) { - auto* mutable_headers = static_cast*>(ctx); - (*mutable_headers)[std::string(e.key().getStringView())] = - std::string(e.value().getStringView()); - } - return Envoy::Http::HeaderMap::Iterate::Continue; - }, - mutable_headers); + auto* mutable_headers = httpreq.mutable_headers(); + headers.iterate([mutable_headers](const Envoy::Http::HeaderEntry& e) { + // Skip any client EnvoyAuthPartialBody header, which could interfere with internal use. + if (e.key().getStringView() != Http::Headers::get().EnvoyAuthPartialBody.get()) { + (*mutable_headers)[std::string(e.key().getStringView())] = + std::string(e.value().getStringView()); + } + return Envoy::Http::HeaderMap::Iterate::Continue; + }); // Set request body. if (max_request_bytes > 0 && decoding_buffer != nullptr) { 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 01894546ab0f7..3fce6c216bc5e 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 @@ -42,28 +42,25 @@ struct SuccessResponse { const MatcherSharedPtr& append_matchers, Response&& response) : headers_(headers), matchers_(matchers), append_matchers_(append_matchers), response_(std::make_unique(response)) { - headers_.iterate( - [](const Http::HeaderEntry& header, void* ctx) -> Http::HeaderMap::Iterate { - auto* context = static_cast(ctx); - // UpstreamHeaderMatcher - if (context->matchers_->matches(header.key().getStringView())) { - context->response_->headers_to_set.emplace_back( - Http::LowerCaseString{std::string(header.key().getStringView())}, - std::string(header.value().getStringView())); - } - if (context->append_matchers_->matches(header.key().getStringView())) { - // If there is an existing matching key in the current headers, the new entry will be - // appended with the same key. For example, given {"key": "value1"} headers, if there is - // a matching "key" from the authorization response headers {"key": "value2"}, the - // request to upstream server will have two entries for "key": {"key": "value1", "key": - // "value2"}. - context->response_->headers_to_add.emplace_back( - Http::LowerCaseString{std::string(header.key().getStringView())}, - std::string(header.value().getStringView())); - } - return Http::HeaderMap::Iterate::Continue; - }, - this); + headers_.iterate([this](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + // UpstreamHeaderMatcher + if (matchers_->matches(header.key().getStringView())) { + response_->headers_to_set.emplace_back( + Http::LowerCaseString{std::string(header.key().getStringView())}, + std::string(header.value().getStringView())); + } + if (append_matchers_->matches(header.key().getStringView())) { + // If there is an existing matching key in the current headers, the new entry will be + // appended with the same key. For example, given {"key": "value1"} headers, if there is + // a matching "key" from the authorization response headers {"key": "value2"}, the + // request to upstream server will have two entries for "key": {"key": "value1", "key": + // "value2"}. + response_->headers_to_add.emplace_back( + Http::LowerCaseString{std::string(header.key().getStringView())}, + std::string(header.value().getStringView())); + } + return Http::HeaderMap::Iterate::Continue; + }); } const Http::HeaderMap& headers_; diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc index cd702fcba3c9c..e6b93b3f90e24 100644 --- a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc +++ b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc @@ -275,24 +275,21 @@ void Filter::jsonizeRequest(Http::RequestHeaderMap const& headers, const Buffer: } // Wrap the headers - headers.iterate( - [](const Http::HeaderEntry& entry, void* ctx) -> Http::HeaderMap::Iterate { - auto* req = static_cast(ctx); - // ignore H2 pseudo-headers - if (absl::StartsWith(entry.key().getStringView(), ":")) { - return Http::HeaderMap::Iterate::Continue; - } - std::string name = std::string(entry.key().getStringView()); - auto it = req->mutable_headers()->find(name); - if (it == req->headers().end()) { - req->mutable_headers()->insert({name, std::string(entry.value().getStringView())}); - } else { - // Coalesce headers with multiple values - it->second += fmt::format(",{}", entry.value().getStringView()); - } - return Http::HeaderMap::Iterate::Continue; - }, - &json_req); + headers.iterate([&json_req](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { + // ignore H2 pseudo-headers + if (absl::StartsWith(entry.key().getStringView(), ":")) { + return Http::HeaderMap::Iterate::Continue; + } + std::string name = std::string(entry.key().getStringView()); + auto it = json_req.mutable_headers()->find(name); + if (it == json_req.headers().end()) { + json_req.mutable_headers()->insert({name, std::string(entry.value().getStringView())}); + } else { + // Coalesce headers with multiple values + it->second += fmt::format(",{}", entry.value().getStringView()); + } + return Http::HeaderMap::Iterate::Continue; + }); // Wrap the Query String if (headers.Path()) { diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 8cecc0a4003b5..7595839881c43 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -203,16 +203,13 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::ResponseTrailerMa // Trailers are expected to come all in once, and will be encoded into one single trailers frame. // Trailers in the trailers frame are separated by CRLFs. Buffer::OwnedImpl temp; - trailers.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - Buffer::Instance* temp = static_cast(context); - temp->add(header.key().getStringView().data(), header.key().size()); - temp->add(":"); - temp->add(header.value().getStringView().data(), header.value().size()); - temp->add("\r\n"); - return Http::HeaderMap::Iterate::Continue; - }, - &temp); + trailers.iterate([&temp](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + temp.add(header.key().getStringView().data(), header.key().size()); + temp.add(":"); + temp.add(header.value().getStringView().data(), header.value().size()); + temp.add("\r\n"); + return Http::HeaderMap::Iterate::Continue; + }); // Clear out the trailers so they don't get added since it is now in the body trailers.clear(); diff --git a/source/extensions/filters/http/lua/lua_filter.cc b/source/extensions/filters/http/lua/lua_filter.cc index 4f69c77bc9e13..4d09421d5e3e4 100644 --- a/source/extensions/filters/http/lua/lua_filter.cc +++ b/source/extensions/filters/http/lua/lua_filter.cc @@ -330,17 +330,15 @@ void StreamHandleWrapper::onSuccess(const Http::AsyncClient::Request&, // We need to build a table with the headers as return param 1. The body will be return param 2. lua_newtable(coroutine_.luaState()); - response->headers().iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - lua_State* state = static_cast(context); - lua_pushlstring(state, header.key().getStringView().data(), - header.key().getStringView().length()); - lua_pushlstring(state, header.value().getStringView().data(), - header.value().getStringView().length()); - lua_settable(state, -3); - return Http::HeaderMap::Iterate::Continue; - }, - coroutine_.luaState()); + response->headers().iterate([lua_State = coroutine_.luaState()]( + const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + lua_pushlstring(lua_State, header.key().getStringView().data(), + header.key().getStringView().length()); + lua_pushlstring(lua_State, header.value().getStringView().data(), + header.value().getStringView().length()); + lua_settable(lua_State, -3); + return Http::HeaderMap::Iterate::Continue; + }); // TODO(mattklein123): Avoid double copy here. if (response->body() != nullptr) { diff --git a/source/extensions/filters/http/lua/wrappers.cc b/source/extensions/filters/http/lua/wrappers.cc index a772f3c1edfe1..4a24fafaa6a62 100644 --- a/source/extensions/filters/http/lua/wrappers.cc +++ b/source/extensions/filters/http/lua/wrappers.cc @@ -11,13 +11,10 @@ namespace Lua { HeaderMapIterator::HeaderMapIterator(HeaderMapWrapper& parent) : parent_(parent) { entries_.reserve(parent_.headers_.size()); - parent_.headers_.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - HeaderMapIterator* iterator = static_cast(context); - iterator->entries_.push_back(&header); - return Http::HeaderMap::Iterate::Continue; - }, - this); + parent_.headers_.iterate([this](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + entries_.push_back(&header); + return Http::HeaderMap::Iterate::Continue; + }); } int HeaderMapIterator::luaPairsIterator(lua_State* state) { diff --git a/source/extensions/filters/http/tap/tap_config_impl.cc b/source/extensions/filters/http/tap/tap_config_impl.cc index fe602fbfb6e7f..dfe31d083ab2b 100644 --- a/source/extensions/filters/http/tap/tap_config_impl.cc +++ b/source/extensions/filters/http/tap/tap_config_impl.cc @@ -15,13 +15,14 @@ namespace TapFilter { namespace TapCommon = Extensions::Common::Tap; namespace { -Http::HeaderMap::Iterate fillHeaderList(const Http::HeaderEntry& header, void* context) { - Protobuf::RepeatedPtrField& header_list = - *reinterpret_cast*>(context); - auto& new_header = *header_list.Add(); - new_header.set_key(std::string(header.key().getStringView())); - new_header.set_value(std::string(header.value().getStringView())); - return Http::HeaderMap::Iterate::Continue; +Http::HeaderMap::ConstIterateCb +fillHeaderList(Protobuf::RepeatedPtrField* output) { + return [output](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + auto& new_header = *output->Add(); + new_header.set_key(std::string(header.key().getStringView())); + new_header.set_value(std::string(header.value().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }; } } // namespace @@ -35,9 +36,8 @@ HttpPerRequestTapperPtr HttpTapConfigImpl::createPerRequestTapper(uint64_t strea void HttpPerRequestTapperImpl::streamRequestHeaders() { TapCommon::TraceWrapperPtr trace = makeTraceSegment(); - request_headers_->iterate( - fillHeaderList, - trace->mutable_http_streamed_trace_segment()->mutable_request_headers()->mutable_headers()); + request_headers_->iterate(fillHeaderList( + trace->mutable_http_streamed_trace_segment()->mutable_request_headers()->mutable_headers())); sink_handle_->submitTrace(std::move(trace)); } @@ -67,9 +67,9 @@ void HttpPerRequestTapperImpl::onRequestBody(const Buffer::Instance& data) { void HttpPerRequestTapperImpl::streamRequestTrailers() { if (request_trailers_ != nullptr) { TapCommon::TraceWrapperPtr trace = makeTraceSegment(); - request_trailers_->iterate(fillHeaderList, trace->mutable_http_streamed_trace_segment() - ->mutable_request_trailers() - ->mutable_headers()); + request_trailers_->iterate(fillHeaderList(trace->mutable_http_streamed_trace_segment() + ->mutable_request_trailers() + ->mutable_headers())); sink_handle_->submitTrace(std::move(trace)); } } @@ -91,9 +91,8 @@ void HttpPerRequestTapperImpl::onRequestTrailers(const Http::RequestTrailerMap& void HttpPerRequestTapperImpl::streamResponseHeaders() { TapCommon::TraceWrapperPtr trace = makeTraceSegment(); - response_headers_->iterate( - fillHeaderList, - trace->mutable_http_streamed_trace_segment()->mutable_response_headers()->mutable_headers()); + response_headers_->iterate(fillHeaderList( + trace->mutable_http_streamed_trace_segment()->mutable_response_headers()->mutable_headers())); sink_handle_->submitTrace(std::move(trace)); } @@ -141,9 +140,9 @@ void HttpPerRequestTapperImpl::onResponseTrailers(const Http::ResponseTrailerMap } TapCommon::TraceWrapperPtr trace = makeTraceSegment(); - trailers.iterate(fillHeaderList, trace->mutable_http_streamed_trace_segment() - ->mutable_response_trailers() - ->mutable_headers()); + trailers.iterate(fillHeaderList(trace->mutable_http_streamed_trace_segment() + ->mutable_response_trailers() + ->mutable_headers())); sink_handle_->submitTrace(std::move(trace)); } } @@ -156,16 +155,16 @@ bool HttpPerRequestTapperImpl::onDestroyLog() { makeBufferedFullTraceIfNeeded(); auto& http_trace = *buffered_full_trace_->mutable_http_buffered_trace(); if (request_headers_ != nullptr) { - request_headers_->iterate(fillHeaderList, http_trace.mutable_request()->mutable_headers()); + request_headers_->iterate(fillHeaderList(http_trace.mutable_request()->mutable_headers())); } if (request_trailers_ != nullptr) { - request_trailers_->iterate(fillHeaderList, http_trace.mutable_request()->mutable_trailers()); + request_trailers_->iterate(fillHeaderList(http_trace.mutable_request()->mutable_trailers())); } if (response_headers_ != nullptr) { - response_headers_->iterate(fillHeaderList, http_trace.mutable_response()->mutable_headers()); + response_headers_->iterate(fillHeaderList(http_trace.mutable_response()->mutable_headers())); } if (response_trailers_ != nullptr) { - response_trailers_->iterate(fillHeaderList, http_trace.mutable_response()->mutable_trailers()); + response_trailers_->iterate(fillHeaderList(http_trace.mutable_response()->mutable_trailers())); } ENVOY_LOG(debug, "submitting buffered trace sink"); diff --git a/source/extensions/filters/network/thrift_proxy/header_transport_impl.cc b/source/extensions/filters/network/thrift_proxy/header_transport_impl.cc index 26885a842de47..08903d539c49c 100644 --- a/source/extensions/filters/network/thrift_proxy/header_transport_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/header_transport_impl.cc @@ -205,14 +205,11 @@ void HeaderTransportImpl::encodeFrame(Buffer::Instance& buffer, const MessageMet // Num headers BufferHelper::writeVarIntI32(header_buffer, static_cast(headers.size())); - headers.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - Buffer::Instance* hb = static_cast(context); - writeVarString(*hb, header.key().getStringView()); - writeVarString(*hb, header.value().getStringView()); - return Http::HeaderMap::Iterate::Continue; - }, - &header_buffer); + headers.iterate([&header_buffer](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + writeVarString(header_buffer, header.key().getStringView()); + writeVarString(header_buffer, header.value().getStringView()); + return Http::HeaderMap::Iterate::Continue; + }); } uint64_t header_size = header_buffer.length(); diff --git a/source/extensions/filters/network/thrift_proxy/twitter_protocol_impl.cc b/source/extensions/filters/network/thrift_proxy/twitter_protocol_impl.cc index 37a2961f891e2..068646839ed28 100644 --- a/source/extensions/filters/network/thrift_proxy/twitter_protocol_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/twitter_protocol_impl.cc @@ -384,28 +384,24 @@ class RequestHeader { sampled_ = metadata.sampled().value(); } - metadata.headers().iterate( - [](const Http::HeaderEntry& header, void* cb) -> Http::HeaderMap::Iterate { - absl::string_view key = header.key().getStringView(); - if (key.empty()) { - return Http::HeaderMap::Iterate::Continue; - } - - RequestHeader& rh = *static_cast(cb); - if (key == Headers::get().ClientId.get()) { - rh.client_id_ = ClientId(std::string(header.value().getStringView())); - } else if (key == Headers::get().Dest.get()) { - rh.dest_ = std::string(header.value().getStringView()); - } else if (key.find(":d:") == 0 && key.size() > 3) { - rh.delegations_.emplace_back(std::string(key.substr(3)), - std::string(header.value().getStringView())); - } else if (key[0] != ':') { - rh.contexts_.emplace_back(std::string(key), - std::string(header.value().getStringView())); - } - return Http::HeaderMap::Iterate::Continue; - }, - this); + metadata.headers().iterate([this](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + absl::string_view key = header.key().getStringView(); + if (key.empty()) { + return Http::HeaderMap::Iterate::Continue; + } + + if (key == Headers::get().ClientId.get()) { + client_id_ = ClientId(std::string(header.value().getStringView())); + } else if (key == Headers::get().Dest.get()) { + dest_ = std::string(header.value().getStringView()); + } else if (key.find(":d:") == 0 && key.size() > 3) { + delegations_.emplace_back(std::string(key.substr(3)), + std::string(header.value().getStringView())); + } else if (key[0] != ':') { + contexts_.emplace_back(std::string(key), std::string(header.value().getStringView())); + } + return Http::HeaderMap::Iterate::Continue; + }); } void write(Buffer::Instance& buffer) { @@ -575,16 +571,13 @@ class ResponseHeader { } } ResponseHeader(const MessageMetadata& metadata) : spans_(metadata.spans()) { - metadata.headers().iterate( - [](const Http::HeaderEntry& header, void* cb) -> Http::HeaderMap::Iterate { - absl::string_view key = header.key().getStringView(); - if (!key.empty() && key[0] != ':') { - static_cast*>(cb)->emplace_back( - std::string(key), std::string(header.value().getStringView())); - } - return Http::HeaderMap::Iterate::Continue; - }, - &contexts_); + metadata.headers().iterate([this](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + absl::string_view key = header.key().getStringView(); + if (!key.empty() && key[0] != ':') { + contexts_.emplace_back(std::string(key), std::string(header.value().getStringView())); + } + return Http::HeaderMap::Iterate::Continue; + }); } void write(Buffer::Instance& buffer) { diff --git a/source/extensions/grpc_credentials/aws_iam/config.cc b/source/extensions/grpc_credentials/aws_iam/config.cc index 5f60eab1464f2..fed537a7c6748 100644 --- a/source/extensions/grpc_credentials/aws_iam/config.cc +++ b/source/extensions/grpc_credentials/aws_iam/config.cc @@ -129,18 +129,15 @@ AwsIamHeaderAuthenticator::buildMessageToSign(absl::string_view service_url, void AwsIamHeaderAuthenticator::signedHeadersToMetadata( const Http::HeaderMap& headers, std::multimap& metadata) { - headers.iterate( - [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { - auto* md = static_cast*>(context); - const auto& key = entry.key().getStringView(); - // Skip pseudo-headers - if (key.empty() || key[0] == ':') { - return Http::HeaderMap::Iterate::Continue; - } - md->emplace(key, entry.value().getStringView()); - return Http::HeaderMap::Iterate::Continue; - }, - &metadata); + headers.iterate([&metadata](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { + const auto& key = entry.key().getStringView(); + // Skip pseudo-headers + if (key.empty() || key[0] == ':') { + return Http::HeaderMap::Iterate::Continue; + } + metadata.emplace(key, entry.value().getStringView()); + return Http::HeaderMap::Iterate::Continue; + }); } REGISTER_FACTORY(AwsIamGrpcCredentialsFactory, Grpc::GoogleGrpcCredentialsFactory); diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc b/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc index 611cf7b7b721c..aefb6a860e5e6 100644 --- a/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc +++ b/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc @@ -47,14 +47,11 @@ quic::QuicSocketAddress envoyAddressInstanceToQuicSocketAddress( spdy::SpdyHeaderBlock envoyHeadersToSpdyHeaderBlock(const Http::HeaderMap& headers) { spdy::SpdyHeaderBlock header_block; - headers.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - auto spdy_headers = static_cast(context); - // The key-value pairs are copied. - spdy_headers->insert({header.key().getStringView(), header.value().getStringView()}); - return Http::HeaderMap::Iterate::Continue; - }, - &header_block); + headers.iterate([&header_block](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + // The key-value pairs are copied. + header_block.insert({header.key().getStringView(), header.value().getStringView()}); + return Http::HeaderMap::Iterate::Continue; + }); return header_block; } diff --git a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc index 710d94615d59e..080742af2dddb 100644 --- a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc +++ b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc @@ -58,25 +58,26 @@ class OpenTracingHTTPHeadersReader : public opentracing::HTTPHeadersReader { } opentracing::expected ForeachKey(OpenTracingCb f) const override { - request_headers_.iterate(headerMapCallback, static_cast(&f)); + request_headers_.iterate(headerMapCallback(f)); return {}; } private: const Http::RequestHeaderMap& request_headers_; - static Http::HeaderMap::Iterate headerMapCallback(const Http::HeaderEntry& header, - void* context) { - auto* callback = static_cast(context); - opentracing::string_view key{header.key().getStringView().data(), - header.key().getStringView().length()}; - opentracing::string_view value{header.value().getStringView().data(), - header.value().getStringView().length()}; - if ((*callback)(key, value)) { - return Http::HeaderMap::Iterate::Continue; - } else { - return Http::HeaderMap::Iterate::Break; - } + static Http::HeaderMap::ConstIterateCb headerMapCallback(OpenTracingCb callback) { + return [callback = + std::move(callback)](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + opentracing::string_view key{header.key().getStringView().data(), + header.key().getStringView().length()}; + opentracing::string_view value{header.value().getStringView().data(), + header.value().getStringView().length()}; + if (callback(key, value)) { + return Http::HeaderMap::Iterate::Continue; + } else { + return Http::HeaderMap::Iterate::Break; + } + }; } }; } // namespace diff --git a/test/common/http/header_map_impl_fuzz_test.cc b/test/common/http/header_map_impl_fuzz_test.cc index 7c3d8e6a42967..97e327ce57f10 100644 --- a/test/common/http/header_map_impl_fuzz_test.cc +++ b/test/common/http/header_map_impl_fuzz_test.cc @@ -172,20 +172,16 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) // Exercise some read-only accessors. header_map->size(); header_map->byteSize(); - header_map->iterate( - [](const Http::HeaderEntry& header, void * /*context*/) -> Http::HeaderMap::Iterate { - header.key(); - header.value(); - return Http::HeaderMap::Iterate::Continue; - }, - nullptr); - header_map->iterateReverse( - [](const Http::HeaderEntry& header, void * /*context*/) -> Http::HeaderMap::Iterate { - header.key(); - header.value(); - return Http::HeaderMap::Iterate::Continue; - }, - nullptr); + header_map->iterate([](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + header.key(); + header.value(); + return Http::HeaderMap::Iterate::Continue; + }); + header_map->iterateReverse([](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + header.key(); + header.value(); + return Http::HeaderMap::Iterate::Continue; + }); } } diff --git a/test/common/http/header_map_impl_speed_test.cc b/test/common/http/header_map_impl_speed_test.cc index 6f45f39825cd3..acfd730214c3b 100644 --- a/test/common/http/header_map_impl_speed_test.cc +++ b/test/common/http/header_map_impl_speed_test.cc @@ -132,12 +132,12 @@ static void headerMapImplIterate(benchmark::State& state) { auto headers = Http::ResponseHeaderMapImpl::create(); size_t num_callbacks = 0; addDummyHeaders(*headers, state.range(0)); - auto counting_callback = [](const HeaderEntry&, void* context) -> HeaderMap::Iterate { - (*static_cast(context))++; + auto counting_callback = [&num_callbacks](const HeaderEntry&) -> HeaderMap::Iterate { + num_callbacks++; return HeaderMap::Iterate::Continue; }; for (auto _ : state) { - headers->iterate(counting_callback, &num_callbacks); + headers->iterate(counting_callback); } benchmark::DoNotOptimize(num_callbacks); } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 6e0eac4b19dc6..84002ed75b5c7 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -561,6 +561,17 @@ TEST(HeaderMapImplTest, RemoveRegex) { EXPECT_EQ(nullptr, headers.ContentLength()); } +class HeaderAndValueCb + : public testing::MockFunction { +public: + HeaderMap::ConstIterateCb AsIterateCb() { + return [this](const Http::HeaderEntry& header) -> HeaderMap::Iterate { + Call(std::string(header.key().getStringView()), std::string(header.value().getStringView())); + return HeaderMap::Iterate::Continue; + }; + } +}; + TEST(HeaderMapImplTest, SetRemovesAllValues) { TestRequestHeaderMapImpl headers; @@ -577,10 +588,8 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { headers.addReference(key1, ref_value3); headers.addReference(key1, ref_value4); - using MockCb = testing::MockFunction; - { - MockCb cb; + HeaderAndValueCb cb; InSequence seq; EXPECT_CALL(cb, Call("hello", "world")); @@ -588,31 +597,19 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { EXPECT_CALL(cb, Call("hello", "globe")); EXPECT_CALL(cb, Call("hello", "earth")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); } headers.setReference(key1, ref_value5); // set moves key to end { - MockCb cb; + HeaderAndValueCb cb; InSequence seq; EXPECT_CALL(cb, Call("olleh", "planet")); EXPECT_CALL(cb, Call("hello", "blue marble")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); } } @@ -714,19 +711,12 @@ TEST(HeaderMapImplTest, SetCopy) { headers.setCopy(foo, "override-monde"); EXPECT_EQ(headers.size(), 2); - using MockCb = testing::MockFunction; - MockCb cb; + HeaderAndValueCb cb; InSequence seq; EXPECT_CALL(cb, Call("hello", "override-monde")); EXPECT_CALL(cb, Call("hello", "monde2")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Test setting an empty string and then overriding. EXPECT_EQ(2UL, headers.remove(foo)); @@ -847,20 +837,13 @@ TEST(HeaderMapImplTest, Iterate) { LowerCaseString foo_key("foo"); headers.setReferenceKey(foo_key, "bar"); // set moves key to end - using MockCb = testing::MockFunction; - MockCb cb; + HeaderAndValueCb cb; InSequence seq; EXPECT_CALL(cb, Call("hello", "world")); EXPECT_CALL(cb, Call("world", "hello")); EXPECT_CALL(cb, Call("foo", "bar")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); } TEST(HeaderMapImplTest, IterateReverse) { @@ -870,24 +853,20 @@ TEST(HeaderMapImplTest, IterateReverse) { LowerCaseString world_key("world"); headers.setReferenceKey(world_key, "hello"); - using MockCb = testing::MockFunction; - MockCb cb; + HeaderAndValueCb cb; InSequence seq; EXPECT_CALL(cb, Call("world", "hello")); EXPECT_CALL(cb, Call("foo", "bar")); // no "hello" - headers.iterateReverse( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - if (header.key().getStringView() != "foo") { - return HeaderMap::Iterate::Continue; - } else { - return HeaderMap::Iterate::Break; - } - }, - &cb); + headers.iterateReverse([&cb](const Http::HeaderEntry& header) -> HeaderMap::Iterate { + cb.Call(std::string(header.key().getStringView()), std::string(header.value().getStringView())); + if (header.key().getStringView() != "foo") { + return HeaderMap::Iterate::Continue; + } else { + return HeaderMap::Iterate::Break; + } + }); } TEST(HeaderMapImplTest, Get) { @@ -1002,8 +981,7 @@ TEST(TestHeaderMapImplDeathTest, TestHeaderLengthChecks) { } TEST(HeaderMapImplTest, PseudoHeaderOrder) { - using MockCb = testing::MockFunction; - MockCb cb; + HeaderAndValueCb cb; { LowerCaseString foo("hello"); @@ -1029,13 +1007,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("hello", "world")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Removal of the header before which pseudo-headers are inserted EXPECT_EQ(1UL, headers.remove(foo)); @@ -1045,13 +1017,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":method", "PUT")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Next pseudo-header goes after other pseudo-headers, but before normal headers headers.setReferenceKey(Headers::get().Path, "/test"); @@ -1062,13 +1028,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":path", "/test")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Removing the last normal header EXPECT_EQ(1UL, headers.remove(Headers::get().ContentType)); @@ -1078,13 +1038,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":method", "PUT")); EXPECT_CALL(cb, Call(":path", "/test")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Adding a new pseudo-header after removing the last normal header headers.setReferenceKey(Headers::get().Host, "host"); @@ -1095,13 +1049,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":path", "/test")); EXPECT_CALL(cb, Call(":authority", "host")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Adding the first normal header headers.setReferenceKey(Headers::get().ContentType, "text/html"); @@ -1113,13 +1061,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":authority", "host")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Removing all pseudo-headers EXPECT_EQ(1UL, headers.remove(Headers::get().Path)); @@ -1130,13 +1072,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); // Removing all headers EXPECT_EQ(1UL, headers.remove(Headers::get().ContentType)); @@ -1150,13 +1086,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":status", "200")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); } // Starting with a normal header @@ -1174,13 +1104,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/plain")); EXPECT_CALL(cb, Call("hello", "world")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); } // Starting with a pseudo-header @@ -1198,13 +1122,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/plain")); EXPECT_CALL(cb, Call("hello", "world")); - headers.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { - static_cast(cb_v)->Call(std::string(header.key().getStringView()), - std::string(header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate(cb.AsIterateCb()); } } diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index e28ec4fd17b93..6f557b1f017d9 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -547,14 +547,11 @@ TEST(HeaderAddTest, HeaderAdd) { HeaderUtility::addHeaders(headers, headers_to_add); - headers_to_add.iterate( - [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { - TestRequestHeaderMapImpl* headers = static_cast(context); - Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; - EXPECT_EQ(entry.value().getStringView(), headers->get(lower_key)->value().getStringView()); - return Http::HeaderMap::Iterate::Continue; - }, - &headers); + headers_to_add.iterate([&headers](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { + Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; + EXPECT_EQ(entry.value().getStringView(), headers.get(lower_key)->value().getStringView()); + return Http::HeaderMap::Iterate::Continue; + }); } TEST(HeaderIsValidTest, HeaderNameContainsUnderscore) { diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 7f5f47b9dba60..40cc86f167633 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -1147,19 +1147,16 @@ match: { prefix: "/new_endpoint" } using CountMap = absl::flat_hash_map; CountMap counts; - header_map.iterate( - [](const Http::HeaderEntry& header, void* cb_v) -> Http::HeaderMap::Iterate { - CountMap* m = static_cast(cb_v); - absl::string_view key = header.key().getStringView(); - CountMap::iterator i = m->find(key); - if (i == m->end()) { - m->insert({std::string(key), 1}); - } else { - i->second++; - } - return Http::HeaderMap::Iterate::Continue; - }, - &counts); + header_map.iterate([&counts](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + absl::string_view key = header.key().getStringView(); + CountMap::iterator i = counts.find(key); + if (i == counts.end()) { + counts.insert({std::string(key), 1}); + } else { + i->second++; + } + return Http::HeaderMap::Iterate::Continue; + }); EXPECT_EQ(1, counts["static-header"]); EXPECT_EQ(1, counts["x-client-ip"]); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 919df0a675453..ddc452e108bf5 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -768,15 +768,12 @@ TEST_F(RouterTest, AddMultipleCookies) { EXPECT_CALL(cb, Call("foo=\"" + foo_c + "\"; Max-Age=1337; Path=/path; HttpOnly")); EXPECT_CALL(cb, Call("choco=\"" + choco_c + "\"; Max-Age=15; HttpOnly")); - headers.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - if (header.key() == Http::Headers::get().SetCookie.get()) { - static_cast*>(context)->Call( - std::string(header.value().getStringView())); - } - return Http::HeaderMap::Iterate::Continue; - }, - &cb); + headers.iterate([&cb](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + if (header.key() == Http::Headers::get().SetCookie.get()) { + cb.Call(std::string(header.value().getStringView())); + } + return Http::HeaderMap::Iterate::Continue; + }); })); expectResponseTimerCreate(); diff --git a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc index 0b2da9a6a3798..c752f4ca651b0 100644 --- a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc +++ b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_integration_test.cc @@ -123,30 +123,22 @@ class AwsLambdaFilterIntegrationTest : public testing::TestWithParam(ctx); + [actual_headers = &response->headers()](const Http::HeaderEntry& expected_entry) { const auto* actual_entry = actual_headers->get( Http::LowerCaseString(std::string(expected_entry.key().getStringView()))); EXPECT_EQ(actual_entry->value().getStringView(), expected_entry.value().getStringView()); return Http::HeaderMap::Iterate::Continue; - }, - // Because headers() returns a pointer to const we have to cast it - // away to match the callback signature. This is safe because we do - // not call any non-const functions on the headers in the callback. - const_cast(&response->headers())); + }); // verify cookies if we have any if (!expected_response_cookies.empty()) { std::vector actual_cookies; - response->headers().iterate( - [](const Http::HeaderEntry& entry, void* ctx) { - auto* list = static_cast*>(ctx); - if (entry.key().getStringView() == Http::Headers::get().SetCookie.get()) { - list->emplace_back(entry.value().getStringView()); - } - return Http::HeaderMap::Iterate::Continue; - }, - &actual_cookies); + response->headers().iterate([&actual_cookies](const Http::HeaderEntry& entry) { + if (entry.key().getStringView() == Http::Headers::get().SetCookie.get()) { + actual_cookies.emplace_back(entry.value().getStringView()); + } + return Http::HeaderMap::Iterate::Continue; + }); EXPECT_EQ(expected_response_cookies, actual_cookies); } diff --git a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc index 0144f67269c45..ab0cf4c2c9004 100644 --- a/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc +++ b/test/extensions/filters/http/aws_lambda/aws_lambda_filter_test.cc @@ -194,16 +194,13 @@ TEST_F(AwsLambdaFilterTest, DecodeHeadersInvocationModeSetsHeader) { EXPECT_EQ(Http::FilterHeadersStatus::Continue, header_result); std::string invocation_header_value; - headers.iterate( - [](const Http::HeaderEntry& entry, void* ctx) { - auto* out = static_cast(ctx); - if (entry.key().getStringView() == "x-amz-invocation-type") { - out->append(std::string(entry.value().getStringView())); - return Http::HeaderMap::Iterate::Break; - } - return Http::HeaderMap::Iterate::Continue; - }, - &invocation_header_value); + headers.iterate([&invocation_header_value](const Http::HeaderEntry& entry) { + if (entry.key().getStringView() == "x-amz-invocation-type") { + invocation_header_value.append(std::string(entry.value().getStringView())); + return Http::HeaderMap::Iterate::Break; + } + return Http::HeaderMap::Iterate::Continue; + }); EXPECT_EQ("RequestResponse", invocation_header_value); } @@ -527,15 +524,12 @@ TEST_F(AwsLambdaFilterTest, EncodeDataJsonModeTransformToHttp) { EXPECT_EQ("awesome value", custom_header->value().getStringView()); std::vector cookies; - headers.iterate( - [](const Http::HeaderEntry& entry, void* ctx) { - auto* list = static_cast*>(ctx); - if (entry.key().getStringView() == Http::Headers::get().SetCookie.get()) { - list->emplace_back(entry.value().getStringView()); - } - return Http::HeaderMap::Iterate::Continue; - }, - &cookies); + headers.iterate([&cookies](const Http::HeaderEntry& entry) { + if (entry.key().getStringView() == Http::Headers::get().SetCookie.get()) { + cookies.emplace_back(entry.value().getStringView()); + } + return Http::HeaderMap::Iterate::Continue; + }); EXPECT_THAT(cookies, ElementsAre("session-id=42; Secure; HttpOnly", "user=joe")); } 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 fe37da4eaa305..e1d4d2e059ac9 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 @@ -230,27 +230,20 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::VersionedGrpcClientIntegrationP // Entries in this headers are not present in the original request headers. new_headers_from_upstream.iterate( - [](const Http::HeaderEntry& h, void* context) -> Http::HeaderMap::Iterate { - auto* entry = static_cast(context) - ->mutable_ok_response() - ->mutable_headers() - ->Add(); + [&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; - }, - &check_response); + }); // Entries in this headers are not present in the original request headers. But we set append = // true and append = false. headers_to_append_multiple.iterate( - [](const Http::HeaderEntry& h, void* context) -> Http::HeaderMap::Iterate { - auto* entry = static_cast(context) - ->mutable_ok_response() - ->mutable_headers() - ->Add(); + [&check_response](const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { + auto* entry = check_response.mutable_ok_response()->mutable_headers()->Add(); const auto key = std::string(h.key().getStringView()); const auto value = std::string(h.value().getStringView()); @@ -259,8 +252,7 @@ class ExtAuthzGrpcIntegrationTest : public Grpc::VersionedGrpcClientIntegrationP entry->mutable_header()->set_key(key); entry->mutable_header()->set_value(value); return Http::HeaderMap::Iterate::Continue; - }, - &check_response); + }); ext_authz_request_->sendGrpcMessage(check_response); ext_authz_request_->finishGrpcStream(Grpc::Status::Ok); diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc index fc130bd47e6dc..c0384a71dc947 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc @@ -130,8 +130,7 @@ class GrpcJsonTranscoderIntegrationTest } response_headers.iterate( - [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { - auto* response = static_cast(context); + [response = response.get()](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; if (entry.value() == UnexpectedHeaderValue) { EXPECT_FALSE(response->headers().get(lower_key)); @@ -140,8 +139,7 @@ class GrpcJsonTranscoderIntegrationTest response->headers().get(lower_key)->value().getStringView()); } return Http::HeaderMap::Iterate::Continue; - }, - response.get()); + }); if (!response_body.empty()) { if (full_response) { EXPECT_EQ(response_body, response->body()); diff --git a/test/extensions/filters/http/ratelimit/ratelimit_integration_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_integration_test.cc index db2c5849c6515..60b7812718e1e 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_integration_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_integration_test.cc @@ -135,25 +135,19 @@ class RatelimitIntegrationTest : public Grpc::VersionedGrpcClientIntegrationPara response_msg.set_overall_code(code); response_headers_to_add.iterate( - [](const Http::HeaderEntry& h, void* context) -> Http::HeaderMap::Iterate { - auto header = static_cast(context) - ->mutable_response_headers_to_add() - ->Add(); + [&response_msg](const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { + auto header = response_msg.mutable_response_headers_to_add()->Add(); header->set_key(std::string(h.key().getStringView())); header->set_value(std::string(h.value().getStringView())); return Http::HeaderMap::Iterate::Continue; - }, - &response_msg); + }); request_headers_to_add.iterate( - [](const Http::HeaderEntry& h, void* context) -> Http::HeaderMap::Iterate { - auto header = static_cast(context) - ->mutable_request_headers_to_add() - ->Add(); + [&response_msg](const Http::HeaderEntry& h) -> Http::HeaderMap::Iterate { + auto header = response_msg.mutable_request_headers_to_add()->Add(); header->set_key(std::string(h.key().getStringView())); header->set_value(std::string(h.value().getStringView())); return Http::HeaderMap::Iterate::Continue; - }, - &response_msg); + }); ratelimit_request_->sendGrpcMessage(response_msg); ratelimit_request_->finishGrpcStream(Grpc::Status::Ok); } @@ -223,22 +217,18 @@ TEST_P(RatelimitIntegrationTest, OkWithHeaders) { waitForSuccessfulUpstreamResponse(); ratelimit_response_headers.iterate( - [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { - IntegrationStreamDecoder* response = static_cast(context); + [response = response_.get()](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; EXPECT_EQ(entry.value(), response->headers().get(lower_key)->value().getStringView()); return Http::HeaderMap::Iterate::Continue; - }, - response_.get()); + }); - request_headers_to_add.iterate( - [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { - FakeStream* upstream = static_cast(context); - Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; - EXPECT_EQ(entry.value(), upstream->headers().get(lower_key)->value().getStringView()); - return Http::HeaderMap::Iterate::Continue; - }, - upstream_request_.get()); + request_headers_to_add.iterate([upstream = upstream_request_.get()]( + const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { + Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; + EXPECT_EQ(entry.value(), upstream->headers().get(lower_key)->value().getStringView()); + return Http::HeaderMap::Iterate::Continue; + }); cleanup(); @@ -270,13 +260,11 @@ TEST_P(RatelimitIntegrationTest, OverLimitWithHeaders) { waitForFailedUpstreamResponse(429); ratelimit_response_headers.iterate( - [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate { - IntegrationStreamDecoder* response = static_cast(context); + [response = response_.get()](const Http::HeaderEntry& entry) -> Http::HeaderMap::Iterate { Http::LowerCaseString lower_key{std::string(entry.key().getStringView())}; EXPECT_EQ(entry.value(), response->headers().get(lower_key)->value().getStringView()); return Http::HeaderMap::Iterate::Continue; - }, - response_.get()); + }); cleanup(); diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index a873f77c01f2f..12041fdd5860e 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -865,11 +865,10 @@ TEST_F(ZipkinDriverTest, DuplicatedHeader) { span->setSampled(true); span->injectContext(request_headers_); request_headers_.iterate( - [](const Http::HeaderEntry& header, void* cb) -> Http::HeaderMap::Iterate { - EXPECT_FALSE(static_cast(cb)->operator()(header.key().getStringView())); + [&dup_callback](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + dup_callback(header.key().getStringView()); return Http::HeaderMap::Iterate::Continue; - }, - &dup_callback); + }); } } // namespace diff --git a/test/fuzz/utility.h b/test/fuzz/utility.h index 8347e09f2e77c..171674a7ff046 100644 --- a/test/fuzz/utility.h +++ b/test/fuzz/utility.h @@ -116,14 +116,12 @@ inline Http::MetadataMapVector fromMetadata(const test::fuzz::Metadata& metadata // Convert from HeaderMap to test proto Headers. inline test::fuzz::Headers toHeaders(const Http::HeaderMap& headers) { test::fuzz::Headers fuzz_headers; - headers.iterate( - [](const Http::HeaderEntry& header, void* ctxt) -> Http::HeaderMap::Iterate { - auto* fuzz_header = static_cast(ctxt)->add_headers(); - fuzz_header->set_key(std::string(header.key().getStringView())); - fuzz_header->set_value(std::string(header.value().getStringView())); - return Http::HeaderMap::Iterate::Continue; - }, - &fuzz_headers); + headers.iterate([&fuzz_headers](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + auto* fuzz_header = fuzz_headers.add_headers(); + fuzz_header->set_key(std::string(header.key().getStringView())); + fuzz_header->set_value(std::string(header.value().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); return fuzz_headers; } diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 3fb6e2627815d..02ae0e96a690a 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -342,16 +342,14 @@ void HttpIntegrationTest::verifyResponse(IntegrationStreamDecoderPtr response, const std::string& expected_body) { EXPECT_TRUE(response->complete()); EXPECT_EQ(response_code, response->headers().getStatusValue()); - expected_headers.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - auto response_headers = static_cast(context); - const Http::HeaderEntry* entry = - response_headers->get(Http::LowerCaseString{std::string(header.key().getStringView())}); - EXPECT_NE(entry, nullptr); - EXPECT_EQ(header.value().getStringView(), entry->value().getStringView()); - return Http::HeaderMap::Iterate::Continue; - }, - const_cast(static_cast(&response->headers()))); + expected_headers.iterate([response_headers = &response->headers()]( + const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + const Http::HeaderEntry* entry = + response_headers->get(Http::LowerCaseString{std::string(header.key().getStringView())}); + EXPECT_NE(entry, nullptr); + EXPECT_EQ(header.value().getStringView(), entry->value().getStringView()); + return Http::HeaderMap::Iterate::Continue; + }); EXPECT_EQ(response->body(), expected_body); } diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index c196501f7c1f8..7f45914faad19 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -425,18 +425,14 @@ class HeaderValueOfMatcherImpl : public testing::MatcherInterface { bool MatchAndExplain(HeaderMapT headers, testing::MatchResultListener* listener) const override { // Get all headers with matching keys. std::vector values; - std::pair*> context = - std::make_pair(key_.get(), &values); Envoy::Http::HeaderMap::ConstIterateCb get_headers_cb = - [](const Envoy::Http::HeaderEntry& header, void* context) { - auto* typed_context = - static_cast*>*>(context); - if (header.key().getStringView() == typed_context->first) { - typed_context->second->push_back(header.value().getStringView()); + [key = key_.get(), &values](const Envoy::Http::HeaderEntry& header) { + if (header.key().getStringView() == key) { + values.push_back(header.value().getStringView()); } return Envoy::Http::HeaderMap::Iterate::Continue; }; - headers.iterate(get_headers_cb, &context); + headers.iterate(get_headers_cb); if (values.empty()) { *listener << "which has no '" << key_.get() << "' header"; @@ -506,6 +502,14 @@ MATCHER_P(HttpStatusIs, expected_code, "") { return true; } +inline HeaderMap::ConstIterateCb +SaveHeaders(std::vector>* output) { + return [output](const HeaderEntry& header) { + output->push_back(std::make_pair(header.key().getStringView(), header.value().getStringView())); + return HeaderMap::Iterate::Continue; + }; +} + template class IsSubsetOfHeadersMatcherImpl : public testing::MatcherInterface { public: @@ -520,15 +524,11 @@ class IsSubsetOfHeadersMatcherImpl : public testing::MatcherInterface>*>(headers) - ->push_back(std::make_pair(header.key().getStringView(), header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }; std::vector> arg_headers_vec; - headers.iterate(get_headers_cb, &arg_headers_vec); + headers.iterate(SaveHeaders(&arg_headers_vec)); + std::vector> expected_headers_vec; - expected_headers_.iterate(get_headers_cb, &expected_headers_vec); + expected_headers_.iterate(SaveHeaders(&expected_headers_vec)); return ExplainMatchResult(testing::IsSubsetOf(expected_headers_vec), arg_headers_vec, listener); } @@ -575,15 +575,11 @@ class IsSupersetOfHeadersMatcherImpl : public testing::MatcherInterface>*>(headers) - ->push_back(std::make_pair(header.key().getStringView(), header.value().getStringView())); - return HeaderMap::Iterate::Continue; - }; std::vector> arg_headers_vec; - headers.iterate(get_headers_cb, &arg_headers_vec); + headers.iterate(SaveHeaders(&arg_headers_vec)); + std::vector> expected_headers_vec; - expected_headers_.iterate(get_headers_cb, &expected_headers_vec); + expected_headers_.iterate(SaveHeaders(&expected_headers_vec)); return ExplainMatchResult(testing::IsSupersetOf(expected_headers_vec), arg_headers_vec, listener); diff --git a/test/test_common/printers.cc b/test/test_common/printers.cc index c6573e80a5853..28f75d1738969 100644 --- a/test/test_common/printers.cc +++ b/test/test_common/printers.cc @@ -9,14 +9,10 @@ namespace Envoy { namespace Http { void PrintTo(const HeaderMapImpl& headers, std::ostream* os) { - headers.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - std::ostream* os = static_cast(context); - *os << "{'" << header.key().getStringView() << "','" << header.value().getStringView() - << "'}"; - return HeaderMap::Iterate::Continue; - }, - os); + headers.iterate([os](const HeaderEntry& header) -> HeaderMap::Iterate { + *os << "{'" << header.key().getStringView() << "','" << header.value().getStringView() << "'}"; + return HeaderMap::Iterate::Continue; + }); } void PrintTo(const HeaderMapPtr& headers, std::ostream* os) { diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 36e266db8e7ce..b9a1adbf06e99 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -68,26 +68,18 @@ bool TestUtility::headerMapEqualIgnoreOrder(const Http::HeaderMap& lhs, return false; } - struct State { - const Http::HeaderMap& lhs; - bool equal; - }; - - State state{lhs, true}; - rhs.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - State* state = static_cast(context); - const Http::HeaderEntry* entry = - state->lhs.get(Http::LowerCaseString(std::string(header.key().getStringView()))); - if (entry == nullptr || (entry->value() != header.value().getStringView())) { - state->equal = false; - return Http::HeaderMap::Iterate::Break; - } - return Http::HeaderMap::Iterate::Continue; - }, - &state); - - return state.equal; + bool equal = true; + rhs.iterate([&lhs, &equal](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + const Http::HeaderEntry* entry = + lhs.get(Http::LowerCaseString(std::string(header.key().getStringView()))); + if (entry == nullptr || (entry->value() != header.value().getStringView())) { + equal = false; + return Http::HeaderMap::Iterate::Break; + } + return Http::HeaderMap::Iterate::Continue; + }); + + return equal; } bool TestUtility::buffersEqual(const Buffer::Instance& lhs, const Buffer::Instance& rhs) { diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 0727b98729c70..d523a4a71df8b 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -906,11 +906,9 @@ template class TestHeaderMapImplBase : public Inte const HeaderEntry* get(const LowerCaseString& key) const override { return header_map_->get(key); } - void iterate(HeaderMap::ConstIterateCb cb, void* context) const override { - header_map_->iterate(cb, context); - } - void iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const override { - header_map_->iterateReverse(cb, context); + void iterate(HeaderMap::ConstIterateCb cb) const override { header_map_->iterate(cb); } + void iterateReverse(HeaderMap::ConstIterateCb cb) const override { + header_map_->iterateReverse(cb); } void clear() override { header_map_->clear(); From d14fb11dea90ede4a563d93dac2847b96241e937 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 15 Jul 2020 15:11:39 -0400 Subject: [PATCH 2/7] Kick CI Signed-off-by: Alex Konradi From f4f07deccea9dba37c2adc7b467f1f0b6ecd6cf5 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Wed, 15 Jul 2020 17:05:21 -0400 Subject: [PATCH 3/7] Add missing region test for AWS IAM This case was not covered and was causing coverage runs to fail Signed-off-by: Alex Konradi --- .../aws_iam/aws_iam_grpc_credentials_test.cc | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc b/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc index 689f37b339459..99b6efc7593df 100644 --- a/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc +++ b/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc @@ -58,30 +58,40 @@ class GrpcAwsIamClientIntegrationTest : public GrpcSslClientIntegrationTest { TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcacert.pem")); std::string config_yaml; - if (region_in_env_) { + switch(region_location_) { + case RegionLocation::kInEnvironment: TestEnvironment::setEnvVar("AWS_REGION", region_name_, 1); + ABSL_FALLTHROUGH_INTENDED; + case RegionLocation::kNotProvided: config_yaml = fmt::format(R"EOF( "@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.AwsIamConfig service_name: {} )EOF", service_name_); - } else { +break; +case RegionLocation::kInConfig: config_yaml = fmt::format(R"EOF( "@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.AwsIamConfig service_name: {} region: {} )EOF", service_name_, region_name_); + break; } auto* plugin_config = google_grpc->add_call_credentials()->mutable_from_plugin(); plugin_config->set_name(credentials_factory_name_); - envoy::config::grpc_credential::v3::AwsIamConfig metadata_config; Envoy::TestUtility::loadFromYaml(config_yaml, *plugin_config->mutable_typed_config()); return config; } - bool region_in_env_{}; + enum class RegionLocation { + kNotProvided, + kInEnvironment, + kInConfig, + }; + + RegionLocation region_location_ = RegionLocation::kNotProvided; std::string service_name_{}; std::string region_name_{}; std::string credentials_factory_name_{}; @@ -94,6 +104,7 @@ TEST_P(GrpcAwsIamClientIntegrationTest, AwsIamGrpcAuth_ConfigRegion) { SKIP_IF_GRPC_CLIENT(ClientType::EnvoyGrpc); service_name_ = "test_service"; region_name_ = "test_region_static"; + region_location_ = RegionLocation::kInConfig; credentials_factory_name_ = Extensions::GrpcCredentials::GrpcCredentialsNames::get().AwsIam; initialize(); auto request = createRequest(empty_metadata_); @@ -105,7 +116,7 @@ TEST_P(GrpcAwsIamClientIntegrationTest, AwsIamGrpcAuth_EnvRegion) { SKIP_IF_GRPC_CLIENT(ClientType::EnvoyGrpc); service_name_ = "test_service"; region_name_ = "test_region_env"; - region_in_env_ = true; + region_location_ = RegionLocation::kInEnvironment; credentials_factory_name_ = Extensions::GrpcCredentials::GrpcCredentialsNames::get().AwsIam; initialize(); auto request = createRequest(empty_metadata_); @@ -113,6 +124,15 @@ TEST_P(GrpcAwsIamClientIntegrationTest, AwsIamGrpcAuth_EnvRegion) { dispatcher_helper_.runDispatcher(); } +TEST_P(GrpcAwsIamClientIntegrationTest, AwsIamGrpcAuth_NoRegion) { + SKIP_IF_GRPC_CLIENT(ClientType::EnvoyGrpc); + service_name_ = "test_service"; + region_name_ = "test_region_env"; + region_location_ = RegionLocation::kNotProvided; + credentials_factory_name_ = Extensions::GrpcCredentials::GrpcCredentialsNames::get().AwsIam; + EXPECT_THROW_WITH_REGEX(initialize();, EnvoyException, "AWS region"); +} + } // namespace } // namespace Grpc } // namespace Envoy From e98bd7727d9f8564b49adc5c2c849ec9880a6a30 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 16 Jul 2020 09:38:44 -0400 Subject: [PATCH 4/7] Fix format Signed-off-by: Alex Konradi --- .../aws_iam/aws_iam_grpc_credentials_test.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc b/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc index 99b6efc7593df..a3c95467e7c0a 100644 --- a/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc +++ b/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc @@ -58,25 +58,25 @@ class GrpcAwsIamClientIntegrationTest : public GrpcSslClientIntegrationTest { TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcacert.pem")); std::string config_yaml; - switch(region_location_) { - case RegionLocation::kInEnvironment: + switch (region_location_) { + case RegionLocation::kInEnvironment: TestEnvironment::setEnvVar("AWS_REGION", region_name_, 1); ABSL_FALLTHROUGH_INTENDED; - case RegionLocation::kNotProvided: + case RegionLocation::kNotProvided: config_yaml = fmt::format(R"EOF( "@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.AwsIamConfig service_name: {} )EOF", service_name_); -break; -case RegionLocation::kInConfig: + break; + case RegionLocation::kInConfig: config_yaml = fmt::format(R"EOF( "@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.AwsIamConfig service_name: {} region: {} )EOF", service_name_, region_name_); - break; + break; } auto* plugin_config = google_grpc->add_call_credentials()->mutable_from_plugin(); From 72e21a493bace47af2c012e40823c4df88bf342d Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 16 Jul 2020 11:50:52 -0400 Subject: [PATCH 5/7] Fix method name case for clang-tidy Signed-off-by: Alex Konradi --- test/common/http/header_map_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 84002ed75b5c7..84917a08f24d7 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -564,7 +564,7 @@ TEST(HeaderMapImplTest, RemoveRegex) { class HeaderAndValueCb : public testing::MockFunction { public: - HeaderMap::ConstIterateCb AsIterateCb() { + HeaderMap::ConstIterateCb asIterateCb() { return [this](const Http::HeaderEntry& header) -> HeaderMap::Iterate { Call(std::string(header.key().getStringView()), std::string(header.value().getStringView())); return HeaderMap::Iterate::Continue; From ca98d73c34ab452cc8e50913ae1ea8e160e9b58f Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 16 Jul 2020 14:03:10 -0400 Subject: [PATCH 6/7] Actually fix name Add the references that were supposed to be included in the previous commit Signed-off-by: Alex Konradi --- test/common/http/header_map_impl_test.cc | 28 ++++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 84917a08f24d7..6f5552156b401 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -597,7 +597,7 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { EXPECT_CALL(cb, Call("hello", "globe")); EXPECT_CALL(cb, Call("hello", "earth")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); } headers.setReference(key1, ref_value5); // set moves key to end @@ -609,7 +609,7 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { EXPECT_CALL(cb, Call("olleh", "planet")); EXPECT_CALL(cb, Call("hello", "blue marble")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); } } @@ -716,7 +716,7 @@ TEST(HeaderMapImplTest, SetCopy) { InSequence seq; EXPECT_CALL(cb, Call("hello", "override-monde")); EXPECT_CALL(cb, Call("hello", "monde2")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Test setting an empty string and then overriding. EXPECT_EQ(2UL, headers.remove(foo)); @@ -843,7 +843,7 @@ TEST(HeaderMapImplTest, Iterate) { EXPECT_CALL(cb, Call("hello", "world")); EXPECT_CALL(cb, Call("world", "hello")); EXPECT_CALL(cb, Call("foo", "bar")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); } TEST(HeaderMapImplTest, IterateReverse) { @@ -1007,7 +1007,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("hello", "world")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Removal of the header before which pseudo-headers are inserted EXPECT_EQ(1UL, headers.remove(foo)); @@ -1017,7 +1017,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":method", "PUT")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Next pseudo-header goes after other pseudo-headers, but before normal headers headers.setReferenceKey(Headers::get().Path, "/test"); @@ -1028,7 +1028,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":path", "/test")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Removing the last normal header EXPECT_EQ(1UL, headers.remove(Headers::get().ContentType)); @@ -1038,7 +1038,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":method", "PUT")); EXPECT_CALL(cb, Call(":path", "/test")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Adding a new pseudo-header after removing the last normal header headers.setReferenceKey(Headers::get().Host, "host"); @@ -1049,7 +1049,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":path", "/test")); EXPECT_CALL(cb, Call(":authority", "host")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Adding the first normal header headers.setReferenceKey(Headers::get().ContentType, "text/html"); @@ -1061,7 +1061,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":authority", "host")); EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Removing all pseudo-headers EXPECT_EQ(1UL, headers.remove(Headers::get().Path)); @@ -1072,7 +1072,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/html")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); // Removing all headers EXPECT_EQ(1UL, headers.remove(Headers::get().ContentType)); @@ -1086,7 +1086,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call(":status", "200")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); } // Starting with a normal header @@ -1104,7 +1104,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/plain")); EXPECT_CALL(cb, Call("hello", "world")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); } // Starting with a pseudo-header @@ -1122,7 +1122,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { EXPECT_CALL(cb, Call("content-type", "text/plain")); EXPECT_CALL(cb, Call("hello", "world")); - headers.iterate(cb.AsIterateCb()); + headers.iterate(cb.asIterateCb()); } } From bbed5748e1c3b97c02e1da138c54691411912b1c Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 16 Jul 2020 17:50:38 -0400 Subject: [PATCH 7/7] Fix more clang-tidy errors Signed-off-by: Alex Konradi --- .../common/http/header_map_impl_speed_test.cc | 22 +++++++++---------- .../aws_iam/aws_iam_grpc_credentials_test.cc | 20 ++++++++--------- test/mocks/http/mocks.h | 13 ++++++----- test/test_common/printers.cc | 1 + 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/test/common/http/header_map_impl_speed_test.cc b/test/common/http/header_map_impl_speed_test.cc index acfd730214c3b..32020e841c902 100644 --- a/test/common/http/header_map_impl_speed_test.cc +++ b/test/common/http/header_map_impl_speed_test.cc @@ -20,7 +20,7 @@ static void addDummyHeaders(HeaderMap& headers, size_t num_headers) { static void headerMapImplCreate(benchmark::State& state) { // Make sure first time construction is not counted. Http::ResponseHeaderMapImpl::create(); - for (auto _ : state) { + for (auto _ : state) { // NOLINT auto headers = Http::ResponseHeaderMapImpl::create(); benchmark::DoNotOptimize(headers->size()); } @@ -39,7 +39,7 @@ static void headerMapImplSetReference(benchmark::State& state) { const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); - for (auto _ : state) { + for (auto _ : state) { // NOLINT headers->setReference(key, value); } benchmark::DoNotOptimize(headers->size()); @@ -61,7 +61,7 @@ static void headerMapImplGet(benchmark::State& state) { addDummyHeaders(*headers, state.range(0)); headers->setReference(key, value); size_t successes = 0; - for (auto _ : state) { + for (auto _ : state) { // NOLINT successes += (headers->get(key) != nullptr); } benchmark::DoNotOptimize(successes); @@ -78,7 +78,7 @@ static void headerMapImplGetInline(benchmark::State& state) { addDummyHeaders(*headers, state.range(0)); headers->setReferenceConnection(value); size_t size = 0; - for (auto _ : state) { + for (auto _ : state) { // NOLINT size += headers->Connection()->value().size(); } benchmark::DoNotOptimize(size); @@ -93,7 +93,7 @@ static void headerMapImplSetInlineMacro(benchmark::State& state) { const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); - for (auto _ : state) { + for (auto _ : state) { // NOLINT headers->setReferenceConnection(value); } benchmark::DoNotOptimize(headers->size()); @@ -108,7 +108,7 @@ static void headerMapImplSetInlineInteger(benchmark::State& state) { uint64_t value = 12345; auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); - for (auto _ : state) { + for (auto _ : state) { // NOLINT headers->setConnection(value); } benchmark::DoNotOptimize(headers->size()); @@ -120,7 +120,7 @@ static void headerMapImplGetByteSize(benchmark::State& state) { auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); uint64_t size = 0; - for (auto _ : state) { + for (auto _ : state) { // NOLINT size += headers->byteSize(); } benchmark::DoNotOptimize(size); @@ -136,7 +136,7 @@ static void headerMapImplIterate(benchmark::State& state) { num_callbacks++; return HeaderMap::Iterate::Continue; }; - for (auto _ : state) { + for (auto _ : state) { // NOLINT headers->iterate(counting_callback); } benchmark::DoNotOptimize(num_callbacks); @@ -153,7 +153,7 @@ static void headerMapImplRemove(benchmark::State& state) { const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); - for (auto _ : state) { + for (auto _ : state) { // NOLINT headers->addReference(key, value); headers->remove(key); } @@ -172,7 +172,7 @@ static void headerMapImplRemoveInline(benchmark::State& state) { const std::string value("01234567890123456789"); auto headers = Http::ResponseHeaderMapImpl::create(); addDummyHeaders(*headers, state.range(0)); - for (auto _ : state) { + for (auto _ : state) { // NOLINT headers->addReference(key, value); headers->remove(key); } @@ -197,7 +197,7 @@ static void headerMapImplPopulate(benchmark::State& state) { {LowerCaseString("set-cookie"), "_cookie1=12345678; path = /; secure"}, {LowerCaseString("set-cookie"), "_cookie2=12345678; path = /; secure"}, }; - for (auto _ : state) { + for (auto _ : state) { // NOLINT auto headers = Http::ResponseHeaderMapImpl::create(); for (const auto& key_value : headers_to_add) { headers->addReference(key_value.first, key_value.second); diff --git a/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc b/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc index a3c95467e7c0a..40d09b78e23b9 100644 --- a/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc +++ b/test/extensions/grpc_credentials/aws_iam/aws_iam_grpc_credentials_test.cc @@ -59,17 +59,17 @@ class GrpcAwsIamClientIntegrationTest : public GrpcSslClientIntegrationTest { std::string config_yaml; switch (region_location_) { - case RegionLocation::kInEnvironment: + case RegionLocation::InEnvironment: TestEnvironment::setEnvVar("AWS_REGION", region_name_, 1); ABSL_FALLTHROUGH_INTENDED; - case RegionLocation::kNotProvided: + case RegionLocation::NotProvided: config_yaml = fmt::format(R"EOF( "@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.AwsIamConfig service_name: {} )EOF", service_name_); break; - case RegionLocation::kInConfig: + case RegionLocation::InConfig: config_yaml = fmt::format(R"EOF( "@type": type.googleapis.com/envoy.config.grpc_credential.v2alpha.AwsIamConfig service_name: {} @@ -86,12 +86,12 @@ region: {} } enum class RegionLocation { - kNotProvided, - kInEnvironment, - kInConfig, + NotProvided, + InEnvironment, + InConfig, }; - RegionLocation region_location_ = RegionLocation::kNotProvided; + RegionLocation region_location_ = RegionLocation::NotProvided; std::string service_name_{}; std::string region_name_{}; std::string credentials_factory_name_{}; @@ -104,7 +104,7 @@ TEST_P(GrpcAwsIamClientIntegrationTest, AwsIamGrpcAuth_ConfigRegion) { SKIP_IF_GRPC_CLIENT(ClientType::EnvoyGrpc); service_name_ = "test_service"; region_name_ = "test_region_static"; - region_location_ = RegionLocation::kInConfig; + region_location_ = RegionLocation::InConfig; credentials_factory_name_ = Extensions::GrpcCredentials::GrpcCredentialsNames::get().AwsIam; initialize(); auto request = createRequest(empty_metadata_); @@ -116,7 +116,7 @@ TEST_P(GrpcAwsIamClientIntegrationTest, AwsIamGrpcAuth_EnvRegion) { SKIP_IF_GRPC_CLIENT(ClientType::EnvoyGrpc); service_name_ = "test_service"; region_name_ = "test_region_env"; - region_location_ = RegionLocation::kInEnvironment; + region_location_ = RegionLocation::InEnvironment; credentials_factory_name_ = Extensions::GrpcCredentials::GrpcCredentialsNames::get().AwsIam; initialize(); auto request = createRequest(empty_metadata_); @@ -128,7 +128,7 @@ TEST_P(GrpcAwsIamClientIntegrationTest, AwsIamGrpcAuth_NoRegion) { SKIP_IF_GRPC_CLIENT(ClientType::EnvoyGrpc); service_name_ = "test_service"; region_name_ = "test_region_env"; - region_location_ = RegionLocation::kNotProvided; + region_location_ = RegionLocation::NotProvided; credentials_factory_name_ = Extensions::GrpcCredentials::GrpcCredentialsNames::get().AwsIam; EXPECT_THROW_WITH_REGEX(initialize();, EnvoyException, "AWS region"); } diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 7f45914faad19..afbad757fcd72 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -422,6 +422,7 @@ class HeaderValueOfMatcherImpl : public testing::MatcherInterface { testing::Matcher matcher) : key_(std::move(key)), matcher_(std::move(matcher)) {} + // NOLINTNEXTLINE(readability-identifier-naming) bool MatchAndExplain(HeaderMapT headers, testing::MatchResultListener* listener) const override { // Get all headers with matching keys. std::vector values; @@ -503,7 +504,7 @@ MATCHER_P(HttpStatusIs, expected_code, "") { } inline HeaderMap::ConstIterateCb -SaveHeaders(std::vector>* output) { +saveHeaders(std::vector>* output) { return [output](const HeaderEntry& header) { output->push_back(std::make_pair(header.key().getStringView(), header.value().getStringView())); return HeaderMap::Iterate::Continue; @@ -522,13 +523,14 @@ class IsSubsetOfHeadersMatcherImpl : public testing::MatcherInterface> arg_headers_vec; - headers.iterate(SaveHeaders(&arg_headers_vec)); + headers.iterate(saveHeaders(&arg_headers_vec)); std::vector> expected_headers_vec; - expected_headers_.iterate(SaveHeaders(&expected_headers_vec)); + expected_headers_.iterate(saveHeaders(&expected_headers_vec)); return ExplainMatchResult(testing::IsSubsetOf(expected_headers_vec), arg_headers_vec, listener); } @@ -573,13 +575,14 @@ class IsSupersetOfHeadersMatcherImpl : public testing::MatcherInterface> arg_headers_vec; - headers.iterate(SaveHeaders(&arg_headers_vec)); + headers.iterate(saveHeaders(&arg_headers_vec)); std::vector> expected_headers_vec; - expected_headers_.iterate(SaveHeaders(&expected_headers_vec)); + expected_headers_.iterate(saveHeaders(&expected_headers_vec)); return ExplainMatchResult(testing::IsSupersetOf(expected_headers_vec), arg_headers_vec, listener); diff --git a/test/test_common/printers.cc b/test/test_common/printers.cc index 28f75d1738969..8a2ece63ad132 100644 --- a/test/test_common/printers.cc +++ b/test/test_common/printers.cc @@ -8,6 +8,7 @@ namespace Envoy { namespace Http { +// NOLINTNEXTLINE(readability-identifier-naming) void PrintTo(const HeaderMapImpl& headers, std::ostream* os) { headers.iterate([os](const HeaderEntry& header) -> HeaderMap::Iterate { *os << "{'" << header.key().getStringView() << "','" << header.value().getStringView() << "'}";