From bd0130e7867937dec472afff0ad4a7b40a6f04b7 Mon Sep 17 00:00:00 2001 From: birenroy Date: Mon, 11 Mar 2024 10:14:13 -0400 Subject: [PATCH] http2: deprecates use of CallbackVisitor in CodecImpl (#32378) This change removes an unnecessary layer of abstraction by implementing Http2VisitorInterface directly, rather than passing HTTP/2 codec events through the QUICHE CallbackVisitor and nghttp2-style callbacks. When the reloadable feature is removed, CodecImpl can be significantly simplified. $ bazel test //test/common/http/http2/... //test/integration/... INFO: Invocation ID: 76f7aa9f-e507-4798-892a-6e58df6deb7e INFO: Analyzed 397 targets (0 packages loaded, 0 targets configured). INFO: Found 295 targets and 102 test targets... INFO: Elapsed time: 745.010s, Critical Path: 744.00s INFO: 399 processes: 11 remote cache hit, 389 remote. INFO: Build completed successfully, 399 total actions Executed 98 out of 102 tests: 102 tests pass. Commit Message: http2: deprecates use of CallbackVisitor in CodecImpl Additional Description: Risk Level: medium, affects handling of HTTP/2 codec events Testing: ran unit and integration tests both with and without the feature enabled. Docs Changes: Release Notes: Platform Specific Features: Runtime guard: envoy.reloadable_features.http2_skip_callback_visitor Signed-off-by: Biren Roy --- changelogs/current.yaml | 4 + source/common/http/http2/codec_impl.cc | 284 ++++++++++++++++-- source/common/http/http2/codec_impl.h | 76 +++++ source/common/runtime/runtime_features.cc | 1 + test/integration/http_protocol_integration.cc | 19 +- test/integration/http_protocol_integration.h | 3 + 6 files changed, 351 insertions(+), 36 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 0fb271b76c28..e77a0bfda18e 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -64,6 +64,10 @@ minor_behavior_changes: change: | Add more conversion in the proxy status utility. It can be disabled by the runtime guard ``envoy.reloadable_features.proxy_status_mapping_more_core_response_flags``. +- area: http2 + change: | + Simplifies integration with the codec by removing translation between nghttp2 callbacks and Http2VisitorInterface events. + Guarded by ``envoy.reloadable_features.http2_skip_callback_visitor``. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index c0e4f569d1bb..b2ae2e444a9c 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -30,6 +30,7 @@ #include "absl/cleanup/cleanup.h" #include "absl/container/fixed_array.h" +#include "quiche/common/quiche_endian.h" #include "quiche/http2/adapter/callback_visitor.h" #include "quiche/http2/adapter/nghttp2_adapter.h" #include "quiche/http2/adapter/oghttp2_adapter.h" @@ -118,8 +119,13 @@ std::unique_ptr ProdNghttp2SessionFactory::create(const nghttp2_session_callbacks* callbacks, ConnectionImpl* connection, const http2::adapter::OgHttp2Adapter::Options& options) { - auto visitor = std::make_unique( - http2::adapter::Perspective::kClient, *callbacks, connection); + std::unique_ptr visitor; + if (connection->skipCallbackVisitor()) { + visitor = std::make_unique(connection); + } else { + visitor = std::make_unique( + http2::adapter::Perspective::kClient, *callbacks, connection); + } std::unique_ptr adapter = http2::adapter::OgHttp2Adapter::Create(*visitor, options); connection->setVisitor(std::move(visitor)); @@ -129,15 +135,26 @@ ProdNghttp2SessionFactory::create(const nghttp2_session_callbacks* callbacks, std::unique_ptr ProdNghttp2SessionFactory::create(const nghttp2_session_callbacks* callbacks, ConnectionImpl* connection, const nghttp2_option* options) { - auto visitor = std::make_unique( - http2::adapter::Perspective::kClient, *callbacks, connection); - auto adapter = http2::adapter::NgHttp2Adapter::CreateClientAdapter(*visitor, options); - auto stream_close_listener = [p = adapter.get()](http2::adapter::Http2StreamId stream_id) { - p->RemoveStream(stream_id); - }; - visitor->set_stream_close_listener(std::move(stream_close_listener)); - connection->setVisitor(std::move(visitor)); - return adapter; + if (connection->skipCallbackVisitor()) { + auto visitor = std::make_unique(connection); + auto adapter = http2::adapter::NgHttp2Adapter::CreateClientAdapter(*visitor, options); + auto stream_close_listener = [p = adapter.get()](http2::adapter::Http2StreamId stream_id) { + p->RemoveStream(stream_id); + }; + visitor->setStreamCloseListener(std::move(stream_close_listener)); + connection->setVisitor(std::move(visitor)); + return adapter; + } else { + auto visitor = std::make_unique( + http2::adapter::Perspective::kClient, *callbacks, connection); + auto adapter = http2::adapter::NgHttp2Adapter::CreateClientAdapter(*visitor, options); + auto stream_close_listener = [p = adapter.get()](http2::adapter::Http2StreamId stream_id) { + p->RemoveStream(stream_id); + }; + visitor->set_stream_close_listener(std::move(stream_close_listener)); + connection->setVisitor(std::move(visitor)); + return adapter; + } } void ProdNghttp2SessionFactory::init(ConnectionImpl* connection, @@ -1755,7 +1772,8 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { nghttp2_session_callbacks_set_on_begin_headers_callback( callbacks_, [](nghttp2_session*, const nghttp2_frame* frame, void* user_data) -> int { - auto status = static_cast(user_data)->onBeginHeaders(frame->hd.stream_id); + Status status = + static_cast(user_data)->onBeginHeaders(frame->hd.stream_id); return static_cast(user_data)->setAndCheckCodecCallbackStatus( std::move(status)); }); @@ -1782,7 +1800,7 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { nghttp2_session_callbacks_set_on_begin_frame_callback( callbacks_, [](nghttp2_session*, const nghttp2_frame_hd* hd, void* user_data) -> int { - auto status = static_cast(user_data)->onBeforeFrameReceived( + Status status = static_cast(user_data)->onBeforeFrameReceived( hd->stream_id, hd->length, hd->type, hd->flags); return static_cast(user_data)->setAndCheckCodecCallbackStatus( std::move(status)); @@ -1790,15 +1808,17 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { nghttp2_session_callbacks_set_on_frame_recv_callback( callbacks_, [](nghttp2_session*, const nghttp2_frame* frame, void* user_data) -> int { - auto status = static_cast(user_data)->onFrameReceived(frame); - return static_cast(user_data)->setAndCheckCodecCallbackStatus( - std::move(status)); + auto* conn = static_cast(user_data); + RELEASE_ASSERT(!conn->skipCallbackVisitor(), "Unexpected use of nghttp2 callback!"); + Status status = conn->onFrameReceived(frame); + return conn->setAndCheckCodecCallbackStatus(std::move(status)); }); nghttp2_session_callbacks_set_on_stream_close_callback( callbacks_, [](nghttp2_session*, int32_t stream_id, uint32_t error_code, void* user_data) -> int { - auto status = static_cast(user_data)->onStreamClose(stream_id, error_code); + Status status = + static_cast(user_data)->onStreamClose(stream_id, error_code); return static_cast(user_data)->setAndCheckCodecCallbackStatus( std::move(status)); }); @@ -1814,8 +1834,10 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { error_code = frame->rst_stream.error_code; break; } - return static_cast(user_data)->onFrameSend( - frame->hd.stream_id, frame->hd.length, frame->hd.type, frame->hd.flags, error_code); + auto* conn = static_cast(user_data); + RELEASE_ASSERT(!conn->skipCallbackVisitor(), "Unexpected use of nghttp2 callback!"); + return conn->onFrameSend(frame->hd.stream_id, frame->hd.length, frame->hd.type, + frame->hd.flags, error_code); }); nghttp2_session_callbacks_set_before_frame_send_callback( @@ -1861,6 +1883,193 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { ConnectionImpl::Http2Callbacks::~Http2Callbacks() { nghttp2_session_callbacks_del(callbacks_); } +ConnectionImpl::Http2Visitor::Http2Visitor(ConnectionImpl* connection) : connection_(connection) {} + +int64_t ConnectionImpl::Http2Visitor::OnReadyToSend(absl::string_view serialized) { + RELEASE_ASSERT(connection_->skipCallbackVisitor(), "Unexpected use of Http2Visitor!"); + return connection_->onSend(reinterpret_cast(serialized.data()), + serialized.size()); +} + +bool ConnectionImpl::Http2Visitor::OnFrameHeader(Http2StreamId stream_id, size_t length, + uint8_t type, uint8_t flags) { + RELEASE_ASSERT(connection_->skipCallbackVisitor(), "Unexpected use of Http2Visitor!"); + ENVOY_CONN_LOG(debug, "Http2Visitor::OnFrameHeader({}, {}, {}, {})", connection_->connection_, + stream_id, length, int(type), int(flags)); + + if (type == NGHTTP2_CONTINUATION) { + if (current_frame_.stream_id != stream_id) { + return false; + } + current_frame_.length += length; + current_frame_.flags |= flags; + } else { + current_frame_ = {stream_id, length, type, flags}; + padding_length_ = 0; + remaining_data_payload_ = 0; + } + Status status = connection_->onBeforeFrameReceived(stream_id, length, type, flags); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); +} + +bool ConnectionImpl::Http2Visitor::OnBeginHeadersForStream(Http2StreamId stream_id) { + Status status = connection_->onBeginHeaders(stream_id); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); +} + +http2::adapter::Http2VisitorInterface::OnHeaderResult +ConnectionImpl::Http2Visitor::OnHeaderForStream(Http2StreamId stream_id, + absl::string_view name_view, + absl::string_view value_view) { + // TODO PERF: Can reference count here to avoid copies. + HeaderString name; + name.setCopy(name_view.data(), name_view.size()); + HeaderString value; + value.setCopy(value_view.data(), value_view.size()); + const int result = connection_->onHeader(stream_id, std::move(name), std::move(value)); + switch (result) { + case 0: + return HEADER_OK; + case NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE: + return HEADER_RST_STREAM; + default: + return HEADER_CONNECTION_ERROR; + } +} + +bool ConnectionImpl::Http2Visitor::OnEndHeadersForStream(Http2StreamId stream_id) { + ENVOY_CONN_LOG(debug, "Http2Visitor::OnEndHeadersForStream({})", connection_->connection_, + stream_id); + Status status = connection_->onHeaders(stream_id, current_frame_.length, current_frame_.flags); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); +} + +bool ConnectionImpl::Http2Visitor::OnBeginDataForStream(Http2StreamId stream_id, + size_t payload_length) { + ENVOY_CONN_LOG(debug, "Http2Visitor::OnBeginDataForStream({}, {})", connection_->connection_, + stream_id, payload_length); + remaining_data_payload_ = payload_length; + padding_length_ = 0; + if (remaining_data_payload_ == 0 && (current_frame_.flags & NGHTTP2_FLAG_END_STREAM) == 0) { + ENVOY_CONN_LOG(debug, "Http2Visitor dispatching DATA for stream {}", connection_->connection_, + stream_id); + Status status = connection_->onBeginData(stream_id, current_frame_.length, current_frame_.flags, + padding_length_); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); + } + ENVOY_CONN_LOG(debug, "Http2Visitor: remaining data payload: {}, end_stream: {}", + connection_->connection_, remaining_data_payload_, + bool(current_frame_.flags & NGHTTP2_FLAG_END_STREAM)); + return true; +} + +bool ConnectionImpl::Http2Visitor::OnDataPaddingLength(Http2StreamId stream_id, + size_t padding_length) { + padding_length_ = padding_length; + remaining_data_payload_ -= padding_length; + if (remaining_data_payload_ == 0 && (current_frame_.flags & NGHTTP2_FLAG_END_STREAM) == 0) { + ENVOY_CONN_LOG(debug, "Http2Visitor dispatching DATA for stream {}", connection_->connection_, + stream_id); + Status status = connection_->onBeginData(stream_id, current_frame_.length, current_frame_.flags, + padding_length_); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); + } + ENVOY_CONN_LOG(debug, "Http2Visitor: remaining data payload: {}, end_stream: {}", + connection_->connection_, remaining_data_payload_, + bool(current_frame_.flags & NGHTTP2_FLAG_END_STREAM)); + return true; +} + +bool ConnectionImpl::Http2Visitor::OnDataForStream(Http2StreamId stream_id, + absl::string_view data) { + const int result = + connection_->onData(stream_id, reinterpret_cast(data.data()), data.size()); + remaining_data_payload_ -= data.size(); + if (result == 0 && remaining_data_payload_ == 0 && + (current_frame_.flags & NGHTTP2_FLAG_END_STREAM) == 0) { + ENVOY_CONN_LOG(debug, "Http2Visitor dispatching DATA for stream {}", connection_->connection_, + stream_id); + Status status = connection_->onBeginData(stream_id, current_frame_.length, current_frame_.flags, + padding_length_); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); + } + ENVOY_CONN_LOG(debug, "Http2Visitor: remaining data payload: {}, end_stream: {}", + connection_->connection_, remaining_data_payload_, + bool(current_frame_.flags & NGHTTP2_FLAG_END_STREAM)); + return result == 0; +} + +bool ConnectionImpl::Http2Visitor::OnEndStream(Http2StreamId stream_id) { + ENVOY_CONN_LOG(debug, "Http2Visitor::OnEndStream({})", connection_->connection_, stream_id); + if (current_frame_.type == NGHTTP2_DATA) { + // `onBeginData` is invoked here to ensure that the connection has successfully validated and + // processed the entire DATA frame. + ENVOY_CONN_LOG(debug, "Http2Visitor dispatching DATA for stream {}", connection_->connection_, + stream_id); + Status status = connection_->onBeginData(stream_id, current_frame_.length, current_frame_.flags, + padding_length_); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); + } + return true; +} + +void ConnectionImpl::Http2Visitor::OnRstStream(Http2StreamId stream_id, Http2ErrorCode error_code) { + (void)connection_->onRstStream(stream_id, static_cast(error_code)); +} + +bool ConnectionImpl::Http2Visitor::OnCloseStream(Http2StreamId stream_id, + Http2ErrorCode error_code) { + Status status = connection_->onStreamClose(stream_id, static_cast(error_code)); + if (stream_close_listener_) { + ENVOY_CONN_LOG(debug, "Http2Visitor invoking stream close listener for {}", + connection_->connection_, stream_id); + stream_close_listener_(stream_id); + } + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); +} + +void ConnectionImpl::Http2Visitor::OnPing(Http2PingId ping_id, bool is_ack) { + const uint64_t network_order_opaque_data = quiche::QuicheEndian::HostToNet64(ping_id); + Status status = connection_->onPing(network_order_opaque_data, is_ack); + connection_->setAndCheckCodecCallbackStatus(std::move(status)); +} + +bool ConnectionImpl::Http2Visitor::OnGoAway(Http2StreamId /*last_accepted_stream_id*/, + Http2ErrorCode error_code, + absl::string_view /*opaque_data*/) { + Status status = connection_->onGoAway(static_cast(error_code)); + return 0 == connection_->setAndCheckCodecCallbackStatus(std::move(status)); +} + +int ConnectionImpl::Http2Visitor::OnBeforeFrameSent(uint8_t frame_type, Http2StreamId stream_id, + size_t length, uint8_t flags) { + return connection_->onBeforeFrameSend(stream_id, length, frame_type, flags); +} + +int ConnectionImpl::Http2Visitor::OnFrameSent(uint8_t frame_type, Http2StreamId stream_id, + size_t length, uint8_t flags, uint32_t error_code) { + return connection_->onFrameSend(stream_id, length, frame_type, flags, error_code); +} + +bool ConnectionImpl::Http2Visitor::OnInvalidFrame(Http2StreamId stream_id, + InvalidFrameError error) { + return 0 == connection_->onInvalidFrame(stream_id, http2::adapter::ToNgHttp2ErrorCode(error)); +} + +bool ConnectionImpl::Http2Visitor::OnMetadataForStream(Http2StreamId stream_id, + absl::string_view metadata) { + return 0 == connection_->onMetadataReceived( + stream_id, reinterpret_cast(metadata.data()), metadata.size()); +} + +bool ConnectionImpl::Http2Visitor::OnMetadataEndForStream(Http2StreamId stream_id) { + return 0 == connection_->onMetadataFrameComplete(stream_id, true); +} + +void ConnectionImpl::Http2Visitor::OnErrorDebug(absl::string_view message) { + connection_->onError(message); +} + ConnectionImpl::Http2Options::Http2Options( const envoy::config::core::v3::Http2ProtocolOptions& http2_options, uint32_t max_headers_kb) { og_options_.perspective = http2::adapter::Perspective::kServer; @@ -2127,20 +2336,37 @@ ServerConnectionImpl::ServerConnectionImpl( "found. Is it configured?"); Http2Options h2_options(http2_options, max_request_headers_kb); - auto visitor = std::make_unique( + auto callback_visitor = std::make_unique( http2::adapter::Perspective::kServer, *http2_callbacks_.callbacks(), base()); + auto direct_visitor = std::make_unique(this); + if (use_oghttp2_library_) { - visitor_ = std::move(visitor); + if (skipCallbackVisitor()) { + visitor_ = std::move(direct_visitor); + } else { + visitor_ = std::move(callback_visitor); + } adapter_ = http2::adapter::OgHttp2Adapter::Create(*visitor_, h2_options.ogOptions()); } else { - auto adapter = - http2::adapter::NgHttp2Adapter::CreateServerAdapter(*visitor, h2_options.options()); - auto stream_close_listener = [p = adapter.get()](http2::adapter::Http2StreamId stream_id) { - p->RemoveStream(stream_id); - }; - visitor->set_stream_close_listener(std::move(stream_close_listener)); - visitor_ = std::move(visitor); - adapter_ = std::move(adapter); + if (skipCallbackVisitor()) { + auto adapter = http2::adapter::NgHttp2Adapter::CreateServerAdapter(*direct_visitor, + h2_options.options()); + auto stream_close_listener = [p = adapter.get()](http2::adapter::Http2StreamId stream_id) { + p->RemoveStream(stream_id); + }; + direct_visitor->setStreamCloseListener(std::move(stream_close_listener)); + visitor_ = std::move(direct_visitor); + adapter_ = std::move(adapter); + } else { + auto adapter = http2::adapter::NgHttp2Adapter::CreateServerAdapter(*callback_visitor, + h2_options.options()); + auto stream_close_listener = [p = adapter.get()](http2::adapter::Http2StreamId stream_id) { + p->RemoveStream(stream_id); + }; + callback_visitor->set_stream_close_listener(std::move(stream_close_listener)); + visitor_ = std::move(callback_visitor); + adapter_ = std::move(adapter); + } } sendSettings(http2_options, false); allow_metadata_ = http2_options.allow_metadata(); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 50e9411eaad7..25bd21fc276f 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -159,12 +159,15 @@ class ConnectionImpl : public virtual Connection, ExecutionContext* executionContext() const override; void dumpState(std::ostream& os, int indent_level) const override; + bool skipCallbackVisitor() const { return skip_callback_visitor_; } + protected: friend class ProdNghttp2SessionFactory; /** * Wrapper for static nghttp2 callback dispatchers. */ + // TODO: remove when removing `envoy.reloadable_features.http2_skip_callback_visitor`. class Http2Callbacks { public: Http2Callbacks(); @@ -176,6 +179,74 @@ class ConnectionImpl : public virtual Connection, nghttp2_session_callbacks* callbacks_; }; + /** + * This class handles protocol events from the codec layer. + */ + class Http2Visitor : public http2::adapter::Http2VisitorInterface { + public: + using Http2ErrorCode = http2::adapter::Http2ErrorCode; + using Http2PingId = http2::adapter::Http2PingId; + using Http2Setting = http2::adapter::Http2Setting; + using Http2StreamId = http2::adapter::Http2StreamId; + + explicit Http2Visitor(ConnectionImpl* connection); + + void setStreamCloseListener(std::function f) { + stream_close_listener_ = std::move(f); + } + int64_t OnReadyToSend(absl::string_view serialized) override; + void OnConnectionError(ConnectionError /*error*/) override {} + bool OnFrameHeader(Http2StreamId stream_id, size_t length, uint8_t type, + uint8_t flags) override; + void OnSettingsStart() override { settings_.clear(); } + void OnSetting(Http2Setting setting) override { settings_.push_back(setting); } + void OnSettingsEnd() override { connection_->onSettings(settings_); } + void OnSettingsAck() override {} + bool OnBeginHeadersForStream(Http2StreamId stream_id) override; + OnHeaderResult OnHeaderForStream(Http2StreamId stream_id, absl::string_view name_view, + absl::string_view value_view) override; + bool OnEndHeadersForStream(Http2StreamId stream_id) override; + bool OnDataPaddingLength(Http2StreamId stream_id, size_t padding_length) override; + bool OnBeginDataForStream(Http2StreamId stream_id, size_t payload_length) override; + bool OnDataForStream(Http2StreamId stream_id, absl::string_view data) override; + bool OnEndStream(Http2StreamId stream_id) override; + void OnRstStream(Http2StreamId stream_id, Http2ErrorCode error_code) override; + bool OnCloseStream(Http2StreamId stream_id, Http2ErrorCode error_code) override; + void OnPriorityForStream(Http2StreamId /*stream_id*/, Http2StreamId /*parent_stream_id*/, + int /*weight*/, bool /*exclusive*/) override {} + void OnPing(Http2PingId ping_id, bool is_ack) override; + void OnPushPromiseForStream(Http2StreamId /*stream_id*/, + Http2StreamId /*promised_stream_id*/) override {} + bool OnGoAway(Http2StreamId last_accepted_stream_id, Http2ErrorCode error_code, + absl::string_view opaque_data) override; + void OnWindowUpdate(Http2StreamId /*stream_id*/, int /*window_increment*/) override {} + int OnBeforeFrameSent(uint8_t frame_type, Http2StreamId stream_id, size_t length, + uint8_t flags) override; + int OnFrameSent(uint8_t frame_type, Http2StreamId stream_id, size_t length, uint8_t flags, + uint32_t error_code) override; + bool OnInvalidFrame(Http2StreamId stream_id, InvalidFrameError error) override; + void OnBeginMetadataForStream(Http2StreamId /*stream_id*/, size_t /*payload_length*/) override { + } + bool OnMetadataForStream(Http2StreamId stream_id, absl::string_view metadata) override; + bool OnMetadataEndForStream(Http2StreamId stream_id) override; + void OnErrorDebug(absl::string_view message) override; + + private: + ConnectionImpl* const connection_; + std::vector settings_; + struct FrameHeaderInfo { + Http2StreamId stream_id; + size_t length; + uint8_t type; + uint8_t flags; + }; + FrameHeaderInfo current_frame_ = {}; + size_t padding_length_ = 0; + size_t remaining_data_payload_ = 0; + // TODO: remove when removing `envoy.reloadable_features.http2_use_oghttp2`. + std::function stream_close_listener_; + }; + /** * Wrapper for static nghttp2 session options. */ @@ -644,6 +715,8 @@ class ConnectionImpl : public virtual Connection, // Whether to use the new HTTP/2 library. bool use_oghttp2_library_; + + // TODO: remove when removing `envoy.reloadable_features.http2_skip_callback_visitor`. static Http2Callbacks http2_callbacks_; // If deferred processing, the streams will be in LRU order based on when the @@ -753,6 +826,9 @@ class ConnectionImpl : public virtual Connection, std::chrono::milliseconds keepalive_interval_; std::chrono::milliseconds keepalive_timeout_; uint32_t keepalive_interval_jitter_percent_; + + const bool skip_callback_visitor_ = + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_skip_callback_visitor"); }; /** diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index bb6d471e71e7..0f2f5862ca25 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -57,6 +57,7 @@ RUNTIME_GUARD(envoy_reloadable_features_http1_connection_close_header_in_redirec RUNTIME_GUARD(envoy_reloadable_features_http1_use_balsa_parser); RUNTIME_GUARD(envoy_reloadable_features_http2_decode_metadata_with_quiche); RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header); +RUNTIME_GUARD(envoy_reloadable_features_http2_skip_callback_visitor); RUNTIME_GUARD(envoy_reloadable_features_http2_validate_authority_with_quiche); RUNTIME_GUARD(envoy_reloadable_features_http_allow_partial_urls_in_referer); RUNTIME_GUARD(envoy_reloadable_features_http_filter_avoid_reentrant_local_reply); diff --git a/test/integration/http_protocol_integration.cc b/test/integration/http_protocol_integration.cc index 9ea5174638f2..50f79fdf6366 100644 --- a/test/integration/http_protocol_integration.cc +++ b/test/integration/http_protocol_integration.cc @@ -26,11 +26,11 @@ std::vector HttpProtocolIntegrationTest::getProtocolTest } std::vector http2_implementations = {Http2Impl::Nghttp2}; - std::vector defer_processing_values = {false}; + std::vector http2_bool_values = {false}; if (downstream_protocol == Http::CodecType::HTTP2 || upstream_protocol == Http::CodecType::HTTP2) { http2_implementations.push_back(Http2Impl::Oghttp2); - defer_processing_values.push_back(true); + http2_bool_values.push_back(true); } std::vector use_header_validator_values; @@ -41,11 +41,14 @@ std::vector HttpProtocolIntegrationTest::getProtocolTest #endif for (Http1ParserImpl http1_implementation : http1_implementations) { for (Http2Impl http2_implementation : http2_implementations) { - for (bool defer_processing : defer_processing_values) { - for (bool use_header_validator : use_header_validator_values) { - ret.push_back(HttpProtocolTestParams{ - ip_version, downstream_protocol, upstream_protocol, http1_implementation, - http2_implementation, defer_processing, use_header_validator}); + for (bool defer_processing : http2_bool_values) { + for (bool deprecate_callback_visitor : http2_bool_values) { + for (bool use_header_validator : use_header_validator_values) { + ret.push_back(HttpProtocolTestParams{ + ip_version, downstream_protocol, upstream_protocol, http1_implementation, + http2_implementation, defer_processing, use_header_validator, + deprecate_callback_visitor}); + } } } } @@ -99,6 +102,8 @@ std::string HttpProtocolIntegrationTest::protocolTestParamsToString( http2ImplementationToString(params.param.http2_implementation), params.param.defer_processing_backedup_streams ? "WithDeferredProcessing" : "NoDeferredProcessing", + params.param.deprecate_callback_visitor ? "WithCallbackVisitor" + : "NoCallbackVisitor", params.param.use_universal_header_validator ? "Uhv" : "Legacy"); } diff --git a/test/integration/http_protocol_integration.h b/test/integration/http_protocol_integration.h index 5988e1ecc061..0c0dd6c1bbc0 100644 --- a/test/integration/http_protocol_integration.h +++ b/test/integration/http_protocol_integration.h @@ -15,6 +15,7 @@ struct HttpProtocolTestParams { Http2Impl http2_implementation; bool defer_processing_backedup_streams; bool use_universal_header_validator; + bool deprecate_callback_visitor; }; absl::string_view http2ImplementationToString(Http2Impl impl); @@ -81,6 +82,8 @@ class HttpProtocolIntegrationTest : public testing::TestWithParam