Merge pull request #682 from glennmatthews/linux_network_ipaddress_cornercase

ipaddress on Linux - default route pointing to unaddressed interface, with route src

2 files changed, 283 insertions, 31 deletions

diff --git a/lib/ohai/plugins/linux/network.rb b/lib/ohai/plugins/linux/network.rb
index 32b77650..b46448ef 100644
--- a/lib/ohai/plugins/linux/network.rb
+++ b/lib/ohai/plugins/linux/network.rb
@@ -113,7 +113,15 @@ Ohai.plugin(:Network) do
 
         # a sanity check, especially for Linux-VServer, OpenVZ and LXC:
         # don't report the route entry if the src address isn't set on the node
-        next if route_entry[:src] and not iface[route_int][:addresses].has_key? route_entry[:src]
+        # unless the interface has no addresses of this type at all
+        if route_entry[:src]
+          addr = iface[route_int][:addresses]
+          unless addr.nil? || addr.has_key?(route_entry[:src]) ||
+              addr.values.all? { |a| a["family"] != family[:name] }
+            Ohai::Log.debug("Skipping route entry whose src does not match the interface IP")
+            next
+          end
+        end
 
         iface[route_int][:routes] = Array.new unless iface[route_int][:routes]
         iface[route_int][:routes] << route_entry
@@ -126,6 +134,7 @@ Ohai.plugin(:Network) do
   # for information, default routes can be of this form :
   # - default via 10.0.2.4 dev br0
   # - default dev br0  scope link
+  # - default dev eth0  scope link src 1.1.1.1
   # - default via 10.0.3.1 dev eth1  src 10.0.3.2  metric 10
   # - default via 10.0.4.1 dev eth2  src 10.0.4.2  metric 20
 
@@ -310,46 +319,71 @@ Ohai.plugin(:Network) do
 
   # returns the macaddress for interface from a hash of interfaces (iface elsewhere in this file)
   def get_mac_for_interface(interfaces, interface)
-    interfaces[interface][:addresses].select { |k, v| v["family"] == "lladdr" }.first.first unless interfaces[interface][:flags].include? "NOARP"
+    interfaces[interface][:addresses].select { |k, v| v["family"] == "lladdr" }.first.first unless interfaces[interface][:addresses].nil? || interfaces[interface][:flags].include?("NOARP")
   end
 
   # returns the default route with the lowest metric (unspecified metric is 0)
   def choose_default_route(routes)
-    default_route = routes.select do |r|
+    routes.select do |r|
       r[:destination] == "default"
     end.sort do |x, y|
       (x[:metric].nil? ? 0 : x[:metric].to_i) <=> (y[:metric].nil? ? 0 : y[:metric].to_i)
     end.first
   end
 
+  def interface_has_no_addresses_in_family?(iface, family)
+    return true if iface[:addresses].nil?
+    iface[:addresses].values.all? { |addr| addr["family"] != family }
+  end
+
+  def interface_have_address?(iface, address)
+    return false if iface[:addresses].nil?
+    iface[:addresses].key?(address)
+  end
+
+  def interface_address_not_link_level?(iface, address)
+    iface[:addresses][address][:scope].downcase != "link"
+  end
+
+  def interface_valid_for_route?(iface, address, family)
+    return true if interface_has_no_addresses_in_family?(iface, family)
+
+    interface_have_address?(iface, address) && interface_address_not_link_level?(iface, address)
+  end
+
+  def route_is_valid_default_route?(route, default_route)
+    # if the route destination is a default route, it's good
+    return true if route[:destination] == "default"
+
+    # the default route has a gateway and the route matches the gateway
+    !default_route[:via].nil? && IPAddress(route[:destination]).include?(IPAddress(default_route[:via]))
+  end
+
   # ipv4/ipv6 routes are different enough that having a single algorithm to select the favored route for both creates unnecessary complexity
   # this method attempts to deduce the route that is most important to the user, which is later used to deduce the favored values for {ip,mac,ip6}address
   # we only consider routes that are default routes, or those routes that get us to the gateway for a default route
   def favored_default_route(routes, iface, default_route, family)
     routes.select do |r|
       if family[:name] == "inet"
