From 817c8af52ac12227fc4603f908443dcde33fb186 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 21 Jul 2017 12:59:46 -0700 Subject: [PATCH] Revert of Move "max IPv6 networks" logic to BasicPortAllocator, and fix sorting. (patchset #2 id:20001 of https://codereview.webrtc.org/2983213002/ ) Reason for revert: Breaks IpcNetworkManagerTest.TestMergeNetworkList, because it has built-in assumptions about network ordering that it shouldn't have. Will reland after fixing that test. Original issue's description: > 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} > Committed: https://chromium.googlesource.com/external/webrtc/+/ad9561404c0cc007574eaedade581928aa3d24ef TBR=zhihuang@webrtc.org,pthatcher@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:7703 Review-Url: https://codereview.webrtc.org/2984853002 Cr-Commit-Position: refs/heads/master@{#19114} --- .../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, 91 insertions(+), 148 deletions(-) diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 3ba2558809..eaa41a5d2e 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -2270,9 +2270,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestBasic) { TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { rtc::ScopedFakeClock clock; AddAddress(0, kPublicAddrs[0]); - // 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); + // 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]); // Use only local ports for simplicity. SetAllocatorFlags(0, kOnlyLocalPorts); @@ -2321,9 +2322,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { // The controlling side has two interfaces and one will die. TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { rtc::ScopedFakeClock clock; - // 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); + // 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]); AddAddress(1, kPublicAddrs[1]); // Use only local ports for simplicity. @@ -2467,9 +2469,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverWithManyConnections) { // increase. TEST_F(P2PTransportChannelMultihomedTest, TestIceRenomination) { rtc::ScopedFakeClock clock; - // 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); + // 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]); AddAddress(1, kPublicAddrs[1]); // Use only local ports for simplicity. @@ -2527,9 +2530,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestConnectionSwitchDampeningControlledSide) { rtc::ScopedFakeClock clock; AddAddress(0, kPublicAddrs[0]); - // 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); + // 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]); // Use only local ports for simplicity. SetAllocatorFlags(0, kOnlyLocalPorts); @@ -2585,9 +2589,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TEST_F(P2PTransportChannelMultihomedTest, TestConnectionSwitchDampeningControllingSide) { rtc::ScopedFakeClock clock; - // 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); + // 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]); 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 45a941a9b7..9540e40d74 100644 --- a/webrtc/p2p/base/portallocator.h +++ b/webrtc/p2p/base/portallocator.h @@ -103,12 +103,6 @@ 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, @@ -330,14 +324,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), - max_ipv6_networks_(kDefaultMaxIPv6Networks), - step_delay_(kDefaultStepDelay), - allow_tcp_listen_(true), - candidate_filter_(CF_ALL) {} + PortAllocator() : + flags_(kDefaultPortAllocatorFlags), + min_port_(0), + max_port_(0), + step_delay_(kDefaultStepDelay), + allow_tcp_listen_(true), + candidate_filter_(CF_ALL) { + } virtual ~PortAllocator() {} @@ -435,17 +429,6 @@ 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; } @@ -489,7 +472,6 @@ 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 091819ac43..2970987900 100644 --- a/webrtc/p2p/client/basicportallocator.cc +++ b/webrtc/p2p/client/basicportallocator.cc @@ -588,14 +588,13 @@ 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) { @@ -608,26 +607,6 @@ 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 638a9c8e87..99e53cf619 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -55,9 +55,6 @@ 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); @@ -802,57 +799,6 @@ 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 26d6520e22..1f4a705db0 100644 --- a/webrtc/rtc_base/network.cc +++ b/webrtc/rtc_base/network.cc @@ -45,6 +45,11 @@ 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; @@ -88,7 +93,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) { @@ -170,6 +175,7 @@ bool NetworkManager::GetDefaultLocalAddress(int family, IPAddress* addr) const { NetworkManagerBase::NetworkManagerBase() : enumeration_permission_(NetworkManager::ENUMERATION_ALLOWED), + max_ipv6_networks_(kMaxIPv6Networks), ipv6_enabled_(true) { } @@ -207,8 +213,18 @@ void NetworkManagerBase::GetAnyAddressNetworks(NetworkList* networks) { } void NetworkManagerBase::GetNetworks(NetworkList* result) const { + int ipv6_networks = 0; result->clear(); - result->insert(result->begin(), networks_.begin(), networks_.end()); + 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); + } } void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks, diff --git a/webrtc/rtc_base/network.h b/webrtc/rtc_base/network.h index ceb96c4760..4cbbfefd42 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,6 +187,7 @@ 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 521e3dc9ee..a6c42ead37 100644 --- a/webrtc/rtc_base/network_unittest.cc +++ b/webrtc/rtc_base/network_unittest.cc @@ -431,6 +431,45 @@ 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. @@ -664,9 +703,7 @@ TEST_F(NetworkTest, MAYBE_TestIPv6Toggle) { } } -// Test that when network interfaces are sorted and given preference values, -// IPv6 comes first. -TEST_F(NetworkTest, IPv6NetworksPreferredOverIPv4) { +TEST_F(NetworkTest, TestNetworkListSorting) { BasicNetworkManager manager; Network ipv4_network1("test_eth0", "Test Network Adapter 1", IPAddress(0x12345600U), 24); @@ -693,29 +730,6 @@ TEST_F(NetworkTest, IPv6NetworksPreferredOverIPv4) { 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);