diff --git a/docs/root/intro/arch_overview/http/http_connection_management.rst b/docs/root/intro/arch_overview/http/http_connection_management.rst index 2cc332d2f814d..575da12520c8e 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -153,7 +153,9 @@ Internal redirects Envoy supports handling 3xx redirects internally, that is capturing a configurable 3xx redirect response, synthesizing a new request, sending it to the upstream specified by the new route match, -and returning the redirected response as the response to the original request. +and returning the redirected response as the response to the original request. The headers and body +of the original request will be sent in the redirect to the new location. Trailers are not yet +supported. Internal redirects are configured via the :ref:`internal redirect policy ` field in route configuration. @@ -162,6 +164,11 @@ When redirect handling is on, any 3xx response from upstream, that matches ` is subject to the redirect being handled by Envoy. +If Envoy is configured to internally redirect HTTP 303 and receives an HTTP 303 response, it will +dispatch the redirect with a bodiless HTTP GET if the original request was not a GET or HEAD +request. Otherwise, Envoy will preserve the original HTTP method. For more information, see `RFC +7231 Section 6.4.4 `_. + For a redirect to be handled successfully it must pass the following checks: 1. Have a response code matching one of :ref:`redirect_response_codes @@ -169,7 +176,8 @@ For a redirect to be handled successfully it must pass the following checks: either 302 (by default), or a set of 3xx codes (301, 302, 303, 307, 308). 2. Have a *location* header with a valid, fully qualified URL. 3. The request must have been fully processed by Envoy. -4. The request must not have a body. +4. The request must be smaller than the :ref:`per_request_buffer_limit_bytes + ` limit. 5. :ref:`allow_cross_scheme_redirect ` is true (default to false), or the scheme of the downstream request and the *location* header are the same. diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a3c7b27d34e01..e5e1cf20f353a 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -47,6 +47,7 @@ Minor Behavior Changes whether receiving `x-envoy-immediate-health-check-fail` will cause exclusion or not. Thus, depending on the Envoy deployment, the feature flag may need to be flipped on both downstream and upstream instances, depending on the reason. +* http: added support for internal redirects with bodies. This behavior can be disabled temporarily by setting `envoy.reloadable_features.internal_redirects_with_body` to false. * http: allow to use path canonicalizer from `googleurl `_ instead of `//source/common/chromium_url`. The new path canonicalizer is enabled by default. To revert to the legacy path canonicalizer, enable the runtime flag diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 96d6581337e55..f3135ad207c56 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -101,7 +101,9 @@ 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) {} + time_source_(time_source), + enable_internal_redirects_with_body_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.internal_redirects_with_body")) {} const ResponseHeaderMap& ConnectionManagerImpl::continueHeader() { static const auto headers = createHeaderMap( @@ -1613,6 +1615,14 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( ResponseEncoder* response_encoder = response_encoder_; response_encoder_ = nullptr; + 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; + if (proxy_body) { + request_data->move(*buffered_request_data); + } + response_encoder->getStream().removeCallbacks(*this); // This functionally deletes the stream (via deferred delete) so do not // reference anything beyond this point. @@ -1631,7 +1641,13 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( filter_state->parent(), StreamInfo::FilterState::LifeSpan::FilterChain); } - new_stream.decodeHeaders(std::move(request_headers_), true); + new_stream.decodeHeaders(std::move(request_headers_), !proxy_body); + if (proxy_body) { + // This functionality is currently only used for internal redirects, which the router only + // allows if the full request has been read (end_stream = true) so we don't need to handle the + // case of upstream sending an early response mid-request. + new_stream.decodeData(*request_data, true); + } } Http1StreamEncoderOptionsOptRef ConnectionManagerImpl::ActiveStream::http1StreamEncoderOptions() { diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 86f99fa118cad..b05257f5b41b6 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -280,6 +280,10 @@ 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 @@ -445,6 +449,7 @@ class ConnectionManagerImpl : Logger::Loggable, const Server::OverloadActionState& overload_disable_keepalive_ref_; TimeSource& time_source_; bool remote_close_{}; + bool enable_internal_redirects_with_body_{}; }; } // namespace Http diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 754ff6e8f5942..71f357ff63faa 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1374,7 +1374,8 @@ 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_.stream_info_.bytesReceived() != 0) { + if (!complete() || + (!parent_.enableInternalRedirectsWithBody() && parent_.stream_info_.bytesReceived() != 0)) { return false; } diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index e72e800041d02..a687f3fc014c6 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -563,6 +563,11 @@ 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; }; /** @@ -901,6 +906,12 @@ class FilterManager : public ScopeTrackedObject, uint64_t streamId() const { return stream_id_; } + Buffer::InstancePtr& bufferedRequestData() { return buffered_request_data_; } + + bool enableInternalRedirectsWithBody() const { + return filter_manager_callbacks_.enableInternalRedirectsWithBody(); + } + private: // Indicates which filter to start the iteration with. enum class FilterIterationStartState { AlwaysStartFromNext, CanStartFromCurrent }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 97cbc0c92250a..9b63c3045acd1 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -625,6 +625,9 @@ 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. @@ -687,7 +690,9 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea // a backoff timer. ASSERT(upstream_requests_.size() <= 1); - bool buffering = (retry_state_ && retry_state_->enabled()) || !active_shadow_policies_.empty(); + bool buffering = (retry_state_ && retry_state_->enabled()) || !active_shadow_policies_.empty() || + (internal_redirects_with_body_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 @@ -695,6 +700,7 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea retry_state_.reset(); buffering = false; active_shadow_policies_.clear(); + request_buffer_overflowed_ = true; // If we had to abandon buffering and there's no request in progress, abort the request and // clean up. This happens if the initial upstream request failed, and we are currently waiting @@ -1475,7 +1481,7 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers, // destruction of this filter before the stream is marked as complete, and onDestroy will reset // the stream. // - // Normally when a stream is complete we signal this by resetting the upstream but this cam not + // Normally when a stream is complete we signal this by resetting the upstream but this cannot // be done in this case because if recreateStream fails, the "failure" path continues to call // code in onUpstreamHeaders which requires the upstream *not* be reset. To avoid onDestroy // performing a spurious stream reset in the case recreateStream() succeeds, we explicitly track @@ -1484,11 +1490,14 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers, attempting_internal_redirect_with_complete_stream_ = upstream_request.upstreamTiming().last_upstream_rx_byte_received_ && downstream_end_stream_; + const uint64_t status_code = Http::Utility::getResponseStatus(headers); + // Redirects are not supported for streaming requests yet. if (downstream_end_stream_ && - !callbacks_->decodingBuffer() && // Redirects with body not yet supported. + ((internal_redirects_with_body_enabled_ && !request_buffer_overflowed_) || + !callbacks_->decodingBuffer()) && location != nullptr && - convertRequestHeadersForInternalRedirect(*downstream_headers_, *location) && + convertRequestHeadersForInternalRedirect(*downstream_headers_, *location, status_code) && callbacks_->recreateStream(&headers)) { cluster_->stats().upstream_internal_redirect_succeeded_total_.inc(); return true; @@ -1502,7 +1511,8 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers, } bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream_headers, - const Http::HeaderEntry& internal_redirect) { + const Http::HeaderEntry& internal_redirect, + uint64_t status_code) { if (!downstream_headers.Path()) { ENVOY_STREAM_LOG(trace, "no path in downstream_headers", *callbacks_); return false; @@ -1581,6 +1591,15 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do } } + // See https://tools.ietf.org/html/rfc7231#section-6.4.4. + if (status_code == enumToInt(Http::Code::SeeOther) && + downstream_headers.getMethodValue() != Http::Headers::get().MethodValues.Get && + downstream_headers.getMethodValue() != Http::Headers::get().MethodValues.Head) { + downstream_headers.setMethod(Http::Headers::get().MethodValues.Get); + downstream_headers.remove(Http::Headers::get().ContentLength); + callbacks_->modifyDecodingBuffer([](Buffer::Instance& data) { data.drain(data.length()); }); + } + num_internal_redirect.increment(); restore_original_headers.cancel(); // Preserve the original request URL for the second pass. diff --git a/source/common/router/router.h b/source/common/router/router.h index 28d287c217b57..e0ba5d4bae993 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -283,7 +283,8 @@ class Filter : Logger::Loggable, : config_(config), final_upstream_request_(nullptr), downstream_100_continue_headers_encoded_(false), downstream_response_started_(false), downstream_end_stream_(false), is_retry_(false), - attempting_internal_redirect_with_complete_stream_(false) {} + attempting_internal_redirect_with_complete_stream_(false), + request_buffer_overflowed_(false) {} ~Filter() override; @@ -495,7 +496,8 @@ class Filter : Logger::Loggable, void sendNoHealthyUpstreamResponse(); bool setupRedirect(const Http::ResponseHeaderMap& headers, UpstreamRequest& upstream_request); bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream_headers, - const Http::HeaderEntry& internal_redirect); + const Http::HeaderEntry& internal_redirect, + uint64_t status_code); void updateOutlierDetection(Upstream::Outlier::Result result, UpstreamRequest& upstream_request, absl::optional code); void doRetry(); @@ -539,6 +541,8 @@ class Filter : Logger::Loggable, bool is_retry_ : 1; bool include_attempt_count_in_request_ : 1; bool attempting_internal_redirect_with_complete_stream_ : 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 90808c6548ebf..461c840d1545c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -78,6 +78,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.http_upstream_wait_connect_response", "envoy.reloadable_features.http2_skip_encoding_empty_trailers", "envoy.reloadable_features.improved_stream_limit_handling", + "envoy.reloadable_features.internal_redirects_with_body", "envoy.reloadable_features.overload_manager_disable_keepalive_drain_http2", "envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing", "envoy.reloadable_features.preserve_downstream_scheme", diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 8980e7cf7a7ae..94033e7721c74 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4045,7 +4045,7 @@ TEST_F(RouterTest, RetryUpstreamGrpcCancelled) { // Verifies that the initial host is select with max host count of one, but during retries // RetryPolicy will be consulted. -TEST_F(RouterTest, RetryRespsectsMaxHostSelectionCount) { +TEST_F(RouterTest, RetryRespectsMaxHostSelectionCount) { router_.reject_all_hosts_ = true; NiceMock encoder1; @@ -4259,7 +4259,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithInvalidLocation) { TEST_F(RouterTest, InternalRedirectRejectedWithoutCompleteRequest) { enableRedirects(); - sendRequest(false); EXPECT_CALL(callbacks_, recreateStream(_)).Times(0); @@ -4275,7 +4274,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutCompleteRequest) { TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { enableRedirects(); - sendRequest(); redirect_headers_->removeLocation(); @@ -4290,9 +4288,12 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { .value()); } -TEST_F(RouterTest, InternalRedirectRejectedWithBody) { - enableRedirects(); +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")); @@ -4307,14 +4308,43 @@ TEST_F(RouterTest, InternalRedirectRejectedWithBody) { .value()); } -TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { +TEST_F(RouterTest, InternalRedirectAcceptedWithRequestBody) { enableRedirects(); + sendRequest(false); + + EXPECT_CALL(callbacks_.dispatcher_, createTimer_); + + Buffer::InstancePtr body_data(new Buffer::OwnedImpl("random_fake_data")); + EXPECT_CALL(callbacks_, addDecodedData(_, true)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, router_.decodeData(*body_data, true)); + + EXPECT_CALL(callbacks_, clearRouteCache()); + EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true)); + + response_decoder_->decodeHeaders(std::move(redirect_headers_), false); + Buffer::OwnedImpl response_data("1234567890"); + response_decoder_->decodeData(response_data, false); + EXPECT_EQ(0U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_internal_redirect_failed_total") + .value()); + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_internal_redirect_succeeded_total") + .value()); + // In production, the HCM recreateStream would have called this. + router_.onDestroy(); + EXPECT_EQ(1, callbacks_.streamInfo() + .filterState() + ->getDataMutable("num_internal_redirects") + .value()); +} + +TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { + enableRedirects(); sendRequest(); redirect_headers_->setLocation("https://www.foo.com"); - EXPECT_CALL(callbacks_, decodingBuffer()); EXPECT_CALL(callbacks_, recreateStream(_)).Times(0); response_decoder_->decodeHeaders(std::move(redirect_headers_), true); @@ -4326,14 +4356,12 @@ TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { TEST_F(RouterTest, InternalRedirectRejectedByPredicate) { enableRedirects(); - sendRequest(); redirect_headers_->setLocation("http://www.foo.com/some/path"); auto mock_predicate = std::make_shared>(); - EXPECT_CALL(callbacks_, decodingBuffer()); EXPECT_CALL(callbacks_, clearRouteCache()); EXPECT_CALL(callbacks_.route_->route_entry_.internal_redirect_policy_, predicates()) .WillOnce(Return(std::vector({mock_predicate}))); @@ -4360,7 +4388,6 @@ TEST_F(RouterTest, HttpInternalRedirectSucceeded) { default_request_headers_.setForwardedProto("http"); sendRequest(); - EXPECT_CALL(callbacks_, decodingBuffer()); EXPECT_CALL(callbacks_, clearRouteCache()); EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true)); response_decoder_->decodeHeaders(std::move(redirect_headers_), false); @@ -4385,7 +4412,6 @@ TEST_F(RouterTest, HttpsInternalRedirectSucceeded) { redirect_headers_->setLocation("https://www.foo.com"); EXPECT_CALL(connection_, ssl()).WillOnce(Return(ssl_connection)); - EXPECT_CALL(callbacks_, decodingBuffer()); EXPECT_CALL(callbacks_, clearRouteCache()); EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true)); response_decoder_->decodeHeaders(std::move(redirect_headers_), false); @@ -4405,7 +4431,6 @@ TEST_F(RouterTest, CrossSchemeRedirectAllowedByPolicy) { redirect_headers_->setLocation("http://www.foo.com"); EXPECT_CALL(connection_, ssl()).WillOnce(Return(ssl_connection)); - EXPECT_CALL(callbacks_, decodingBuffer()); EXPECT_CALL(callbacks_.route_->route_entry_.internal_redirect_policy_, isCrossSchemeRedirectAllowed()) .WillOnce(Return(true)); diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index c0224f3077d17..4fac5f20765dc 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -145,6 +145,7 @@ TEST_P(RedirectIntegrationTest, BasicInternalRedirect) { codec_client_->makeHeaderOnlyRequest(default_request_headers_); waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(redirect_response_, true); waitForNextUpstreamRequest(); @@ -171,6 +172,252 @@ TEST_P(RedirectIntegrationTest, BasicInternalRedirect) { EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("200 via_upstream -\n")); } +TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { + useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); + // Validate that header sanitization is only called once. + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { hcm.set_via("via_value"); }); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + default_request_headers_.setHost("handle.internal.redirect"); + default_request_headers_.setMethod("POST"); + const std::string& request_body = "foobarbizbaz"; + + // First request to original upstream. + IntegrationStreamDecoderPtr response = + codec_client_->makeRequestWithBody(default_request_headers_, request_body); + waitForNextUpstreamRequest(); + EXPECT_EQ(request_body, upstream_request_->body().toString()); + + // Respond with a redirect. + upstream_request_->encodeHeaders(redirect_response_, true); + + // Second request to redirected upstream. + waitForNextUpstreamRequest(); + EXPECT_EQ(request_body, upstream_request_->body().toString()); + ASSERT(upstream_request_->headers().EnvoyOriginalUrl() != nullptr); + EXPECT_EQ("http://handle.internal.redirect/test/long/url", + upstream_request_->headers().getEnvoyOriginalUrlValue()); + EXPECT_EQ("/new/url", upstream_request_->headers().getPathValue()); + EXPECT_EQ("authority2", upstream_request_->headers().getHostValue()); + EXPECT_EQ("via_value", upstream_request_->headers().getViaValue()); + + // Return the response from the redirect upstream. + upstream_request_->encodeHeaders(default_response_headers_, true); + + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_internal_redirect_succeeded_total") + ->value()); + // 302 was never returned downstream + EXPECT_EQ(0, test_server_->counter("http.config_test.downstream_rq_3xx")->value()); + EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_2xx")->value()); + EXPECT_THAT(waitForAccessLog(access_log_name_, 0), + HasSubstr("302 internal_redirect test-header-value\n")); + // No test header + EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("200 via_upstream -\n")); +} + +TEST_P(RedirectIntegrationTest, InternalRedirectHandlesHttp303) { + useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + hcm.set_via("via_value"); + + auto* route = hcm.mutable_route_config()->mutable_virtual_hosts(2)->mutable_routes(0); + route->mutable_route() + ->mutable_internal_redirect_policy() + ->mutable_redirect_response_codes() + ->Add(303); + }); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + const std::string& request_body = "foobarbizbaz"; + default_request_headers_.setHost("handle.internal.redirect"); + default_request_headers_.setMethod("POST"); + default_request_headers_.setContentLength(request_body.length()); + + // First request to original upstream. + IntegrationStreamDecoderPtr response = + codec_client_->makeRequestWithBody(default_request_headers_, request_body); + waitForNextUpstreamRequest(); + EXPECT_EQ(request_body, upstream_request_->body().toString()); + + // Respond with a redirect. + redirect_response_.setStatus(303); + upstream_request_->encodeHeaders(redirect_response_, true); + + // Second request to redirected upstream. + waitForNextUpstreamRequest(); + EXPECT_EQ("", upstream_request_->body().toString()); + ASSERT(upstream_request_->headers().EnvoyOriginalUrl() != nullptr); + EXPECT_EQ("http://handle.internal.redirect/test/long/url", + upstream_request_->headers().getEnvoyOriginalUrlValue()); + EXPECT_EQ("/new/url", upstream_request_->headers().getPathValue()); + EXPECT_EQ("authority2", upstream_request_->headers().getHostValue()); + EXPECT_EQ("via_value", upstream_request_->headers().getViaValue()); + EXPECT_EQ("GET", upstream_request_->headers().getMethodValue()); + EXPECT_EQ("", upstream_request_->headers().getContentLengthValue()); + + // Return the response from the redirect upstream. + upstream_request_->encodeHeaders(default_response_headers_, true); + + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_internal_redirect_succeeded_total") + ->value()); + // 302 was never returned downstream + EXPECT_EQ(0, test_server_->counter("http.config_test.downstream_rq_3xx")->value()); + EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_2xx")->value()); + EXPECT_THAT(waitForAccessLog(access_log_name_, 0), + HasSubstr("303 internal_redirect test-header-value\n")); + // No test header + EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("200 via_upstream -\n")); +} + +TEST_P(RedirectIntegrationTest, InternalRedirectHttp303PreservesHeadMethod) { + useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + hcm.set_via("via_value"); + + auto* route = hcm.mutable_route_config()->mutable_virtual_hosts(2)->mutable_routes(0); + route->mutable_route() + ->mutable_internal_redirect_policy() + ->mutable_redirect_response_codes() + ->Add(303); + }); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + default_request_headers_.setHost("handle.internal.redirect"); + default_request_headers_.setMethod("HEAD"); + + // First request to original upstream. + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(); + + // Respond with a redirect. + redirect_response_.setStatus(303); + upstream_request_->encodeHeaders(redirect_response_, true); + + // Second request to redirected upstream. + waitForNextUpstreamRequest(); + EXPECT_EQ("", upstream_request_->body().toString()); + ASSERT(upstream_request_->headers().EnvoyOriginalUrl() != nullptr); + EXPECT_EQ("http://handle.internal.redirect/test/long/url", + upstream_request_->headers().getEnvoyOriginalUrlValue()); + EXPECT_EQ("/new/url", upstream_request_->headers().getPathValue()); + EXPECT_EQ("authority2", upstream_request_->headers().getHostValue()); + EXPECT_EQ("via_value", upstream_request_->headers().getViaValue()); + EXPECT_EQ("HEAD", upstream_request_->headers().getMethodValue()); + + // Return the response from the redirect upstream. + upstream_request_->encodeHeaders(default_response_headers_, true); + + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_internal_redirect_succeeded_total") + ->value()); + // 302 was never returned downstream + EXPECT_EQ(0, test_server_->counter("http.config_test.downstream_rq_3xx")->value()); + EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_2xx")->value()); + EXPECT_THAT(waitForAccessLog(access_log_name_, 0), + HasSubstr("303 internal_redirect test-header-value\n")); + // No test header + EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("200 via_upstream -\n")); +} + +TEST_P(RedirectIntegrationTest, InternalRedirectCancelledDueToBufferOverflow) { + useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); + // Validate that header sanitization is only called once. + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { + auto* route = hcm.mutable_route_config()->mutable_virtual_hosts(2)->mutable_routes(0); + route->mutable_per_request_buffer_limit_bytes()->set_value(1024); + }); + + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + default_request_headers_.setHost("handle.internal.redirect"); + default_request_headers_.setMethod("POST"); + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + auto& encoder = encoder_decoder.first; + auto& response = encoder_decoder.second; + + // Send more data than what we can buffer. + std::string data(2048, 'a'); + Buffer::OwnedImpl send1(data); + encoder.encodeData(send1, true); + + // Wait for a redirect response. + waitForNextUpstreamRequest(); + EXPECT_EQ(data, upstream_request_->body().toString()); + upstream_request_->encodeHeaders(redirect_response_, true); + + // Ensure the redirect was returned to the client. + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("302", response->headers().getStatusValue()); +} + +TEST_P(RedirectIntegrationTest, InternalRedirectCancelledDueToEarlyResponse) { + useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); + + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + default_request_headers_.setHost("handle.internal.redirect"); + default_request_headers_.setMethod("POST"); + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + auto& response = encoder_decoder.second; + + // Wait for the request headers to be received upstream. + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + + // Respond with a redirect before the request is complete. + upstream_request_->encodeHeaders(redirect_response_, true); + ASSERT_TRUE(response->waitForEndStream()); + + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + } else { + ASSERT_TRUE(upstream_request_->waitForReset()); + ASSERT_TRUE(fake_upstream_connection_->close()); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + } + + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } else { + codec_client_->close(); + } + + EXPECT_FALSE(upstream_request_->complete()); + + // Ensure the redirect was returned to the client and not handled internally. + EXPECT_TRUE(response->complete()); + EXPECT_EQ("302", response->headers().getStatusValue()); +} + TEST_P(RedirectIntegrationTest, InternalRedirectWithThreeHopLimit) { useAccessLog("%RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); // Validate that header sanitization is only called once. @@ -223,7 +470,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithThreeHopLimit) { response->headers().get(test_header_key_)[0]->value().getStringView()); } -TEST_P(RedirectIntegrationTest, InternalRedirectToDestinationWithBody) { +TEST_P(RedirectIntegrationTest, InternalRedirectToDestinationWithResponseBody) { useAccessLog("%RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); // Validate that header sanitization is only called once. config_helper_.addConfigModifier( diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 56e7f3ff80099..d41a90c9435df 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -106,6 +106,7 @@ class MockFilterManagerCallbacks : public FilterManagerCallbacks { MOCK_METHOD(void, onLocalReply, (Code code)); MOCK_METHOD(Tracing::Config&, tracingConfig, ()); MOCK_METHOD(const ScopeTrackedObject&, scope, ()); + MOCK_METHOD(bool, enableInternalRedirectsWithBody, (), (const)); ResponseHeaderMapPtr continue_headers_; ResponseHeaderMapPtr response_headers_;