Add config to prune TURN ports

webrtc/p2p/client/basicportallocator.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 29 matching lines @@
   MSG_CONFIG_STOP,
 };
 
 const int PHASE_UDP = 0;
 const int PHASE_RELAY = 1;
 const int PHASE_TCP = 2;
 const int PHASE_SSLTCP = 3;
 
 const int kNumPhases = 4;
 
+enum class AddressFamily { UNSPEC, IPV4, IPV6 };
+
+AddressFamily ToAddressFamily(int ip_family) {
+  switch (ip_family) {
+    case AF_UNSPEC:
+      return AddressFamily::UNSPEC;
+    case AF_INET:
+      return AddressFamily::IPV4;
+    case AF_INET6:
+      return AddressFamily::IPV6;
+    default:
+      RTC_DCHECK(false);
+      return AddressFamily::UNSPEC;
+  }
+}

[pthatcher1, 2016/06/27 20:35:55]

Is this just for sorting?  If so, instead of making an emum for address family,
it seems like we should just make it part a int GetAddressFamilyPriority(int
ip_family).  UNSPEC -> 0, IPV4 -> 1, IPV6 -> 2.

Actually, why are we preferring ipv4 over ipv6?  Shouldn't be the other way
around?  Or should we just prefer which ever TURN candidate works first (lowest
RTT)?

[honghaiz3, 2016/06/28 01:49:25]

You are right. Make it return an integer. 
I am giving IPv6 higher priority than Ipv4 already. 
If we choose whichever candidate works first, it will essentially choose the
first port becoming ready. I think we discussed that and we don't want that
approach, although not particular about IpV4 vs IPv6.  

