From b395f5bd5c5be7a3b54fc85ca482759a56a0cd6e Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 25 Oct 2022 13:53:41 +0200 Subject: [PATCH] move relay server priority assignment to port_allocator which knows more about the internals of ICE. Remove the relay server config priority field which was used to specify the relative priority of TURN servers. This is now handled internally by CreateRelayPortArgs without being exposed. Also rename BasicPortAllocator::AddTurnServer to BasicPortAllocator::AddTurnServerForTesting since it is a test-only method. BUG=webrtc:13195,webrtc:14539 Change-Id: Id36cbf0187b7a84d1a9b53860f31994f3c7589f0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280224 Commit-Queue: Philipp Hancke Reviewed-by: Harald Alvestrand Reviewed-by: Jonas Oreland Cr-Commit-Position: refs/heads/main@{#38520} --- p2p/base/p2p_transport_channel_unittest.cc | 6 ++--- p2p/base/port_allocator.h | 4 +-- p2p/base/turn_port.h | 4 +-- p2p/client/basic_port_allocator.cc | 13 +++++++--- p2p/client/basic_port_allocator.h | 8 +++--- p2p/client/basic_port_allocator_unittest.cc | 27 +++++++++++++++++++-- p2p/client/relay_port_factory_interface.h | 4 +++ pc/ice_server_parsing.cc | 7 ------ pc/ice_server_parsing_unittest.cc | 18 -------------- 9 files changed, 48 insertions(+), 43 deletions(-) diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index d3fad7e9aa..c1df1d7520 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -2737,8 +2737,8 @@ TEST_P(P2PTransportChannelMultihomedTest, TestFailoverWithManyConnections) { RelayServerConfig turn_server; turn_server.credentials = kRelayCredentials; turn_server.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP)); - GetAllocator(0)->AddTurnServer(turn_server); - GetAllocator(1)->AddTurnServer(turn_server); + GetAllocator(0)->AddTurnServerForTesting(turn_server); + GetAllocator(1)->AddTurnServerForTesting(turn_server); // Enable IPv6 SetAllocatorFlags( 0, PORTALLOCATOR_ENABLE_IPV6 | PORTALLOCATOR_ENABLE_IPV6_ON_WIFI); @@ -5238,7 +5238,7 @@ TEST_P(P2PTransportChannelMostLikelyToWorkFirstTest, TestTcpTurn) { RelayServerConfig config; config.credentials = kRelayCredentials; config.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP)); - allocator()->AddTurnServer(config); + allocator()->AddTurnServerForTesting(config); P2PTransportChannel& ch = StartTransportChannel(true, 500, &field_trials_); EXPECT_TRUE_WAIT(ch.ports().size() == 3, kDefaultTimeout); diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index 46358439ac..3ca63cbd41 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -171,14 +171,12 @@ struct RTC_EXPORT RelayServerConfig { ~RelayServerConfig(); bool operator==(const RelayServerConfig& o) const { - return ports == o.ports && credentials == o.credentials && - priority == o.priority; + return ports == o.ports && credentials == o.credentials; } bool operator!=(const RelayServerConfig& o) const { return !(*this == o); } PortList ports; RelayCredentials credentials; - int priority = 0; TlsCertPolicy tls_cert_policy = TlsCertPolicy::TLS_CERT_POLICY_SECURE; std::vector tls_alpn_protocols; std::vector tls_elliptic_curves; diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h index d0b2079465..0672931d1c 100644 --- a/p2p/base/turn_port.h +++ b/p2p/base/turn_port.h @@ -81,7 +81,7 @@ class TurnPort : public Port { return absl::WrapUnique( new TurnPort(args.network_thread, args.socket_factory, args.network, socket, args.username, args.password, *args.server_address, - args.config->credentials, args.config->priority, + args.config->credentials, args.relative_priority, args.config->tls_alpn_protocols, args.config->tls_elliptic_curves, args.turn_customizer, args.config->tls_cert_verifier, args.field_trials)); @@ -100,7 +100,7 @@ class TurnPort : public Port { new TurnPort(args.network_thread, args.socket_factory, args.network, min_port, max_port, args.username, args.password, *args.server_address, args.config->credentials, - args.config->priority, args.config->tls_alpn_protocols, + args.relative_priority, args.config->tls_alpn_protocols, args.config->tls_elliptic_curves, args.turn_customizer, args.config->tls_cert_verifier, args.field_trials)); } diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index 5611403bb2..e36f266f15 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -298,7 +298,8 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal( return session; } -void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) { +void BasicPortAllocator::AddTurnServerForTesting( + const RelayServerConfig& turn_server) { CheckRunOnValidThreadAndInitialized(); std::vector new_turn_servers = turn_servers(); new_turn_servers.push_back(turn_server); @@ -1647,12 +1648,17 @@ void AllocationSequence::CreateRelayPorts() { return; } + // Relative priority of candidates from this TURN server in relation + // to the candidates from other servers. Required because ICE priorities + // need to be unique. + int relative_priority = config_->relays.size(); for (RelayServerConfig& relay : config_->relays) { - CreateTurnPort(relay); + CreateTurnPort(relay, relative_priority--); } } -void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) { +void AllocationSequence::CreateTurnPort(const RelayServerConfig& config, + int relative_priority) { PortList::const_iterator relay_port; for (relay_port = config.ports.begin(); relay_port != config.ports.end(); ++relay_port) { @@ -1685,6 +1691,7 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) { args.config = &config; args.turn_customizer = session_->allocator()->turn_customizer(); args.field_trials = session_->allocator()->field_trials(); + args.relative_priority = relative_priority; std::unique_ptr port; // Shared socket mode must be enabled only for UDP based ports. Hence diff --git a/p2p/client/basic_port_allocator.h b/p2p/client/basic_port_allocator.h index 173d789545..38c3835ee8 100644 --- a/p2p/client/basic_port_allocator.h +++ b/p2p/client/basic_port_allocator.h @@ -79,7 +79,7 @@ class RTC_EXPORT BasicPortAllocator : public PortAllocator { absl::string_view ice_pwd) override; // Convenience method that adds a TURN server to the configuration. - void AddTurnServer(const RelayServerConfig& turn_server); + void AddTurnServerForTesting(const RelayServerConfig& turn_server); RelayPortFactoryInterface* relay_port_factory() { CheckRunOnValidThreadIfInitialized(); @@ -386,11 +386,9 @@ class AllocationSequence : public sigslot::has_slots<> { void Start(); void Stop(); - protected: - // For testing. - void CreateTurnPort(const RelayServerConfig& config); - private: + void CreateTurnPort(const RelayServerConfig& config, int relative_priority); + typedef std::vector ProtocolList; void Process(int epoch); diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc index 0e32bc7a32..d1a91afd63 100644 --- a/p2p/client/basic_port_allocator_unittest.cc +++ b/p2p/client/basic_port_allocator_unittest.cc @@ -251,7 +251,7 @@ class BasicPortAllocatorTestBase : public ::testing::Test, void AddTurnServers(const rtc::SocketAddress& udp_turn, const rtc::SocketAddress& tcp_turn) { RelayServerConfig turn_server = CreateTurnServers(udp_turn, tcp_turn); - allocator_->AddTurnServer(turn_server); + allocator_->AddTurnServerForTesting(turn_server); } bool CreateSession(int component) { @@ -1818,7 +1818,7 @@ TEST_F(BasicPortAllocatorTestWithRealClock, turn_server.credentials = credentials; turn_server.ports.push_back( ProtocolAddress(rtc::SocketAddress("localhost", 3478), PROTO_UDP)); - allocator_->AddTurnServer(turn_server); + allocator_->AddTurnServerForTesting(turn_server); allocator_->set_step_delay(kMinimumStepDelay); allocator_->set_flags(allocator().flags() | @@ -2496,6 +2496,29 @@ TEST_F(BasicPortAllocatorTest, TestDoNotUseTurnServerAsStunSever) { EXPECT_EQ(1U, port_config.StunServers().size()); } +// Test that candidates from different servers get assigned a unique local +// preference (the middle 16 bits of the priority) +TEST_F(BasicPortAllocatorTest, AssignsUniqueLocalPreferencetoRelayCandidates) { + allocator_->SetCandidateFilter(CF_RELAY); + allocator_->AddTurnServerForTesting( + CreateTurnServers(kTurnUdpIntAddr, SocketAddress())); + allocator_->AddTurnServerForTesting( + CreateTurnServers(kTurnUdpIntAddr, SocketAddress())); + allocator_->AddTurnServerForTesting( + CreateTurnServers(kTurnUdpIntAddr, SocketAddress())); + + AddInterface(kClientAddr); + ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); + session_->StartGettingPorts(); + ASSERT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, + kDefaultAllocationTimeout, fake_clock); + EXPECT_EQ(3u, candidates_.size()); + EXPECT_GT((candidates_[0].priority() >> 8) & 0xFFFF, + (candidates_[1].priority() >> 8) & 0xFFFF); + EXPECT_GT((candidates_[1].priority() >> 8) & 0xFFFF, + (candidates_[2].priority() >> 8) & 0xFFFF); +} + // Test that no more than allocator.max_ipv6_networks() IPv6 networks are used // to gather candidates. TEST_F(BasicPortAllocatorTest, TwoIPv6AreSelectedBecauseOfMaxIpv6Limit) { diff --git a/p2p/client/relay_port_factory_interface.h b/p2p/client/relay_port_factory_interface.h index 4eec5dbf28..edfca3697b 100644 --- a/p2p/client/relay_port_factory_interface.h +++ b/p2p/client/relay_port_factory_interface.h @@ -45,6 +45,10 @@ struct CreateRelayPortArgs { std::string password; webrtc::TurnCustomizer* turn_customizer = nullptr; const webrtc::FieldTrialsView* field_trials = nullptr; + // Relative priority of candidates from this TURN server in relation + // to the candidates from other servers. Required because ICE priorities + // need to be unique. + int relative_priority = 0; }; // A factory for creating RelayPort's. diff --git a/pc/ice_server_parsing.cc b/pc/ice_server_parsing.cc index 1a30d2def5..9322fd12d4 100644 --- a/pc/ice_server_parsing.cc +++ b/pc/ice_server_parsing.cc @@ -338,13 +338,6 @@ RTCError ParseIceServersOrError( "ICE server parsing failed: Empty uri."); } } - // Candidates must have unique priorities, so that connectivity checks - // are performed in a well-defined order. - int priority = static_cast(turn_servers->size() - 1); - for (cricket::RelayServerConfig& turn_server : *turn_servers) { - // First in the list gets highest priority. - turn_server.priority = priority--; - } return RTCError::OK(); } diff --git a/pc/ice_server_parsing_unittest.cc b/pc/ice_server_parsing_unittest.cc index 408c790346..4cb7c47b0b 100644 --- a/pc/ice_server_parsing_unittest.cc +++ b/pc/ice_server_parsing_unittest.cc @@ -237,22 +237,4 @@ TEST_F(IceServerParsingTest, ParseMultipleUrls) { EXPECT_EQ(1U, turn_servers_.size()); } -// Ensure that TURN servers are given unique priorities, -// so that their resulting candidates have unique priorities. -TEST_F(IceServerParsingTest, TurnServerPrioritiesUnique) { - PeerConnectionInterface::IceServers servers; - PeerConnectionInterface::IceServer server; - server.urls.push_back("turn:hostname"); - server.urls.push_back("turn:hostname2"); - server.username = "foo"; - server.password = "bar"; - servers.push_back(server); - - EXPECT_TRUE( - webrtc::ParseIceServersOrError(servers, &stun_servers_, &turn_servers_) - .ok()); - EXPECT_EQ(2U, turn_servers_.size()); - EXPECT_NE(turn_servers_[0].priority, turn_servers_[1].priority); -} - } // namespace webrtc