-        # selecting routes for ipv4
-        # using the source field when it's specified :
-        # 1) in the default route
-        # 2) in the route entry used to reach the default gateway
-        r[:src] and # it has a src field
-          iface[r[:dev]] and # the iface exists
-          iface[r[:dev]][:addresses].has_key? r[:src] and # the src ip is set on the node
-          iface[r[:dev]][:addresses][r[:src]][:scope].downcase != "link" and # this isn't a link level addresse
-          ( r[:destination] == "default" or
-            ( default_route[:via] and # the default route has a gateway
-              IPAddress(r[:destination]).include? IPAddress(default_route[:via]) # the route matches the gateway
-              )
-            )
+        # the route must have a source address
+        next if r[:src].nil? || r[:src].empty?
+
+        # the interface specified in the route must exist
+        route_interface = iface[r[:dev]]
+        next if route_interface.nil? # the interface specified in the route must exist
+
+        # the interface must have no addresses, or if it has the source address, the address must not
+        # be a link-level address
+        next unless interface_valid_for_route?(route_interface, r[:src], "inet")
+
+        # the route must either be a default route, or it must have a gateway which is accessible via the route
+        next unless route_is_valid_default_route?(r, default_route)
+
+        true
       elsif family[:name] == "inet6"
-        # selecting routes for ipv6
-        iface[r[:dev]] and # the iface exists
-          iface[r[:dev]][:state] == "up" and # the iface is up
-          ( r[:destination] == "default" or
-            ( default_route[:via] and # the default route has a gateway
-              IPAddress(r[:destination]).include? IPAddress(default_route[:via]) # the route matches the gateway
-              )
-            )
+        iface[r[:dev]] &&
+          iface[r[:dev]][:state] == "up" &&
+          route_is_valid_default_route?(r, default_route)
       end
     end.sort_by do |r|
       # sorting the selected routes:
@@ -430,11 +464,11 @@ Ohai.plugin(:Network) do
         default_route = choose_default_route(routes)
 
         if default_route.nil? or default_route.empty?
-          attribute_name == if family[:name] == "inet"
-                              "default_interface"
-                            else
-                              "default_#{family[:name]}_interface"
-                            end
+          attribute_name = if family[:name] == "inet"
+                             "default_interface"
+                           else
+                             "default_#{family[:name]}_interface"
+                           end
           Ohai::Log.debug("Unable to determine '#{attribute_name}' as no default routes were found for that interface family")
         else
           network["#{default_prefix}_interface"] = default_route[:dev]
diff --git a/spec/unit/plugins/linux/network_spec.rb b/spec/unit/plugins/linux/network_spec.rb
index 213d0e2d..40bbdc17 100644
--- a/spec/unit/plugins/linux/network_spec.rb
+++ b/spec/unit/plugins/linux/network_spec.rb
@@ -138,6 +138,14 @@ xapi1     Link encap:Ethernet  HWaddr E8:39:35:C5:C8:50
           TX packets:6 errors:0 dropped:0 overruns:0 carrier:0
           collisions:0 txqueuelen:0
           RX bytes:21515031 (20.5 MiB)  TX bytes:2052 (2.0 KiB)
+
+fwdintf   Link encap:Ethernet  HWaddr 00:00:00:00:00:0a
+          inet6 addr: fe80::200:ff:fe00:a/64 Scope:Link
+          UP RUNNING NOARP MULTICAST  MTU:1496  Metric:1
+          RX packets:0 errors:0 dropped:0 overruns:0 frame:0
+          TX packets:2 errors:0 dropped:1 overruns:0 carrier:0
+          collisions:0 txqueuelen:1000
+          RX bytes:0 (0.0 B)  TX bytes:140 (140.0 B)
 EOM
 # Note that ifconfig shows foo:veth0@eth0 but fails to show any address information.
 # This was not a mistake collecting the output and Apparently ifconfig is broken in this regard.
@@ -225,6 +233,8 @@ EOM
     link/ether e8:39:35:c5:c8:50 brd ff:ff:ff:ff:ff:ff
     inet 192.168.13.34/24 brd 192.168.13.255 scope global xapi1
        valid_lft forever preferred_lft forever
+13: fwdintf: <MULTICAST,NOARP,UP,LOWER_UP> mtu 1496 qdisc pfifo_fast state UNKNOWN group default qlen 1000
+    link/ether 00:00:00:00:00:0a brd ff:ff:ff:ff:ff:ff
 EOM
   }
 
