Skip to content

Commit

Permalink
Centralize interface connectivity in the RouteManager
Browse files Browse the repository at this point in the history
Network interfaces can be disconnected, lan-connected or
internet-connected. This information was managed both in the
RouteManager and in the connectivity checkers. This moves management to
the RouteManager. The logic is:

1. The RouteManager has the final say, since it countrols the routing
   tables.
2. The RouteManager publishes the summary properties that say what the
   active interface is and whether the device as a whole is
   internet-connected, lan-connected, or disconnected. It was possible
   for the RouteManager to think that the device as a whole was lan-
   connected when it really was internet-connected. Managing this in
   one place removes the bug.
  • Loading branch information
fhunleth committed May 20, 2021
1 parent f07ffc3 commit 2daa2df
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 45 deletions.
6 changes: 3 additions & 3 deletions lib/vintage_net/interface.ex
Original file line number Diff line number Diff line change
Expand Up @@ -736,9 +736,9 @@ defmodule VintageNet.Interface do

if state != :configured do
# Once a state is `:configured`, then the configuration provides the connection
# status. When not configured, report it as `:disconnected` to avoid any confusion
# with stale or unset values.
PropertyTable.put(VintageNet, ["interface", ifname, "connection"], :disconnected)
# status. When not configured, make sure there are no routing tables entries or
# stale properties.
RouteManager.set_connection_status(ifname, :disconnected)
end
end

Expand Down
7 changes: 3 additions & 4 deletions lib/vintage_net/interface/internet_connectivity_checker.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule VintageNet.Interface.InternetConnectivityChecker do
require Logger

alias VintageNet.Interface.{Classification, InternetTester}
alias VintageNet.{PropertyTable, RouteManager}
alias VintageNet.RouteManager

@moduledoc """
This GenServer monitors a network interface for Internet connectivity
Expand Down Expand Up @@ -163,10 +163,9 @@ defmodule VintageNet.Interface.InternetConnectivityChecker do
# It's desirable to set these even if redundant since the checks in this
# modules are authoritative. I.e., the internet isn't connected unless we
# declare it detected. Other modules can reset the connection to :lan
# if, for example, a new IP address gets set by DHCP. The following functions
# will optimize out redundant calls if they really are redundant.
# if, for example, a new IP address gets set by DHCP. The following call
# will optimize out redundant updates if they really are redundant.
RouteManager.set_connection_status(ifname, connectivity)
PropertyTable.put(VintageNet, ["interface", ifname, "connection"], connectivity)
state
end

Expand Down
17 changes: 6 additions & 11 deletions lib/vintage_net/interface/lan_connectivity_checker.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ defmodule VintageNet.Interface.LANConnectivityChecker do
use GenServer
require Logger

alias VintageNet.{PropertyTable, RouteManager}
alias VintageNet.RouteManager

@moduledoc """
This GenServer monitors a network interface for LAN connectivity
Expand Down Expand Up @@ -35,12 +35,12 @@ defmodule VintageNet.Interface.LANConnectivityChecker do

case VintageNet.get(lower_up_property(ifname)) do
true ->
set_connectivity(ifname, :lan)
RouteManager.set_connection_status(ifname, :lan)

_not_true ->
# If the physical layer isn't up, don't start polling until
# we're notified that it is available.
set_connectivity(ifname, :disconnected)
RouteManager.set_connection_status(ifname, :disconnected)
end

{:noreply, state}
Expand All @@ -52,7 +52,7 @@ defmodule VintageNet.Interface.LANConnectivityChecker do
%{ifname: ifname} = state
) do
# Physical layer is down. We're definitely disconnected.
set_connectivity(ifname, :disconnected)
RouteManager.set_connection_status(ifname, :disconnected)
{:noreply, state}
end

Expand All @@ -64,7 +64,7 @@ defmodule VintageNet.Interface.LANConnectivityChecker do
# Physical layer is up. Optimistically assume that the LAN is accessible.

# NOTE: Consider triggering based on whether the interface has an IP address or not.
set_connectivity(ifname, :lan)
RouteManager.set_connection_status(ifname, :lan)

{:noreply, state}
end
Expand All @@ -75,15 +75,10 @@ defmodule VintageNet.Interface.LANConnectivityChecker do
%{ifname: ifname} = state
) do
# The interface was completely removed!
if old_value, do: set_connectivity(ifname, :disconnected)
if old_value, do: RouteManager.set_connection_status(ifname, :disconnected)
{:noreply, state}
end

defp set_connectivity(ifname, connectivity) do
RouteManager.set_connection_status(ifname, connectivity)
PropertyTable.put(VintageNet, ["interface", ifname, "connection"], connectivity)
end

defp lower_up_property(ifname) do
["interface", ifname, "lower_up"]
end
Expand Down
10 changes: 10 additions & 0 deletions lib/vintage_net/route/properties.ex
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,14 @@ defmodule VintageNet.Route.Properties do
for {:local_route, ifname, _address, _subnet_bits, metric, :main} <- routes,
do: {metric, ifname}
end

@doc """
Update the every interface's connection status
"""
@spec update_connection_status(Calculator.interface_infos()) :: :ok
def update_connection_status(infos) do
Enum.each(infos, fn {ifname, %{status: status}} ->
VintageNet.PropertyTable.put(VintageNet, ["interface", ifname, "connection"], status)
end)
end
end
77 changes: 50 additions & 27 deletions lib/vintage_net/route_manager.ex
Original file line number Diff line number Diff line change
Expand Up @@ -125,25 +125,7 @@ defmodule VintageNet.RouteManager do
def handle_call({:set_route, ifname, ip_subnets, default_gateway, status}, _from, state) do
Logger.info("RouteManager: set_route #{ifname} -> #{inspect(status)}")

# The weight parameter prioritizes interfaces of the same type and connectivity.
# All weights for interfaces of the same time must be different. I.e., we don't
# leave it to chance which one is used. Also, bandwidth sharing of interfaces
# can't be accomplished by giving interfaces the same low level priority with
# how things are set up anyway.
#
# It will likely be necessary to expose this to users who have more than one
# of the same interface type available. For now, lower numbered interfaces
# have priority. For example, eth0 is used over eth1, etc. The 10 is hardcoded
# to correspond to the calculation in classification.ex.
weight = rem(Classification.to_instance(ifname), 10)

ifentry = %InterfaceInfo{
interface_type: Classification.to_type(ifname),
weight: weight,
ip_subnets: ip_subnets,
default_gateway: default_gateway,
status: status
}
ifentry = new_interface_info(ifname, ip_subnets, default_gateway, status)

new_state =
put_in(state.interfaces[ifname], ifentry)
Expand All @@ -163,17 +145,20 @@ defmodule VintageNet.RouteManager do

@impl GenServer
def handle_call({:clear_route, ifname}, _from, state) do
new_state =
if Map.has_key?(state.interfaces, ifname) do
Logger.info("RouteManager: clear_route #{ifname}")
if Map.has_key?(state.interfaces, ifname) do
Logger.info("RouteManager: clear_route #{ifname}")

# Need to force the property to disconnected since we're removing it from the map.
VintageNet.PropertyTable.put(VintageNet, ["interface", ifname, "connection"], :disconnected)

new_state =
%{state | interfaces: Map.delete(state.interfaces, ifname)}
|> update_route_tables()
else
state
end

{:reply, :ok, new_state}
{:reply, :ok, new_state}
else
{:reply, :ok, state}
end
end

@impl GenServer
Expand All @@ -186,6 +171,28 @@ defmodule VintageNet.RouteManager do
{:reply, :ok, new_state}
end

defp new_interface_info(ifname, ip_subnets, default_gateway, status) do
# The weight parameter prioritizes interfaces of the same type and connectivity.
# All weights for interfaces of the same time must be different. I.e., we don't
# leave it to chance which one is used. Also, bandwidth sharing of interfaces
# can't be accomplished by giving interfaces the same low level priority with
# how things are set up anyway.
#
# It will likely be necessary to expose this to users who have more than one
# of the same interface type available. For now, lower numbered interfaces
# have priority. For example, eth0 is used over eth1, etc. The 10 is hardcoded
# to correspond to the calculation in classification.ex.
weight = rem(Classification.to_instance(ifname), 10)

%InterfaceInfo{
interface_type: Classification.to_type(ifname),
weight: weight,
ip_subnets: ip_subnets,
default_gateway: default_gateway,
status: status
}
end

# Only process routes if the status changes
defp update_connection_status(
%State{interfaces: interfaces} = state,
Expand All @@ -194,7 +201,14 @@ defmodule VintageNet.RouteManager do
) do
case interfaces[ifname] do
nil ->
state
Logger.warn(
"RouteManager: set_connection_status to #{inspect(new_status)} on unknown ifname: #{ifname}"
)

ifentry = new_interface_info(ifname, [], nil, new_status)

put_in(state.interfaces[ifname], ifentry)
|> update_route_tables()

ifentry ->
if ifentry.status != new_status do
Expand All @@ -219,8 +233,17 @@ defmodule VintageNet.RouteManager do
Enum.each(route_delta, &handle_delta/1)

# Update the global routing properties in the property table
# NOTE: These next three calls can update zero or more entries
# in the property table. There's no notion of atomicity,
# so it's possible for listeners to detect inconsistencies.
# All orderings are problematic in some scenario. However,
# in practice, a user is mostly listening either to the
# overall state (which interfaces are available on the device
# or whether the device can reach the internet in any way) or
# it's specifically interested in one interface.
Properties.update_available_interfaces(new_routes)
Properties.update_best_connection(state.interfaces)
Properties.update_connection_status(state.interfaces)

%{state | route_state: new_route_state, routes: new_routes}
end
Expand Down
40 changes: 40 additions & 0 deletions test/vintage_net/route/properties_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,44 @@ defmodule VintageNet.Route.PropertiesTest do
assert status == VintageNet.get(["connection"])
end
end

test "updates connection status" do
interfaces = %{
"eth0" => %InterfaceInfo{
interface_type: :ethernet,
status: :lan,
weight: 0,
ip_subnets: [{{192, 168, 1, 50}, 24}],
default_gateway: {192, 168, 1, 1}
},
"wlan0" => %InterfaceInfo{
interface_type: :wifi,
status: :internet,
weight: 0,
ip_subnets: [{{192, 168, 1, 60}, 24}],
default_gateway: {192, 168, 1, 1}
},
"usb0" => %InterfaceInfo{
interface_type: :local,
status: :disconnected,
weight: 0,
ip_subnets: [{{192, 168, 1, 70}, 24}],
default_gateway: {192, 168, 1, 1}
},
"wwan0" => %InterfaceInfo{
interface_type: :mobile,
status: :internet,
weight: 0,
ip_subnets: [{{192, 168, 1, 70}, 24}],
default_gateway: {192, 168, 1, 1}
}
}

:ok = Properties.update_connection_status(interfaces)

assert :lan == VintageNet.get(["interface", "eth0", "connection"])
assert :internet == VintageNet.get(["interface", "wlan0", "connection"])
assert :disconnected == VintageNet.get(["interface", "usb0", "connection"])
assert :internet == VintageNet.get(["interface", "wwan0", "connection"])
end
end

0 comments on commit 2daa2df

Please sign in to comment.