diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index eaa41a5d2e..3ba2558809 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -2270,10 +2270,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestBasic) { TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { rtc::ScopedFakeClock clock; AddAddress(0, kPublicAddrs[0]); - // Adding alternate address will make sure |kPublicAddrs| has the higher - // priority than others. This is due to FakeNetwork::AddInterface method. - AddAddress(1, kAlternateAddrs[1]); - AddAddress(1, kPublicAddrs[1]); + // Simulate failing over from Wi-Fi to cell interface. + AddAddress(1, kPublicAddrs[1], "eth0", rtc::ADAPTER_TYPE_WIFI); + AddAddress(1, kAlternateAddrs[1], "wlan0", rtc::ADAPTER_TYPE_CELLULAR); // Use only local ports for simplicity. SetAllocatorFlags(0, kOnlyLocalPorts); @@ -2322,10 +2321,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { // The controlling side has two interfaces and one will die. TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { rtc::ScopedFakeClock clock; - // Adding alternate address will make sure |kPublicAddrs| has the higher - // priority than others. This is due to FakeNetwork::AddInterface method. - AddAddress(0, kAlternateAddrs[0]); - AddAddress(0, kPublicAddrs[0]); + // Simulate failing over from Wi-Fi to cell interface. + AddAddress(0, kPublicAddrs[0], "eth0", rtc::ADAPTER_TYPE_WIFI); + AddAddress(0, kAlternateAddrs[0], "wlan0", rtc::ADAPTER_TYPE_CELLULAR); AddAddress(1, kPublicAddrs[1]); // Use only local ports for simplicity. @@ -2469,10 +2467,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverWithManyConnections) { // increase. TEST_F(P2PTransportChannelMultihomedTest, TestIceRenomination) { rtc::ScopedFakeClock clock; - // Adding alternate address will make sure |kPublicAddrs| has the higher - // priority than others. This is due to FakeNetwork::AddInterface method. - AddAddress(0, kAlternateAddrs[0]); - AddAddress(0, kPublicAddrs[0]); + // Simulate failing over from Wi-Fi to cell interface. + AddAddress(0, kPublicAddrs[0], "eth0", rtc::ADAPTER_TYPE_WIFI); + AddAddress(0, kAlternateAddrs[0], "wlan0", rtc::ADAPTER_TYPE_CELLULAR); AddAddress(1, kPublicAddrs[1]); // Use only local ports for simplicity. @@ -2530,10 +2527,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestConnectionSwitchDampeningControlledSide) { rtc::ScopedFakeClock clock; AddAddress(0, kPublicAddrs[0]); - // Adding alternate address will make sure |kPublicAddrs| has the higher - // priority than others. This is due to FakeNetwork::AddInterface method. - AddAddress(1, kAlternateAddrs[1]); - AddAddress(1, kPublicAddrs[1]); + // Simulate failing over from Wi-Fi to cell interface. + AddAddress(1, kPublicAddrs[1], "eth0", rtc::ADAPTER_TYPE_WIFI); + AddAddress(1, kAlternateAddrs[1], "wlan0", rtc::ADAPTER_TYPE_CELLULAR); // Use only local ports for simplicity. SetAllocatorFlags(0, kOnlyLocalPorts); @@ -2589,10 +2585,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TEST_F(P2PTransportChannelMultihomedTest, TestConnectionSwitchDampeningControllingSide) { rtc::ScopedFakeClock clock; - // Adding alternate address will make sure |kPublicAddrs| has the higher - // priority than others. This is due to FakeNetwork::AddInterface method. - AddAddress(0, kAlternateAddrs[0]); - AddAddress(0, kPublicAddrs[0]); + // Simulate failing over from Wi-Fi to cell interface. + AddAddress(0, kPublicAddrs[0], "eth0", rtc::ADAPTER_TYPE_WIFI); + AddAddress(0, kAlternateAddrs[0], "wlan0", rtc::ADAPTER_TYPE_CELLULAR); AddAddress(1, kPublicAddrs[1]); // Use only local ports for simplicity. diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h index 9540e40d74..45a941a9b7 100644 --- a/webrtc/p2p/base/portallocator.h +++ b/webrtc/p2p/base/portallocator.h @@ -103,6 +103,12 @@ const uint32_t kDefaultStepDelay = 1000; // 1 sec step delay. // internal. Less than 20ms is not acceptable. We choose 50ms as our default. const uint32_t kMinimumStepDelay = 50; +// Turning on IPv6 could make many IPv6 interfaces available for connectivity +// check and delay the call setup time. kDefaultMaxIPv6Networks is the default +// upper limit of IPv6 networks but could be changed by +// set_max_ipv6_networks(). +constexpr int kDefaultMaxIPv6Networks = 5; + // CF = CANDIDATE FILTER enum { CF_NONE = 0x0, @@ -324,14 +330,14 @@ class PortAllocatorSession : public sigslot::has_slots<> { // passing it into an object that uses it on a different thread. class PortAllocator : public sigslot::has_slots<> { public: - PortAllocator() : - flags_(kDefaultPortAllocatorFlags), - min_port_(0), - max_port_(0), - step_delay_(kDefaultStepDelay), - allow_tcp_listen_(true), - candidate_filter_(CF_ALL) { - } + PortAllocator() + : flags_(kDefaultPortAllocatorFlags), + min_port_(0), + max_port_(0), + max_ipv6_networks_(kDefaultMaxIPv6Networks), + step_delay_(kDefaultStepDelay), + allow_tcp_listen_(true), + candidate_filter_(CF_ALL) {} virtual ~PortAllocator() {} @@ -429,6 +435,17 @@ class PortAllocator : public sigslot::has_slots<> { return true; } + // Can be used to change the default numer of IPv6 network interfaces used + // (5). Can set to INT_MAX to effectively disable the limit. + // + // TODO(deadbeef): Applications shouldn't have to arbitrarily limit the + // number of available IPv6 network interfaces just because they could slow + // ICE down. We should work on making our ICE logic smarter (for example, + // prioritizing pinging connections that are most likely to work) so that + // every network interface can be used without impacting ICE's speed. + void set_max_ipv6_networks(int networks) { max_ipv6_networks_ = networks; } + int max_ipv6_networks() { return max_ipv6_networks_; } + uint32_t step_delay() const { return step_delay_; } void set_step_delay(uint32_t delay) { step_delay_ = delay; } @@ -472,6 +489,7 @@ class PortAllocator : public sigslot::has_slots<> { rtc::ProxyInfo proxy_; int min_port_; int max_port_; + int max_ipv6_networks_; uint32_t step_delay_; bool allow_tcp_listen_; uint32_t candidate_filter_; diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc index 2970987900..091819ac43 100644 --- a/webrtc/p2p/client/basicportallocator.cc +++ b/webrtc/p2p/client/basicportallocator.cc @@ -588,13 +588,14 @@ std::vector BasicPortAllocatorSession::GetNetworks() { network_manager->GetAnyAddressNetworks(&networks); } } + // Do some more filtering, depending on the network ignore mask and "disable + // costly networks" flag. networks.erase(std::remove_if(networks.begin(), networks.end(), [this](rtc::Network* network) { return allocator_->network_ignore_mask() & network->type(); }), networks.end()); - if (flags() & PORTALLOCATOR_DISABLE_COSTLY_NETWORKS) { uint16_t lowest_cost = rtc::kNetworkCostMax; for (rtc::Network* network : networks) { @@ -607,6 +608,26 @@ std::vector BasicPortAllocatorSession::GetNetworks() { }), networks.end()); } + // Lastly, if we have a limit for the number of IPv6 network interfaces (by + // default, it's 5), remove networks to ensure that limit is satisfied. + // + // TODO(deadbeef): Instead of just taking the first N arbitrary IPv6 + // networks, we could try to choose a set that's "most likely to work". It's + // hard to define what that means though; it's not just "lowest cost". + // Alternatively, we could just focus on making our ICE pinging logic smarter + // such that this filtering isn't necessary in the first place. + int ipv6_networks = 0; + for (auto it = networks.begin(); it != networks.end();) { + if ((*it)->prefix().family() == AF_INET6) { + if (ipv6_networks >= allocator_->max_ipv6_networks()) { + it = networks.erase(it); + continue; + } else { + ++ipv6_networks; + } + } + ++it; + } return networks; } diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc index 99e53cf619..638a9c8e87 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -55,6 +55,9 @@ static const SocketAddress kClientIPv6Addr("2401:fa00:4:1000:be30:5bff:fee5:c3", static const SocketAddress kClientIPv6Addr2( "2401:fa00:4:2000:be30:5bff:fee5:c3", 0); +static const SocketAddress kClientIPv6Addr3( + "2401:fa00:4:3000:be30:5bff:fee5:c3", + 0); static const SocketAddress kNatUdpAddr("77.77.77.77", rtc::NAT_SERVER_UDP_PORT); static const SocketAddress kNatTcpAddr("77.77.77.77", rtc::NAT_SERVER_TCP_PORT); static const SocketAddress kRemoteClientAddr("22.22.22.22", 0); @@ -799,6 +802,57 @@ TEST_F(BasicPortAllocatorTest, TestGatherLowCostNetworkOnly) { EXPECT_TRUE(addr_wifi.EqualIPs(candidates_[0].address())); } +// Test that no more than allocator.max_ipv6_networks() IPv6 networks are used +// to gather candidates. +TEST_F(BasicPortAllocatorTest, MaxIpv6NetworksLimitEnforced) { + // Add three IPv6 network interfaces, but tell the allocator to only use two. + allocator().set_max_ipv6_networks(2); + AddInterface(kClientIPv6Addr, "eth0", rtc::ADAPTER_TYPE_ETHERNET); + AddInterface(kClientIPv6Addr2, "eth1", rtc::ADAPTER_TYPE_ETHERNET); + AddInterface(kClientIPv6Addr3, "eth2", rtc::ADAPTER_TYPE_ETHERNET); + + // To simplify the test, only gather UDP host candidates. + allocator().set_flags(PORTALLOCATOR_ENABLE_IPV6 | PORTALLOCATOR_DISABLE_TCP | + PORTALLOCATOR_DISABLE_STUN | + PORTALLOCATOR_DISABLE_RELAY); + + EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); + session_->StartGettingPorts(); + EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, + kDefaultAllocationTimeout, fake_clock); + EXPECT_EQ(2U, candidates_.size()); + // Ensure the expected two interfaces (eth0 and eth1) were used. + EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", kClientIPv6Addr); + EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", kClientIPv6Addr2); +} + +// Ensure that allocator.max_ipv6_networks() doesn't prevent IPv4 networks from +// being used. +TEST_F(BasicPortAllocatorTest, MaxIpv6NetworksLimitDoesNotImpactIpv4Networks) { + // Set the "max IPv6" limit to 1, adding two IPv6 and two IPv4 networks. + allocator().set_max_ipv6_networks(1); + AddInterface(kClientIPv6Addr, "eth0", rtc::ADAPTER_TYPE_ETHERNET); + AddInterface(kClientIPv6Addr2, "eth1", rtc::ADAPTER_TYPE_ETHERNET); + AddInterface(kClientAddr, "eth2", rtc::ADAPTER_TYPE_ETHERNET); + AddInterface(kClientAddr2, "eth3", rtc::ADAPTER_TYPE_ETHERNET); + + // To simplify the test, only gather UDP host candidates. + allocator().set_flags(PORTALLOCATOR_ENABLE_IPV6 | PORTALLOCATOR_DISABLE_TCP | + PORTALLOCATOR_DISABLE_STUN | + PORTALLOCATOR_DISABLE_RELAY); + + EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); + session_->StartGettingPorts(); + EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, + kDefaultAllocationTimeout, fake_clock); + EXPECT_EQ(3U, candidates_.size()); + // Ensure that only one IPv6 interface was used, but both IPv4 interfaces + // were used. + EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", kClientIPv6Addr); + EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", kClientAddr); + EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", kClientAddr2); +} + // Test that we could use loopback interface as host candidate. TEST_F(BasicPortAllocatorTest, TestLoopbackNetworkInterface) { AddInterface(kLoopbackAddr, "test_loopback", rtc::ADAPTER_TYPE_LOOPBACK); diff --git a/webrtc/rtc_base/network.cc b/webrtc/rtc_base/network.cc index 1f4a705db0..26d6520e22 100644 --- a/webrtc/rtc_base/network.cc +++ b/webrtc/rtc_base/network.cc @@ -45,11 +45,6 @@ namespace rtc { namespace { -// Turning on IPv6 could make many IPv6 interfaces available for connectivity -// check and delay the call setup time. kMaxIPv6Networks is the default upper -// limit of IPv6 networks but could be changed by set_max_ipv6_networks(). -const int kMaxIPv6Networks = 5; - const uint32_t kUpdateNetworksMessage = 1; const uint32_t kSignalNetworksMessage = 2; @@ -93,7 +88,7 @@ bool SortNetworks(const Network* a, const Network* b) { // TODO(mallinath) - Add VPN and Link speed conditions while sorting. // Networks are sorted last by key. - return a->key() > b->key(); + return a->key() < b->key(); } std::string AdapterTypeToString(AdapterType type) { @@ -175,7 +170,6 @@ bool NetworkManager::GetDefaultLocalAddress(int family, IPAddress* addr) const { NetworkManagerBase::NetworkManagerBase() : enumeration_permission_(NetworkManager::ENUMERATION_ALLOWED), - max_ipv6_networks_(kMaxIPv6Networks), ipv6_enabled_(true) { } @@ -213,18 +207,8 @@ void NetworkManagerBase::GetAnyAddressNetworks(NetworkList* networks) { } void NetworkManagerBase::GetNetworks(NetworkList* result) const { - int ipv6_networks = 0; result->clear(); - for (Network* network : networks_) { - // Keep the number of IPv6 networks under |max_ipv6_networks_|. - if (network->prefix().family() == AF_INET6) { - if (ipv6_networks >= max_ipv6_networks_) { - continue; - } - ++ipv6_networks; - } - result->push_back(network); - } + result->insert(result->begin(), networks_.begin(), networks_.end()); } void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks, diff --git a/webrtc/rtc_base/network.h b/webrtc/rtc_base/network.h index 4cbbfefd42..ceb96c4760 100644 --- a/webrtc/rtc_base/network.h +++ b/webrtc/rtc_base/network.h @@ -147,13 +147,13 @@ class NetworkManagerBase : public NetworkManager { void GetNetworks(NetworkList* networks) const override; void GetAnyAddressNetworks(NetworkList* networks) override; + // Defaults to true. + // TODO(deadbeef): Remove this. Nothing but tests use this; IPv6 is enabled + // by default everywhere else. bool ipv6_enabled() const { return ipv6_enabled_; } void set_ipv6_enabled(bool enabled) { ipv6_enabled_ = enabled; } - void set_max_ipv6_networks(int networks) { max_ipv6_networks_ = networks; } - int max_ipv6_networks() { return max_ipv6_networks_; } - EnumerationPermission enumeration_permission() const override; bool GetDefaultLocalAddress(int family, IPAddress* ipaddr) const override; @@ -187,7 +187,6 @@ class NetworkManagerBase : public NetworkManager { EnumerationPermission enumeration_permission_; NetworkList networks_; - int max_ipv6_networks_; NetworkMap networks_map_; bool ipv6_enabled_; diff --git a/webrtc/rtc_base/network_unittest.cc b/webrtc/rtc_base/network_unittest.cc index a6c42ead37..521e3dc9ee 100644 --- a/webrtc/rtc_base/network_unittest.cc +++ b/webrtc/rtc_base/network_unittest.cc @@ -431,45 +431,6 @@ TEST_F(NetworkTest, TestIPv6MergeNetworkList) { } } -// Test that no more than manager.max_ipv6_networks() IPv6 networks get -// returned. -TEST_F(NetworkTest, TestIPv6MergeNetworkListTrimExcessive) { - BasicNetworkManager manager; - manager.SignalNetworksChanged.connect(static_cast(this), - &NetworkTest::OnNetworksChanged); - NetworkManager::NetworkList original_list; - - // Add twice the allowed number of IPv6 networks. - for (int i = 0; i < 2 * manager.max_ipv6_networks(); i++) { - // Make a network with different prefix length. - IPAddress ip; - EXPECT_TRUE(IPFromString("2401:fa01:4:1000:be30:faa:fee:faa", &ip)); - IPAddress prefix = TruncateIP(ip, 64 - i); - Network* ipv6_network = - new Network("test_eth0", "Test Network Adapter 1", prefix, 64 - i); - ipv6_network->AddIP(ip); - original_list.push_back(ipv6_network); - } - - // Add one IPv4 network. - Network* ipv4_network = new Network("test_eth0", "Test Network Adapter 1", - IPAddress(0x12345600U), 24); - ipv4_network->AddIP(IPAddress(0x12345600U)); - original_list.push_back(ipv4_network); - - bool changed = false; - MergeNetworkList(manager, original_list, &changed); - EXPECT_TRUE(changed); - NetworkManager::NetworkList list; - manager.GetNetworks(&list); - - // List size should be the max allowed IPv6 networks plus one IPv4 network. - EXPECT_EQ(manager.max_ipv6_networks() + 1, (int)list.size()); - - // Verify that the IPv4 network is in the list. - EXPECT_NE(list.end(), std::find(list.begin(), list.end(), ipv4_network)); -} - // Tests that when two network lists that describe the same set of networks are // merged, that the changed callback is not called, and that the original // objects remain in the result list. @@ -703,7 +664,9 @@ TEST_F(NetworkTest, MAYBE_TestIPv6Toggle) { } } -TEST_F(NetworkTest, TestNetworkListSorting) { +// Test that when network interfaces are sorted and given preference values, +// IPv6 comes first. +TEST_F(NetworkTest, IPv6NetworksPreferredOverIPv4) { BasicNetworkManager manager; Network ipv4_network1("test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); @@ -730,6 +693,29 @@ TEST_F(NetworkTest, TestNetworkListSorting) { EXPECT_TRUE(net1->preference() < net2->preference()); } +// When two interfaces are equivalent in everything but name, they're expected +// to be preference-ordered by name. For example, "eth0" before "eth1". +TEST_F(NetworkTest, NetworksSortedByInterfaceName) { + BasicNetworkManager manager; + Network* eth0 = new Network("test_eth0", "Test Network Adapter 1", + IPAddress(0x65432100U), 24); + eth0->AddIP(IPAddress(0x65432100U)); + Network* eth1 = new Network("test_eth1", "Test Network Adapter 2", + IPAddress(0x12345600U), 24); + eth1->AddIP(IPAddress(0x12345600U)); + NetworkManager::NetworkList list; + // Add them to the list in the opposite of the expected sorted order, to + // ensure sorting actually occurs. + list.push_back(eth1); + list.push_back(eth0); + + bool changed = false; + MergeNetworkList(manager, list, &changed); + ASSERT_TRUE(changed); + // "test_eth0" should be preferred over "test_eth1". + EXPECT_TRUE(eth0->preference() > eth1->preference()); +} + TEST_F(NetworkTest, TestNetworkAdapterTypes) { Network wifi("wlan0", "Wireless Adapter", IPAddress(0x12345600U), 24, ADAPTER_TYPE_WIFI);