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 {