@@ -278,6 +288,12 @@ EOM
     21468183   159866   0       0       0       0
     TX: bytes  packets  errors  dropped carrier collsns
     2052       6        0       0       0       0
+13: fwdintf: <MULTICAST,NOARP,UP,LOWER_UP> mtu 1496 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 1000
+    link/ether 00:00:00:00:00:0a brd ff:ff:ff:ff:ff:ff promiscuity 0
+    RX: bytes  packets  errors  dropped overrun mcast
+    0          0        0       0       0       0
+    TX: bytes  packets  errors  dropped carrier collsns
+    140        2        0       1       0       0
 EOM
   }
 
@@ -359,8 +375,162 @@ EOM
     end
   end
 
-  %w{ifconfig iproute2}.each do |network_method|
+  describe '#interface_has_no_addresses_in_family?' do
+    context "when interface has no addresses" do
+      let(:iface) { {} }
+
+      it "returns true" do
+        expect(plugin.interface_has_no_addresses_in_family?(iface, "inet")).to eq(true)
+      end
+    end
+
+    context "when an interface has no addresses in family" do
+      let(:iface) { { addresses: { "1.2.3.4" => { "family" => "inet6" } } } }
+
+      it "returns true" do
+        expect(plugin.interface_has_no_addresses_in_family?(iface, "inet")).to eq(true)
+      end
+    end
+
+    context "when an interface has addresses in family" do
+      let(:iface) { { addresses: { "1.2.3.4" => { "family" => "inet" } } } }
+
+      it "returns false" do
+        expect(plugin.interface_has_no_addresses_in_family?(iface, "inet")).to eq(false)
+      end
+    end
+  end
+
+  describe '#interface_have_address?' do
+    context "when interface has no addresses" do
+      let(:iface) { {} }
+
+      it "returns false" do
+        expect(plugin.interface_have_address?(iface, "1.2.3.4")).to eq(false)
+      end
+    end
+
+    context "when interface has a matching address" do
+      let(:iface) { { addresses: { "1.2.3.4" => {} } } }
+
+      it "returns true" do
+        expect(plugin.interface_have_address?(iface, "1.2.3.4")).to eq(true)
+      end
+    end
+
+    context "when interface does not have a matching address" do
+      let(:iface) { { addresses: { "4.3.2.1" => {} } } }
+
+      it "returns false" do
+        expect(plugin.interface_have_address?(iface, "1.2.3.4")).to eq(false)
+      end
+    end
+  end
+
+  describe '#interface_address_not_link_level?' do
+    context "when the address scope is link" do
+      let(:iface) { { addresses: { "1.2.3.4" => { scope: "Link" } } } }
+
+      it "returns false" do
+        expect(plugin.interface_address_not_link_level?(iface, "1.2.3.4")).to eq(false)
+      end
+    end
+
+    context "when the address scope is global" do
+      let(:iface) { { addresses: { "1.2.3.4" => { scope: "Global" } } } }
+
+      it "returns true" do
+        expect(plugin.interface_address_not_link_level?(iface, "1.2.3.4")).to eq(true)
+      end
+    end
+  end
+
+  describe '#interface_valid_for_route?' do
+    let(:iface)   { double("iface") }
+    let(:address) { "1.2.3.4" }
+    let(:family)  { "inet" }
+
+    context "when interface has no addresses" do
+      it "returns true" do
+        expect(plugin).to receive(:interface_has_no_addresses_in_family?).with(iface, family).and_return(true)
+        expect(plugin.interface_valid_for_route?(iface, address, family)).to eq(true)
+      end
+    end
+
+    context "when interface has addresses" do
+      before do
+        expect(plugin).to receive(:interface_has_no_addresses_in_family?).with(iface, family).and_return(false)
+      end
+
+      context "when interface contains the address" do
+        before do
+          expect(plugin).to receive(:interface_have_address?).with(iface, address).and_return(true)
+        end
+
+        context "when interface address is not a link-level address" do
+          it "returns true" do
+            expect(plugin).to receive(:interface_address_not_link_level?).with(iface, address).and_return(true)
+            expect(plugin.interface_valid_for_route?(iface, address, family)).to eq(true)
+          end
+        end
+
+        context "when the interface address is a link-level address" do
+          it "returns false" do
+            expect(plugin).to receive(:interface_address_not_link_level?).with(iface, address).and_return(false)
+            expect(plugin.interface_valid_for_route?(iface, address, family)).to eq(false)
+          end
+        end
+      end
+
+      context "when interface does not contain the address" do
+        it "returns false" do
+          expect(plugin).to receive(:interface_have_address?).with(iface, address).and_return(false)
+          expect(plugin.interface_valid_for_route?(iface, address, family)).to eq(false)
+        end
+      end
+    end
+  end
+
+  describe '#route_is_valid_default_route?' do
+    context "when the route destination is default" do
+      let(:route)         { { destination: "default" } }
+      let(:default_route) { double("default_route") }
+
+      it "returns true" do
+        expect(plugin.route_is_valid_default_route?(route, default_route)).to eq(true)
+      end
+    end
+
+    context "when the route destination is not default" do
+      let(:route) { { destination: "10.0.0.0/24" } }
+
+      context "when the default route does not have a gateway" do
+        let(:default_route) { {} }
+
+        it "returns false" do
+          expect(plugin.route_is_valid_default_route?(route, default_route)).to eq(false)
+        end
+      end
+
+      context "when the gateway is within the destination" do
+        let(:default_route) { { via: "10.0.0.1" } }
+
+        it "returns true" do
+          expect(plugin.route_is_valid_default_route?(route, default_route)).to eq(true)
+        end
+      end
+
+      context "when the gateway is not within the destination" do
+        let(:default_route) { { via: "20.0.0.1" } }
 
