http: envoy.reloadable_features.internal_redirects_with_body deprecat…

…ion (envoyproxy#19670)

Commit Message: http: envoy.reloadable_features.internal_redirects_with_body deprecation
[Optional Fixes #Issue] envoyproxy#19506

Signed-off-by: Loong <loong.dai@intel.com>
Signed-off-by: Josh Perry <josh.perry@mx.com>

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.
 

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<ResponseHeaderMapImpl>(
@@ -1668,8 +1665,7 @@ void ConnectionManagerImpl::ActiveStream::recreateStream(
 
   Buffer::InstancePtr request_data = std::make_unique<Buffer::OwnedImpl>();
   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);
   }

source/common/http/conn_manager_impl.h

@@ -285,10 +285,6 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     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<Logger::Id::http>,
   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};

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;
   }
 

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; }

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_);

source/common/router/router.h

@@ -600,7 +600,6 @@ class Filter : Logger::Loggable<Logger::Id::router>,
   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};
 

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",

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);

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_;