refactor #favored_default_route to be easier to understand and debug, tests added

2 files changed, 199 insertions, 36 deletions

diff --git a/lib/ohai/plugins/linux/network.rb b/lib/ohai/plugins/linux/network.rb
index 1244bcfd..74343231 100644
--- a/lib/ohai/plugins/linux/network.rb
+++ b/lib/ohai/plugins/linux/network.rb
@@ -319,57 +319,65 @@ 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)
-<<<<<<< ac9f5a2ae4ada9f2e6e821418ed3a9039bda9a91
-    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")
->>>>>>> Report an ipaddress in a Linux corner case observed on Cisco IOS XR
+    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] && # it has a src field
-          iface[r[:dev]] && # the iface exists
-          (
-            iface[r[:dev]][:addresses].nil? || # this int has no addresses
-            iface[r[:dev]][:addresses].values.all? { |addr| addr['family'] != family[:name] } || # this int has no ip
-            (
-              iface[r[:dev]][:addresses].has_key?(r[:src]) && # the src ip is set on the node
-              iface[r[:dev]][:addresses][r[:src]][:scope].downcase != "link" # this isn't a link level address
-            )
-          ) && (
-            r[:destination] == "default" ||
-            (
-              default_route[:via] && # 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 field
+        return false unless defined?(r[:src])
+
+        route_interface = iface[r[:dev]]
+
+        # the interface specified in the route must exist
+        return false unless defined?(route_interface) # the interface specified in the route must exist
+
+        interface_valid_for_route?(route_interface, r[:src], "inet") && route_is_valid_default_route?(r, default_route)
       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:
@@ -450,7 +458,7 @@ Ohai.plugin(:Network) do
         default_route = choose_default_route(routes)
 
         if default_route.nil? or default_route.empty?
-          attribute_name == if family[:name] == "inet"
+          attribute_name = if family[:name] == "inet"
                               "default_interface"
                             else
                               "default_#{family[:name]}_interface"
diff --git a/spec/unit/plugins/linux/network_spec.rb b/spec/unit/plugins/linux/network_spec.rb
index 2ff39385..d3ef1e85 100644
--- a/spec/unit/plugins/linux/network_spec.rb
+++ b/spec/unit/plugins/linux/network_spec.rb
@@ -375,8 +375,163 @@ 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" )