From d1a3652a4c4a22dc20f235711753feb83b5130ad Mon Sep 17 00:00:00 2001 From: Matt Joras Date: Wed, 4 Mar 2020 22:05:03 -0800 Subject: [PATCH] Iterate QuicVersion::MVFST Summary: This iterates the mvfst version to be semantically equivalent to draft-27, and leaves support for the old mvfst version. The client will not yet be moved to draft-27 by default. Reviewed By: lnicco Differential Revision: D20182452 fbshipit-source-id: 1e11ad7296a6cd8d15ca5ed359d9ed82af79bb17 --- quic/QuicConstants.cpp | 1 + quic/QuicConstants.h | 3 ++- quic/client/test/QuicClientTransportTest.cpp | 1 + quic/codec/Types.cpp | 2 ++ quic/common/test/TestUtils.h | 4 ++-- quic/fizz/handshake/FizzCryptoFactory.cpp | 6 +++++- quic/fizz/handshake/FizzTransportParameters.h | 12 ++++++------ .../handshake/test/FizzTransportParametersTest.cpp | 12 ++++++------ quic/server/QuicServer.h | 2 +- quic/server/state/ServerStateMachine.cpp | 2 +- quic/server/state/ServerStateMachine.h | 4 ++-- quic/server/test/QuicServerTransportTest.cpp | 1 + 12 files changed, 30 insertions(+), 20 deletions(-) diff --git a/quic/QuicConstants.cpp b/quic/QuicConstants.cpp index 51966eea2..da36f631d 100644 --- a/quic/QuicConstants.cpp +++ b/quic/QuicConstants.cpp @@ -67,6 +67,7 @@ std::vector filterSupportedVersions( std::back_inserter(filteredVersions), [](auto version) { return version == QuicVersion::MVFST || + version == QuicVersion::MVFST_D24 || version == QuicVersion::QUIC_DRAFT || version == QuicVersion::MVFST_INVALID; }); diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 9ef57c543..315faecd5 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -183,7 +183,8 @@ enum class QuicNodeType : bool { enum class QuicVersion : uint32_t { VERSION_NEGOTIATION = 0x00000000, - MVFST = 0xfaceb001, + MVFST_D24 = 0xfaceb001, + MVFST = 0xfaceb002, QUIC_DRAFT = 0xFF00001b, // Draft-27 MVFST_INVALID = 0xfaceb00f, }; diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index 7626195b7..0e08641c5 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/quic/client/test/QuicClientTransportTest.cpp @@ -1072,6 +1072,7 @@ INSTANTIATE_TEST_CASE_P( QuicClientTransportIntegrationTest, ::testing::Values( TestingParams(QuicVersion::MVFST), + TestingParams(QuicVersion::MVFST_D24), TestingParams(QuicVersion::QUIC_DRAFT), TestingParams(QuicVersion::QUIC_DRAFT, 0))); diff --git a/quic/codec/Types.cpp b/quic/codec/Types.cpp index c7d43ca12..5137ef0b1 100644 --- a/quic/codec/Types.cpp +++ b/quic/codec/Types.cpp @@ -407,6 +407,8 @@ std::string toString(QuicVersion version) { switch (version) { case QuicVersion::VERSION_NEGOTIATION: return "VERSION_NEGOTIATION"; + case QuicVersion::MVFST_D24: + return "MVFST_D24"; case QuicVersion::MVFST: return "MVFST"; case QuicVersion::QUIC_DRAFT: diff --git a/quic/common/test/TestUtils.h b/quic/common/test/TestUtils.h index 43fed45fe..183940b0a 100644 --- a/quic/common/test/TestUtils.h +++ b/quic/common/test/TestUtils.h @@ -38,8 +38,8 @@ class MockClock { } }; -constexpr QuicVersion MVFST1 = static_cast(0xfaceb002); -constexpr QuicVersion MVFST2 = static_cast(0xfaceb003); +constexpr QuicVersion MVFST1 = static_cast(0xfaceb00d); +constexpr QuicVersion MVFST2 = static_cast(0xfaceb00e); constexpr folly::StringPiece kTestHost = "host"; diff --git a/quic/fizz/handshake/FizzCryptoFactory.cpp b/quic/fizz/handshake/FizzCryptoFactory.cpp index d633317e2..979dd431e 100644 --- a/quic/fizz/handshake/FizzCryptoFactory.cpp +++ b/quic/fizz/handshake/FizzCryptoFactory.cpp @@ -23,10 +23,14 @@ Buf FizzCryptoFactory::makeInitialTrafficSecret( auto connIdRange = folly::range(clientDestinationConnId); folly::StringPiece salt; switch (version) { - case QuicVersion::MVFST: + // Our transport version is equivalent to d-24 mostly, but we never + // updated the salt to avoid a version transition. + case QuicVersion::MVFST_D24: salt = kQuicDraft22Salt; break; + // The salt has not changed since d-23. case QuicVersion::QUIC_DRAFT: + case QuicVersion::MVFST: salt = kQuicDraft23Salt; break; default: diff --git a/quic/fizz/handshake/FizzTransportParameters.h b/quic/fizz/handshake/FizzTransportParameters.h index bd7605df8..996d5c685 100644 --- a/quic/fizz/handshake/FizzTransportParameters.h +++ b/quic/fizz/handshake/FizzTransportParameters.h @@ -53,7 +53,7 @@ inline fizz::Extension encodeExtension( fizz::Extension ext; ext.extension_type = fizz::ExtensionType::quic_transport_parameters; ext.extension_data = folly::IOBuf::create(0); - if (encodingVersion != QuicVersion::QUIC_DRAFT) { + if (encodingVersion == QuicVersion::MVFST_D24) { folly::io::Appender appender(ext.extension_data.get(), 40); fizz::detail::writeVector(params.parameters, appender); } else { @@ -69,7 +69,7 @@ inline fizz::Extension encodeExtension( fizz::Extension ext; ext.extension_type = fizz::ExtensionType::quic_transport_parameters; ext.extension_data = folly::IOBuf::create(0); - if (encodingVersion != QuicVersion::QUIC_DRAFT) { + if (encodingVersion == QuicVersion::MVFST_D24) { folly::io::Appender appender(ext.extension_data.get(), 40); fizz::detail::writeVector(params.parameters, appender); } else { @@ -85,7 +85,7 @@ inline fizz::Extension encodeExtension( fizz::Extension ext; ext.extension_type = fizz::ExtensionType::quic_transport_parameters; ext.extension_data = folly::IOBuf::create(0); - if (encodingVersion != QuicVersion::QUIC_DRAFT) { + if (encodingVersion == QuicVersion::MVFST_D24) { folly::io::Appender appender(ext.extension_data.get(), 40); fizz::detail::writeVector(params.parameters, appender); } else { @@ -108,7 +108,7 @@ inline folly::Optional getClientExtension( } quic::ClientTransportParameters parameters; folly::io::Cursor cursor(it->extension_data.get()); - if (encodingVersion != quic::QuicVersion::QUIC_DRAFT) { + if (encodingVersion == quic::QuicVersion::MVFST_D24) { detail::readVector(parameters.parameters, cursor); } else { decodeVarintParams(parameters.parameters, cursor); @@ -125,7 +125,7 @@ inline folly::Optional getServerExtension( } quic::ServerTransportParameters parameters; folly::io::Cursor cursor(it->extension_data.get()); - if (encodingVersion != quic::QuicVersion::QUIC_DRAFT) { + if (encodingVersion == quic::QuicVersion::MVFST_D24) { detail::readVector(parameters.parameters, cursor); } else { decodeVarintParams(parameters.parameters, cursor); @@ -142,7 +142,7 @@ inline folly::Optional getTicketExtension( } quic::TicketTransportParameters parameters; folly::io::Cursor cursor(it->extension_data.get()); - if (encodingVersion != quic::QuicVersion::QUIC_DRAFT) { + if (encodingVersion == quic::QuicVersion::MVFST_D24) { detail::readVector(parameters.parameters, cursor); } else { decodeVarintParams(parameters.parameters, cursor); diff --git a/quic/fizz/handshake/test/FizzTransportParametersTest.cpp b/quic/fizz/handshake/test/FizzTransportParametersTest.cpp index 9848ccfc7..0481c0589 100644 --- a/quic/fizz/handshake/test/FizzTransportParametersTest.cpp +++ b/quic/fizz/handshake/test/FizzTransportParametersTest.cpp @@ -60,7 +60,7 @@ StringPiece ticketParamsD27{"ffa5000604049d7f3e7d"}; TEST_F(QuicExtensionsTest, TestClientParamsD24) { auto exts = getExtensions(clientParamsD24); - auto ext = getClientExtension(exts, QuicVersion::MVFST); + auto ext = getClientExtension(exts, QuicVersion::MVFST_D24); EXPECT_EQ(ext->parameters.size(), 1); EXPECT_EQ( ext->parameters[0].parameter, TransportParameterId::initial_max_data); @@ -68,7 +68,7 @@ TEST_F(QuicExtensionsTest, TestClientParamsD24) { *getIntegerParameter( TransportParameterId::initial_max_data, ext->parameters), 494878333ULL); - checkEncode(std::move(*ext), clientParamsD24, QuicVersion::MVFST); + checkEncode(std::move(*ext), clientParamsD24, QuicVersion::MVFST_D24); } TEST_F(QuicExtensionsTest, TestClientParamsD27) { @@ -86,7 +86,7 @@ TEST_F(QuicExtensionsTest, TestClientParamsD27) { TEST_F(QuicExtensionsTest, TestServerParamsD24) { auto exts = getExtensions(serverParamsD24); - auto ext = getServerExtension(exts, QuicVersion::MVFST); + auto ext = getServerExtension(exts, QuicVersion::MVFST_D24); EXPECT_EQ( ext->parameters[0].parameter, TransportParameterId::initial_max_data); @@ -94,7 +94,7 @@ TEST_F(QuicExtensionsTest, TestServerParamsD24) { *getIntegerParameter( TransportParameterId::initial_max_data, ext->parameters), 494878333ULL); - checkEncode(std::move(*ext), serverParamsD24, QuicVersion::MVFST); + checkEncode(std::move(*ext), serverParamsD24, QuicVersion::MVFST_D24); } TEST_F(QuicExtensionsTest, TestServerParamsD27) { @@ -112,7 +112,7 @@ TEST_F(QuicExtensionsTest, TestServerParamsD27) { TEST_F(QuicExtensionsTest, TestTicketParamsD24) { auto exts = getExtensions(ticketParamsD24); - auto ext = getTicketExtension(exts, QuicVersion::MVFST); + auto ext = getTicketExtension(exts, QuicVersion::MVFST_D24); EXPECT_EQ(ext->parameters.size(), 1); EXPECT_EQ( @@ -121,7 +121,7 @@ TEST_F(QuicExtensionsTest, TestTicketParamsD24) { *getIntegerParameter( TransportParameterId::initial_max_data, ext->parameters), 494878333ULL); - checkEncode(std::move(*ext), ticketParamsD24, QuicVersion::MVFST); + checkEncode(std::move(*ext), ticketParamsD24, QuicVersion::MVFST_D24); } TEST_F(QuicExtensionsTest, TestTicketParamsD27) { diff --git a/quic/server/QuicServer.h b/quic/server/QuicServer.h index 5bba012b8..25a9ce17f 100644 --- a/quic/server/QuicServer.h +++ b/quic/server/QuicServer.h @@ -335,7 +335,7 @@ class QuicServer : public QuicServerWorker::WorkerCallback, const std::vector& evbs); std::vector supportedVersions_{ - {QuicVersion::MVFST, QuicVersion::QUIC_DRAFT}}; + {QuicVersion::MVFST, QuicVersion::MVFST_D24, QuicVersion::QUIC_DRAFT}}; std::atomic shutdown_{true}; std::shared_ptr ctx_; TransportSettings transportSettings_; diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 1e6d8b2f4..738cdda75 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -263,7 +263,7 @@ void updateHandshakeState(QuicServerConnectionState& conn) { auto doneTime = conn.readCodec->getHandshakeDoneTime(); if (!doneTime) { conn.readCodec->onHandshakeDone(Clock::now()); - if (conn.version == QuicVersion::QUIC_DRAFT) { + if (conn.version != QuicVersion::MVFST_D24) { sendSimpleFrame(conn, HandshakeDoneFrame()); } } diff --git a/quic/server/state/ServerStateMachine.h b/quic/server/state/ServerStateMachine.h index 96893ee0a..59650188c 100644 --- a/quic/server/state/ServerStateMachine.h +++ b/quic/server/state/ServerStateMachine.h @@ -128,8 +128,8 @@ struct QuicServerConnectionState : public QuicConnectionStateBase { // TODO: this is wrong, it should be the handshake finish time. But i need // a relatively sane time now to make the timestamps all sane. connectionTime = Clock::now(); - supportedVersions = - std::vector{{QuicVersion::MVFST, QuicVersion::QUIC_DRAFT}}; + supportedVersions = std::vector{ + {QuicVersion::MVFST, QuicVersion::MVFST_D24, QuicVersion::QUIC_DRAFT}}; originalVersion = QuicVersion::MVFST; serverHandshakeLayer = new ServerHandshake(this); handshakeLayer.reset(serverHandshakeLayer); diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 1ca6e9e3c..7fe892dec 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -833,6 +833,7 @@ TEST_F(QuicServerTransportTest, IdleTimerNotResetWhenDataOutstanding) { // Clear the receivedNewPacketBeforeWrite flag, since we may reveice from // client during the SetUp of the test case. server->getNonConstConn().receivedNewPacketBeforeWrite = false; + server->getNonConstConn().outstandingPackets.clear(); StreamId streamId = server->createBidirectionalStream().value(); server->idleTimeout().cancelTimeout();