From 72e2a4676159881b042712d8f37d0794e1f2a247 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Wed, 12 Mar 2025 12:38:58 +0100 Subject: [PATCH 1/8] Add support for aggressive nomination in controlling agent --- lib/ex_ice/ice_agent.ex | 7 + .../priv/conn_check_handler/controlled.ex | 3 +- .../priv/conn_check_handler/controlling.ex | 12 +- lib/ex_ice/priv/ice_agent.ex | 44 +++- test/priv/ice_agent_test.exs | 218 +++++++++++++++++- 5 files changed, 269 insertions(+), 15 deletions(-) diff --git a/lib/ex_ice/ice_agent.ex b/lib/ex_ice/ice_agent.ex index fd61863..61cc23d 100644 --- a/lib/ex_ice/ice_agent.ex +++ b/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 (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 start sending data. + Defaults to true. * `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, diff --git a/lib/ex_ice/priv/conn_check_handler/controlled.ex b/lib/ex_ice/priv/conn_check_handler/controlled.ex index bf6ebf4..60ee72e 100644 --- a/lib/ex_ice/priv/conn_check_handler/controlled.ex +++ b/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 diff --git a/lib/ex_ice/priv/conn_check_handler/controlling.ex b/lib/ex_ice/priv/conn_check_handler/controlling.ex index de6b42f..a414a85 100644 --- a/lib/ex_ice/priv/conn_check_handler/controlling.ex +++ b/lib/ex_ice/priv/conn_check_handler/controlling.ex @@ -97,7 +97,12 @@ 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) @@ -106,12 +111,11 @@ defmodule ExICE.Priv.ConnCheckHandler.Controlling do # 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}, selected_pair_id: pair.id} end defp resolve_pair(ice_agent, pair) do diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index 293e846..d9a8d41 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -59,6 +59,7 @@ defmodule ExICE.Priv.ICEAgent do :on_gathering_state_change, :on_data, :on_new_candidate, + :aggressive_nomination, :if_discovery_module, :transport_module, :gatherer, @@ -149,6 +150,7 @@ defmodule ExICE.Priv.ICEAgent do on_gathering_state_change: opts[:on_gathering_state_change] || controlling_process, on_data: opts[:on_data] || controlling_process, on_new_candidate: opts[:on_new_candidate] || controlling_process, + aggressive_nomination: Keyword.get(opts, :aggressive_nomination, true), if_discovery_module: if_discovery_module, transport_module: transport_module, gatherer: Gatherer.new(if_discovery_module, transport_module, ip_filter, ports), @@ -430,6 +432,13 @@ defmodule ExICE.Priv.ICEAgent do update_connection_state(ice_agent) end + def end_of_candidates(%__MODULE__{role: :controlling, aggressive_nomination: true} = ice_agent) do + Logger.debug("Setting end-of-candidates flag") + ice_agent = %{ice_agent | eoc: true} + # we might need to move to the completed state + update_connection_state(ice_agent) + end + def end_of_candidates(%__MODULE__{role: :controlling} = ice_agent) do Logger.debug("Setting end-of-candidates flag.") ice_agent = %{ice_agent | eoc: true} @@ -2189,7 +2198,11 @@ defmodule ExICE.Priv.ICEAgent do end end - defp time_to_nominate?(%__MODULE__{state: :connected} = ice_agent) do + # In aggressive nomination, there is no additional connectivity check. + # Instead, every connectivity check includes UseCandidate flag. + defp time_to_nominate?(%__MODULE__{aggressive_nomination: true}), do: false + + defp time_to_nominate?(%__MODULE__{aggressive_nomination: false, state: :connected} = ice_agent) do {nominating?, _} = ice_agent.nominating? # if we are not during nomination and we know there won't be further candidates, # there are no checks waiting or in-progress, @@ -2515,8 +2528,19 @@ defmodule ExICE.Priv.ICEAgent do end end + # credo:disable-for-next-line Credo.Check.Refactor.CyclomaticComplexity defp update_connection_state(%__MODULE__{state: :checking} = ice_agent) do cond do + # in aggressive nomination, we might move directly from checking to completed + ice_agent.selected_pair_id != nil and ice_agent.eoc == true and + ice_agent.gathering_state == :complete and Checklist.finished?(ice_agent.checklist) -> + Logger.debug(""" + Found a valid pair, there won't be any further local or remote candidates. \ + Changing connection state to complete.\ + """) + + change_connection_state(ice_agent, :completed) + Checklist.get_valid_pair(ice_agent.checklist) != nil -> Logger.debug("Found a valid pair. Changing connection state to connected") change_connection_state(ice_agent, :connected) @@ -2550,7 +2574,7 @@ defmodule ExICE.Priv.ICEAgent do Checklist.finished?(ice_agent.checklist) -> change_connection_state(ice_agent, :failed) - # Assuming the controlling side uses regulard nomination, + # Assuming the controlling side uses regular nomination, # the controlled side could move to the completed # state as soon as it receives nomination request (or after # successful triggered check caused by nomination request). @@ -2570,9 +2594,12 @@ defmodule ExICE.Priv.ICEAgent do change_connection_state(ice_agent, :completed) - ice_agent.role == :controlling and ice_agent.selected_pair_id != nil -> + ice_agent.role == :controlling and ice_agent.selected_pair_id != nil and + ice_agent.eoc == true and ice_agent.gathering_state == :complete and + Checklist.finished?(ice_agent.checklist) -> Logger.debug(""" - We have selected pair and we are the controlling agent. Changing state to completed.\ + Finished all conn checks, there won't be any further local or remote candidates + and we have selected pair. Changing connection state to completed.\ """) change_connection_state(ice_agent, :completed) @@ -2727,7 +2754,14 @@ defmodule ExICE.Priv.ICEAgent do # we can nominate only when being the controlling agent # the controlled agent uses nominate? flag according to 7.3.1.5 - nominate = pair.nominate? and ice_agent.role == :controlling + nominate = + ice_agent.role == :controlling and (pair.nominate? or ice_agent.aggressive_nomination) + + # set nominate? flag in case we are running aggressive nomination + # but don't override it if we are controlled agent and it was already set to true + pair = %CandidatePair{pair | nominate?: pair.nominate? || nominate} + ice_agent = put_in(ice_agent.checklist[pair.id], pair) + req = binding_request(ice_agent, nominate) raw_req = Message.encode(req) diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index 40989d5..2dc3eae 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -1120,6 +1120,7 @@ defmodule ExICE.Priv.ICEAgentTest do ice_agent = ICEAgent.new( controlling_process: self(), + aggressive_nomination: false, role: :controlling, if_discovery_module: IfDiscovery.Mock, transport_module: Transport.Mock @@ -1184,6 +1185,7 @@ defmodule ExICE.Priv.ICEAgentTest do assert [new_pair] = Map.values(ice_agent.checklist) assert new_pair.state == :succeeded assert new_pair.responses_received == pair.responses_received + 1 + assert ice_agent.state == :connected end test "success response with non-matching message integrity", %{ @@ -1392,14 +1394,219 @@ defmodule ExICE.Priv.ICEAgentTest do pair.non_symmetric_responses_received + 1 end - defp read_binding_request(socket, remote_pwd) do - packet = Transport.Mock.recv(socket) - {:ok, req} = ExSTUN.Message.decode(packet) - :ok = ExSTUN.Message.authenticate(req, remote_pwd) - req + test "concluding", %{ice_agent: ice_agent, remote_cand: remote_cand} do + [socket] = ice_agent.sockets + + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + req = read_binding_request(socket, ice_agent.remote_pwd) + + resp = + binding_response( + req.transaction_id, + ice_agent.transport_module, + socket, + ice_agent.remote_pwd + ) + + ice_agent = + ICEAgent.handle_udp(ice_agent, socket, remote_cand.address, remote_cand.port, resp) + + assert ice_agent.state == :connected + + # assert that setting end-of-candidates triggers nomination check and concludes ICE + ice_agent = ICEAgent.end_of_candidates(ice_agent) + assert ice_agent.state == :connected + req = read_binding_request(socket, ice_agent.remote_pwd) + + resp = + binding_response( + req.transaction_id, + ice_agent.transport_module, + socket, + ice_agent.remote_pwd + ) + + ice_agent = + ICEAgent.handle_udp(ice_agent, socket, remote_cand.address, remote_cand.port, resp) + + assert ice_agent.state == :completed + end + end + + describe "connectivity check with aggressive nomination" do + setup do + ice_agent = + ICEAgent.new( + controlling_process: self(), + role: :controlling, + aggressive_nomination: true, + if_discovery_module: IfDiscovery.Mock, + transport_module: Transport.Mock + ) + |> ICEAgent.set_remote_credentials("someufrag", "somepwd") + |> ICEAgent.gather_candidates() + + %{ice_agent: ice_agent} + end + + test "request", %{ice_agent: ice_agent} do + rcand1 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 2}, port: 8445) + ice_agent = ICEAgent.add_remote_candidate(ice_agent, rcand1) + + [socket] = ice_agent.sockets + + [pair] = Map.values(ice_agent.checklist) + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + assert packet = Transport.Mock.recv(socket) + assert is_binary(packet) + assert {:ok, req} = ExSTUN.Message.decode(packet) + assert :ok == ExSTUN.Message.check_fingerprint(req) + assert :ok == ExSTUN.Message.authenticate(req, ice_agent.remote_pwd) + + assert length(req.attributes) == 6 + + assert {:ok, %Username{value: "#{ice_agent.remote_ufrag}:#{ice_agent.local_ufrag}"}} == + ExSTUN.Message.get_attribute(req, Username) + + assert {:ok, %ICEControlling{}} = ExSTUN.Message.get_attribute(req, ICEControlling) + assert {:ok, %Priority{}} = ExSTUN.Message.get_attribute(req, Priority) + assert {:ok, %UseCandidate{}} = ExSTUN.Message.get_attribute(req, UseCandidate) + + assert [new_pair] = Map.values(ice_agent.checklist) + assert new_pair.state == :in_progress + assert new_pair.requests_sent == pair.requests_sent + 1 + end + + test "success response", %{ice_agent: ice_agent} do + rcand1 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 2}, port: 8445) + rcand2 = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445) + + ice_agent = ICEAgent.add_remote_candidate(ice_agent, rcand1) + + [socket] = ice_agent.sockets + + # execute cc on srflx cand + [srflx_pair] = Map.values(ice_agent.checklist) + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + req = read_binding_request(socket, ice_agent.remote_pwd) + + resp = + binding_response( + req.transaction_id, + ice_agent.transport_module, + socket, + ice_agent.remote_pwd + ) + + ice_agent = ICEAgent.handle_udp(ice_agent, socket, rcand1.address, rcand1.port, resp) + + # make sure that ICE has not moved to the completed state + assert ice_agent.state == :connected + assert [new_srflx_pair] = Map.values(ice_agent.checklist) + assert new_srflx_pair.state == :succeeded + assert new_srflx_pair.nominated? == true + assert new_srflx_pair.responses_received == srflx_pair.responses_received + 1 + assert ice_agent.selected_pair_id == new_srflx_pair.id + + # execut cc on host cand + ice_agent = ICEAgent.add_remote_candidate(ice_agent, rcand2) + [host_pair] = Map.values(ice_agent.checklist) |> Enum.filter(&(&1.id != srflx_pair.id)) + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + req = read_binding_request(socket, ice_agent.remote_pwd) + + resp = + binding_response( + req.transaction_id, + ice_agent.transport_module, + socket, + ice_agent.remote_pwd + ) + + ice_agent = ICEAgent.handle_udp(ice_agent, socket, rcand2.address, rcand2.port, resp) + + assert [new_host_pair] = + Map.values(ice_agent.checklist) |> Enum.filter(&(&1.id != srflx_pair.id)) + + assert new_host_pair.state == :succeeded + assert new_host_pair.nominated? == true + assert new_host_pair.responses_received == host_pair.responses_received + 1 + assert ice_agent.selected_pair_id == new_host_pair.id + end + + test "success response after setting eoc and finishing candidate gathering", %{ + ice_agent: ice_agent + } do + # this test checks if we move from checking directly to complete + # when eoc is set and local candidates gathering has finished + + rcand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445) + + ice_agent = + ice_agent + |> ICEAgent.add_remote_candidate(rcand1) + |> ICEAgent.end_of_candidates() + + [socket] = ice_agent.sockets + + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + req = read_binding_request(socket, ice_agent.remote_pwd) + + resp = + binding_response( + req.transaction_id, + ice_agent.transport_module, + socket, + ice_agent.remote_pwd + ) + + assert ice_agent.state == :checking + ice_agent = ICEAgent.handle_udp(ice_agent, socket, rcand1.address, rcand1.port, resp) + assert ice_agent.state == :completed + end + + test "concluding", %{ice_agent: ice_agent} do + rcand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445) + + ice_agent = ICEAgent.add_remote_candidate(ice_agent, rcand1) + + [socket] = ice_agent.sockets + + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + req = read_binding_request(socket, ice_agent.remote_pwd) + + resp = + binding_response( + req.transaction_id, + ice_agent.transport_module, + socket, + ice_agent.remote_pwd + ) + + ice_agent = ICEAgent.handle_udp(ice_agent, socket, rcand1.address, rcand1.port, resp) + + assert ice_agent.state == :connected + + # assert that setting end-of-candidates flag concludes ice + # and there is no additional conn check sent (as we use aggressive nomination) + ice_agent = ICEAgent.end_of_candidates(ice_agent) + assert ice_agent.state == :completed + assert Transport.Mock.recv(socket) == nil end end + defp read_binding_request(socket, remote_pwd) do + packet = Transport.Mock.recv(socket) + {:ok, req} = ExSTUN.Message.decode(packet) + :ok = ExSTUN.Message.authenticate(req, remote_pwd) + req + end + describe "connectivity check rtx" do setup do ice_agent = @@ -1692,6 +1899,7 @@ defmodule ExICE.Priv.ICEAgentTest do assert failed_ice_agent == new_ice_agent end + @tag :debug test "agent state and behavior after it completes" do r_cand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445) r_cand2 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 4}, port: 8445) From 0efc5ad98365ae917dc09bd8c1cfd5215ba11575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Thu, 13 Mar 2025 15:16:38 +0100 Subject: [PATCH 2/8] Improve candidate priority computation --- lib/ex_ice/candidate.ex | 9 ++++++++- lib/ex_ice/priv/candidate.ex | 9 ++++++--- lib/ex_ice/priv/candidate_base.ex | 8 +++++++- lib/ex_ice/priv/ice_agent.ex | 8 ++++---- 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/lib/ex_ice/candidate.ex b/lib/ex_ice/candidate.ex index 6212e86..6ad0f19 100644 --- a/lib/ex_ice/candidate.ex +++ b/lib/ex_ice/candidate.ex @@ -94,7 +94,14 @@ defmodule ExICE.Candidate do 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) + priority = + if config[:priority] do + config[:priority] + else + base_address = Keyword.fetch!(config, :base_address) + ExICE.Priv.Candidate.priority(base_address, type) + end + address = Keyword.fetch!(config, :address) %__MODULE__{ diff --git a/lib/ex_ice/priv/candidate.ex b/lib/ex_ice/priv/candidate.ex index bc7d151..dad2fef 100644 --- a/lib/ex_ice/priv/candidate.ex +++ b/lib/ex_ice/priv/candidate.ex @@ -27,8 +27,8 @@ 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(), type()) :: integer() + def priority(address, type) do type_preference = case type do :host -> 126 @@ -40,7 +40,10 @@ defmodule ExICE.Priv.Candidate do # 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 + # local_preference = 65_535 + + # just make sure that for different ip addresses, we have different local preference + local_preference = :erlang.phash2(address, 65535) 2 ** 24 * type_preference + 2 ** 8 * local_preference + 2 ** 0 * (256 - 1) end diff --git a/lib/ex_ice/priv/candidate_base.ex b/lib/ex_ice/priv/candidate_base.ex index 06a7e36..8c52b08 100644 --- a/lib/ex_ice/priv/candidate_base.ex +++ b/lib/ex_ice/priv/candidate_base.ex @@ -35,7 +35,13 @@ defmodule ExICE.Priv.CandidateBase do transport = :udp address = Keyword.fetch!(config, :address) - priority = config[:priority] || Candidate.priority(type) + priority = + if config[:priority] do + config[:priority] + else + base_address = Keyword.fetch!(config, :base_address) + ExICE.Priv.Candidate.priority(base_address, type) + end %__MODULE__{ id: Utils.id(), diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index d9a8d41..080dae3 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -2732,7 +2732,7 @@ defmodule ExICE.Priv.ICEAgent do local_cand = Map.fetch!(ice_agent.local_cands, pair.local_cand_id) remote_cand = Map.fetch!(ice_agent.remote_cands, pair.remote_cand_id) - req = binding_request(ice_agent, false) + req = binding_request(ice_agent, local_cand, false) dst = {remote_cand.address, remote_cand.port} @@ -2762,7 +2762,7 @@ defmodule ExICE.Priv.ICEAgent do pair = %CandidatePair{pair | nominate?: pair.nominate? || nominate} ice_agent = put_in(ice_agent.checklist[pair.id], pair) - req = binding_request(ice_agent, nominate) + req = binding_request(ice_agent, local_cand, nominate) raw_req = Message.encode(req) @@ -2789,7 +2789,7 @@ defmodule ExICE.Priv.ICEAgent do end end - defp binding_request(ice_agent, nominate) do + defp binding_request(ice_agent, local_candidate, nominate) do type = %Type{class: :request, method: :binding} role_attr = @@ -2802,7 +2802,7 @@ defmodule ExICE.Priv.ICEAgent do # priority sent to the other side has to be # computed with the candidate type preference of # peer-reflexive; refer to sec 7.1.1 - priority = Candidate.priority(:prflx) + priority = Candidate.priority(local_candidate.base.base_address, :prflx) attrs = [ %Username{value: "#{ice_agent.remote_ufrag}:#{ice_agent.local_ufrag}"}, From 99186744652cd1d8f2c745efb7362c07a148ca5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Thu, 13 Mar 2025 16:55:43 +0100 Subject: [PATCH 3/8] Select a new pair only if it has the same or greater priority --- lib/ex_ice/candidate.ex | 14 +++++----- .../priv/conn_check_handler/controlling.ex | 28 ++++++++++++++++++- lib/ex_ice/priv/ice_agent.ex | 10 +++++-- 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/lib/ex_ice/candidate.ex b/lib/ex_ice/candidate.ex index 6ad0f19..b05e05c 100644 --- a/lib/ex_ice/candidate.ex +++ b/lib/ex_ice/candidate.ex @@ -94,13 +94,13 @@ defmodule ExICE.Candidate do def new(type, config) when type in [:host, :srflx, :prflx, :relay] do transport = Keyword.get(config, :transport, :udp) - priority = - if config[:priority] do - config[:priority] - else - base_address = Keyword.fetch!(config, :base_address) - ExICE.Priv.Candidate.priority(base_address, type) - end + priority = Keyword.fetch!(config, :priority) + # if config[:priority] do + # config[:priority] + # else + # base_address = config[:base_address] || config[:address] || raise "no address" + # ExICE.Priv.Candidate.priority(base_address, type) + # end address = Keyword.fetch!(config, :address) diff --git a/lib/ex_ice/priv/conn_check_handler/controlling.ex b/lib/ex_ice/priv/conn_check_handler/controlling.ex index a414a85..63776fd 100644 --- a/lib/ex_ice/priv/conn_check_handler/controlling.ex +++ b/lib/ex_ice/priv/conn_check_handler/controlling.ex @@ -109,13 +109,39 @@ defmodule ExICE.Priv.ConnCheckHandler.Controlling do 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 if not ice_agent.aggressive_nomination and not Checklist.finished?(ice_agent.checklist) do Logger.warning("Nomination succeeded but checklist hasn't finished.") end - %ICEAgent{ice_agent | nominating?: {false, nil}, selected_pair_id: pair.id} + %ICEAgent{ice_agent | nominating?: {false, nil}} end defp resolve_pair(ice_agent, pair) do diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index 080dae3..0060bf9 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -2079,11 +2079,17 @@ defmodule ExICE.Priv.ICEAgent do end end - defp get_or_create_remote_cand(ice_agent, src_ip, src_port, _prio_attr) do + defp get_or_create_remote_cand(ice_agent, src_ip, src_port, prio_attr) do case find_remote_cand(Map.values(ice_agent.remote_cands), src_ip, src_port) do nil -> # TODO calculate correct prio using prio_attr - cand = ExICE.Candidate.new(:prflx, address: src_ip, port: src_port) + cand = + ExICE.Candidate.new(:prflx, + address: src_ip, + port: src_port, + priority: prio_attr.priority + ) + Logger.debug("Adding new remote prflx candidate: #{inspect(cand)}") ice_agent = put_in(ice_agent.remote_cands[cand.id], cand) {cand, ice_agent} From e5006b695f7d866de47abd11537143f0fa3e8633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Fri, 14 Mar 2025 13:15:23 +0100 Subject: [PATCH 4/8] Correctly calculate priority --- lib/ex_ice/priv/candidate.ex | 43 ++++++++++++++---- lib/ex_ice/priv/candidate_base.ex | 10 +---- lib/ex_ice/priv/gatherer.ex | 21 ++++++--- lib/ex_ice/priv/ice_agent.ex | 45 +++++++++++++++++-- test/candidate_test.exs | 32 +++++++++++-- test/priv/candidate_pair_test.exs | 8 ++-- test/priv/checklist_test.exs | 7 ++- .../conn_check_handler/controlled_test.exs | 4 +- .../conn_check_handler/controlling_test.exs | 4 +- test/priv/gatherer_test.exs | 9 +++- test/priv/ice_agent_test.exs | 24 +++++----- 11 files changed, 153 insertions(+), 54 deletions(-) diff --git a/lib/ex_ice/priv/candidate.ex b/lib/ex_ice/priv/candidate.ex index dad2fef..ab053b7 100644 --- a/lib/ex_ice/priv/candidate.ex +++ b/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(:inet.ip_address(), type()) :: integer() - def priority(address, 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,15 +54,23 @@ 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 - # just make sure that for different ip addresses, we have different local preference - local_preference = :erlang.phash2(address, 65535) + defp generate_local_preference(local_preferences, attempts \\ 200) - 2 ** 24 * type_preference + 2 ** 8 * local_preference + 2 ** 0 * (256 - 1) + 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 + <> = :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()) :: diff --git a/lib/ex_ice/priv/candidate_base.ex b/lib/ex_ice/priv/candidate_base.ex index 8c52b08..1ac18fd 100644 --- a/lib/ex_ice/priv/candidate_base.ex +++ b/lib/ex_ice/priv/candidate_base.ex @@ -35,14 +35,6 @@ defmodule ExICE.Priv.CandidateBase do transport = :udp address = Keyword.fetch!(config, :address) - priority = - if config[:priority] do - config[:priority] - else - base_address = Keyword.fetch!(config, :base_address) - ExICE.Priv.Candidate.priority(base_address, type) - end - %__MODULE__{ id: Utils.id(), address: address, @@ -50,7 +42,7 @@ defmodule ExICE.Priv.CandidateBase do 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), diff --git a/lib/ex_ice/priv/gatherer.ex b/lib/ex_ice/priv/gatherer.ex index 3836ec1..fb84373 100644 --- a/lib/ex_ice/priv/gatherer.ex +++ b/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 diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index 0060bf9..907b0f1 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -86,6 +86,7 @@ defmodule ExICE.Priv.ICEAgent do sockets: [], local_cands: %{}, remote_cands: %{}, + local_preferences: %{}, stun_servers: [], turn_servers: [], resolved_turn_servers: [], @@ -305,7 +306,11 @@ defmodule ExICE.Priv.ICEAgent do ice_agent = change_gathering_state(ice_agent, :gathering) {:ok, sockets} = Gatherer.open_sockets(ice_agent.gatherer) - host_cands = Gatherer.gather_host_candidates(ice_agent.gatherer, sockets) + + {local_preferences, host_cands} = + Gatherer.gather_host_candidates(ice_agent.gatherer, ice_agent.local_preferences, sockets) + + ice_agent = %__MODULE__{ice_agent | local_preferences: local_preferences} ice_agent = Enum.reduce(host_cands, ice_agent, fn host_cand, ice_agent -> @@ -1074,7 +1079,18 @@ defmodule ExICE.Priv.ICEAgent do {client.turn_ip, client.turn_port} | ice_agent.resolved_turn_servers ] - ice_agent = %{ice_agent | resolved_turn_servers: resolved_turn_servers} + # Use sock_addr for calculating priority. + # In other case, we might duplicate priority. + {:ok, {sock_addr, _sock_port}} = ice_agent.transport_module.sockname(tr.socket) + + {local_preferences, priority} = + Candidate.priority(ice_agent.local_preferences, sock_addr, :relay) + + ice_agent = %{ + ice_agent + | resolved_turn_servers: resolved_turn_servers, + local_preferences: local_preferences + } relay_cand = Candidate.Relay.new( @@ -1082,6 +1098,7 @@ defmodule ExICE.Priv.ICEAgent do port: alloc_port, base_address: alloc_ip, base_port: alloc_port, + priority: priority, transport_module: ice_agent.transport_module, socket: tr.socket, client: client @@ -1741,12 +1758,18 @@ defmodule ExICE.Priv.ICEAgent do nil -> {:ok, {base_addr, base_port}} = ice_agent.transport_module.sockname(tr.socket) + {local_preferences, priority} = + Candidate.priority(ice_agent.local_preferences, base_addr, :srflx) + + ice_agent = %__MODULE__{ice_agent | local_preferences: local_preferences} + cand = Candidate.Srflx.new( address: xor_addr, port: xor_port, base_address: base_addr, base_port: base_port, + priority: priority, transport_module: ice_agent.transport_module, socket: tr.socket ) @@ -2058,12 +2081,16 @@ defmodule ExICE.Priv.ICEAgent do # TODO calculate correct prio and foundation local_cand = Map.fetch!(ice_agent.local_cands, conn_check_pair.local_cand_id) + priority = + Candidate.priority!(ice_agent.local_preferences, local_cand.base.base_address, :prflx) + cand = Candidate.Prflx.new( address: xor_addr.address, port: xor_addr.port, base_address: local_cand.base.base_address, base_port: local_cand.base.base_port, + priority: priority, transport_module: ice_agent.transport_module, socket: local_cand.base.socket ) @@ -2808,7 +2835,19 @@ defmodule ExICE.Priv.ICEAgent do # priority sent to the other side has to be # computed with the candidate type preference of # peer-reflexive; refer to sec 7.1.1 - priority = Candidate.priority(local_candidate.base.base_address, :prflx) + priority = + if local_candidate.base.type == :relay do + {:ok, {sock_addr, _sock_port}} = + ice_agent.transport_module.sockname(local_candidate.base.socket) + + Candidate.priority!(ice_agent.local_preferences, sock_addr, :prflx) + else + Candidate.priority!( + ice_agent.local_preferences, + local_candidate.base.base_address, + :prflx + ) + end attrs = [ %Username{value: "#{ice_agent.remote_ufrag}:#{ice_agent.local_ufrag}"}, diff --git a/test/candidate_test.exs b/test/candidate_test.exs index 6bb9009..68bfdef 100644 --- a/test/candidate_test.exs +++ b/test/candidate_test.exs @@ -8,10 +8,22 @@ defmodule ExICE.CandidateTest do port = 12_345 %Candidate{foundation: f1} = - Candidate.new(:host, address: ip, port: port, base_address: ip, base_port: port) + Candidate.new(:host, + address: ip, + port: port, + base_address: ip, + base_port: port, + priority: 123 + ) %Candidate{foundation: f2} = - Candidate.new(:host, address: ip, port: port, base_address: ip, base_port: port) + Candidate.new(:host, + address: ip, + port: port, + base_address: ip, + base_port: port, + priority: 123 + ) assert f1 == f2 @@ -19,10 +31,22 @@ defmodule ExICE.CandidateTest do port2 = 23_456 %Candidate{foundation: f1} = - Candidate.new(:host, address: ip, port: port, base_address: ip, base_port: port) + Candidate.new(:host, + address: ip, + port: port, + base_address: ip, + base_port: port, + priority: 123 + ) %Candidate{foundation: f2} = - Candidate.new(:host, address: ip2, port: port2, base_address: ip2, base_port: port2) + Candidate.new(:host, + address: ip2, + port: port2, + base_address: ip2, + base_port: port2, + priority: 122 + ) assert f1 != f2 end diff --git a/test/priv/candidate_pair_test.exs b/test/priv/candidate_pair_test.exs index c610f13..a6e0a2f 100644 --- a/test/priv/candidate_pair_test.exs +++ b/test/priv/candidate_pair_test.exs @@ -13,11 +13,10 @@ defmodule ExICE.Priv.CandidatePairTest do port: port1, base_address: addr1, base_port: port1, + priority: 100, socket: nil ) - c1 = %Candidate.Host{c1 | base: %{c1.base | priority: 100}} - addr2 = {192, 168, 1, 2} port2 = 23_456 @@ -26,11 +25,10 @@ defmodule ExICE.Priv.CandidatePairTest do address: addr2, port: port2, base_address: addr2, - base_port: port2 + base_port: port2, + priority: 200 ) - c2 = %ExICE.Candidate{c2 | priority: 200} - c1c2 = CandidatePair.new(c1, c2, :controlling, :frozen) assert c1c2.priority == 429_496_730_000 diff --git a/test/priv/checklist_test.exs b/test/priv/checklist_test.exs index a8f3078..8a4ea8f 100644 --- a/test/priv/checklist_test.exs +++ b/test/priv/checklist_test.exs @@ -17,6 +17,7 @@ defmodule ExICE.Priv.ChecklistTest do port: local_port, base_address: local_addr, base_port: local_port, + priority: 123, socket: nil ) @@ -25,7 +26,8 @@ defmodule ExICE.Priv.ChecklistTest do address: local_addr, port: local_port, base_address: local_addr, - base_port: local_port + base_port: local_port, + priority: 123 ) remote_srflx_cand = @@ -33,7 +35,8 @@ defmodule ExICE.Priv.ChecklistTest do address: remote_srflx_addr, port: remote_srflx_port, base_address: remote_addr, - base_port: remote_port + base_port: remote_port, + priority: 122 ) host_pair = diff --git a/test/priv/conn_check_handler/controlled_test.exs b/test/priv/conn_check_handler/controlled_test.exs index 3df3572..a6497d4 100644 --- a/test/priv/conn_check_handler/controlled_test.exs +++ b/test/priv/conn_check_handler/controlled_test.exs @@ -20,8 +20,8 @@ defmodule ExICE.Priv.ConnCheckHandler.ControlledTest do end end - @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445) - @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445) + @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445, priority: 123) + @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 122) describe "incoming binding request" do setup do diff --git a/test/priv/conn_check_handler/controlling_test.exs b/test/priv/conn_check_handler/controlling_test.exs index 9bce8ff..335e3c6 100644 --- a/test/priv/conn_check_handler/controlling_test.exs +++ b/test/priv/conn_check_handler/controlling_test.exs @@ -20,8 +20,8 @@ defmodule ExICE.Priv.ConnCheckHandler.ControllingTest do end end - @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445) - @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445) + @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445, priority: 123) + @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 122) describe "incoming binding request" do setup do diff --git a/test/priv/gatherer_test.exs b/test/priv/gatherer_test.exs index 04ea1e6..c3e4c77 100644 --- a/test/priv/gatherer_test.exs +++ b/test/priv/gatherer_test.exs @@ -42,7 +42,11 @@ defmodule ExICE.Priv.GathererTest do assert {:ok, sockets} = Gatherer.open_sockets(gatherer) # there should only be one candidate - assert [%Candidate.Host{} = c] = Gatherer.gather_host_candidates(gatherer, sockets) + assert {local_preferences, [%Candidate.Host{} = c]} = + Gatherer.gather_host_candidates(gatherer, %{}, sockets) + + assert Map.has_key?(local_preferences, c.base.address) + assert map_size(local_preferences) == 1 assert c.base.address == {192, 168, 0, 1} assert c.base.base_address == {192, 168, 0, 1} assert c.base.port == c.base.base_port @@ -65,7 +69,8 @@ defmodule ExICE.Priv.GathererTest do {:ok, sockets} = Gatherer.open_sockets(gatherer) - [%Candidate.Host{} = c] = Gatherer.gather_host_candidates(gatherer, sockets) + {_local_preferences, [%Candidate.Host{} = c]} = + Gatherer.gather_host_candidates(gatherer, %{}, sockets) assert :ok = Gatherer.gather_srflx_candidate(gatherer, 1234, c.base.socket, stun_server) assert packet = Transport.Mock.recv(c.base.socket) diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index 2dc3eae..acf834c 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -62,8 +62,8 @@ defmodule ExICE.Priv.ICEAgentTest do end end - @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445) - @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445) + @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445, priority: 123) + @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 122) describe "unmarshal_remote_candidate/1" do test "with correct candidate" do @@ -294,6 +294,7 @@ defmodule ExICE.Priv.ICEAgentTest do port: 1234, base_address: host_cand.base.base_address, base_port: host_cand.base.base_port, + priority: host_cand.base.priority - 1, transport_module: ice_agent.transport_module, socket: socket ) @@ -1451,7 +1452,7 @@ defmodule ExICE.Priv.ICEAgentTest do end test "request", %{ice_agent: ice_agent} do - rcand1 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 2}, port: 8445) + rcand1 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 2}, port: 8445, priority: 123) ice_agent = ICEAgent.add_remote_candidate(ice_agent, rcand1) [socket] = ice_agent.sockets @@ -1480,8 +1481,8 @@ defmodule ExICE.Priv.ICEAgentTest do end test "success response", %{ice_agent: ice_agent} do - rcand1 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 2}, port: 8445) - rcand2 = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445) + rcand1 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 2}, port: 8445, priority: 122) + rcand2 = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 123) ice_agent = ICEAgent.add_remote_candidate(ice_agent, rcand1) @@ -1543,7 +1544,7 @@ defmodule ExICE.Priv.ICEAgentTest do # this test checks if we move from checking directly to complete # when eoc is set and local candidates gathering has finished - rcand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445) + rcand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445, priority: 123) ice_agent = ice_agent @@ -1570,7 +1571,7 @@ defmodule ExICE.Priv.ICEAgentTest do end test "concluding", %{ice_agent: ice_agent} do - rcand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445) + rcand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445, priority: 123) ice_agent = ICEAgent.add_remote_candidate(ice_agent, rcand1) @@ -1901,9 +1902,9 @@ defmodule ExICE.Priv.ICEAgentTest do @tag :debug test "agent state and behavior after it completes" do - r_cand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445) - r_cand2 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 4}, port: 8445) - r_cand3 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 5}, port: 8445) + r_cand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 123) + r_cand2 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 4}, port: 8445, priority: 120) + r_cand3 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 5}, port: 8445, priority: 119) ice_agent = ICEAgent.new( @@ -2027,7 +2028,7 @@ defmodule ExICE.Priv.ICEAgentTest do assert nil == Transport.Mock.recv(socket) # try to handle binding request from unknown remote candidate - prflx_cand = ExICE.Candidate.new(:prflx, address: {192, 168, 0, 6}, port: 8445) + prflx_cand = ExICE.Candidate.new(:prflx, address: {192, 168, 0, 6}, port: 8445, priority: 122) new_ice_agent = ICEAgent.handle_udp(ice_agent, socket, prflx_cand.address, prflx_cand.port, req) @@ -2395,6 +2396,7 @@ defmodule ExICE.Priv.ICEAgentTest do assert [%{state: :failed}] = Map.values(ice_agent.checklist) end + @tag :debug test "relay connection" do remote_cand_ip = @remote_cand.address remote_cand_port = @remote_cand.port From 036670670f133cd908bc5f81aecd9dcb8b588a8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Fri, 14 Mar 2025 13:17:26 +0100 Subject: [PATCH 5/8] Remove dbg, debug tags and commented code --- lib/ex_ice/candidate.ex | 11 +---------- test/priv/ice_agent_test.exs | 2 -- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/lib/ex_ice/candidate.ex b/lib/ex_ice/candidate.ex index b05e05c..4113b6a 100644 --- a/lib/ex_ice/candidate.ex +++ b/lib/ex_ice/candidate.ex @@ -93,15 +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 = Keyword.fetch!(config, :priority) - # if config[:priority] do - # config[:priority] - # else - # base_address = config[:base_address] || config[:address] || raise "no address" - # ExICE.Priv.Candidate.priority(base_address, type) - # end - address = Keyword.fetch!(config, :address) %__MODULE__{ @@ -111,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 } diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index acf834c..04393d7 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -1900,7 +1900,6 @@ defmodule ExICE.Priv.ICEAgentTest do assert failed_ice_agent == new_ice_agent end - @tag :debug test "agent state and behavior after it completes" do r_cand1 = ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 123) r_cand2 = ExICE.Candidate.new(:srflx, address: {192, 168, 0, 4}, port: 8445, priority: 120) @@ -2396,7 +2395,6 @@ defmodule ExICE.Priv.ICEAgentTest do assert [%{state: :failed}] = Map.values(ice_agent.checklist) end - @tag :debug test "relay connection" do remote_cand_ip = @remote_cand.address remote_cand_port = @remote_cand.port From b7534b5756d14db773813d4d1c92f85012e05308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Fri, 14 Mar 2025 13:26:37 +0100 Subject: [PATCH 6/8] Use Candidate.priority! when possible, remove solved TODO --- lib/ex_ice/ice_agent.ex | 4 ++-- lib/ex_ice/priv/ice_agent.ex | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/ex_ice/ice_agent.ex b/lib/ex_ice/ice_agent.ex index 61cc23d..296e882 100644 --- a/lib/ex_ice/ice_agent.ex +++ b/lib/ex_ice/ice_agent.ex @@ -63,9 +63,9 @@ defmodule ExICE.ICEAgent do * `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 (no need for 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 start sending data. + a pair to be nominated by the controlling agent before they can start sending data. Defaults to true. * `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`. diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index 907b0f1..f485193 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -1080,7 +1080,7 @@ defmodule ExICE.Priv.ICEAgent do ] # Use sock_addr for calculating priority. - # In other case, we might duplicate priority. + # In other case, we might get duplicates. {:ok, {sock_addr, _sock_port}} = ice_agent.transport_module.sockname(tr.socket) {local_preferences, priority} = @@ -1758,10 +1758,7 @@ defmodule ExICE.Priv.ICEAgent do nil -> {:ok, {base_addr, base_port}} = ice_agent.transport_module.sockname(tr.socket) - {local_preferences, priority} = - Candidate.priority(ice_agent.local_preferences, base_addr, :srflx) - - ice_agent = %__MODULE__{ice_agent | local_preferences: local_preferences} + priority = Candidate.priority!(ice_agent.local_preferences, base_addr, :srflx) cand = Candidate.Srflx.new( @@ -2109,7 +2106,6 @@ defmodule ExICE.Priv.ICEAgent do defp get_or_create_remote_cand(ice_agent, src_ip, src_port, prio_attr) do case find_remote_cand(Map.values(ice_agent.remote_cands), src_ip, src_port) do nil -> - # TODO calculate correct prio using prio_attr cand = ExICE.Candidate.new(:prflx, address: src_ip, From 2d49987243446efa3efc89790667d415d4e7aaa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Fri, 14 Mar 2025 13:42:31 +0100 Subject: [PATCH 7/8] Test Candidate.priority/3 --- test/priv/candidate_test.exs | 43 ++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 test/priv/candidate_test.exs diff --git a/test/priv/candidate_test.exs b/test/priv/candidate_test.exs new file mode 100644 index 0000000..7ba719f --- /dev/null +++ b/test/priv/candidate_test.exs @@ -0,0 +1,43 @@ +defmodule ExICE.Priv.CandidateTest do + use ExUnit.Case, async: true + + alias ExICE.Priv.Candidate + + test "priority/3" do + {local_preferences1, prio1} = Candidate.priority(%{}, {192, 168, 0, 1}, :host) + + assert map_size(local_preferences1) == 1 + assert Map.has_key?(local_preferences1, {192, 168, 0, 1}) + + # is idempotent + {^local_preferences1, ^prio1} = + Candidate.priority(local_preferences1, {192, 168, 0, 1}, :host) + + {local_preferences2, prio2} = Candidate.priority(local_preferences1, {192, 168, 0, 2}, :host) + assert map_size(local_preferences2) == 2 + assert Map.has_key?(local_preferences2, {192, 168, 0, 1}) + assert Map.has_key?(local_preferences2, {192, 168, 0, 2}) + assert prio2 != prio1 + + # the same base address that created srflx candidate + {^local_preferences2, prio3} = + Candidate.priority(local_preferences2, {192, 168, 0, 1}, :srflx) + + assert prio3 < prio2 + assert prio3 < prio1 + + # the same base address that created relay candidate + {^local_preferences2, prio4} = + Candidate.priority(local_preferences2, {192, 168, 0, 1}, :relay) + + assert prio4 < prio3 + + # the same base address that created prflx candidate + {^local_preferences2, prio5} = + Candidate.priority(local_preferences2, {192, 168, 0, 1}, :prflx) + + assert prio5 < prio1 + assert prio5 < prio2 + assert prio5 > prio3 + end +end From 72953cd6182aac3626159e107756865b2a9aa286 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=9Aled=C5=BA?= Date: Mon, 17 Mar 2025 15:21:34 +0100 Subject: [PATCH 8/8] Turn aggressive nomination off by default --- lib/ex_ice/ice_agent.ex | 2 +- lib/ex_ice/priv/ice_agent.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ex_ice/ice_agent.ex b/lib/ex_ice/ice_agent.ex index 296e882..1d88956 100644 --- a/lib/ex_ice/ice_agent.ex +++ b/lib/ex_ice/ice_agent.ex @@ -66,7 +66,7 @@ defmodule ExICE.ICEAgent do 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 true. + 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`. diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index f485193..5ed373a 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -151,7 +151,7 @@ defmodule ExICE.Priv.ICEAgent do on_gathering_state_change: opts[:on_gathering_state_change] || controlling_process, on_data: opts[:on_data] || controlling_process, on_new_candidate: opts[:on_new_candidate] || controlling_process, - aggressive_nomination: Keyword.get(opts, :aggressive_nomination, true), + aggressive_nomination: Keyword.get(opts, :aggressive_nomination, false), if_discovery_module: if_discovery_module, transport_module: transport_module, gatherer: Gatherer.new(if_discovery_module, transport_module, ip_filter, ports),