Add OgHttp2Session support for SETTINGS ack callbacks and apply to MAX_CONCURRENT_STREAMS.
This CL adds support to OgHttp2Session for applying sent SETTINGS parameters on
receipt of a SETTINGS ack from the peer. This support is in line with behavior
in both nghttp2 [1] and Http2Dispatcher [2].
This CL also uses this newly added support to enforce locally sent
MAX_CONCURRENT_STREAMS on ack. Note that Envoy behavior currently assumes that
peer violations of MAX_CONCURRENT_STREAMS are connection-level [3, 4], so
OgHttp2Session treats violations as a PROTOCOL_ERROR for parity with Envoy.
With this change, Http2FloodMitigationTest.TooManyStreams now passes with
oghttp2, completing flood frame mitigation integration test parity.
Before: http://sponge2/6275a2b6-178b-4ea7-b819-dbd0103564da
After: http://sponge2/463d751a-22df-446f-872c-2a6015c52969
[1] http://google3/third_party/nghttp2/src/lib/nghttp2_session.c;l=7065-7088;rcl=409864218
[2] http://google3/gfe/gfe2/http2/http2_dispatcher.h;l=1185;rcl=410682472
[3] http://google3/third_party/nghttp2/src/lib/nghttp2_session.c;l=3880-3884;rcl=410682472
[4] http://google3/third_party/envoy/src/source/common/http/http2/codec_impl.cc;l=1180,1192;rcl=410881808
PiperOrigin-RevId: 413288939
diff --git a/http2/adapter/http2_util.cc b/http2/adapter/http2_util.cc
index 39b71dc..553c0bd 100644
--- a/http2/adapter/http2_util.cc
+++ b/http2/adapter/http2_util.cc
@@ -95,6 +95,8 @@
return "kWrongFrameSequence";
case ConnectionError::kInvalidPushPromise:
return "InvalidPushPromise";
+ case ConnectionError::kExceededMaxConcurrentStreams:
+ return "ExceededMaxConcurrentStreams";
}
return "UnknownConnectionError";
}
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h
index 08b9ba5..6d843a3 100644
--- a/http2/adapter/http2_visitor_interface.h
+++ b/http2/adapter/http2_visitor_interface.h
@@ -76,6 +76,8 @@
kWrongFrameSequence,
// The peer sent an invalid PUSH_PROMISE frame.
kInvalidPushPromise,
+ // The peer exceeded the max concurrent streams limit.
+ kExceededMaxConcurrentStreams,
};
virtual void OnConnectionError(ConnectionError error) = 0;
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index d406994..3f25f2b 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -3062,6 +3062,161 @@
EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::GOAWAY}));
}
+TEST(NgHttp2AdapterTest, ServerForbidsNewStreamAboveStreamLimit) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+ adapter->SubmitSettings({{MAX_CONCURRENT_STREAMS, 1}});
+
+ const std::string initial_frames =
+ TestFrameSequence().ClientPreface().Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ const int64_t initial_result = adapter->ProcessBytes(initial_frames);
+ EXPECT_EQ(initial_frames.size(), initial_result);
+
+ EXPECT_TRUE(adapter->want_write());
+
+ // Server initial SETTINGS (with MAX_CONCURRENT_STREAMS) and SETTINGS ack.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ int send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS}));
+ visitor.Clear();
+
+ // Let the client send a SETTINGS ack and then attempt to open more than the
+ // advertised number of streams. The overflow stream should be rejected.
+ const std::string stream_frames =
+ TestFrameSequence()
+ .SettingsAck()
+ .Headers(1,
+ {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/true)
+ .Headers(3,
+ {{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/two"}},
+ /*fin=*/true)
+ .Serialize();
+
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0x1));
+ EXPECT_CALL(visitor, OnSettingsAck());
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnEndStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 0x5));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(3, Http2VisitorInterface::InvalidFrameError::kProtocol));
+
+ const int64_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(stream_frames.size(), stream_result);
+
+ // Apparently nghttp2 sends a GOAWAY for this error, even though
+ // OnInvalidFrame() returns true.
+ EXPECT_TRUE(adapter->want_write());
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::GOAWAY}));
+}
+
+TEST(NgHttp2AdapterTest, ServerRstStreamsNewStreamAboveStreamLimitBeforeAck) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+ adapter->SubmitSettings({{MAX_CONCURRENT_STREAMS, 1}});
+
+ const std::string initial_frames =
+ TestFrameSequence().ClientPreface().Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ const int64_t initial_result = adapter->ProcessBytes(initial_frames);
+ EXPECT_EQ(initial_frames.size(), initial_result);
+
+ EXPECT_TRUE(adapter->want_write());
+
+ // Server initial SETTINGS (with MAX_CONCURRENT_STREAMS) and SETTINGS ack.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ int send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS}));
+ visitor.Clear();
+
+ // Let the client avoid sending a SETTINGS ack and attempt to open more than
+ // the advertised number of streams. Apparently nghttp2 still rejects the
+ // overflow stream, albeit with a RST_STREAM by default instead of a GOAWAY.
+ const std::string stream_frames =
+ TestFrameSequence()
+ .Headers(1,
+ {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/true)
+ .Headers(3,
+ {{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/two"}},
+ /*fin=*/true)
+ .Serialize();
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnEndStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 0x5));
+ EXPECT_CALL(visitor,
+ OnInvalidFrame(
+ 3, Http2VisitorInterface::InvalidFrameError::kRefusedStream));
+
+ const int64_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(stream_frames.size(), stream_result);
+
+ // The server sends a RST_STREAM for the offending stream.
+ EXPECT_TRUE(adapter->want_write());
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 3, _, 0x0,
+ static_cast<int>(Http2ErrorCode::REFUSED_STREAM)));
+
+ send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::RST_STREAM}));
+}
+
TEST(NgHttp2AdapterTest, AutomaticSettingsAndPingAcks) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index c3f4356..86d5f72 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -2725,6 +2725,154 @@
spdy::SpdyFrameType::GOAWAY}));
}
+TEST(OgHttp2AdapterServerTest, ServerForbidsNewStreamAboveStreamLimit) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+ adapter->SubmitSettings({{MAX_CONCURRENT_STREAMS, 1}});
+
+ const std::string initial_frames =
+ TestFrameSequence().ClientPreface().Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ const int64_t initial_result = adapter->ProcessBytes(initial_frames);
+ EXPECT_EQ(static_cast<size_t>(initial_result), initial_frames.size());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ // Server initial SETTINGS (with MAX_CONCURRENT_STREAMS) and SETTINGS ack.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ int send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS}));
+ visitor.Clear();
+
+ // Let the client send a SETTINGS ack and then attempt to open more than the
+ // advertised number of streams. The overflow stream should be rejected.
+ const std::string stream_frames =
+ TestFrameSequence()
+ .SettingsAck()
+ .Headers(1,
+ {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/true)
+ .Headers(3,
+ {{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/two"}},
+ /*fin=*/true)
+ .Serialize();
+
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0x1));
+ EXPECT_CALL(visitor, OnSettingsAck());
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnEndStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 0x5));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(3, Http2VisitorInterface::InvalidFrameError::kProtocol));
+
+ const int64_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(static_cast<size_t>(stream_result), stream_frames.size());
+
+ // The server should send a RST_STREAM for the offending stream.
+ EXPECT_TRUE(adapter->want_write());
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 3, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+
+ send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::RST_STREAM}));
+}
+
+TEST(OgHttp2AdapterServerTest, ServerAllowsNewStreamAboveStreamLimitBeforeAck) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+ adapter->SubmitSettings({{MAX_CONCURRENT_STREAMS, 1}});
+
+ const std::string initial_frames =
+ TestFrameSequence().ClientPreface().Serialize();
+
+ testing::InSequence s;
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ const int64_t initial_result = adapter->ProcessBytes(initial_frames);
+ EXPECT_EQ(static_cast<size_t>(initial_result), initial_frames.size());
+
+ EXPECT_TRUE(adapter->want_write());
+
+ // Server initial SETTINGS (with MAX_CONCURRENT_STREAMS) and SETTINGS ack.
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 6, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 6, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0));
+
+ int send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::SETTINGS}));
+ visitor.Clear();
+
+ // Let the client avoid sending a SETTINGS ack and attempt to open more than
+ // the advertised number of streams. The overflow stream should be allowed,
+ // due to the lack of SETTINGS ack.
+ const std::string stream_frames =
+ TestFrameSequence()
+ .Headers(1,
+ {{":method", "GET"},
+ {":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}},
+ /*fin=*/true)
+ .Headers(3,
+ {{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/two"}},
+ /*fin=*/true)
+ .Serialize();
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnEndStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 0x5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(3));
+ EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(4);
+ EXPECT_CALL(visitor, OnEndHeadersForStream(3));
+ EXPECT_CALL(visitor, OnEndStream(3));
+
+ // The server should process the bytes without complaint.
+ const int64_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(static_cast<size_t>(stream_result), stream_frames.size());
+ EXPECT_FALSE(adapter->want_write());
+}
+
} // namespace
} // namespace test
} // namespace adapter
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 16d302a..fb37bdd 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -927,6 +927,12 @@
}
void OgHttp2Session::OnSettingsAck() {
+ if (!settings_ack_callbacks_.empty()) {
+ SettingsAckCallback callback = std::move(settings_ack_callbacks_.front());
+ settings_ack_callbacks_.pop_front();
+ callback();
+ }
+
visitor_.OnSettingsAck();
}
@@ -976,6 +982,23 @@
ConnectionError::kInvalidNewStreamId);
return;
}
+
+ if (stream_map_.size() >= max_inbound_concurrent_streams_) {
+ // The new stream would exceed our advertised MAX_CONCURRENT_STREAMS.
+ // Currently, use PROTOCOL_ERROR for behavior parity in Envoy.
+ // TODO(diannahu): Change to GOAWAY, and add RST_STREAM for exceeding the
+ // pending max inbound concurrent streams value.
+ EnqueueFrame(absl::make_unique<spdy::SpdyRstStreamIR>(
+ stream_id, spdy::ERROR_CODE_PROTOCOL_ERROR));
+ const bool ok = visitor_.OnInvalidFrame(
+ stream_id, Http2VisitorInterface::InvalidFrameError::kProtocol);
+ if (!ok) {
+ LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR,
+ ConnectionError::kExceededMaxConcurrentStreams);
+ }
+ return;
+ }
+
CreateStream(stream_id);
}
}
@@ -1138,6 +1161,17 @@
for (const Http2Setting& setting : settings) {
settings_ir->AddSetting(setting.id, setting.value);
}
+
+ // Copy the (small) map of settings we are about to send so that we can set
+ // values in the SETTINGS ack callback.
+ settings_ack_callbacks_.push_back(
+ [this, settings_map = settings_ir->values()]() {
+ for (const auto id_and_value : settings_map) {
+ if (id_and_value.first == spdy::SETTINGS_MAX_CONCURRENT_STREAMS) {
+ max_inbound_concurrent_streams_ = id_and_value.second;
+ }
+ }
+ });
return settings_ir;
}
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h
index 3126e00..58f34ec 100644
--- a/http2/adapter/oghttp2_session.h
+++ b/http2/adapter/oghttp2_session.h
@@ -2,6 +2,7 @@
#define QUICHE_HTTP2_ADAPTER_OGHTTP2_SESSION_H_
#include <cstdint>
+#include <limits>
#include <list>
#include <memory>
#include <vector>
@@ -259,7 +260,6 @@
std::vector<Http2Setting> GetInitialSettings() const;
// Prepares and returns a SETTINGS frame with the given `settings`.
- // TODO(diannahu): Add the SETTINGS ack callback here.
std::unique_ptr<spdy::SpdySettingsIR> PrepareSettingsFrame(
absl::Span<const Http2Setting> settings);
@@ -362,6 +362,11 @@
using WriteScheduler = PriorityWriteScheduler<Http2StreamId>;
WriteScheduler write_scheduler_;
+ // Stores the queue of callbacks to invoke upon receiving SETTINGS acks. At
+ // most one callback is invoked for each SETTINGS ack.
+ using SettingsAckCallback = std::function<void()>;
+ std::list<SettingsAckCallback> settings_ack_callbacks_;
+
// Delivers header name-value pairs to the visitor.
PassthroughHeadersHandler headers_handler_;
@@ -391,8 +396,15 @@
// The initial flow control receive window size for any newly created streams.
int32_t stream_receive_window_limit_ = kInitialFlowControlWindowSize;
uint32_t max_frame_payload_ = 16384u;
- // The spec encourages a value of at least 100 concurrent streams.
+ // The maximum number of concurrent streams that this connection can open to
+ // its peer and allow from its peer, respectively. Although the initial value
+ // is unlimited, the spec encourages a value of at least 100. We limit
+ // ourselves to opening 100 until told otherwise by the peer and allow an
+ // unlimited number from the peer until updated from SETTINGS we send.
+ // TODO(diannahu): Add a pending/unacked max inbound concurrent streams value.
uint32_t max_outbound_concurrent_streams_ = 100u;
+ uint32_t max_inbound_concurrent_streams_ =
+ std::numeric_limits<uint32_t>::max();
Options options_;
bool received_goaway_ = false;
bool queued_preface_ = false;
