quiche: implement http stream interfaces by danzh2010 · Pull Request #8556 · envoyproxy/envoy

danzh2010 · 2019-10-09T22:56:32Z

Implement encodeHeader|Data|Trailer.

Implement watermark buffer for QUIC stream and session to limit data buffered in stream send buffer.

Implement readDisable() for QUIC stream to block pushing data when upper stream receive buffer is full.

Risk Level: low, not in use
Testing: added tests in stream test.
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

This reverts commit 3e39fbe. Signed-off-by: Dan Zhang <danzh@google.com>

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-10-15T15:23:47Z

This one is ready for review. PTAL

alyssawilk

Thanks for tackling this? Got partway through and I'll tackle the rest once we've worked out some docs

alyssawilk · 2019-10-15T16:27:10Z

source/extensions/quic_listeners/quiche/codec_impl.cc

+
+void QuicHttpServerConnectionImpl::onUnderlyingConnectionAboveWriteBufferHighWatermark() {
+  for (auto& it : quic_server_session_.stream_map()) {
+    if (!it.second->is_static()) {


I am completely failing to find what is_static checks. can you clue me in?

is_static is true for static streams, for IETF QUIC, it's crypto stream, and for GQuic, also headers stream.

Nowadays, all the streams are managed in one place: QuicSession::stream_map_

So we're willing to infinite buffer on headers stream? We had a recent attack on the H2 stack where when an incoming request could generate an immediate response (like a 400 or 404) the flow control "didn't work" because we had assumed downstream buffers were fed by upstream (backends) not downstream (new bad requests) and adversary could keep feeding bad requests, generating header-only responses and adding more and more to the downstream connection. I want to make sure we don't reintroduce that flaw for HTTP/3

One workaround would be #7619 to have an upper upper bound but that hasn't landed yet. The other was the series of PRs @yanavlasov did to cap outbound frames (search for "flood" in the h2 codec). I think we need one or the other here if headers are otherwise unbounded. TODO is fine.

If we leave this as is for now I'd suggest a comment with the TODO
"we do not yet cap bytes for static streams, like the gQUIC headers stream or the crypto stream" to make it clear what is_static means

QuicHeaderStream always buffer data, its WriteHeaders() doesn't check this threshold. In IETF QUIC, there is no headers stream, and headers will be serialized into stream payload. That would solve this problem, right?

Not having a headers stream helps, but it'd be good to have hard bounds on the crypto stream too.
Again maybe we can have a TODO which admits it will be solved by #1719 but as that's closed and not actively being worked on, I think we need something tracking it as an active problem

If we need a hard bound on crypto stream, it needs to be in QUICHE because the handshake is hidden from envoy.

But I still don't understand how can crypto stream subject to this sort of attack?

Checked QUICHE code, the IETF quic data stream should be robust against this sort of attack because the buffered headers in data stream's send buffer should have prevented stream from being closed still the header is ACK'ed. So the totally data buffered is capped by max allowed header size * max allowed stream number.

For GQuic, because all the headers are sent on header stream and stream is closed once header is buffered in send buffer, the header stream can buffer infinite number of response headers. We need a hard bound for headers stream. But this probably not worth to be added in QUICHE now as QUICHE is moving to IETF quic.

crypto stream also has infinite send buffer, if handshake implementation is wrong, it can also buffer infinitely. I added TODO in EnvoyQuicServerStream::encodeHeaders() about this vulnerability.

alyssawilk · 2019-10-15T16:33:45Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  // This is counting not serialized bytes in the send buffer.
+  uint64_t bytes_to_send_old = BufferedDataBytes();
+  // QUIC stream must take all.
+  WriteBodySlices(quic::QuicMemSliceSpan(quic::QuicMemSliceSpanImpl(data)), end_stream);


I'm a bit confused about how this works.
I thought that WriteBodySlices consumed some amount of data, and returned how many bytes are consumed. Where are the rest going? Are they getting drained from data and then deleted as the transient QuicMemSlice bits go away or OnCanWrite?
I think this could benefit from some doccing up like in source/docs/flow_control.md talking about how things should work, and I can take another pass once we have it written up?

Added docs/quiche_integration.md. PTAL

It seems dangerous to ignore the return value of WriteBodySlices. Can we consider the follow behavior instead:

Use a very large buffered_data_threshold. This threshold has little to do with upstream flow control window etc, it is only used to prevent pathological conditions.

When we WriteBodySlices, we only consider it success if all data is consumed. Otherwise close the connection.

After a successful WriteBodySlices, run the watermark callbacks if needed.

added Reset(QUIC_BAD_APPLICATION_PAYLOAD) and early return.

alyssawilk · 2019-10-15T16:39:29Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  local_end_stream_ = end_stream;
+  // This is counting not serialized bytes in the send buffer.
+  uint64_t bytes_to_send_old = BufferedDataBytes();
+  // QUIC stream must take all.


Are we setting buffered_data_threshold_ high enough we think this is guaranteed?

yes, and that's why I assume WriteBodySlices() always consumes all the data.

So the only relevant thing I find here is
EnvoyQuicStream(2 * GetQuicFlag(FLAGS_quic_buffered_data_threshold) ...which is
EnvoyQuicStream(2 * 8 * 1024
which doesn't look remotely high enough
what am I missing? maybe the docs can call out where it's set since it's not (to me) obvious?

I didn't adjust that flag yet, because I did't have a good sense of how much we want to buffer. I set the flag to be 2 * max flow control window during QuicDispatcher construction now, considering that QUIC will continue encodeData() for up to 1 flow control window of data.

EnvoyQuicStream() here takes in the high watermark actually, it should be less than what the threshold allows to block upper stream before the buffer rejects taking more data. I changed it to be 32k for now.

alyssawilk · 2019-10-15T16:46:20Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  if (bytes_to_send_new > bytes_to_send_old) {
+    // If buffered bytes changed, update stream and session's watermark book
+    // keeping.
+    sendBufferSimulation().checkHighWatermark(bytes_to_send_new);


So we're going to adjust this when we try to encode new data, but do we also adjust it when the QUIC stack drains data from the buffer? Like if pacing is on, will we trigger low watermrks as we ship data? I'm not clear on where this happens.

All the write is driven by OnCanWrite() where we call checkLowWatermark()

alyssawilk · 2019-10-15T16:47:33Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

  // Network::WriteBufferSource
  Network::StreamBuffer getWriteBuffer() override { NOT_REACHED_GCOVR_EXCL_LINE; }

+  void adjustBytesToSend(int64_t delta);


comment please

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

Cool, the docs are awesome! Sent docs fixups offline but here's another round for review.

alyssawilk · 2019-10-16T13:57:36Z

source/extensions/quic_listeners/quiche/codec_impl.cc

+
+void QuicHttpServerConnectionImpl::onUnderlyingConnectionAboveWriteBufferHighWatermark() {
+  for (auto& it : quic_server_session_.stream_map()) {
+    if (!it.second->is_static()) {


So we're willing to infinite buffer on headers stream? We had a recent attack on the H2 stack where when an incoming request could generate an immediate response (like a 400 or 404) the flow control "didn't work" because we had assumed downstream buffers were fed by upstream (backends) not downstream (new bad requests) and adversary could keep feeding bad requests, generating header-only responses and adding more and more to the downstream connection. I want to make sure we don't reintroduce that flaw for HTTP/3

One workaround would be #7619 to have an upper upper bound but that hasn't landed yet. The other was the series of PRs @yanavlasov did to cap outbound frames (search for "flood" in the h2 codec). I think we need one or the other here if headers are otherwise unbounded. TODO is fine.

If we leave this as is for now I'd suggest a comment with the TODO
"we do not yet cap bytes for static streams, like the gQUIC headers stream or the crypto stream" to make it clear what is_static means

alyssawilk · 2019-10-16T14:07:29Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  local_end_stream_ = end_stream;
+  // This is counting not serialized bytes in the send buffer.
+  uint64_t bytes_to_send_old = BufferedDataBytes();
+  // QUIC stream must take all.


So the only relevant thing I find here is
EnvoyQuicStream(2 * GetQuicFlag(FLAGS_quic_buffered_data_threshold) ...which is
EnvoyQuicStream(2 * 8 * 1024
which doesn't look remotely high enough
what am I missing? maybe the docs can call out where it's set since it's not (to me) obvious?

alyssawilk · 2019-10-16T14:13:13Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  ASSERT(data.length() == 0);
+
+  uint64_t bytes_to_send_new = BufferedDataBytes();
+  ASSERT(bytes_to_send_old <= bytes_to_send_new);


sanity check on quic stack functionality - if we'd had buffered data above the high watermark, and QUIC pacing allowed us to write more (OnCanWrite was queued up in the dispatcher loop) and we get data from a backend and send WriteBodySlices, could we immediately write, and so drain, and have fewer bytes to send?

As an aside, it's weird to me we'd have an assert and immediately after we'd have the if. I wonder if we should have a wrapper function checkWatermarks(bytes_to_send_old, bytes_to_send_new)
which simply returns if they're equal, and checks high watermarks if new > old and low if new < old and we can call checkWatermarks in all the places we're manually checking. Thoughts?

sanity check on quic stack functionality - if we'd had buffered data above the high watermark, and QUIC pacing allowed us to write more (OnCanWrite was queued up in the dispatcher loop) and we get data from a backend and send WriteBodySlices, could we immediately write, and so drain, and have fewer bytes to send?

Yes, WriteBodySlices() will still write before OnCanWrite() get called via send_alarm in the queue. And OnCanWrite() might not able to write anymore afterwards as WriteBodySlices() already takes the chance to write.

As an aside, it's weird to me we'd have an assert and immediately after we'd have the if. I wonder if we should have a wrapper function checkWatermarks(bytes_to_send_old, bytes_to_send_new)
which simply returns if they're equal, and checks high watermarks if new > old and low if new < old and we can call checkWatermarks in all the places we're manually checking. Thoughts?

Yes, makes sense.

alyssawilk · 2019-10-16T14:16:12Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+
+  void checkHighWatermark(uint32_t bytes_buffered) {
+    if (high_watermark_ > 0 && !is_above_high_watermark_ && bytes_buffered > high_watermark_) {
+      // Just exceeds high watermark.


exceeds -> exceeded the

alyssawilk · 2019-10-16T14:16:20Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+
+  void checkLowWatermark(uint32_t bytes_buffered) {
+    if (low_watermark_ > 0 && !is_below_low_watermark_ && bytes_buffered < low_watermark_) {
+      // Just cross low watermark.


cross -> crossed the

alyssawilk · 2019-10-16T14:17:02Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+// A class, together with a stand alone buffer, used to achieve the purpose of WatermarkBuffer.
+// Itself doesn't have buffer or do bookeeping of buffered bytes. But provided with buffered_bytes,
+// it re-acts upon crossing high/low watermarks.
+class EnvoyQuicSimulatedWatermarkBuffer {


unit tests for this please

Adding it now.

alyssawilk · 2019-10-16T14:19:35Z

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

+    bool status_changed{false};
+    if (disable) {
+      ++read_disable_counter_;
+      ASSERT(read_disable_counter_ == 1);


The docs (and Envoy flow control) allows readDisable(true) to be called multiple times. I'd think the second time, read_disable_counter would be 2.

Sorry, this assert is added for debugging. Removed.

alyssawilk · 2019-10-16T14:22:25Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

  // TODO(danzh): add interface to quic for connection level buffer throttling.
  // Currently read buffer is capped by connection level flow control. And
-  // write buffer is not capped.
+  // write buffer limit is set during construction. Change buffer limit during


Change -> Changing the
connection -> the connection

wu-bin

Here are some comments so far, I'll continue tomorrow.

wu-bin · 2019-10-21T18:58:22Z

source/common/common/logger.h

  FUNCTION(misc)                 \
  FUNCTION(mongo)                \
  FUNCTION(quic)                 \
+  FUNCTION(quic_stream)          \


Why do we need 'quic_stream'? Looking at the list, most dependent libraries only use one logger id.

EnvoyQuicSimulatedWatermarkBuffer is used in both session and stream, to make the log more clear, I pass in logger instance from its owner. And the logger needs a different id to distinguish each other.

I see. I'm inclined to replace EnvoyQuicSimulatedWatermarkBuffer.logger_ by EnvoyQuicSimulatedWatermarkBuffer.stream_id, use QuicUtils::GetInvalidStreamId() if it's for a session. That way you can log to the 'quic' logger with a stream id in it.

EnvoyQuicSimulatedWatermarkBuffer.stream_id is a good idea. That way I can use ENVOY_STREAM_LOG.

But I'm not sure if moving all the quic related log under 'quic' logger is what we want. Quic session and connections are using 'connection' logger currently in parallel to Network::ConnectionImpl

wu-bin · 2019-10-22T18:55:32Z

source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc

    : quic::QuicServerSessionBase(config, supported_versions, connection.get(), visitor, helper,
                                  crypto_config, compressed_certs_cache),
-      QuicFilterManagerConnectionImpl(std::move(connection), dispatcher) {}
+      QuicFilterManagerConnectionImpl(connection.get(), dispatcher, send_buffer_limit),


As we talked offline, let's try simplify the lifetimes by changing QuicFilterManagerConnectionImpl to a member of EnvoyQuicServerSession, such that the Impl doesn't need to check if quic_connection_ is nullptr. Feel free to TODO it in a future PR.

Defer to #8496 so the refactoring can be tested. TODO added.

wu-bin · 2019-10-22T19:00:57Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

 #include "quiche/quic/core/quic_session.h"
 #include "quiche/spdy/core/spdy_header_block.h"
-
+#include "extensions/quic_listeners/quiche/platform/quic_mem_slice_span_impl.h"


Could we include platform/api/quic_mem_slice_span.h instead of impl?

nope, the weird dependency of QuicMemSlice API is app -> impl -> api

wu-bin · 2019-10-22T19:05:03Z

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

+                        public Http::StreamCallbackHelper,
+                        protected Logger::Loggable<Logger::Id::quic_stream> {
 public:
+  EnvoyQuicStream(uint32_t buffer_limit, std::function<void()> below_low_watermark,


Please add a comment for |buffer_limit|.

wu-bin · 2019-10-22T19:29:59Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  // This is counting not serialized bytes in the send buffer.
+  uint64_t bytes_to_send_old = BufferedDataBytes();
+  // QUIC stream must take all.
+  WriteBodySlices(quic::QuicMemSliceSpan(quic::QuicMemSliceSpanImpl(data)), end_stream);


It seems dangerous to ignore the return value of WriteBodySlices. Can we consider the follow behavior instead:

Use a very large buffered_data_threshold. This threshold has little to do with upstream flow control window etc, it is only used to prevent pathological conditions.

When we WriteBodySlices, we only consider it success if all data is consumed. Otherwise close the connection.

After a successful WriteBodySlices, run the watermark callbacks if needed.

wu-bin · 2019-10-22T19:32:40Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

-void EnvoyQuicServerStream::resetStream(Http::StreamResetReason /*reason*/) {
-  NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
+void EnvoyQuicServerStream::resetStream(Http::StreamResetReason reason) {
+  // Higher layers expect calling resetStream() to immediately raise reset callbacks.


Higher layers => Upper layers

wu-bin · 2019-10-22T19:39:25Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc


-void EnvoyQuicServerStream::readDisable(bool /*disable*/) { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
+void EnvoyQuicServerStream::switchStreamBlockState(bool should_block) {
+  ASSERT(FinishedReadingHeaders(),


It certainly sounds unlikely, but if we can handle block before finishing headers, we should not ASSERT, a warning log seems not appropriate to me.
Also, can we add some information to the error message? Like the upstream buffer limit and bytes read for headers.

stream blocking and unblocking can only happen to request body. decodeHeaders() doesn't have buffer limit. The ASSERT here is just for debugging purpose.

wu-bin · 2019-10-22T19:48:40Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  if (should_block) {
+    sequencer()->SetBlockedUntilFlush();
+  } else {
+    ASSERT(read_disable_counter_ == 0, "readDisable called in btw.");


What does 'btw' mean?

btw->between

Got it, I thought it is 'by the way'.

wu-bin

I'm still reviewing it. Here are the comments before quic_filter_manager_connection_impl.cc.

wu-bin · 2019-10-23T13:45:58Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  bool empty_payload_with_fin = buffer->length() == 0 && finished_reading;
+  if (!empty_payload_with_fin || !end_stream_decoded_) {
+    ASSERT(decoder() != nullptr);
+    decoder()->decodeData(*buffer, finished_reading);


What happens if empty_payload_with_fin==false and end_stream_decoded_==true?

That's impossible. If fin is already delivered, no new data should arrive.

Sure. Consider add an assert for that so we can catch it when the assumption is violated.

I changed the logic here to avoid decoding empty payload with end of stream in case of trailer. ASSERT doesn't apply any more.

wu-bin · 2019-10-23T14:30:34Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+    in_decode_data_callstack_ = false;
+    if (read_disable_counter_ > 0) {
+      // If readDisable() was ever called during decodeData() and it meant to disable
+      // reading from downstream, the call must have been deferred. Call it now.


Q: Can you elaborate (in this comment) on why the switch call needs to be deferred?

Commented in EnvoyQuicStream::readDisable() when deferring switch call.

wu-bin · 2019-10-23T14:45:00Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  if (BufferedDataBytes() > 0) {
+    // If the stream is closed without sending out all bufferred data, regard
+    // them as sent now and adjust connection buffer book keeping.
+    dynamic_cast<QuicFilterManagerConnectionImpl*>(session())->adjustBytesToSend(


nit: add a filter_manager_connection_impl() function to return the QuicFilterManagerConnectionImpl*. (Feel free to choose a different function name)

wu-bin · 2019-10-23T14:47:19Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h

+  // EnvoyQuicStream
+  void switchStreamBlockState(bool should_block) override;
+  uint32_t streamId() override;
+  Network::Connection* connection() override;


nit: How about network_connection(), to distinguish from quic connection.

This is for ENVOY_CONN_LOG which assumes passed in object has connection() interface.

wu-bin · 2019-10-23T14:52:40Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+
+// A class, together with a stand alone buffer, used to achieve the purpose of WatermarkBuffer.
+// Itself doesn't have buffer or do bookeeping of buffered bytes. But provided with buffered_bytes,
+// it re-acts upon crossing high/low watermarks.


nit: Document the (no-op) behavior when both high and low watermarks are 0.

wu-bin · 2019-10-23T14:57:43Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+
+  void checkHighWatermark(uint32_t bytes_buffered) {
+    if (high_watermark_ > 0 && !is_above_high_watermark_ && bytes_buffered > high_watermark_) {
+      // This is the first time for the buffer to cross the high watermark


nit: How about: // Transitioning from below to above high watermark.

Similar for checkLowWatermark.

wu-bin · 2019-10-23T15:01:53Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+  bool is_below_low_watermark_{true};
+  uint32_t high_watermark_{0};
+  bool is_above_high_watermark_{false};


Seems like is_below_low_watermark_ and is_above_high_watermark_ can be replaced by a single last_exceeded_watermark_ which is a tri-state: {NONE, LOW_WATERMARK, HIGH_WATERMARK}. Your call.

The NONE state seems not necessary. I simplified to two state indicated by a bool.

wu-bin · 2019-10-23T15:59:28Z

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

+    } else {
+      send_buffer_simulation_.checkLowWatermark(buffered_data_new);
+    }
+    connection.adjustBytesToSend(buffered_data_new - buffered_data_old);


Can we add some code to handle integer overflow/unsigned wraparound? (buffered_data_new - buffered_data_old may not be representable as a int64_t)

I don't think it's possible to over overflow or underflow as the delta is capped by flow control and congestion control.

SG. (I agree it's extremely unlikely, I just feel that we can still handle it gracefully even if it happens.)

wu-bin · 2019-10-23T16:00:11Z

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

    return decoder_;
  }

+  // True once end of stream is propergated to Envoy. Envoy doesn't expect to be


propergated => propagated

wu-bin · 2019-10-23T16:01:53Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

 }

+void QuicFilterManagerConnectionImpl::adjustBytesToSend(int64_t delta) {
+  bytes_to_send_ += delta;


bytes_to_send_ is uint32_t but delta is int64. Can we make bytes_to_send_ 64 bit?

bytes_to_send_ is used to compare with buffer limits which are all uint32_t. delta is converted from size_t which is very unlikely to overflow given QUIC's flow control and congestion control window.

wu-bin · 2019-10-23T17:28:16Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

      // of trying to map it into a 64-bit space.
-      stream_info_(dispatcher.timeSource()), id_(quic_connection_->connection_id().Hash()) {
-  // TODO(danzh): Use QUIC specific enum value.
+      stream_info_(dispatcher.timeSource()), id_(quic_connection_->connection_id().Hash()),


Q: How is id_ used? What happens if there is a hash collision?

Used by Network::Connection::id() which is just for logging and stats as far as I observed.

SG. Please add a comment for id_ to prevent it from being used as a connection id in the future.

wu-bin · 2019-10-23T17:53:35Z

test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc

+// Derive to have simpler priority mechanism.
+class TestEnvoyQuicServerSession : public EnvoyQuicServerSession {
+public:
+  TestEnvoyQuicServerSession(const quic::QuicConfig& config,


nit: You can do using EnvoyQuicServerSession::EnvoyQuicServerSession; to "inherit" constructors. Similar for TestQuicCryptoServerStream.

wu-bin · 2019-10-23T18:01:44Z

test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc

+      quic::ENCRYPTION_FORWARD_SECURE,
+      std::make_unique<quic::NullEncrypter>(quic::Perspective::IS_SERVER));
+  // Drive congestion control manually.
+  auto send_algorithm = new testing::StrictMock<quic::test::MockSendAlgorithm>;


nit: Consider a NiceMock and only do EXPECT_CALL only the interesting calls.(CanSend, GetCongestionWindow, ...)

wu-bin · 2019-10-23T18:14:03Z

test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc

+                  decoded_headers->Method()->value().getStringView());
+      }));
+  EXPECT_CALL(request_decoder, decodeData(_, true))
+      .Times(testing::AtMost(1))


Since end_stream is already true in decodeHeaders, can we make sure decodeData is not called?

Please change to .Times(0) for decodeData, or just remove the EXPECT_CALL.

wu-bin · 2019-10-23T19:40:42Z

test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc

+          EXPECT_EQ(Http::Headers::get().MethodValues.Get,
+                    headers->Method()->value().getStringView());
+        }));
+    if (quic_version_.transport_version == quic::QUIC_VERSION_99) {


How about if (VersionUsesHttp3(quic_version_.transport_version))?

QUICHE deps is behind, switch to VersionUsesQpack for now. And it should be updated when next QUICHE update.

wu-bin · 2019-10-23T19:40:59Z

test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc

+          }
+        }));
+    std::string data = payload;
+    if (quic_version_.transport_version == quic::QUIC_VERSION_99) {


wu-bin · 2019-10-23T19:53:01Z

test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc

+    if (quic_version_.transport_version == quic::QUIC_VERSION_99) {
+      std::unique_ptr<char[]> data_buffer;
+      quic::HttpEncoder encoder;
+      quic::QuicByteCount data_frame_header_length =
+          encoder.SerializeDataFrameHeader(payload.length(), &data_buffer);
+      quic::QuicStringPiece data_frame_header(data_buffer.get(), data_frame_header_length);
+      data = absl::StrCat(data_frame_header, payload);
+    }
+    quic::QuicStreamFrame frame(stream_id_, fin, 0, data);


This pattern is used multiple times in the PR, consider wrap it in a function

quic::QuicStreamFrame MakeQuicStreamFrame(version, stream_id, fin, offset, payload) { std::string frame_data = VersionUsesHttp3(version) ? frame_header + payload : payload; return quic::QuicStreamFrame(stream_id, fin, offset, frame_data); }

QuicStreamFrame keeps a string view, so it can't be returned. I moved the string manipulating logic into a helper function.

wu-bin · 2019-10-23T20:18:45Z

test/extensions/quic_listeners/quiche/test_utils.h

+namespace Envoy {
+namespace Quic {
+
+class MockEnvoyQuicSession : public quic::QuicSpdySession, public QuicFilterManagerConnectionImpl {


Q: if MockEnvoyQuicSession inherits MockQuicSpdySession, can we remove the MOCK_METHODs?

We don't want to inherit from MockQuicSpdySession because it uses quic::test::DefaultQuicConfig() which sets flow control window to very large value.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-10-28T14:58:30Z

ping?

wu-bin · 2019-10-28T15:45:19Z

test/extensions/quic_listeners/quiche/test_utils.h

+namespace Envoy {
+namespace Quic {
+
+class MockEnvoyQuicSession : public quic::QuicSpdySession, public QuicFilterManagerConnectionImpl {


wu-bin · 2019-10-28T15:46:42Z

test/extensions/quic_listeners/quiche/envoy_quic_server_session_test.cc

+                  decoded_headers->Method()->value().getStringView());
+      }));
+  EXPECT_CALL(request_decoder, decodeData(_, true))
+      .Times(testing::AtMost(1))


Please change to .Times(0) for decodeData, or just remove the EXPECT_CALL.

wu-bin · 2019-10-28T16:09:10Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

 }

+void QuicFilterManagerConnectionImpl::adjustBytesToSend(int64_t delta) {
+  bytes_to_send_ += delta;


wu-bin · 2019-10-28T16:12:09Z

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

+    } else {
+      send_buffer_simulation_.checkLowWatermark(buffered_data_new);
+    }
+    connection.adjustBytesToSend(buffered_data_new - buffered_data_old);


SG. (I agree it's extremely unlikely, I just feel that we can still handle it gracefully even if it happens.)

wu-bin · 2019-10-28T16:14:22Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+
+  // True after the buffer goes below low watermark and hasn't come up above high
+  // watermark yet, even though the buffered data might be between high and low
+  // watermarks.


Consider add a comment for the behavior when neither high nor low watermarks have been crossed.

This is the beginning state of the buffer. Comment added.

wu-bin · 2019-10-28T16:14:47Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h

+  // EnvoyQuicStream
+  void switchStreamBlockState(bool should_block) override;
+  uint32_t streamId() override;
+  Network::Connection* connection() override;


wu-bin · 2019-10-28T16:15:54Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc


-void EnvoyQuicServerStream::readDisable(bool /*disable*/) { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
+void EnvoyQuicServerStream::switchStreamBlockState(bool should_block) {
+  ASSERT(FinishedReadingHeaders(),


Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Super awesome to see this coming together. Just flushing out a few small nits/questions from a skim through. Generally LGTM and will defer to @alyssawilk @wu-bin on the detailed review.

/wait

mattklein123 · 2019-10-29T16:42:36Z

docs/quiche_integration.md

@@ -0,0 +1,33 @@
+### Overview


nit: can we move to https://github.com/envoyproxy/envoy/tree/master/source/docs? This is where we currently have our dev docs. The top level docs directory is for user docs.

mattklein123 · 2019-10-29T16:47:30Z

source/extensions/quic_listeners/quiche/codec_impl.cc

+  for (auto& it : quic_server_session_.stream_map()) {
+    if (!it.second->is_static()) {
+      ENVOY_LOG(debug, "runHighWatermarkCallbacks on stream {}", it.first);
+      dynamic_cast<EnvoyQuicServerStream*>(it.second.get())->runHighWatermarkCallbacks();


Why do we need the dynamic cast here and below? Can the stored stream just derive from the right interface?

stream_map() is a QUICHE interface, for sure it doesn't know about Envoy stream interface.

I see so this map is storing an opaque pointer of some type? Could it eventually just store a common interface that our classes derive from? Can this be a TODO here and below?

QuicSession::stream_map() store all QuicStream objects. Our envoy quic stream also derives from that class and from EnvoyQuicStream as well. And the latter has runHighWatermarkCallbacks() interface. So we need a cross cast here I believe.

I'm happy with the TODO there. Can we have some comment the first time we reference is_static just explaining it's crypto and gQUIC headers?

mattklein123 · 2019-10-29T16:49:04Z

source/extensions/quic_listeners/quiche/envoy_quic_dispatcher.cc

+  // flow control window of upstream. The per-stream high watermark should be
+  // smaller than max flow control window to make sure upper stream can be flow
+  // control blocked early enough not to send more than the threshold allows.
+  SetQuicFlag(FLAGS_quic_buffered_data_threshold, 2 * 16 * 1024 * 1024); // 32MB


This seems pretty huge compared to what we typically recommend for HTTP/2 at the edge. Will we eventually make this configurable?

This threshold needs to be large enough to hold all the data from upstream even after readDisable(). So it should be larger than HTTP2 per stream flow control window whose negotiated value is not accessible at this level. I will add a TODO to plumb through to get that value.

But before that, for QUICHE to work correctly, we need this threshold to be large enough what each encodeData() can consume all the data passed in. This doesn't mean that we are actually buffering that much data because upstream's flow control window probably is not that large after negotiation.

mattklein123 · 2019-10-29T16:51:05Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+          // the stream to buffer all the data.
+          // Ideally this limit should also corelate to peer's receive window
+          // but not fully depends on that.
+          16 * 1024, [this]() { runLowWatermarkCallbacks(); },


See my comment above. IMO for QUIC we should start with good edge defaults, since this is really practically only going to be used at the edge. So yeah 256MiB default is way too large. Can we just use different defaults for QUIC and get them right now? If we want to TODO this for discussion during a production readiness pass that is fine with me.

mattklein123 · 2019-10-29T16:52:20Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+Network::Connection* EnvoyQuicServerStream::connection() { return filterManagerConnection(); }
+
+QuicFilterManagerConnectionImpl* EnvoyQuicServerStream::filterManagerConnection() {
+  return dynamic_cast<QuicFilterManagerConnectionImpl*>(session());


Same question about the dynamic cast?

See my previous comment.

mattklein123 · 2019-10-29T16:53:30Z

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

+  // notified more than once about end of stream. So once this is true, no need
+  // to set it in the callback to Envoy stream any more.
+  bool end_stream_decoded_{false};
+  int32_t read_disable_counter_{0};


nit: unsigned

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-10-30T15:42:58Z

@alyssawilk Comments and TODOs about buffer size are added. PTAL!

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Thanks, LGTM modulo format fix and some nits. Will defer to @alyssawilk @wu-bin for further review.

@danzh2010 can you please open an explicit issue for tuning/hardening QUIC buffer sizes before we call the MVP ready? I want to make sure we are tracking that. Thank you!

mattklein123 · 2019-10-30T19:50:33Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc


 void EnvoyQuicServerStream::encodeHeaders(const Http::HeaderMap& headers, bool end_stream) {
  ENVOY_STREAM_LOG(debug, "encodeHeaders (end_stream={}) {}.", *this, end_stream, headers);
+  // QUICHE guarantees to take all the headers. This could cause inifit data to


typo "inifit"

mattklein123 · 2019-10-30T19:52:37Z

source/extensions/quic_listeners/quiche/codec_impl.cc

+  for (auto& it : quic_server_session_.stream_map()) {
+    if (!it.second->is_static()) {
+      ENVOY_LOG(debug, "runHighWatermarkCallbacks on stream {}", it.first);
+      dynamic_cast<EnvoyQuicServerStream*>(it.second.get())->runHighWatermarkCallbacks();


I see so this map is storing an opaque pointer of some type? Could it eventually just store a common interface that our classes derive from? Can this be a TODO here and below?

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010

#8826 is added for tracking stream send buffer size concerns.

danzh2010 · 2019-10-31T15:23:15Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc


 void EnvoyQuicServerStream::encodeHeaders(const Http::HeaderMap& headers, bool end_stream) {
  ENVOY_STREAM_LOG(debug, "encodeHeaders (end_stream={}) {}.", *this, end_stream, headers);
+  // QUICHE guarantees to take all the headers. This could cause inifit data to


danzh2010 · 2019-10-31T15:47:19Z

source/extensions/quic_listeners/quiche/codec_impl.cc

+  for (auto& it : quic_server_session_.stream_map()) {
+    if (!it.second->is_static()) {
+      ENVOY_LOG(debug, "runHighWatermarkCallbacks on stream {}", it.first);
+      dynamic_cast<EnvoyQuicServerStream*>(it.second.get())->runHighWatermarkCallbacks();


QuicSession::stream_map() store all QuicStream objects. Our envoy quic stream also derives from that class and from EnvoyQuicStream as well. And the latter has runHighWatermarkCallbacks() interface. So we need a cross cast here I believe.

danzh2010 · 2019-10-31T18:13:52Z

@alyssawilk mind take another look?

alyssawilk

Haha, I was already halfway through! It's looking great - only a few nits left on my end

alyssawilk · 2019-10-31T20:13:20Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+  runResetCallbacks(reason);
+  if (local_end_stream_ && !reading_stopped()) {
+    // This is after 200 early response. Reset with QUIC_STREAM_NO_ERROR instead
+    // of propagating original reset reason.


Can we get a bit more detail here? It isn't obvious how this relates to 200 early response, or how StopReading() translates into Reset(QUIC_STREAM_NO_ERROR)

alyssawilk · 2019-10-31T20:26:52Z

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

+          // the stream to buffer all the data.
+          // Ideally this limit should also corelate to peer's receive window
+          // but not fully depends on that.
+          16 * 1024, [this]() { runLowWatermarkCallbacks(); },


Yeah, I thought this one was tied to the upstream limit, i.e. if we set it low we'd end up advertising to the peer we were willing to buffer data when we wouldn't locally. Sounds like I had the wrong field, they can be decoupled and we can start with smaller limits, which SGTM.

alyssawilk · 2019-10-31T20:31:00Z

source/extensions/quic_listeners/quiche/codec_impl.cc

+  for (auto& it : quic_server_session_.stream_map()) {
+    if (!it.second->is_static()) {
+      ENVOY_LOG(debug, "runHighWatermarkCallbacks on stream {}", it.first);
+      dynamic_cast<EnvoyQuicServerStream*>(it.second.get())->runHighWatermarkCallbacks();


I'm happy with the TODO there. Can we have some comment the first time we reference is_static just explaining it's crypto and gQUIC headers?

alyssawilk · 2019-10-31T20:32:04Z

source/extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h

+
+// A class, together with a stand alone buffer, used to achieve the purpose of WatermarkBuffer.
+// Itself doesn't have buffer or bookkeep buffered bytes. But provided with buffered_bytes,
+// it re-acts upon crossing high/low watermarks.


rea-acts -> reacts

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-11-04T17:48:45Z

Haha, I was already halfway through! It's looking great - only a few nits left on my end

Commented about QUIC_STREAM_NO_ERROR and static_stream. PTAL!

alyssawilk

Wohoo!

danzh1989 added 12 commits October 4, 2019 14:51

merged all commits

edfbf4b

Signed-off-by: Dan Zhang <danzh@google.com>

fix mem slice warning

f574a58

Signed-off-by: Dan Zhang <danzh@google.com>

remove broken include

5cc1622

Signed-off-by: Dan Zhang <danzh@google.com>

cryptoConnect

469dc2a

Signed-off-by: Dan Zhang <danzh@google.com>

fix xfcc test

68b6501

Signed-off-by: Dan Zhang <danzh@google.com>

remove stream libs

3a42546

Signed-off-by: Dan Zhang <danzh@google.com>

fix asan

c0ede9a

Signed-off-by: Dan Zhang <danzh@google.com>

revert non-HCM change

3e39fbe

Signed-off-by: Dan Zhang <danzh@google.com>

Revert "revert non-HCM change"

d7c610b

This reverts commit 3e39fbe. Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into quicintegration

013701b

Signed-off-by: Dan Zhang <danzh@google.com>

revert HCM, client and integration test

95eb51d

Signed-off-by: Dan Zhang <danzh@google.com>

remove unrelated change

e040f72

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners October 9, 2019 22:56

danzh2010 changed the title ~~Quicencode~~ quiche: implement http stream interfaces Oct 9, 2019

alyssawilk self-assigned this Oct 10, 2019

danzh1989 added 7 commits October 10, 2019 11:49

fix asan

29128aa

Signed-off-by: Dan Zhang <danzh@google.com>

typo

31340f2

Signed-off-by: Dan Zhang <danzh@google.com>

add test for readDisable

88ebd06

Signed-off-by: Dan Zhang <danzh@google.com>

rename var

8a2740d

Signed-off-by: Dan Zhang <danzh@google.com>

typo

e43a1fe

Signed-off-by: Dan Zhang <danzh@google.com>

bufferLimit

438a8d7

Signed-off-by: Dan Zhang <danzh@google.com>

typo

4056e85

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk reviewed Oct 15, 2019

View reviewed changes

alyssawilk added the waiting label Oct 15, 2019

add more

b6b10ac

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Oct 15, 2019

alyssawilk reviewed Oct 16, 2019

View reviewed changes

alyssawilk added the waiting label Oct 16, 2019

wu-bin reviewed Oct 22, 2019

View reviewed changes

wu-bin reviewed Oct 23, 2019

View reviewed changes

wu-bin suggested changes Oct 23, 2019

View reviewed changes

danzh1989 added 4 commits October 23, 2019 19:11

improve comment

90a6953

Signed-off-by: Dan Zhang <danzh@google.com>

code cleanup in test

bddb942

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into quicencode

3cad65e

Signed-off-by: Dan Zhang <danzh@google.com>

fix quiche_copts

81e2fa9

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin previously approved these changes Oct 28, 2019

View reviewed changes

danzh1989 added 2 commits October 28, 2019 13:27

remove AtMost EXPECT_CALL

9b72210

Signed-off-by: Dan Zhang <danzh@google.com>

comment in watermark buffer

6c03488

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed wu-bin’s stale review via 6c03488 October 28, 2019 18:50

mattklein123 requested changes Oct 29, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 29, 2019

comment about buffer limit

86c8ec1

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Oct 29, 2019

fix namespace

a23de7f

Signed-off-by: Dan Zhang <danzh@google.com>

format

6f33c26

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 previously approved these changes Oct 30, 2019

View reviewed changes

add tracking issue

e1e9fa4

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed mattklein123’s stale review via e1e9fa4 October 31, 2019 16:36

danzh2010 commented Oct 31, 2019

View reviewed changes

alyssawilk reviewed Oct 31, 2019

View reviewed changes

comments

2d7f6ac

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk approved these changes Nov 5, 2019

View reviewed changes

alyssawilk merged commit b589cf6 into envoyproxy:master Nov 5, 2019

Conversation

danzh2010 commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Oct 15, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Oct 9, 2019 •

edited

Loading

danzh2010 Oct 16, 2019 •

edited

Loading

danzh2010 Oct 16, 2019 •

edited

Loading

danzh2010 Oct 15, 2019 •

edited

Loading