+        it "returns false" do
+          expect(plugin.route_is_valid_default_route?(route, default_route)).to eq(false)
+        end
+      end
+    end
+  end
+
+  %w{ifconfig iproute2}.each do |network_method|
     describe "gathering IP layer address info via #{network_method}" do
       before(:each) do
         allow(plugin).to receive(:iproute2_binary_available?).and_return( network_method == "iproute2" )
@@ -374,7 +544,7 @@ EOM
       end
 
       it "detects the interfaces" do
-        expect(plugin["network"]["interfaces"].keys.sort).to eq(["eth0", "eth0.11", "eth0.151", "eth0.152", "eth0.153", "eth0:5", "eth3", "foo:veth0@eth0", "lo", "ovs-system", "tun0", "venet0", "venet0:0", "xapi1"])
+        expect(plugin["network"]["interfaces"].keys.sort).to eq(["eth0", "eth0.11", "eth0.151", "eth0.152", "eth0.153", "eth0:5", "eth3", "foo:veth0@eth0", "fwdintf", "lo", "ovs-system", "tun0", "venet0", "venet0:0", "xapi1"])
       end
 
       it "detects the layer one details of an ethernet interface" do
@@ -896,6 +1066,54 @@ EOM
         end
       end
 
+      describe "with a link level default route to an unaddressed int" do
+        let(:linux_ip_route) { <<-EOM
+default dev eth3 scope link
+EOM
+        }
+
+        before(:each) do
+          plugin.run
+        end
+
+        it "completes the run" do
+          expect(Ohai::Log).not_to receive(:debug).with(/Plugin linux::network threw exception/)
+          expect(plugin["network"]).not_to be_nil
+        end
+
+        it "sets default_interface" do
+          expect(plugin["network"]["default_interface"]).to eq("eth3")
+        end
+
+        it "doesn't set ipaddress" do
+          expect(plugin["ipaddress"]).to be_nil
+        end
+      end
+
+      describe "with a link level default route with a source" do
+        let(:linux_ip_route) { <<-EOM
+default dev fwdintf scope link src 2.2.2.2
+EOM
+        }
+
+        before(:each) do
+          plugin.run
+        end
+
+        it "completes the run" do
+          expect(Ohai::Log).not_to receive(:debug).with(/Plugin linux::network threw exception/)
+          expect(plugin["network"]).not_to be_nil
+        end
+
+        it "sets default_interface" do
+          expect(plugin["network"]["default_interface"]).to eq("fwdintf")
+        end
+
+        it "sets ipaddress" do
+          expect(plugin["ipaddress"]).to eq("2.2.2.2")
+        end
+      end
+
       describe "when not having a global scope ipv6 address" do
         let(:linux_ip_route_inet6) { <<-EOM
 fe80::/64 dev eth0  proto kernel  metric 256