Do not update peer address on the client side. Also move IsKnownServerAddress logic from QuicSpdyClientSession to QuicConnection. PiperOrigin-RevId: 496954701
diff --git a/quiche/quic/core/http/quic_spdy_client_session.cc b/quiche/quic/core/http/quic_spdy_client_session.cc index 99b25c2..b6fa05d 100644 --- a/quiche/quic/core/http/quic_spdy_client_session.cc +++ b/quiche/quic/core/http/quic_spdy_client_session.cc
@@ -100,18 +100,6 @@ return crypto_stream_.get(); } -bool QuicSpdyClientSession::IsKnownServerAddress( - const QuicSocketAddress& address) const { - return std::find(known_server_addresses_.cbegin(), - known_server_addresses_.cend(), - address) != known_server_addresses_.cend(); -} - -void QuicSpdyClientSession::AddKnownServerAddress( - const QuicSocketAddress& address) { - known_server_addresses_.push_back(address); -} - void QuicSpdyClientSession::CryptoConnect() { QUICHE_DCHECK(flow_controller()); crypto_stream_->CryptoConnect();
diff --git a/quiche/quic/core/http/quic_spdy_client_session.h b/quiche/quic/core/http/quic_spdy_client_session.h index ceab083..df97adc 100644 --- a/quiche/quic/core/http/quic_spdy_client_session.h +++ b/quiche/quic/core/http/quic_spdy_client_session.h
@@ -42,9 +42,6 @@ QuicSpdyClientStream* CreateOutgoingUnidirectionalStream() override; QuicCryptoClientStreamBase* GetMutableCryptoStream() override; const QuicCryptoClientStreamBase* GetCryptoStream() const override; - bool IsKnownServerAddress(const QuicSocketAddress& address) const override; - - void AddKnownServerAddress(const QuicSocketAddress& address); bool IsAuthorized(const std::string& authority) override; @@ -114,8 +111,6 @@ std::unique_ptr<QuicCryptoClientStreamBase> crypto_stream_; QuicServerId server_id_; QuicCryptoClientConfig* crypto_config_; - // Server addresses that are known to the client. - std::vector<QuicSocketAddress> known_server_addresses_; // If this is set to false, the client will ignore server GOAWAYs and allow // the creation of streams regardless of the high chance they will fail.
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index 82283f5..0ad3770 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -384,6 +384,9 @@ blackhole_detection_disabled_ = true; } } + if (perspective_ == Perspective::IS_CLIENT) { + AddKnownServerAddress(initial_peer_address); + } packet_creator_.SetDefaultPeerAddress(initial_peer_address); } @@ -1213,12 +1216,16 @@ if (perspective_ == Perspective::IS_CLIENT) { if (!GetLargestReceivedPacket().IsInitialized() || header.packet_number > GetLargestReceivedPacket()) { - // Update direct_peer_address_ and default path peer_address immediately - // for client connections. - // TODO(fayang): only change peer addresses in application data packet - // number space. - UpdatePeerAddress(last_received_packet_info_.source_address); - default_path_.peer_address = GetEffectivePeerAddressFromCurrentPacket(); + if (version().HasIetfQuicFrames()) { + // Do not update server address. + } else { + // Update direct_peer_address_ and default path peer_address immediately + // for client connections. + // TODO(fayang): only change peer addresses in application data packet + // number space. + UpdatePeerAddress(last_received_packet_info_.source_address); + default_path_.peer_address = GetEffectivePeerAddressFromCurrentPacket(); + } } } else { // At server, remember the address change type of effective_peer_address @@ -2651,6 +2658,9 @@ } if (!direct_peer_address_.IsInitialized()) { + if (perspective_ == Perspective::IS_CLIENT) { + AddKnownServerAddress(last_received_packet_info_.source_address); + } UpdatePeerAddress(last_received_packet_info_.source_address); } @@ -2894,8 +2904,7 @@ direct_peer_address_.IsInitialized() && last_received_packet_info_.source_address.IsInitialized() && direct_peer_address_ != last_received_packet_info_.source_address && - !visitor_->IsKnownServerAddress( - last_received_packet_info_.source_address)) { + !IsKnownServerAddress(last_received_packet_info_.source_address)) { // TODO(haoyuewang) Revisit this when preferred_address transport parameter // is used on the client side. // Discard packets received from unseen server addresses. @@ -4004,6 +4013,14 @@ } } +bool QuicConnection::IsKnownServerAddress( + const QuicSocketAddress& address) const { + QUICHE_DCHECK(address.IsInitialized()); + return std::find(known_server_addresses_.cbegin(), + known_server_addresses_.cend(), + address) != known_server_addresses_.cend(); +} + void QuicConnection::OnRetransmissionTimeout() { ScopedRetransmissionTimeoutIndicator indicator(this); #ifndef NDEBUG @@ -5309,6 +5326,9 @@ bool QuicConnection::UpdatePacketContent(QuicFrameType type) { last_received_packet_info_.frames.push_back(type); if (version().HasIetfQuicFrames()) { + if (perspective_ == Perspective::IS_CLIENT) { + return connected_; + } if (!QuicUtils::IsProbingFrame(type)) { MaybeStartIetfPeerMigration(); return connected_; @@ -6318,6 +6338,14 @@ return std::max(min_delay, blackhole_delay); } +void QuicConnection::AddKnownServerAddress(const QuicSocketAddress& address) { + QUICHE_DCHECK(perspective_ == Perspective::IS_CLIENT); + if (!address.IsInitialized() || IsKnownServerAddress(address)) { + return; + } + known_server_addresses_.push_back(address); +} + bool QuicConnection::ShouldDetectBlackhole() const { if (!connected_ || blackhole_detection_disabled_) { return false;
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index bb5188d..02a218b 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -234,9 +234,6 @@ // Return false if the crypto stream fail to generate one. virtual bool MaybeSendAddressToken() = 0; - // Whether the server address is known to the connection. - virtual bool IsKnownServerAddress(const QuicSocketAddress& address) const = 0; - // When bandwidth update alarms. virtual void OnBandwidthUpdateTimeout() = 0; @@ -1265,6 +1262,8 @@ void DisableLivenessTesting() { liveness_testing_disabled_ = true; } + void AddKnownServerAddress(const QuicSocketAddress& address); + protected: // Calls cancel() on all the alarms owned by this connection. void CancelAllAlarms(); @@ -1898,6 +1897,9 @@ EncryptionLevel GetEncryptionLevelToSendPingForSpace( PacketNumberSpace space) const; + // Returns true if |address| is known server address. + bool IsKnownServerAddress(const QuicSocketAddress& address) const; + QuicConnectionContext context_; QuicFramer framer_; @@ -2265,6 +2267,9 @@ RetransmittableOnWireBehavior retransmittable_on_wire_behavior_ = DEFAULT; + // Server addresses that are known to the client. + std::vector<QuicSocketAddress> known_server_addresses_; + // If true, throttle sending if next created packet will exceed amplification // limit. const bool enforce_strict_amplification_factor_ =
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index b9d4dd8..94aa39e 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -2836,18 +2836,18 @@ QuicSocketAddress()); EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); - if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { - EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); + if (connection_.version().HasIetfQuicFrames()) { + // Verify the 2nd packet from unknown server address gets dropped. + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(1); + } else if (QuicVersionUsesCryptoFrames(connection_.transport_version())) { + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(2); } else { - EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(2); } ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, kPeerAddress, ENCRYPTION_INITIAL); EXPECT_EQ(kPeerAddress, connection_.peer_address()); EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); - - // Process another packet with a different peer address on client side will - // only update peer address. const QuicSocketAddress kNewPeerAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); @@ -2863,6 +2863,48 @@ } } +TEST_P(QuicConnectionTest, ServerAddressChangesToKnownAddress) { + if (!connection_.version().HasIetfQuicFrames()) { + return; + } + EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); + set_perspective(Perspective::IS_CLIENT); + EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective()); + + // Clear direct_peer_address. + QuicConnectionPeer::SetDirectPeerAddress(&connection_, QuicSocketAddress()); + // Clear effective_peer_address, it is the same as direct_peer_address for + // this test. + QuicConnectionPeer::SetEffectivePeerAddress(&connection_, + QuicSocketAddress()); + EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); + + // Verify all 3 packets get processed. + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(3); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, kPeerAddress, + ENCRYPTION_INITIAL); + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); + + // Process another packet with a different but known server address. + const QuicSocketAddress kNewPeerAddress = + QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); + connection_.AddKnownServerAddress(kNewPeerAddress); + EXPECT_CALL(visitor_, OnConnectionMigration(_)).Times(0); + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, + kNewPeerAddress, ENCRYPTION_INITIAL); + // Verify peer address does not change. + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); + + // Process 3rd packet from previous server address. + ProcessFramePacketWithAddresses(MakeCryptoFrame(), kSelfAddress, kPeerAddress, + ENCRYPTION_INITIAL); + // Verify peer address does not change. + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); +} + TEST_P(QuicConnectionTest, MaxPacketSize) { EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective()); EXPECT_EQ(1250u, connection_.max_packet_length());
diff --git a/quiche/quic/core/quic_session.h b/quiche/quic/core/quic_session.h index ecdabf9..b2b8403 100644 --- a/quiche/quic/core/quic_session.h +++ b/quiche/quic/core/quic_session.h
@@ -174,10 +174,6 @@ void BeforeConnectionCloseSent() override {} bool ValidateToken(absl::string_view token) override; bool MaybeSendAddressToken() override; - bool IsKnownServerAddress( - const QuicSocketAddress& /*address*/) const override { - return false; - } void OnBandwidthUpdateTimeout() override {} std::unique_ptr<QuicPathValidationContext> CreateContextForMultiPortPath() override {
diff --git a/quiche/quic/test_tools/quic_test_client.cc b/quiche/quic/test_tools/quic_test_client.cc index 17e1d73..ecf67c4 100644 --- a/quiche/quic/test_tools/quic_test_client.cc +++ b/quiche/quic/test_tools/quic_test_client.cc
@@ -268,7 +268,7 @@ void MockableQuicClient::set_peer_address(const QuicSocketAddress& address) { mockable_network_helper()->set_peer_address(address); if (client_session() != nullptr) { - client_session()->AddKnownServerAddress(address); + client_session()->connection()->AddKnownServerAddress(address); } }
diff --git a/quiche/quic/test_tools/quic_test_utils.h b/quiche/quic/test_tools/quic_test_utils.h index 0d4aa8b..35f51a8 100644 --- a/quiche/quic/test_tools/quic_test_utils.h +++ b/quiche/quic/test_tools/quic_test_utils.h
@@ -503,11 +503,6 @@ MOCK_METHOD(std::unique_ptr<QuicPathValidationContext>, CreateContextForMultiPortPath, (), (override)); - bool IsKnownServerAddress( - const QuicSocketAddress& /*address*/) const override { - return false; - } - void OnBandwidthUpdateTimeout() override {} };
diff --git a/quiche/quic/test_tools/simulator/quic_endpoint.h b/quiche/quic/test_tools/simulator/quic_endpoint.h index 99b3915..03e86f3 100644 --- a/quiche/quic/test_tools/simulator/quic_endpoint.h +++ b/quiche/quic/test_tools/simulator/quic_endpoint.h
@@ -104,10 +104,6 @@ void BeforeConnectionCloseSent() override {} bool ValidateToken(absl::string_view /*token*/) override { return true; } bool MaybeSendAddressToken() override { return false; } - bool IsKnownServerAddress( - const QuicSocketAddress& /*address*/) const override { - return false; - } void OnBandwidthUpdateTimeout() override {} std::unique_ptr<QuicPathValidationContext> CreateContextForMultiPortPath() override {