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