From 759e22ef40a1153e124ebda52b573dc19e29ef74 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 1 Dec 2019 20:32:47 -0500 Subject: [PATCH 01/14] patch in PR 9114. Signed-off-by: Joshua Marantz --- include/envoy/access_log/access_log.h | 4 - include/envoy/http/header_map.h | 94 +++-- source/common/grpc/common.cc | 7 +- source/common/grpc/common.h | 8 +- source/common/http/conn_manager_impl.cc | 12 - source/common/http/conn_manager_utility.cc | 13 +- source/common/http/header_map_impl.cc | 129 ++++--- source/common/http/header_map_impl.h | 63 ++-- source/common/http/http1/codec_impl.cc | 12 +- source/common/http/http2/codec_impl.cc | 14 +- source/common/http/path_utility.cc | 12 +- source/common/http/path_utility.h | 6 +- source/common/http/utility.cc | 17 +- source/common/router/config_impl.cc | 6 +- source/common/router/router.cc | 27 +- source/common/router/router.h | 4 +- source/common/router/shadow_writer_impl.cc | 2 +- source/common/upstream/health_checker_impl.cc | 3 +- .../access_loggers/common/access_log_base.h | 5 - .../grpc/http_grpc_access_log_impl.cc | 7 +- .../extensions/filters/common/expr/context.cc | 2 +- .../dynamic_forward_proxy/proxy_filter.cc | 4 +- .../filters/http/ext_authz/ext_authz.cc | 15 +- .../grpc_http1_bridge/http1_bridge_filter.cc | 2 +- .../http/grpc_http1_reverse_bridge/filter.cc | 6 +- .../json_transcoder_filter.cc | 6 +- .../filters/http/health_check/health_check.cc | 2 +- .../http/ip_tagging/ip_tagging_filter.cc | 2 +- .../extensions/filters/http/lua/wrappers.cc | 7 +- .../filters/http/rbac/rbac_filter.cc | 8 - .../common/ot/opentracing_driver_impl.cc | 2 +- test/common/grpc/common_test.cc | 30 +- test/common/http/header_map_impl_fuzz.proto | 24 +- test/common/http/header_map_impl_fuzz_test.cc | 100 +++--- .../common/http/header_map_impl_speed_test.cc | 19 +- test/common/http/header_map_impl_test.cc | 336 ++++++++++++------ test/common/http/http2/codec_impl_test.cc | 6 +- test/common/http/path_utility_test.cc | 14 +- test/common/router/router_test.cc | 6 +- .../filters/call_decodedata_once_filter.cc | 6 +- .../decode_headers_return_stop_all_filter.cc | 11 +- .../encode_headers_return_stop_all_filter.cc | 9 +- .../filters/metadata_stop_all_filter.cc | 3 +- .../integration/websocket_integration_test.cc | 2 +- test/server/http/admin_test.cc | 2 +- 45 files changed, 558 insertions(+), 511 deletions(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index 394df8f26ccb7..3648a6e44a679 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -78,10 +78,6 @@ class Instance { /** * Log a completed request. - * Prior to logging, call refreshByteSize() on HeaderMaps to ensure that an accurate byte size - * count is logged. - * TODO(asraa): Remove refreshByteSize() requirement when entries in HeaderMap can no longer be - * modified by reference and headerMap holds an accurate internal byte size count. * @param request_headers supplies the incoming request headers after filtering. * @param response_headers supplies response headers. * @param response_trailers supplies response trailers. diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 53703e1fd5b19..05c48b0f95bc6 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -113,7 +113,7 @@ class HeaderString { * @param ref_value MUST point to data that will live beyond the lifetime of any request/response * using the string (since a codec may optimize for zero copy). */ - explicit HeaderString(const std::string& ref_value); + explicit HeaderString(absl::string_view ref_value); HeaderString(HeaderString&& move_value) noexcept; ~HeaderString(); @@ -224,13 +224,6 @@ class HeaderEntry { */ virtual const HeaderString& key() const PURE; - /** - * Set the header value by copying data into it (deprecated, use absl::string_view variant - * instead). - * TODO(htuch): Cleanup deprecated call sites. - */ - virtual void value(const char* value, uint32_t size) PURE; - /** * Set the header value by copying data into it. */ @@ -345,19 +338,22 @@ class HeaderEntry { HEADER_FUNC(Via) /** - * The following functions are defined for each inline header above. E.g., for ContentLength we - * have: + * The following functions are defined for each inline header above. + + * E.g., for path we have: + * Path() -> returns the header entry if it exists or nullptr. + * appendPath(path, "/") -> appends the string path with delimiter "/" to the header value. + * setReferencePath(PATH) -> sets header value to reference string PATH. + * setPath(path_string) -> sets the header value to the string path_string by copying the data. + * removePath() -> removes the header if it exists. * - * ContentLength() -> returns the header entry if it exists or nullptr. - * insertContentLength() -> inserts the header if it does not exist, and returns a reference to it. - * removeContentLength() -> removes the header if it exists. - * - * TODO(asraa): Remove functions with a non-const HeaderEntry return value. + * For inline headers that use integers, we have: + * // TODO(asraa): Remove this method for other inline headers. + * setContentLength(5) -> sets the header value to the integer 5. */ #define DEFINE_INLINE_HEADER(name) \ virtual const HeaderEntry* name() const PURE; \ - virtual HeaderEntry* name() PURE; \ - virtual HeaderEntry& insert##name() PURE; \ + virtual void append##name(absl::string_view data, absl::string_view delimiter) PURE; \ virtual void setReference##name(absl::string_view value) PURE; \ virtual void set##name(absl::string_view value) PURE; \ virtual void set##name(uint64_t value) PURE; \ @@ -381,12 +377,11 @@ class HeaderMap { * Calling addReference multiple times for the same header will result in: * - Comma concatenation for predefined inline headers. * - Multiple headers being present in the HeaderMap for other headers. - * TODO(asraa): Replace const std::string& param with an absl::string_view. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL NOT be copied. */ - virtual void addReference(const LowerCaseString& key, const std::string& value) PURE; + virtual void addReference(const LowerCaseString& key, absl::string_view value) PURE; /** * Add a header with a reference key to the map. The key MUST point to data that will live beyond @@ -414,7 +409,7 @@ class HeaderMap { * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL be copied. */ - virtual void addReferenceKey(const LowerCaseString& key, const std::string& value) PURE; + virtual void addReferenceKey(const LowerCaseString& key, absl::string_view value) PURE; /** * Add a header by copying both the header key and the value. @@ -438,7 +433,15 @@ class HeaderMap { * @param key specifies the name of the header to add; it WILL be copied. * @param value specifies the value of the header to add; it WILL be copied. */ - virtual void addCopy(const LowerCaseString& key, const std::string& value) PURE; + virtual void addCopy(const LowerCaseString& key, absl::string_view value) PURE; + + /** + * Appends data to header. If header already has a value, the string "," is added between the + * existing value and data. + * @param key specifies the name of the header to append; it WILL be copied. + * @param value specifies the value of the header to add; it WILL be copied. + */ + virtual void append(const LowerCaseString& key, absl::string_view value) PURE; /** * Set a reference header in the map. Both key and value MUST point to data that will live beyond @@ -447,12 +450,11 @@ class HeaderMap { * * Calling setReference multiple times for the same header will result in only the last header * being present in the HeaderMap. - * TODO(asraa): Replace const std::string& param with an absl::string_view. * * @param key specifies the name of the header to set; it WILL NOT be copied. * @param value specifies the value of the header to set; it WILL NOT be copied. */ - virtual void setReference(const LowerCaseString& key, const std::string& value) PURE; + virtual void setReference(const LowerCaseString& key, absl::string_view value) PURE; /** * Set a header with a reference key in the map. The key MUST point to point to data that will @@ -465,44 +467,24 @@ class HeaderMap { * @param key specifies the name of the header to set; it WILL NOT be copied. * @param value specifies the value of the header to set; it WILL be copied. */ - virtual void setReferenceKey(const LowerCaseString& key, const std::string& value) PURE; + virtual void setReferenceKey(const LowerCaseString& key, absl::string_view value) PURE; /** - * HeaderMap contains an internal byte size count, updated as entries are added, removed, or - * modified through the HeaderMap interface. However, HeaderEntries can be accessed and modified - * by reference so that the HeaderMap can no longer accurately update the internal byte size - * count. - * - * Calling byteSize before a HeaderEntry is accessed will return the internal byte size count. The - * value is cleared when a HeaderEntry is accessed, and the value is updated and set again when - * refreshByteSize is called. + * Replaces a header value by copying the value. Copies the key if the key does not exist. * - * To guarantee an accurate byte size count, call refreshByteSize. - * - * @return uint64_t the approximate size of the header map in bytes if valid. - */ - virtual absl::optional byteSize() const PURE; - - /** - * This returns the sum of the byte sizes of the keys and values in the HeaderMap. This also - * updates and sets the byte size count. - * - * To guarantee an accurate byte size count, use this. If it is known HeaderEntries have not been - * manipulated since a call to refreshByteSize, it is safe to use byteSize. + * Calling setCopy multiple times for the same header will result in only the last header + * being present in the HeaderMap. * - * @return uint64_t the approximate size of the header map in bytes. + * @param key specifies the name of the header to set; it WILL be copied. + * @param value specifies the value of the header to set; it WILL be copied. */ - virtual uint64_t refreshByteSize() PURE; + virtual void setCopy(const LowerCaseString& key, absl::string_view value) PURE; /** - * This returns the sum of the byte sizes of the keys and values in the HeaderMap. - * - * This iterates over the HeaderMap to calculate size and should only be called directly when the - * user wants an explicit recalculation of the byte size. - * - * @return uint64_t the approximate size of the header map in bytes. + * @return uint64_t the size of the header map in bytes. This is the sum of the header keys and + * values and does not account for data structure overhead. */ - virtual uint64_t byteSizeInternal() const PURE; + virtual uint64_t byteSize() const PURE; /** * Get a header by key. @@ -510,7 +492,6 @@ class HeaderMap { * @return the header entry if it exists otherwise nullptr. */ virtual const HeaderEntry* get(const LowerCaseString& key) const PURE; - virtual HeaderEntry* get(const LowerCaseString& key) PURE; // aliases to make iterate() and iterateReverse() callbacks easier to read enum class Iterate { Continue, Break }; @@ -549,6 +530,11 @@ class HeaderMap { */ virtual Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const PURE; + /** + * Clears the headers in the map. + */ + virtual void clear() PURE; + /** * Remove all instances of a header by key. * @param key supplies the header key to remove. diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 7b20c5d2d1587..7129cf1f081a1 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -170,7 +170,7 @@ std::chrono::milliseconds Common::getGrpcTimeout(const Http::HeaderMap& request_ return timeout; } -void Common::toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderString& value) { +void Common::toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderMap& headers) { uint64_t time = timeout.count(); static const char units[] = "mSMH"; const char* unit = units; // start with milliseconds @@ -187,8 +187,7 @@ void Common::toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::Heade unit++; } } - value.setInteger(time); - value.append(unit, 1); + headers.setGrpcTimeout(absl::StrCat(time, absl::string_view(unit, 1))); } Http::MessagePtr Common::prepareHeaders(const std::string& upstream_cluster, @@ -203,7 +202,7 @@ Http::MessagePtr Common::prepareHeaders(const std::string& upstream_cluster, // before Timeout and ContentType. message->headers().setReferenceTE(Http::Headers::get().TEValues.Trailers); if (timeout) { - toGrpcTimeout(timeout.value(), message->headers().insertGrpcTimeout().value()); + toGrpcTimeout(timeout.value(), message->headers()); } message->headers().setReferenceContentType(Http::Headers::get().ContentTypeValues.Grpc); diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index cae1a34b96211..a44117a82161c 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -81,12 +81,12 @@ class Common { static std::chrono::milliseconds getGrpcTimeout(const Http::HeaderMap& request_headers); /** - * Encode 'timeout' into 'grpc-timeout' format. + * Encode 'timeout' into 'grpc-timeout' format in the grpc-timeout header. * @param timeout the duration in std::chrono::milliseconds. - * @param value the HeaderString onto which format the timeout in 'grpc-timeout' format, up to - * 8 decimal digits and a letter indicating the unit. + * @param headers the HeaderMap in which the grpc-timeout header will be set with the timeout in + * 'grpc-timeout' format, up to 8 decimal digits and a letter indicating the unit. */ - static void toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderString& value); + static void toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderMap& headers); /** * Serialize protobuf message with gRPC frame header. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index e9c4375e31c58..7744018dc446f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -549,18 +549,6 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { } connection_manager_.stats_.named_.downstream_rq_active_.dec(); - // Refresh byte sizes of the HeaderMaps before logging. - // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and - // HeaderMap holds an accurate internal byte size count. - if (request_headers_ != nullptr) { - request_headers_->refreshByteSize(); - } - if (response_headers_ != nullptr) { - response_headers_->refreshByteSize(); - } - if (response_trailers_ != nullptr) { - response_trailers_->refreshByteSize(); - } for (const AccessLog::InstanceSharedPtr& access_log : connection_manager_.config_.accessLogs()) { access_log->log(request_headers_.get(), response_headers_.get(), response_trailers_.get(), stream_info_); diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 5a9f968a74366..c118d94bdbb26 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -200,8 +200,8 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest if (config.userAgent()) { request_headers.setEnvoyDownstreamServiceCluster(config.userAgent().value()); - const HeaderEntry& user_agent_header = request_headers.insertUserAgent(); - if (user_agent_header.value().empty()) { + const HeaderEntry* user_agent_header = request_headers.UserAgent(); + if (!user_agent_header || user_agent_header->value().empty()) { // Following setReference() is safe because user agent is constant for the life of the // listener. request_headers.setReferenceUserAgent(config.userAgent().value()); @@ -283,7 +283,7 @@ void ConnectionManagerUtility::mutateTracingRequestHeader(HeaderMap& request_hea UuidUtils::setTraceableUuid(x_request_id, UuidTraceStatus::NoTrace); } - request_headers.RequestId()->value(x_request_id); + request_headers.setRequestId(x_request_id); } void ConnectionManagerUtility::mutateXfccRequestHeader(HeaderMap& request_headers, @@ -364,8 +364,7 @@ void ConnectionManagerUtility::mutateXfccRequestHeader(HeaderMap& request_header const std::string client_cert_details_str = absl::StrJoin(client_cert_details, ";"); if (config.forwardClientCert() == ForwardClientCertType::AppendForward) { - HeaderMapImpl::appendToHeader(request_headers.insertForwardedClientCert().value(), - client_cert_details_str); + request_headers.appendForwardedClientCert(client_cert_details_str, ","); } else if (config.forwardClientCert() == ForwardClientCertType::SanitizeSet) { request_headers.setForwardedClientCert(client_cert_details_str); } else { @@ -411,11 +410,11 @@ bool ConnectionManagerUtility::maybeNormalizePath(HeaderMap& request_headers, ASSERT(request_headers.Path()); bool is_valid_path = true; if (config.shouldNormalizePath()) { - is_valid_path = PathUtil::canonicalPath(*request_headers.Path()); + is_valid_path = PathUtil::canonicalPath(request_headers); } // Merge slashes after path normalization to catch potential edge cases with percent encoding. if (is_valid_path && config.shouldMergeSlashes()) { - PathUtil::mergeSlashes(*request_headers.Path()); + PathUtil::mergeSlashes(request_headers); } return is_valid_path; } diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index bd870f6672b58..ae3a02d84ee9a 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -49,8 +49,8 @@ HeaderString::HeaderString(const LowerCaseString& ref_value) : type_(Type::Refer ASSERT(valid()); } -HeaderString::HeaderString(const std::string& ref_value) : type_(Type::Reference) { - buffer_.ref_ = ref_value.c_str(); +HeaderString::HeaderString(absl::string_view ref_value) : type_(Type::Reference) { + buffer_.ref_ = ref_value.data(); string_length_ = ref_value.size(); ASSERT(valid()); } @@ -261,13 +261,7 @@ HeaderMapImpl::HeaderEntryImpl::HeaderEntryImpl(const LowerCaseString& key, Head HeaderMapImpl::HeaderEntryImpl::HeaderEntryImpl(HeaderString&& key, HeaderString&& value) : key_(std::move(key)), value_(std::move(value)) {} -void HeaderMapImpl::HeaderEntryImpl::value(const char* value, uint32_t size) { - value_.setCopy(value, size); -} - -void HeaderMapImpl::HeaderEntryImpl::value(absl::string_view value) { - this->value(value.data(), static_cast(value.size())); -} +void HeaderMapImpl::HeaderEntryImpl::value(absl::string_view value) { value_.setCopy(value); } void HeaderMapImpl::HeaderEntryImpl::value(uint64_t value) { value_.setInteger(value); } @@ -295,14 +289,15 @@ struct HeaderMapImpl::StaticLookupTable : public TrieLookupTable { } }; -uint64_t HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data) { +uint64_t HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data, + absl::string_view delimiter) { if (data.empty()) { return 0; } uint64_t byte_size = 0; if (!header.empty()) { - header.append(",", 1); - byte_size += 1; + header.append(delimiter.data(), delimiter.size()); + byte_size += delimiter.size(); } header.append(data.data(), data.size()); return data.size() + byte_size; @@ -320,29 +315,20 @@ HeaderMapImpl::HeaderMapImpl( value_string.setCopy(value.second.c_str(), value.second.size()); addViaMove(std::move(key_string), std::move(value_string)); } + verifyByteSize(); } void HeaderMapImpl::updateSize(uint64_t from_size, uint64_t to_size) { - // Removes from_size from cached_byte_size_ and adds to_size. - if (cached_byte_size_.has_value()) { - ASSERT(cached_byte_size_ >= from_size); - cached_byte_size_.value() -= from_size; - cached_byte_size_.value() += to_size; - } + ASSERT(cached_byte_size_ >= from_size); + cached_byte_size_ -= from_size; + cached_byte_size_ += to_size; } -void HeaderMapImpl::addSize(uint64_t size) { - // Adds size to cached_byte_size_ if it exists. - if (cached_byte_size_.has_value()) { - cached_byte_size_.value() += size; - } -} +void HeaderMapImpl::addSize(uint64_t size) { cached_byte_size_ += size; } void HeaderMapImpl::subtractSize(uint64_t size) { - if (cached_byte_size_.has_value()) { - ASSERT(cached_byte_size_ >= size); - cached_byte_size_.value() -= size; - } + ASSERT(cached_byte_size_ >= size); + cached_byte_size_ -= size; } void HeaderMapImpl::copyFrom(const HeaderMap& header_map) { @@ -359,6 +345,7 @@ void HeaderMapImpl::copyFrom(const HeaderMap& header_map) { return HeaderMap::Iterate::Continue; }, this); + verifyByteSize(); } bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const { @@ -409,12 +396,14 @@ void HeaderMapImpl::addViaMove(HeaderString&& key, HeaderString&& value) { } else { insertByKey(std::move(key), std::move(value)); } + verifyByteSize(); } -void HeaderMapImpl::addReference(const LowerCaseString& key, const std::string& value) { +void HeaderMapImpl::addReference(const LowerCaseString& key, absl::string_view value) { HeaderString ref_key(key); HeaderString ref_value(value); addViaMove(std::move(ref_key), std::move(ref_value)); + verifyByteSize(); } void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, uint64_t value) { @@ -423,14 +412,16 @@ void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, uint64_t value) new_value.setInteger(value); insertByKey(std::move(ref_key), std::move(new_value)); ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) + verifyByteSize(); } -void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, const std::string& value) { +void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, absl::string_view value) { HeaderString ref_key(key); HeaderString new_value; - new_value.setCopy(value.c_str(), value.size()); + new_value.setCopy(value); insertByKey(std::move(ref_key), std::move(new_value)); ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) + verifyByteSize(); } void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) { @@ -443,15 +434,16 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) { return; } HeaderString new_key; - new_key.setCopy(key.get().c_str(), key.get().size()); + new_key.setCopy(key.get()); HeaderString new_value; new_value.setInteger(value); insertByKey(std::move(new_key), std::move(new_value)); ASSERT(new_key.empty()); // NOLINT(bugprone-use-after-move) ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) + verifyByteSize(); } -void HeaderMapImpl::addCopy(const LowerCaseString& key, const std::string& value) { +void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value) { auto* entry = getExistingInline(key.get()); if (entry != nullptr) { const uint64_t added_size = appendToHeader(entry->value(), value); @@ -459,41 +451,68 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, const std::string& value return; } HeaderString new_key; - new_key.setCopy(key.get().c_str(), key.get().size()); + new_key.setCopy(key.get()); HeaderString new_value; - new_value.setCopy(value.c_str(), value.size()); + new_value.setCopy(value); insertByKey(std::move(new_key), std::move(new_value)); ASSERT(new_key.empty()); // NOLINT(bugprone-use-after-move) ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) + verifyByteSize(); +} + +void HeaderMapImpl::append(const LowerCaseString& key, absl::string_view value) { + bool found = false; + for (HeaderEntryImpl& header : headers_) { + if (header.key().getStringView() == key.get()) { + const uint64_t added_size = appendToHeader(header.value(), value); + addSize(added_size); + found = true; + } + } + + if (!found) { + addCopy(key, value); + } + verifyByteSize(); } -void HeaderMapImpl::setReference(const LowerCaseString& key, const std::string& value) { +void HeaderMapImpl::setReference(const LowerCaseString& key, absl::string_view value) { HeaderString ref_key(key); HeaderString ref_value(value); remove(key); insertByKey(std::move(ref_key), std::move(ref_value)); + verifyByteSize(); } -void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, const std::string& value) { +void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, absl::string_view value) { HeaderString ref_key(key); HeaderString new_value; - new_value.setCopy(value.c_str(), value.size()); + new_value.setCopy(value); remove(key); insertByKey(std::move(ref_key), std::move(new_value)); ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) + verifyByteSize(); } -absl::optional HeaderMapImpl::byteSize() const { return cached_byte_size_; } +void HeaderMapImpl::setCopy(const LowerCaseString& key, absl::string_view value) { + // Replaces a header if it exists, otherwise adds by copy. + bool found = false; + for (HeaderEntryImpl& header : headers_) { + if (header.key().getStringView() == key.get()) { + updateSize(header.value().size(), value.size()); + header.value(value); + found = true; + } + } -uint64_t HeaderMapImpl::refreshByteSize() { - if (!cached_byte_size_.has_value()) { - // In this case, the cached byte size is not valid, and the byte size is computed via an - // iteration over the HeaderMap. The cached byte size is updated. - cached_byte_size_ = byteSizeInternal(); + if (!found) { + addCopy(key, value); } - return cached_byte_size_.value(); + verifyByteSize(); } +uint64_t HeaderMapImpl::byteSize() const { return cached_byte_size_; } + uint64_t HeaderMapImpl::byteSizeInternal() const { // Computes the total byte size by summing the byte size of the keys and values. uint64_t byte_size = 0; @@ -514,17 +533,6 @@ const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { return nullptr; } -HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) { - for (HeaderEntryImpl& header : headers_) { - if (header.key() == key.get().c_str()) { - cached_byte_size_.reset(); - return &header; - } - } - - return nullptr; -} - void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const { for (const HeaderEntryImpl& header : headers_) { if (cb(header, context) == HeaderMap::Iterate::Break) { @@ -564,6 +572,12 @@ HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key, } } +void HeaderMapImpl::clear() { + inline_headers_.clear(); + headers_.clear(); + cached_byte_size_ = 0; +} + void HeaderMapImpl::remove(const LowerCaseString& key) { EntryCb cb = ConstSingleton::get().find(key.get()); if (cb) { @@ -579,6 +593,7 @@ void HeaderMapImpl::remove(const LowerCaseString& key) { } } } + verifyByteSize(); } void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { @@ -602,6 +617,7 @@ void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { } return to_remove; }); + verifyByteSize(); } void HeaderMapImpl::dumpState(std::ostream& os, int indent_level) const { @@ -665,6 +681,7 @@ void HeaderMapImpl::removeInline(HeaderEntryImpl** ptr_to_entry) { subtractSize(size_to_subtract); *ptr_to_entry = nullptr; headers_.erase(entry->entry_); + verifyByteSize(); } } // namespace Http diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index b46370ca5b14a..ef78fe41be352 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -16,38 +16,33 @@ namespace Http { /** * These are definitions of all of the inline header access functions described inside header_map.h - * - * When a non-const reference or pointer to a HeaderEntry is returned, the internal byte size count - * will be cleared, since HeaderMap will no longer be able to accurately update the size of that - * HeaderEntry. - * TODO(asraa): Remove functions with a non-const HeaderEntry return value. */ #define DEFINE_INLINE_HEADER_FUNCS(name) \ public: \ const HeaderEntry* name() const override { return inline_headers_.name##_; } \ - HeaderEntry* name() override { \ - cached_byte_size_.reset(); \ - return inline_headers_.name##_; \ - } \ - HeaderEntry& insert##name() override { \ - cached_byte_size_.reset(); \ - return maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ + void append##name(absl::string_view data, absl::string_view delimiter) override { \ + HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ + addSize(HeaderMapImpl::appendToHeader(entry.value(), data, delimiter)); \ + verifyByteSize(); \ } \ void setReference##name(absl::string_view value) override { \ HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ updateSize(entry.value().size(), value.size()); \ entry.value().setReference(value); \ + verifyByteSize(); \ } \ void set##name(absl::string_view value) override { \ HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ updateSize(entry.value().size(), value.size()); \ entry.value().setCopy(value); \ + verifyByteSize(); \ } \ void set##name(uint64_t value) override { \ HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ subtractSize(inline_headers_.name##_->value().size()); \ entry.value().setInteger(value); \ addSize(inline_headers_.name##_->value().size()); \ + verifyByteSize(); \ } \ void remove##name() override { removeInline(&inline_headers_.name##_); } @@ -62,14 +57,6 @@ public: */ class HeaderMapImpl : public HeaderMap, NonCopyable { public: - /** - * Appends data to header. If header already has a value, the string ',' is added between the - * existing value and data. - * @param header the header to append to. - * @param data to append to the header. - */ - static uint64_t appendToHeader(HeaderString& header, absl::string_view data); - HeaderMapImpl(); explicit HeaderMapImpl( const std::initializer_list>& values); @@ -89,21 +76,21 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { bool operator!=(const HeaderMapImpl& rhs) const; // Http::HeaderMap - void addReference(const LowerCaseString& key, const std::string& value) override; + void addReference(const LowerCaseString& key, absl::string_view value) override; void addReferenceKey(const LowerCaseString& key, uint64_t value) override; - void addReferenceKey(const LowerCaseString& key, const std::string& value) override; + void addReferenceKey(const LowerCaseString& key, absl::string_view value) override; void addCopy(const LowerCaseString& key, uint64_t value) override; - void addCopy(const LowerCaseString& key, const std::string& value) override; - void setReference(const LowerCaseString& key, const std::string& value) override; - void setReferenceKey(const LowerCaseString& key, const std::string& value) override; - absl::optional byteSize() const override; - uint64_t refreshByteSize() override; - uint64_t byteSizeInternal() const override; + void addCopy(const LowerCaseString& key, absl::string_view value) override; + void append(const LowerCaseString& key, absl::string_view value) override; + void setReference(const LowerCaseString& key, absl::string_view value) override; + void setReferenceKey(const LowerCaseString& key, absl::string_view value) override; + void setCopy(const LowerCaseString& key, absl::string_view value) override; + uint64_t byteSize() const override; const HeaderEntry* get(const LowerCaseString& key) const override; - HeaderEntry* get(const LowerCaseString& key) override; void iterate(ConstIterateCb cb, void* context) const override; void iterateReverse(ConstIterateCb cb, void* context) const override; Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override; + void clear() override; void remove(const LowerCaseString& key) override; void removePrefix(const LowerCaseString& key) override; size_t size() const override { return headers_.size(); } @@ -113,7 +100,6 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { protected: // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. void copyFrom(const HeaderMap& rhs); - void clear() { removePrefix(LowerCaseString("")); } struct HeaderEntryImpl : public HeaderEntry, NonCopyable { HeaderEntryImpl(const LowerCaseString& key); @@ -122,7 +108,6 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { // HeaderEntry const HeaderString& key() const override { return key_; } - void value(const char* value, uint32_t size) override; void value(absl::string_view value) override; void value(uint64_t value) override; void value(const HeaderEntry& header) override; @@ -148,6 +133,8 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { struct StaticLookupTable; // Defined in header_map_impl.cc. struct AllInlineHeaders { + void clear() { memset(this, 0, sizeof(*this)); } + ALL_INLINE_HEADERS(DEFINE_INLINE_HEADER_STRUCT) }; @@ -209,6 +196,10 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { std::list::const_reverse_iterator rend() const { return headers_.rend(); } size_t size() const { return headers_.size(); } bool empty() const { return headers_.empty(); } + void clear() { + headers_.clear(); + pseudo_headers_end_ = headers_.end(); + } private: std::list headers_; @@ -216,6 +207,8 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { }; void insertByKey(HeaderString&& key, HeaderString&& value); + static uint64_t appendToHeader(HeaderString& header, absl::string_view data, + absl::string_view delimiter = ","); HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key); HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key, HeaderString&& value); @@ -229,9 +222,11 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { AllInlineHeaders inline_headers_; HeaderList headers_; - // When present, this holds the internal byte size of the HeaderMap. The value is removed once an - // inline header entry is accessed and updated when refreshByteSize() is called. - absl::optional cached_byte_size_ = 0; + // This holds the internal byte size of the HeaderMap. + uint64_t cached_byte_size_ = 0; + // Performs a manual byte size count. + uint64_t byteSizeInternal() const; + void verifyByteSize() { ASSERT(cached_byte_size_ == byteSizeInternal()); } ALL_INLINE_HEADERS(DEFINE_INLINE_HEADER_FUNCS) }; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 3cb3b24eb3368..8f2d33d29d37b 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -489,10 +489,8 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { header_parsing_state_ = HeaderParsingState::Value; current_header_value_.append(data, length); - // Verify that the cached value in byte size exists. - ASSERT(current_header_map_->byteSize().has_value()); - const uint32_t total = current_header_field_.size() + current_header_value_.size() + - current_header_map_->byteSize().value(); + const uint32_t total = + current_header_field_.size() + current_header_value_.size() + current_header_map_->byteSize(); if (total > (max_headers_kb_ * 1024)) { error_code_ = Http::Code::RequestHeaderFieldsTooLarge; @@ -504,10 +502,6 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { int ConnectionImpl::onHeadersCompleteBase() { ENVOY_CONN_LOG(trace, "headers complete", connection_); completeLastHeader(); - // Validate that the completed HeaderMap's cached byte size exists and is correct. - // This assert iterates over the HeaderMap. - ASSERT(current_header_map_->byteSize().has_value() && - current_header_map_->byteSize() == current_header_map_->byteSizeInternal()); if (!(parser_.http_major == 1 && parser_.http_minor == 1)) { // This is not necessarily true, but it's good enough since higher layers only care if this is // HTTP/1.1 or not. @@ -528,7 +522,7 @@ int ConnectionImpl::onHeadersCompleteBase() { if (new_value.empty()) { current_header_map_->removeConnection(); } else { - current_header_map_->Connection()->value(new_value); + current_header_map_->setConnection(new_value); } } current_header_map_->remove(Headers::get().Http2Settings); diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 4a756b58bcb2f..f69bb7b4b850f 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -509,10 +509,6 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { switch (frame->hd.type) { case NGHTTP2_HEADERS: { - // Verify that the final HeaderMap's byte size is under the limit before decoding headers. - // This assert iterates over the HeaderMap. - ASSERT(stream->headers_->byteSize().has_value() && - stream->headers_->byteSize().value() == stream->headers_->byteSizeInternal()); stream->remote_end_stream_ = frame->hd.flags & NGHTTP2_FLAG_END_STREAM; if (!stream->cookies_.empty()) { HeaderString key(Headers::get().Cookie); @@ -624,12 +620,6 @@ int ConnectionImpl::onFrameSend(const nghttp2_frame* frame) { case NGHTTP2_HEADERS: case NGHTTP2_DATA: { StreamImpl* stream = getStream(frame->hd.stream_id); - if (stream->headers_) { - // Verify that the final HeaderMap's byte size is under the limit before sending frames. - // This assert iterates over the HeaderMap. - ASSERT(stream->headers_->byteSize().has_value() && - stream->headers_->byteSize().value() == stream->headers_->byteSizeInternal()); - } stream->local_end_stream_sent_ = frame->hd.flags & NGHTTP2_FLAG_END_STREAM; break; } @@ -820,9 +810,7 @@ int ConnectionImpl::saveHeader(const nghttp2_frame* frame, HeaderString&& name, } stream->saveHeader(std::move(name), std::move(value)); - // Verify that the cached value in byte size exists. - ASSERT(stream->headers_->byteSize().has_value()); - if (stream->headers_->byteSize().value() > max_headers_kb_ * 1024 || + if (stream->headers_->byteSize() > max_headers_kb_ * 1024 || stream->headers_->size() > max_headers_count_) { // This will cause the library to reset/close the stream. stats_.header_overflow_.inc(); diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 49372fc50dbf8..0266f0b634670 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -29,8 +29,8 @@ absl::optional canonicalizePath(absl::string_view original_path) { } // namespace /* static */ -bool PathUtil::canonicalPath(HeaderEntry& path_header) { - const auto original_path = path_header.value().getStringView(); +bool PathUtil::canonicalPath(HeaderMap& headers) { + const auto original_path = headers.Path()->value().getStringView(); // canonicalPath is supposed to apply on path component in URL instead of :path header const auto query_pos = original_path.find('?'); auto normalized_path_opt = canonicalizePath( @@ -50,12 +50,12 @@ bool PathUtil::canonicalPath(HeaderEntry& path_header) { if (!query_suffix.empty()) { normalized_path.insert(normalized_path.end(), query_suffix.begin(), query_suffix.end()); } - path_header.value(normalized_path); + headers.setPath(normalized_path); return true; } -void PathUtil::mergeSlashes(HeaderEntry& path_header) { - const auto original_path = path_header.value().getStringView(); +void PathUtil::mergeSlashes(HeaderMap& headers) { + const auto original_path = headers.Path()->value().getStringView(); // Only operate on path component in URL. const absl::string_view::size_type query_start = original_path.find('?'); const absl::string_view path = original_path.substr(0, query_start); @@ -65,7 +65,7 @@ void PathUtil::mergeSlashes(HeaderEntry& path_header) { } const absl::string_view prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view(); const absl::string_view suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view(); - path_header.value(absl::StrCat( + headers.setPath(absl::StrCat( prefix, absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), query, suffix)); } diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index a588f39de46eb..8e635ed7d3976 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -11,10 +11,10 @@ namespace Http { class PathUtil { public: // Returns if the normalization succeeds. - // If it is successful, the param will be updated with the normalized path. - static bool canonicalPath(HeaderEntry& path_header); + // If it is successful, the path header in header path will be updated with the normalized path. + static bool canonicalPath(HeaderMap& headers); // Merges two or more adjacent slashes in path part of URI into one. - static void mergeSlashes(HeaderEntry& path_header); + static void mergeSlashes(HeaderMap& headers); }; } // namespace Http diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index c6dad78c85520..c5db36bc5e434 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -73,17 +73,11 @@ void Utility::appendXff(HeaderMap& headers, const Network::Address::Instance& re return; } - HeaderString& header = headers.insertForwardedFor().value(); - const std::string& address_as_string = remote_address.ip()->addressAsString(); - HeaderMapImpl::appendToHeader(header, address_as_string.c_str()); + headers.appendForwardedFor(remote_address.ip()->addressAsString(), ","); } void Utility::appendVia(HeaderMap& headers, const std::string& via) { - HeaderString& header = headers.insertVia().value(); - if (!header.empty()) { - header.append(", ", 2); - } - header.append(via.c_str(), via.size()); + headers.appendVia(via, ", "); } std::string Utility::createSslRedirectPath(const HeaderMap& headers) { @@ -429,7 +423,7 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) { bool keep_header = false; // Determine whether the nominated header contains invalid values - HeaderEntry* nominated_header = NULL; + const HeaderEntry* nominated_header = NULL; if (lcs_header_to_remove == Http::Headers::get().Connection) { // Remove the connection header from the nominated tokens if it's self nominated @@ -483,8 +477,7 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) { } if (keep_header) { - nominated_header->value().setCopy(Http::Headers::get().TEValues.Trailers.data(), - Http::Headers::get().TEValues.Trailers.size()); + headers.setTE(Http::Headers::get().TEValues.Trailers); } } } @@ -504,7 +497,7 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) { if (new_value.empty()) { headers.removeConnection(); } else { - headers.Connection()->value(new_value); + headers.setConnection(new_value); } } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index c3c59669a2f55..172b165dae743 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -424,13 +424,13 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers, } if (!host_rewrite_.empty()) { - headers.Host()->value(host_rewrite_); + headers.setHost(host_rewrite_); } else if (auto_host_rewrite_header_) { - Http::HeaderEntry* header = headers.get(*auto_host_rewrite_header_); + const Http::HeaderEntry* header = headers.get(*auto_host_rewrite_header_); if (header != nullptr) { absl::string_view header_value = header->value().getStringView(); if (!header_value.empty()) { - headers.Host()->value(header_value); + headers.setHost(header_value); } } } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 648a5d4911357..eb48dbff40a6e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -160,26 +160,28 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he // If present, use that value as route timeout and don't override // *x-envoy-expected-rq-timeout-ms* header. At this point *x-envoy-upstream-rq-timeout-ms* // header should have been sanitized by egress Envoy. - Http::HeaderEntry* header_expected_timeout_entry = + const Http::HeaderEntry* header_expected_timeout_entry = request_headers.EnvoyExpectedRequestTimeoutMs(); if (header_expected_timeout_entry) { trySetGlobalTimeout(header_expected_timeout_entry, timeout); } else { - Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + const Http::HeaderEntry* header_timeout_entry = + request_headers.EnvoyUpstreamRequestTimeoutMs(); if (trySetGlobalTimeout(header_timeout_entry, timeout)) { request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } } } else { - Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + const Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); if (trySetGlobalTimeout(header_timeout_entry, timeout)) { request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } } // See if there is a per try/retry timeout. If it's >= global we just ignore it. - Http::HeaderEntry* per_try_timeout_entry = request_headers.EnvoyUpstreamRequestPerTryTimeoutMs(); + const Http::HeaderEntry* per_try_timeout_entry = + request_headers.EnvoyUpstreamRequestPerTryTimeoutMs(); if (per_try_timeout_entry) { if (absl::SimpleAtoi(per_try_timeout_entry->value().getStringView(), &header_timeout)) { timeout.per_try_timeout_ = std::chrono::milliseconds(header_timeout); @@ -209,8 +211,7 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he // in grpc-timeout, ensuring that the upstream gRPC server is aware of the actual timeout. // If the expected timeout is 0 set no timeout, as Envoy treats 0 as infinite timeout. if (grpc_request && route.maxGrpcTimeout() && expected_timeout != 0) { - Grpc::Common::toGrpcTimeout(std::chrono::milliseconds(expected_timeout), - request_headers.insertGrpcTimeout().value()); + Grpc::Common::toGrpcTimeout(std::chrono::milliseconds(expected_timeout), request_headers); } return timeout; @@ -233,7 +234,8 @@ FilterUtility::HedgingParams FilterUtility::finalHedgingParams(const RouteEntry& HedgingParams hedging_params; hedging_params.hedge_on_per_try_timeout_ = route.hedgePolicy().hedgeOnPerTryTimeout(); - Http::HeaderEntry* hedge_on_per_try_timeout_entry = request_headers.EnvoyHedgeOnPerTryTimeout(); + const Http::HeaderEntry* hedge_on_per_try_timeout_entry = + request_headers.EnvoyHedgeOnPerTryTimeout(); if (hedge_on_per_try_timeout_entry) { if (hedge_on_per_try_timeout_entry->value() == "true") { hedging_params.hedge_on_per_try_timeout_ = true; @@ -1389,15 +1391,6 @@ Filter::UpstreamRequest::~UpstreamRequest() { stream_info_.setUpstreamTiming(upstream_timing_); stream_info_.onRequestComplete(); - // Prior to logging, refresh the byte size of the HeaderMaps. - // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and - // HeaderMap holds an accurate internal byte size count. - if (upstream_headers_ != nullptr) { - upstream_headers_->refreshByteSize(); - } - if (upstream_trailers_ != nullptr) { - upstream_trailers_->refreshByteSize(); - } for (const auto& upstream_log : parent_.config_.upstream_logs_) { upstream_log->log(parent_.downstream_headers_, upstream_headers_.get(), upstream_trailers_.get(), stream_info_); @@ -1639,7 +1632,7 @@ void Filter::UpstreamRequest::onPoolReady(Http::StreamEncoder& request_encoder, setRequestEncoder(request_encoder); calling_encode_headers_ = true; if (parent_.route_entry_->autoHostRewrite() && !host->hostname().empty()) { - parent_.downstream_headers_->Host()->value(host->hostname()); + parent_.downstream_headers_->setHost(host->hostname()); } if (span_ != nullptr) { diff --git a/source/common/router/router.h b/source/common/router/router.h index 29c4ed8175a4f..c16624a2741a9 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -91,7 +91,7 @@ class FilterUtility { using ParseRetryFlagsFunc = std::function(absl::string_view)>; private: - static HeaderCheckResult hasValidRetryFields(Http::HeaderEntry* header_entry, + static HeaderCheckResult hasValidRetryFields(const Http::HeaderEntry* header_entry, const ParseRetryFlagsFunc& parse_fn) { HeaderCheckResult r; if (header_entry) { @@ -102,7 +102,7 @@ class FilterUtility { return r; } - static HeaderCheckResult isInteger(Http::HeaderEntry* header_entry) { + static HeaderCheckResult isInteger(const Http::HeaderEntry* header_entry) { HeaderCheckResult r; if (header_entry) { uint64_t out; diff --git a/source/common/router/shadow_writer_impl.cc b/source/common/router/shadow_writer_impl.cc index 1597fbb8486b7..73906beb91a2d 100644 --- a/source/common/router/shadow_writer_impl.cc +++ b/source/common/router/shadow_writer_impl.cc @@ -25,7 +25,7 @@ void ShadowWriterImpl::shadow(const std::string& cluster, Http::MessagePtr&& req // Switch authority to add a shadow postfix. This allows upstream logging to make more sense. auto parts = StringUtil::splitToken(request->headers().Host()->value().getStringView(), ":"); ASSERT(!parts.empty() && parts.size() <= 2); - request->headers().Host()->value( + request->headers().setHost( parts.size() == 2 ? absl::StrJoin(parts, "-shadow:") : absl::StrCat(request->headers().Host()->value().getStringView(), "-shadow")); diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index bb35f32277b70..c1aad5a96ca2e 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -638,8 +638,7 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() { headers_message->headers().setReferenceUserAgent( Http::Headers::get().UserAgentValues.EnvoyHealthChecker); - Grpc::Common::toGrpcTimeout(parent_.timeout_, - headers_message->headers().insertGrpcTimeout().value()); + Grpc::Common::toGrpcTimeout(parent_.timeout_, headers_message->headers()); Router::FilterUtility::setUpstreamScheme( headers_message->headers(), host_->transportSocketFactory().implementsSecureTransport()); diff --git a/source/extensions/access_loggers/common/access_log_base.h b/source/extensions/access_loggers/common/access_log_base.h index abe7f009d66f0..9a6d6caa6668a 100644 --- a/source/extensions/access_loggers/common/access_log_base.h +++ b/source/extensions/access_loggers/common/access_log_base.h @@ -27,11 +27,6 @@ class ImplBase : public AccessLog::Instance { /** * Log a completed request if the underlying AccessLog `filter_` allows it. - * - * Prior to logging, call refreshByteSize() on HeaderMaps to ensure that an accurate byte size - * count is logged. - * TODO(asraa): Remove refreshByteSize() requirement when entries in HeaderMap can no longer be - * modified by reference and HeaderMap holds an accurate internal byte size count. */ void log(const Http::HeaderMap* request_headers, const Http::HeaderMap* response_headers, const Http::HeaderMap* response_trailers, diff --git a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc index 8c3bb139d069c..1eda50f092746 100644 --- a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc @@ -98,9 +98,7 @@ void HttpGrpcAccessLog::emitLog(const Http::HeaderMap& request_headers, request_properties->set_original_path( std::string(request_headers.EnvoyOriginalPath()->value().getStringView())); } - // TODO(asraa): This causes a manual iteration over the request_headers. Instead, refresh the byte - // size of this HeaderMap outside the loggers and use byteSize(). - request_properties->set_request_headers_bytes(request_headers.byteSizeInternal()); + request_properties->set_request_headers_bytes(request_headers.byteSize()); request_properties->set_request_body_bytes(stream_info.bytesReceived()); if (request_headers.Method() != nullptr) { envoy::api::v2::core::RequestMethod method = @@ -128,8 +126,7 @@ void HttpGrpcAccessLog::emitLog(const Http::HeaderMap& request_headers, if (stream_info.responseCodeDetails()) { response_properties->set_response_code_details(stream_info.responseCodeDetails().value()); } - ASSERT(response_headers.byteSize().has_value()); - response_properties->set_response_headers_bytes(response_headers.byteSize().value()); + response_properties->set_response_headers_bytes(response_headers.byteSize()); response_properties->set_response_body_bytes(stream_info.bytesSent()); if (!response_headers_to_log_.empty()) { auto* logged_headers = response_properties->mutable_response_headers(); diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 7b2912e2b4d15..3b83f67c6db4c 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -107,7 +107,7 @@ absl::optional RequestWrapper::operator[](CelValue key) const { } else if (value == UserAgent) { return convertHeaderEntry(headers_.value_->UserAgent()); } else if (value == TotalSize) { - return CelValue::CreateInt64(info_.bytesReceived() + headers_.value_->byteSize().value()); + return CelValue::CreateInt64(info_.bytesReceived() + headers_.value_->byteSize()); } } return {}; diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc index bbc9526df0867..214f11b87ce78 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc @@ -74,7 +74,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::HeaderMap& headers, b if (config != nullptr) { const auto& host_rewrite = config->hostRewrite(); if (!host_rewrite.empty()) { - headers.Host()->value(host_rewrite); + headers.setHost(host_rewrite); } const auto& host_rewrite_header = config->hostRewriteHeader(); @@ -82,7 +82,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::HeaderMap& headers, b const auto* header = headers.get(host_rewrite_header); if (header != nullptr) { const auto& header_value = header->value().getStringView(); - headers.Host()->value(header_value); + headers.setHost(header_value); } } } diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index ffb336c12f2c5..b3e6c0a7e36ce 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -169,19 +169,14 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { callbacks_->clearRouteCache(); } for (const auto& header : response->headers_to_add) { - ENVOY_STREAM_LOG(trace, " '{}':'{}'", *callbacks_, header.first.get(), header.second); - Http::HeaderEntry* header_to_modify = request_headers_->get(header.first); - if (header_to_modify) { - header_to_modify->value(header.second.c_str(), header.second.size()); - } else { - request_headers_->addCopy(header.first, header.second); - } + ENVOY_STREAM_LOG(trace, "'{}':'{}'", *callbacks_, header.first.get(), header.second); + request_headers_->setCopy(header.first, header.second); } for (const auto& header : response->headers_to_append) { - Http::HeaderEntry* header_to_modify = request_headers_->get(header.first); + const Http::HeaderEntry* header_to_modify = request_headers_->get(header.first); if (header_to_modify) { - ENVOY_STREAM_LOG(trace, " '{}':'{}'", *callbacks_, header.first.get(), header.second); - Http::HeaderMapImpl::appendToHeader(header_to_modify->value(), header.second); + ENVOY_STREAM_LOG(trace, "'{}':'{}'", *callbacks_, header.first.get(), header.second); + request_headers_->append(header.first, header.second); } } if (cluster_) { diff --git a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc index 1bf8bc4a2c2f6..ab947cb9940e1 100644 --- a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc +++ b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc @@ -75,7 +75,7 @@ Http::FilterTrailersStatus Http1BridgeFilter::encodeTrailers(Http::HeaderMap& tr uint64_t grpc_status_code; if (!absl::SimpleAtoi(grpc_status_header->value().getStringView(), &grpc_status_code) || grpc_status_code != 0) { - response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable)); + response_headers_->setStatus(enumToInt(Http::Code::ServiceUnavailable)); } response_headers_->setGrpcStatus(grpc_status_header->value().getStringView()); } diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc index ffec82888466d..b669d6741ba78 100644 --- a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc +++ b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc @@ -57,7 +57,7 @@ void adjustContentLength(Http::HeaderMap& headers, if (length_header != nullptr) { uint64_t length; if (absl::SimpleAtoi(length_header->value().getStringView(), &length)) { - length_header->value(adjustment(length)); + headers.setContentLength(adjustment(length)); } } } @@ -137,7 +137,7 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::HeaderMap& headers, bool) headers.setStatus(enumToInt(Http::Code::OK)); if (content_type != nullptr) { - content_type->value(content_type_); + headers.setContentType(content_type_); } decoder_callbacks_->streamInfo().setResponseCodeDetails( @@ -146,7 +146,7 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::HeaderMap& headers, bool) } // Restore the content-type to match what the downstream sent. - content_type->value(content_type_); + headers.setContentType(content_type_); if (withhold_grpc_frames_) { // Adjust content-length to account for the frame header that's added. diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 345b0bf4e4eed..a89960942d60a 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -487,9 +487,9 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& bool is_trailers_only_response = response_headers_ == &trailers; if (!grpc_status || grpc_status.value() == Grpc::Status::WellKnownGrpcStatus::InvalidCode) { - response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable)); + response_headers_->setStatus(enumToInt(Http::Code::ServiceUnavailable)); } else { - response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status.value())); + response_headers_->setStatus(Grpc::Utility::grpcToHttpStatus(grpc_status.value())); if (!is_trailers_only_response) { response_headers_->setGrpcStatus(grpc_status.value()); } @@ -594,7 +594,7 @@ bool JsonTranscoderFilter::maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_ return false; } - response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status)); + response_headers_->setStatus(Grpc::Utility::grpcToHttpStatus(grpc_status)); bool is_trailers_only_response = response_headers_ == &trailers; if (is_trailers_only_response) { diff --git a/source/extensions/filters/http/health_check/health_check.cc b/source/extensions/filters/http/health_check/health_check.cc index f22f39d1c8b4c..e66f82f8fe7be 100644 --- a/source/extensions/filters/http/health_check/health_check.cc +++ b/source/extensions/filters/http/health_check/health_check.cc @@ -178,7 +178,7 @@ void HealthCheckFilter::onComplete() { final_status, "", [degraded](auto& headers) { if (degraded) { - headers.insertEnvoyDegraded(); + headers.setEnvoyDegraded(""); } }, absl::nullopt, *details); diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc index 6f92aac73a01d..f4449bdb0315b 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc @@ -78,7 +78,7 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::HeaderMap& header if (!tags.empty()) { const std::string tags_join = absl::StrJoin(tags, ","); - Http::HeaderMapImpl::appendToHeader(headers.insertEnvoyIpTags().value(), tags_join); + headers.appendEnvoyIpTags(tags_join, ","); // We must clear the route cache or else we can't match on x-envoy-ip-tags. // TODO(rgs): this should either be configurable, because it's expensive, or optimized. diff --git a/source/extensions/filters/http/lua/wrappers.cc b/source/extensions/filters/http/lua/wrappers.cc index 4874a716cec99..a772f3c1edfe1 100644 --- a/source/extensions/filters/http/lua/wrappers.cc +++ b/source/extensions/filters/http/lua/wrappers.cc @@ -79,12 +79,7 @@ int HeaderMapWrapper::luaReplace(lua_State* state) { const char* value = luaL_checkstring(state, 3); const Http::LowerCaseString lower_key(key); - Http::HeaderEntry* entry = headers_.get(lower_key); - if (entry != nullptr) { - entry->value(value, strlen(value)); - } else { - headers_.addCopy(lower_key, value); - } + headers_.setCopy(lower_key, value); return 0; } diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index f184234f4d4cd..a750dd47cc47c 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -73,10 +73,6 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head if (shadow_engine != nullptr) { std::string shadow_resp_code = Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultAllowed; - // Refresh headers byte size before checking if allowed. - // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and - // HeaderMap holds an accurate internal byte size count. - headers.refreshByteSize(); if (shadow_engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), &effective_policy_id)) { ENVOY_LOG(debug, "shadow allowed"); @@ -106,10 +102,6 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head const auto engine = config_->engine(callbacks_->route(), Filters::Common::RBAC::EnforcementMode::Enforced); if (engine != nullptr) { - // Refresh headers byte size before checking if allowed. - // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and - // HeaderMap holds an accurate internal byte size count. - headers.refreshByteSize(); if (engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), nullptr)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); diff --git a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc index 38c798551bd6c..e5445320a4c9a 100644 --- a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc +++ b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc @@ -26,7 +26,7 @@ class OpenTracingHTTPHeadersWriter : public opentracing::HTTPHeadersWriter { opentracing::string_view value) const override { Http::LowerCaseString lowercase_key{key}; request_headers_.remove(lowercase_key); - request_headers_.addCopy(std::move(lowercase_key), value); + request_headers_.addCopy(std::move(lowercase_key), {value.data(), value.size()}); return {}; } diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 88761b4026766..3059ed6a303b1 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -107,28 +107,28 @@ TEST(GrpcCommonTest, GrpcStatusDetailsBin) { } TEST(GrpcContextTest, ToGrpcTimeout) { - Http::HeaderString value; + Http::TestHeaderMapImpl headers; - Common::toGrpcTimeout(std::chrono::milliseconds(0UL), value); - EXPECT_EQ("0m", value.getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(0UL), headers); + EXPECT_EQ("0m", headers.GrpcTimeout()->value().getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(1UL), value); - EXPECT_EQ("1m", value.getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(1UL), headers); + EXPECT_EQ("1m", headers.GrpcTimeout()->value().getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(100000000UL), value); - EXPECT_EQ("100000S", value.getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(100000000UL), headers); + EXPECT_EQ("100000S", headers.GrpcTimeout()->value().getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(100000000000UL), value); - EXPECT_EQ("1666666M", value.getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(100000000000UL), headers); + EXPECT_EQ("1666666M", headers.GrpcTimeout()->value().getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(9000000000000UL), value); - EXPECT_EQ("2500000H", value.getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(9000000000000UL), headers); + EXPECT_EQ("2500000H", headers.GrpcTimeout()->value().getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(360000000000000UL), value); - EXPECT_EQ("99999999H", value.getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(360000000000000UL), headers); + EXPECT_EQ("99999999H", headers.GrpcTimeout()->value().getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(UINT64_MAX), value); - EXPECT_EQ("99999999H", value.getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(UINT64_MAX), headers); + EXPECT_EQ("99999999H", headers.GrpcTimeout()->value().getStringView()); } TEST(GrpcContextTest, PrepareHeaders) { diff --git a/test/common/http/header_map_impl_fuzz.proto b/test/common/http/header_map_impl_fuzz.proto index 9ff5279f022fa..bebe373b6ae5b 100644 --- a/test/common/http/header_map_impl_fuzz.proto +++ b/test/common/http/header_map_impl_fuzz.proto @@ -51,6 +51,25 @@ message GetAndMutate { } } +message MutateAndMove { + string key = 1; + oneof mutate_selector { + string append = 2; + string set_copy = 3; + uint64 set_integer = 4; + string set_reference = 5; + } +} + +message Append { + string key = 1; + string value = 2; +} + +message Get { + string key = 1; +} + message Action { oneof action_selector { option (validate.required) = true; @@ -59,7 +78,10 @@ message Action { AddCopy add_copy = 3; SetReference set_reference = 4; SetReferenceKey set_reference_key = 5; - GetAndMutate get_and_mutate = 6; + GetAndMutate get_and_mutate = 6 [deprecated = true]; + Get get = 13; + MutateAndMove mutate_and_move = 12; + Append append = 11; google.protobuf.Empty copy = 7; string lookup = 8; string remove = 9; diff --git a/test/common/http/header_map_impl_fuzz_test.cc b/test/common/http/header_map_impl_fuzz_test.cc index b10c2f695ad84..1c68488e462f4 100644 --- a/test/common/http/header_map_impl_fuzz_test.cc +++ b/test/common/http/header_map_impl_fuzz_test.cc @@ -15,24 +15,15 @@ namespace Envoy { // Fuzz the header map implementation. DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) { Http::HeaderMapImplPtr header_map = std::make_unique(); - const auto predefined_exists = [&header_map](const std::string& s) -> bool { - const Http::HeaderEntry* entry; - return header_map->lookup(Http::LowerCaseString(replaceInvalidCharacters(s)), &entry) == - Http::HeaderMap::Lookup::Found; - }; std::vector> lower_case_strings; std::vector> strings; - constexpr auto max_actions = 1024; + constexpr auto max_actions = 128; for (int i = 0; i < std::min(max_actions, input.actions().size()); ++i) { const auto& action = input.actions(i); ENVOY_LOG_MISC(debug, "Action {}", action.DebugString()); switch (action.action_selector_case()) { case test::common::http::Action::kAddReference: { const auto& add_reference = action.add_reference(); - // Workaround for https://github.com/envoyproxy/envoy/issues/3919. - if (predefined_exists(add_reference.key())) { - continue; - } lower_case_strings.emplace_back( std::make_unique(replaceInvalidCharacters(add_reference.key()))); strings.emplace_back( @@ -42,10 +33,6 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) } case test::common::http::Action::kAddReferenceKey: { const auto& add_reference_key = action.add_reference_key(); - // Workaround for https://github.com/envoyproxy/envoy/issues/3919. - if (predefined_exists(add_reference_key.key())) { - continue; - } lower_case_strings.emplace_back(std::make_unique( replaceInvalidCharacters(add_reference_key.key()))); switch (add_reference_key.value_selector_case()) { @@ -63,10 +50,6 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) } case test::common::http::Action::kAddCopy: { const auto& add_copy = action.add_copy(); - // Workaround for https://github.com/envoyproxy/envoy/issues/3919. - if (predefined_exists(add_copy.key())) { - continue; - } const Http::LowerCaseString key{replaceInvalidCharacters(add_copy.key())}; switch (add_copy.value_selector_case()) { case test::common::http::AddCopy::kStringValue: @@ -97,47 +80,58 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) replaceInvalidCharacters(set_reference_key.value())); break; } - case test::common::http::Action::kGetAndMutate: { - const auto& get_and_mutate = action.get_and_mutate(); - auto* header_entry = - header_map->get(Http::LowerCaseString(replaceInvalidCharacters(get_and_mutate.key()))); + case test::common::http::Action::kGet: { + const auto& get = action.get(); + const auto* header_entry = + header_map->get(Http::LowerCaseString(replaceInvalidCharacters(get.key()))); if (header_entry != nullptr) { // Do some read-only stuff. (void)strlen(std::string(header_entry->key().getStringView()).c_str()); (void)strlen(std::string(header_entry->value().getStringView()).c_str()); - (void)strlen(header_entry->value().buffer()); header_entry->key().empty(); header_entry->value().empty(); - // Do some mutation or parameterized action. - switch (get_and_mutate.mutate_selector_case()) { - case test::common::http::GetAndMutate::kAppend: - header_entry->value().append(replaceInvalidCharacters(get_and_mutate.append()).c_str(), - get_and_mutate.append().size()); - break; - case test::common::http::GetAndMutate::kClear: - header_entry->value().clear(); - break; - case test::common::http::GetAndMutate::kFind: - header_entry->value().getStringView().find(get_and_mutate.find()); - break; - case test::common::http::GetAndMutate::kSetCopy: - header_entry->value().setCopy(replaceInvalidCharacters(get_and_mutate.set_copy()).c_str(), - get_and_mutate.set_copy().size()); - break; - case test::common::http::GetAndMutate::kSetInteger: - header_entry->value().setInteger(get_and_mutate.set_integer()); - break; - case test::common::http::GetAndMutate::kSetReference: - strings.emplace_back(std::make_unique( - replaceInvalidCharacters(get_and_mutate.set_reference()))); - header_entry->value().setReference(*strings.back()); - break; - default: - break; - } } break; } + case test::common::http::Action::kMutateAndMove: { + const auto& mutate_and_move = action.mutate_and_move(); + Http::HeaderString header_field( + Http::LowerCaseString(replaceInvalidCharacters(mutate_and_move.key()))); + Http::HeaderString header_value; + // Do some mutation or parameterized action. + switch (mutate_and_move.mutate_selector_case()) { + case test::common::http::MutateAndMove::kAppend: + header_value.append(replaceInvalidCharacters(mutate_and_move.append()).c_str(), + mutate_and_move.append().size()); + break; + case test::common::http::MutateAndMove::kSetCopy: + header_value.setCopy(replaceInvalidCharacters(mutate_and_move.set_copy())); + break; + case test::common::http::MutateAndMove::kSetInteger: + header_value.setInteger(mutate_and_move.set_integer()); + break; + case test::common::http::MutateAndMove::kSetReference: + strings.emplace_back(std::make_unique( + replaceInvalidCharacters(mutate_and_move.set_reference()))); + header_value.setReference(*strings.back()); + break; + default: + break; + } + // Can't addViaMove on an empty header value. + if (!header_value.empty()) { + header_map->addViaMove(std::move(header_field), std::move(header_value)); + } + break; + } + case test::common::http::Action::kAppend: { + const auto& append = action.append(); + lower_case_strings.emplace_back( + std::make_unique(replaceInvalidCharacters(append.key()))); + strings.emplace_back(std::make_unique(replaceInvalidCharacters(append.value()))); + header_map->append(*lower_case_strings.back(), *strings.back()); + break; + } case test::common::http::Action::kCopy: { header_map = std::make_unique( *reinterpret_cast(header_map.get())); @@ -158,13 +152,17 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) Http::LowerCaseString(replaceInvalidCharacters(action.remove_prefix()))); break; } + case test::common::http::Action::kGetAndMutate: { + // Deprecated. Can not get and mutate entries. + break; + } default: // Maybe nothing is set? break; } // Exercise some read-only accessors. - header_map->byteSize(); header_map->size(); + header_map->byteSize(); header_map->iterate( [](const Http::HeaderEntry& header, void * /*context*/) -> Http::HeaderMap::Iterate { header.key(); diff --git a/test/common/http/header_map_impl_speed_test.cc b/test/common/http/header_map_impl_speed_test.cc index b59cb5130c7f7..f313d26b4a173 100644 --- a/test/common/http/header_map_impl_speed_test.cc +++ b/test/common/http/header_map_impl_speed_test.cc @@ -74,7 +74,7 @@ static void HeaderMapImplGetInline(benchmark::State& state) { const std::string value("01234567890123456789"); HeaderMapImpl headers; addDummyHeaders(headers, state.range(0)); - headers.insertConnection().value().setReference(value); + headers.setReferenceConnection(value); size_t size = 0; for (auto _ : state) { size += headers.Connection()->value().size(); @@ -83,21 +83,6 @@ static void HeaderMapImplGetInline(benchmark::State& state) { } BENCHMARK(HeaderMapImplGetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); -/** - * Measure the speed of writing to a header for which HeaderMapImpl is expected to - * provide special optimizations. - */ -static void HeaderMapImplSetInline(benchmark::State& state) { - const std::string value("01234567890123456789"); - HeaderMapImpl headers; - addDummyHeaders(headers, state.range(0)); - for (auto _ : state) { - headers.insertConnection().value().setReference(value); - } - benchmark::DoNotOptimize(headers.size()); -} -BENCHMARK(HeaderMapImplSetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); - /** * Measure the speed of writing to a header for which HeaderMapImpl is expected to * provide special optimizations. @@ -134,7 +119,7 @@ static void HeaderMapImplGetByteSize(benchmark::State& state) { addDummyHeaders(headers, state.range(0)); uint64_t size = 0; for (auto _ : state) { - size += headers.byteSize().value(); + size += headers.byteSize(); } benchmark::DoNotOptimize(size); } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 4ade5f79bbce9..3652091b090d8 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -57,7 +57,7 @@ TEST(HeaderStringTest, All) { // Inline move constructor { HeaderString string; - string.setCopy("hello", 5); + string.setCopy("hello"); EXPECT_EQ(HeaderString::Type::Inline, string.type()); HeaderString string2(std::move(string)); EXPECT_TRUE(string.empty()); // NOLINT(bugprone-use-after-move) @@ -74,7 +74,7 @@ TEST(HeaderStringTest, All) { { std::string large(4096, 'a'); HeaderString string; - string.setCopy(large.c_str(), large.size()); + string.setCopy(large); EXPECT_EQ(HeaderString::Type::Dynamic, string.type()); HeaderString string2(std::move(string)); EXPECT_TRUE(string.empty()); // NOLINT(bugprone-use-after-move) @@ -100,7 +100,7 @@ TEST(HeaderStringTest, All) { { std::string static_string("HELLO"); HeaderString string(static_string); - string.setCopy(static_string.c_str(), static_string.size()); + string.setCopy(static_string); EXPECT_EQ(HeaderString::Type::Inline, string.type()); EXPECT_EQ("HELLO", string.getStringView()); } @@ -127,7 +127,7 @@ TEST(HeaderStringTest, All) { // Copy inline { HeaderString string; - string.setCopy("hello", 5); + string.setCopy("hello"); EXPECT_EQ("hello", string.getStringView()); EXPECT_EQ(5U, string.size()); } @@ -136,7 +136,7 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value(4096, 'a'); - string.setCopy(large_value.c_str(), large_value.size()); + string.setCopy(large_value); EXPECT_EQ(large_value, string.getStringView()); EXPECT_NE(large_value.c_str(), string.getStringView().data()); EXPECT_EQ(4096U, string.size()); @@ -146,9 +146,9 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value1(4096, 'a'); - string.setCopy(large_value1.c_str(), large_value1.size()); + string.setCopy(large_value1); std::string large_value2(2048, 'b'); - string.setCopy(large_value2.c_str(), large_value2.size()); + string.setCopy(large_value2); EXPECT_EQ(large_value2, string.getStringView()); EXPECT_NE(large_value2.c_str(), string.getStringView().data()); EXPECT_EQ(2048U, string.size()); @@ -158,9 +158,9 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value1(4096, 'a'); - string.setCopy(large_value1.c_str(), large_value1.size()); + string.setCopy(large_value1); std::string large_value2(16384, 'b'); - string.setCopy(large_value2.c_str(), large_value2.size()); + string.setCopy(large_value2); EXPECT_EQ(large_value2, string.getStringView()); EXPECT_NE(large_value2.c_str(), string.getStringView().data()); EXPECT_EQ(16384U, string.size()); @@ -170,9 +170,9 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value1(16, 'a'); - string.setCopy(large_value1.c_str(), large_value1.size()); + string.setCopy(large_value1); std::string large_value2(16384, 'b'); - string.setCopy(large_value2.c_str(), large_value2.size()); + string.setCopy(large_value2); EXPECT_EQ(large_value2, string.getStringView()); EXPECT_NE(large_value2.c_str(), string.getStringView().data()); EXPECT_EQ(16384U, string.size()); @@ -187,7 +187,7 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large(128, 'z'); - string.setCopy(large.c_str(), large.size()); + string.setCopy(large); EXPECT_EQ(string.type(), HeaderString::Type::Inline); EXPECT_EQ(string.getStringView(), large); } @@ -196,11 +196,11 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large(128, 'z'); - string.setCopy(large.c_str(), large.size()); + string.setCopy(large); EXPECT_EQ(string.type(), HeaderString::Type::Inline); EXPECT_EQ(string.getStringView(), large); std::string small(1, 'a'); - string.setCopy(small.c_str(), small.size()); + string.setCopy(small); EXPECT_EQ(string.type(), HeaderString::Type::Inline); EXPECT_EQ(string.getStringView(), small); // If we peek past the valid first character of the @@ -218,13 +218,13 @@ TEST(HeaderStringTest, All) { HeaderString string; // Force Dynamic with setCopy of inline buffer size + 1. std::string large1(129, 'z'); - string.setCopy(large1.c_str(), large1.size()); + string.setCopy(large1); EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); const void* dynamic_buffer_address = string.getStringView().data(); // Dynamic capacity in setCopy is 2x required by the size. // So to fill it exactly setCopy with a total of 258 chars. std::string large2(258, 'z'); - string.setCopy(large2.c_str(), large2.size()); + string.setCopy(large2); EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); // The actual buffer address should be the same as it was after // setCopy(large1), ensuring no reallocation occurred. @@ -295,7 +295,7 @@ TEST(HeaderStringTest, All) { HeaderString string; // Force Dynamic with setCopy of inline buffer size + 1. std::string large1(129, 'z'); - string.setCopy(large1.c_str(), large1.size()); + string.setCopy(large1); EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); const void* dynamic_buffer_address = string.getStringView().data(); // Dynamic capacity in setCopy is 2x required by the size. @@ -338,7 +338,7 @@ TEST(HeaderStringTest, All) { EXPECT_EQ(HeaderString::Type::Reference, string.type()); const std::string large(129, 'a'); - string.setCopy(large.c_str(), large.size()); + string.setCopy(large); EXPECT_NE(string.getStringView().data(), large.c_str()); EXPECT_EQ(HeaderString::Type::Dynamic, string.type()); @@ -366,9 +366,8 @@ TEST(HeaderMapImplTest, InlineInsert) { HeaderMapImpl headers; EXPECT_TRUE(headers.empty()); EXPECT_EQ(0, headers.size()); - EXPECT_EQ(headers.byteSize().value(), 0); EXPECT_EQ(nullptr, headers.Host()); - headers.insertHost().value(std::string("hello")); + headers.setHost("hello"); EXPECT_FALSE(headers.empty()); EXPECT_EQ(1, headers.size()); EXPECT_EQ(":authority", headers.Host()->key().getStringView()); @@ -376,17 +375,50 @@ TEST(HeaderMapImplTest, InlineInsert) { EXPECT_EQ("hello", headers.get(Headers::get().Host)->value().getStringView()); } -// Utility function for testing byteSize() against a manual byte count. -uint64_t countBytesForTest(const HeaderMapImpl& headers) { - uint64_t byte_size = 0; - headers.iterate( - [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { - auto* byte_size = static_cast(context); - *byte_size += header.key().getStringView().size() + header.value().getStringView().size(); - return Http::HeaderMap::Iterate::Continue; - }, - &byte_size); - return byte_size; +TEST(HeaderMapImplTest, InlineAppend) { + { + HeaderMapImpl headers; + // Create via header and append. + headers.setVia(""); + headers.appendVia("1.0 fred", ","); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); + headers.appendVia("1.1 nowhere.com", ","); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 nowhere.com"); + } + { + // Append to via header without explicitly creating first. + HeaderMapImpl headers; + headers.appendVia("1.0 fred", ","); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); + headers.appendVia("1.1 nowhere.com", ","); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 nowhere.com"); + } + { + // Custom delimiter. + HeaderMapImpl headers; + headers.setVia(""); + headers.appendVia("1.0 fred", ", "); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); + headers.appendVia("1.1 nowhere.com", ", "); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred, 1.1 nowhere.com"); + } + { + // Append and then later set. + HeaderMapImpl headers; + headers.appendVia("1.0 fred", ","); + headers.appendVia("1.1 nowhere.com", ","); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 nowhere.com"); + headers.setVia("2.0 override"); + EXPECT_EQ(headers.Via()->value().getStringView(), "2.0 override"); + } + { + // Set and then append. This mimics how GrpcTimeout is set. + HeaderMapImpl headers; + headers.setGrpcTimeout(42); + EXPECT_EQ(headers.GrpcTimeout()->value().getStringView(), "42"); + headers.appendGrpcTimeout("s", ""); + EXPECT_EQ(headers.GrpcTimeout()->value().getStringView(), "42s"); + } } TEST(HeaderMapImplTest, MoveIntoInline) { @@ -394,19 +426,18 @@ TEST(HeaderMapImplTest, MoveIntoInline) { HeaderString key; key.setCopy(Headers::get().CacheControl.get()); HeaderString value; - value.setCopy("hello", 5); + value.setCopy("hello"); headers.addViaMove(std::move(key), std::move(value)); EXPECT_EQ("cache-control", headers.CacheControl()->key().getStringView()); EXPECT_EQ("hello", headers.CacheControl()->value().getStringView()); HeaderString key2; - key2.setCopy(Headers::get().CacheControl.get().c_str(), Headers::get().CacheControl.get().size()); + key2.setCopy(Headers::get().CacheControl.get()); HeaderString value2; - value2.setCopy("there", 5); + value2.setCopy("there"); headers.addViaMove(std::move(key2), std::move(value2)); EXPECT_EQ("cache-control", headers.CacheControl()->key().getStringView()); EXPECT_EQ("hello,there", headers.CacheControl()->value().getStringView()); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); } TEST(HeaderMapImplTest, Remove) { @@ -416,7 +447,6 @@ TEST(HeaderMapImplTest, Remove) { LowerCaseString static_key("hello"); std::string ref_value("value"); headers.addReference(static_key, ref_value); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("value", headers.get(static_key)->value().getStringView()); EXPECT_EQ(HeaderString::Type::Reference, headers.get(static_key)->value().type()); EXPECT_EQ(1UL, headers.size()); @@ -425,11 +455,9 @@ TEST(HeaderMapImplTest, Remove) { EXPECT_EQ(nullptr, headers.get(static_key)); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); - EXPECT_EQ(headers.refreshByteSize(), 0); // Add and remove by inline. - headers.insertContentLength().value(5); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); + headers.setContentLength(5); EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); EXPECT_FALSE(headers.empty()); @@ -437,19 +465,16 @@ TEST(HeaderMapImplTest, Remove) { EXPECT_EQ(nullptr, headers.ContentLength()); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); // Add inline and remove by name. - headers.insertContentLength().value(5); + headers.setContentLength(5); EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); EXPECT_FALSE(headers.empty()); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); headers.remove(Headers::get().ContentLength); EXPECT_EQ(nullptr, headers.ContentLength()); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); - EXPECT_EQ(headers.refreshByteSize(), 0); } TEST(HeaderMapImplTest, RemoveRegex) { @@ -467,11 +492,9 @@ TEST(HeaderMapImplTest, RemoveRegex) { headers.addReference(key3, "value"); headers.addReference(key4, "value"); headers.addReference(key5, "value"); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); // Test removing the first header, middle headers, and the end header. headers.removePrefix(LowerCaseString("x-prefix-")); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ(nullptr, headers.get(key1)); EXPECT_NE(nullptr, headers.get(key2)); EXPECT_EQ(nullptr, headers.get(key3)); @@ -479,21 +502,17 @@ TEST(HeaderMapImplTest, RemoveRegex) { EXPECT_EQ(nullptr, headers.get(key5)); // Remove all headers. - headers.refreshByteSize(); headers.removePrefix(LowerCaseString("")); - EXPECT_EQ(headers.byteSize().value(), 0); EXPECT_EQ(nullptr, headers.get(key2)); EXPECT_EQ(nullptr, headers.get(key4)); // Add inline and remove by regex - headers.insertContentLength().value(5); + headers.setContentLength(5); EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); EXPECT_FALSE(headers.empty()); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); headers.removePrefix(LowerCaseString("content")); EXPECT_EQ(nullptr, headers.ContentLength()); - EXPECT_EQ(headers.refreshByteSize(), 0); } TEST(HeaderMapImplTest, SetRemovesAllValues) { @@ -511,7 +530,6 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { headers.addReference(key2, ref_value2); headers.addReference(key1, ref_value3); headers.addReference(key1, ref_value4); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); using MockCb = testing::MockFunction; @@ -559,7 +577,6 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { const std::string bar("bar"); headers.addReference(Headers::get().ContentLength, foo); headers.addReference(Headers::get().ContentLength, bar); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("foo,bar", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -567,7 +584,6 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { HeaderMapImpl headers; headers.addReferenceKey(Headers::get().ContentLength, "foo"); headers.addReferenceKey(Headers::get().ContentLength, "bar"); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("foo,bar", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -575,7 +591,6 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { HeaderMapImpl headers; headers.addReferenceKey(Headers::get().ContentLength, 5); headers.addReferenceKey(Headers::get().ContentLength, 6); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("5,6", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -584,7 +599,6 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { const std::string foo("foo"); headers.addReference(Headers::get().ContentLength, foo); headers.addReferenceKey(Headers::get().ContentLength, 6); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("foo,6", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -600,7 +614,6 @@ TEST(HeaderMapImplTest, DoubleCookieAdd) { headers.addReference(set_cookie, foo); headers.addReference(set_cookie, bar); EXPECT_EQ(2UL, headers.size()); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); std::vector out; Http::HeaderUtility::getAllOfHeader(headers, "set-cookie", out); @@ -613,7 +626,6 @@ TEST(HeaderMapImplTest, DoubleInlineSet) { HeaderMapImpl headers; headers.setReferenceKey(Headers::get().ContentType, "blah"); headers.setReferenceKey(Headers::get().ContentType, "text/html"); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("text/html", headers.ContentType()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -622,7 +634,6 @@ TEST(HeaderMapImplTest, AddReferenceKey) { HeaderMapImpl headers; LowerCaseString foo("hello"); headers.addReferenceKey(foo, "world"); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_NE("world", headers.get(foo)->value().getStringView().data()); EXPECT_EQ("world", headers.get(foo)->value().getStringView()); } @@ -631,24 +642,72 @@ TEST(HeaderMapImplTest, SetReferenceKey) { HeaderMapImpl headers; LowerCaseString foo("hello"); headers.setReferenceKey(foo, "world"); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_NE("world", headers.get(foo)->value().getStringView().data()); EXPECT_EQ("world", headers.get(foo)->value().getStringView()); - headers.refreshByteSize(); headers.setReferenceKey(foo, "monde"); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_NE("monde", headers.get(foo)->value().getStringView().data()); EXPECT_EQ("monde", headers.get(foo)->value().getStringView()); } +TEST(HeaderMapImplTest, SetCopy) { + HeaderMapImpl headers; + LowerCaseString foo("hello"); + headers.setCopy(foo, "world"); + EXPECT_EQ("world", headers.get(foo)->value().getStringView()); + + // Overwrite value. + headers.setCopy(foo, "monde"); + EXPECT_EQ("monde", headers.get(foo)->value().getStringView()); + + // Add another foo header. + headers.addCopy(foo, "monde2"); + EXPECT_EQ(headers.size(), 2); + + // Make sure all foo headers are overridden. + headers.setCopy(foo, "override-monde"); + EXPECT_EQ(headers.size(), 2); + + using MockCb = testing::MockFunction; + MockCb cb; + + InSequence seq; + EXPECT_CALL(cb, Call("hello", "override-monde")); + EXPECT_CALL(cb, Call("hello", "override-monde")); + 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); + + // Test setting an empty string and then overriding. + headers.remove(foo); + EXPECT_EQ(headers.size(), 0); + const std::string empty; + headers.setCopy(foo, empty); + EXPECT_EQ(headers.size(), 1); + headers.setCopy(foo, "not-empty"); + EXPECT_EQ(headers.get(foo)->value().getStringView(), "not-empty"); + + // Use setCopy with inline headers both indirectly and directly. + headers.clear(); + EXPECT_EQ(headers.size(), 0); + headers.setCopy(Headers::get().Path, "/"); + EXPECT_EQ(headers.size(), 1); + EXPECT_EQ(headers.Path()->value().getStringView(), "/"); + headers.setPath("/foo"); + EXPECT_EQ(headers.size(), 1); + EXPECT_EQ(headers.Path()->value().getStringView(), "/foo"); +} + TEST(HeaderMapImplTest, AddCopy) { HeaderMapImpl headers; // Start with a string value. std::unique_ptr lcKeyPtr(new LowerCaseString("hello")); headers.addCopy(*lcKeyPtr, "world"); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); const HeaderString& value = headers.get(*lcKeyPtr)->value(); @@ -669,18 +728,15 @@ TEST(HeaderMapImplTest, AddCopy) { // addReferenceKey and addCopy can both add multiple instances of a // given header, so we need to delete the old "hello" header. // Test that removing will return 0 byte size. - headers.refreshByteSize(); headers.remove(LowerCaseString("hello")); - EXPECT_EQ(headers.byteSize().value(), 0); + EXPECT_EQ(headers.byteSize(), 0); // Build "hello" with string concatenation to make it unlikely that the // compiler is just reusing the same string constant for everything. lcKeyPtr = std::make_unique(std::string("he") + "llo"); EXPECT_STREQ("hello", lcKeyPtr->get().c_str()); - headers.refreshByteSize(); headers.addCopy(*lcKeyPtr, 42); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); const HeaderString& value3 = headers.get(*lcKeyPtr)->value(); @@ -706,20 +762,15 @@ TEST(HeaderMapImplTest, AddCopy) { headers.addCopy(cache_control, "max-age=1345"); EXPECT_EQ("max-age=1345", headers.get(cache_control)->value().getStringView()); EXPECT_EQ("max-age=1345", headers.CacheControl()->value().getStringView()); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); headers.addCopy(cache_control, "public"); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); EXPECT_EQ("max-age=1345,public", headers.get(cache_control)->value().getStringView()); headers.addCopy(cache_control, ""); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); EXPECT_EQ("max-age=1345,public", headers.get(cache_control)->value().getStringView()); headers.addCopy(cache_control, 123); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); EXPECT_EQ("max-age=1345,public,123", headers.get(cache_control)->value().getStringView()); headers.addCopy(cache_control, std::numeric_limits::max()); EXPECT_EQ("max-age=1345,public,123,18446744073709551615", headers.get(cache_control)->value().getStringView()); - EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); } TEST(HeaderMapImplTest, Equality) { @@ -739,7 +790,6 @@ TEST(HeaderMapImplTest, LargeCharInHeader) { LowerCaseString static_key("\x90hello"); std::string ref_value("value"); headers.addReference(static_key, ref_value); - EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("value", headers.get(static_key)->value().getStringView()); } @@ -797,7 +847,7 @@ TEST(HeaderMapImplTest, IterateReverse) { TEST(HeaderMapImplTest, Lookup) { TestHeaderMapImpl headers; headers.addCopy("hello", "world"); - headers.insertContentLength().value(5); + headers.setContentLength(5); // Lookup is not supported for non predefined inline headers. { @@ -831,10 +881,12 @@ TEST(HeaderMapImplTest, Get) { { TestHeaderMapImpl headers{{":path", "/"}, {"hello", "world"}}; - headers.get(LowerCaseString(":path"))->value(std::string("/new_path")); + // There is not HeaderMap method to set a header and copy both the key and value. + headers.setReferenceKey(LowerCaseString(":path"), "/new_path"); EXPECT_EQ("/new_path", headers.get(LowerCaseString(":path"))->value().getStringView()); - headers.get(LowerCaseString("hello"))->value(std::string("world2")); - EXPECT_EQ("world2", headers.get(LowerCaseString("hello"))->value().getStringView()); + LowerCaseString foo("hello"); + headers.setReferenceKey(foo, "world2"); + EXPECT_EQ("world2", headers.get(foo)->value().getStringView()); EXPECT_EQ(nullptr, headers.get(LowerCaseString("foo"))); } } @@ -842,41 +894,57 @@ TEST(HeaderMapImplTest, Get) { TEST(HeaderMapImplTest, TestAppendHeader) { // Test appending to a string with a value. { - HeaderString value1; - value1.setCopy("some;", 5); - HeaderMapImpl::appendToHeader(value1, "test"); - EXPECT_EQ(value1, "some;,test"); + TestHeaderMapImpl headers; + LowerCaseString foo("key1"); + headers.addCopy(foo, "some;"); + headers.append(foo, "test"); + EXPECT_EQ(headers.get(foo)->value().getStringView(), "some;,test"); } // Test appending to an empty string. { - HeaderString value2; - HeaderMapImpl::appendToHeader(value2, "my tag data"); - EXPECT_EQ(value2, "my tag data"); + TestHeaderMapImpl headers; + LowerCaseString key2("key2"); + headers.append(key2, "my tag data"); + EXPECT_EQ(headers.get(key2)->value().getStringView(), "my tag data"); } // Test empty data case. { - HeaderString value3; - value3.setCopy("empty", 5); - HeaderMapImpl::appendToHeader(value3, ""); - EXPECT_EQ(value3, "empty"); + TestHeaderMapImpl headers; + LowerCaseString key3("key3"); + headers.addCopy(key3, "empty"); + headers.append(key3, ""); + EXPECT_EQ(headers.get(key3)->value().getStringView(), "empty"); } // Regression test for appending to an empty string with a short string, then // setting integer. { + TestHeaderMapImpl headers; const std::string empty; - HeaderString value4(empty); - HeaderMapImpl::appendToHeader(value4, " "); - value4.setInteger(0); - EXPECT_EQ("0", value4.getStringView()); - EXPECT_EQ(1U, value4.size()); + headers.setPath(empty); + // Append with default delimiter. + headers.appendPath(" ", ","); + headers.setPath(0); + EXPECT_EQ("0", headers.Path()->value().getStringView()); + EXPECT_EQ(1U, headers.Path()->value().size()); + } + // Test append for inline headers using this method and append##name. + { + TestHeaderMapImpl headers; + headers.addCopy(Headers::get().Via, "1.0 fred"); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); + headers.append(Headers::get().Via, "1.1 p.example.net"); + EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 p.example.net"); + headers.appendVia("1.1 new.example.net", ","); + EXPECT_EQ(headers.Via()->value().getStringView(), + "1.0 fred,1.1 p.example.net,1.1 new.example.net"); } } TEST(HeaderMapImplDeathTest, TestHeaderLengthChecks) { HeaderString value; - value.setCopy("some;", 5); + value.setCopy("some;"); EXPECT_DEATH_LOG_TO_STDERR(value.append(nullptr, std::numeric_limits::max()), "Trying to allocate overly large headers."); @@ -894,7 +962,6 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { { LowerCaseString foo("hello"); Http::TestHeaderMapImpl headers{}; - EXPECT_EQ(headers.refreshByteSize(), 0); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); @@ -1118,51 +1185,98 @@ TEST(HeaderMapImplTest, TestInlineHeaderAdd) { EXPECT_TRUE(foo.Path() != nullptr); } +TEST(HeaderMapImplTest, ClearHeaderMap) { + HeaderMapImpl headers; + LowerCaseString static_key("hello"); + std::string ref_value("value"); + + // Add random header and then clear. + headers.addReference(static_key, ref_value); + EXPECT_EQ("value", headers.get(static_key)->value().getStringView()); + EXPECT_EQ(HeaderString::Type::Reference, headers.get(static_key)->value().type()); + EXPECT_EQ(1UL, headers.size()); + EXPECT_FALSE(headers.empty()); + headers.clear(); + EXPECT_EQ(nullptr, headers.get(static_key)); + EXPECT_EQ(0UL, headers.size()); + EXPECT_EQ(headers.byteSize(), 0); + EXPECT_TRUE(headers.empty()); + + // Add inline and clear. + headers.setContentLength(5); + EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); + EXPECT_EQ(1UL, headers.size()); + EXPECT_FALSE(headers.empty()); + headers.clear(); + EXPECT_EQ(nullptr, headers.ContentLength()); + EXPECT_EQ(0UL, headers.size()); + EXPECT_EQ(headers.byteSize(), 0); + EXPECT_TRUE(headers.empty()); + + // Add mixture of headers. + headers.addReference(static_key, ref_value); + headers.setContentLength(5); + headers.addCopy(static_key, "new_value"); + EXPECT_EQ(3UL, headers.size()); + EXPECT_FALSE(headers.empty()); + headers.clear(); + EXPECT_EQ(nullptr, headers.ContentLength()); + EXPECT_EQ(0UL, headers.size()); + EXPECT_EQ(headers.byteSize(), 0); + EXPECT_TRUE(headers.empty()); +} + // Validates byte size is properly accounted for in different inline header setting scenarios. TEST(HeaderMapImplTest, InlineHeaderByteSize) { - uint64_t hostKeySize = Headers::get().Host.get().size(); - uint64_t statusKeySize = Headers::get().Status.get().size(); { HeaderMapImpl headers; std::string foo = "foo"; - EXPECT_EQ(headers.byteSize().value(), 0); headers.setHost(foo); - EXPECT_EQ(headers.byteSize().value(), foo.size() + hostKeySize); + EXPECT_EQ(headers.byteSize(), 13); } { - // Overwrite an inline headers. + // Overwrite an inline headers with set. HeaderMapImpl headers; std::string foo = "foo"; - EXPECT_EQ(headers.byteSize().value(), 0); headers.setHost(foo); - EXPECT_EQ(headers.byteSize().value(), foo.size() + hostKeySize); std::string big_foo = "big_foo"; headers.setHost(big_foo); - EXPECT_EQ(headers.byteSize().value(), big_foo.size() + hostKeySize); + EXPECT_EQ(headers.byteSize(), 17); } { - // Overwrite an inline headers with reference value and clear. + // Overwrite an inline headers with setReference and clear. HeaderMapImpl headers; std::string foo = "foo"; - EXPECT_EQ(headers.byteSize().value(), 0); headers.setHost(foo); - EXPECT_EQ(headers.byteSize().value(), foo.size() + hostKeySize); std::string big_foo = "big_foo"; headers.setReferenceHost(big_foo); - EXPECT_EQ(headers.byteSize().value(), big_foo.size() + hostKeySize); + EXPECT_EQ(headers.byteSize(), 17); headers.removeHost(); - EXPECT_EQ(headers.byteSize().value(), 0); + EXPECT_EQ(headers.byteSize(), 0); + } + { + // Overwrite an inline headers with set integer value. + HeaderMapImpl headers; + uint64_t status = 200; + headers.setStatus(status); + EXPECT_EQ(headers.byteSize(), 10); + uint64_t newStatus = 500; + headers.setStatus(newStatus); + EXPECT_EQ(headers.byteSize(), 10); + headers.removeStatus(); + EXPECT_EQ(headers.byteSize(), 0); } { - // Overwrite an inline headers with integer value. + // Set an inline header, remove, and rewrite. HeaderMapImpl headers; uint64_t status = 200; - EXPECT_EQ(headers.byteSize().value(), 0); headers.setStatus(status); - EXPECT_EQ(headers.byteSize().value(), 3 + statusKeySize); + EXPECT_EQ(headers.byteSize(), 10); + headers.removeStatus(); + EXPECT_EQ(headers.byteSize(), 0); uint64_t newStatus = 500; headers.setStatus(newStatus); - EXPECT_EQ(headers.byteSize().value(), 3 + statusKeySize); + EXPECT_EQ(headers.byteSize(), 10); } } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index ea9d5c4809569..c2784c8dcf19c 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1143,19 +1143,17 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAtLimitAccepted) { TestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); - // Refresh byte size after adding default inline headers by reference. - request_headers.refreshByteSize(); std::string key = "big"; uint32_t head_room = 77; uint32_t long_string_length = - codec_limit_kb * 1024 - request_headers.byteSize().value() - key.length() - head_room; + codec_limit_kb * 1024 - request_headers.byteSize() - key.length() - head_room; std::string long_string = std::string(long_string_length, 'q'); request_headers.addCopy(key, long_string); // The amount of data sent to the codec is not equivalent to the size of the // request headers that Envoy computes, as the codec limits based on the // entire http2 frame. The exact head room needed (76) was found through iteration. - ASSERT_EQ(request_headers.byteSize().value() + head_room, codec_limit_kb * 1024); + ASSERT_EQ(request_headers.byteSize() + head_room, codec_limit_kb * 1024); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); request_encoder_->encodeHeaders(request_headers, true); diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index 7f82742403884..c2a7230f63005 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -14,7 +14,7 @@ class PathUtilityTest : public testing::Test { // This is an indirect way to build a header entry for // PathUtil::canonicalPath(), since we don't have direct access to the // HeaderMapImpl constructor. - HeaderEntry& pathHeaderEntry(const std::string& path_value) { + const HeaderEntry& pathHeaderEntry(const std::string& path_value) { headers_.setPath(path_value); return *headers_.Path(); } @@ -26,7 +26,7 @@ TEST_F(PathUtilityTest, AlreadyNormalPaths) { const std::vector normal_paths{"/xyz", "/x/y/z"}; for (const auto& path : normal_paths) { auto& path_header = pathHeaderEntry(path); - const auto result = PathUtil::canonicalPath(path_header); + const auto result = PathUtil::canonicalPath(headers_); EXPECT_TRUE(result) << "original path: " << path; EXPECT_EQ(path_header.value().getStringView(), absl::string_view(path)); } @@ -37,8 +37,8 @@ TEST_F(PathUtilityTest, InvalidPaths) { const std::vector invalid_paths{"/xyz/.%00../abc", "/xyz/%00.%00./abc", "/xyz/AAAAA%%0000/abc"}; for (const auto& path : invalid_paths) { - auto& path_header = pathHeaderEntry(path); - EXPECT_FALSE(PathUtil::canonicalPath(path_header)) << "original path: " << path; + pathHeaderEntry(path); + EXPECT_FALSE(PathUtil::canonicalPath(headers_)) << "original path: " << path; } } @@ -57,7 +57,7 @@ TEST_F(PathUtilityTest, NormalizeValidPaths) { for (const auto& path_pair : non_normal_pairs) { auto& path_header = pathHeaderEntry(path_pair.first); - const auto result = PathUtil::canonicalPath(path_header); + const auto result = PathUtil::canonicalPath(headers_); EXPECT_TRUE(result) << "original path: " << path_pair.first; EXPECT_EQ(path_header.value().getStringView(), path_pair.second) << "original path: " << path_pair.second; @@ -75,7 +75,7 @@ TEST_F(PathUtilityTest, NormalizeCasePath) { for (const auto& path_pair : non_normal_pairs) { auto& path_header = pathHeaderEntry(path_pair.first); - const auto result = PathUtil::canonicalPath(path_header); + const auto result = PathUtil::canonicalPath(headers_); EXPECT_TRUE(result) << "original path: " << path_pair.first; EXPECT_EQ(path_header.value().getStringView(), path_pair.second) << "original path: " << path_pair.second; @@ -89,7 +89,7 @@ TEST_F(PathUtilityTest, NormalizeCasePath) { TEST_F(PathUtilityTest, MergeSlashes) { auto mergeSlashes = [this](const std::string& path_value) { auto& path_header = pathHeaderEntry(path_value); - PathUtil::mergeSlashes(path_header); + PathUtil::mergeSlashes(headers_); auto sanitized_path_value = path_header.value().getStringView(); return std::string(sanitized_path_value); }; diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 8676fc3dd6ae1..55a20f4f74807 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4237,12 +4237,12 @@ TEST_F(RouterTest, AutoHostRewriteEnabled) { Http::TestHeaderMapImpl incoming_headers; HttpTestUtility::addDefaultHeaders(incoming_headers); - incoming_headers.Host()->value(req_host); + incoming_headers.setHost(req_host); cm_.conn_pool_.host_->hostname_ = "scooby.doo"; Http::TestHeaderMapImpl outgoing_headers; HttpTestUtility::addDefaultHeaders(outgoing_headers); - outgoing_headers.Host()->value(cm_.conn_pool_.host_->hostname_); + outgoing_headers.setHost(cm_.conn_pool_.host_->hostname_); EXPECT_CALL(callbacks_.route_->route_entry_, timeout()) .WillOnce(Return(std::chrono::milliseconds(0))); @@ -4275,7 +4275,7 @@ TEST_F(RouterTest, AutoHostRewriteDisabled) { Http::TestHeaderMapImpl incoming_headers; HttpTestUtility::addDefaultHeaders(incoming_headers); - incoming_headers.Host()->value(req_host); + incoming_headers.setHost(req_host); cm_.conn_pool_.host_->hostname_ = "scooby.doo"; diff --git a/test/integration/filters/call_decodedata_once_filter.cc b/test/integration/filters/call_decodedata_once_filter.cc index 15abc0658fd05..ff387d9741048 100644 --- a/test/integration/filters/call_decodedata_once_filter.cc +++ b/test/integration/filters/call_decodedata_once_filter.cc @@ -16,8 +16,10 @@ class CallDecodeDataOnceFilter : public Http::PassThroughFilter { constexpr static char name[] = "call-decodedata-once-filter"; Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& header_map, bool) override { - Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); - Http::HeaderEntry* entry_added = header_map.get(Envoy::Http::LowerCaseString("added_size")); + const Http::HeaderEntry* entry_content = + header_map.get(Envoy::Http::LowerCaseString("content_size")); + const Http::HeaderEntry* entry_added = + header_map.get(Envoy::Http::LowerCaseString("added_size")); ASSERT(entry_content != nullptr && entry_added != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); added_size_ = std::stoul(std::string(entry_added->value().getStringView())); diff --git a/test/integration/filters/decode_headers_return_stop_all_filter.cc b/test/integration/filters/decode_headers_return_stop_all_filter.cc index e2049155ef98a..151a26e0fe036 100644 --- a/test/integration/filters/decode_headers_return_stop_all_filter.cc +++ b/test/integration/filters/decode_headers_return_stop_all_filter.cc @@ -27,12 +27,14 @@ class DecodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { // Http::FilterHeadersStatus::StopAllIterationAndWatermark for headers. Triggers a timer to // continue iteration after 5s. Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& header_map, bool) override { - Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); - Http::HeaderEntry* entry_added = header_map.get(Envoy::Http::LowerCaseString("added_size")); + const Http::HeaderEntry* entry_content = + header_map.get(Envoy::Http::LowerCaseString("content_size")); + const Http::HeaderEntry* entry_added = + header_map.get(Envoy::Http::LowerCaseString("added_size")); ASSERT(entry_content != nullptr && entry_added != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); added_size_ = std::stoul(std::string(entry_added->value().getStringView())); - Http::HeaderEntry* entry_is_first_trigger = + const Http::HeaderEntry* entry_is_first_trigger = header_map.get(Envoy::Http::LowerCaseString("is_first_trigger")); is_first_trigger_ = entry_is_first_trigger != nullptr; // Remove "first_trigger" headers so that if the filter is registered twice in a filter chain, @@ -41,7 +43,8 @@ class DecodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { createTimerForContinue(); - Http::HeaderEntry* entry_buffer = header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); + const Http::HeaderEntry* entry_buffer = + header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); if (entry_buffer == nullptr || !is_first_trigger_) { return Http::FilterHeadersStatus::StopAllIterationAndBuffer; } else { diff --git a/test/integration/filters/encode_headers_return_stop_all_filter.cc b/test/integration/filters/encode_headers_return_stop_all_filter.cc index 778c4897a15a9..768fc1ad343ad 100644 --- a/test/integration/filters/encode_headers_return_stop_all_filter.cc +++ b/test/integration/filters/encode_headers_return_stop_all_filter.cc @@ -27,8 +27,10 @@ class EncodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { // Http::FilterHeadersStatus::StopAllIterationAndWatermark for headers. Triggers a timer to // continue iteration after 5s. Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& header_map, bool) override { - Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); - Http::HeaderEntry* entry_added = header_map.get(Envoy::Http::LowerCaseString("added_size")); + const Http::HeaderEntry* entry_content = + header_map.get(Envoy::Http::LowerCaseString("content_size")); + const Http::HeaderEntry* entry_added = + header_map.get(Envoy::Http::LowerCaseString("added_size")); ASSERT(entry_content != nullptr && entry_added != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); added_size_ = std::stoul(std::string(entry_added->value().getStringView())); @@ -39,7 +41,8 @@ class EncodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { Http::MetadataMapPtr metadata_map_ptr = std::make_unique(metadata_map); encoder_callbacks_->addEncodedMetadata(std::move(metadata_map_ptr)); - Http::HeaderEntry* entry_buffer = header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); + const Http::HeaderEntry* entry_buffer = + header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); if (entry_buffer == nullptr) { return Http::FilterHeadersStatus::StopAllIterationAndBuffer; } else { diff --git a/test/integration/filters/metadata_stop_all_filter.cc b/test/integration/filters/metadata_stop_all_filter.cc index 3ff6b7983d010..423df634ab5de 100644 --- a/test/integration/filters/metadata_stop_all_filter.cc +++ b/test/integration/filters/metadata_stop_all_filter.cc @@ -22,7 +22,8 @@ class MetadataStopAllFilter : public Http::PassThroughFilter { constexpr static char name[] = "metadata-stop-all-filter"; Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& header_map, bool) override { - Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); + const Http::HeaderEntry* entry_content = + header_map.get(Envoy::Http::LowerCaseString("content_size")); ASSERT(entry_content != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index 90ead02098e5b..618e874071e31 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -100,7 +100,7 @@ void WebsocketIntegrationTest::commonValidate(Http::HeaderMap& proxied_headers, original_headers.Connection()->value() == "keep-alive, upgrade") { // The keep-alive is implicit for HTTP/1.1, so Envoy only sets the upgrade // header when converting from HTTP/1.1 to H2 - proxied_headers.Connection()->value().setCopy("keep-alive, upgrade", 19); + proxied_headers.setConnection("keep-alive, upgrade"); } } diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index c65862f5f58f2..9fab43a8ff354 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -839,7 +839,7 @@ TEST_P(AdminInstanceTest, EscapeHelpTextWithPunctuation) { Http::HeaderMapImpl header_map; Buffer::OwnedImpl response; EXPECT_EQ(Http::Code::OK, getCallback("/", header_map, response)); - Http::HeaderString& content_type = header_map.ContentType()->value(); + const Http::HeaderString& content_type = header_map.ContentType()->value(); EXPECT_THAT(std::string(content_type.getStringView()), testing::HasSubstr("text/html")); EXPECT_EQ(-1, response.search(planets.data(), planets.size(), 0)); const std::string escaped_planets = "jupiter>saturn>mars"; From 1d6f528e4a860db1180a024506a38abf7fbc019d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 1 Dec 2019 20:36:13 -0500 Subject: [PATCH 02/14] Add lazy map implementation and make the speed tests show its impact more clearly. Signed-off-by: Joshua Marantz --- include/envoy/http/header_map.h | 3 +- source/common/http/header_map_impl.cc | 123 +++++++++--------- source/common/http/header_map_impl.h | 85 +++++++----- .../common/http/header_map_impl_speed_test.cc | 38 ++++-- 4 files changed, 145 insertions(+), 104 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 05c48b0f95bc6..43d062ec99a18 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -487,7 +487,8 @@ class HeaderMap { virtual uint64_t byteSize() const PURE; /** - * Get a header by key. + * Get the first instance of a header by key. Subsequent entries for the same key are ignored. + * * @param key supplies the header key. * @return the header entry if it exists otherwise nullptr. */ diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index ae3a02d84ee9a..ff9bb30f0518f 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -289,10 +289,10 @@ struct HeaderMapImpl::StaticLookupTable : public TrieLookupTable { } }; -uint64_t HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data, - absl::string_view delimiter) { +void HeaderMapImpl::HeaderList::appendToHeader(HeaderString& header, absl::string_view data, + absl::string_view delimiter) { if (data.empty()) { - return 0; + return; } uint64_t byte_size = 0; if (!header.empty()) { @@ -300,7 +300,7 @@ uint64_t HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view d byte_size += delimiter.size(); } header.append(data.data(), data.size()); - return data.size() + byte_size; + addSize(data.size() + byte_size); } HeaderMapImpl::HeaderMapImpl() { memset(&inline_headers_, 0, sizeof(inline_headers_)); } @@ -318,15 +318,15 @@ HeaderMapImpl::HeaderMapImpl( verifyByteSize(); } -void HeaderMapImpl::updateSize(uint64_t from_size, uint64_t to_size) { +void HeaderMapImpl::HeaderList::updateSize(uint64_t from_size, uint64_t to_size) { ASSERT(cached_byte_size_ >= from_size); cached_byte_size_ -= from_size; cached_byte_size_ += to_size; } -void HeaderMapImpl::addSize(uint64_t size) { cached_byte_size_ += size; } +void HeaderMapImpl::HeaderList::addSize(uint64_t size) { cached_byte_size_ += size; } -void HeaderMapImpl::subtractSize(uint64_t size) { +void HeaderMapImpl::HeaderList::subtractSize(uint64_t size) { ASSERT(cached_byte_size_ >= size); cached_byte_size_ -= size; } @@ -372,14 +372,11 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) { if (*ref_lookup_response.entry_ == nullptr) { maybeCreateInline(ref_lookup_response.entry_, *ref_lookup_response.key_, std::move(value)); } else { - const uint64_t added_size = - appendToHeader((*ref_lookup_response.entry_)->value(), value.getStringView()); - addSize(added_size); + headers_.appendToHeader((*ref_lookup_response.entry_)->value(), value.getStringView()); value.clear(); } } else { - addSize(key.size() + value.size()); - std::list::iterator i = headers_.insert(std::move(key), std::move(value)); + HeaderNode i = headers_.insert(std::move(key), std::move(value)); i->entry_ = i; } } @@ -389,8 +386,7 @@ void HeaderMapImpl::addViaMove(HeaderString&& key, HeaderString&& value) { // the existing value. auto* entry = getExistingInline(key.getStringView()); if (entry != nullptr) { - const uint64_t added_size = appendToHeader(entry->value(), value.getStringView()); - addSize(added_size); + headers_.appendToHeader(entry->value(), value.getStringView()); key.clear(); value.clear(); } else { @@ -403,7 +399,6 @@ void HeaderMapImpl::addReference(const LowerCaseString& key, absl::string_view v HeaderString ref_key(key); HeaderString ref_value(value); addViaMove(std::move(ref_key), std::move(ref_value)); - verifyByteSize(); } void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, uint64_t value) { @@ -429,8 +424,7 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) { if (entry != nullptr) { char buf[32]; StringUtil::itoa(buf, sizeof(buf), value); - const uint64_t added_size = appendToHeader(entry->value(), buf); - addSize(added_size); + headers_.appendToHeader(entry->value(), buf); return; } HeaderString new_key; @@ -446,8 +440,7 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) { void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value) { auto* entry = getExistingInline(key.get()); if (entry != nullptr) { - const uint64_t added_size = appendToHeader(entry->value(), value); - addSize(added_size); + headers_.appendToHeader(entry->value(), value); return; } HeaderString new_key; @@ -461,16 +454,13 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value) } void HeaderMapImpl::append(const LowerCaseString& key, absl::string_view value) { - bool found = false; - for (HeaderEntryImpl& header : headers_) { - if (header.key().getStringView() == key.get()) { - const uint64_t added_size = appendToHeader(header.value(), value); - addSize(added_size); - found = true; + HeaderLazyMap::iterator iter = headers_.find(key.get()); + if (iter != headers_.findEnd()) { + HeaderNodeVector& v = iter->second; + for (HeaderNode node : v) { + headers_.appendToHeader(node->value(), value); } - } - - if (!found) { + } else { addCopy(key, value); } verifyByteSize(); @@ -496,24 +486,20 @@ void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, absl::string_vie void HeaderMapImpl::setCopy(const LowerCaseString& key, absl::string_view value) { // Replaces a header if it exists, otherwise adds by copy. - bool found = false; - for (HeaderEntryImpl& header : headers_) { - if (header.key().getStringView() == key.get()) { - updateSize(header.value().size(), value.size()); - header.value(value); - found = true; + HeaderLazyMap::iterator iter = headers_.find(key.get()); + if (iter != headers_.findEnd()) { + HeaderNodeVector& v = iter->second; + for (HeaderNode node : v) { + headers_.updateSize(node->value().size(), value.size()); + node->value(value); } - } - - if (!found) { + } else { addCopy(key, value); } verifyByteSize(); } -uint64_t HeaderMapImpl::byteSize() const { return cached_byte_size_; } - -uint64_t HeaderMapImpl::byteSizeInternal() const { +uint64_t HeaderMapImpl::HeaderList::byteSizeInternal() const { // Computes the total byte size by summing the byte size of the keys and values. uint64_t byte_size = 0; for (const HeaderEntryImpl& header : headers_) { @@ -524,12 +510,13 @@ uint64_t HeaderMapImpl::byteSizeInternal() const { } const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { - for (const HeaderEntryImpl& header : headers_) { - if (header.key() == key.get().c_str()) { - return &header; - } + HeaderLazyMap::iterator iter = headers_.find(key.get()); + if (iter != headers_.findEnd()) { + const HeaderNodeVector& v = iter->second; + ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value. + HeaderEntryImpl& header_entry = *v[0]; + return &header_entry; } - return nullptr; } @@ -575,7 +562,18 @@ HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key, void HeaderMapImpl::clear() { inline_headers_.clear(); headers_.clear(); - cached_byte_size_ = 0; +} + +HeaderMapImpl::HeaderLazyMap::iterator +HeaderMapImpl::HeaderList::find(absl::string_view key) const { + if (lazy_map_.empty()) { + for (auto node = headers_.begin(); node != headers_.end(); ++node) { + absl::string_view key = node->key().getStringView(); + HeaderNodeVector& v = lazy_map_[key]; + v.push_back(node); + } + } + return lazy_map_.find(key); } void HeaderMapImpl::remove(const LowerCaseString& key) { @@ -584,18 +582,23 @@ void HeaderMapImpl::remove(const LowerCaseString& key) { StaticLookupResponse ref_lookup_response = cb(*this); removeInline(ref_lookup_response.entry_); } else { - for (auto i = headers_.begin(); i != headers_.end();) { - if (i->key() == key.get().c_str()) { - subtractSize(i->key().size() + i->value().size()); - i = headers_.erase(i); - } else { - ++i; - } - } + headers_.remove(key.get()); } verifyByteSize(); } +void HeaderMapImpl::HeaderList::remove(absl::string_view key) { + auto iter = find(key); + if (iter != lazy_map_.end()) { + HeaderNodeVector header_nodes = std::move(iter->second); + lazy_map_.erase(iter); + for (const HeaderNode& node : header_nodes) { + ASSERT(node->key() == key); + erase(node); + } + } +} + void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { headers_.remove_if([&prefix, this](const HeaderEntryImpl& entry) { bool to_remove = absl::StartsWith(entry.key().getStringView(), prefix.get()); @@ -608,11 +611,11 @@ void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { if (ref_lookup_response.entry_) { const uint32_t key_value_size = (*ref_lookup_response.entry_)->key().size() + (*ref_lookup_response.entry_)->value().size(); - subtractSize(key_value_size); + headers_.subtractSize(key_value_size); *ref_lookup_response.entry_ = nullptr; } } else { - subtractSize(entry.key().size() + entry.value().size()); + headers_.subtractSize(entry.key().size() + entry.value().size()); } } return to_remove; @@ -640,8 +643,7 @@ HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl return **entry; } - addSize(key.get().size()); - std::list::iterator i = headers_.insert(key); + HeaderNode i = headers_.insert(key); i->entry_ = i; *entry = &(*i); return **entry; @@ -655,8 +657,7 @@ HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl return **entry; } - addSize(key.get().size() + value.size()); - std::list::iterator i = headers_.insert(key, std::move(value)); + HeaderNode i = headers_.insert(key, std::move(value)); i->entry_ = i; *entry = &(*i); return **entry; @@ -677,8 +678,6 @@ void HeaderMapImpl::removeInline(HeaderEntryImpl** ptr_to_entry) { } HeaderEntryImpl* entry = *ptr_to_entry; - const uint64_t size_to_subtract = entry->entry_->key().size() + entry->entry_->value().size(); - subtractSize(size_to_subtract); *ptr_to_entry = nullptr; headers_.erase(entry->entry_); verifyByteSize(); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index ef78fe41be352..fa5e6417fc26c 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -11,6 +11,8 @@ #include "common/common/non_copyable.h" #include "common/http/headers.h" +#include "absl/container/flat_hash_map.h" + namespace Envoy { namespace Http { @@ -22,26 +24,26 @@ public: const HeaderEntry* name() const override { return inline_headers_.name##_; } \ void append##name(absl::string_view data, absl::string_view delimiter) override { \ HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - addSize(HeaderMapImpl::appendToHeader(entry.value(), data, delimiter)); \ + headers_.appendToHeader(entry.value(), data, delimiter); \ verifyByteSize(); \ } \ void setReference##name(absl::string_view value) override { \ HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - updateSize(entry.value().size(), value.size()); \ + headers_.updateSize(entry.value().size(), value.size()); \ entry.value().setReference(value); \ verifyByteSize(); \ } \ void set##name(absl::string_view value) override { \ HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - updateSize(entry.value().size(), value.size()); \ + headers_.updateSize(entry.value().size(), value.size()); \ entry.value().setCopy(value); \ verifyByteSize(); \ } \ void set##name(uint64_t value) override { \ HeaderEntry& entry = maybeCreateInline(&inline_headers_.name##_, Headers::get().name); \ - subtractSize(inline_headers_.name##_->value().size()); \ + headers_.subtractSize(inline_headers_.name##_->value().size()); \ entry.value().setInteger(value); \ - addSize(inline_headers_.name##_->value().size()); \ + headers_.addSize(inline_headers_.name##_->value().size()); \ verifyByteSize(); \ } \ void remove##name() override { removeInline(&inline_headers_.name##_); } @@ -85,7 +87,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { void setReference(const LowerCaseString& key, absl::string_view value) override; void setReferenceKey(const LowerCaseString& key, absl::string_view value) override; void setCopy(const LowerCaseString& key, absl::string_view value) override; - uint64_t byteSize() const override; + uint64_t byteSize() const override { return headers_.byteSize(); } const HeaderEntry* get(const LowerCaseString& key) const override; void iterate(ConstIterateCb cb, void* context) const override; void iterateReverse(ConstIterateCb cb, void* context) const override; @@ -98,6 +100,11 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { void dumpState(std::ostream& os, int indent_level = 0) const override; protected: + struct HeaderEntryImpl; + using HeaderNode = std::list::iterator; + using HeaderNodeVector = std::vector; + using HeaderLazyMap = absl::flat_hash_map; + // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. void copyFrom(const HeaderMap& rhs); @@ -116,7 +123,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { HeaderString key_; HeaderString value_; - std::list::iterator entry_; + HeaderNode entry_; }; struct StaticLookupResponse { @@ -157,23 +164,26 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { return !key.getStringView().empty() && key.getStringView()[0] == ':'; } - template - std::list::iterator insert(Key&& key, Value&&... value) { + template HeaderNode insert(Key&& key, Value&&... value) { const bool is_pseudo_header = isPseudoHeader(key); - std::list::iterator i = - headers_.emplace(is_pseudo_header ? pseudo_headers_end_ : headers_.end(), - std::forward(key), std::forward(value)...); + HeaderNode i = headers_.emplace(is_pseudo_header ? pseudo_headers_end_ : headers_.end(), + std::forward(key), std::forward(value)...); + addSize(i->key().size() + i->value().size()); + if (!lazy_map_.empty()) { + lazy_map_[i->key().getStringView()].push_back(i); + } if (!is_pseudo_header && pseudo_headers_end_ == headers_.end()) { pseudo_headers_end_ = i; } return i; } - std::list::iterator erase(std::list::iterator i) { + void erase(HeaderNode i) { if (pseudo_headers_end_ == i) { pseudo_headers_end_++; } - return headers_.erase(i); + subtractSize(i->key().size() + i->value().size()); + headers_.erase(i); } template void remove_if(UnaryPredicate p) { @@ -186,10 +196,16 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { } return to_remove; }); + // It is possible to keep the lazy_map_ valid, but it's not clear whether + // it's worth it for this use case. Another option is to use a + // std::multi_map which would allow us to efficiently run this operation + // in the first place. + lazy_map_.clear(); } - std::list::iterator begin() { return headers_.begin(); } - std::list::iterator end() { return headers_.end(); } + HeaderLazyMap::iterator find(absl::string_view key) const; + HeaderLazyMap::iterator findEnd() const { return lazy_map_.end(); } + std::list::const_iterator begin() const { return headers_.begin(); } std::list::const_iterator end() const { return headers_.end(); } std::list::const_reverse_iterator rbegin() const { return headers_.rbegin(); } @@ -198,36 +214,47 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { bool empty() const { return headers_.empty(); } void clear() { headers_.clear(); + lazy_map_.clear(); pseudo_headers_end_ = headers_.end(); + cached_byte_size_ = 0; } + uint64_t byteSize() const { return cached_byte_size_; } + void remove(absl::string_view key); + + void verifyByteSize() { ASSERT(cached_byte_size_ == byteSizeInternal()); } + void addSize(uint64_t size); + void updateSize(uint64_t from_size, uint64_t to_size); + void subtractSize(uint64_t size); + void appendToHeader(HeaderString& header, absl::string_view data, + absl::string_view delimiter = ","); + private: - std::list headers_; - std::list::iterator pseudo_headers_end_; + // We make headers_ and lazy_map_ mutable to allow find() to populate lazy_map_, associating + // keys with a vector of nodes. We could also accomplish this by making HeaderMap::get() be + // non-const, or by using const_cast in he implementation. + mutable std::list headers_; + HeaderNode pseudo_headers_end_; + mutable HeaderLazyMap lazy_map_; + + // This holds the internal byte size of the HeaderMap. + uint64_t cached_byte_size_ = 0; + // Performs a manual byte size count. + uint64_t byteSizeInternal() const; }; void insertByKey(HeaderString&& key, HeaderString&& value); - static uint64_t appendToHeader(HeaderString& header, absl::string_view data, - absl::string_view delimiter = ","); HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key); HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key, HeaderString&& value); HeaderEntryImpl* getExistingInline(absl::string_view key); void removeInline(HeaderEntryImpl** entry); - void updateSize(uint64_t from_size, uint64_t to_size); - void addSize(uint64_t size); - void subtractSize(uint64_t size); + void verifyByteSize() { headers_.verifyByteSize(); } AllInlineHeaders inline_headers_; HeaderList headers_; - // This holds the internal byte size of the HeaderMap. - uint64_t cached_byte_size_ = 0; - // Performs a manual byte size count. - uint64_t byteSizeInternal() const; - void verifyByteSize() { ASSERT(cached_byte_size_ == byteSizeInternal()); } - ALL_INLINE_HEADERS(DEFINE_INLINE_HEADER_FUNCS) }; diff --git a/test/common/http/header_map_impl_speed_test.cc b/test/common/http/header_map_impl_speed_test.cc index f313d26b4a173..6989b51517d7e 100644 --- a/test/common/http/header_map_impl_speed_test.cc +++ b/test/common/http/header_map_impl_speed_test.cc @@ -42,7 +42,7 @@ static void HeaderMapImplSetReference(benchmark::State& state) { } benchmark::DoNotOptimize(headers.size()); } -BENCHMARK(HeaderMapImplSetReference)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplSetReference)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** * Measure the speed of retrieving a header value. The numeric Arg passed by the @@ -64,7 +64,7 @@ static void HeaderMapImplGet(benchmark::State& state) { } benchmark::DoNotOptimize(successes); } -BENCHMARK(HeaderMapImplGet)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplGet)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** * Measure the retrieval speed of a header for which HeaderMapImpl is expected to @@ -81,7 +81,7 @@ static void HeaderMapImplGetInline(benchmark::State& state) { } benchmark::DoNotOptimize(size); } -BENCHMARK(HeaderMapImplGetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplGetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** * Measure the speed of writing to a header for which HeaderMapImpl is expected to @@ -96,7 +96,7 @@ static void HeaderMapImplSetInlineMacro(benchmark::State& state) { } benchmark::DoNotOptimize(headers.size()); } -BENCHMARK(HeaderMapImplSetInlineMacro)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplSetInlineMacro)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** * Measure the speed of writing to a header for which HeaderMapImpl is expected to @@ -111,7 +111,7 @@ static void HeaderMapImplSetInlineInteger(benchmark::State& state) { } benchmark::DoNotOptimize(headers.size()); } -BENCHMARK(HeaderMapImplSetInlineInteger)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplSetInlineInteger)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** Measure the speed of the byteSize() estimation method. */ static void HeaderMapImplGetByteSize(benchmark::State& state) { @@ -123,7 +123,7 @@ static void HeaderMapImplGetByteSize(benchmark::State& state) { } benchmark::DoNotOptimize(size); } -BENCHMARK(HeaderMapImplGetByteSize)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplGetByteSize)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** Measure the speed of iteration with a lightweight callback. */ static void HeaderMapImplIterate(benchmark::State& state) { @@ -139,10 +139,10 @@ static void HeaderMapImplIterate(benchmark::State& state) { } benchmark::DoNotOptimize(num_callbacks); } -BENCHMARK(HeaderMapImplIterate)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplIterate)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); -/** Measure the speed of the HeaderMapImpl lookup() method. */ -static void HeaderMapImplLookup(benchmark::State& state) { +/** Measure the speed of the HeaderMapImpl lookup() method on an inline header */ +static void HeaderMapImplLookupInline(benchmark::State& state) { const LowerCaseString key("connection"); const std::string value("01234567890123456789"); HeaderMapImpl headers; @@ -154,7 +154,21 @@ static void HeaderMapImplLookup(benchmark::State& state) { benchmark::DoNotOptimize(result); } } -BENCHMARK(HeaderMapImplLookup)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplLookupInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); + +/** Measure the speed of the HeaderMapImpl lookup() method on a custom header. */ +static void HeaderMapImplGetCustom(benchmark::State& state) { + const LowerCaseString key("custom-header"); + const std::string value("01234567890123456789"); + HeaderMapImpl headers; + addDummyHeaders(headers, state.range(0)); + headers.addReference(key, value); + for (auto _ : state) { + auto result = headers.get(key); + benchmark::DoNotOptimize(result); + } +} +BENCHMARK(HeaderMapImplGetCustom)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** * Measure the speed of removing a header by key name. @@ -172,7 +186,7 @@ static void HeaderMapImplRemove(benchmark::State& state) { } benchmark::DoNotOptimize(headers.size()); } -BENCHMARK(HeaderMapImplRemove)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplRemove)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** * Measure the speed of removing a header by key name, for the special case of @@ -191,7 +205,7 @@ static void HeaderMapImplRemoveInline(benchmark::State& state) { } benchmark::DoNotOptimize(headers.size()); } -BENCHMARK(HeaderMapImplRemoveInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); +BENCHMARK(HeaderMapImplRemoveInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); /** * Measure the speed of creating a HeaderMapImpl and populating it with a realistic From be74058181abe28a88fe9c762d59134377e80269 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 1 Dec 2019 22:09:34 -0500 Subject: [PATCH 03/14] Removed inline headers from the map. Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.cc | 15 +++++++++++++-- source/common/http/header_map_impl.h | 8 +------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index ff9bb30f0518f..e57c2ebc6023a 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -594,11 +594,22 @@ void HeaderMapImpl::HeaderList::remove(absl::string_view key) { lazy_map_.erase(iter); for (const HeaderNode& node : header_nodes) { ASSERT(node->key() == key); - erase(node); + erase(node, false /* clear_from_map */); } } } +void HeaderMapImpl::HeaderList::erase(HeaderNode i, bool clear_from_map) { + if (pseudo_headers_end_ == i) { + pseudo_headers_end_++; + } + subtractSize(i->key().size() + i->value().size()); + if (clear_from_map) { + lazy_map_.erase(i->key().getStringView()); + } + headers_.erase(i); +} + void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { headers_.remove_if([&prefix, this](const HeaderEntryImpl& entry) { bool to_remove = absl::StartsWith(entry.key().getStringView(), prefix.get()); @@ -679,7 +690,7 @@ void HeaderMapImpl::removeInline(HeaderEntryImpl** ptr_to_entry) { HeaderEntryImpl* entry = *ptr_to_entry; *ptr_to_entry = nullptr; - headers_.erase(entry->entry_); + headers_.erase(entry->entry_, true); verifyByteSize(); } diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index fa5e6417fc26c..3b68b81808c70 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -178,13 +178,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { return i; } - void erase(HeaderNode i) { - if (pseudo_headers_end_ == i) { - pseudo_headers_end_++; - } - subtractSize(i->key().size() + i->value().size()); - headers_.erase(i); - } + void erase(HeaderNode i, bool clear_from_map); template void remove_if(UnaryPredicate p) { headers_.remove_if([&](const HeaderEntryImpl& entry) { From 1dd454cf1e8dba1f0ab140cee68b77a7dfbca5c8 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 2 Dec 2019 17:02:31 -0500 Subject: [PATCH 04/14] add ifdefs to selecft between multi-map mode and flat-hash-map. Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.cc | 41 +++++++++++++++++++++++++++ source/common/http/header_map_impl.h | 19 +++++++++++++ 2 files changed, 60 insertions(+) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index e57c2ebc6023a..83ea1b7971d4f 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -456,10 +456,19 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value) void HeaderMapImpl::append(const LowerCaseString& key, absl::string_view value) { HeaderLazyMap::iterator iter = headers_.find(key.get()); if (iter != headers_.findEnd()) { +#if HEADER_MAP_USE_FLAT_HASH_MAP HeaderNodeVector& v = iter->second; for (HeaderNode node : v) { headers_.appendToHeader(node->value(), value); } +#endif +#if HEADER_MAP_USE_MULTI_MAP + do { + HeaderNode node = iter->second; + headers_.appendToHeader(node->value(), value); + ++iter; + } while (iter != headers_.findEnd() && iter->second->key().getStringView() == key.get()); +#endif } else { addCopy(key, value); } @@ -488,11 +497,21 @@ void HeaderMapImpl::setCopy(const LowerCaseString& key, absl::string_view value) // Replaces a header if it exists, otherwise adds by copy. HeaderLazyMap::iterator iter = headers_.find(key.get()); if (iter != headers_.findEnd()) { +#if HEADER_MAP_USE_FLAT_HASH_MAP HeaderNodeVector& v = iter->second; for (HeaderNode node : v) { headers_.updateSize(node->value().size(), value.size()); node->value(value); } +#endif +#if HEADER_MAP_USE_MULTI_MAP + do { + HeaderNode node = iter->second; + headers_.updateSize(node->value().size(), value.size()); + node->value(value); + ++iter; + } while (iter != headers_.findEnd() && iter->second->key().getStringView() == key.get()); +#endif } else { addCopy(key, value); } @@ -512,9 +531,14 @@ uint64_t HeaderMapImpl::HeaderList::byteSizeInternal() const { const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { HeaderLazyMap::iterator iter = headers_.find(key.get()); if (iter != headers_.findEnd()) { +#if HEADER_MAP_USE_FLAT_HASH_MAP const HeaderNodeVector& v = iter->second; ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value. HeaderEntryImpl& header_entry = *v[0]; +#endif +#if HEADER_MAP_USE_MULTI_MAP + HeaderEntryImpl& header_entry = *(iter->second); +#endif return &header_entry; } return nullptr; @@ -569,8 +593,13 @@ HeaderMapImpl::HeaderList::find(absl::string_view key) const { if (lazy_map_.empty()) { for (auto node = headers_.begin(); node != headers_.end(); ++node) { absl::string_view key = node->key().getStringView(); +#if HEADER_MAP_USE_FLAT_HASH_MAP HeaderNodeVector& v = lazy_map_[key]; v.push_back(node); +#endif +#if HEADER_MAP_USE_MULTI_MAP + lazy_map_.insert(std::make_pair(key, node)); +#endif } } return lazy_map_.find(key); @@ -590,12 +619,24 @@ void HeaderMapImpl::remove(const LowerCaseString& key) { void HeaderMapImpl::HeaderList::remove(absl::string_view key) { auto iter = find(key); if (iter != lazy_map_.end()) { +#if HEADER_MAP_USE_FLAT_HASH_MAP HeaderNodeVector header_nodes = std::move(iter->second); lazy_map_.erase(iter); for (const HeaderNode& node : header_nodes) { ASSERT(node->key() == key); erase(node, false /* clear_from_map */); } +#endif +#if HEADER_MAP_USE_MULTI_MAP + ASSERT(iter->second->key() == key); + do { + auto iter_to_erase = iter; + ++iter; + HeaderNode& node = iter_to_erase->second; + erase(node, false /* clear_from_map */); + lazy_map_.erase(iter_to_erase); + } while (iter != lazy_map_.end() && iter->second->key() == key); +#endif } } diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 3b68b81808c70..5aed8af630256 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -1,8 +1,14 @@ #pragma once +#define HEADER_MAP_USE_MULTI_MAP true +#define HEADER_MAP_USE_FLAT_HASH_MAP false + #include #include #include +#if HEADER_MAP_USE_MULTI_MAP +#include +#endif #include #include @@ -11,7 +17,9 @@ #include "common/common/non_copyable.h" #include "common/http/headers.h" +#if HEADER_MAP_USE_FLAT_HASH_MAP #include "absl/container/flat_hash_map.h" +#endif namespace Envoy { namespace Http { @@ -102,8 +110,14 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { protected: struct HeaderEntryImpl; using HeaderNode = std::list::iterator; +#if HEADER_MAP_USE_FLAT_HASH_MAP using HeaderNodeVector = std::vector; using HeaderLazyMap = absl::flat_hash_map; + //using HeaderLazyMap = std::map; +#endif +#if HEADER_MAP_USE_MULTI_MAP + using HeaderLazyMap = std::multimap; +#endif // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. void copyFrom(const HeaderMap& rhs); @@ -170,7 +184,12 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { std::forward(key), std::forward(value)...); addSize(i->key().size() + i->value().size()); if (!lazy_map_.empty()) { +#if HEADER_MAP_USE_FLAT_HASH_MAP lazy_map_[i->key().getStringView()].push_back(i); +#endif +#if HEADER_MAP_USE_MULTI_MAP + lazy_map_.insert(std::make_pair(i->key().getStringView(), i)); +#endif } if (!is_pseudo_header && pseudo_headers_end_ == headers_.end()) { pseudo_headers_end_ = i; From 61912d2e217006d6e143ae9cb5bcc19e3056c5dd Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 6 Dec 2019 18:51:11 -0500 Subject: [PATCH 05/14] unpatch 9114 Signed-off-by: Joshua Marantz --- include/envoy/access_log/access_log.h | 4 + include/envoy/http/header_map.h | 94 ++--- source/common/grpc/common.cc | 7 +- source/common/grpc/common.h | 8 +- source/common/http/conn_manager_impl.cc | 12 + source/common/http/conn_manager_utility.cc | 13 +- source/common/http/header_map_impl.cc | 38 +- source/common/http/header_map_impl.h | 12 +- source/common/http/http1/codec_impl.cc | 12 +- source/common/http/http2/codec_impl.cc | 14 +- source/common/http/path_utility.cc | 12 +- source/common/http/path_utility.h | 6 +- source/common/http/utility.cc | 17 +- source/common/router/config_impl.cc | 6 +- source/common/router/router.cc | 27 +- source/common/router/router.h | 4 +- source/common/router/shadow_writer_impl.cc | 2 +- source/common/upstream/health_checker_impl.cc | 3 +- .../access_loggers/common/access_log_base.h | 5 + .../grpc/http_grpc_access_log_impl.cc | 7 +- .../extensions/filters/common/expr/context.cc | 2 +- .../dynamic_forward_proxy/proxy_filter.cc | 4 +- .../filters/http/ext_authz/ext_authz.cc | 15 +- .../grpc_http1_bridge/http1_bridge_filter.cc | 2 +- .../http/grpc_http1_reverse_bridge/filter.cc | 6 +- .../json_transcoder_filter.cc | 6 +- .../filters/http/health_check/health_check.cc | 2 +- .../http/ip_tagging/ip_tagging_filter.cc | 2 +- .../extensions/filters/http/lua/wrappers.cc | 7 +- .../filters/http/rbac/rbac_filter.cc | 8 + .../common/ot/opentracing_driver_impl.cc | 2 +- test/common/grpc/common_test.cc | 30 +- test/common/http/header_map_impl_fuzz.proto | 24 +- test/common/http/header_map_impl_fuzz_test.cc | 100 +++--- .../common/http/header_map_impl_speed_test.cc | 19 +- test/common/http/header_map_impl_test.cc | 336 ++++++------------ test/common/http/http2/codec_impl_test.cc | 6 +- test/common/http/path_utility_test.cc | 14 +- test/common/router/router_test.cc | 6 +- .../filters/call_decodedata_once_filter.cc | 6 +- .../decode_headers_return_stop_all_filter.cc | 11 +- .../encode_headers_return_stop_all_filter.cc | 9 +- .../filters/metadata_stop_all_filter.cc | 3 +- .../integration/websocket_integration_test.cc | 2 +- test/server/http/admin_test.cc | 2 +- 45 files changed, 455 insertions(+), 472 deletions(-) diff --git a/include/envoy/access_log/access_log.h b/include/envoy/access_log/access_log.h index 3648a6e44a679..394df8f26ccb7 100644 --- a/include/envoy/access_log/access_log.h +++ b/include/envoy/access_log/access_log.h @@ -78,6 +78,10 @@ class Instance { /** * Log a completed request. + * Prior to logging, call refreshByteSize() on HeaderMaps to ensure that an accurate byte size + * count is logged. + * TODO(asraa): Remove refreshByteSize() requirement when entries in HeaderMap can no longer be + * modified by reference and headerMap holds an accurate internal byte size count. * @param request_headers supplies the incoming request headers after filtering. * @param response_headers supplies response headers. * @param response_trailers supplies response trailers. diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 43d062ec99a18..3e35595d4a2ba 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -113,7 +113,7 @@ class HeaderString { * @param ref_value MUST point to data that will live beyond the lifetime of any request/response * using the string (since a codec may optimize for zero copy). */ - explicit HeaderString(absl::string_view ref_value); + explicit HeaderString(const std::string& ref_value); HeaderString(HeaderString&& move_value) noexcept; ~HeaderString(); @@ -224,6 +224,13 @@ class HeaderEntry { */ virtual const HeaderString& key() const PURE; + /** + * Set the header value by copying data into it (deprecated, use absl::string_view variant + * instead). + * TODO(htuch): Cleanup deprecated call sites. + */ + virtual void value(const char* value, uint32_t size) PURE; + /** * Set the header value by copying data into it. */ @@ -338,22 +345,19 @@ class HeaderEntry { HEADER_FUNC(Via) /** - * The following functions are defined for each inline header above. - - * E.g., for path we have: - * Path() -> returns the header entry if it exists or nullptr. - * appendPath(path, "/") -> appends the string path with delimiter "/" to the header value. - * setReferencePath(PATH) -> sets header value to reference string PATH. - * setPath(path_string) -> sets the header value to the string path_string by copying the data. - * removePath() -> removes the header if it exists. + * The following functions are defined for each inline header above. E.g., for ContentLength we + * have: * - * For inline headers that use integers, we have: - * // TODO(asraa): Remove this method for other inline headers. - * setContentLength(5) -> sets the header value to the integer 5. + * ContentLength() -> returns the header entry if it exists or nullptr. + * insertContentLength() -> inserts the header if it does not exist, and returns a reference to it. + * removeContentLength() -> removes the header if it exists. + * + * TODO(asraa): Remove functions with a non-const HeaderEntry return value. */ #define DEFINE_INLINE_HEADER(name) \ virtual const HeaderEntry* name() const PURE; \ - virtual void append##name(absl::string_view data, absl::string_view delimiter) PURE; \ + virtual HeaderEntry* name() PURE; \ + virtual HeaderEntry& insert##name() PURE; \ virtual void setReference##name(absl::string_view value) PURE; \ virtual void set##name(absl::string_view value) PURE; \ virtual void set##name(uint64_t value) PURE; \ @@ -377,11 +381,12 @@ class HeaderMap { * Calling addReference multiple times for the same header will result in: * - Comma concatenation for predefined inline headers. * - Multiple headers being present in the HeaderMap for other headers. + * TODO(asraa): Replace const std::string& param with an absl::string_view. * * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL NOT be copied. */ - virtual void addReference(const LowerCaseString& key, absl::string_view value) PURE; + virtual void addReference(const LowerCaseString& key, const std::string& value) PURE; /** * Add a header with a reference key to the map. The key MUST point to data that will live beyond @@ -409,7 +414,7 @@ class HeaderMap { * @param key specifies the name of the header to add; it WILL NOT be copied. * @param value specifies the value of the header to add; it WILL be copied. */ - virtual void addReferenceKey(const LowerCaseString& key, absl::string_view value) PURE; + virtual void addReferenceKey(const LowerCaseString& key, const std::string& value) PURE; /** * Add a header by copying both the header key and the value. @@ -433,15 +438,7 @@ class HeaderMap { * @param key specifies the name of the header to add; it WILL be copied. * @param value specifies the value of the header to add; it WILL be copied. */ - virtual void addCopy(const LowerCaseString& key, absl::string_view value) PURE; - - /** - * Appends data to header. If header already has a value, the string "," is added between the - * existing value and data. - * @param key specifies the name of the header to append; it WILL be copied. - * @param value specifies the value of the header to add; it WILL be copied. - */ - virtual void append(const LowerCaseString& key, absl::string_view value) PURE; + virtual void addCopy(const LowerCaseString& key, const std::string& value) PURE; /** * Set a reference header in the map. Both key and value MUST point to data that will live beyond @@ -450,11 +447,12 @@ class HeaderMap { * * Calling setReference multiple times for the same header will result in only the last header * being present in the HeaderMap. + * TODO(asraa): Replace const std::string& param with an absl::string_view. * * @param key specifies the name of the header to set; it WILL NOT be copied. * @param value specifies the value of the header to set; it WILL NOT be copied. */ - virtual void setReference(const LowerCaseString& key, absl::string_view value) PURE; + virtual void setReference(const LowerCaseString& key, const std::string& value) PURE; /** * Set a header with a reference key in the map. The key MUST point to point to data that will @@ -467,24 +465,44 @@ class HeaderMap { * @param key specifies the name of the header to set; it WILL NOT be copied. * @param value specifies the value of the header to set; it WILL be copied. */ - virtual void setReferenceKey(const LowerCaseString& key, absl::string_view value) PURE; + virtual void setReferenceKey(const LowerCaseString& key, const std::string& value) PURE; /** - * Replaces a header value by copying the value. Copies the key if the key does not exist. + * HeaderMap contains an internal byte size count, updated as entries are added, removed, or + * modified through the HeaderMap interface. However, HeaderEntries can be accessed and modified + * by reference so that the HeaderMap can no longer accurately update the internal byte size + * count. * - * Calling setCopy multiple times for the same header will result in only the last header - * being present in the HeaderMap. + * Calling byteSize before a HeaderEntry is accessed will return the internal byte size count. The + * value is cleared when a HeaderEntry is accessed, and the value is updated and set again when + * refreshByteSize is called. * - * @param key specifies the name of the header to set; it WILL be copied. - * @param value specifies the value of the header to set; it WILL be copied. + * To guarantee an accurate byte size count, call refreshByteSize. + * + * @return uint64_t the approximate size of the header map in bytes if valid. */ - virtual void setCopy(const LowerCaseString& key, absl::string_view value) PURE; + virtual absl::optional byteSize() const PURE; /** - * @return uint64_t the size of the header map in bytes. This is the sum of the header keys and - * values and does not account for data structure overhead. + * This returns the sum of the byte sizes of the keys and values in the HeaderMap. This also + * updates and sets the byte size count. + * + * To guarantee an accurate byte size count, use this. If it is known HeaderEntries have not been + * manipulated since a call to refreshByteSize, it is safe to use byteSize. + * + * @return uint64_t the approximate size of the header map in bytes. + */ + virtual uint64_t refreshByteSize() PURE; + + /** + * This returns the sum of the byte sizes of the keys and values in the HeaderMap. + * + * This iterates over the HeaderMap to calculate size and should only be called directly when the + * user wants an explicit recalculation of the byte size. + * + * @return uint64_t the approximate size of the header map in bytes. */ - virtual uint64_t byteSize() const PURE; + virtual uint64_t byteSizeInternal() const PURE; /** * Get the first instance of a header by key. Subsequent entries for the same key are ignored. @@ -493,6 +511,7 @@ class HeaderMap { * @return the header entry if it exists otherwise nullptr. */ virtual const HeaderEntry* get(const LowerCaseString& key) const PURE; + virtual HeaderEntry* get(const LowerCaseString& key) PURE; // aliases to make iterate() and iterateReverse() callbacks easier to read enum class Iterate { Continue, Break }; @@ -531,11 +550,6 @@ class HeaderMap { */ virtual Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const PURE; - /** - * Clears the headers in the map. - */ - virtual void clear() PURE; - /** * Remove all instances of a header by key. * @param key supplies the header key to remove. diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 7129cf1f081a1..7b20c5d2d1587 100644 --- a/source/common/grpc/common.cc +++ b/source/common/grpc/common.cc @@ -170,7 +170,7 @@ std::chrono::milliseconds Common::getGrpcTimeout(const Http::HeaderMap& request_ return timeout; } -void Common::toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderMap& headers) { +void Common::toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderString& value) { uint64_t time = timeout.count(); static const char units[] = "mSMH"; const char* unit = units; // start with milliseconds @@ -187,7 +187,8 @@ void Common::toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::Heade unit++; } } - headers.setGrpcTimeout(absl::StrCat(time, absl::string_view(unit, 1))); + value.setInteger(time); + value.append(unit, 1); } Http::MessagePtr Common::prepareHeaders(const std::string& upstream_cluster, @@ -202,7 +203,7 @@ Http::MessagePtr Common::prepareHeaders(const std::string& upstream_cluster, // before Timeout and ContentType. message->headers().setReferenceTE(Http::Headers::get().TEValues.Trailers); if (timeout) { - toGrpcTimeout(timeout.value(), message->headers()); + toGrpcTimeout(timeout.value(), message->headers().insertGrpcTimeout().value()); } message->headers().setReferenceContentType(Http::Headers::get().ContentTypeValues.Grpc); diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index a44117a82161c..cae1a34b96211 100644 --- a/source/common/grpc/common.h +++ b/source/common/grpc/common.h @@ -81,12 +81,12 @@ class Common { static std::chrono::milliseconds getGrpcTimeout(const Http::HeaderMap& request_headers); /** - * Encode 'timeout' into 'grpc-timeout' format in the grpc-timeout header. + * Encode 'timeout' into 'grpc-timeout' format. * @param timeout the duration in std::chrono::milliseconds. - * @param headers the HeaderMap in which the grpc-timeout header will be set with the timeout in - * 'grpc-timeout' format, up to 8 decimal digits and a letter indicating the unit. + * @param value the HeaderString onto which format the timeout in 'grpc-timeout' format, up to + * 8 decimal digits and a letter indicating the unit. */ - static void toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderMap& headers); + static void toGrpcTimeout(const std::chrono::milliseconds& timeout, Http::HeaderString& value); /** * Serialize protobuf message with gRPC frame header. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 7744018dc446f..e9c4375e31c58 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -549,6 +549,18 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() { } connection_manager_.stats_.named_.downstream_rq_active_.dec(); + // Refresh byte sizes of the HeaderMaps before logging. + // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and + // HeaderMap holds an accurate internal byte size count. + if (request_headers_ != nullptr) { + request_headers_->refreshByteSize(); + } + if (response_headers_ != nullptr) { + response_headers_->refreshByteSize(); + } + if (response_trailers_ != nullptr) { + response_trailers_->refreshByteSize(); + } for (const AccessLog::InstanceSharedPtr& access_log : connection_manager_.config_.accessLogs()) { access_log->log(request_headers_.get(), response_headers_.get(), response_trailers_.get(), stream_info_); diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index c118d94bdbb26..5a9f968a74366 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -200,8 +200,8 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest if (config.userAgent()) { request_headers.setEnvoyDownstreamServiceCluster(config.userAgent().value()); - const HeaderEntry* user_agent_header = request_headers.UserAgent(); - if (!user_agent_header || user_agent_header->value().empty()) { + const HeaderEntry& user_agent_header = request_headers.insertUserAgent(); + if (user_agent_header.value().empty()) { // Following setReference() is safe because user agent is constant for the life of the // listener. request_headers.setReferenceUserAgent(config.userAgent().value()); @@ -283,7 +283,7 @@ void ConnectionManagerUtility::mutateTracingRequestHeader(HeaderMap& request_hea UuidUtils::setTraceableUuid(x_request_id, UuidTraceStatus::NoTrace); } - request_headers.setRequestId(x_request_id); + request_headers.RequestId()->value(x_request_id); } void ConnectionManagerUtility::mutateXfccRequestHeader(HeaderMap& request_headers, @@ -364,7 +364,8 @@ void ConnectionManagerUtility::mutateXfccRequestHeader(HeaderMap& request_header const std::string client_cert_details_str = absl::StrJoin(client_cert_details, ";"); if (config.forwardClientCert() == ForwardClientCertType::AppendForward) { - request_headers.appendForwardedClientCert(client_cert_details_str, ","); + HeaderMapImpl::appendToHeader(request_headers.insertForwardedClientCert().value(), + client_cert_details_str); } else if (config.forwardClientCert() == ForwardClientCertType::SanitizeSet) { request_headers.setForwardedClientCert(client_cert_details_str); } else { @@ -410,11 +411,11 @@ bool ConnectionManagerUtility::maybeNormalizePath(HeaderMap& request_headers, ASSERT(request_headers.Path()); bool is_valid_path = true; if (config.shouldNormalizePath()) { - is_valid_path = PathUtil::canonicalPath(request_headers); + is_valid_path = PathUtil::canonicalPath(*request_headers.Path()); } // Merge slashes after path normalization to catch potential edge cases with percent encoding. if (is_valid_path && config.shouldMergeSlashes()) { - PathUtil::mergeSlashes(request_headers); + PathUtil::mergeSlashes(*request_headers.Path()); } return is_valid_path; } diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 83ea1b7971d4f..80694bb95a3c4 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -49,8 +49,8 @@ HeaderString::HeaderString(const LowerCaseString& ref_value) : type_(Type::Refer ASSERT(valid()); } -HeaderString::HeaderString(absl::string_view ref_value) : type_(Type::Reference) { - buffer_.ref_ = ref_value.data(); +HeaderString::HeaderString(const std::string& ref_value) : type_(Type::Reference) { + buffer_.ref_ = ref_value.c_str(); string_length_ = ref_value.size(); ASSERT(valid()); } @@ -261,7 +261,13 @@ HeaderMapImpl::HeaderEntryImpl::HeaderEntryImpl(const LowerCaseString& key, Head HeaderMapImpl::HeaderEntryImpl::HeaderEntryImpl(HeaderString&& key, HeaderString&& value) : key_(std::move(key)), value_(std::move(value)) {} -void HeaderMapImpl::HeaderEntryImpl::value(absl::string_view value) { value_.setCopy(value); } +void HeaderMapImpl::HeaderEntryImpl::value(const char* value, uint32_t size) { + value_.setCopy(value, size); +} + +void HeaderMapImpl::HeaderEntryImpl::value(absl::string_view value) { + this->value(value.data(), static_cast(value.size())); +} void HeaderMapImpl::HeaderEntryImpl::value(uint64_t value) { value_.setInteger(value); } @@ -315,7 +321,6 @@ HeaderMapImpl::HeaderMapImpl( value_string.setCopy(value.second.c_str(), value.second.size()); addViaMove(std::move(key_string), std::move(value_string)); } - verifyByteSize(); } void HeaderMapImpl::HeaderList::updateSize(uint64_t from_size, uint64_t to_size) { @@ -345,7 +350,6 @@ void HeaderMapImpl::copyFrom(const HeaderMap& header_map) { return HeaderMap::Iterate::Continue; }, this); - verifyByteSize(); } bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const { @@ -407,16 +411,14 @@ void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, uint64_t value) new_value.setInteger(value); insertByKey(std::move(ref_key), std::move(new_value)); ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) - verifyByteSize(); } -void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, absl::string_view value) { +void HeaderMapImpl::addReferenceKey(const LowerCaseString& key, const std::string& value) { HeaderString ref_key(key); HeaderString new_value; - new_value.setCopy(value); + new_value.setCopy(value.c_str(), value.size()); insertByKey(std::move(ref_key), std::move(new_value)); ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) - verifyByteSize(); } void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) { @@ -428,16 +430,15 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) { return; } HeaderString new_key; - new_key.setCopy(key.get()); + new_key.setCopy(key.get().c_str(), key.get().size()); HeaderString new_value; new_value.setInteger(value); insertByKey(std::move(new_key), std::move(new_value)); ASSERT(new_key.empty()); // NOLINT(bugprone-use-after-move) ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) - verifyByteSize(); } -void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value) { +void HeaderMapImpl::addCopy(const LowerCaseString& key, const std::string& value) { auto* entry = getExistingInline(key.get()); if (entry != nullptr) { headers_.appendToHeader(entry->value(), value); @@ -544,6 +545,17 @@ const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { return nullptr; } +HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) { + for (HeaderEntryImpl& header : headers_) { + if (header.key() == key.get().c_str()) { + cached_byte_size_.reset(); + return &header; + } + } + + return nullptr; +} + void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const { for (const HeaderEntryImpl& header : headers_) { if (cb(header, context) == HeaderMap::Iterate::Break) { @@ -613,7 +625,6 @@ void HeaderMapImpl::remove(const LowerCaseString& key) { } else { headers_.remove(key.get()); } - verifyByteSize(); } void HeaderMapImpl::HeaderList::remove(absl::string_view key) { @@ -672,7 +683,6 @@ void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { } return to_remove; }); - verifyByteSize(); } void HeaderMapImpl::dumpState(std::ostream& os, int indent_level) const { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 5aed8af630256..594052d2f8e0c 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -67,6 +67,14 @@ public: */ class HeaderMapImpl : public HeaderMap, NonCopyable { public: + /** + * Appends data to header. If header already has a value, the string ',' is added between the + * existing value and data. + * @param header the header to append to. + * @param data to append to the header. + */ + static uint64_t appendToHeader(HeaderString& header, absl::string_view data); + HeaderMapImpl(); explicit HeaderMapImpl( const std::initializer_list>& values); @@ -121,6 +129,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. void copyFrom(const HeaderMap& rhs); + void clear() { removePrefix(LowerCaseString("")); } struct HeaderEntryImpl : public HeaderEntry, NonCopyable { HeaderEntryImpl(const LowerCaseString& key); @@ -129,6 +138,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { // HeaderEntry const HeaderString& key() const override { return key_; } + void value(const char* value, uint32_t size) override; void value(absl::string_view value) override; void value(uint64_t value) override; void value(const HeaderEntry& header) override; @@ -154,8 +164,6 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { struct StaticLookupTable; // Defined in header_map_impl.cc. struct AllInlineHeaders { - void clear() { memset(this, 0, sizeof(*this)); } - ALL_INLINE_HEADERS(DEFINE_INLINE_HEADER_STRUCT) }; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 8f2d33d29d37b..3cb3b24eb3368 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -489,8 +489,10 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { header_parsing_state_ = HeaderParsingState::Value; current_header_value_.append(data, length); - const uint32_t total = - current_header_field_.size() + current_header_value_.size() + current_header_map_->byteSize(); + // Verify that the cached value in byte size exists. + ASSERT(current_header_map_->byteSize().has_value()); + const uint32_t total = current_header_field_.size() + current_header_value_.size() + + current_header_map_->byteSize().value(); if (total > (max_headers_kb_ * 1024)) { error_code_ = Http::Code::RequestHeaderFieldsTooLarge; @@ -502,6 +504,10 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { int ConnectionImpl::onHeadersCompleteBase() { ENVOY_CONN_LOG(trace, "headers complete", connection_); completeLastHeader(); + // Validate that the completed HeaderMap's cached byte size exists and is correct. + // This assert iterates over the HeaderMap. + ASSERT(current_header_map_->byteSize().has_value() && + current_header_map_->byteSize() == current_header_map_->byteSizeInternal()); if (!(parser_.http_major == 1 && parser_.http_minor == 1)) { // This is not necessarily true, but it's good enough since higher layers only care if this is // HTTP/1.1 or not. @@ -522,7 +528,7 @@ int ConnectionImpl::onHeadersCompleteBase() { if (new_value.empty()) { current_header_map_->removeConnection(); } else { - current_header_map_->setConnection(new_value); + current_header_map_->Connection()->value(new_value); } } current_header_map_->remove(Headers::get().Http2Settings); diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index f69bb7b4b850f..4a756b58bcb2f 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -509,6 +509,10 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { switch (frame->hd.type) { case NGHTTP2_HEADERS: { + // Verify that the final HeaderMap's byte size is under the limit before decoding headers. + // This assert iterates over the HeaderMap. + ASSERT(stream->headers_->byteSize().has_value() && + stream->headers_->byteSize().value() == stream->headers_->byteSizeInternal()); stream->remote_end_stream_ = frame->hd.flags & NGHTTP2_FLAG_END_STREAM; if (!stream->cookies_.empty()) { HeaderString key(Headers::get().Cookie); @@ -620,6 +624,12 @@ int ConnectionImpl::onFrameSend(const nghttp2_frame* frame) { case NGHTTP2_HEADERS: case NGHTTP2_DATA: { StreamImpl* stream = getStream(frame->hd.stream_id); + if (stream->headers_) { + // Verify that the final HeaderMap's byte size is under the limit before sending frames. + // This assert iterates over the HeaderMap. + ASSERT(stream->headers_->byteSize().has_value() && + stream->headers_->byteSize().value() == stream->headers_->byteSizeInternal()); + } stream->local_end_stream_sent_ = frame->hd.flags & NGHTTP2_FLAG_END_STREAM; break; } @@ -810,7 +820,9 @@ int ConnectionImpl::saveHeader(const nghttp2_frame* frame, HeaderString&& name, } stream->saveHeader(std::move(name), std::move(value)); - if (stream->headers_->byteSize() > max_headers_kb_ * 1024 || + // Verify that the cached value in byte size exists. + ASSERT(stream->headers_->byteSize().has_value()); + if (stream->headers_->byteSize().value() > max_headers_kb_ * 1024 || stream->headers_->size() > max_headers_count_) { // This will cause the library to reset/close the stream. stats_.header_overflow_.inc(); diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc index 0266f0b634670..49372fc50dbf8 100644 --- a/source/common/http/path_utility.cc +++ b/source/common/http/path_utility.cc @@ -29,8 +29,8 @@ absl::optional canonicalizePath(absl::string_view original_path) { } // namespace /* static */ -bool PathUtil::canonicalPath(HeaderMap& headers) { - const auto original_path = headers.Path()->value().getStringView(); +bool PathUtil::canonicalPath(HeaderEntry& path_header) { + const auto original_path = path_header.value().getStringView(); // canonicalPath is supposed to apply on path component in URL instead of :path header const auto query_pos = original_path.find('?'); auto normalized_path_opt = canonicalizePath( @@ -50,12 +50,12 @@ bool PathUtil::canonicalPath(HeaderMap& headers) { if (!query_suffix.empty()) { normalized_path.insert(normalized_path.end(), query_suffix.begin(), query_suffix.end()); } - headers.setPath(normalized_path); + path_header.value(normalized_path); return true; } -void PathUtil::mergeSlashes(HeaderMap& headers) { - const auto original_path = headers.Path()->value().getStringView(); +void PathUtil::mergeSlashes(HeaderEntry& path_header) { + const auto original_path = path_header.value().getStringView(); // Only operate on path component in URL. const absl::string_view::size_type query_start = original_path.find('?'); const absl::string_view path = original_path.substr(0, query_start); @@ -65,7 +65,7 @@ void PathUtil::mergeSlashes(HeaderMap& headers) { } const absl::string_view prefix = absl::StartsWith(path, "/") ? "/" : absl::string_view(); const absl::string_view suffix = absl::EndsWith(path, "/") ? "/" : absl::string_view(); - headers.setPath(absl::StrCat( + path_header.value(absl::StrCat( prefix, absl::StrJoin(absl::StrSplit(path, '/', absl::SkipEmpty()), "/"), query, suffix)); } diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h index 8e635ed7d3976..a588f39de46eb 100644 --- a/source/common/http/path_utility.h +++ b/source/common/http/path_utility.h @@ -11,10 +11,10 @@ namespace Http { class PathUtil { public: // Returns if the normalization succeeds. - // If it is successful, the path header in header path will be updated with the normalized path. - static bool canonicalPath(HeaderMap& headers); + // If it is successful, the param will be updated with the normalized path. + static bool canonicalPath(HeaderEntry& path_header); // Merges two or more adjacent slashes in path part of URI into one. - static void mergeSlashes(HeaderMap& headers); + static void mergeSlashes(HeaderEntry& path_header); }; } // namespace Http diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index c5db36bc5e434..c6dad78c85520 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -73,11 +73,17 @@ void Utility::appendXff(HeaderMap& headers, const Network::Address::Instance& re return; } - headers.appendForwardedFor(remote_address.ip()->addressAsString(), ","); + HeaderString& header = headers.insertForwardedFor().value(); + const std::string& address_as_string = remote_address.ip()->addressAsString(); + HeaderMapImpl::appendToHeader(header, address_as_string.c_str()); } void Utility::appendVia(HeaderMap& headers, const std::string& via) { - headers.appendVia(via, ", "); + HeaderString& header = headers.insertVia().value(); + if (!header.empty()) { + header.append(", ", 2); + } + header.append(via.c_str(), via.size()); } std::string Utility::createSslRedirectPath(const HeaderMap& headers) { @@ -423,7 +429,7 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) { bool keep_header = false; // Determine whether the nominated header contains invalid values - const HeaderEntry* nominated_header = NULL; + HeaderEntry* nominated_header = NULL; if (lcs_header_to_remove == Http::Headers::get().Connection) { // Remove the connection header from the nominated tokens if it's self nominated @@ -477,7 +483,8 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) { } if (keep_header) { - headers.setTE(Http::Headers::get().TEValues.Trailers); + nominated_header->value().setCopy(Http::Headers::get().TEValues.Trailers.data(), + Http::Headers::get().TEValues.Trailers.size()); } } } @@ -497,7 +504,7 @@ bool Utility::sanitizeConnectionHeader(Http::HeaderMap& headers) { if (new_value.empty()) { headers.removeConnection(); } else { - headers.setConnection(new_value); + headers.Connection()->value(new_value); } } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 172b165dae743..c3c59669a2f55 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -424,13 +424,13 @@ void RouteEntryImplBase::finalizeRequestHeaders(Http::HeaderMap& headers, } if (!host_rewrite_.empty()) { - headers.setHost(host_rewrite_); + headers.Host()->value(host_rewrite_); } else if (auto_host_rewrite_header_) { - const Http::HeaderEntry* header = headers.get(*auto_host_rewrite_header_); + Http::HeaderEntry* header = headers.get(*auto_host_rewrite_header_); if (header != nullptr) { absl::string_view header_value = header->value().getStringView(); if (!header_value.empty()) { - headers.setHost(header_value); + headers.Host()->value(header_value); } } } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index eb48dbff40a6e..648a5d4911357 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -160,28 +160,26 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he // If present, use that value as route timeout and don't override // *x-envoy-expected-rq-timeout-ms* header. At this point *x-envoy-upstream-rq-timeout-ms* // header should have been sanitized by egress Envoy. - const Http::HeaderEntry* header_expected_timeout_entry = + Http::HeaderEntry* header_expected_timeout_entry = request_headers.EnvoyExpectedRequestTimeoutMs(); if (header_expected_timeout_entry) { trySetGlobalTimeout(header_expected_timeout_entry, timeout); } else { - const Http::HeaderEntry* header_timeout_entry = - request_headers.EnvoyUpstreamRequestTimeoutMs(); + Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); if (trySetGlobalTimeout(header_timeout_entry, timeout)) { request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } } } else { - const Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); if (trySetGlobalTimeout(header_timeout_entry, timeout)) { request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } } // See if there is a per try/retry timeout. If it's >= global we just ignore it. - const Http::HeaderEntry* per_try_timeout_entry = - request_headers.EnvoyUpstreamRequestPerTryTimeoutMs(); + Http::HeaderEntry* per_try_timeout_entry = request_headers.EnvoyUpstreamRequestPerTryTimeoutMs(); if (per_try_timeout_entry) { if (absl::SimpleAtoi(per_try_timeout_entry->value().getStringView(), &header_timeout)) { timeout.per_try_timeout_ = std::chrono::milliseconds(header_timeout); @@ -211,7 +209,8 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he // in grpc-timeout, ensuring that the upstream gRPC server is aware of the actual timeout. // If the expected timeout is 0 set no timeout, as Envoy treats 0 as infinite timeout. if (grpc_request && route.maxGrpcTimeout() && expected_timeout != 0) { - Grpc::Common::toGrpcTimeout(std::chrono::milliseconds(expected_timeout), request_headers); + Grpc::Common::toGrpcTimeout(std::chrono::milliseconds(expected_timeout), + request_headers.insertGrpcTimeout().value()); } return timeout; @@ -234,8 +233,7 @@ FilterUtility::HedgingParams FilterUtility::finalHedgingParams(const RouteEntry& HedgingParams hedging_params; hedging_params.hedge_on_per_try_timeout_ = route.hedgePolicy().hedgeOnPerTryTimeout(); - const Http::HeaderEntry* hedge_on_per_try_timeout_entry = - request_headers.EnvoyHedgeOnPerTryTimeout(); + Http::HeaderEntry* hedge_on_per_try_timeout_entry = request_headers.EnvoyHedgeOnPerTryTimeout(); if (hedge_on_per_try_timeout_entry) { if (hedge_on_per_try_timeout_entry->value() == "true") { hedging_params.hedge_on_per_try_timeout_ = true; @@ -1391,6 +1389,15 @@ Filter::UpstreamRequest::~UpstreamRequest() { stream_info_.setUpstreamTiming(upstream_timing_); stream_info_.onRequestComplete(); + // Prior to logging, refresh the byte size of the HeaderMaps. + // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and + // HeaderMap holds an accurate internal byte size count. + if (upstream_headers_ != nullptr) { + upstream_headers_->refreshByteSize(); + } + if (upstream_trailers_ != nullptr) { + upstream_trailers_->refreshByteSize(); + } for (const auto& upstream_log : parent_.config_.upstream_logs_) { upstream_log->log(parent_.downstream_headers_, upstream_headers_.get(), upstream_trailers_.get(), stream_info_); @@ -1632,7 +1639,7 @@ void Filter::UpstreamRequest::onPoolReady(Http::StreamEncoder& request_encoder, setRequestEncoder(request_encoder); calling_encode_headers_ = true; if (parent_.route_entry_->autoHostRewrite() && !host->hostname().empty()) { - parent_.downstream_headers_->setHost(host->hostname()); + parent_.downstream_headers_->Host()->value(host->hostname()); } if (span_ != nullptr) { diff --git a/source/common/router/router.h b/source/common/router/router.h index c16624a2741a9..29c4ed8175a4f 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -91,7 +91,7 @@ class FilterUtility { using ParseRetryFlagsFunc = std::function(absl::string_view)>; private: - static HeaderCheckResult hasValidRetryFields(const Http::HeaderEntry* header_entry, + static HeaderCheckResult hasValidRetryFields(Http::HeaderEntry* header_entry, const ParseRetryFlagsFunc& parse_fn) { HeaderCheckResult r; if (header_entry) { @@ -102,7 +102,7 @@ class FilterUtility { return r; } - static HeaderCheckResult isInteger(const Http::HeaderEntry* header_entry) { + static HeaderCheckResult isInteger(Http::HeaderEntry* header_entry) { HeaderCheckResult r; if (header_entry) { uint64_t out; diff --git a/source/common/router/shadow_writer_impl.cc b/source/common/router/shadow_writer_impl.cc index 73906beb91a2d..1597fbb8486b7 100644 --- a/source/common/router/shadow_writer_impl.cc +++ b/source/common/router/shadow_writer_impl.cc @@ -25,7 +25,7 @@ void ShadowWriterImpl::shadow(const std::string& cluster, Http::MessagePtr&& req // Switch authority to add a shadow postfix. This allows upstream logging to make more sense. auto parts = StringUtil::splitToken(request->headers().Host()->value().getStringView(), ":"); ASSERT(!parts.empty() && parts.size() <= 2); - request->headers().setHost( + request->headers().Host()->value( parts.size() == 2 ? absl::StrJoin(parts, "-shadow:") : absl::StrCat(request->headers().Host()->value().getStringView(), "-shadow")); diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index c1aad5a96ca2e..bb35f32277b70 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/source/common/upstream/health_checker_impl.cc @@ -638,7 +638,8 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() { headers_message->headers().setReferenceUserAgent( Http::Headers::get().UserAgentValues.EnvoyHealthChecker); - Grpc::Common::toGrpcTimeout(parent_.timeout_, headers_message->headers()); + Grpc::Common::toGrpcTimeout(parent_.timeout_, + headers_message->headers().insertGrpcTimeout().value()); Router::FilterUtility::setUpstreamScheme( headers_message->headers(), host_->transportSocketFactory().implementsSecureTransport()); diff --git a/source/extensions/access_loggers/common/access_log_base.h b/source/extensions/access_loggers/common/access_log_base.h index 9a6d6caa6668a..abe7f009d66f0 100644 --- a/source/extensions/access_loggers/common/access_log_base.h +++ b/source/extensions/access_loggers/common/access_log_base.h @@ -27,6 +27,11 @@ class ImplBase : public AccessLog::Instance { /** * Log a completed request if the underlying AccessLog `filter_` allows it. + * + * Prior to logging, call refreshByteSize() on HeaderMaps to ensure that an accurate byte size + * count is logged. + * TODO(asraa): Remove refreshByteSize() requirement when entries in HeaderMap can no longer be + * modified by reference and HeaderMap holds an accurate internal byte size count. */ void log(const Http::HeaderMap* request_headers, const Http::HeaderMap* response_headers, const Http::HeaderMap* response_trailers, diff --git a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc index 1eda50f092746..8c3bb139d069c 100644 --- a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc @@ -98,7 +98,9 @@ void HttpGrpcAccessLog::emitLog(const Http::HeaderMap& request_headers, request_properties->set_original_path( std::string(request_headers.EnvoyOriginalPath()->value().getStringView())); } - request_properties->set_request_headers_bytes(request_headers.byteSize()); + // TODO(asraa): This causes a manual iteration over the request_headers. Instead, refresh the byte + // size of this HeaderMap outside the loggers and use byteSize(). + request_properties->set_request_headers_bytes(request_headers.byteSizeInternal()); request_properties->set_request_body_bytes(stream_info.bytesReceived()); if (request_headers.Method() != nullptr) { envoy::api::v2::core::RequestMethod method = @@ -126,7 +128,8 @@ void HttpGrpcAccessLog::emitLog(const Http::HeaderMap& request_headers, if (stream_info.responseCodeDetails()) { response_properties->set_response_code_details(stream_info.responseCodeDetails().value()); } - response_properties->set_response_headers_bytes(response_headers.byteSize()); + ASSERT(response_headers.byteSize().has_value()); + response_properties->set_response_headers_bytes(response_headers.byteSize().value()); response_properties->set_response_body_bytes(stream_info.bytesSent()); if (!response_headers_to_log_.empty()) { auto* logged_headers = response_properties->mutable_response_headers(); diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 3b83f67c6db4c..7b2912e2b4d15 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -107,7 +107,7 @@ absl::optional RequestWrapper::operator[](CelValue key) const { } else if (value == UserAgent) { return convertHeaderEntry(headers_.value_->UserAgent()); } else if (value == TotalSize) { - return CelValue::CreateInt64(info_.bytesReceived() + headers_.value_->byteSize()); + return CelValue::CreateInt64(info_.bytesReceived() + headers_.value_->byteSize().value()); } } return {}; diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc index 214f11b87ce78..bbc9526df0867 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc @@ -74,7 +74,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::HeaderMap& headers, b if (config != nullptr) { const auto& host_rewrite = config->hostRewrite(); if (!host_rewrite.empty()) { - headers.setHost(host_rewrite); + headers.Host()->value(host_rewrite); } const auto& host_rewrite_header = config->hostRewriteHeader(); @@ -82,7 +82,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::HeaderMap& headers, b const auto* header = headers.get(host_rewrite_header); if (header != nullptr) { const auto& header_value = header->value().getStringView(); - headers.setHost(header_value); + headers.Host()->value(header_value); } } } diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index b3e6c0a7e36ce..ffb336c12f2c5 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -169,14 +169,19 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { callbacks_->clearRouteCache(); } for (const auto& header : response->headers_to_add) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *callbacks_, header.first.get(), header.second); - request_headers_->setCopy(header.first, header.second); + ENVOY_STREAM_LOG(trace, " '{}':'{}'", *callbacks_, header.first.get(), header.second); + Http::HeaderEntry* header_to_modify = request_headers_->get(header.first); + if (header_to_modify) { + header_to_modify->value(header.second.c_str(), header.second.size()); + } else { + request_headers_->addCopy(header.first, header.second); + } } for (const auto& header : response->headers_to_append) { - const Http::HeaderEntry* header_to_modify = request_headers_->get(header.first); + Http::HeaderEntry* header_to_modify = request_headers_->get(header.first); if (header_to_modify) { - ENVOY_STREAM_LOG(trace, "'{}':'{}'", *callbacks_, header.first.get(), header.second); - request_headers_->append(header.first, header.second); + ENVOY_STREAM_LOG(trace, " '{}':'{}'", *callbacks_, header.first.get(), header.second); + Http::HeaderMapImpl::appendToHeader(header_to_modify->value(), header.second); } } if (cluster_) { diff --git a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc index ab947cb9940e1..1bf8bc4a2c2f6 100644 --- a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc +++ b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc @@ -75,7 +75,7 @@ Http::FilterTrailersStatus Http1BridgeFilter::encodeTrailers(Http::HeaderMap& tr uint64_t grpc_status_code; if (!absl::SimpleAtoi(grpc_status_header->value().getStringView(), &grpc_status_code) || grpc_status_code != 0) { - response_headers_->setStatus(enumToInt(Http::Code::ServiceUnavailable)); + response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable)); } response_headers_->setGrpcStatus(grpc_status_header->value().getStringView()); } diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc index b669d6741ba78..ffec82888466d 100644 --- a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc +++ b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc @@ -57,7 +57,7 @@ void adjustContentLength(Http::HeaderMap& headers, if (length_header != nullptr) { uint64_t length; if (absl::SimpleAtoi(length_header->value().getStringView(), &length)) { - headers.setContentLength(adjustment(length)); + length_header->value(adjustment(length)); } } } @@ -137,7 +137,7 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::HeaderMap& headers, bool) headers.setStatus(enumToInt(Http::Code::OK)); if (content_type != nullptr) { - headers.setContentType(content_type_); + content_type->value(content_type_); } decoder_callbacks_->streamInfo().setResponseCodeDetails( @@ -146,7 +146,7 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::HeaderMap& headers, bool) } // Restore the content-type to match what the downstream sent. - headers.setContentType(content_type_); + content_type->value(content_type_); if (withhold_grpc_frames_) { // Adjust content-length to account for the frame header that's added. diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index a89960942d60a..345b0bf4e4eed 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -487,9 +487,9 @@ Http::FilterTrailersStatus JsonTranscoderFilter::encodeTrailers(Http::HeaderMap& bool is_trailers_only_response = response_headers_ == &trailers; if (!grpc_status || grpc_status.value() == Grpc::Status::WellKnownGrpcStatus::InvalidCode) { - response_headers_->setStatus(enumToInt(Http::Code::ServiceUnavailable)); + response_headers_->Status()->value(enumToInt(Http::Code::ServiceUnavailable)); } else { - response_headers_->setStatus(Grpc::Utility::grpcToHttpStatus(grpc_status.value())); + response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status.value())); if (!is_trailers_only_response) { response_headers_->setGrpcStatus(grpc_status.value()); } @@ -594,7 +594,7 @@ bool JsonTranscoderFilter::maybeConvertGrpcStatus(Grpc::Status::GrpcStatus grpc_ return false; } - response_headers_->setStatus(Grpc::Utility::grpcToHttpStatus(grpc_status)); + response_headers_->Status()->value(Grpc::Utility::grpcToHttpStatus(grpc_status)); bool is_trailers_only_response = response_headers_ == &trailers; if (is_trailers_only_response) { diff --git a/source/extensions/filters/http/health_check/health_check.cc b/source/extensions/filters/http/health_check/health_check.cc index e66f82f8fe7be..f22f39d1c8b4c 100644 --- a/source/extensions/filters/http/health_check/health_check.cc +++ b/source/extensions/filters/http/health_check/health_check.cc @@ -178,7 +178,7 @@ void HealthCheckFilter::onComplete() { final_status, "", [degraded](auto& headers) { if (degraded) { - headers.setEnvoyDegraded(""); + headers.insertEnvoyDegraded(); } }, absl::nullopt, *details); diff --git a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc index f4449bdb0315b..6f92aac73a01d 100644 --- a/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc +++ b/source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc @@ -78,7 +78,7 @@ Http::FilterHeadersStatus IpTaggingFilter::decodeHeaders(Http::HeaderMap& header if (!tags.empty()) { const std::string tags_join = absl::StrJoin(tags, ","); - headers.appendEnvoyIpTags(tags_join, ","); + Http::HeaderMapImpl::appendToHeader(headers.insertEnvoyIpTags().value(), tags_join); // We must clear the route cache or else we can't match on x-envoy-ip-tags. // TODO(rgs): this should either be configurable, because it's expensive, or optimized. diff --git a/source/extensions/filters/http/lua/wrappers.cc b/source/extensions/filters/http/lua/wrappers.cc index a772f3c1edfe1..4874a716cec99 100644 --- a/source/extensions/filters/http/lua/wrappers.cc +++ b/source/extensions/filters/http/lua/wrappers.cc @@ -79,7 +79,12 @@ int HeaderMapWrapper::luaReplace(lua_State* state) { const char* value = luaL_checkstring(state, 3); const Http::LowerCaseString lower_key(key); - headers_.setCopy(lower_key, value); + Http::HeaderEntry* entry = headers_.get(lower_key); + if (entry != nullptr) { + entry->value(value, strlen(value)); + } else { + headers_.addCopy(lower_key, value); + } return 0; } diff --git a/source/extensions/filters/http/rbac/rbac_filter.cc b/source/extensions/filters/http/rbac/rbac_filter.cc index a750dd47cc47c..f184234f4d4cd 100644 --- a/source/extensions/filters/http/rbac/rbac_filter.cc +++ b/source/extensions/filters/http/rbac/rbac_filter.cc @@ -73,6 +73,10 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head if (shadow_engine != nullptr) { std::string shadow_resp_code = Filters::Common::RBAC::DynamicMetadataKeysSingleton::get().EngineResultAllowed; + // Refresh headers byte size before checking if allowed. + // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and + // HeaderMap holds an accurate internal byte size count. + headers.refreshByteSize(); if (shadow_engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), &effective_policy_id)) { ENVOY_LOG(debug, "shadow allowed"); @@ -102,6 +106,10 @@ Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::Head const auto engine = config_->engine(callbacks_->route(), Filters::Common::RBAC::EnforcementMode::Enforced); if (engine != nullptr) { + // Refresh headers byte size before checking if allowed. + // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and + // HeaderMap holds an accurate internal byte size count. + headers.refreshByteSize(); if (engine->allowed(*callbacks_->connection(), headers, callbacks_->streamInfo(), nullptr)) { ENVOY_LOG(debug, "enforced allowed"); config_->stats().allowed_.inc(); diff --git a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc index e5445320a4c9a..38c798551bd6c 100644 --- a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc +++ b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc @@ -26,7 +26,7 @@ class OpenTracingHTTPHeadersWriter : public opentracing::HTTPHeadersWriter { opentracing::string_view value) const override { Http::LowerCaseString lowercase_key{key}; request_headers_.remove(lowercase_key); - request_headers_.addCopy(std::move(lowercase_key), {value.data(), value.size()}); + request_headers_.addCopy(std::move(lowercase_key), value); return {}; } diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 3059ed6a303b1..88761b4026766 100644 --- a/test/common/grpc/common_test.cc +++ b/test/common/grpc/common_test.cc @@ -107,28 +107,28 @@ TEST(GrpcCommonTest, GrpcStatusDetailsBin) { } TEST(GrpcContextTest, ToGrpcTimeout) { - Http::TestHeaderMapImpl headers; + Http::HeaderString value; - Common::toGrpcTimeout(std::chrono::milliseconds(0UL), headers); - EXPECT_EQ("0m", headers.GrpcTimeout()->value().getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(0UL), value); + EXPECT_EQ("0m", value.getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(1UL), headers); - EXPECT_EQ("1m", headers.GrpcTimeout()->value().getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(1UL), value); + EXPECT_EQ("1m", value.getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(100000000UL), headers); - EXPECT_EQ("100000S", headers.GrpcTimeout()->value().getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(100000000UL), value); + EXPECT_EQ("100000S", value.getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(100000000000UL), headers); - EXPECT_EQ("1666666M", headers.GrpcTimeout()->value().getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(100000000000UL), value); + EXPECT_EQ("1666666M", value.getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(9000000000000UL), headers); - EXPECT_EQ("2500000H", headers.GrpcTimeout()->value().getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(9000000000000UL), value); + EXPECT_EQ("2500000H", value.getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(360000000000000UL), headers); - EXPECT_EQ("99999999H", headers.GrpcTimeout()->value().getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(360000000000000UL), value); + EXPECT_EQ("99999999H", value.getStringView()); - Common::toGrpcTimeout(std::chrono::milliseconds(UINT64_MAX), headers); - EXPECT_EQ("99999999H", headers.GrpcTimeout()->value().getStringView()); + Common::toGrpcTimeout(std::chrono::milliseconds(UINT64_MAX), value); + EXPECT_EQ("99999999H", value.getStringView()); } TEST(GrpcContextTest, PrepareHeaders) { diff --git a/test/common/http/header_map_impl_fuzz.proto b/test/common/http/header_map_impl_fuzz.proto index bebe373b6ae5b..9ff5279f022fa 100644 --- a/test/common/http/header_map_impl_fuzz.proto +++ b/test/common/http/header_map_impl_fuzz.proto @@ -51,25 +51,6 @@ message GetAndMutate { } } -message MutateAndMove { - string key = 1; - oneof mutate_selector { - string append = 2; - string set_copy = 3; - uint64 set_integer = 4; - string set_reference = 5; - } -} - -message Append { - string key = 1; - string value = 2; -} - -message Get { - string key = 1; -} - message Action { oneof action_selector { option (validate.required) = true; @@ -78,10 +59,7 @@ message Action { AddCopy add_copy = 3; SetReference set_reference = 4; SetReferenceKey set_reference_key = 5; - GetAndMutate get_and_mutate = 6 [deprecated = true]; - Get get = 13; - MutateAndMove mutate_and_move = 12; - Append append = 11; + GetAndMutate get_and_mutate = 6; google.protobuf.Empty copy = 7; string lookup = 8; string remove = 9; diff --git a/test/common/http/header_map_impl_fuzz_test.cc b/test/common/http/header_map_impl_fuzz_test.cc index 1c68488e462f4..b10c2f695ad84 100644 --- a/test/common/http/header_map_impl_fuzz_test.cc +++ b/test/common/http/header_map_impl_fuzz_test.cc @@ -15,15 +15,24 @@ namespace Envoy { // Fuzz the header map implementation. DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) { Http::HeaderMapImplPtr header_map = std::make_unique(); + const auto predefined_exists = [&header_map](const std::string& s) -> bool { + const Http::HeaderEntry* entry; + return header_map->lookup(Http::LowerCaseString(replaceInvalidCharacters(s)), &entry) == + Http::HeaderMap::Lookup::Found; + }; std::vector> lower_case_strings; std::vector> strings; - constexpr auto max_actions = 128; + constexpr auto max_actions = 1024; for (int i = 0; i < std::min(max_actions, input.actions().size()); ++i) { const auto& action = input.actions(i); ENVOY_LOG_MISC(debug, "Action {}", action.DebugString()); switch (action.action_selector_case()) { case test::common::http::Action::kAddReference: { const auto& add_reference = action.add_reference(); + // Workaround for https://github.com/envoyproxy/envoy/issues/3919. + if (predefined_exists(add_reference.key())) { + continue; + } lower_case_strings.emplace_back( std::make_unique(replaceInvalidCharacters(add_reference.key()))); strings.emplace_back( @@ -33,6 +42,10 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) } case test::common::http::Action::kAddReferenceKey: { const auto& add_reference_key = action.add_reference_key(); + // Workaround for https://github.com/envoyproxy/envoy/issues/3919. + if (predefined_exists(add_reference_key.key())) { + continue; + } lower_case_strings.emplace_back(std::make_unique( replaceInvalidCharacters(add_reference_key.key()))); switch (add_reference_key.value_selector_case()) { @@ -50,6 +63,10 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) } case test::common::http::Action::kAddCopy: { const auto& add_copy = action.add_copy(); + // Workaround for https://github.com/envoyproxy/envoy/issues/3919. + if (predefined_exists(add_copy.key())) { + continue; + } const Http::LowerCaseString key{replaceInvalidCharacters(add_copy.key())}; switch (add_copy.value_selector_case()) { case test::common::http::AddCopy::kStringValue: @@ -80,58 +97,47 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) replaceInvalidCharacters(set_reference_key.value())); break; } - case test::common::http::Action::kGet: { - const auto& get = action.get(); - const auto* header_entry = - header_map->get(Http::LowerCaseString(replaceInvalidCharacters(get.key()))); + case test::common::http::Action::kGetAndMutate: { + const auto& get_and_mutate = action.get_and_mutate(); + auto* header_entry = + header_map->get(Http::LowerCaseString(replaceInvalidCharacters(get_and_mutate.key()))); if (header_entry != nullptr) { // Do some read-only stuff. (void)strlen(std::string(header_entry->key().getStringView()).c_str()); (void)strlen(std::string(header_entry->value().getStringView()).c_str()); + (void)strlen(header_entry->value().buffer()); header_entry->key().empty(); header_entry->value().empty(); + // Do some mutation or parameterized action. + switch (get_and_mutate.mutate_selector_case()) { + case test::common::http::GetAndMutate::kAppend: + header_entry->value().append(replaceInvalidCharacters(get_and_mutate.append()).c_str(), + get_and_mutate.append().size()); + break; + case test::common::http::GetAndMutate::kClear: + header_entry->value().clear(); + break; + case test::common::http::GetAndMutate::kFind: + header_entry->value().getStringView().find(get_and_mutate.find()); + break; + case test::common::http::GetAndMutate::kSetCopy: + header_entry->value().setCopy(replaceInvalidCharacters(get_and_mutate.set_copy()).c_str(), + get_and_mutate.set_copy().size()); + break; + case test::common::http::GetAndMutate::kSetInteger: + header_entry->value().setInteger(get_and_mutate.set_integer()); + break; + case test::common::http::GetAndMutate::kSetReference: + strings.emplace_back(std::make_unique( + replaceInvalidCharacters(get_and_mutate.set_reference()))); + header_entry->value().setReference(*strings.back()); + break; + default: + break; + } } break; } - case test::common::http::Action::kMutateAndMove: { - const auto& mutate_and_move = action.mutate_and_move(); - Http::HeaderString header_field( - Http::LowerCaseString(replaceInvalidCharacters(mutate_and_move.key()))); - Http::HeaderString header_value; - // Do some mutation or parameterized action. - switch (mutate_and_move.mutate_selector_case()) { - case test::common::http::MutateAndMove::kAppend: - header_value.append(replaceInvalidCharacters(mutate_and_move.append()).c_str(), - mutate_and_move.append().size()); - break; - case test::common::http::MutateAndMove::kSetCopy: - header_value.setCopy(replaceInvalidCharacters(mutate_and_move.set_copy())); - break; - case test::common::http::MutateAndMove::kSetInteger: - header_value.setInteger(mutate_and_move.set_integer()); - break; - case test::common::http::MutateAndMove::kSetReference: - strings.emplace_back(std::make_unique( - replaceInvalidCharacters(mutate_and_move.set_reference()))); - header_value.setReference(*strings.back()); - break; - default: - break; - } - // Can't addViaMove on an empty header value. - if (!header_value.empty()) { - header_map->addViaMove(std::move(header_field), std::move(header_value)); - } - break; - } - case test::common::http::Action::kAppend: { - const auto& append = action.append(); - lower_case_strings.emplace_back( - std::make_unique(replaceInvalidCharacters(append.key()))); - strings.emplace_back(std::make_unique(replaceInvalidCharacters(append.value()))); - header_map->append(*lower_case_strings.back(), *strings.back()); - break; - } case test::common::http::Action::kCopy: { header_map = std::make_unique( *reinterpret_cast(header_map.get())); @@ -152,17 +158,13 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) Http::LowerCaseString(replaceInvalidCharacters(action.remove_prefix()))); break; } - case test::common::http::Action::kGetAndMutate: { - // Deprecated. Can not get and mutate entries. - break; - } default: // Maybe nothing is set? break; } // Exercise some read-only accessors. - header_map->size(); header_map->byteSize(); + header_map->size(); header_map->iterate( [](const Http::HeaderEntry& header, void * /*context*/) -> Http::HeaderMap::Iterate { header.key(); diff --git a/test/common/http/header_map_impl_speed_test.cc b/test/common/http/header_map_impl_speed_test.cc index 6989b51517d7e..62c883419ab15 100644 --- a/test/common/http/header_map_impl_speed_test.cc +++ b/test/common/http/header_map_impl_speed_test.cc @@ -74,7 +74,7 @@ static void HeaderMapImplGetInline(benchmark::State& state) { const std::string value("01234567890123456789"); HeaderMapImpl headers; addDummyHeaders(headers, state.range(0)); - headers.setReferenceConnection(value); + headers.insertConnection().value().setReference(value); size_t size = 0; for (auto _ : state) { size += headers.Connection()->value().size(); @@ -83,6 +83,21 @@ static void HeaderMapImplGetInline(benchmark::State& state) { } BENCHMARK(HeaderMapImplGetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50)->Arg(200); +/** + * Measure the speed of writing to a header for which HeaderMapImpl is expected to + * provide special optimizations. + */ +static void HeaderMapImplSetInline(benchmark::State& state) { + const std::string value("01234567890123456789"); + HeaderMapImpl headers; + addDummyHeaders(headers, state.range(0)); + for (auto _ : state) { + headers.insertConnection().value().setReference(value); + } + benchmark::DoNotOptimize(headers.size()); +} +BENCHMARK(HeaderMapImplSetInline)->Arg(0)->Arg(1)->Arg(10)->Arg(50); + /** * Measure the speed of writing to a header for which HeaderMapImpl is expected to * provide special optimizations. @@ -119,7 +134,7 @@ static void HeaderMapImplGetByteSize(benchmark::State& state) { addDummyHeaders(headers, state.range(0)); uint64_t size = 0; for (auto _ : state) { - size += headers.byteSize(); + size += headers.byteSize().value(); } benchmark::DoNotOptimize(size); } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 3652091b090d8..4ade5f79bbce9 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -57,7 +57,7 @@ TEST(HeaderStringTest, All) { // Inline move constructor { HeaderString string; - string.setCopy("hello"); + string.setCopy("hello", 5); EXPECT_EQ(HeaderString::Type::Inline, string.type()); HeaderString string2(std::move(string)); EXPECT_TRUE(string.empty()); // NOLINT(bugprone-use-after-move) @@ -74,7 +74,7 @@ TEST(HeaderStringTest, All) { { std::string large(4096, 'a'); HeaderString string; - string.setCopy(large); + string.setCopy(large.c_str(), large.size()); EXPECT_EQ(HeaderString::Type::Dynamic, string.type()); HeaderString string2(std::move(string)); EXPECT_TRUE(string.empty()); // NOLINT(bugprone-use-after-move) @@ -100,7 +100,7 @@ TEST(HeaderStringTest, All) { { std::string static_string("HELLO"); HeaderString string(static_string); - string.setCopy(static_string); + string.setCopy(static_string.c_str(), static_string.size()); EXPECT_EQ(HeaderString::Type::Inline, string.type()); EXPECT_EQ("HELLO", string.getStringView()); } @@ -127,7 +127,7 @@ TEST(HeaderStringTest, All) { // Copy inline { HeaderString string; - string.setCopy("hello"); + string.setCopy("hello", 5); EXPECT_EQ("hello", string.getStringView()); EXPECT_EQ(5U, string.size()); } @@ -136,7 +136,7 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value(4096, 'a'); - string.setCopy(large_value); + string.setCopy(large_value.c_str(), large_value.size()); EXPECT_EQ(large_value, string.getStringView()); EXPECT_NE(large_value.c_str(), string.getStringView().data()); EXPECT_EQ(4096U, string.size()); @@ -146,9 +146,9 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value1(4096, 'a'); - string.setCopy(large_value1); + string.setCopy(large_value1.c_str(), large_value1.size()); std::string large_value2(2048, 'b'); - string.setCopy(large_value2); + string.setCopy(large_value2.c_str(), large_value2.size()); EXPECT_EQ(large_value2, string.getStringView()); EXPECT_NE(large_value2.c_str(), string.getStringView().data()); EXPECT_EQ(2048U, string.size()); @@ -158,9 +158,9 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value1(4096, 'a'); - string.setCopy(large_value1); + string.setCopy(large_value1.c_str(), large_value1.size()); std::string large_value2(16384, 'b'); - string.setCopy(large_value2); + string.setCopy(large_value2.c_str(), large_value2.size()); EXPECT_EQ(large_value2, string.getStringView()); EXPECT_NE(large_value2.c_str(), string.getStringView().data()); EXPECT_EQ(16384U, string.size()); @@ -170,9 +170,9 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large_value1(16, 'a'); - string.setCopy(large_value1); + string.setCopy(large_value1.c_str(), large_value1.size()); std::string large_value2(16384, 'b'); - string.setCopy(large_value2); + string.setCopy(large_value2.c_str(), large_value2.size()); EXPECT_EQ(large_value2, string.getStringView()); EXPECT_NE(large_value2.c_str(), string.getStringView().data()); EXPECT_EQ(16384U, string.size()); @@ -187,7 +187,7 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large(128, 'z'); - string.setCopy(large); + string.setCopy(large.c_str(), large.size()); EXPECT_EQ(string.type(), HeaderString::Type::Inline); EXPECT_EQ(string.getStringView(), large); } @@ -196,11 +196,11 @@ TEST(HeaderStringTest, All) { { HeaderString string; std::string large(128, 'z'); - string.setCopy(large); + string.setCopy(large.c_str(), large.size()); EXPECT_EQ(string.type(), HeaderString::Type::Inline); EXPECT_EQ(string.getStringView(), large); std::string small(1, 'a'); - string.setCopy(small); + string.setCopy(small.c_str(), small.size()); EXPECT_EQ(string.type(), HeaderString::Type::Inline); EXPECT_EQ(string.getStringView(), small); // If we peek past the valid first character of the @@ -218,13 +218,13 @@ TEST(HeaderStringTest, All) { HeaderString string; // Force Dynamic with setCopy of inline buffer size + 1. std::string large1(129, 'z'); - string.setCopy(large1); + string.setCopy(large1.c_str(), large1.size()); EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); const void* dynamic_buffer_address = string.getStringView().data(); // Dynamic capacity in setCopy is 2x required by the size. // So to fill it exactly setCopy with a total of 258 chars. std::string large2(258, 'z'); - string.setCopy(large2); + string.setCopy(large2.c_str(), large2.size()); EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); // The actual buffer address should be the same as it was after // setCopy(large1), ensuring no reallocation occurred. @@ -295,7 +295,7 @@ TEST(HeaderStringTest, All) { HeaderString string; // Force Dynamic with setCopy of inline buffer size + 1. std::string large1(129, 'z'); - string.setCopy(large1); + string.setCopy(large1.c_str(), large1.size()); EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); const void* dynamic_buffer_address = string.getStringView().data(); // Dynamic capacity in setCopy is 2x required by the size. @@ -338,7 +338,7 @@ TEST(HeaderStringTest, All) { EXPECT_EQ(HeaderString::Type::Reference, string.type()); const std::string large(129, 'a'); - string.setCopy(large); + string.setCopy(large.c_str(), large.size()); EXPECT_NE(string.getStringView().data(), large.c_str()); EXPECT_EQ(HeaderString::Type::Dynamic, string.type()); @@ -366,8 +366,9 @@ TEST(HeaderMapImplTest, InlineInsert) { HeaderMapImpl headers; EXPECT_TRUE(headers.empty()); EXPECT_EQ(0, headers.size()); + EXPECT_EQ(headers.byteSize().value(), 0); EXPECT_EQ(nullptr, headers.Host()); - headers.setHost("hello"); + headers.insertHost().value(std::string("hello")); EXPECT_FALSE(headers.empty()); EXPECT_EQ(1, headers.size()); EXPECT_EQ(":authority", headers.Host()->key().getStringView()); @@ -375,50 +376,17 @@ TEST(HeaderMapImplTest, InlineInsert) { EXPECT_EQ("hello", headers.get(Headers::get().Host)->value().getStringView()); } -TEST(HeaderMapImplTest, InlineAppend) { - { - HeaderMapImpl headers; - // Create via header and append. - headers.setVia(""); - headers.appendVia("1.0 fred", ","); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); - headers.appendVia("1.1 nowhere.com", ","); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 nowhere.com"); - } - { - // Append to via header without explicitly creating first. - HeaderMapImpl headers; - headers.appendVia("1.0 fred", ","); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); - headers.appendVia("1.1 nowhere.com", ","); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 nowhere.com"); - } - { - // Custom delimiter. - HeaderMapImpl headers; - headers.setVia(""); - headers.appendVia("1.0 fred", ", "); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); - headers.appendVia("1.1 nowhere.com", ", "); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred, 1.1 nowhere.com"); - } - { - // Append and then later set. - HeaderMapImpl headers; - headers.appendVia("1.0 fred", ","); - headers.appendVia("1.1 nowhere.com", ","); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 nowhere.com"); - headers.setVia("2.0 override"); - EXPECT_EQ(headers.Via()->value().getStringView(), "2.0 override"); - } - { - // Set and then append. This mimics how GrpcTimeout is set. - HeaderMapImpl headers; - headers.setGrpcTimeout(42); - EXPECT_EQ(headers.GrpcTimeout()->value().getStringView(), "42"); - headers.appendGrpcTimeout("s", ""); - EXPECT_EQ(headers.GrpcTimeout()->value().getStringView(), "42s"); - } +// Utility function for testing byteSize() against a manual byte count. +uint64_t countBytesForTest(const HeaderMapImpl& headers) { + uint64_t byte_size = 0; + headers.iterate( + [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { + auto* byte_size = static_cast(context); + *byte_size += header.key().getStringView().size() + header.value().getStringView().size(); + return Http::HeaderMap::Iterate::Continue; + }, + &byte_size); + return byte_size; } TEST(HeaderMapImplTest, MoveIntoInline) { @@ -426,18 +394,19 @@ TEST(HeaderMapImplTest, MoveIntoInline) { HeaderString key; key.setCopy(Headers::get().CacheControl.get()); HeaderString value; - value.setCopy("hello"); + value.setCopy("hello", 5); headers.addViaMove(std::move(key), std::move(value)); EXPECT_EQ("cache-control", headers.CacheControl()->key().getStringView()); EXPECT_EQ("hello", headers.CacheControl()->value().getStringView()); HeaderString key2; - key2.setCopy(Headers::get().CacheControl.get()); + key2.setCopy(Headers::get().CacheControl.get().c_str(), Headers::get().CacheControl.get().size()); HeaderString value2; - value2.setCopy("there"); + value2.setCopy("there", 5); headers.addViaMove(std::move(key2), std::move(value2)); EXPECT_EQ("cache-control", headers.CacheControl()->key().getStringView()); EXPECT_EQ("hello,there", headers.CacheControl()->value().getStringView()); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); } TEST(HeaderMapImplTest, Remove) { @@ -447,6 +416,7 @@ TEST(HeaderMapImplTest, Remove) { LowerCaseString static_key("hello"); std::string ref_value("value"); headers.addReference(static_key, ref_value); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("value", headers.get(static_key)->value().getStringView()); EXPECT_EQ(HeaderString::Type::Reference, headers.get(static_key)->value().type()); EXPECT_EQ(1UL, headers.size()); @@ -455,9 +425,11 @@ TEST(HeaderMapImplTest, Remove) { EXPECT_EQ(nullptr, headers.get(static_key)); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); + EXPECT_EQ(headers.refreshByteSize(), 0); // Add and remove by inline. - headers.setContentLength(5); + headers.insertContentLength().value(5); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); EXPECT_FALSE(headers.empty()); @@ -465,16 +437,19 @@ TEST(HeaderMapImplTest, Remove) { EXPECT_EQ(nullptr, headers.ContentLength()); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); // Add inline and remove by name. - headers.setContentLength(5); + headers.insertContentLength().value(5); EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); EXPECT_FALSE(headers.empty()); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); headers.remove(Headers::get().ContentLength); EXPECT_EQ(nullptr, headers.ContentLength()); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); + EXPECT_EQ(headers.refreshByteSize(), 0); } TEST(HeaderMapImplTest, RemoveRegex) { @@ -492,9 +467,11 @@ TEST(HeaderMapImplTest, RemoveRegex) { headers.addReference(key3, "value"); headers.addReference(key4, "value"); headers.addReference(key5, "value"); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); // Test removing the first header, middle headers, and the end header. headers.removePrefix(LowerCaseString("x-prefix-")); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ(nullptr, headers.get(key1)); EXPECT_NE(nullptr, headers.get(key2)); EXPECT_EQ(nullptr, headers.get(key3)); @@ -502,17 +479,21 @@ TEST(HeaderMapImplTest, RemoveRegex) { EXPECT_EQ(nullptr, headers.get(key5)); // Remove all headers. + headers.refreshByteSize(); headers.removePrefix(LowerCaseString("")); + EXPECT_EQ(headers.byteSize().value(), 0); EXPECT_EQ(nullptr, headers.get(key2)); EXPECT_EQ(nullptr, headers.get(key4)); // Add inline and remove by regex - headers.setContentLength(5); + headers.insertContentLength().value(5); EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); EXPECT_FALSE(headers.empty()); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); headers.removePrefix(LowerCaseString("content")); EXPECT_EQ(nullptr, headers.ContentLength()); + EXPECT_EQ(headers.refreshByteSize(), 0); } TEST(HeaderMapImplTest, SetRemovesAllValues) { @@ -530,6 +511,7 @@ TEST(HeaderMapImplTest, SetRemovesAllValues) { headers.addReference(key2, ref_value2); headers.addReference(key1, ref_value3); headers.addReference(key1, ref_value4); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); using MockCb = testing::MockFunction; @@ -577,6 +559,7 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { const std::string bar("bar"); headers.addReference(Headers::get().ContentLength, foo); headers.addReference(Headers::get().ContentLength, bar); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("foo,bar", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -584,6 +567,7 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { HeaderMapImpl headers; headers.addReferenceKey(Headers::get().ContentLength, "foo"); headers.addReferenceKey(Headers::get().ContentLength, "bar"); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("foo,bar", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -591,6 +575,7 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { HeaderMapImpl headers; headers.addReferenceKey(Headers::get().ContentLength, 5); headers.addReferenceKey(Headers::get().ContentLength, 6); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("5,6", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -599,6 +584,7 @@ TEST(HeaderMapImplTest, DoubleInlineAdd) { const std::string foo("foo"); headers.addReference(Headers::get().ContentLength, foo); headers.addReferenceKey(Headers::get().ContentLength, 6); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("foo,6", headers.ContentLength()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -614,6 +600,7 @@ TEST(HeaderMapImplTest, DoubleCookieAdd) { headers.addReference(set_cookie, foo); headers.addReference(set_cookie, bar); EXPECT_EQ(2UL, headers.size()); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); std::vector out; Http::HeaderUtility::getAllOfHeader(headers, "set-cookie", out); @@ -626,6 +613,7 @@ TEST(HeaderMapImplTest, DoubleInlineSet) { HeaderMapImpl headers; headers.setReferenceKey(Headers::get().ContentType, "blah"); headers.setReferenceKey(Headers::get().ContentType, "text/html"); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("text/html", headers.ContentType()->value().getStringView()); EXPECT_EQ(1UL, headers.size()); } @@ -634,6 +622,7 @@ TEST(HeaderMapImplTest, AddReferenceKey) { HeaderMapImpl headers; LowerCaseString foo("hello"); headers.addReferenceKey(foo, "world"); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_NE("world", headers.get(foo)->value().getStringView().data()); EXPECT_EQ("world", headers.get(foo)->value().getStringView()); } @@ -642,72 +631,24 @@ TEST(HeaderMapImplTest, SetReferenceKey) { HeaderMapImpl headers; LowerCaseString foo("hello"); headers.setReferenceKey(foo, "world"); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_NE("world", headers.get(foo)->value().getStringView().data()); EXPECT_EQ("world", headers.get(foo)->value().getStringView()); + headers.refreshByteSize(); headers.setReferenceKey(foo, "monde"); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_NE("monde", headers.get(foo)->value().getStringView().data()); EXPECT_EQ("monde", headers.get(foo)->value().getStringView()); } -TEST(HeaderMapImplTest, SetCopy) { - HeaderMapImpl headers; - LowerCaseString foo("hello"); - headers.setCopy(foo, "world"); - EXPECT_EQ("world", headers.get(foo)->value().getStringView()); - - // Overwrite value. - headers.setCopy(foo, "monde"); - EXPECT_EQ("monde", headers.get(foo)->value().getStringView()); - - // Add another foo header. - headers.addCopy(foo, "monde2"); - EXPECT_EQ(headers.size(), 2); - - // Make sure all foo headers are overridden. - headers.setCopy(foo, "override-monde"); - EXPECT_EQ(headers.size(), 2); - - using MockCb = testing::MockFunction; - MockCb cb; - - InSequence seq; - EXPECT_CALL(cb, Call("hello", "override-monde")); - EXPECT_CALL(cb, Call("hello", "override-monde")); - 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); - - // Test setting an empty string and then overriding. - headers.remove(foo); - EXPECT_EQ(headers.size(), 0); - const std::string empty; - headers.setCopy(foo, empty); - EXPECT_EQ(headers.size(), 1); - headers.setCopy(foo, "not-empty"); - EXPECT_EQ(headers.get(foo)->value().getStringView(), "not-empty"); - - // Use setCopy with inline headers both indirectly and directly. - headers.clear(); - EXPECT_EQ(headers.size(), 0); - headers.setCopy(Headers::get().Path, "/"); - EXPECT_EQ(headers.size(), 1); - EXPECT_EQ(headers.Path()->value().getStringView(), "/"); - headers.setPath("/foo"); - EXPECT_EQ(headers.size(), 1); - EXPECT_EQ(headers.Path()->value().getStringView(), "/foo"); -} - TEST(HeaderMapImplTest, AddCopy) { HeaderMapImpl headers; // Start with a string value. std::unique_ptr lcKeyPtr(new LowerCaseString("hello")); headers.addCopy(*lcKeyPtr, "world"); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); const HeaderString& value = headers.get(*lcKeyPtr)->value(); @@ -728,15 +669,18 @@ TEST(HeaderMapImplTest, AddCopy) { // addReferenceKey and addCopy can both add multiple instances of a // given header, so we need to delete the old "hello" header. // Test that removing will return 0 byte size. + headers.refreshByteSize(); headers.remove(LowerCaseString("hello")); - EXPECT_EQ(headers.byteSize(), 0); + EXPECT_EQ(headers.byteSize().value(), 0); // Build "hello" with string concatenation to make it unlikely that the // compiler is just reusing the same string constant for everything. lcKeyPtr = std::make_unique(std::string("he") + "llo"); EXPECT_STREQ("hello", lcKeyPtr->get().c_str()); + headers.refreshByteSize(); headers.addCopy(*lcKeyPtr, 42); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); const HeaderString& value3 = headers.get(*lcKeyPtr)->value(); @@ -762,15 +706,20 @@ TEST(HeaderMapImplTest, AddCopy) { headers.addCopy(cache_control, "max-age=1345"); EXPECT_EQ("max-age=1345", headers.get(cache_control)->value().getStringView()); EXPECT_EQ("max-age=1345", headers.CacheControl()->value().getStringView()); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); headers.addCopy(cache_control, "public"); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); EXPECT_EQ("max-age=1345,public", headers.get(cache_control)->value().getStringView()); headers.addCopy(cache_control, ""); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); EXPECT_EQ("max-age=1345,public", headers.get(cache_control)->value().getStringView()); headers.addCopy(cache_control, 123); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); EXPECT_EQ("max-age=1345,public,123", headers.get(cache_control)->value().getStringView()); headers.addCopy(cache_control, std::numeric_limits::max()); EXPECT_EQ("max-age=1345,public,123,18446744073709551615", headers.get(cache_control)->value().getStringView()); + EXPECT_EQ(headers.refreshByteSize(), countBytesForTest(headers)); } TEST(HeaderMapImplTest, Equality) { @@ -790,6 +739,7 @@ TEST(HeaderMapImplTest, LargeCharInHeader) { LowerCaseString static_key("\x90hello"); std::string ref_value("value"); headers.addReference(static_key, ref_value); + EXPECT_EQ(headers.byteSize().value(), countBytesForTest(headers)); EXPECT_EQ("value", headers.get(static_key)->value().getStringView()); } @@ -847,7 +797,7 @@ TEST(HeaderMapImplTest, IterateReverse) { TEST(HeaderMapImplTest, Lookup) { TestHeaderMapImpl headers; headers.addCopy("hello", "world"); - headers.setContentLength(5); + headers.insertContentLength().value(5); // Lookup is not supported for non predefined inline headers. { @@ -881,12 +831,10 @@ TEST(HeaderMapImplTest, Get) { { TestHeaderMapImpl headers{{":path", "/"}, {"hello", "world"}}; - // There is not HeaderMap method to set a header and copy both the key and value. - headers.setReferenceKey(LowerCaseString(":path"), "/new_path"); + headers.get(LowerCaseString(":path"))->value(std::string("/new_path")); EXPECT_EQ("/new_path", headers.get(LowerCaseString(":path"))->value().getStringView()); - LowerCaseString foo("hello"); - headers.setReferenceKey(foo, "world2"); - EXPECT_EQ("world2", headers.get(foo)->value().getStringView()); + headers.get(LowerCaseString("hello"))->value(std::string("world2")); + EXPECT_EQ("world2", headers.get(LowerCaseString("hello"))->value().getStringView()); EXPECT_EQ(nullptr, headers.get(LowerCaseString("foo"))); } } @@ -894,57 +842,41 @@ TEST(HeaderMapImplTest, Get) { TEST(HeaderMapImplTest, TestAppendHeader) { // Test appending to a string with a value. { - TestHeaderMapImpl headers; - LowerCaseString foo("key1"); - headers.addCopy(foo, "some;"); - headers.append(foo, "test"); - EXPECT_EQ(headers.get(foo)->value().getStringView(), "some;,test"); + HeaderString value1; + value1.setCopy("some;", 5); + HeaderMapImpl::appendToHeader(value1, "test"); + EXPECT_EQ(value1, "some;,test"); } // Test appending to an empty string. { - TestHeaderMapImpl headers; - LowerCaseString key2("key2"); - headers.append(key2, "my tag data"); - EXPECT_EQ(headers.get(key2)->value().getStringView(), "my tag data"); + HeaderString value2; + HeaderMapImpl::appendToHeader(value2, "my tag data"); + EXPECT_EQ(value2, "my tag data"); } // Test empty data case. { - TestHeaderMapImpl headers; - LowerCaseString key3("key3"); - headers.addCopy(key3, "empty"); - headers.append(key3, ""); - EXPECT_EQ(headers.get(key3)->value().getStringView(), "empty"); + HeaderString value3; + value3.setCopy("empty", 5); + HeaderMapImpl::appendToHeader(value3, ""); + EXPECT_EQ(value3, "empty"); } // Regression test for appending to an empty string with a short string, then // setting integer. { - TestHeaderMapImpl headers; const std::string empty; - headers.setPath(empty); - // Append with default delimiter. - headers.appendPath(" ", ","); - headers.setPath(0); - EXPECT_EQ("0", headers.Path()->value().getStringView()); - EXPECT_EQ(1U, headers.Path()->value().size()); - } - // Test append for inline headers using this method and append##name. - { - TestHeaderMapImpl headers; - headers.addCopy(Headers::get().Via, "1.0 fred"); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred"); - headers.append(Headers::get().Via, "1.1 p.example.net"); - EXPECT_EQ(headers.Via()->value().getStringView(), "1.0 fred,1.1 p.example.net"); - headers.appendVia("1.1 new.example.net", ","); - EXPECT_EQ(headers.Via()->value().getStringView(), - "1.0 fred,1.1 p.example.net,1.1 new.example.net"); + HeaderString value4(empty); + HeaderMapImpl::appendToHeader(value4, " "); + value4.setInteger(0); + EXPECT_EQ("0", value4.getStringView()); + EXPECT_EQ(1U, value4.size()); } } TEST(HeaderMapImplDeathTest, TestHeaderLengthChecks) { HeaderString value; - value.setCopy("some;"); + value.setCopy("some;", 5); EXPECT_DEATH_LOG_TO_STDERR(value.append(nullptr, std::numeric_limits::max()), "Trying to allocate overly large headers."); @@ -962,6 +894,7 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { { LowerCaseString foo("hello"); Http::TestHeaderMapImpl headers{}; + EXPECT_EQ(headers.refreshByteSize(), 0); EXPECT_EQ(0UL, headers.size()); EXPECT_TRUE(headers.empty()); @@ -1185,98 +1118,51 @@ TEST(HeaderMapImplTest, TestInlineHeaderAdd) { EXPECT_TRUE(foo.Path() != nullptr); } -TEST(HeaderMapImplTest, ClearHeaderMap) { - HeaderMapImpl headers; - LowerCaseString static_key("hello"); - std::string ref_value("value"); - - // Add random header and then clear. - headers.addReference(static_key, ref_value); - EXPECT_EQ("value", headers.get(static_key)->value().getStringView()); - EXPECT_EQ(HeaderString::Type::Reference, headers.get(static_key)->value().type()); - EXPECT_EQ(1UL, headers.size()); - EXPECT_FALSE(headers.empty()); - headers.clear(); - EXPECT_EQ(nullptr, headers.get(static_key)); - EXPECT_EQ(0UL, headers.size()); - EXPECT_EQ(headers.byteSize(), 0); - EXPECT_TRUE(headers.empty()); - - // Add inline and clear. - headers.setContentLength(5); - EXPECT_EQ("5", headers.ContentLength()->value().getStringView()); - EXPECT_EQ(1UL, headers.size()); - EXPECT_FALSE(headers.empty()); - headers.clear(); - EXPECT_EQ(nullptr, headers.ContentLength()); - EXPECT_EQ(0UL, headers.size()); - EXPECT_EQ(headers.byteSize(), 0); - EXPECT_TRUE(headers.empty()); - - // Add mixture of headers. - headers.addReference(static_key, ref_value); - headers.setContentLength(5); - headers.addCopy(static_key, "new_value"); - EXPECT_EQ(3UL, headers.size()); - EXPECT_FALSE(headers.empty()); - headers.clear(); - EXPECT_EQ(nullptr, headers.ContentLength()); - EXPECT_EQ(0UL, headers.size()); - EXPECT_EQ(headers.byteSize(), 0); - EXPECT_TRUE(headers.empty()); -} - // Validates byte size is properly accounted for in different inline header setting scenarios. TEST(HeaderMapImplTest, InlineHeaderByteSize) { + uint64_t hostKeySize = Headers::get().Host.get().size(); + uint64_t statusKeySize = Headers::get().Status.get().size(); { HeaderMapImpl headers; std::string foo = "foo"; + EXPECT_EQ(headers.byteSize().value(), 0); headers.setHost(foo); - EXPECT_EQ(headers.byteSize(), 13); + EXPECT_EQ(headers.byteSize().value(), foo.size() + hostKeySize); } { - // Overwrite an inline headers with set. + // Overwrite an inline headers. HeaderMapImpl headers; std::string foo = "foo"; + EXPECT_EQ(headers.byteSize().value(), 0); headers.setHost(foo); + EXPECT_EQ(headers.byteSize().value(), foo.size() + hostKeySize); std::string big_foo = "big_foo"; headers.setHost(big_foo); - EXPECT_EQ(headers.byteSize(), 17); + EXPECT_EQ(headers.byteSize().value(), big_foo.size() + hostKeySize); } { - // Overwrite an inline headers with setReference and clear. + // Overwrite an inline headers with reference value and clear. HeaderMapImpl headers; std::string foo = "foo"; + EXPECT_EQ(headers.byteSize().value(), 0); headers.setHost(foo); + EXPECT_EQ(headers.byteSize().value(), foo.size() + hostKeySize); std::string big_foo = "big_foo"; headers.setReferenceHost(big_foo); - EXPECT_EQ(headers.byteSize(), 17); + EXPECT_EQ(headers.byteSize().value(), big_foo.size() + hostKeySize); headers.removeHost(); - EXPECT_EQ(headers.byteSize(), 0); - } - { - // Overwrite an inline headers with set integer value. - HeaderMapImpl headers; - uint64_t status = 200; - headers.setStatus(status); - EXPECT_EQ(headers.byteSize(), 10); - uint64_t newStatus = 500; - headers.setStatus(newStatus); - EXPECT_EQ(headers.byteSize(), 10); - headers.removeStatus(); - EXPECT_EQ(headers.byteSize(), 0); + EXPECT_EQ(headers.byteSize().value(), 0); } { - // Set an inline header, remove, and rewrite. + // Overwrite an inline headers with integer value. HeaderMapImpl headers; uint64_t status = 200; + EXPECT_EQ(headers.byteSize().value(), 0); headers.setStatus(status); - EXPECT_EQ(headers.byteSize(), 10); - headers.removeStatus(); - EXPECT_EQ(headers.byteSize(), 0); + EXPECT_EQ(headers.byteSize().value(), 3 + statusKeySize); uint64_t newStatus = 500; headers.setStatus(newStatus); - EXPECT_EQ(headers.byteSize(), 10); + EXPECT_EQ(headers.byteSize().value(), 3 + statusKeySize); } } diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index c2784c8dcf19c..ea9d5c4809569 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1143,17 +1143,19 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAtLimitAccepted) { TestHeaderMapImpl request_headers; HttpTestUtility::addDefaultHeaders(request_headers); + // Refresh byte size after adding default inline headers by reference. + request_headers.refreshByteSize(); std::string key = "big"; uint32_t head_room = 77; uint32_t long_string_length = - codec_limit_kb * 1024 - request_headers.byteSize() - key.length() - head_room; + codec_limit_kb * 1024 - request_headers.byteSize().value() - key.length() - head_room; std::string long_string = std::string(long_string_length, 'q'); request_headers.addCopy(key, long_string); // The amount of data sent to the codec is not equivalent to the size of the // request headers that Envoy computes, as the codec limits based on the // entire http2 frame. The exact head room needed (76) was found through iteration. - ASSERT_EQ(request_headers.byteSize() + head_room, codec_limit_kb * 1024); + ASSERT_EQ(request_headers.byteSize().value() + head_room, codec_limit_kb * 1024); EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)); request_encoder_->encodeHeaders(request_headers, true); diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc index c2a7230f63005..7f82742403884 100644 --- a/test/common/http/path_utility_test.cc +++ b/test/common/http/path_utility_test.cc @@ -14,7 +14,7 @@ class PathUtilityTest : public testing::Test { // This is an indirect way to build a header entry for // PathUtil::canonicalPath(), since we don't have direct access to the // HeaderMapImpl constructor. - const HeaderEntry& pathHeaderEntry(const std::string& path_value) { + HeaderEntry& pathHeaderEntry(const std::string& path_value) { headers_.setPath(path_value); return *headers_.Path(); } @@ -26,7 +26,7 @@ TEST_F(PathUtilityTest, AlreadyNormalPaths) { const std::vector normal_paths{"/xyz", "/x/y/z"}; for (const auto& path : normal_paths) { auto& path_header = pathHeaderEntry(path); - const auto result = PathUtil::canonicalPath(headers_); + const auto result = PathUtil::canonicalPath(path_header); EXPECT_TRUE(result) << "original path: " << path; EXPECT_EQ(path_header.value().getStringView(), absl::string_view(path)); } @@ -37,8 +37,8 @@ TEST_F(PathUtilityTest, InvalidPaths) { const std::vector invalid_paths{"/xyz/.%00../abc", "/xyz/%00.%00./abc", "/xyz/AAAAA%%0000/abc"}; for (const auto& path : invalid_paths) { - pathHeaderEntry(path); - EXPECT_FALSE(PathUtil::canonicalPath(headers_)) << "original path: " << path; + auto& path_header = pathHeaderEntry(path); + EXPECT_FALSE(PathUtil::canonicalPath(path_header)) << "original path: " << path; } } @@ -57,7 +57,7 @@ TEST_F(PathUtilityTest, NormalizeValidPaths) { for (const auto& path_pair : non_normal_pairs) { auto& path_header = pathHeaderEntry(path_pair.first); - const auto result = PathUtil::canonicalPath(headers_); + const auto result = PathUtil::canonicalPath(path_header); EXPECT_TRUE(result) << "original path: " << path_pair.first; EXPECT_EQ(path_header.value().getStringView(), path_pair.second) << "original path: " << path_pair.second; @@ -75,7 +75,7 @@ TEST_F(PathUtilityTest, NormalizeCasePath) { for (const auto& path_pair : non_normal_pairs) { auto& path_header = pathHeaderEntry(path_pair.first); - const auto result = PathUtil::canonicalPath(headers_); + const auto result = PathUtil::canonicalPath(path_header); EXPECT_TRUE(result) << "original path: " << path_pair.first; EXPECT_EQ(path_header.value().getStringView(), path_pair.second) << "original path: " << path_pair.second; @@ -89,7 +89,7 @@ TEST_F(PathUtilityTest, NormalizeCasePath) { TEST_F(PathUtilityTest, MergeSlashes) { auto mergeSlashes = [this](const std::string& path_value) { auto& path_header = pathHeaderEntry(path_value); - PathUtil::mergeSlashes(headers_); + PathUtil::mergeSlashes(path_header); auto sanitized_path_value = path_header.value().getStringView(); return std::string(sanitized_path_value); }; diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 55a20f4f74807..8676fc3dd6ae1 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4237,12 +4237,12 @@ TEST_F(RouterTest, AutoHostRewriteEnabled) { Http::TestHeaderMapImpl incoming_headers; HttpTestUtility::addDefaultHeaders(incoming_headers); - incoming_headers.setHost(req_host); + incoming_headers.Host()->value(req_host); cm_.conn_pool_.host_->hostname_ = "scooby.doo"; Http::TestHeaderMapImpl outgoing_headers; HttpTestUtility::addDefaultHeaders(outgoing_headers); - outgoing_headers.setHost(cm_.conn_pool_.host_->hostname_); + outgoing_headers.Host()->value(cm_.conn_pool_.host_->hostname_); EXPECT_CALL(callbacks_.route_->route_entry_, timeout()) .WillOnce(Return(std::chrono::milliseconds(0))); @@ -4275,7 +4275,7 @@ TEST_F(RouterTest, AutoHostRewriteDisabled) { Http::TestHeaderMapImpl incoming_headers; HttpTestUtility::addDefaultHeaders(incoming_headers); - incoming_headers.setHost(req_host); + incoming_headers.Host()->value(req_host); cm_.conn_pool_.host_->hostname_ = "scooby.doo"; diff --git a/test/integration/filters/call_decodedata_once_filter.cc b/test/integration/filters/call_decodedata_once_filter.cc index ff387d9741048..15abc0658fd05 100644 --- a/test/integration/filters/call_decodedata_once_filter.cc +++ b/test/integration/filters/call_decodedata_once_filter.cc @@ -16,10 +16,8 @@ class CallDecodeDataOnceFilter : public Http::PassThroughFilter { constexpr static char name[] = "call-decodedata-once-filter"; Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& header_map, bool) override { - const Http::HeaderEntry* entry_content = - header_map.get(Envoy::Http::LowerCaseString("content_size")); - const Http::HeaderEntry* entry_added = - header_map.get(Envoy::Http::LowerCaseString("added_size")); + Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); + Http::HeaderEntry* entry_added = header_map.get(Envoy::Http::LowerCaseString("added_size")); ASSERT(entry_content != nullptr && entry_added != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); added_size_ = std::stoul(std::string(entry_added->value().getStringView())); diff --git a/test/integration/filters/decode_headers_return_stop_all_filter.cc b/test/integration/filters/decode_headers_return_stop_all_filter.cc index 151a26e0fe036..e2049155ef98a 100644 --- a/test/integration/filters/decode_headers_return_stop_all_filter.cc +++ b/test/integration/filters/decode_headers_return_stop_all_filter.cc @@ -27,14 +27,12 @@ class DecodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { // Http::FilterHeadersStatus::StopAllIterationAndWatermark for headers. Triggers a timer to // continue iteration after 5s. Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& header_map, bool) override { - const Http::HeaderEntry* entry_content = - header_map.get(Envoy::Http::LowerCaseString("content_size")); - const Http::HeaderEntry* entry_added = - header_map.get(Envoy::Http::LowerCaseString("added_size")); + Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); + Http::HeaderEntry* entry_added = header_map.get(Envoy::Http::LowerCaseString("added_size")); ASSERT(entry_content != nullptr && entry_added != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); added_size_ = std::stoul(std::string(entry_added->value().getStringView())); - const Http::HeaderEntry* entry_is_first_trigger = + Http::HeaderEntry* entry_is_first_trigger = header_map.get(Envoy::Http::LowerCaseString("is_first_trigger")); is_first_trigger_ = entry_is_first_trigger != nullptr; // Remove "first_trigger" headers so that if the filter is registered twice in a filter chain, @@ -43,8 +41,7 @@ class DecodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { createTimerForContinue(); - const Http::HeaderEntry* entry_buffer = - header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); + Http::HeaderEntry* entry_buffer = header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); if (entry_buffer == nullptr || !is_first_trigger_) { return Http::FilterHeadersStatus::StopAllIterationAndBuffer; } else { diff --git a/test/integration/filters/encode_headers_return_stop_all_filter.cc b/test/integration/filters/encode_headers_return_stop_all_filter.cc index 768fc1ad343ad..778c4897a15a9 100644 --- a/test/integration/filters/encode_headers_return_stop_all_filter.cc +++ b/test/integration/filters/encode_headers_return_stop_all_filter.cc @@ -27,10 +27,8 @@ class EncodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { // Http::FilterHeadersStatus::StopAllIterationAndWatermark for headers. Triggers a timer to // continue iteration after 5s. Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& header_map, bool) override { - const Http::HeaderEntry* entry_content = - header_map.get(Envoy::Http::LowerCaseString("content_size")); - const Http::HeaderEntry* entry_added = - header_map.get(Envoy::Http::LowerCaseString("added_size")); + Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); + Http::HeaderEntry* entry_added = header_map.get(Envoy::Http::LowerCaseString("added_size")); ASSERT(entry_content != nullptr && entry_added != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); added_size_ = std::stoul(std::string(entry_added->value().getStringView())); @@ -41,8 +39,7 @@ class EncodeHeadersReturnStopAllFilter : public Http::PassThroughFilter { Http::MetadataMapPtr metadata_map_ptr = std::make_unique(metadata_map); encoder_callbacks_->addEncodedMetadata(std::move(metadata_map_ptr)); - const Http::HeaderEntry* entry_buffer = - header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); + Http::HeaderEntry* entry_buffer = header_map.get(Envoy::Http::LowerCaseString("buffer_limit")); if (entry_buffer == nullptr) { return Http::FilterHeadersStatus::StopAllIterationAndBuffer; } else { diff --git a/test/integration/filters/metadata_stop_all_filter.cc b/test/integration/filters/metadata_stop_all_filter.cc index 423df634ab5de..3ff6b7983d010 100644 --- a/test/integration/filters/metadata_stop_all_filter.cc +++ b/test/integration/filters/metadata_stop_all_filter.cc @@ -22,8 +22,7 @@ class MetadataStopAllFilter : public Http::PassThroughFilter { constexpr static char name[] = "metadata-stop-all-filter"; Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& header_map, bool) override { - const Http::HeaderEntry* entry_content = - header_map.get(Envoy::Http::LowerCaseString("content_size")); + Http::HeaderEntry* entry_content = header_map.get(Envoy::Http::LowerCaseString("content_size")); ASSERT(entry_content != nullptr); content_size_ = std::stoul(std::string(entry_content->value().getStringView())); diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index 618e874071e31..90ead02098e5b 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -100,7 +100,7 @@ void WebsocketIntegrationTest::commonValidate(Http::HeaderMap& proxied_headers, original_headers.Connection()->value() == "keep-alive, upgrade") { // The keep-alive is implicit for HTTP/1.1, so Envoy only sets the upgrade // header when converting from HTTP/1.1 to H2 - proxied_headers.setConnection("keep-alive, upgrade"); + proxied_headers.Connection()->value().setCopy("keep-alive, upgrade", 19); } } diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 9fab43a8ff354..c65862f5f58f2 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -839,7 +839,7 @@ TEST_P(AdminInstanceTest, EscapeHelpTextWithPunctuation) { Http::HeaderMapImpl header_map; Buffer::OwnedImpl response; EXPECT_EQ(Http::Code::OK, getCallback("/", header_map, response)); - const Http::HeaderString& content_type = header_map.ContentType()->value(); + Http::HeaderString& content_type = header_map.ContentType()->value(); EXPECT_THAT(std::string(content_type.getStringView()), testing::HasSubstr("text/html")); EXPECT_EQ(-1, response.search(planets.data(), planets.size(), 0)); const std::string escaped_planets = "jupiter>saturn>mars"; From 21c7df8a27d3d3082cf6768545c06caf48c0ef78 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 7 Dec 2019 09:28:23 -0500 Subject: [PATCH 06/14] format Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 7bce89dc7f7ca..f3d288d98e1fa 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -1,11 +1,12 @@ #pragma once -#define HEADER_MAP_USE_MULTI_MAP true -#define HEADER_MAP_USE_FLAT_HASH_MAP false +#define HEADER_MAP_USE_MULTI_MAP false +#define HEADER_MAP_USE_FLAT_HASH_MAP true #include #include #include + #if HEADER_MAP_USE_MULTI_MAP #include #endif @@ -114,7 +115,6 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { #if HEADER_MAP_USE_FLAT_HASH_MAP using HeaderNodeVector = std::vector; using HeaderLazyMap = absl::flat_hash_map; - //using HeaderLazyMap = std::map; #endif #if HEADER_MAP_USE_MULTI_MAP using HeaderLazyMap = std::multimap; @@ -243,6 +243,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { void subtractSize(uint64_t size); void appendToHeader(HeaderString& header, absl::string_view data, absl::string_view delimiter = ","); + // Performs a manual byte size count. uint64_t byteSizeInternal() const; private: @@ -270,15 +271,12 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { AllInlineHeaders inline_headers_; HeaderList headers_; - // Performs a manual byte size count. - //uint64_t byteSizeInternal() const { return headers_.byteSizeInternal(); } - // In TestHeaderMapImpl and VerifiedHeaderMapImpl, this method is overridden to performs a // time-consuming manual byte size count on each operation to verify the byte size. For prod // HeaderMaps, this verification is skipped. // TODO(asraa): Move this verification out of prod code and wrap virtual Http::HeaderMap methods // in Http::TestHeaderMapImpl with the verification. - virtual void verifyByteSize() { } + virtual void verifyByteSize() {} ALL_INLINE_HEADERS(DEFINE_INLINE_HEADER_FUNCS) }; From 25d96efd467eee93b375cf2f4f6987a596babd9f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 7 Dec 2019 13:15:04 -0500 Subject: [PATCH 07/14] Only create a map if there's more than a certain number of headers. This adds complexity but makes common cases faster. Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.cc | 88 +++++++++++++++++++-------- source/common/http/header_map_impl.h | 9 ++- 2 files changed, 69 insertions(+), 28 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 147320c03d2f6..9146665a54584 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -20,6 +20,7 @@ namespace { constexpr size_t MinDynamicCapacity{32}; // This includes the NULL (StringUtil::itoa technically only needs 21). constexpr size_t MaxIntegerLength{32}; +constexpr size_t MapSizeThreshold{8}; uint64_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) { return (static_cast(existing_capacity) + size_to_append) * 2; @@ -532,17 +533,25 @@ uint64_t HeaderMapImpl::HeaderList::byteSizeInternal() const { } const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { - HeaderLazyMap::iterator iter = headers_.find(key.get()); - if (iter != headers_.findEnd()) { + if (headers_.maybeMakeMap()) { + HeaderLazyMap::iterator iter = headers_.find(key.get()); + if (iter != headers_.findEnd()) { #if HEADER_MAP_USE_FLAT_HASH_MAP - const HeaderNodeVector& v = iter->second; - ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value. - HeaderEntryImpl& header_entry = *v[0]; + const HeaderNodeVector& v = iter->second; + ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value. + HeaderEntryImpl& header_entry = *v[0]; #endif #if HEADER_MAP_USE_MULTI_MAP - HeaderEntryImpl& header_entry = *(iter->second); + HeaderEntryImpl& header_entry = *(iter->second); #endif - return &header_entry; + return &header_entry; + } + } else { + for (const HeaderEntryImpl& header : headers_) { + if (header.key() == key.get().c_str()) { + return &header; + } + } } return nullptr; } @@ -595,6 +604,25 @@ void HeaderMapImpl::clear() { headers_.clear(); } +bool HeaderMapImpl::HeaderList::maybeMakeMap() const { + if (lazy_map_.empty()) { + if (headers_.size() < MapSizeThreshold) { + return false; + } + for (auto node = headers_.begin(); node != headers_.end(); ++node) { + absl::string_view key = node->key().getStringView(); +#if HEADER_MAP_USE_FLAT_HASH_MAP + HeaderNodeVector& v = lazy_map_[key]; + v.push_back(node); +#endif +#if HEADER_MAP_USE_MULTI_MAP + lazy_map_.insert(std::make_pair(key, node)); +#endif + } + } + return true; +} + HeaderMapImpl::HeaderLazyMap::iterator HeaderMapImpl::HeaderList::find(absl::string_view key) const { if (lazy_map_.empty()) { @@ -623,30 +651,40 @@ void HeaderMapImpl::remove(const LowerCaseString& key) { } void HeaderMapImpl::HeaderList::remove(absl::string_view key) { - auto iter = find(key); - if (iter != lazy_map_.end()) { + if (maybeMakeMap()) { + auto iter = find(key); + if (iter != lazy_map_.end()) { #if HEADER_MAP_USE_FLAT_HASH_MAP - HeaderNodeVector header_nodes = std::move(iter->second); - lazy_map_.erase(iter); - for (const HeaderNode& node : header_nodes) { - ASSERT(node->key() == key); - erase(node, false /* clear_from_map */); - } + HeaderNodeVector header_nodes = std::move(iter->second); + lazy_map_.erase(iter); + for (const HeaderNode& node : header_nodes) { + ASSERT(node->key() == key); + erase(node, false /* clear_from_map */); + } #endif #if HEADER_MAP_USE_MULTI_MAP - ASSERT(iter->second->key() == key); - do { - auto iter_to_erase = iter; - ++iter; - HeaderNode& node = iter_to_erase->second; - erase(node, false /* clear_from_map */); - lazy_map_.erase(iter_to_erase); - } while (iter != lazy_map_.end() && iter->second->key() == key); + ASSERT(iter->second->key() == key); + do { + auto iter_to_erase = iter; + ++iter; + HeaderNode& node = iter_to_erase->second; + erase(node, false /* clear_from_map */); + lazy_map_.erase(iter_to_erase); + } while (iter != lazy_map_.end() && iter->second->key() == key); #endif + } + } else { + for (HeaderNode i = headers_.begin(); i != headers_.end();) { + if (i->key() == key) { + i = erase(i, false); + } else { + ++i; + } + } } } -void HeaderMapImpl::HeaderList::erase(HeaderNode i, bool clear_from_map) { +HeaderMapImpl::HeaderNode HeaderMapImpl::HeaderList::erase(HeaderNode i, bool clear_from_map) { if (pseudo_headers_end_ == i) { pseudo_headers_end_++; } @@ -654,7 +692,7 @@ void HeaderMapImpl::HeaderList::erase(HeaderNode i, bool clear_from_map) { if (clear_from_map) { lazy_map_.erase(i->key().getStringView()); } - headers_.erase(i); + return headers_.erase(i); } void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index f3d288d98e1fa..f22a17708d41b 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -1,7 +1,7 @@ #pragma once -#define HEADER_MAP_USE_MULTI_MAP false -#define HEADER_MAP_USE_FLAT_HASH_MAP true +#define HEADER_MAP_USE_MULTI_MAP true +#define HEADER_MAP_USE_FLAT_HASH_MAP false #include #include @@ -198,7 +198,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { return i; } - void erase(HeaderNode i, bool clear_from_map); + HeaderNode erase(HeaderNode i, bool clear_from_map); template void remove_if(UnaryPredicate p) { headers_.remove_if([&](const HeaderEntryImpl& entry) { @@ -217,6 +217,9 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { lazy_map_.clear(); } + // Makes a map. + bool maybeMakeMap() const; + HeaderLazyMap::iterator find(absl::string_view key) const; HeaderLazyMap::iterator findEnd() const { return lazy_map_.end(); } From e4d228d3d9655ec6a9f90cdf616797f7584ffa0a Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 8 Dec 2019 16:43:27 -0500 Subject: [PATCH 08/14] add absl::btree variant; not working yet Signed-off-by: Joshua Marantz --- bazel/envoy_library.bzl | 1 + bazel/repositories.bzl | 4 ++++ bazel/repository_locations.bzl | 8 ++++---- source/common/http/header_map_impl.h | 11 ++++++++++- 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/bazel/envoy_library.bzl b/bazel/envoy_library.bzl index 9855f9a7bf565..ccfff3c3854e3 100644 --- a/bazel/envoy_library.bzl +++ b/bazel/envoy_library.bzl @@ -105,6 +105,7 @@ def envoy_cc_library( deps = deps + [envoy_external_dep_path(dep) for dep in external_deps] + [ repository + "//include/envoy/common:base_includes", repository + "//source/common/common:fmt_lib", + envoy_external_dep_path("abseil_btree_map"), envoy_external_dep_path("abseil_flat_hash_map"), envoy_external_dep_path("abseil_flat_hash_set"), envoy_external_dep_path("abseil_strings"), diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 4fbe72ecf497c..19c924e7e1666 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -457,6 +457,10 @@ def _com_google_absl(): name = "absl-base", actual = "@com_google_absl//absl/base", ) + native.bind( + name = "abseil_btree_map", + actual = "@com_google_absl//absl/container:btree_map", + ) native.bind( name = "abseil_flat_hash_map", actual = "@com_google_absl//absl/container:flat_hash_map", diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index ad3cecdecee13..a188668a4cd41 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -27,7 +27,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/envoyproxy/envoy-build-tools/archive/07314d549e27e9a4033af6236888d2a9ee0ad443.tar.gz"], ), boringssl = dict( - sha256 = "3eea198c8e3f587ffc8ea6acf87d7575f571bbe6dd88ec90405e236303f3dc01", + sha256 = "2529b7071a71e447dcc8eb4ac01838b12acad75ecd0dafa73ddac38168e5d857", strip_prefix = "boringssl-65e0aad1b721a5aa67f2a8041cf48f691139bb9f", # To update BoringSSL, which tracks Chromium releases: # 1. Open https://omahaproxy.appspot.com/ and note of linux/beta release. @@ -44,9 +44,9 @@ REPOSITORY_LOCATIONS = dict( ), com_google_absl = dict( sha256 = "3df5970908ed9a09ba51388d04661803a6af18c373866f442cede7f381e0b94a", - strip_prefix = "abseil-cpp-14550beb3b7b97195e483fb74b5efb906395c31e", - # 2019-07-31 - urls = ["https://github.com/abseil/abseil-cpp/archive/14550beb3b7b97195e483fb74b5efb906395c31e.tar.gz"], + strip_prefix = "abseil-cpp-77f87009a34c745255bd84d8f2647040d831a2b3", + # 2019-12-06 + urls = ["https://github.com/abseil/abseil-cpp/archive/77f87009a34c745255bd84d8f2647040d831a2b3.tar.gz"], ), com_github_apache_thrift = dict( sha256 = "7d59ac4fdcb2c58037ebd4a9da5f9a49e3e034bf75b3f26d9fe48ba3d8806e6b", diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index f22a17708d41b..af348391e49aa 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -2,13 +2,18 @@ #define HEADER_MAP_USE_MULTI_MAP true #define HEADER_MAP_USE_FLAT_HASH_MAP false +#define HEADER_MAP_USE_BTREE true #include #include #include #if HEADER_MAP_USE_MULTI_MAP -#include +# if HEADER_MAP_USE_BTREE +# include "absl/container/btree_map.h" +# else +# include +# endif #endif #include #include @@ -117,7 +122,11 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { using HeaderLazyMap = absl::flat_hash_map; #endif #if HEADER_MAP_USE_MULTI_MAP +# if HEADER_MAP_USE_BTREE using HeaderLazyMap = std::multimap; +# else + using HeaderLazyMap = absl::btree_multimap; +# endif #endif // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. From b88bdce89021cd9d79b2e88ba9d1aa17f7763a1f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Dec 2019 08:25:40 -0500 Subject: [PATCH 09/14] experiment with other absl containers for headers. Signed-off-by: Joshua Marantz --- bazel/envoy_library.bzl | 3 ++- bazel/repositories.bzl | 4 ++-- bazel/repository_locations.bzl | 4 ++-- source/common/http/header_map_impl.cc | 18 +++++++++++++++++- source/common/http/header_map_impl.h | 24 +++++++++++++++++++----- 5 files changed, 42 insertions(+), 11 deletions(-) diff --git a/bazel/envoy_library.bzl b/bazel/envoy_library.bzl index ccfff3c3854e3..59b66091363ae 100644 --- a/bazel/envoy_library.bzl +++ b/bazel/envoy_library.bzl @@ -105,9 +105,10 @@ def envoy_cc_library( deps = deps + [envoy_external_dep_path(dep) for dep in external_deps] + [ repository + "//include/envoy/common:base_includes", repository + "//source/common/common:fmt_lib", - envoy_external_dep_path("abseil_btree_map"), + envoy_external_dep_path("abseil_btree"), envoy_external_dep_path("abseil_flat_hash_map"), envoy_external_dep_path("abseil_flat_hash_set"), + envoy_external_dep_path("abseil_inlined_vector"), envoy_external_dep_path("abseil_strings"), envoy_external_dep_path("spdlog"), envoy_external_dep_path("fmtlib"), diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 058cf8fcee74e..f64c874518ea9 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -439,8 +439,8 @@ def _com_google_absl(): actual = "@com_google_absl//absl/base", ) native.bind( - name = "abseil_btree_map", - actual = "@com_google_absl//absl/container:btree_map", + name = "abseil_btree", + actual = "@com_google_absl//absl/container:btree", ) native.bind( name = "abseil_flat_hash_map", diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index a188668a4cd41..7ad29e0d4f430 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -27,7 +27,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/envoyproxy/envoy-build-tools/archive/07314d549e27e9a4033af6236888d2a9ee0ad443.tar.gz"], ), boringssl = dict( - sha256 = "2529b7071a71e447dcc8eb4ac01838b12acad75ecd0dafa73ddac38168e5d857", + sha256 = "3eea198c8e3f587ffc8ea6acf87d7575f571bbe6dd88ec90405e236303f3dc01", strip_prefix = "boringssl-65e0aad1b721a5aa67f2a8041cf48f691139bb9f", # To update BoringSSL, which tracks Chromium releases: # 1. Open https://omahaproxy.appspot.com/ and note of linux/beta release. @@ -43,7 +43,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://commondatastorage.googleapis.com/chromium-boringssl-docs/fips/boringssl-66005f41fbc3529ffe8d007708756720529da20d.tar.xz"], ), com_google_absl = dict( - sha256 = "3df5970908ed9a09ba51388d04661803a6af18c373866f442cede7f381e0b94a", + sha256 = "2529b7071a71e447dcc8eb4ac01838b12acad75ecd0dafa73ddac38168e5d857", strip_prefix = "abseil-cpp-77f87009a34c745255bd84d8f2647040d831a2b3", # 2019-12-06 urls = ["https://github.com/abseil/abseil-cpp/archive/77f87009a34c745255bd84d8f2647040d831a2b3.tar.gz"], diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 9146665a54584..e089127339700 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -20,7 +20,7 @@ namespace { constexpr size_t MinDynamicCapacity{32}; // This includes the NULL (StringUtil::itoa technically only needs 21). constexpr size_t MaxIntegerLength{32}; -constexpr size_t MapSizeThreshold{8}; +constexpr size_t MapSizeThreshold{0}; uint64_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) { return (static_cast(existing_capacity) + size_to_append) * 2; @@ -537,9 +537,14 @@ const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { HeaderLazyMap::iterator iter = headers_.find(key.get()); if (iter != headers_.findEnd()) { #if HEADER_MAP_USE_FLAT_HASH_MAP +# if HEADER_MAP_USE_SLIST + const HeaderMapCell& cell = iter->second; + HeaderEntryImpl& header_entry = *cell.node; +# else const HeaderNodeVector& v = iter->second; ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value. HeaderEntryImpl& header_entry = *v[0]; + endif #endif #if HEADER_MAP_USE_MULTI_MAP HeaderEntryImpl& header_entry = *(iter->second); @@ -655,12 +660,23 @@ void HeaderMapImpl::HeaderList::remove(absl::string_view key) { auto iter = find(key); if (iter != lazy_map_.end()) { #if HEADER_MAP_USE_FLAT_HASH_MAP +# if HEADER_MAP_USE_SLIST + HeaderCell* cell = &iter->second; + do { + const HeaderNode& node = cell->node; + ASSERT(node->key() == key); + erase(node, false /* clear_from_map */); + cell = cell->next; + } while (cell != nullptr); + lazy_map_.erase(iter); +# else HeaderNodeVector header_nodes = std::move(iter->second); lazy_map_.erase(iter); for (const HeaderNode& node : header_nodes) { ASSERT(node->key() == key); erase(node, false /* clear_from_map */); } +# endif #endif #if HEADER_MAP_USE_MULTI_MAP ASSERT(iter->second->key() == key); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index af348391e49aa..090986218e039 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -1,8 +1,9 @@ #pragma once -#define HEADER_MAP_USE_MULTI_MAP true -#define HEADER_MAP_USE_FLAT_HASH_MAP false -#define HEADER_MAP_USE_BTREE true +#define HEADER_MAP_USE_MULTI_MAP false +#define HEADER_MAP_USE_FLAT_HASH_MAP true +#define HEADER_MAP_USE_BTREE false +#define HEADER_MAP_USE_SLIST true #include #include @@ -24,7 +25,10 @@ #include "common/http/headers.h" #if HEADER_MAP_USE_FLAT_HASH_MAP -#include "absl/container/flat_hash_map.h" +# include "absl/container/flat_hash_map.h" +# if !HEADER_MAP_USE_SLIST +# include "absl/container/inlined_vector.h" +# endif #endif namespace Envoy { @@ -118,8 +122,13 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { struct HeaderEntryImpl; using HeaderNode = std::list::iterator; #if HEADER_MAP_USE_FLAT_HASH_MAP - using HeaderNodeVector = std::vector; +# if HEADER_MAP_USE_SLIST + struct HeaderCell { HeaderNode node; HeaderCell* next; }; + using HeaderLazyMap = absl::flat_hash_map; +# else + using HeaderNodeVector = absl::InlinedVector; using HeaderLazyMap = absl::flat_hash_map; +# endif #endif #if HEADER_MAP_USE_MULTI_MAP # if HEADER_MAP_USE_BTREE @@ -195,7 +204,12 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { addSize(i->key().size() + i->value().size()); if (!lazy_map_.empty()) { #if HEADER_MAP_USE_FLAT_HASH_MAP +# if HEADER_MAP_USE_SLIST lazy_map_[i->key().getStringView()].push_back(i); +# else + HeaderCell& cell = lazy_map_[i->key().getStringView()]; + cell.node = i; +# endif #endif #if HEADER_MAP_USE_MULTI_MAP lazy_map_.insert(std::make_pair(i->key().getStringView(), i)); From c01320c409316a0e5006ca59677399bbee1c02ce Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 14 Jan 2020 23:36:09 -0500 Subject: [PATCH 10/14] get the SLIST variant working, sort of (modulo setCopy behavior). Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.cc | 46 +++++++++--------- source/common/http/header_map_impl.h | 59 +++++++++++++----------- test/common/http/header_map_impl_test.cc | 7 +++ 3 files changed, 61 insertions(+), 51 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 72ccabed4ff54..31924de341299 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -537,15 +537,15 @@ const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { HeaderLazyMap::iterator iter = headers_.lazy_map_.find(key.get()); if (iter != headers_.lazy_map_.end()) { #if HEADER_MAP_USE_FLAT_HASH_MAP -# if HEADER_MAP_USE_SLIST - const HeaderCell* cell = iter->second; +#if HEADER_MAP_USE_SLIST + const HeaderCellPtr& cell = iter->second; ASSERT(cell != nullptr); // It's impossible to have a map entry with a null cell. - HeaderEntryImpl& header_entry = *cell->node; -# else + HeaderEntryImpl& header_entry = *cell->node_; +#else const HeaderNodeVector& v = iter->second; ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value. HeaderEntryImpl& header_entry = *v[0]; -# endif +#endif #endif #if HEADER_MAP_USE_MULTI_MAP HeaderEntryImpl& header_entry = *(iter->second); @@ -618,16 +618,16 @@ bool HeaderMapImpl::HeaderList::maybeMakeMap() const { for (auto node = headers_.begin(); node != headers_.end(); ++node) { absl::string_view key = node->key().getStringView(); #if HEADER_MAP_USE_FLAT_HASH_MAP -# if HEADER_MAP_USE_SLIST - HeaderCell* cell = new HeaderCell; - cell->node = node; - HeaderCell*& cellref = lazy_map_[key]; - cell->next = cellref; - cellref = cell; -# else +#if HEADER_MAP_USE_SLIST + HeaderCellPtr cell = std::make_unique(); + cell->node_ = node; + HeaderCellPtr& cellref = lazy_map_[key]; + cell->next_ = std::move(cellref); + cellref = std::move(cell); +#else HeaderNodeVector& v = lazy_map_[key]; v.push_back(node); -# endif +#endif #endif #if HEADER_MAP_USE_MULTI_MAP lazy_map_.insert(std::make_pair(key, node)); @@ -648,13 +648,13 @@ HeaderMapImpl::HeaderList::find(absl::string_view key) const { if (lazy_map_.empty()) { for (auto node = headers_.begin(); node != headers_.end(); ++node) { #if HEADER_MAP_USE_FLAT_HASH_MAP -# if HEADER_MAP_USE_SLIST +#if HEADER_MAP_USE_SLIST // ???? -# else +#else absl::string_view key = node->key().getStringView(); HeaderNodeVector& v = lazy_map_[key]; v.push_back(node); -# endif +#endif #endif #if HEADER_MAP_USE_MULTI_MAP lazy_map_.insert(std::make_pair(key, node)); @@ -680,25 +680,23 @@ void HeaderMapImpl::HeaderList::remove(absl::string_view key) { auto iter = lazy_map_.find(key); if (iter != lazy_map_.end()) { #if HEADER_MAP_USE_FLAT_HASH_MAP -# if HEADER_MAP_USE_SLIST - HeaderCell* cell = iter->second; +#if HEADER_MAP_USE_SLIST + HeaderCellPtr cell = std::move(iter->second); do { - const HeaderNode& node = cell->node; + const HeaderNode& node = cell->node_; ASSERT(node->key() == key); erase(node, false /* clear_from_map */); - HeaderCell* next = cell->next; - delete cell; - cell = next; + cell = std::move(cell->next_); } while (cell != nullptr); lazy_map_.erase(iter); -# else +#else HeaderNodeVector header_nodes = std::move(iter->second); lazy_map_.erase(iter); for (const HeaderNode& node : header_nodes) { ASSERT(node->key() == key); erase(node, false /* clear_from_map */); } -# endif +#endif #endif #if HEADER_MAP_USE_MULTI_MAP ASSERT(iter->second->key() == key); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index ef5055ec0f116..203cd420826c1 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -10,11 +10,11 @@ #include #if HEADER_MAP_USE_MULTI_MAP -# if HEADER_MAP_USE_BTREE -# include "absl/container/btree_map.h" -# else -# include -# endif +#if HEADER_MAP_USE_BTREE +#include "absl/container/btree_map.h" +#else +#include +#endif #endif #include #include @@ -25,10 +25,10 @@ #include "common/http/headers.h" #if HEADER_MAP_USE_FLAT_HASH_MAP -# include "absl/container/flat_hash_map.h" -# if !HEADER_MAP_USE_SLIST -# include "absl/container/inlined_vector.h" -# endif +#include "absl/container/flat_hash_map.h" +#if !HEADER_MAP_USE_SLIST +#include "absl/container/inlined_vector.h" +#endif #endif namespace Envoy { @@ -122,20 +122,25 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { struct HeaderEntryImpl; using HeaderNode = std::list::iterator; #if HEADER_MAP_USE_FLAT_HASH_MAP -# if HEADER_MAP_USE_SLIST - struct HeaderCell { HeaderNode node; HeaderCell* next; }; - using HeaderLazyMap = absl::flat_hash_map; -# else +#if HEADER_MAP_USE_SLIST + struct HeaderCell; + using HeaderCellPtr = std::unique_ptr; + struct HeaderCell { + HeaderNode node_; + HeaderCellPtr next_; + }; + using HeaderLazyMap = absl::flat_hash_map; +#else using HeaderNodeVector = absl::InlinedVector; using HeaderLazyMap = absl::flat_hash_map; -# endif +#endif #endif #if HEADER_MAP_USE_MULTI_MAP -# if HEADER_MAP_USE_BTREE +#if HEADER_MAP_USE_BTREE using HeaderLazyMap = std::multimap; -# else +#else using HeaderLazyMap = absl::btree_multimap; -# endif +#endif #endif // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. @@ -206,15 +211,15 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { addSize(i->key().size() + i->value().size()); if (!lazy_map_.empty()) { #if HEADER_MAP_USE_FLAT_HASH_MAP -# if HEADER_MAP_USE_SLIST - HeaderCell*& cellref = lazy_map_[i->key().getStringView()]; - HeaderCell* cell = new HeaderCell; - cell->node = i; - cell->next = cellref; - cellref = cell; -# else +#if HEADER_MAP_USE_SLIST + HeaderCellPtr& cellref = lazy_map_[i->key().getStringView()]; + HeaderCellPtr cell = std::make_unique(); + cell->node_ = i; + cell->next_ = std::move(cellref); + cellref = std::move(cell); +#else lazy_map_[i->key().getStringView()].push_back(i); -# endif +#endif #endif #if HEADER_MAP_USE_MULTI_MAP lazy_map_.insert(std::make_pair(i->key().getStringView(), i)); @@ -248,8 +253,8 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { // Makes a map. bool maybeMakeMap() const; - //HeaderLazyMap::iterator find(absl::string_view key) const; - //HeaderLazyMap::iterator findEnd() const { return lazy_map_.end(); } + // HeaderLazyMap::iterator find(absl::string_view key) const; + // HeaderLazyMap::iterator findEnd() const { return lazy_map_.end(); } std::list::const_iterator begin() const { return headers_.begin(); } std::list::const_iterator end() const { return headers_.end(); } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index cdc5b887ef60b..653eb81f4c6a5 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -682,8 +682,15 @@ TEST(HeaderMapImplTest, SetCopy) { MockCb cb; InSequence seq; +#if HEADER_MAP_USE_SLIST + // With a singly linked list, we actually overwrite the last header, rather + // than the first. Does this matter? + EXPECT_CALL(cb, Call("hello", "monde")); + EXPECT_CALL(cb, Call("hello", "override-monde")); +#else EXPECT_CALL(cb, Call("hello", "override-monde")); EXPECT_CALL(cb, Call("hello", "monde2")); +#endif headers.iterate( [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { static_cast(cb_v)->Call(std::string(header.key().getStringView()), From 764ce9b79816310a13cb8bdf5ecf8738b27a4016 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 15 Jan 2020 13:12:34 -0500 Subject: [PATCH 11/14] switch away from SLIST as there are some semantic diffs currently that break tests. Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.h | 2 +- source/extensions/quic_listeners/quiche/platform/BUILD | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 203cd420826c1..73cca56d4cfe6 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -3,7 +3,7 @@ #define HEADER_MAP_USE_MULTI_MAP false #define HEADER_MAP_USE_FLAT_HASH_MAP true #define HEADER_MAP_USE_BTREE false -#define HEADER_MAP_USE_SLIST true +#define HEADER_MAP_USE_SLIST false #include #include diff --git a/source/extensions/quic_listeners/quiche/platform/BUILD b/source/extensions/quic_listeners/quiche/platform/BUILD index 8b03fa291ea79..3cb703c744a86 100644 --- a/source/extensions/quic_listeners/quiche/platform/BUILD +++ b/source/extensions/quic_listeners/quiche/platform/BUILD @@ -139,7 +139,6 @@ envoy_cc_library( external_deps = [ "abseil_base", "abseil_hash", - "abseil_inlined_vector", "abseil_memory", "abseil_node_hash_map", "abseil_node_hash_set", @@ -296,7 +295,6 @@ envoy_cc_library( external_deps = [ "abseil_base", "abseil_hash", - "abseil_inlined_vector", "abseil_memory", "abseil_str_format", ], From 86cab1e3df51b61bba5cc1ac6041b097725a14a9 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 15 Jan 2020 14:21:46 -0500 Subject: [PATCH 12/14] Fix always-false warning. Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 31924de341299..d82d6ec432af9 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -612,7 +612,7 @@ void HeaderMapImpl::clear() { bool HeaderMapImpl::HeaderList::maybeMakeMap() const { if (lazy_map_.empty()) { - if (headers_.size() < MapSizeThreshold) { + if (MapSizeThreshold == 0 || headers_.size() < MapSizeThreshold) { return false; } for (auto node = headers_.begin(); node != headers_.end(); ++node) { From 5026a1943d39148b157be141dc3af420170be4e9 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 15 Jan 2020 20:58:18 -0500 Subject: [PATCH 13/14] Use a define for MAP_SIZE_THRESHOLD to avoid compile always-false warnings. Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index d82d6ec432af9..03834fe28ca91 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -20,7 +20,7 @@ namespace { constexpr size_t MinDynamicCapacity{32}; // This includes the NULL (StringUtil::itoa technically only needs 21). constexpr size_t MaxIntegerLength{32}; -constexpr size_t MapSizeThreshold{0}; +#define MAP_SIZE_THRESHOLD 0 uint64_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) { return (static_cast(existing_capacity) + size_to_append) * 2; @@ -612,9 +612,11 @@ void HeaderMapImpl::clear() { bool HeaderMapImpl::HeaderList::maybeMakeMap() const { if (lazy_map_.empty()) { - if (MapSizeThreshold == 0 || headers_.size() < MapSizeThreshold) { +#if MAP_SIZE_THRESHOLD != 0 + if (headers_.size() < MAP_SIZE_THRESHOLD) { return false; } +#endif for (auto node = headers_.begin(); node != headers_.end(); ++node) { absl::string_view key = node->key().getStringView(); #if HEADER_MAP_USE_FLAT_HASH_MAP From 7f3f3c5bb0de7dd4646bc31a3e207ef138e09cd5 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 16 Jan 2020 09:19:38 -0500 Subject: [PATCH 14/14] Make SLIST behave correctly. Signed-off-by: Joshua Marantz --- source/common/http/header_map_impl.cc | 91 ++++++------------------ source/common/http/header_map_impl.h | 42 ++++++----- test/common/http/header_map_impl_test.cc | 7 -- 3 files changed, 48 insertions(+), 92 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 03834fe28ca91..52b4aed1d9ff0 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -349,7 +349,7 @@ void HeaderMapImpl::copyFrom(const HeaderMap& header_map) { verifyByteSize(); } -bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const { +bool HeaderMapImpl::HeaderList::operator==(const HeaderList& rhs) const { if (size() != rhs.size()) { return false; } @@ -363,8 +363,6 @@ bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const { return true; } -bool HeaderMapImpl::operator!=(const HeaderMapImpl& rhs) const { return !operator==(rhs); } - void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) { EntryCb cb = ConstSingleton::get().find(key.getStringView()); if (cb) { @@ -455,30 +453,6 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value) verifyByteSize(); } -#if 0 -void HeaderMapImpl::append(const LowerCaseString& key, absl::string_view value) { - HeaderLazyMap::iterator iter = headers_.find(key.get()); - if (iter != headers_.findEnd()) { -#if HEADER_MAP_USE_FLAT_HASH_MAP - HeaderNodeVector& v = iter->second; - for (HeaderNode node : v) { - headers_.appendToHeader(node->value(), value); - } -#endif -#if HEADER_MAP_USE_MULTI_MAP - do { - HeaderNode node = iter->second; - headers_.appendToHeader(node->value(), value); - ++iter; - } while (iter != headers_.findEnd() && iter->second->key().getStringView() == key.get()); -#endif - } else { - addCopy(key, value); - } - verifyByteSize(); -} -#endif - void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view value) { // TODO(#9221): converge on and document a policy for coalescing multiple headers. auto* entry = getExisting(key); @@ -532,15 +506,27 @@ uint64_t HeaderMapImpl::HeaderList::byteSizeInternal() const { return byte_size; } +#if HEADER_MAP_USE_SLIST +void HeaderMapImpl::HeaderList::addSlistEntry(HeaderNode node) const { + HeaderSlistEntry& entry = lazy_map_[node->key().getStringView()]; + HeaderCellPtr cell = std::make_unique(); + cell->node_ = node; + if (entry.first_ == nullptr) { + entry.first_ = cell.get(); + } else { + cell->next_ = std::move(entry.list_); + } + entry.list_ = std::move(cell); +} +#endif + const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { if (headers_.maybeMakeMap()) { HeaderLazyMap::iterator iter = headers_.lazy_map_.find(key.get()); if (iter != headers_.lazy_map_.end()) { #if HEADER_MAP_USE_FLAT_HASH_MAP #if HEADER_MAP_USE_SLIST - const HeaderCellPtr& cell = iter->second; - ASSERT(cell != nullptr); // It's impossible to have a map entry with a null cell. - HeaderEntryImpl& header_entry = *cell->node_; + HeaderEntryImpl& header_entry = *(iter->second.first_->node_); #else const HeaderNodeVector& v = iter->second; ASSERT(!v.empty()); // It's impossible to have a map entry with an empty vector as its value. @@ -553,7 +539,7 @@ const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { return &header_entry; } } else { - for (const HeaderEntryImpl& header : headers_) { + for (const HeaderEntryImpl& header : headers_.headers_) { if (header.key() == key.get().c_str()) { return &header; } @@ -567,7 +553,7 @@ HeaderEntry* HeaderMapImpl::getExisting(const LowerCaseString& key) { } void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const { - for (const HeaderEntryImpl& header : headers_) { + for (const HeaderEntryImpl& header : headers_.headers_) { if (cb(header, context) == HeaderMap::Iterate::Break) { break; } @@ -575,7 +561,7 @@ void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const { } void HeaderMapImpl::iterateReverse(ConstIterateCb cb, void* context) const { - for (auto it = headers_.rbegin(); it != headers_.rend(); it++) { + for (auto it = headers_.headers_.rbegin(); it != headers_.headers_.rend(); it++) { if (cb(*it, context) == HeaderMap::Iterate::Break) { break; } @@ -618,16 +604,11 @@ bool HeaderMapImpl::HeaderList::maybeMakeMap() const { } #endif for (auto node = headers_.begin(); node != headers_.end(); ++node) { - absl::string_view key = node->key().getStringView(); #if HEADER_MAP_USE_FLAT_HASH_MAP #if HEADER_MAP_USE_SLIST - HeaderCellPtr cell = std::make_unique(); - cell->node_ = node; - HeaderCellPtr& cellref = lazy_map_[key]; - cell->next_ = std::move(cellref); - cellref = std::move(cell); + addSlistEntry(node); #else - HeaderNodeVector& v = lazy_map_[key]; + HeaderNodeVector& v = lazy_map_[node->key().getStringView()]; v.push_back(node); #endif #endif @@ -639,34 +620,6 @@ bool HeaderMapImpl::HeaderList::maybeMakeMap() const { return true; } -#if 0 -HeaderMapImpl::HeaderLazyMap::iterator -HeaderMapImpl::HeaderList::find(absl::string_view key) const { - if (maybeMakeMap()) { - return lazy_map_.find(key); - } - // If there were too few entries for a map, do a linear search. - - if (lazy_map_.empty()) { - for (auto node = headers_.begin(); node != headers_.end(); ++node) { -#if HEADER_MAP_USE_FLAT_HASH_MAP -#if HEADER_MAP_USE_SLIST - // ???? -#else - absl::string_view key = node->key().getStringView(); - HeaderNodeVector& v = lazy_map_[key]; - v.push_back(node); -#endif -#endif -#if HEADER_MAP_USE_MULTI_MAP - lazy_map_.insert(std::make_pair(key, node)); -#endif - } - } - return lazy_map_.find(key); -} -#endif - void HeaderMapImpl::remove(const LowerCaseString& key) { EntryCb cb = ConstSingleton::get().find(key.get()); if (cb) { @@ -683,7 +636,7 @@ void HeaderMapImpl::HeaderList::remove(absl::string_view key) { if (iter != lazy_map_.end()) { #if HEADER_MAP_USE_FLAT_HASH_MAP #if HEADER_MAP_USE_SLIST - HeaderCellPtr cell = std::move(iter->second); + HeaderCellPtr cell = std::move(iter->second.list_); do { const HeaderNode& node = cell->node_; ASSERT(node->key() == key); diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 73cca56d4cfe6..9d64b21b4048a 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -3,7 +3,7 @@ #define HEADER_MAP_USE_MULTI_MAP false #define HEADER_MAP_USE_FLAT_HASH_MAP true #define HEADER_MAP_USE_BTREE false -#define HEADER_MAP_USE_SLIST false +#define HEADER_MAP_USE_SLIST true #include #include @@ -93,8 +93,8 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { * For testing. Equality is based on equality of the backing list. This is an exact match * comparison (order matters). */ - bool operator==(const HeaderMapImpl& rhs) const; - bool operator!=(const HeaderMapImpl& rhs) const; + bool operator==(const HeaderMapImpl& rhs) const { return headers_ == rhs.headers_; } + bool operator!=(const HeaderMapImpl& rhs) const { return !(headers_ == rhs.headers_); } // Http::HeaderMap void addReference(const LowerCaseString& key, absl::string_view value) override; @@ -129,7 +129,23 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { HeaderNode node_; HeaderCellPtr next_; }; - using HeaderLazyMap = absl::flat_hash_map; + // The mechanics of the single-linked list implementation result in the + // elements reversing in order as they are inserted. HeaderMap::get() + // requires we retrieve the first item. To avoid having to walk down + // the entire list on get(), we store a pointer to the first element as + // our map value, giving us instant access. We need access to the remaining + // occurrences of a key only for HeaderMap::remove(), which removes all + // the instances. + // + // We could simplify this code if it was feasible to change the semantics + // of get(), e.g. maybe we want get() to return all the elements too, or + // for setCopy() to leave only one element behind, instead of replacing + // only the first occurrence (which seems non-obvious). + struct HeaderSlistEntry { + HeaderCell* first_{nullptr}; + HeaderCellPtr list_; + }; + using HeaderLazyMap = absl::flat_hash_map; #else using HeaderNodeVector = absl::InlinedVector; using HeaderLazyMap = absl::flat_hash_map; @@ -204,6 +220,10 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { return !key.getStringView().empty() && key.getStringView()[0] == ':'; } +#if HEADER_MAP_USE_SLIST + void addSlistEntry(HeaderNode node) const; +#endif + template HeaderNode insert(Key&& key, Value&&... value) { const bool is_pseudo_header = isPseudoHeader(key); HeaderNode i = headers_.emplace(is_pseudo_header ? pseudo_headers_end_ : headers_.end(), @@ -212,11 +232,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { if (!lazy_map_.empty()) { #if HEADER_MAP_USE_FLAT_HASH_MAP #if HEADER_MAP_USE_SLIST - HeaderCellPtr& cellref = lazy_map_[i->key().getStringView()]; - HeaderCellPtr cell = std::make_unique(); - cell->node_ = i; - cell->next_ = std::move(cellref); - cellref = std::move(cell); + addSlistEntry(i); #else lazy_map_[i->key().getStringView()].push_back(i); #endif @@ -253,13 +269,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { // Makes a map. bool maybeMakeMap() const; - // HeaderLazyMap::iterator find(absl::string_view key) const; - // HeaderLazyMap::iterator findEnd() const { return lazy_map_.end(); } - - std::list::const_iterator begin() const { return headers_.begin(); } - std::list::const_iterator end() const { return headers_.end(); } - std::list::const_reverse_iterator rbegin() const { return headers_.rbegin(); } - std::list::const_reverse_iterator rend() const { return headers_.rend(); } + bool operator==(const HeaderList& rhs) const; size_t size() const { return headers_.size(); } bool empty() const { return headers_.empty(); } void clear() { diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 653eb81f4c6a5..cdc5b887ef60b 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -682,15 +682,8 @@ TEST(HeaderMapImplTest, SetCopy) { MockCb cb; InSequence seq; -#if HEADER_MAP_USE_SLIST - // With a singly linked list, we actually overwrite the last header, rather - // than the first. Does this matter? - EXPECT_CALL(cb, Call("hello", "monde")); - EXPECT_CALL(cb, Call("hello", "override-monde")); -#else EXPECT_CALL(cb, Call("hello", "override-monde")); EXPECT_CALL(cb, Call("hello", "monde2")); -#endif headers.iterate( [](const Http::HeaderEntry& header, void* cb_v) -> HeaderMap::Iterate { static_cast(cb_v)->Call(std::string(header.key().getStringView()),