Add support for aggressive nomination in controlling agent (#73)

Author: mickel8

lib/ex_ice/candidate.ex

@@ -93,8 +93,6 @@ defmodule ExICE.Candidate do
   @spec new(type(), Keyword.t()) :: t()
   def new(type, config) when type in [:host, :srflx, :prflx, :relay] do
     transport = Keyword.get(config, :transport, :udp)
-
-    priority = config[:priority] || ExICE.Priv.Candidate.priority(type)
     address = Keyword.fetch!(config, :address)
 
     %__MODULE__{
@@ -104,7 +102,7 @@ defmodule ExICE.Candidate do
       base_port: config[:base_port],
       foundation: ExICE.Priv.Candidate.foundation(type, address, nil, transport),
       port: Keyword.fetch!(config, :port),
-      priority: priority,
+      priority: Keyword.fetch!(config, :priority),
       transport: transport,
       type: type
     }

lib/ex_ice/ice_agent.ex

@@ -61,6 +61,12 @@ defmodule ExICE.ICEAgent do
   * `ice_transport_policy` - candidate types to be used.
     * `all` - all ICE candidates will be considered (default).
     * `relay` - only relay candidates will be considered.
+  * `aggressive_nomination` - whether to use aggressive nomination from RFC 5245.
+  ExICE aims to implement RFC 8445, which removes aggressive nomination.
+  In particular, RFC 8445 assumes that data can be sent on any valid pair (there is no need for nomination).
+  While this behavior is supported by most of the implementations, some of them still require
+  a pair to be nominated by the controlling agent before they can start sending data.
+  Defaults to false.
   * `on_gathering_state_change` - where to send gathering state change notifications. Defaults to a process that spawns `ExICE`.
   * `on_connection_state_change` - where to send connection state change notifications. Defaults to a process that spawns `ExICE`.
   * `on_data` - where to send data. Defaults to a process that spawns `ExICE`.
@@ -78,6 +84,7 @@ defmodule ExICE.ICEAgent do
             }
           ],
           ice_transport_policy: :all | :relay,
+          aggressive_nomination: boolean(),
           on_gathering_state_change: pid() | nil,
           on_connection_state_change: pid() | nil,
           on_data: pid() | nil,

lib/ex_ice/priv/candidate.ex

@@ -27,8 +27,25 @@ defmodule ExICE.Priv.Candidate do
   @callback send_data(t(), :inet.ip_address(), :inet.port_number(), binary()) ::
               {:ok, t()} | {:error, term(), t()}
 
-  @spec priority(type()) :: integer()
-  def priority(type) do
+  @spec priority!(%{:inet.ip_address() => non_neg_integer()}, :inet.ip_address(), type()) ::
+          non_neg_integer()
+  def priority!(local_preferences, base_address, type) do
+    local_preference = Map.fetch!(local_preferences, base_address)
+    do_priority(local_preference, type)
+  end
+
+  @spec priority(%{:inet.ip_address() => non_neg_integer()}, :inet.ip_address(), type()) ::
+          {%{:inet.ip_address() => non_neg_integer()}, non_neg_integer()}
+  def priority(local_preferences, base_address, type) do
+    local_preference =
+      Map.get(local_preferences, base_address) || generate_local_preference(local_preferences)
+
+    local_preferences = Map.put(local_preferences, base_address, local_preference)
+
+    {local_preferences, do_priority(local_preference, type)}
+  end
+
+  defp do_priority(local_preference, type) do
     type_preference =
       case type do
         :host -> 126
@@ -37,14 +54,25 @@ defmodule ExICE.Priv.Candidate do
         :relay -> 0
       end
 
-    # That's not fully correct as according to RFC 8445 sec. 5.1.2.1 we should:
-    # * use value of 65535 when there is only one IP address
-    # * use different values when there are multiple IP addresses
-    local_preference = 65_535
-
     2 ** 24 * type_preference + 2 ** 8 * local_preference + 2 ** 0 * (256 - 1)
   end
 
+  defp generate_local_preference(local_preferences, attempts \\ 200)
+
+  defp generate_local_preference(_local_preferences, 0),
+    do: raise("Couldn't generate local preference")
+
+  defp generate_local_preference(local_preferences, attempts) do
+    # this should give us a number from 0 to 2**16-1
+    <<pref::16>> = :crypto.strong_rand_bytes(2)
+
+    if Map.has_key?(local_preferences, pref) do
+      generate_local_preference(local_preferences, attempts - 1)
+    else
+      pref
+    end
+  end
+
   @spec foundation(type(), :inet.ip_address() | String.t(), :inet.ip_address() | nil, atom()) ::
           integer()
   def foundation(type, ip, stun_turn_ip, transport) do

lib/ex_ice/priv/candidate_base.ex

@@ -35,16 +35,14 @@ defmodule ExICE.Priv.CandidateBase do
     transport = :udp
     address = Keyword.fetch!(config, :address)
 
