From c3904437f1b507ece49a826af1ef71701eec7b22 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Tue, 24 Oct 2023 19:02:20 +0000 Subject: [PATCH] Remove expand agnostic stream lifetime runtime guard. Signed-off-by: Kevin Baichoo --- changelogs/current.yaml | 3 + source/common/http/conn_manager_impl.cc | 28 +++--- source/common/http/conn_manager_impl.h | 2 - source/common/runtime/runtime_features.cc | 1 - source/common/runtime/runtime_features.h | 2 - .../buffer_accounting_integration_test.cc | 99 ------------------- test/integration/filter_integration_test.cc | 12 +-- .../multiplexed_integration_test.cc | 17 +--- test/integration/protocol_integration_test.cc | 24 ++--- 9 files changed, 28 insertions(+), 160 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 8ddf455f3da8..236e52e2f891 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -22,6 +22,9 @@ bug_fixes: removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` +- area: http + change: | + Removed ``envoy.reloadable_features.expand_agnostic_stream_lifetime`` and legacy code paths. - area: http change: | removed ``envoy.reloadable_features.correctly_validate_alpn`` and legacy code paths. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index fe0a08615e39..4b8b59d8238c 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -314,19 +314,17 @@ void ConnectionManagerImpl::doDeferredStreamDestroy(ActiveStream& stream) { stream.access_log_flush_timer_ = nullptr; } - if (stream.expand_agnostic_stream_lifetime_) { - // Only destroy the active stream if the underlying codec has notified us of - // completion or we've internal redirect the stream. - if (!stream.canDestroyStream()) { - // Track that this stream is not expecting any additional calls apart from - // codec notification. - stream.state_.is_zombie_stream_ = true; - return; - } + // Only destroy the active stream if the underlying codec has notified us of + // completion or we've internal redirect the stream. + if (!stream.canDestroyStream()) { + // Track that this stream is not expecting any additional calls apart from + // codec notification. + stream.state_.is_zombie_stream_ = true; + return; + } - if (stream.response_encoder_ != nullptr) { - stream.response_encoder_->getStream().registerCodecEventCallbacks(nullptr); - } + if (stream.response_encoder_ != nullptr) { + stream.response_encoder_->getStream().registerCodecEventCallbacks(nullptr); } stream.completeRequest(); @@ -421,9 +419,7 @@ RequestDecoder& ConnectionManagerImpl::newStream(ResponseEncoder& response_encod new_stream->state_.is_internally_created_ = is_internally_created; new_stream->response_encoder_ = &response_encoder; new_stream->response_encoder_->getStream().addCallbacks(*new_stream); - if (new_stream->expand_agnostic_stream_lifetime_) { - new_stream->response_encoder_->getStream().registerCodecEventCallbacks(new_stream.get()); - } + new_stream->response_encoder_->getStream().registerCodecEventCallbacks(new_stream.get()); new_stream->response_encoder_->getStream().setFlushTimeout(new_stream->idle_timeout_ms_); new_stream->streamInfo().setDownstreamBytesMeter(response_encoder.getStream().bytesMeter()); // If the network connection is backed up, the stream should be made aware of it on creation. @@ -845,8 +841,6 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect StreamInfo::FilterState::LifeSpan::Connection), request_response_timespan_(new Stats::HistogramCompletableTimespanImpl( connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), - expand_agnostic_stream_lifetime_( - Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)), header_validator_( connection_manager.config_.makeHeaderValidator(connection_manager.codec_->protocol())) { ASSERT(!connection_manager.config_.isRoutable() || diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 30a9d5039719..3b6c9c5a5a41 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -472,8 +472,6 @@ class ConnectionManagerImpl : Logger::Loggable, std::chrono::milliseconds idle_timeout_ms_{}; State state_; - const bool expand_agnostic_stream_lifetime_; - // Snapshot of the route configuration at the time of request is started. This is used to ensure // that the same route configuration is used throughout the lifetime of the request. This // snapshot will be cleared when the cached route is blocked. Because after that we will not diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index dc538c51dc77..4edd64a9dd7c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -44,7 +44,6 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_compression_bomb_protection); RUNTIME_GUARD(envoy_reloadable_features_enable_connect_udp_support); RUNTIME_GUARD(envoy_reloadable_features_enable_intermediate_ca); RUNTIME_GUARD(envoy_reloadable_features_enable_zone_routing_different_zone_counts); -RUNTIME_GUARD(envoy_reloadable_features_expand_agnostic_stream_lifetime); RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_send_original_xff); RUNTIME_GUARD(envoy_reloadable_features_format_ports_as_numbers); RUNTIME_GUARD(envoy_reloadable_features_handle_uppercase_scheme); diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h index 0f89b781c352..0c2fb2d9b9e7 100644 --- a/source/common/runtime/runtime_features.h +++ b/source/common/runtime/runtime_features.h @@ -26,8 +26,6 @@ void maybeSetRuntimeGuard(absl::string_view name, bool value); void maybeSetDeprecatedInts(absl::string_view name, uint32_t value); constexpr absl::string_view defer_processing_backedup_streams = "envoy.reloadable_features.defer_processing_backedup_streams"; -constexpr absl::string_view expand_agnostic_stream_lifetime = - "envoy.reloadable_features.expand_agnostic_stream_lifetime"; } // namespace Runtime } // namespace Envoy diff --git a/test/integration/buffer_accounting_integration_test.cc b/test/integration/buffer_accounting_integration_test.cc index 05bb8c1ae3ea..46d5194893d1 100644 --- a/test/integration/buffer_accounting_integration_test.cc +++ b/test/integration/buffer_accounting_integration_test.cc @@ -878,105 +878,6 @@ TEST_P(Http2OverloadManagerIntegrationTest, EXPECT_EQ(smallest_response->headers().getStatusValue(), "200"); } -TEST_P(Http2OverloadManagerIntegrationTest, CanResetStreamIfEnvoyLevelStreamEnded) { - // This test is not applicable if expand_agnostic_stream_lifetime is enabled - // as the gap between lifetimes of the codec level and envoy level stream - // shrinks. - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - return; - } - - useAccessLog("%RESPONSE_CODE%"); - initializeOverloadManagerInBootstrap( - TestUtility::parseYaml(R"EOF( - name: "envoy.overload_actions.reset_high_memory_stream" - triggers: - - name: "envoy.resource_monitors.testonly.fake_resource_monitor" - scaled: - scaling_threshold: 0.90 - saturation_threshold: 0.98 - )EOF")); - initialize(); - - // Set 10MiB receive window for the client. - const int downstream_window_size = 10 * 1024 * 1024; - envoy::config::core::v3::Http2ProtocolOptions http2_options = - ::Envoy::Http2::Utility::initializeAndValidateOptions( - envoy::config::core::v3::Http2ProtocolOptions()); - http2_options.mutable_initial_stream_window_size()->set_value(downstream_window_size); - http2_options.mutable_initial_connection_window_size()->set_value(downstream_window_size); - codec_client_ = makeRawHttpConnection(makeClientConnection(lookupPort("http")), http2_options); - - // Makes us have Envoy's writes to downstream return EAGAIN - write_matcher_->setSourcePort(lookupPort("http")); - write_matcher_->setWriteReturnsEgain(); - - // Send a request - auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ - {":method", "POST"}, - {":path", "/"}, - {":scheme", "http"}, - {":authority", "host"}, - {"content-length", "10"}, - }); - auto& encoder = encoder_decoder.first; - const std::string data(10, 'a'); - codec_client_->sendData(encoder, data, true); - auto response = std::move(encoder_decoder.second); - - waitForNextUpstreamRequest(); - FakeStreamPtr upstream_request_for_response = std::move(upstream_request_); - - // Send the responses back. It is larger than the downstream's receive window - // size. Thus, the codec will not end the stream, but the Envoy level stream - // should. - upstream_request_for_response->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, - false); - const int response_size = downstream_window_size + 1024; // Slightly over the window size. - upstream_request_for_response->encodeData(response_size, true); - - if (streamBufferAccounting()) { - if (deferProcessingBackedUpStreams()) { - // Wait for an accumulation of data, as we cannot rely on the access log - // output since we're deferring the processing of the stream data. - EXPECT_TRUE(buffer_factory_->waitUntilTotalBufferedExceeds(10 * 10 * 1024)); - - } else { - // Wait for access log to know the Envoy level stream has been deleted. - EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("200")); - } - } - - // Set the pressure so the overload action kills the response if doing stream - // accounting - updateResource(0.95); - test_server_->waitForGaugeEq( - "overload.envoy.overload_actions.reset_high_memory_stream.scale_percent", 62); - - if (streamBufferAccounting()) { - test_server_->waitForCounterGe("envoy.overload_actions.reset_high_memory_stream.count", 1); - } - - // Reduce resource pressure - updateResource(0.80); - test_server_->waitForGaugeEq( - "overload.envoy.overload_actions.reset_high_memory_stream.scale_percent", 0); - - // Resume writes to downstream. - write_matcher_->setResumeWrites(); - - if (streamBufferAccounting()) { - EXPECT_TRUE(response->waitForReset()); - EXPECT_TRUE(response->reset()); - } else { - // If we're not doing the accounting, we didn't end up resetting the - // streams. - ASSERT_TRUE(response->waitForEndStream()); - ASSERT_TRUE(response->complete()); - EXPECT_EQ(response->headers().getStatusValue(), "200"); - } -} - class Http2DeferredProcessingIntegrationTest : public Http2BufferWatermarksTest { public: Http2DeferredProcessingIntegrationTest() : registered_tee_factory_(tee_filter_factory_) { diff --git a/test/integration/filter_integration_test.cc b/test/integration/filter_integration_test.cc index 312326bef37b..8657a98034dd 100644 --- a/test/integration/filter_integration_test.cc +++ b/test/integration/filter_integration_test.cc @@ -184,15 +184,9 @@ TEST_P(FilterIntegrationTest, MissingHeadersLocalReplyDownstreamBytesCount) { EXPECT_EQ("200", response->headers().getStatusValue()); if (testing_downstream_filter_) { - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(40, 58, 40, 58), - BytesCountExpectation(7, 10, 7, 8)); - } else { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(0, 58, 0, 58), - BytesCountExpectation(7, 10, 7, 8)); - } + expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), + BytesCountExpectation(40, 58, 40, 58), + BytesCountExpectation(7, 10, 7, 8)); } } diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 2d24888fc92e..580eb8b40716 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -2159,11 +2159,6 @@ TEST_P(Http2FrameIntegrationTest, AccessLogOfWireBytesIfResponseSizeGreaterThanW // Check access log if the agnostic stream lifetime is not extended. // It should have access logged since it has received the entire response. int hcm_logged_wire_bytes_sent, hcm_logged_wire_header_bytes_sent; - if (!Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - auto access_log_values = stoiAccessLogString(waitForAccessLog(access_log_name_)); - hcm_logged_wire_bytes_sent = access_log_values[0]; - hcm_logged_wire_header_bytes_sent = access_log_values[1]; - } // Grant the sender (Envoy) additional window so it can finish sending the // stream. @@ -2181,13 +2176,11 @@ TEST_P(Http2FrameIntegrationTest, AccessLogOfWireBytesIfResponseSizeGreaterThanW EXPECT_EQ(accumulator.bodyWireBytesReceivedDiscountingHeaders(), accumulator.bodyWireBytesReceivedGivenPayloadAndFrames()); - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - // Access logs are only available now due to the expanded agnostic stream - // lifetime. - auto access_log_values = stoiAccessLogString(waitForAccessLog(access_log_name_)); - hcm_logged_wire_bytes_sent = access_log_values[0]; - hcm_logged_wire_header_bytes_sent = access_log_values[1]; - } + // Access logs are only available now due to the expanded agnostic stream + // lifetime. + auto access_log_values = stoiAccessLogString(waitForAccessLog(access_log_name_)); + hcm_logged_wire_bytes_sent = access_log_values[0]; + hcm_logged_wire_header_bytes_sent = access_log_values[1]; EXPECT_EQ(accumulator.stream_wire_header_bytes_recieved_, hcm_logged_wire_header_bytes_sent); EXPECT_EQ(accumulator.stream_wire_bytes_recieved_, hcm_logged_wire_bytes_sent) << "Received " << accumulator.stream_wire_bytes_recieved_ diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e3f35929356c..e554cedad5ea 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -201,15 +201,9 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterRedirectHttpRequest) { EXPECT_EQ("301", response->headers().getStatusValue()); EXPECT_EQ("https://www.redirect.com/foo", response->headers().get(Http::Headers::get().Location)[0]->value().getStringView()); - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(145, 45, 111, 23), - BytesCountExpectation(69, 30, 69, 30), - BytesCountExpectation(0, 30, 0, 30)); - } else { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(145, 45, 111, 23), - BytesCountExpectation(0, 30, 0, 30), - BytesCountExpectation(0, 30, 0, 30)); - } + expectDownstreamBytesSentAndReceived(BytesCountExpectation(145, 45, 111, 23), + BytesCountExpectation(69, 30, 69, 30), + BytesCountExpectation(0, 30, 0, 30)); } else { // All QUIC requests use https, and should not be redirected. (Even those sent with http scheme // will be overridden to https by HCM.) @@ -717,15 +711,9 @@ TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReplyDownstreamByte ASSERT_TRUE(response->waitForEndStream()); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - if (Runtime::runtimeFeatureEnabled(Runtime::expand_agnostic_stream_lifetime)) { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(40, 58, 40, 58), - BytesCountExpectation(7, 10, 7, 8)); - } else { - expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), - BytesCountExpectation(0, 58, 0, 58), - BytesCountExpectation(7, 10, 7, 8)); - } + expectDownstreamBytesSentAndReceived(BytesCountExpectation(90, 88, 71, 54), + BytesCountExpectation(40, 58, 40, 58), + BytesCountExpectation(7, 10, 7, 8)); } TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReplyUpstreamBytesCount) {