Deprecate flag --gfe2_reloadable_flag_quic_connection_migration_use_new_cid_v2. Make IETF compliant connection migration behavior depend on QUIC version instead of the reloadable flag. Removed QuicConnection::connection_migration_use_new_cid(). To QUICHE mergers, if it is called anywhere outside of QUICHE, replace it with version check, i.e. version_.HasIetfQuicFrames(), and remove the unreachable code if its call site is only reachable under IETF version today. PiperOrigin-RevId: 538855777
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc index 8260997..7196df1 100644 --- a/quiche/quic/core/http/end_to_end_test.cc +++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -2899,7 +2899,7 @@ TEST_P(EndToEndTest, IetfConnectionMigrationClientIPChangedMultipleTimes) { ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid() || + if (!version_.HasIetfQuicFrames() || GetQuicFlag(quic_enforce_strict_amplification_factor)) { return; } @@ -3011,16 +3011,13 @@ TEST_P(EndToEndTest, ConnectionMigrationWithNonZeroConnectionIDClientIPChangedMultipleTimes) { - if (!version_.SupportsClientConnectionIds() || + if (!version_.HasIetfQuicFrames() || GetQuicFlag(quic_enforce_strict_amplification_factor)) { ASSERT_TRUE(Initialize()); return; } override_client_connection_id_length_ = kQuicDefaultConnectionIdLength; ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { - return; - } SendSynchronousFooRequestAndCheckResponse(); // Store the client IP address which was used to send the first request. @@ -3230,7 +3227,7 @@ TEST_P(EndToEndTest, ClientAddressSpoofedForSomePeriod) { ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } auto writer = new DuplicatePacketWithSpoofedSelfAddressWriter(); @@ -3285,7 +3282,7 @@ TEST_P(EndToEndTest, AsynchronousConnectionMigrationClientIPChangedMultipleTimes) { ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } client_.reset(CreateQuicClient(nullptr)); @@ -3376,15 +3373,12 @@ TEST_P(EndToEndTest, AsynchronousConnectionMigrationClientIPChangedWithNonEmptyClientCID) { - if (!version_.SupportsClientConnectionIds()) { + if (!version_.HasIetfQuicFrames()) { ASSERT_TRUE(Initialize()); return; } override_client_connection_id_length_ = kQuicDefaultConnectionIdLength; ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { - return; - } client_.reset(CreateQuicClient(nullptr)); SendSynchronousFooRequestAndCheckResponse(); @@ -5352,7 +5346,7 @@ TEST_P(EndToEndTest, ClientMultiPortConnection) { client_config_.SetClientConnectionOptions(QuicTagVector{kMPQC}); ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } client_.reset(EndToEndTest::CreateQuicClient(nullptr)); @@ -5398,7 +5392,7 @@ TEST_P(EndToEndTest, ClientMultiPortMigrationOnPathDegrading) { client_config_.SetClientConnectionOptions(QuicTagVector{kMPQC}); ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } client_.reset(EndToEndTest::CreateQuicClient(nullptr)); @@ -5442,7 +5436,7 @@ TEST_P(EndToEndTest, SimpleServerPreferredAddressTest) { use_preferred_address_ = true; ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } client_.reset(CreateQuicClient(nullptr)); @@ -5470,7 +5464,7 @@ TEST_P(EndToEndTest, OptimizedServerPreferredAddress) { use_preferred_address_ = true; ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } client_config_.SetClientConnectionOptions(QuicTagVector{kSPA2}); @@ -5593,7 +5587,7 @@ TEST_P(EndToEndPacketReorderingTest, MigrateAgainAfterPathValidationFailure) { ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } @@ -5686,7 +5680,7 @@ TEST_P(EndToEndPacketReorderingTest, MigrateAgainAfterPathValidationFailureWithNonZeroClientCid) { - if (!version_.SupportsClientConnectionIds()) { + if (!version_.HasIetfQuicFrames()) { ASSERT_TRUE(Initialize()); return; } @@ -5694,9 +5688,6 @@ true); override_client_connection_id_length_ = kQuicDefaultConnectionIdLength; ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { - return; - } client_.reset(CreateQuicClient(nullptr)); // Finish one request to make sure handshake established. @@ -7290,7 +7281,7 @@ TEST_P(EndToEndTest, ClientMigrationAfterHalfwayServerMigration) { use_preferred_address_ = true; ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; } client_.reset(EndToEndTest::CreateQuicClient(nullptr)); @@ -7348,7 +7339,7 @@ TEST_P(EndToEndTest, MultiPortCreationFollowingServerMigration) { use_preferred_address_ = true; ASSERT_TRUE(Initialize()); - if (!GetClientConnection()->connection_migration_use_new_cid()) { + if (!version_.HasIetfQuicFrames()) { return; }
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index e1ec2d8..f22f9b0 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -645,16 +645,8 @@ if (config.HasClientRequestedIndependentOption(kINVC, perspective_)) { send_connection_close_for_invalid_version_ = true; } - // Having connection_migration_use_new_cid_ depends on the same set of flags - // and connection option on both client and server sides has the advantage of: - // 1) Less chance of skew in using new connection ID or not between client - // and server in unit tests with random flag combinations. - // 2) Client side's rollout can be protected by the same connection option. - connection_migration_use_new_cid_ = - framer_.version().HasIetfQuicFrames() && - GetQuicReloadableFlag(quic_connection_migration_use_new_cid_v2); - if (connection_migration_use_new_cid_ && + if (version().HasIetfQuicFrames() && config.HasReceivedPreferredAddressConnectionIdAndToken() && config.HasClientSentConnectionOption(kSPAD, perspective_)) { if (self_address().host().IsIPv4() && @@ -695,8 +687,7 @@ UpdateReleaseTimeIntoFuture(); } - if (perspective_ == Perspective::IS_CLIENT && - connection_migration_use_new_cid_ && + if (perspective_ == Perspective::IS_CLIENT && version().HasIetfQuicFrames() && config.HasClientRequestedIndependentOption(kMPQC, perspective_)) { multi_port_stats_ = std::make_unique<MultiPortStats>(); } @@ -991,8 +982,7 @@ return true; } - if (connection_migration_use_new_cid_ && - perspective_ == Perspective::IS_SERVER && + if (version().HasIetfQuicFrames() && perspective_ == Perspective::IS_SERVER && self_issued_cid_manager_ != nullptr && self_issued_cid_manager_->IsConnectionIdInUse(server_connection_id)) { return true; @@ -1065,8 +1055,7 @@ return true; } - if (connection_migration_use_new_cid_ && - perspective_ == Perspective::IS_CLIENT && + if (version().HasIetfQuicFrames() && perspective_ == Perspective::IS_CLIENT && self_issued_cid_manager_ != nullptr && self_issued_cid_manager_->IsConnectionIdInUse(client_connection_id)) { return true; @@ -1261,7 +1250,7 @@ default_path_.peer_address, GetEffectivePeerAddressFromCurrentPacket()); - if (connection_migration_use_new_cid_) { + if (version().HasIetfQuicFrames()) { auto effective_peer_address = GetEffectivePeerAddressFromCurrentPacket(); // Since server does not send new connection ID to client before handshake // completion and source connection ID is omitted in short header packet, @@ -1711,7 +1700,7 @@ { QuicPacketCreator::ScopedPeerAddressContext context( &packet_creator_, direct_peer_address_to_respond, client_cid, - server_cid, connection_migration_use_new_cid_); + server_cid); if (should_proactively_validate_peer_address_on_path_challenge_) { // Conditions to proactively validate peer address: // The perspective is server @@ -1904,9 +1893,6 @@ return; } if (default_path_.client_connection_id.IsEmpty()) { - // Count client connection ID patched onto the default path. - QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 3, - 6); const QuicConnectionIdData* unused_cid_data = peer_issued_cid_manager_->ConsumeOneUnusedConnectionId(); QUIC_DVLOG(1) << ENDPOINT << "Patch connection ID " @@ -1921,9 +1907,6 @@ } if (alternative_path_.peer_address.IsInitialized() && alternative_path_.client_connection_id.IsEmpty()) { - // Count client connection ID patched onto the alternative path. - QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 4, - 6); const QuicConnectionIdData* unused_cid_data = peer_issued_cid_manager_->ConsumeOneUnusedConnectionId(); QUIC_DVLOG(1) << ENDPOINT << "Patch connection ID " @@ -1996,10 +1979,6 @@ if (debug_visitor_ != nullptr) { debug_visitor_->OnRetireConnectionIdFrame(frame); } - if (!connection_migration_use_new_cid_) { - // Do not respond to RetireConnectionId frame. - return true; - } if (self_issued_cid_manager_ == nullptr) { CloseConnection( IETF_QUIC_PROTOCOL_VIOLATION, @@ -2016,7 +1995,6 @@ return false; } // Count successfully received RETIRE_CONNECTION_ID frames. - QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 5, 6); MaybeUpdateAckTimeout(); return true; } @@ -2787,8 +2765,8 @@ } void QuicConnection::MaybeClearQueuedPacketsOnPathChange() { - if (connection_migration_use_new_cid_ && - peer_issued_cid_manager_ != nullptr && HasQueuedPackets()) { + if (version().HasIetfQuicFrames() && peer_issued_cid_manager_ != nullptr && + HasQueuedPackets()) { // Discard packets serialized with the connection ID on the old code path. // It is possible to clear queued packets only if connection ID changes. // However, the case where connection ID is unchanged and queued packets are @@ -2830,7 +2808,8 @@ const QuicConnectionId& server_connection_id, QuicConnectionId* client_connection_id, absl::optional<StatelessResetToken>* stateless_reset_token) { - QUICHE_DCHECK(perspective_ == Perspective::IS_SERVER); + QUICHE_DCHECK(perspective_ == Perspective::IS_SERVER && + version().HasIetfQuicFrames()); if (peer_issued_cid_manager_ == nullptr || server_connection_id == default_path.server_connection_id) { *client_connection_id = default_path.client_connection_id; @@ -2842,10 +2821,6 @@ *stateless_reset_token = alternative_path.stateless_reset_token; return; } - if (!connection_migration_use_new_cid_) { - QUIC_BUG(quic_bug_46004) << "Cannot find matching connection ID."; - return; - } auto* connection_id_data = peer_issued_cid_manager_->ConsumeOneUnusedConnectionId(); if (connection_id_data == nullptr) { @@ -2882,11 +2857,10 @@ } void QuicConnection::SetDefaultPathState(PathState new_path_state) { + QUICHE_DCHECK(version().HasIetfQuicFrames()); default_path_ = std::move(new_path_state); - if (connection_migration_use_new_cid_) { - packet_creator_.SetClientConnectionId(default_path_.client_connection_id); - packet_creator_.SetServerConnectionId(default_path_.server_connection_id); - } + packet_creator_.SetClientConnectionId(default_path_.client_connection_id); + packet_creator_.SetServerConnectionId(default_path_.server_connection_id); } bool QuicConnection::ProcessValidatedPacket(const QuicPacketHeader& header) { @@ -3947,8 +3921,7 @@ void QuicConnection::OnHandshakeComplete() { sent_packet_manager_.SetHandshakeConfirmed(); - if (connection_migration_use_new_cid_ && - perspective_ == Perspective::IS_SERVER && + if (version().HasIetfQuicFrames() && perspective_ == Perspective::IS_SERVER && self_issued_cid_manager_ != nullptr) { self_issued_cid_manager_->MaybeSendNewConnectionIds(); } @@ -4496,7 +4469,7 @@ // Always use the current path to send CONNECTION_CLOSE. QuicPacketCreator::ScopedPeerAddressContext context( &packet_creator_, peer_address(), default_path_.client_connection_id, - default_path_.server_connection_id, connection_migration_use_new_cid_); + default_path_.server_connection_id); if (!SupportsMultiplePacketNumberSpaces()) { QUIC_DLOG(INFO) << ENDPOINT << "Sending connection close packet."; ScopedEncryptionLevelContext context(this, @@ -6528,57 +6501,33 @@ if (!framer_.HasEncrypterOfEncryptionLevel(ENCRYPTION_FORWARD_SECURE)) { return connected_; } - if (connection_migration_use_new_cid_) { - QuicConnectionId client_cid, server_cid; - FindOnPathConnectionIds(self_address, effective_peer_address, &client_cid, - &server_cid); - if (writer == writer_) { - ScopedPacketFlusher flusher(this); - { - QuicPacketCreator::ScopedPeerAddressContext context( - &packet_creator_, peer_address, client_cid, server_cid, - connection_migration_use_new_cid_); - // It's using the default writer, add the PATH_CHALLENGE the same way as - // other frames. This may cause connection to be closed. - packet_creator_.AddPathChallengeFrame(data_buffer); - } - } else { - // Switch to the right CID and source/peer addresses. - QuicPacketCreator::ScopedPeerAddressContext context( - &packet_creator_, peer_address, client_cid, server_cid, - connection_migration_use_new_cid_); - std::unique_ptr<SerializedPacket> probing_packet = - packet_creator_.SerializePathChallengeConnectivityProbingPacket( - data_buffer); - QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet), - NO_RETRANSMITTABLE_DATA); - QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address); - WritePacketUsingWriter(std::move(probing_packet), writer, self_address, - peer_address, /*measure_rtt=*/false); - } - return connected_; - } + + QuicConnectionId client_cid, server_cid; + FindOnPathConnectionIds(self_address, effective_peer_address, &client_cid, + &server_cid); if (writer == writer_) { ScopedPacketFlusher flusher(this); { - // It's on current path, add the PATH_CHALLENGE the same way as other - // frames. QuicPacketCreator::ScopedPeerAddressContext context( - &packet_creator_, peer_address, /*update_connection_id=*/false); - // This may cause connection to be closed. + &packet_creator_, peer_address, client_cid, server_cid); + // It's using the default writer, add the PATH_CHALLENGE the same way as + // other frames. This may cause connection to be closed. packet_creator_.AddPathChallengeFrame(data_buffer); } - // Return outside of the scope so that the flush result can be reflected. - return connected_; + } else { + // Switch to the right CID and source/peer addresses. + QuicPacketCreator::ScopedPeerAddressContext context( + &packet_creator_, peer_address, client_cid, server_cid); + std::unique_ptr<SerializedPacket> probing_packet = + packet_creator_.SerializePathChallengeConnectivityProbingPacket( + data_buffer); + QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet), + NO_RETRANSMITTABLE_DATA); + QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address); + WritePacketUsingWriter(std::move(probing_packet), writer, self_address, + peer_address, /*measure_rtt=*/false); } - std::unique_ptr<SerializedPacket> probing_packet = - packet_creator_.SerializePathChallengeConnectivityProbingPacket( - data_buffer); - QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet), NO_RETRANSMITTABLE_DATA); - QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address); - WritePacketUsingWriter(std::move(probing_packet), writer, self_address, - peer_address, /*measure_rtt=*/false); - return true; + return connected_; } QuicTime QuicConnection::GetRetryTimeout( @@ -6595,14 +6544,7 @@ std::unique_ptr<QuicPathValidationContext> context, std::unique_ptr<QuicPathValidator::ResultDelegate> result_delegate, PathValidationReason reason) { - if (!connection_migration_use_new_cid_ && - perspective_ == Perspective::IS_CLIENT && - !IsDefaultPath(context->self_address(), context->peer_address())) { - alternative_path_ = PathState( - context->self_address(), context->peer_address(), - default_path_.client_connection_id, default_path_.server_connection_id, - default_path_.stateless_reset_token); - } + QUICHE_DCHECK(version().HasIetfQuicFrames()); if (path_validator_.HasPendingPathValidation()) { if (perspective_ == Perspective::IS_CLIENT && IsValidatingServerPreferredAddress()) { @@ -6614,8 +6556,7 @@ // Cancel and fail any earlier validation. path_validator_.CancelPathValidation(); } - if (connection_migration_use_new_cid_ && - perspective_ == Perspective::IS_CLIENT && + if (perspective_ == Perspective::IS_CLIENT && !IsDefaultPath(context->self_address(), context->peer_address())) { if (self_issued_cid_manager_ != nullptr) { self_issued_cid_manager_->MaybeSendNewConnectionIds(); @@ -6664,16 +6605,13 @@ return false; } QuicConnectionId client_cid, server_cid; - if (connection_migration_use_new_cid_) { - FindOnPathConnectionIds(last_received_packet_info_.destination_address, - effective_peer_address, &client_cid, &server_cid); - } + FindOnPathConnectionIds(last_received_packet_info_.destination_address, + effective_peer_address, &client_cid, &server_cid); // Send PATH_RESPONSE using the provided peer address. If the creator has been // using a different peer address, it will flush before and after serializing // the current PATH_RESPONSE. QuicPacketCreator::ScopedPeerAddressContext context( - &packet_creator_, peer_address_to_send, client_cid, server_cid, - connection_migration_use_new_cid_); + &packet_creator_, peer_address_to_send, client_cid, server_cid); QUIC_DVLOG(1) << ENDPOINT << "Send PATH_RESPONSE to " << peer_address_to_send; if (default_path_.self_address == last_received_packet_info_.destination_address) { @@ -6773,8 +6711,7 @@ } void QuicConnection::RetirePeerIssuedConnectionIdsNoLongerOnPath() { - if (!connection_migration_use_new_cid_ || - peer_issued_cid_manager_ == nullptr) { + if (!version().HasIetfQuicFrames() || peer_issued_cid_manager_ == nullptr) { return; } if (perspective_ == Perspective::IS_CLIENT) { @@ -6816,7 +6753,7 @@ QUICHE_DCHECK(!version().UsesHttp3() || IsHandshakeConfirmed() || accelerated_server_preferred_address_); - if (connection_migration_use_new_cid_) { + if (version().UsesHttp3()) { if (!UpdateConnectionIdsOnMigration(self_address, peer_address)) { if (owns_writer) { delete writer; @@ -6854,10 +6791,9 @@ void QuicConnection::OnPathValidationFailureAtClient( bool is_multi_port, const QuicPathValidationContext& context) { - if (connection_migration_use_new_cid_) { - QUICHE_DCHECK(perspective_ == Perspective::IS_CLIENT); - alternative_path_.Clear(); - } + QUICHE_DCHECK(perspective_ == Perspective::IS_CLIENT && + version().HasIetfQuicFrames()); + alternative_path_.Clear(); if (is_multi_port && multi_port_stats_ != nullptr) { if (is_path_degrading_) {
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index a650978..90e5018 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -1260,10 +1260,6 @@ // session map. virtual std::vector<QuicConnectionId> GetActiveServerConnectionIds() const; - bool connection_migration_use_new_cid() const { - return connection_migration_use_new_cid_; - } - // Instantiates connection ID manager. void CreateConnectionIdManager(); @@ -2340,9 +2336,6 @@ // PATH_CHALLENGE received. bool should_proactively_validate_peer_address_on_path_challenge_ = false; - // Enable this via reloadable flag once this feature is complete. - bool connection_migration_use_new_cid_ = false; - // If true, send connection close packet on INVALID_VERSION. bool send_connection_close_for_invalid_version_ = false;
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index 00f02b2..a26731f 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -1441,7 +1441,6 @@ set_perspective(Perspective::IS_SERVER); connection_.CreateConnectionIdManager(); QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); - SetQuicReloadableFlag(quic_connection_migration_use_new_cid_v2, true); SetQuicReloadableFlag(quic_use_received_client_addresses_cache, true); EXPECT_CALL(visitor_, AllowSelfAddressChange()) .WillRepeatedly(Return(true)); @@ -1486,7 +1485,6 @@ ASSERT_EQ(Perspective::IS_CLIENT, connection_.perspective()); ASSERT_TRUE(version().HasIetfQuicFrames()); ASSERT_TRUE(connection_.self_address().host().IsIPv6()); - SetQuicReloadableFlag(quic_connection_migration_use_new_cid_v2, true); const QuicConnectionId connection_id = TestConnectionId(17); const StatelessResetToken reset_token = QuicUtils::GenerateStatelessResetToken(connection_id); @@ -1924,7 +1922,7 @@ TEST_P(QuicConnectionTest, PeerIpAddressChangeAtServerWithMissingConnectionId) { set_perspective(Perspective::IS_SERVER); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); @@ -1981,7 +1979,7 @@ peer_creator_.SetServerConnectionId(server_cid1); EXPECT_CALL(visitor_, OnConnectionMigration(IPV6_TO_IPV4_CHANGE)).Times(1); // Do not propagate OnCanWrite() to session notifier. - EXPECT_CALL(visitor_, OnCanWrite()).Times(AtLeast(1u)); + EXPECT_CALL(visitor_, OnCanWrite()).Times(testing::AtMost(1u)); QuicFrames frames2; frames2.push_back(QuicFrame(frame2_)); @@ -2134,7 +2132,7 @@ TEST_P(QuicConnectionTest, ConnectionMigrationWithPendingPaddingBytes) { // TODO(haoyuewang) Move these test setup code to a common member function. set_perspective(Perspective::IS_SERVER); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); @@ -2193,7 +2191,7 @@ TEST_P(QuicConnectionTest, ReversePathValidationResponseReceivedFromUnexpectedPeerAddress) { set_perspective(Perspective::IS_SERVER); - if (!connection_.connection_migration_use_new_cid() || + if (!version().HasIetfQuicFrames() || GetQuicFlag(quic_enforce_strict_amplification_factor)) { return; } @@ -2259,7 +2257,7 @@ TEST_P(QuicConnectionTest, ReversePathValidationFailureAtServer) { set_perspective(Perspective::IS_SERVER); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); @@ -3042,7 +3040,6 @@ } ASSERT_EQ(Perspective::IS_CLIENT, connection_.perspective()); ASSERT_TRUE(connection_.self_address().host().IsIPv6()); - SetQuicReloadableFlag(quic_connection_migration_use_new_cid_v2, true); const QuicConnectionId connection_id = TestConnectionId(17); const StatelessResetToken reset_token = QuicUtils::GenerateStatelessResetToken(connection_id); @@ -11440,17 +11437,6 @@ const QuicSocketAddress kNewSelfAddress2(QuicIpAddress::Any4(), 12346); EXPECT_NE(kNewSelfAddress2, connection_.self_address()); TestPacketWriter new_writer2(version(), &clock_, Perspective::IS_CLIENT); - if (!connection_.connection_migration_use_new_cid()) { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)) - .Times(AtLeast(1u)) - .WillOnce(Invoke([&]() { - EXPECT_EQ(1u, new_writer2.packets_write_attempts()); - EXPECT_EQ(1u, new_writer2.path_challenge_frames().size()); - EXPECT_EQ(1u, new_writer2.padding_frames().size()); - EXPECT_EQ(kNewSelfAddress2.host(), - new_writer2.last_write_source_address()); - })); - } bool success2 = false; connection_.ValidatePath( std::make_unique<TestQuicPathValidationContext>( @@ -11460,13 +11446,8 @@ &success2), PathValidationReason::kReasonUnknown); EXPECT_FALSE(success); - if (connection_.connection_migration_use_new_cid()) { - // There is no pening path validation as there is no available connection - // ID. - EXPECT_FALSE(connection_.HasPendingPathValidation()); - } else { - EXPECT_TRUE(connection_.HasPendingPathValidation()); - } + // There is no pening path validation as there is no available connection ID. + EXPECT_FALSE(connection_.HasPendingPathValidation()); } // Regression test for b/182571515. @@ -11550,8 +11531,7 @@ // Tests that PATH_CHALLENGE is dropped if it is sent via a blocked alternative // writer. TEST_P(QuicConnectionTest, SendPathChallengeUsingBlockedNewSocket) { - if (!VersionHasIetfQuicFrames(connection_.version().transport_version) || - !connection_.connection_migration_use_new_cid()) { + if (!VersionHasIetfQuicFrames(connection_.version().transport_version)) { return; } PathProbeTestInit(Perspective::IS_CLIENT); @@ -12903,6 +12883,9 @@ } TEST_P(QuicConnectionTest, MigratePath) { + connection_.CreateConnectionIdManager(); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + connection_.OnHandshakeComplete(); EXPECT_CALL(visitor_, GetHandshakeState()) .WillRepeatedly(Return(HANDSHAKE_CONFIRMED)); EXPECT_CALL(visitor_, OnPathDegrading()); @@ -12916,16 +12899,28 @@ connection_.SendMtuDiscoveryPacket(kMaxOutgoingPacketSize); EXPECT_EQ(1u, connection_.NumQueuedPackets()); + if (version().HasIetfQuicFrames()) { + QuicNewConnectionIdFrame frame; + frame.connection_id = TestConnectionId(1234); + ASSERT_NE(frame.connection_id, connection_.connection_id()); + frame.stateless_reset_token = + QuicUtils::GenerateStatelessResetToken(frame.connection_id); + frame.retire_prior_to = 0u; + frame.sequence_number = 1u; + connection_.OnNewConnectionIdFrame(frame); + } + TestPacketWriter new_writer(version(), &clock_, Perspective::IS_CLIENT); EXPECT_CALL(visitor_, OnForwardProgressMadeAfterPathDegrading()); - connection_.MigratePath(kNewSelfAddress, connection_.peer_address(), - &new_writer, /*owns_writer=*/false); + EXPECT_TRUE(connection_.MigratePath(kNewSelfAddress, + connection_.peer_address(), &new_writer, + /*owns_writer=*/false)); EXPECT_EQ(kNewSelfAddress, connection_.self_address()); EXPECT_EQ(&new_writer, QuicConnectionPeer::GetWriter(&connection_)); EXPECT_FALSE(connection_.IsPathDegrading()); // Buffered packet on the old path should be discarded. - if (connection_.connection_migration_use_new_cid()) { + if (version().HasIetfQuicFrames()) { EXPECT_EQ(0u, connection_.NumQueuedPackets()); } else { EXPECT_EQ(1u, connection_.NumQueuedPackets()); @@ -12966,7 +12961,7 @@ config.SetClientConnectionOptions(QuicTagVector{kMPQC}); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } connection_.CreateConnectionIdManager(); @@ -13100,7 +13095,7 @@ config.SetClientConnectionOptions(QuicTagVector{kMPQC}); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } connection_.CreateConnectionIdManager(); @@ -13215,7 +13210,7 @@ config.SetClientConnectionOptions(QuicTagVector{kMPQC}); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } connection_.CreateConnectionIdManager(); @@ -13268,6 +13263,9 @@ // Test that if the client's active migration is disabled, multi-port will not // be attempted. TEST_P(QuicConnectionTest, MultiPortPathRespectsActiveMigrationConfig) { + if (!version().HasIetfQuicFrames()) { + return; + } set_perspective(Perspective::IS_CLIENT); QuicConfig config; QuicConfigPeer::SetReceivedStatelessResetToken(&config, @@ -13276,9 +13274,6 @@ config.SetClientConnectionOptions(QuicTagVector{kMPQC}); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (!connection_.connection_migration_use_new_cid()) { - return; - } connection_.CreateConnectionIdManager(); connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); connection_.OnHandshakeComplete(); @@ -13306,7 +13301,7 @@ config.SetClientConnectionOptions(QuicTagVector{kMPQC}); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } connection_.CreateConnectionIdManager(); @@ -13376,7 +13371,7 @@ config.SetClientConnectionOptions(QuicTagVector{kMPQC}); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } connection_.CreateConnectionIdManager(); @@ -13454,7 +13449,7 @@ config.SetClientConnectionOptions(QuicTagVector{kMPQC}); EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); connection_.SetFromConfig(config); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } connection_.CreateConnectionIdManager(); @@ -13792,7 +13787,7 @@ TEST_P(QuicConnectionTest, PathChallengeBeforePeerIpAddressChangeAtServer) { set_perspective(Perspective::IS_SERVER); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } PathProbeTestInit(Perspective::IS_SERVER); @@ -13948,7 +13943,7 @@ TEST_P(QuicConnectionTest, PathValidationSucceedsBeforePeerIpAddressChangeAtServer) { set_perspective(Perspective::IS_SERVER); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } PathProbeTestInit(Perspective::IS_SERVER); @@ -14069,7 +14064,7 @@ // Regression test of b/228645208. TEST_P(QuicConnectionTest, NoNonProbingFrameOnAlternativePath) { - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } @@ -14184,7 +14179,7 @@ TEST_P(QuicConnectionTest, DoNotIssueNewCidIfVisitorSaysNo) { set_perspective(Perspective::IS_SERVER); - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } @@ -14268,7 +14263,7 @@ TEST_P(QuicConnectionTest, PathValidationFailedOnClientDueToLackOfServerConnectionId) { - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } PathProbeTestInit(Perspective::IS_CLIENT, @@ -14290,7 +14285,7 @@ TEST_P(QuicConnectionTest, PathValidationFailedOnClientDueToLackOfClientConnectionIdTheSecondTime) { - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } PathProbeTestInit(Perspective::IS_CLIENT, @@ -14378,7 +14373,7 @@ } TEST_P(QuicConnectionTest, ServerConnectionIdRetiredUponPathValidationFailure) { - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } PathProbeTestInit(Perspective::IS_CLIENT); @@ -14422,7 +14417,7 @@ TEST_P(QuicConnectionTest, MigratePathDirectlyFailedDueToLackOfServerConnectionId) { - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } PathProbeTestInit(Perspective::IS_CLIENT, @@ -14438,7 +14433,7 @@ TEST_P(QuicConnectionTest, MigratePathDirectlyFailedDueToLackOfClientConnectionIdTheSecondTime) { - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } PathProbeTestInit(Perspective::IS_CLIENT, @@ -14684,8 +14679,7 @@ TEST_P(QuicConnectionTest, CloseConnectionAfterReceiveRetireConnectionIdWhenNoCIDIssued) { - if (!version().HasIetfQuicFrames() || - !connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } set_perspective(Perspective::IS_SERVER); @@ -14704,8 +14698,7 @@ } TEST_P(QuicConnectionTest, RetireConnectionIdFrameResultsInError) { - if (!version().HasIetfQuicFrames() || - !connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } set_perspective(Perspective::IS_SERVER); @@ -14715,7 +14708,7 @@ EXPECT_CALL(connection_id_generator_, GenerateNextConnectionId(_)) .WillOnce(Return(TestConnectionId(456))); } - EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)).WillOnce(Return(true)); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.MaybeSendConnectionIdToClient(); @@ -14747,24 +14740,23 @@ QuicConnectionId cid0 = connection_id_; QuicRetireConnectionIdFrame frame; frame.sequence_number = 0u; - if (connection_.connection_migration_use_new_cid()) { - if (!connection_.connection_id().IsEmpty()) { - EXPECT_CALL(connection_id_generator_, GenerateNextConnectionId(cid0)) - .WillOnce(Return(TestConnectionId(456))); - EXPECT_CALL(connection_id_generator_, - GenerateNextConnectionId(TestConnectionId(456))) - .WillOnce(Return(TestConnectionId(789))); - } - EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) - .Times(2) - .WillRepeatedly(Return(true)); - EXPECT_CALL(visitor_, SendNewConnectionId(_)).Times(2); + + if (!connection_.connection_id().IsEmpty()) { + EXPECT_CALL(connection_id_generator_, GenerateNextConnectionId(cid0)) + .WillOnce(Return(TestConnectionId(456))); + EXPECT_CALL(connection_id_generator_, + GenerateNextConnectionId(TestConnectionId(456))) + .WillOnce(Return(TestConnectionId(789))); } + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .Times(2) + .WillRepeatedly(Return(true)); + EXPECT_CALL(visitor_, SendNewConnectionId(_)).Times(2); EXPECT_TRUE(connection_.OnRetireConnectionIdFrame(frame)); } TEST_P(QuicConnectionTest, ServerRetireSelfIssuedConnectionId) { - if (!connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } set_perspective(Perspective::IS_SERVER); @@ -14957,8 +14949,7 @@ // No connection ID is available as context is created without any. QuicPacketCreator::ScopedPeerAddressContext context( packet_creator, peer_address1, EmptyQuicConnectionId(), - EmptyQuicConnectionId(), - /*update_connection_id=*/true); + EmptyQuicConnectionId()); ASSERT_FALSE(connection_.ShouldGeneratePacket(NO_RETRANSMITTABLE_DATA, NOT_HANDSHAKE)); } @@ -15207,8 +15198,7 @@ } TEST_P(QuicConnectionTest, AckElicitingFrames) { - if (!version().HasIetfQuicFrames() || - !connection_.connection_migration_use_new_cid()) { + if (!version().HasIetfQuicFrames()) { return; } EXPECT_CALL(connection_id_generator_, @@ -16482,9 +16472,6 @@ QuicConfig config; config.SetClientConnectionOptions(QuicTagVector{kMPQC}); ServerPreferredAddressInit(config); - if (!connection_.connection_migration_use_new_cid()) { - return; - } connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); QuicConnectionId cid_for_preferred_address = TestConnectionId(17); const QuicSocketAddress kNewSelfAddress =
diff --git a/quiche/quic/core/quic_dispatcher.cc b/quiche/quic/core/quic_dispatcher.cc index 70e4e6b..d778d96 100644 --- a/quiche/quic/core/quic_dispatcher.cc +++ b/quiche/quic/core/quic_dispatcher.cc
@@ -981,8 +981,6 @@ << server_connection_id << " new_connection_id: " << new_connection_id; return false; } - // Count new connection ID added to the dispatcher map. - QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 6, 6); auto insertion_result = reference_counted_session_map_.insert( std::make_pair(new_connection_id, it->second)); if (!insertion_result.second) {
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index e1e6645..0747a59 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -71,8 +71,6 @@ QUIC_FLAG(quic_reloadable_flag_quic_default_to_bbr_v2, false) // If true, use a LRU cache to record client addresses of packets received on server\'s original address. QUIC_FLAG(quic_reloadable_flag_quic_use_received_client_addresses_cache, true) -// If true, use new connection ID in connection migration. -QUIC_FLAG(quic_reloadable_flag_quic_connection_migration_use_new_cid_v2, true) // If true, use next_connection_id_sequence_number to validate retired cid number. QUIC_FLAG(quic_reloadable_flag_quic_check_retire_cid_with_next_cid_sequence_number, true) // If true, uses conservative cwnd gain and pacing gain when cwnd gets bootstrapped.
diff --git a/quiche/quic/core/quic_packet_creator.cc b/quiche/quic/core/quic_packet_creator.cc index 63c07b6..f62f9d1 100644 --- a/quiche/quic/core/quic_packet_creator.cc +++ b/quiche/quic/core/quic_packet_creator.cc
@@ -2137,25 +2137,18 @@ QuicPacketCreator::ScopedPeerAddressContext::ScopedPeerAddressContext( QuicPacketCreator* creator, QuicSocketAddress address, - bool update_connection_id) - : ScopedPeerAddressContext(creator, address, EmptyQuicConnectionId(), - EmptyQuicConnectionId(), update_connection_id) {} - -QuicPacketCreator::ScopedPeerAddressContext::ScopedPeerAddressContext( - QuicPacketCreator* creator, QuicSocketAddress address, const QuicConnectionId& client_connection_id, - const QuicConnectionId& server_connection_id, bool update_connection_id) + const QuicConnectionId& server_connection_id) : creator_(creator), old_peer_address_(creator_->packet_.peer_address), old_client_connection_id_(creator_->GetClientConnectionId()), - old_server_connection_id_(creator_->GetServerConnectionId()), - update_connection_id_(update_connection_id) { + old_server_connection_id_(creator_->GetServerConnectionId()) { QUIC_BUG_IF(quic_bug_12398_19, !old_peer_address_.IsInitialized()) << ENDPOINT2 << "Context is used before serialized packet's peer address is " "initialized."; creator_->SetDefaultPeerAddress(address); - if (update_connection_id_) { + if (creator_->version().HasIetfQuicFrames()) { // Flush current packet if connection ID length changes. if (address == old_peer_address_ && ((client_connection_id.length() != @@ -2171,7 +2164,7 @@ QuicPacketCreator::ScopedPeerAddressContext::~ScopedPeerAddressContext() { creator_->SetDefaultPeerAddress(old_peer_address_); - if (update_connection_id_) { + if (creator_->version().HasIetfQuicFrames()) { creator_->SetClientConnectionId(old_client_connection_id_); creator_->SetServerConnectionId(old_server_connection_id_); }
diff --git a/quiche/quic/core/quic_packet_creator.h b/quiche/quic/core/quic_packet_creator.h index 8396b1c..7c53414 100644 --- a/quiche/quic/core/quic_packet_creator.h +++ b/quiche/quic/core/quic_packet_creator.h
@@ -93,13 +93,8 @@ public: ScopedPeerAddressContext(QuicPacketCreator* creator, QuicSocketAddress address, - bool update_connection_id); - - ScopedPeerAddressContext(QuicPacketCreator* creator, - QuicSocketAddress address, const QuicConnectionId& client_connection_id, - const QuicConnectionId& server_connection_id, - bool update_connection_id); + const QuicConnectionId& server_connection_id); ~ScopedPeerAddressContext(); private: @@ -107,7 +102,6 @@ QuicSocketAddress old_peer_address_; QuicConnectionId old_client_connection_id_; QuicConnectionId old_server_connection_id_; - bool update_connection_id_; }; QuicPacketCreator(QuicConnectionId server_connection_id, QuicFramer* framer,
diff --git a/quiche/quic/core/quic_packet_creator_test.cc b/quiche/quic/core/quic_packet_creator_test.cc index 6ff1bc1..aa919db 100644 --- a/quiche/quic/core/quic_packet_creator_test.cc +++ b/quiche/quic/core/quic_packet_creator_test.cc
@@ -3945,8 +3945,7 @@ { // Set the same address via context which should not trigger flush. QuicPacketCreator::ScopedPeerAddressContext context( - &creator_, peer_addr, client_connection_id, server_connection_id, - /*update_connection_id=*/true); + &creator_, peer_addr, client_connection_id, server_connection_id); ASSERT_EQ(client_connection_id, creator_.GetClientConnectionId()); ASSERT_EQ(server_connection_id, creator_.GetServerConnectionId()); EXPECT_TRUE(creator_.HasPendingFrames()); @@ -4000,8 +3999,7 @@ QuicConnectionId server_connection_id = TestConnectionId(2); // Set a different address via context which should trigger flush. QuicPacketCreator::ScopedPeerAddressContext context( - &creator_, peer_addr1, client_connection_id, server_connection_id, - /*update_connection_id=*/true); + &creator_, peer_addr1, client_connection_id, server_connection_id); ASSERT_EQ(client_connection_id, creator_.GetClientConnectionId()); ASSERT_EQ(server_connection_id, creator_.GetServerConnectionId()); EXPECT_FALSE(creator_.HasPendingFrames()); @@ -4024,8 +4022,7 @@ QuicSocketAddress peer_addr(QuicIpAddress::Any4(), 12345); creator_.SetDefaultPeerAddress(peer_addr); QuicPacketCreator::ScopedPeerAddressContext context( - &creator_, peer_addr, client_connection_id1, server_connection_id1, - /*update_connection_id=*/true); + &creator_, peer_addr, client_connection_id1, server_connection_id1); ASSERT_EQ(client_connection_id1, creator_.GetClientConnectionId()); ASSERT_EQ(server_connection_id1, creator_.GetServerConnectionId()); @@ -4050,8 +4047,8 @@ QuicConnectionId server_connection_id2 = TestConnectionId(4); // Set up another context with a different address. QuicPacketCreator::ScopedPeerAddressContext context( - &creator_, peer_addr1, client_connection_id2, server_connection_id2, - /*update_connection_id=*/true); + &creator_, peer_addr1, client_connection_id2, + server_connection_id2); ASSERT_EQ(client_connection_id2, creator_.GetClientConnectionId()); ASSERT_EQ(server_connection_id2, creator_.GetServerConnectionId()); EXPECT_CALL(delegate_, ShouldGeneratePacket(_, _))
diff --git a/quiche/quic/core/quic_session.cc b/quiche/quic/core/quic_session.cc index 74a0fc0..0720fa1 100644 --- a/quiche/quic/core/quic_session.cc +++ b/quiche/quic/core/quic_session.cc
@@ -2161,16 +2161,12 @@ } void QuicSession::SendNewConnectionId(const QuicNewConnectionIdFrame& frame) { - // Count NEW_CONNECTION_ID frames sent to client. - QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 1, 6); control_frame_manager_.WriteOrBufferNewConnectionId( frame.connection_id, frame.sequence_number, frame.retire_prior_to, frame.stateless_reset_token); } void QuicSession::SendRetireConnectionId(uint64_t sequence_number) { - // Count RETIRE_CONNECTION_ID frames sent to client. - QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 2, 6); control_frame_manager_.WriteOrBufferRetireConnectionId(sequence_number); }
diff --git a/quiche/quic/core/quic_session_test.cc b/quiche/quic/core/quic_session_test.cc index 1827f30..fd3ea97 100644 --- a/quiche/quic/core/quic_session_test.cc +++ b/quiche/quic/core/quic_session_test.cc
@@ -2133,6 +2133,9 @@ } TEST_P(QuicSessionTestClient, NewStreamCreationResumesMultiPortProbing) { + if (!VersionHasIetfQuicFrames(transport_version())) { + return; + } session_.config()->SetClientConnectionOptions({kMPQC}); session_.Initialize(); connection_->CreateConnectionIdManager(); @@ -2140,10 +2143,6 @@ connection_->OnHandshakeComplete(); session_.OnConfigNegotiated(); - if (!connection_->connection_migration_use_new_cid()) { - return; - } - EXPECT_CALL(*connection_, MaybeProbeMultiPortPath()); session_.CreateOutgoingBidirectionalStream(); }