-    priority = config[:priority] || Candidate.priority(type)
-
     %__MODULE__{
       id: Utils.id(),
       address: address,
       base_address: config[:base_address],
       base_port: config[:base_port],
       foundation: Candidate.foundation(type, address, nil, transport),
       port: Keyword.fetch!(config, :port),
-      priority: priority,
+      priority: Keyword.fetch!(config, :priority),
       transport: transport,
       transport_module: Keyword.get(config, :transport_module, ExICE.Priv.Transport.UDP),
       socket: Keyword.fetch!(config, :socket),

lib/ex_ice/priv/conn_check_handler/controlled.ex

@@ -203,11 +203,12 @@ defmodule ExICE.Priv.ConnCheckHandler.Controlled do
 
           %ICEAgent{ice_agent | selected_pair_id: pair.id}
         else
+          Logger.debug("Not selecting a new pair as it has lower priority.")
           ice_agent
         end
 
       true ->
-        Logger.debug("Not selecting a new pair as it has lower priority or has the same id")
+        Logger.debug("Not selecting a new pair as it has the same id")
         ice_agent
     end
   end

lib/ex_ice/priv/conn_check_handler/controlling.ex

@@ -97,21 +97,51 @@ defmodule ExICE.Priv.ConnCheckHandler.Controlling do
   def update_nominated_flag(ice_agent, _pair_id, false), do: ice_agent
 
   @impl true
-  def update_nominated_flag(%ICEAgent{eoc: true} = ice_agent, pair_id, true) do
+  def update_nominated_flag(
+        %ICEAgent{eoc: eoc, aggressive_nomination: aggressive_nomination} = ice_agent,
+        pair_id,
+        true
+      )
+      when (aggressive_nomination == false and eoc == true) or aggressive_nomination == true do
     Logger.debug("Nomination succeeded. Selecting pair: #{inspect(pair_id)}")
 
     pair = Map.fetch!(ice_agent.checklist, pair_id)
     pair = %CandidatePair{pair | nominate?: false, nominated?: true}
     ice_agent = put_in(ice_agent.checklist[pair.id], pair)
 
+    ice_agent =
+      cond do
+        ice_agent.selected_pair_id == nil ->
+          Logger.debug("Selecting pair: #{pair_id}")
+          %ICEAgent{ice_agent | selected_pair_id: pair.id}
+
+        ice_agent.selected_pair_id != nil and pair.id != ice_agent.selected_pair_id ->
+          selected_pair = Map.fetch!(ice_agent.checklist, ice_agent.selected_pair_id)
+
+          if pair.priority >= selected_pair.priority do
+            Logger.debug("""
+            Selecting new pair with higher priority. \
+            New pair: #{pair_id}, old pair: #{ice_agent.selected_pair_id}.\
+            """)
+
+            %ICEAgent{ice_agent | selected_pair_id: pair.id}
+          else
+            Logger.debug("Not selecting a new pair as it has lower priority.")
+            ice_agent
+          end
+
+        true ->
+          Logger.debug("Not selecting a new pair as it has the same id")
+          ice_agent
+      end
+
     # the controlling agent could nominate only when eoc was set
     # and checklist finished
-    unless Checklist.finished?(ice_agent.checklist) do
+    if not ice_agent.aggressive_nomination and not Checklist.finished?(ice_agent.checklist) do
       Logger.warning("Nomination succeeded but checklist hasn't finished.")
     end
 
-    ice_agent = %ICEAgent{ice_agent | nominating?: {false, nil}, selected_pair_id: pair.id}
-    ICEAgent.change_connection_state(ice_agent, :completed)
+    %ICEAgent{ice_agent | nominating?: {false, nil}}
   end
 
   defp resolve_pair(ice_agent, pair) do

lib/ex_ice/priv/gatherer.ex

@@ -84,9 +84,17 @@ defmodule ExICE.Priv.Gatherer do
     end)
   end
 
-  @spec gather_host_candidates(t(), [Transport.socket()]) :: [Candidate.t()]
-  def gather_host_candidates(gatherer, sockets) do
-    Enum.map(sockets, &create_new_host_candidate(gatherer, &1))
+  @spec gather_host_candidates(t(), %{:inet.ip_address() => non_neg_integer()}, [
+          Transport.socket()
+        ]) :: [Candidate.t()]
+  def gather_host_candidates(gatherer, local_preferences, sockets) do
+    {local_preferences, cands} =
+      Enum.reduce(sockets, {local_preferences, []}, fn socket, {local_preferences, cands} ->
+        {local_preferences, cand} = create_new_host_candidate(gatherer, local_preferences, socket)
+        {local_preferences, [cand | cands]}
+      end)
+
+    {local_preferences, Enum.reverse(cands)}
   end
 
   @spec gather_srflx_candidate(t(), integer(), Transport.socket(), ExSTUN.URI.t()) ::
@@ -155,21 +163,24 @@ defmodule ExICE.Priv.Gatherer do
     Keyword.get_values(int, :addr)
   end
 
-  defp create_new_host_candidate(gatherer, socket) do
+  defp create_new_host_candidate(gatherer, local_preferences, socket) do
     {:ok, {sock_ip, sock_port}} = gatherer.transport_module.sockname(socket)
 
+    {local_preferences, priority} = Candidate.priority(local_preferences, sock_ip, :host)
+
     cand =
       Candidate.Host.new(
         address: sock_ip,
         port: sock_port,
         base_address: sock_ip,
         base_port: sock_port,
+        priority: priority,
         transport_module: gatherer.transport_module,
         socket: socket
       )
 
     Logger.debug("New candidate: #{inspect(cand)}")
 
-    cand
+    {local_preferences, cand}
   end
 end