From 30a70ccfb01f370e8619772cd472cb77f856858c Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Mon, 5 Dec 2022 21:19:53 +0000 Subject: [PATCH 1/8] Add setRequestDecoder to ResponseEncoder interface Signed-off-by: Paul Sohn --- envoy/http/codec.h | 7 +++++++ source/common/http/http1/codec_impl.h | 2 ++ source/common/http/http2/codec_impl.cc | 2 +- source/common/http/http2/codec_impl.h | 5 ++++- source/common/quic/envoy_quic_server_stream.h | 2 +- test/mocks/http/stream_encoder.h | 1 + 6 files changed, 16 insertions(+), 3 deletions(-) diff --git a/envoy/http/codec.h b/envoy/http/codec.h index 99c696964c58..cddccd897b00 100644 --- a/envoy/http/codec.h +++ b/envoy/http/codec.h @@ -45,6 +45,7 @@ const char MaxResponseHeadersCountOverrideKey[] = "envoy.reloadable_features.max_response_headers_count"; class Stream; +class RequestDecoder; /** * Error codes used to convey the reason for a GOAWAY. @@ -165,6 +166,12 @@ class ResponseEncoder : public virtual StreamEncoder { * error. */ virtual bool streamErrorOnInvalidHttpMessage() const PURE; + + /** + * Set a new request decoder for this ResponseEncoder. + * @param decoder new request decoder. + */ + virtual void setRequestDecoder(RequestDecoder& decoder) PURE; }; /** diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index ec2cdc51826a..7a33b71b5a0a 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -156,6 +156,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { return stream_error_on_invalid_http_message_; } + void setRequestDecoder(Http::RequestDecoder& /*decoder*/) override {} + // Http1::StreamEncoderImpl void resetStream(StreamResetReason reason) override; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 812e3d88f211..e03f8e097e71 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -2076,7 +2076,7 @@ Status ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { if (connection_.aboveHighWatermark()) { stream->runHighWatermarkCallbacks(); } - stream->request_decoder_ = &callbacks_.newStream(*stream); + stream->setRequestDecoder(callbacks_.newStream(*stream)); stream->stream_id_ = frame->hd.stream_id; LinkedList::moveIntoList(std::move(stream), active_streams_); adapter_->SetStreamUserData(frame->hd.stream_id, active_streams_.front().get()); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index ed7808c285bf..1176c6058a78 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -497,16 +497,19 @@ class ConnectionImpl : public virtual Connection, void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); } + void setRequestDecoder(Http::RequestDecoder& decoder) override { request_decoder_ = &decoder; } // ScopeTrackedObject void dumpState(std::ostream& os, int indent_level) const override; - RequestDecoder* request_decoder_{}; absl::variant headers_or_trailers_; bool streamErrorOnInvalidHttpMessage() const override { return parent_.stream_error_on_invalid_http_messaging_; } + + private: + RequestDecoder* request_decoder_{}; }; using ServerStreamImplPtr = std::unique_ptr; diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index edd3aa60887c..3a2ae67c316d 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -18,7 +18,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action); - void setRequestDecoder(Http::RequestDecoder& decoder) { request_decoder_ = &decoder; } + void setRequestDecoder(Http::RequestDecoder& decoder) override { request_decoder_ = &decoder; } // Http::StreamEncoder void encode1xxHeaders(const Http::ResponseHeaderMap& headers) override; diff --git a/test/mocks/http/stream_encoder.h b/test/mocks/http/stream_encoder.h index c15da88f170d..efe3626af3ab 100644 --- a/test/mocks/http/stream_encoder.h +++ b/test/mocks/http/stream_encoder.h @@ -48,6 +48,7 @@ class MockResponseEncoder : public ResponseEncoder { MOCK_METHOD(void, encode1xxHeaders, (const ResponseHeaderMap& headers)); MOCK_METHOD(void, encodeHeaders, (const ResponseHeaderMap& headers, bool end_stream)); MOCK_METHOD(void, encodeTrailers, (const ResponseTrailerMap& trailers)); + MOCK_METHOD(void, setRequestDecoder, (RequestDecoder & decoder)); // Http::StreamEncoder MOCK_METHOD(void, encodeData, (Buffer::Instance & data, bool end_stream)); From 351ae732661a5177ce4c4f1c2bca1582070cda5e Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Tue, 6 Dec 2022 16:31:23 +0000 Subject: [PATCH 2/8] In recreateStream, set request decoder for new response encoder Signed-off-by: Paul Sohn --- source/common/http/conn_manager_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f9b3c21de475..bc34fde31940 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1767,6 +1767,7 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( connection_manager_.doEndStream(*this, /*check_for_deferred_close*/ false); RequestDecoder& new_stream = connection_manager_.newStream(*response_encoder, true); + response_encoder->setRequestDecoder(new_stream); // We don't need to copy over the old parent FilterState from the old StreamInfo if it did not // store any objects with a LifeSpan at or above DownstreamRequest. This is to avoid unnecessary // heap allocation. From fed169f4de6fc3e8d8d9a36e175edadfcbc2b538 Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Tue, 6 Dec 2022 17:01:01 +0000 Subject: [PATCH 3/8] add comments Signed-off-by: Paul Sohn --- envoy/http/codec.h | 4 +++- source/common/http/conn_manager_impl.cc | 2 ++ source/common/http/http1/codec_impl.h | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/envoy/http/codec.h b/envoy/http/codec.h index cddccd897b00..236a4da6f250 100644 --- a/envoy/http/codec.h +++ b/envoy/http/codec.h @@ -168,7 +168,9 @@ class ResponseEncoder : public virtual StreamEncoder { virtual bool streamErrorOnInvalidHttpMessage() const PURE; /** - * Set a new request decoder for this ResponseEncoder. + * Set a new request decoder for this ResponseEncoder. This is helpful in the case of an internal + * redirect, in which a new request decoder is created in the context of the same downstream + * request. * @param decoder new request decoder. */ virtual void setRequestDecoder(RequestDecoder& decoder) PURE; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index bc34fde31940..01383e6986ee 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1767,6 +1767,8 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( connection_manager_.doEndStream(*this, /*check_for_deferred_close*/ false); RequestDecoder& new_stream = connection_manager_.newStream(*response_encoder, true); + // This doesn't currently work for HTTP/1 as the H/1 ResponseEncoder doesn't hold the active + // stream's pointer to the RequestDecoder. response_encoder->setRequestDecoder(new_stream); // We don't need to copy over the old parent FilterState from the old StreamInfo if it did not // store any objects with a LifeSpan at or above DownstreamRequest. This is to avoid unnecessary diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 7a33b71b5a0a..fe9e45024ead 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -156,6 +156,7 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { return stream_error_on_invalid_http_message_; } + // For H/1, ResponseEncoder doesn't hold a pointer to RequestDecoder. void setRequestDecoder(Http::RequestDecoder& /*decoder*/) override {} // Http1::StreamEncoderImpl From 41876eed32132edf299f3c90883f04bc9d075602 Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Tue, 6 Dec 2022 17:53:32 +0000 Subject: [PATCH 4/8] rm outdated comment Signed-off-by: Paul Sohn --- source/common/http/conn_manager_impl.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 01383e6986ee..138d218c26c1 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1747,9 +1747,6 @@ void ConnectionManagerImpl::ActiveStream::onRequestDataTooLarge() { void ConnectionManagerImpl::ActiveStream::recreateStream( StreamInfo::FilterStateSharedPtr filter_state) { - // n.b. we do not currently change the codecs to point at the new stream - // decoder because the decoder callbacks are complete. It would be good to - // null out that pointer but should not be necessary. ResponseEncoder* response_encoder = response_encoder_; response_encoder_ = nullptr; From e897a9dd41ba3b7d43e5d6f41b6af7262aa844d8 Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Tue, 6 Dec 2022 18:03:10 +0000 Subject: [PATCH 5/8] add explanatory comment Signed-off-by: Paul Sohn --- source/common/http/conn_manager_impl.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 138d218c26c1..2faa6fa82097 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1764,6 +1764,9 @@ void ConnectionManagerImpl::ActiveStream::recreateStream( connection_manager_.doEndStream(*this, /*check_for_deferred_close*/ false); RequestDecoder& new_stream = connection_manager_.newStream(*response_encoder, true); + // Set the new RequestDecoder on the ResponseEncoder. Even though all of the decoder callbacks + // have already been called at this point, the encoder still needs the new decoder for deferred + // logging in some cases. // This doesn't currently work for HTTP/1 as the H/1 ResponseEncoder doesn't hold the active // stream's pointer to the RequestDecoder. response_encoder->setRequestDecoder(new_stream); From 7b6e5d6419ce4376f7a3586b63f1a2612a71bc74 Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Tue, 6 Dec 2022 18:24:29 +0000 Subject: [PATCH 6/8] mobile client: implement setRequestDecoder Signed-off-by: Paul Sohn --- mobile/library/common/http/client.h | 1 + 1 file changed, 1 insertion(+) diff --git a/mobile/library/common/http/client.h b/mobile/library/common/http/client.h index 68f7eab06750..fc5532c9582e 100644 --- a/mobile/library/common/http/client.h +++ b/mobile/library/common/http/client.h @@ -163,6 +163,7 @@ class Client : public Logger::Loggable { PANIC("not implemented"); } bool streamErrorOnInvalidHttpMessage() const override { return false; } + void setRequestDecoder(RequestDecoder& /*decoder*/) override {}; void encodeMetadata(const MetadataMapVector&) override { PANIC("not implemented"); } From 6af9d70c180d628e3aadc011bbb3f07584e3b21a Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Tue, 6 Dec 2022 18:42:25 +0000 Subject: [PATCH 7/8] fix formatting Signed-off-by: Paul Sohn --- mobile/library/common/http/client.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/library/common/http/client.h b/mobile/library/common/http/client.h index fc5532c9582e..4a2d459df034 100644 --- a/mobile/library/common/http/client.h +++ b/mobile/library/common/http/client.h @@ -163,7 +163,7 @@ class Client : public Logger::Loggable { PANIC("not implemented"); } bool streamErrorOnInvalidHttpMessage() const override { return false; } - void setRequestDecoder(RequestDecoder& /*decoder*/) override {}; + void setRequestDecoder(RequestDecoder& /*decoder*/) override{}; void encodeMetadata(const MetadataMapVector&) override { PANIC("not implemented"); } From e6431119145314eab9fbdd3a35c28b14a24f2127 Mon Sep 17 00:00:00 2001 From: Paul Sohn Date: Tue, 6 Dec 2022 21:57:25 +0000 Subject: [PATCH 8/8] add todo for H/1 Signed-off-by: Paul Sohn --- source/common/http/http1/codec_impl.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index fe9e45024ead..1e1bd36dc131 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -157,6 +157,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { } // For H/1, ResponseEncoder doesn't hold a pointer to RequestDecoder. + // TODO(paulsohn): Enable H/1 codec to get a pointer to the new + // request decoder on recreateStream, here or elsewhere. void setRequestDecoder(Http::RequestDecoder& /*decoder*/) override {} // Http1::StreamEncoderImpl