diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3e28393047f0..aadd7a8b6eb6 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -41,6 +41,7 @@ Removed Config or Runtime * http: removed ``envoy.reloadable_features.http_transport_failure_reason_in_body`` and legacy code paths. * http: removed ``envoy.reloadable_features.allow_response_for_timeout`` and legacy code paths. * http: removed ``envoy.reloadable_features.http2_consume_stream_refused_errors`` and legacy code paths. +* http: removed ``envoy.reloadable_features.internal_redirects_with_body`` and legacy code paths. * udp: removed ``envoy.reloadable_features.udp_per_event_loop_read_limit`` and legacy code paths. * upstream: removed ``envoy.reloadable_features.health_check.graceful_goaway_handling`` and legacy code paths. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 461bcc895c29..a43b837e5b5c 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -103,13 +103,10 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, overload_state_.getState(Server::OverloadActionNames::get().StopAcceptingRequests)), overload_disable_keepalive_ref_( overload_state_.getState(Server::OverloadActionNames::get().DisableHttpKeepAlive)), - time_source_(time_source), - enable_internal_redirects_with_body_( - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body")), - proxy_name_(StreamInfo::ProxyStatusUtils::makeProxyName( - /*node_id=*/local_info_.node().id(), - /*server_name=*/config_.serverName(), - /*proxy_status_config=*/config_.proxyStatusConfig())) {} + time_source_(time_source), proxy_name_(StreamInfo::ProxyStatusUtils::makeProxyName( + /*node_id=*/local_info_.node().id(), + /*server_name=*/config_.serverName(), + /*proxy_status_config=*/config_.proxyStatusConfig())) {} const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() { static const auto headers = createHeaderMap( @@ -1668,8 +1665,7 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( Buffer::InstancePtr request_data = std::make_unique(); const auto& buffered_request_data = filter_manager_.bufferedRequestData(); - const bool proxy_body = connection_manager_.enable_internal_redirects_with_body_ && - buffered_request_data != nullptr && buffered_request_data->length() > 0; + const bool proxy_body = buffered_request_data != nullptr && buffered_request_data->length() > 0; if (proxy_body) { request_data->move(*buffered_request_data); } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 626f64696c91..3eda4a33f86f 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -285,10 +285,6 @@ class ConnectionManagerImpl : Logger::Loggable, Tracing::Config& tracingConfig() override; const ScopeTrackedObject& scope() override; - bool enableInternalRedirectsWithBody() const override { - return connection_manager_.enable_internal_redirects_with_body_; - } - void traceRequest(); // Updates the snapped_route_config_ (by reselecting scoped route configuration), if a scope is @@ -454,7 +450,6 @@ class ConnectionManagerImpl : Logger::Loggable, const Server::OverloadActionState& overload_disable_keepalive_ref_; TimeSource& time_source_; bool remote_close_{}; - bool enable_internal_redirects_with_body_{}; // Hop by hop headers should always be cleared for Envoy-as-a-proxy but will // not be for Envoy-mobile. bool clear_hop_by_hop_response_headers_{true}; diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 3d1e269a01bb..a121d084a274 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1483,8 +1483,7 @@ bool ActiveStreamDecoderFilter::recreateStream(const ResponseHeaderMap* headers) // Because the filter's and the HCM view of if the stream has a body and if // the stream is complete may differ, re-check bytesReceived() to make sure // there was no body from the HCM's point of view. - if (!complete() || - (!parent_.enableInternalRedirectsWithBody() && parent_.stream_info_.bytesReceived() != 0)) { + if (!complete()) { return false; } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 06de03b77e18..ff3c02ab85b9 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -577,11 +577,6 @@ class FilterManagerCallbacks { * Returns the tracked scope to use for this stream. */ virtual const ScopeTrackedObject& scope() PURE; - - /** - * Returns whether internal redirects with request bodies is enabled. - */ - virtual bool enableInternalRedirectsWithBody() const PURE; }; /** @@ -941,10 +936,6 @@ class FilterManager : public ScopeTrackedObject, Buffer::InstancePtr& bufferedRequestData() { return buffered_request_data_; } - bool enableInternalRedirectsWithBody() const { - return filter_manager_callbacks_.enableInternalRedirectsWithBody(); - } - void contextOnContinue(ScopeTrackedObjectStack& tracked_object_stack); void onDownstreamReset() { state_.saw_downstream_reset_ = true; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4e67e9e46df1..5a27b6d27283 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -688,9 +688,6 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, } } - internal_redirects_with_body_enabled_ = - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body"); - ENVOY_STREAM_LOG(debug, "router decoding headers:\n{}", *callbacks_, headers); // Hang onto the modify_headers function for later use in handling upstream responses. @@ -753,8 +750,7 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea ASSERT(upstream_requests_.size() <= 1); bool buffering = (retry_state_ && retry_state_->enabled()) || !active_shadow_policies_.empty() || - (internal_redirects_with_body_enabled_ && route_entry_ && - route_entry_->internalRedirectPolicy().enabled()); + (route_entry_ && route_entry_->internalRedirectPolicy().enabled()); if (buffering && getLength(callbacks_->decodingBuffer()) + data.length() > retry_shadow_buffer_limit_) { // The request is larger than we should buffer. Give up on the retry/shadow @@ -1554,9 +1550,7 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers) { const uint64_t status_code = Http::Utility::getResponseStatus(headers); // Redirects are not supported for streaming requests yet. - if (downstream_end_stream_ && - ((internal_redirects_with_body_enabled_ && !request_buffer_overflowed_) || - !callbacks_->decodingBuffer()) && + if (downstream_end_stream_ && (!request_buffer_overflowed_ || !callbacks_->decodingBuffer()) && location != nullptr && convertRequestHeadersForInternalRedirect(*downstream_headers_, *location, status_code) && callbacks_->recreateStream(&headers)) { @@ -1568,7 +1562,7 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers) { // details for other failure modes here. if (!downstream_end_stream_) { ENVOY_STREAM_LOG(trace, "Internal redirect failed: request incomplete", *callbacks_); - } else if (internal_redirects_with_body_enabled_ && request_buffer_overflowed_) { + } else if (request_buffer_overflowed_) { ENVOY_STREAM_LOG(trace, "Internal redirect failed: request body overflow", *callbacks_); } else if (location == nullptr) { ENVOY_STREAM_LOG(trace, "Internal redirect failed: missing location header", *callbacks_); diff --git a/source/common/router/router.h b/source/common/router/router.h index 9c9ae021574c..42344b8ce097 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -600,7 +600,6 @@ class Filter : Logger::Loggable, bool is_retry_ : 1; bool include_attempt_count_in_request_ : 1; bool request_buffer_overflowed_ : 1; - bool internal_redirects_with_body_enabled_ : 1; uint32_t attempt_count_{1}; uint32_t pending_retries_{0}; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9efab0519cf1..f69bbb3f796c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -71,7 +71,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.http_reject_path_with_fragment", "envoy.reloadable_features.http_strip_fragment_from_path_unsafe_if_disabled", "envoy.reloadable_features.internal_address", - "envoy.reloadable_features.internal_redirects_with_body", "envoy.reloadable_features.listener_reuse_port_default_enabled", "envoy.reloadable_features.listener_wildcard_match_ip_family", "envoy.reloadable_features.new_tcp_connection_pool", diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 2294d3ea3996..f1e89ee69f3b 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4540,26 +4540,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { .value()); } -TEST_F(RouterTest, InternalRedirectRejectedWithRequestBodyDisabledLegacy) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.internal_redirects_with_body", "false"}}); - - enableRedirects(); - sendRequest(); - - Buffer::InstancePtr body_data(new Buffer::OwnedImpl("random_fake_data")); - EXPECT_CALL(callbacks_, decodingBuffer()).WillOnce(Return(body_data.get())); - EXPECT_CALL(callbacks_, recreateStream(_)).Times(0); - - response_decoder_->decodeHeaders(std::move(redirect_headers_), false); - Buffer::OwnedImpl data("1234567890"); - response_decoder_->decodeData(data, true); - EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ - .counter("upstream_internal_redirect_failed_total") - .value()); -} - TEST_F(RouterTest, InternalRedirectAcceptedWithRequestBody) { enableRedirects(); sendRequest(false); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 490ada748b75..01c7a65e72bf 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -108,7 +108,6 @@ class MockFilterManagerCallbacks : public FilterManagerCallbacks { MOCK_METHOD(Tracing::Config&, tracingConfig, ()); MOCK_METHOD(const ScopeTrackedObject&, scope, ()); MOCK_METHOD(void, restoreContextOnContinue, (ScopeTrackedObjectStack&)); - MOCK_METHOD(bool, enableInternalRedirectsWithBody, (), (const)); ResponseHeaderMapPtr informational_headers_; ResponseHeaderMapPtr response_headers_;