From ad9561404c0cc007574eaedade581928aa3d24ef Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 21 Jul 2017 11:03:53 -0700 Subject: [PATCH] Move "max IPv6 networks" logic to BasicPortAllocator, and fix sorting. This CL moves the responsibility for restricting the number of IPv6 interfaces used for ICE to BasicPortAllocator. This is the right place to do it in the first place; it's where all the rest of the filtering occurs. And NetworkManager shouldn't need to know about ICE limitations; only the ICE classes should. Part of the reason I'm doing this is that I want to add a "max_ipv6_networks" API to RTCConfiguration, so that applications can override the default easily (see linked bug). But that means that PeerConnection would need to be able to call "set_max_ipv6_networks" on the underlying object that does the filtering, and that method isn't available on the "NetworkManager" base class. So rather than adding another method to a place it doesn't belong, I'm moving it to the place it does belong. In the process, I noticed that "CompareNetworks" is inconsistent with "SortNetworks"; the former orders interfaces alphabetically, and the latter reverse-alphabetically. I believe this was unintentional, and results in undesirable behavior (like "eth1" being preferred over "eth0"), so I'm fixing it and adding a test. BUG=webrtc:7703 Review-Url: https://codereview.webrtc.org/2983213002 Cr-Commit-Position: refs/heads/master@{#19112} --- .../p2p/base/p2ptransportchannel_unittest.cc | 35 +++++----- webrtc/p2p/base/portallocator.h | 34 +++++++--- webrtc/p2p/client/basicportallocator.cc | 23 ++++++- .../p2p/client/basicportallocator_unittest.cc | 54 +++++++++++++++ webrtc/rtc_base/network.cc | 20 +----- webrtc/rtc_base/network.h | 7 +- webrtc/rtc_base/network_unittest.cc | 66 ++++++++----------- 7 files changed, 148 insertions(+), 91 deletions(-) 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);