+
+// Returns positive if a is better, negative if b is better, and 0 otherwise.
+int ComparePort(const cricket::Port* a, const cricket::Port* b) {
+  static constexpr int a_is_better = 1;
+  static constexpr int b_is_better = -1;
+  // Protocol type is defined as UDP = 0, TCP = 1, SSLTCP = 2.
+  if (a->GetProtocol() < b->GetProtocol()) {
+    return a_is_better;
+  }
+  if (a->GetProtocol() > b->GetProtocol()) {
+    return b_is_better;
+  }
+
+  AddressFamily a_family = ToAddressFamily(a->Network()->GetBestIP().family());
+  AddressFamily b_family = ToAddressFamily(b->Network()->GetBestIP().family());
+  if (a_family > b_family) {
+    return a_is_better;
+  }
+  if (a_family < b_family) {
+    return b_is_better;
+  }
+  return 0;
+}
+
 }  // namespace
 
 namespace cricket {
 const uint32_t DISABLE_ALL_PHASES =
     PORTALLOCATOR_DISABLE_UDP | PORTALLOCATOR_DISABLE_TCP |
     PORTALLOCATOR_DISABLE_STUN | PORTALLOCATOR_DISABLE_RELAY;
 
 // BasicPortAllocator
 BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager,
                                        rtc::PacketSocketFactory* socket_factory)
     : network_manager_(network_manager), socket_factory_(socket_factory) {
   ASSERT(network_manager_ != nullptr);
   ASSERT(socket_factory_ != nullptr);
   Construct();
 }
 
 BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager)
     : network_manager_(network_manager), socket_factory_(nullptr) {
   ASSERT(network_manager_ != nullptr);
   Construct();
 }
 
 BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager,
                                        rtc::PacketSocketFactory* socket_factory,
                                        const ServerAddresses& stun_servers)
     : network_manager_(network_manager), socket_factory_(socket_factory) {
   ASSERT(socket_factory_ != NULL);
-  SetConfiguration(stun_servers, std::vector<RelayServerConfig>(), 0);
+  SetConfiguration(stun_servers, std::vector<RelayServerConfig>(), 0, false);
   Construct();
 }
 
 BasicPortAllocator::BasicPortAllocator(
     rtc::NetworkManager* network_manager,
     const ServerAddresses& stun_servers,
     const rtc::SocketAddress& relay_address_udp,
     const rtc::SocketAddress& relay_address_tcp,
     const rtc::SocketAddress& relay_address_ssl)
     : network_manager_(network_manager), socket_factory_(NULL) {
   std::vector<RelayServerConfig> turn_servers;
   RelayServerConfig config(RELAY_GTURN);
   if (!relay_address_udp.IsNil()) {
     config.ports.push_back(ProtocolAddress(relay_address_udp, PROTO_UDP));
   }
   if (!relay_address_tcp.IsNil()) {
     config.ports.push_back(ProtocolAddress(relay_address_tcp, PROTO_TCP));
   }
   if (!relay_address_ssl.IsNil()) {
     config.ports.push_back(ProtocolAddress(relay_address_ssl, PROTO_SSLTCP));
   }
 
   if (!config.ports.empty()) {
     turn_servers.push_back(config);
   }
 
-  SetConfiguration(stun_servers, turn_servers, 0);
+  SetConfiguration(stun_servers, turn_servers, 0, false);
   Construct();
 }
 
 void BasicPortAllocator::Construct() {
   allow_tcp_listen_ = true;
 }
 
 BasicPortAllocator::~BasicPortAllocator() {
 }
 
 PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
     const std::string& content_name, int component,
     const std::string& ice_ufrag, const std::string& ice_pwd) {
   return new BasicPortAllocatorSession(
       this, content_name, component, ice_ufrag, ice_pwd);
 }
 
 void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) {
   std::vector<RelayServerConfig> new_turn_servers = turn_servers();
   new_turn_servers.push_back(turn_server);
-  SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size());
+  SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size(),
+                   disable_low_priority_turn_ports());
 }
 
 // BasicPortAllocatorSession
 BasicPortAllocatorSession::BasicPortAllocatorSession(
     BasicPortAllocator *allocator,
     const std::string& content_name,
     int component,
     const std::string& ice_ufrag,
     const std::string& ice_pwd)
     : PortAllocatorSession(content_name, component,
@@ skipping 74 matching lines @@
 }
 
 void BasicPortAllocatorSession::ClearGettingPorts() {
   network_thread_->Clear(this, MSG_ALLOCATE);
   for (uint32_t i = 0; i < sequences_.size(); ++i)
     sequences_[i]->Stop();
 }
 
 std::vector<PortInterface*> BasicPortAllocatorSession::ReadyPorts() const {
   std::vector<PortInterface*> ret;
+  // Add all non-TURN ports.
   for (const PortData& port : ports_) {
-    if (port.has_pairable_candidate() && !port.error()) {
+    if (IsPortInUse(port.port()) && port.has_pairable_candidate() &&
+        !port.error()) {
       ret.push_back(port.port());
     }
   }
   return ret;
 }
 
 std::vector<Candidate> BasicPortAllocatorSession::ReadyCandidates() const {
   std::vector<Candidate> candidates;
   for (const PortData& data : ports_) {
+    if (!IsPortInUse(data.port())) {
+      continue;
+    }
+
     for (const Candidate& candidate : data.port()->Candidates()) {
       if (!CheckCandidateFilter(candidate)) {
         continue;
       }
       ProtocolType pvalue;
       if (!StringToProto(candidate.protocol().c_str(), &pvalue) ||
           !data.sequence()->ProtocolEnabled(pvalue)) {
         continue;
       }
       candidates.push_back(SanitizeRelatedAddress(candidate));
@@ skipping 190 matching lines @@
                     networks->end());
   }
 }
 
 // For each network, see if we have a sequence that covers it already.  If not,
 // create a new sequence to create the appropriate ports.
 void BasicPortAllocatorSession::DoAllocate() {
   bool done_signal_needed = false;
   std::vector<rtc::Network*> networks;
   GetNetworks(&networks);
+  turn_ports_in_use_by_network_names_.clear();
 
   if (networks.empty()) {
     LOG(LS_WARNING) << "Machine has no networks; no ports will be allocated";
     done_signal_needed = true;
   } else {
+    PortConfiguration* config = configs_.empty() ? nullptr : configs_.back();
     for (uint32_t i = 0; i < networks.size(); ++i) {
-      PortConfiguration* config = NULL;
-      if (configs_.size() > 0)
-        config = configs_.back();
-
       uint32_t sequence_flags = flags();
       if ((sequence_flags & DISABLE_ALL_PHASES) == DISABLE_ALL_PHASES) {
         // If all the ports are disabled we should just fire the allocation
         // done event and return.
         done_signal_needed = true;
         break;
       }
 
       if (!config || config->relays.empty()) {
         // No relay ports specified in this config.
@@ skipping 105 matching lines @@
     Port* port, const Candidate& c) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
   // Discarding any candidate signal if port allocation status is
   // already in completed state.
   if (data->complete() || data->error()) {
     return;
   }
 
+  // Mark that the port has a pairable candidate, either because we have a
+  // usable candidate from the port, or simply because the port is bound to the
+  // any address and therefore has no host candidate. This will trigger the port
+  // to start creating candidate pairs (connections) and issue connectivity
+  // checks. If port has already been marked as having a pairable candidate,
+  // do nothing here.
+  // Note: Need to do MaybeSignalCandidateReady after this because we will
+  // check whether the candidate is generated by the port in use.
+  if (!data->has_pairable_candidate() && CandidatePairable(c, port)) {
+    data->set_has_pairable_candidate(true);
+    MaybeSignalPortReady(port);

[pthatcher1, 2016/06/27 20:35:55]

If this is only called once, can we just inline it?

[honghaiz3, 2016/06/28 01:49:25]

A separate method will make it more readable here. 
Plus it allows early return in the method to be called. 

+  }
+
   ProtocolType pvalue;
   bool candidate_protocol_enabled =
       StringToProto(c.protocol().c_str(), &pvalue) &&
       data->sequence()->ProtocolEnabled(pvalue);
 
   if (CheckCandidateFilter(c) && candidate_protocol_enabled) {
     std::vector<Candidate> candidates;
     candidates.push_back(SanitizeRelatedAddress(c));
+    MaybeSignalCandidatesReady(port, candidates);

[pthatcher1, 2016/06/27 20:35:54]

Since this is only called once, can you inline it?

[honghaiz3, 2016/06/28 01:49:25]

Done.

+  }
+}
+
+void BasicPortAllocatorSession::MaybeSignalPortReady(Port* port) {
+  if (!allocator_->disable_low_priority_turn_ports() ||
+      port->Type() != RELAY_PORT_TYPE) {
+    SignalPortReady(this, port);
+    return;
+  }
+  std::vector<Port*>& turn_ports =
+      turn_ports_in_use_by_network_names_[port->Network()->name()];

[pthatcher1, 2016/06/27 20:35:55]

Shouldn't we do a find() just in case it's not in the map?

[honghaiz3, 2016/06/28 01:49:25]

If it is not in the map, we create a new vector and add it to the map. 
That's what we want. 

+
+  bool use_port = true;
+  auto iter = turn_ports.begin();
+  while (iter != turn_ports.end()) {
+    int cmp = ComparePort(port, *iter);
+    if (cmp > 0) {
+      // The new port has higher precedence. Deprecate the existing port.
+      SignalPortDeprecated(this, *iter);
+      iter = turn_ports.erase(iter);
+      // TODO(honghaiz): Maybe also remove the candidates in the port.
+    } else {
+      ++iter;
+      if (cmp < 0) {
+        // If this new port has a lower precedence than any existing port, do
+        // not use it.
+        use_port = false;
+      }
+    }
+  }

[pthatcher1, 2016/06/27 20:35:54]

Correct me if I'm wrong, but it looks like turn_ports are always equal.  In that
case, you don't need to check all of them.  Just check the first. So all you
have to do is something like this:

int cmp = ComparePorts(port, turn_ports[0]);
if (cmp > 0) {
  // Ignore the port;  It's worse than ones we already have.
  return;  
} 
if (cmp < 0) {
  // Stop using all the rest;  They are worse than the new one we have.
  for (Port* deprecated_port : turn_ports) {
    SignalPortDeprecated(this, deprecated_port);
  }
  turn_ports.Clear();
}
turn_ports.push_back(port);
SignalPortReady(port);

[honghaiz3, 2016/06/28 01:49:25]

You are right, although we have removed the map now. 

+  if (use_port) {

[pthatcher1, 2016/06/27 20:35:55]

Actually, if we just store all ports in ports_ and mark them deprecated, then
this would be much more simple.  Just add this one, then use std::max to find
the best relay port.  Then, go through ports_ and mark all relay ports worse
than the best one as deprecated. 

[honghaiz3, 2016/06/28 01:49:25]

Done. Did not use std::max as it is not really much more easier than find the
max directly. 

+    turn_ports.push_back(port);
+    SignalPortReady(this, port);

[pthatcher1, 2016/06/27 20:35:54]

I think we need a better pair of names than
SignalPortReady/SignalPortDeprecated.  How about
SignalPortPairable/SignalPortUnpairable?

[honghaiz3, 2016/06/28 01:49:25]

SignalPortReady is used elsewhere by downstream code. 
Changing this will break the other usage. 
Should we keep it (until we want to change the name everywhere) and perhaps just
change the name of the newly added signal?

+  }
+}
+
+void BasicPortAllocatorSession::MaybeSignalCandidatesReady(
+    Port* port,
+    const std::vector<Candidate>& candidates) {
+  if (IsPortInUse(port)) {
     SignalCandidatesReady(this, candidates);
   }
+}
 
-  // Port has already been marked as having a pairable candidate.
-  // Nothing to do here.
-  if (data->has_pairable_candidate()) {
-    return;
+bool BasicPortAllocatorSession::IsPortInUse(Port* port) const {
+  // If the port is not disabling low priority ports, every port is in use.
+  if (!allocator_->disable_low_priority_turn_ports()) {
+    return true;
   }
-
-  // Mark that the port has a pairable candidate, either because we have a
-  // usable candidate from the port, or simply because the port is bound to the
-  // any address and therefore has no host candidate. This will trigger the port
-  // to start creating candidate pairs (connections) and issue connectivity
-  // checks.
-  if (CandidatePairable(c, port)) {
-    data->set_has_pairable_candidate(true);
-    SignalPortReady(this, port);
+  // If it is not a turn port, it is in use.
+  if (port->Type() != RELAY_PORT_TYPE) {
+    return true;
   }
+  // For TURN port, if it can be found in |turn_ports_in_use_by_network_name_|,
+  // it is in use.
+  const auto iter =
+      turn_ports_in_use_by_network_names_.find(port->Network()->name());
+  if (iter == turn_ports_in_use_by_network_names_.end()) {
+    return false;
+  }
+  const std::vector<Port*>& turn_ports = iter->second;
+  return std::find(turn_ports.begin(), turn_ports.end(), port) !=
+         turn_ports.end();

[pthatcher1, 2016/06/27 20:35:54]

If we store all port in ports_ and just mark them deprecated, then this would be
as simple as finding the port in ports_ and then looking at the state.   

[honghaiz3, 2016/06/28 01:49:26]

Done.

 }
 
 void BasicPortAllocatorSession::OnPortComplete(Port* port) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
 
   // Ignore any late signals.
   if (data->complete() || data->error()) {
     return;
@@ skipping 409 matching lines @@
 
   // If BasicPortAllocatorSession::OnAllocate left relay ports enabled then we
   // ought to have a relay list for them here.
   ASSERT(config_ && !config_->relays.empty());
   if (!(config_ && !config_->relays.empty())) {
     LOG(LS_WARNING)
         << "AllocationSequence: No relay server configured, skipping.";
     return;
   }
 
-  PortConfiguration::RelayList::const_iterator relay;
-  for (relay = config_->relays.begin();
-       relay != config_->relays.end(); ++relay) {
-    if (relay->type == RELAY_GTURN) {
-      CreateGturnPort(*relay);
-    } else if (relay->type == RELAY_TURN) {
-      CreateTurnPort(*relay);
+  for (RelayServerConfig& relay : config_->relays) {
+    if (relay.type == RELAY_GTURN) {
+      CreateGturnPort(relay);
+    } else if (relay.type == RELAY_TURN) {
+      CreateTurnPort(relay);
     } else {
       ASSERT(false);
     }
   }
 }
 
 void AllocationSequence::CreateGturnPort(const RelayServerConfig& config) {
   // TODO(mallinath) - Rename RelayPort to GTurnPort.
   RelayPort* port = RelayPort::Create(session_->network_thread(),
                                       session_->socket_factory(),
@@ skipping 29 matching lines @@
   for (relay_port = config.ports.begin();
        relay_port != config.ports.end(); ++relay_port) {
     TurnPort* port = NULL;
 
     // Skip UDP connections to relay servers if it's disallowed.
     if (IsFlagSet(PORTALLOCATOR_DISABLE_UDP_RELAY) &&
         relay_port->proto == PROTO_UDP) {
       continue;
     }
 
+    // Do not create the port if the server address family is known and does
+    // not match the local ip address family.
+    int server_ip_family = relay_port->address.ipaddr().family();
+    int local_ip_family = network_->GetBestIP().family();
+    if (server_ip_family != AF_UNSPEC && server_ip_family != local_ip_family) {
+      LOG(LS_INFO) << "Server and local address families are not compatible. "
+                   << "Server address: "
+                   << relay_port->address.ipaddr().ToString()
+                   << " Local address: " << network_->GetBestIP().ToString();
+      continue;
+    }

[pthatcher1, 2016/06/27 20:35:54]

Should this be a separate CL?  It seems like a bug fix.  

[honghaiz3, 2016/06/28 01:49:26]

Done.

+
     // Shared socket mode must be enabled only for UDP based ports. Hence
     // don't pass shared socket for ports which will create TCP sockets.
     // TODO(mallinath) - Enable shared socket mode for TURN ports. Disabled
     // due to webrtc bug https://code.google.com/p/webrtc/issues/detail?id=3537
     if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET) &&
         relay_port->proto == PROTO_UDP && udp_socket_) {
       port = TurnPort::Create(session_->network_thread(),
                               session_->socket_factory(),
                               network_, udp_socket_.get(),
                               session_->username(), session_->password(),
@@ skipping 138 matching lines @@
   ServerAddresses servers;
   for (size_t i = 0; i < relays.size(); ++i) {
     if (relays[i].type == turn_type && SupportsProtocol(relays[i], type)) {
       servers.insert(relays[i].ports.front().address);
     }
   }
   return servers;
 }
 
 }  // namespace cricket