From 062ba893bc6e5c8108afe2e473d0caa6d04ad3c0 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Tue, 23 Mar 2021 11:59:29 -0700 Subject: [PATCH 01/27] inital working implementation of internal redirects with request body Signed-off-by: Derek Argueta --- source/common/http/conn_manager_impl.cc | 12 ++++- source/common/http/filter_manager.cc | 7 --- source/common/http/filter_manager.h | 2 + source/common/router/router.cc | 9 ++-- test/common/router/router_test.cc | 24 +--------- test/integration/redirect_integration_test.cc | 45 ++++++++++++++++++- 6 files changed, 62 insertions(+), 37 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 4b01a5da8498d..ca7d35e66fbd9 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1614,6 +1614,13 @@ 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 = buffered_request_data != nullptr && buffered_request_data->length() > 0; + if (proxy_body) { + request_data->move(*filter_manager_.bufferedRequestData()); + } + response_encoder->getStream().removeCallbacks(*this); // This functionally deletes the stream (via deferred delete) so do not // reference anything beyond this point. @@ -1632,7 +1639,10 @@ 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) { + new_stream.decodeData(*request_data, true); + } } Http1StreamEncoderOptionsOptRef ConnectionManagerImpl::ActiveStream::http1StreamEncoderOptions() { diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 345d24f56f26b..7133c2e84e1f6 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1372,13 +1372,6 @@ void ActiveStreamDecoderFilter::setDecoderBufferLimit(uint32_t limit) { uint32_t ActiveStreamDecoderFilter::decoderBufferLimit() { return parent_.buffer_limit_; } 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) { - return false; - } - parent_.stream_info_.setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().InternalRedirect); diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index e72e800041d02..9b8ec3c90c429 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -901,6 +901,8 @@ class FilterManager : public ScopeTrackedObject, uint64_t streamId() const { return stream_id_; } + Buffer::InstancePtr& bufferedRequestData() { return buffered_request_data_; } + 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..1cc4073cbd742 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -687,7 +687,8 @@ 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() || + (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 @@ -1475,7 +1476,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 @@ -1485,9 +1486,7 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers, upstream_request.upstreamTiming().last_upstream_rx_byte_received_ && downstream_end_stream_; // Redirects are not supported for streaming requests yet. - if (downstream_end_stream_ && - !callbacks_->decodingBuffer() && // Redirects with body not yet supported. - location != nullptr && + if (downstream_end_stream_ && location != nullptr && convertRequestHeadersForInternalRedirect(*downstream_headers_, *location) && callbacks_->recreateStream(&headers)) { cluster_->stats().upstream_internal_redirect_succeeded_total_.inc(); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index bb45248a36ba4..a6bedb40e1478 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; @@ -4290,23 +4290,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { .value()); } -TEST_F(RouterTest, InternalRedirectRejectedWithBody) { - 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, CrossSchemeRedirectRejectedByPolicy) { enableRedirects(); @@ -4314,7 +4297,6 @@ TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { 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); @@ -4333,7 +4315,6 @@ TEST_F(RouterTest, InternalRedirectRejectedByPredicate) { 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 +4341,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 +4365,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 +4384,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 d837d2e2b2f11..5fbd825ce7f25 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -145,6 +145,49 @@ TEST_P(RedirectIntegrationTest, BasicInternalRedirect) { codec_client_->makeHeaderOnlyRequest(default_request_headers_); waitForNextUpstreamRequest(); + + upstream_request_->encodeHeaders(redirect_response_, true); + + waitForNextUpstreamRequest(); + 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()); + + upstream_request_->encodeHeaders(default_response_headers_, 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, 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"); + IntegrationStreamDecoderPtr response = + codec_client_->makeRequestWithBody(default_request_headers_, "foobarbizbaz"); + + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(redirect_response_, true); waitForNextUpstreamRequest(); @@ -223,7 +266,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( From 9c78a33aee8d6b9177030b121ab84a3711615485 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Tue, 23 Mar 2021 14:21:03 -0700 Subject: [PATCH 02/27] docs updates Signed-off-by: Derek Argueta --- .../arch_overview/http/http_connection_management.rst | 11 +++++------ docs/root/version_history/current.rst | 1 + 2 files changed, 6 insertions(+), 6 deletions(-) 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 74e8d90b99e82..337dc33aef76f 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -64,7 +64,7 @@ can be used to modify this behavior, and they fall into two categories: Hosts are marked by setting ``canary: true`` for the ``envoy.lb`` filter in the endpoint's filter metadata. See :ref:`LbEndpoint ` for more details. - * *envoy.retry_host_predicates.omit_host_metadata*: This will reject any host based on predefined metadata match criteria. + * *envoy.retry_host_predicates.omit_host_metadata*: This will reject any host based on predefined metadata match criteria. See the configuration example below for more details. * :ref:`Priority Predicates`: These predicates can @@ -169,15 +169,14 @@ 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. -5. :ref:`allow_cross_scheme_redirect +4. :ref:`allow_cross_scheme_redirect ` is true (default to false), or the scheme of the downstream request and the *location* header are the same. -6. The number of previously handled internal redirect within a given downstream request does not +5. The number of previously handled internal redirect within a given downstream request does not exceed :ref:`max internal redirects ` of the route that the request or redirected request is hitting. -7. All :ref:`predicates ` accept +6. All :ref:`predicates ` accept the target route. Any failure will result in redirect being passed downstream instead. @@ -201,7 +200,7 @@ Specifically, the *allow listed routes* predicate defines edges of individual no and the *previous routes* predicate defines "visited" state of the edges, so that loop can be avoided if so desired. -A third predicate :ref:`safe_cross_scheme +A third predicate :ref:`safe_cross_scheme ` can be used to prevent HTTP -> HTTPS redirect. diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0e9b565afbb12..1635e745a0877 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -55,6 +55,7 @@ Minor Behavior Changes * http: upstream flood and abuse checks increment the count of opened HTTP/2 streams when Envoy sends initial HEADERS frame for the new stream. Before the counter was incrementred when Envoy received response HEADERS frame with the END_HEADERS flag set from upstream server. +* http: internal redirects for requests with body data will now be handled. * oauth filter: added the optional parameter :ref:`auth_scopes ` with default value of 'user' if not provided. Enables this value to be overridden in the Authorization request to the OAuth provider. * perf: allow reading more bytes per operation from raw sockets to improve performance. * router: extended custom date formatting to DOWNSTREAM_PEER_CERT_V_START and DOWNSTREAM_PEER_CERT_V_END when using :ref:`custom request/response header formats `. From 121897eaf8836eb1e29468c1ae198c3cebe650fe Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 12:08:55 -0700 Subject: [PATCH 03/27] add runtime guard for internal redirects with request body Signed-off-by: Derek Argueta --- docs/root/version_history/current.rst | 2 +- source/common/http/conn_manager_impl.cc | 13 +++++++++---- source/common/http/conn_manager_impl.h | 5 +++++ source/common/http/filter_manager.cc | 7 +++++++ source/common/http/filter_manager.h | 9 +++++++++ source/common/router/router.cc | 11 ++++++++--- source/common/runtime/runtime_features.cc | 2 ++ test/integration/redirect_integration_test.cc | 9 ++++++++- 8 files changed, 49 insertions(+), 9 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 1635e745a0877..d435562ced757 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -55,7 +55,7 @@ Minor Behavior Changes * http: upstream flood and abuse checks increment the count of opened HTTP/2 streams when Envoy sends initial HEADERS frame for the new stream. Before the counter was incrementred when Envoy received response HEADERS frame with the END_HEADERS flag set from upstream server. -* http: internal redirects for requests with body data will now be handled. +* http: add runtime flag to enable internal redirects for requests with body data. * oauth filter: added the optional parameter :ref:`auth_scopes ` with default value of 'user' if not provided. Enables this value to be overridden in the Authorization request to the OAuth provider. * perf: allow reading more bytes per operation from raw sockets to improve performance. * router: extended custom date formatting to DOWNSTREAM_PEER_CERT_V_START and DOWNSTREAM_PEER_CERT_V_END when using :ref:`custom request/response header formats `. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index ca7d35e66fbd9..a0c780abb6929 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( @@ -1615,10 +1617,13 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( response_encoder_ = nullptr; Buffer::InstancePtr request_data = std::make_unique(); - const auto& buffered_request_data = filter_manager_.bufferedRequestData(); - const bool proxy_body = buffered_request_data != nullptr && buffered_request_data->length() > 0; + bool proxy_body = connection_manager_.enable_internal_redirects_with_body_; if (proxy_body) { - request_data->move(*filter_manager_.bufferedRequestData()); + const auto& buffered_request_data = filter_manager_.bufferedRequestData(); + proxy_body = buffered_request_data != nullptr && buffered_request_data->length() > 0; + if (proxy_body) { + request_data->move(*filter_manager_.bufferedRequestData()); + } } response_encoder->getStream().removeCallbacks(*this); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index b745bc05ca56d..3545bca0e5117 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -277,6 +277,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 @@ -442,6 +446,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 7133c2e84e1f6..cb9f61b4704a4 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1372,6 +1372,13 @@ void ActiveStreamDecoderFilter::setDecoderBufferLimit(uint32_t limit) { uint32_t ActiveStreamDecoderFilter::decoderBufferLimit() { return parent_.buffer_limit_; } 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 (!parent_.enableInternalRedirectsWithBody() && (!complete() || parent_.stream_info_.bytesReceived() != 0)) { + return false; + } + parent_.stream_info_.setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().InternalRedirect); diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 9b8ec3c90c429..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; }; /** @@ -903,6 +908,10 @@ class FilterManager : public ScopeTrackedObject, 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 1cc4073cbd742..55a607dfab401 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -687,8 +687,10 @@ 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() || - (route_entry_ && route_entry_->internalRedirectPolicy().enabled()); + bool buffering = + (retry_state_ && retry_state_->enabled()) || !active_shadow_policies_.empty() || + (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body") && + 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 @@ -1486,7 +1488,10 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers, upstream_request.upstreamTiming().last_upstream_rx_byte_received_ && downstream_end_stream_; // Redirects are not supported for streaming requests yet. - if (downstream_end_stream_ && location != nullptr && + if (downstream_end_stream_ && + (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body") || + !callbacks_->decodingBuffer()) && + location != nullptr && convertRequestHeadersForInternalRedirect(*downstream_headers_, *location) && callbacks_->recreateStream(&headers)) { cluster_->stats().upstream_internal_redirect_succeeded_total_.inc(); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 1503e70f21953..faa9006a75f98 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -118,6 +118,8 @@ constexpr const char* disabled_runtime_features[] = { "envoy.reloadable_features.remove_legacy_json", // Sentinel and test flag. "envoy.reloadable_features.test_feature_false", + // TODO(derekargueta) flip to true after a release cycle. + "envoy.reloadable_features.internal_redirects_with_body", }; RuntimeFeatures::RuntimeFeatures() { diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index 5fbd825ce7f25..49352e754caef 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -178,18 +178,24 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) { hcm.set_via("via_value"); }); + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.internal_redirects_with_body", "true"); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); default_request_headers_.setHost("handle.internal.redirect"); + default_request_headers_.setMethod("POST"); + + // First request to original upstream. IntegrationStreamDecoderPtr response = codec_client_->makeRequestWithBody(default_request_headers_, "foobarbizbaz"); - waitForNextUpstreamRequest(); + // Respond with a redirect. upstream_request_->encodeHeaders(redirect_response_, true); + // Second request to redirected upstream. waitForNextUpstreamRequest(); ASSERT(upstream_request_->headers().EnvoyOriginalUrl() != nullptr); EXPECT_EQ("http://handle.internal.redirect/test/long/url", @@ -198,6 +204,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { 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); response->waitForEndStream(); From 7ff59b20b00095b708e6705c4b052252f0add9c6 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 12:14:55 -0700 Subject: [PATCH 04/27] ensure that full body was read in integration test Signed-off-by: Derek Argueta --- test/integration/redirect_integration_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index 49352e754caef..fd935cac89972 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -186,17 +186,20 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { 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_, "foobarbizbaz"); + 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()); From be591abcf02b32f38d7df875458b7124dcbad7c4 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 12:23:11 -0700 Subject: [PATCH 05/27] handle request buffer overflows for internal redirects Signed-off-by: Derek Argueta --- source/common/router/router.cc | 4 +- source/common/router/router.h | 4 +- test/integration/redirect_integration_test.cc | 43 ++++++++++++++++++- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 55a607dfab401..224a8b453d172 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -698,6 +698,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 @@ -1489,7 +1490,8 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers, // Redirects are not supported for streaming requests yet. if (downstream_end_stream_ && - (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body") || + ((Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body") && + !request_buffer_overflowed_) || !callbacks_->decodingBuffer()) && location != nullptr && convertRequestHeadersForInternalRedirect(*downstream_headers_, *location) && diff --git a/source/common/router/router.h b/source/common/router/router.h index 28d287c217b57..4f8fb6d730039 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; @@ -539,6 +540,7 @@ 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; uint32_t attempt_count_{1}; uint32_t pending_retries_{0}; diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index fd935cac89972..652e8adf23d4b 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -178,8 +178,8 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) { hcm.set_via("via_value"); }); - config_helper_.addRuntimeOverride( - "envoy.reloadable_features.internal_redirects_with_body", "true"); + config_helper_.addRuntimeOverride("envoy.reloadable_features.internal_redirects_with_body", + "true"); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -224,6 +224,45 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { 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) { + hcm.set_via("via_value"); + + auto* route = hcm.mutable_route_config()->mutable_virtual_hosts(2)->mutable_routes(0); + route->mutable_per_request_buffer_limit_bytes()->set_value(1024); + }); + config_helper_.addRuntimeOverride("envoy.reloadable_features.internal_redirects_with_body", + "true"); + 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. + response->waitForEndStream(); + ASSERT_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. From faac7a87d71fb230c1757b1062b6b79cf105c60a Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 14:13:17 -0700 Subject: [PATCH 06/27] update docs with reuqirements for internal redirects Signed-off-by: Derek Argueta --- .../arch_overview/http/http_connection_management.rst | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 337dc33aef76f..28bfa4e1a15c8 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -169,14 +169,17 @@ 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. :ref:`allow_cross_scheme_redirect +4. The request must not have a body unless the `envoy.reloadable_features.internal_redirects_with_body` runtime guard + is enabled, in which case the request must be smaller than the :ref:`per_request_buffer_limit_bytes + ` setting. +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. -5. The number of previously handled internal redirect within a given downstream request does not +6. The number of previously handled internal redirect within a given downstream request does not exceed :ref:`max internal redirects ` of the route that the request or redirected request is hitting. -6. All :ref:`predicates ` accept +7. All :ref:`predicates ` accept the target route. Any failure will result in redirect being passed downstream instead. From fca3fb1b22c969406126e684d1b1c4491c104830 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 16:19:52 -0700 Subject: [PATCH 07/27] update router unit tests Signed-off-by: Derek Argueta --- test/common/router/router_test.cc | 57 +++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index cbffa325c41e2..611cc9564d18d 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4290,6 +4290,58 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { .value()); } +TEST_F(RouterTest, InternalRedirectRejectedWithBody) { + 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, InternalRedirectAcceptedWithBodyAndRuntimeGuard) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_redirects_with_body", "true"}}); + + enableRedirects(); + + sendRequest(false); + + EXPECT_CALL(callbacks_.dispatcher_, createTimer_); + + Buffer::InstancePtr body_data(new Buffer::OwnedImpl("random_fake_data")); + 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, true); + 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(); @@ -4297,6 +4349,7 @@ TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { 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); @@ -4315,6 +4368,7 @@ TEST_F(RouterTest, InternalRedirectRejectedByPredicate) { 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}))); @@ -4341,6 +4395,7 @@ 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); @@ -4365,6 +4420,7 @@ 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); @@ -4384,6 +4440,7 @@ 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)); From 954b790a7581bcd508fdc40e9727220de2300cc0 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 16:40:24 -0700 Subject: [PATCH 08/27] fix format Signed-off-by: Derek Argueta --- source/common/http/filter_manager.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index dc1dd322f0945..a01b4a86b7eab 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 (!parent_.enableInternalRedirectsWithBody() && (!complete() || parent_.stream_info_.bytesReceived() != 0)) { + if (!parent_.enableInternalRedirectsWithBody() && + (!complete() || parent_.stream_info_.bytesReceived() != 0)) { return false; } From 30aad9e975da04c99ed3f798af0461d20f13b6b1 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 17:09:45 -0700 Subject: [PATCH 09/27] learned that current.rst is alphabetically sorted Signed-off-by: Derek Argueta --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index cfbf226125ce2..0d20d0534812d 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: add runtime flag to enable internal redirects for requests with body data. * 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 @@ -56,7 +57,6 @@ Minor Behavior Changes * http: upstream flood and abuse checks increment the count of opened HTTP/2 streams when Envoy sends initial HEADERS frame for the new stream. Before the counter was incrementred when Envoy received response HEADERS frame with the END_HEADERS flag set from upstream server. -* http: add runtime flag to enable internal redirects for requests with body data. * oauth filter: added the optional parameter :ref:`auth_scopes ` with default value of 'user' if not provided. Enables this value to be overridden in the Authorization request to the OAuth provider. * perf: allow reading more bytes per operation from raw sockets to improve performance. * router: extended custom date formatting to DOWNSTREAM_PEER_CERT_V_START and DOWNSTREAM_PEER_CERT_V_END when using :ref:`custom request/response header formats `. From 25c766a53a7f5f368a6559c5a75de7409b265bff Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 26 Mar 2021 18:37:08 -0700 Subject: [PATCH 10/27] add missing mock Signed-off-by: Derek Argueta --- test/mocks/http/mocks.h | 1 + 1 file changed, 1 insertion(+) 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_; From 877f90e1660cf244020922a7ed9f429f6b7311c7 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Mon, 29 Mar 2021 19:32:05 -0700 Subject: [PATCH 11/27] review feedback Signed-off-by: Derek Argueta --- .../arch_overview/http/http_connection_management.rst | 5 ++--- docs/root/version_history/current.rst | 2 +- source/common/http/conn_manager_impl.cc | 3 +++ source/common/runtime/runtime_features.cc | 3 +-- test/common/router/router_test.cc | 8 ++++---- test/integration/redirect_integration_test.cc | 5 +---- 6 files changed, 12 insertions(+), 14 deletions(-) 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 28bfa4e1a15c8..e11127d5f5325 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -169,9 +169,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 unless the `envoy.reloadable_features.internal_redirects_with_body` runtime guard - is enabled, in which case the request must be smaller than the :ref:`per_request_buffer_limit_bytes - ` setting. +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 0d20d0534812d..22a430afcd321 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -47,7 +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: add runtime flag to enable internal redirects for requests with body data. +* 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 a0c780abb6929..34ff079ee2ae3 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1646,6 +1646,9 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( 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); } } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index faa9006a75f98..1cf02433d7fda 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", @@ -118,8 +119,6 @@ constexpr const char* disabled_runtime_features[] = { "envoy.reloadable_features.remove_legacy_json", // Sentinel and test flag. "envoy.reloadable_features.test_feature_false", - // TODO(derekargueta) flip to true after a release cycle. - "envoy.reloadable_features.internal_redirects_with_body", }; RuntimeFeatures::RuntimeFeatures() { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 611cc9564d18d..9b9c11b2ab47d 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4291,6 +4291,10 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { } TEST_F(RouterTest, InternalRedirectRejectedWithBody) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.internal_redirects_with_body", "false"}}); + enableRedirects(); sendRequest(); @@ -4308,10 +4312,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithBody) { } TEST_F(RouterTest, InternalRedirectAcceptedWithBodyAndRuntimeGuard) { - TestScopedRuntime scoped_runtime; - Runtime::LoaderSingleton::getExisting()->mergeValues( - {{"envoy.reloadable_features.internal_redirects_with_body", "true"}}); - enableRedirects(); sendRequest(false); diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index 652e8adf23d4b..f722a1218d9d9 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -178,8 +178,6 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { config_helper_.addConfigModifier( [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) { hcm.set_via("via_value"); }); - config_helper_.addRuntimeOverride("envoy.reloadable_features.internal_redirects_with_body", - "true"); initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -235,8 +233,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectCancelledDueToBufferOverflow) { auto* route = hcm.mutable_route_config()->mutable_virtual_hosts(2)->mutable_routes(0); route->mutable_per_request_buffer_limit_bytes()->set_value(1024); }); - config_helper_.addRuntimeOverride("envoy.reloadable_features.internal_redirects_with_body", - "true"); + initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); From 9a99cef772fe27f7d09d42ef40989c9b7e1cc01e Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Tue, 30 Mar 2021 12:59:20 -0700 Subject: [PATCH 12/27] fix tests Signed-off-by: Derek Argueta --- test/common/router/router_test.cc | 9 +--- test/integration/redirect_integration_test.cc | 44 ++++++++++++++++++- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 9b9c11b2ab47d..ef1fd1d764079 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4290,7 +4290,7 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { .value()); } -TEST_F(RouterTest, InternalRedirectRejectedWithBody) { +TEST_F(RouterTest, InternalRedirectRejectedWithRequestBodyDisabled) { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( {{"envoy.reloadable_features.internal_redirects_with_body", "false"}}); @@ -4311,7 +4311,7 @@ TEST_F(RouterTest, InternalRedirectRejectedWithBody) { .value()); } -TEST_F(RouterTest, InternalRedirectAcceptedWithBodyAndRuntimeGuard) { +TEST_F(RouterTest, InternalRedirectAcceptedWithRequestBody) { enableRedirects(); sendRequest(false); @@ -4349,7 +4349,6 @@ TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { 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); @@ -4368,7 +4367,6 @@ TEST_F(RouterTest, InternalRedirectRejectedByPredicate) { 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}))); @@ -4395,7 +4393,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); @@ -4420,7 +4417,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); @@ -4440,7 +4436,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 f722a1218d9d9..7ac9fd0ccb516 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -228,8 +228,6 @@ TEST_P(RedirectIntegrationTest, InternalRedirectCancelledDueToBufferOverflow) { 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_per_request_buffer_limit_bytes()->set_value(1024); }); @@ -260,6 +258,48 @@ TEST_P(RedirectIntegrationTest, InternalRedirectCancelledDueToBufferOverflow) { 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); + 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. From d35327c8946387fc41616e7a35377ad3da1a50ee Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Tue, 30 Mar 2021 18:10:19 -0700 Subject: [PATCH 13/27] docs note about trailers Signed-off-by: Derek Argueta --- .../intro/arch_overview/http/http_connection_management.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 e11127d5f5325..c6990ad825bb7 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. From c8bfad111f68a2ff02404e71bdb2cab27fdacac6 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Tue, 30 Mar 2021 20:26:28 -0700 Subject: [PATCH 14/27] fix formatting Signed-off-by: Derek Argueta --- source/common/http/conn_manager_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 34ff079ee2ae3..45e55f9c8bfd6 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1646,9 +1646,9 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( 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. + // 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); } } From 920b32cc03aa4801a2c93c64d1c7d94d89736625 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 31 Mar 2021 09:33:16 -0700 Subject: [PATCH 15/27] attempt ASAN fix Signed-off-by: Derek Argueta --- test/common/router/router_test.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index ef1fd1d764079..b157329b54c50 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -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(); @@ -4296,7 +4294,6 @@ TEST_F(RouterTest, InternalRedirectRejectedWithRequestBodyDisabled) { {{"envoy.reloadable_features.internal_redirects_with_body", "false"}}); enableRedirects(); - sendRequest(); Buffer::InstancePtr body_data(new Buffer::OwnedImpl("random_fake_data")); @@ -4313,12 +4310,12 @@ TEST_F(RouterTest, InternalRedirectRejectedWithRequestBodyDisabled) { 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()); @@ -4344,7 +4341,6 @@ TEST_F(RouterTest, InternalRedirectAcceptedWithRequestBody) { TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { enableRedirects(); - sendRequest(); redirect_headers_->setLocation("https://www.foo.com"); @@ -4360,7 +4356,6 @@ TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { TEST_F(RouterTest, InternalRedirectRejectedByPredicate) { enableRedirects(); - sendRequest(); redirect_headers_->setLocation("http://www.foo.com/some/path"); From 47ff9bf61f90aea509e588ed4fbedec5d0e12cd3 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 31 Mar 2021 09:42:01 -0700 Subject: [PATCH 16/27] more sure fix for ASAN Signed-off-by: Derek Argueta --- test/common/router/router_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index b157329b54c50..c919970d69e8c 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4323,7 +4323,7 @@ TEST_F(RouterTest, InternalRedirectAcceptedWithRequestBody) { response_decoder_->decodeHeaders(std::move(redirect_headers_), false); Buffer::OwnedImpl response_data("1234567890"); - response_decoder_->decodeData(response_data, true); + response_decoder_->decodeData(response_data, false); EXPECT_EQ(0U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ .counter("upstream_internal_redirect_failed_total") .value()); From e454bbcfc267ffb3a3232543f995399f56cd197e Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 31 Mar 2021 10:09:31 -0700 Subject: [PATCH 17/27] rename test to include Legacy suffix Signed-off-by: Derek Argueta --- test/common/router/router_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index c919970d69e8c..94033e7721c74 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4288,7 +4288,7 @@ TEST_F(RouterTest, InternalRedirectRejectedWithoutLocation) { .value()); } -TEST_F(RouterTest, InternalRedirectRejectedWithRequestBodyDisabled) { +TEST_F(RouterTest, InternalRedirectRejectedWithRequestBodyDisabledLegacy) { TestScopedRuntime scoped_runtime; Runtime::LoaderSingleton::getExisting()->mergeValues( {{"envoy.reloadable_features.internal_redirects_with_body", "false"}}); From 1e0bcf24bbef8eb87474d4f3dc428754b8c45806 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 31 Mar 2021 13:26:07 -0700 Subject: [PATCH 18/27] fix logic for FilterManager early exit in recreateStream Signed-off-by: Derek Argueta --- source/common/http/filter_manager.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index a01b4a86b7eab..71f357ff63faa 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1374,8 +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 (!parent_.enableInternalRedirectsWithBody() && - (!complete() || parent_.stream_info_.bytesReceived() != 0)) { + if (!complete() || + (!parent_.enableInternalRedirectsWithBody() && parent_.stream_info_.bytesReceived() != 0)) { return false; } From cf21cce48363db85e66c263a28707b6d489de733 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 2 Apr 2021 18:04:36 -0700 Subject: [PATCH 19/27] support HTTP 303 (tests pending) Signed-off-by: Derek Argueta --- source/common/router/router.cc | 13 +++++++++++-- source/common/router/router.h | 3 ++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 224a8b453d172..6770bcdea13af 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1488,13 +1488,15 @@ 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_ && ((Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body") && !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; @@ -1508,7 +1510,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; @@ -1587,6 +1590,12 @@ 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.setMethod(Http::Headers::get().MethodValues.Get); + 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 4f8fb6d730039..f6810717d32a0 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -496,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(); From a887d4955a87f035b7532895b6de42ee1f6a7fb6 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Fri, 2 Apr 2021 19:11:29 -0700 Subject: [PATCH 20/27] add integration test for HTTP 303 Signed-off-by: Derek Argueta --- test/integration/redirect_integration_test.cc | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index 7ac9fd0ccb516..f4ab89c877d4b 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -222,6 +222,65 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { 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")); + + 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. + 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()); + + // Return the response from the redirect upstream. + upstream_request_->encodeHeaders(default_response_headers_, 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. From 6aae34360300cc27ec97a5b926e2199894439f33 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Sat, 3 Apr 2021 06:31:41 -0700 Subject: [PATCH 21/27] add documentation note on HTTP 303 handling Signed-off-by: Derek Argueta --- .../intro/arch_overview/http/http_connection_management.rst | 4 ++++ 1 file changed, 4 insertions(+) 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 c6990ad825bb7..c669c82a8f1c8 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -164,6 +164,10 @@ 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, it will dispatch the redirect with a +bodiless HTTP GET regardless of 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 From 64f3ac9d47fe349b395eef0e00a09560cd53aa8d Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Mon, 5 Apr 2021 11:23:10 -0700 Subject: [PATCH 22/27] preserve HTTP HEAD, check content-length on HTTP 303 redirects Signed-off-by: Derek Argueta --- .../http/http_connection_management.rst | 7 ++- source/common/router/router.cc | 4 +- test/integration/redirect_integration_test.cc | 62 +++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) 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 c669c82a8f1c8..575da12520c8e 100644 --- a/docs/root/intro/arch_overview/http/http_connection_management.rst +++ b/docs/root/intro/arch_overview/http/http_connection_management.rst @@ -164,9 +164,10 @@ 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, it will dispatch the redirect with a -bodiless HTTP GET regardless of the original HTTP method. For more information, see `RFC 7231 -Section 6.4.4 `_. +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: diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 6770bcdea13af..87e94c07089a9 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1591,7 +1591,9 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do } // See https://tools.ietf.org/html/rfc7231#section-6.4.4. - if (status_code == enumToInt(Http::Code::SeeOther)) { + 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); callbacks_->modifyDecodingBuffer([](Buffer::Instance& data) { data.drain(data.length()); }); } diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index f4ab89c877d4b..cfba0de050d3d 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -267,6 +267,68 @@ TEST_P(RedirectIntegrationTest, InternalRedirectHandlesHttp303) { // Return the response from the redirect upstream. upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2 && downstream_protocol_ == Http::CodecClient::Type::HTTP2) { + EXPECT_EQ(nullptr, response->headers().ContentLength()); + } else { + EXPECT_EQ("0", response->headers().getContentLengthValue()); + } + 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); + response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); From ce75e217c6dab3a5fda2754d140a578d8cfacf63 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Mon, 5 Apr 2021 11:26:55 -0700 Subject: [PATCH 23/27] format redirect_integration_test.cc Signed-off-by: Derek Argueta --- test/integration/redirect_integration_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index cfba0de050d3d..7b0f32a995d6f 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -270,7 +270,8 @@ TEST_P(RedirectIntegrationTest, InternalRedirectHandlesHttp303) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2 && downstream_protocol_ == Http::CodecClient::Type::HTTP2) { + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2 && + downstream_protocol_ == Http::CodecClient::Type::HTTP2) { EXPECT_EQ(nullptr, response->headers().ContentLength()); } else { EXPECT_EQ("0", response->headers().getContentLengthValue()); From 1578b2338778f1ee8dfb46b243dd48a9dc57fb58 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Mon, 5 Apr 2021 11:55:01 -0700 Subject: [PATCH 24/27] fix validation of content-length Signed-off-by: Derek Argueta --- source/common/router/router.cc | 1 + test/integration/redirect_integration_test.cc | 10 +++------- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 87e94c07089a9..84424171ce247 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1595,6 +1595,7 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do 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()); }); } diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index 7b0f32a995d6f..a1b48b7f7a567 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -239,9 +239,10 @@ TEST_P(RedirectIntegrationTest, InternalRedirectHandlesHttp303) { codec_client_ = makeHttpConnection(lookupPort("http")); + const std::string& request_body = "foobarbizbaz"; default_request_headers_.setHost("handle.internal.redirect"); default_request_headers_.setMethod("POST"); - const std::string& request_body = "foobarbizbaz"; + default_request_headers_.setContentLength(request_body.length()); // First request to original upstream. IntegrationStreamDecoderPtr response = @@ -263,6 +264,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectHandlesHttp303) { 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); @@ -270,12 +272,6 @@ TEST_P(RedirectIntegrationTest, InternalRedirectHandlesHttp303) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2 && - downstream_protocol_ == Http::CodecClient::Type::HTTP2) { - EXPECT_EQ(nullptr, response->headers().ContentLength()); - } else { - EXPECT_EQ("0", response->headers().getContentLengthValue()); - } EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_internal_redirect_succeeded_total") ->value()); // 302 was never returned downstream From 014fbc53c938a79fdf39f8223dcf7e63c9214603 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 7 Apr 2021 08:27:17 -0700 Subject: [PATCH 25/27] review feedback: collapse if-statements and snap runtime value to filter Signed-off-by: Derek Argueta --- source/common/http/conn_manager_impl.cc | 10 ++++------ source/common/router/router.cc | 13 +++++++------ source/common/router/router.h | 1 + 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 6ea6591ce3a58..f3ad68a649efa 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1613,13 +1613,11 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( response_encoder_ = nullptr; Buffer::InstancePtr request_data = std::make_unique(); - bool proxy_body = connection_manager_.enable_internal_redirects_with_body_; + 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) { - const auto& buffered_request_data = filter_manager_.bufferedRequestData(); - proxy_body = buffered_request_data != nullptr && buffered_request_data->length() > 0; - if (proxy_body) { - request_data->move(*filter_manager_.bufferedRequestData()); - } + request_data->move(*filter_manager_.bufferedRequestData()); } response_encoder->getStream().removeCallbacks(*this); diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 84424171ce247..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,10 +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() || - (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body") && - route_entry_ && route_entry_->internalRedirectPolicy().enabled()); + 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 @@ -1492,8 +1494,7 @@ bool Filter::setupRedirect(const Http::ResponseHeaderMap& headers, // Redirects are not supported for streaming requests yet. if (downstream_end_stream_ && - ((Runtime::runtimeFeatureEnabled("envoy.reloadable_features.internal_redirects_with_body") && - !request_buffer_overflowed_) || + ((internal_redirects_with_body_enabled_ && !request_buffer_overflowed_) || !callbacks_->decodingBuffer()) && location != nullptr && convertRequestHeadersForInternalRedirect(*downstream_headers_, *location, status_code) && diff --git a/source/common/router/router.h b/source/common/router/router.h index f6810717d32a0..e0ba5d4bae993 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -542,6 +542,7 @@ class Filter : Logger::Loggable, 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}; From 13cdac01d6eb752e5219527d08ab99f65f15c7f0 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 7 Apr 2021 12:32:37 -0700 Subject: [PATCH 26/27] add ASSERT_TRUE for waitForEndStream() as done in d6acb14 Signed-off-by: Derek Argueta --- test/integration/redirect_integration_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index 5ccc037df7daa..4fac5f20765dc 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -208,7 +208,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectWithRequestBody) { // Return the response from the redirect upstream. upstream_request_->encodeHeaders(default_response_headers_, true); - response->waitForEndStream(); + 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") @@ -269,7 +269,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectHandlesHttp303) { // Return the response from the redirect upstream. upstream_request_->encodeHeaders(default_response_headers_, true); - response->waitForEndStream(); + 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") @@ -326,7 +326,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectHttp303PreservesHeadMethod) { // Return the response from the redirect upstream. upstream_request_->encodeHeaders(default_response_headers_, true); - response->waitForEndStream(); + 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") @@ -371,7 +371,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectCancelledDueToBufferOverflow) { upstream_request_->encodeHeaders(redirect_response_, true); // Ensure the redirect was returned to the client. - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("302", response->headers().getStatusValue()); } @@ -395,7 +395,7 @@ TEST_P(RedirectIntegrationTest, InternalRedirectCancelledDueToEarlyResponse) { // Respond with a redirect before the request is complete. upstream_request_->encodeHeaders(redirect_response_, true); - response->waitForEndStream(); + ASSERT_TRUE(response->waitForEndStream()); if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); From 721938fc9a8b6ef37c1792a6e1c8b2981b063544 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Wed, 7 Apr 2021 13:24:11 -0700 Subject: [PATCH 27/27] re-use buffered_request_data variable instead of calling filter_manager_.bufferedRequestData() a second time Signed-off-by: Derek Argueta --- source/common/http/conn_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f3ad68a649efa..790e164737879 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1617,7 +1617,7 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( 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(*filter_manager_.bufferedRequestData()); + request_data->move(*buffered_request_data); } response_encoder->getStream().removeCallbacks(*this);