Properly detect mac address on ipv6 systems

4 files changed, 39 insertions, 33 deletions

diff --git a/lib/ohai/plugins/linux/network.rb b/lib/ohai/plugins/linux/network.rb
index 689001dc..7aba7943 100644
--- a/lib/ohai/plugins/linux/network.rb
+++ b/lib/ohai/plugins/linux/network.rb
@@ -138,6 +138,7 @@ Ohai.plugin(:Network) do
     end.compact.flatten
   end
 
+  # determine layer 1 details for the interface using ethtool
   def ethernet_layer_one(iface)
     return iface unless ethtool_binary = find_ethtool_binary
     keys = %w[ Speed Duplex Port Transceiver Auto-negotiation MDI-X ]
@@ -161,6 +162,7 @@ Ohai.plugin(:Network) do
     iface
   end
 
+  # determine link stats, vlans, queue length, and state for an interface using ip
   def link_statistics(iface, net_counters)
     so = shell_out("ip -d -s link")
     tmp_int = nil
@@ -251,7 +253,6 @@ Ohai.plugin(:Network) do
     end
   end
 
-
   def parse_ip_addr_link_line(cint, iface, line)
     if line =~ /link\/(\w+) ([\da-f\:]+) /
       iface[cint][:encapsulation] = linux_encaps_lookup($1)
@@ -293,7 +294,7 @@ Ohai.plugin(:Network) do
         iface[cint][:addresses][tmp_addr][:scope] = ($1.eql?("host") ? "Node" : $1.capitalize)
       end
 
-      # If we found we were an an alias interface, restore cint to its original value
+      # If we found we were an alias interface, restore cint to its original value
       cint = original_int unless original_int.nil?
     end
     cint
@@ -364,7 +365,7 @@ Ohai.plugin(:Network) do
         end.first
 
         if default_route.nil? or default_route.empty?
-          Ohai::Log.debug("Unable to determine default #{family[:name]} interface")
+          Ohai::Log.debug("Unable to determine default_#{family[:name]}_interface as no default routes found")
         else
           network["#{default_prefix}_interface"] = default_route[:dev]
           Ohai::Log.debug("#{default_prefix}_interface set to #{default_route[:dev]}")
@@ -406,17 +407,19 @@ Ohai.plugin(:Network) do
             ]
           end.first
 
-          unless route.nil? or route.empty?
+
+          if route && !route.empty?
+            macaddress iface[route[:dev]][:addresses].select{|k,v| v["family"]=="lladdr"}.first.first unless iface[route[:dev]][:flags].include? "NOARP"
             if family[:name] == "inet"
               ipaddress route[:src]
-              macaddress iface[route[:dev]][:addresses].select{|k,v| v["family"]=="lladdr"}.first.first unless iface[route[:dev]][:flags].include? "NOARP"
             else
               ip6address route[:src]
             end
+          else
+            macaddress iface[default_route[:dev]][:addresses].select{|k,v| v["family"]=="lladdr"}.first.first unless iface[default_route[:dev]][:flags].include? "NOARP"
           end
         end
       end
-
     else
 
       begin
diff --git a/lib/ohai/plugins/network.rb b/lib/ohai/plugins/network.rb
index 1cfb9fee..756fa139 100644
--- a/lib/ohai/plugins/network.rb
+++ b/lib/ohai/plugins/network.rb
@@ -26,19 +26,21 @@ Ohai.plugin(:NetworkAddresses) do
 
   depends "network/interfaces"
 
+  # from interf data create array of hashes with ipaddress, scope, and iface
+  # sorted by scope, prefixlen and then ip address where longest prefixes first
   def sorted_ips(family = "inet")
     raise "bad family #{family}" unless [ "inet", "inet6" ].include? family
 
-    # going to use that later to sort by scope
+    # priority of ipv6 link scopes to sort by later
     scope_prio = [ "global", "site", "link", "host", "node", nil ]
 
+    # grab ipaddress, scope, and iface for sorting later
     ipaddresses = []
-    # ipaddresses going to hold #{family} ipaddresses and their scope
     Mash[network['interfaces']].each do |iface, iface_v|
-      next if iface_v.nil? or not iface_v.has_key? 'addresses'
+      next if iface_v.nil? || !iface_v.has_key?('addresses')
       iface_v['addresses'].each do |addr, addr_v|
         next if addr_v.nil? or not addr_v.has_key? "family" or addr_v['family'] != family
-        ipaddresses <<  {
+        ipaddresses << {
           :ipaddress => addr_v["prefixlen"] ? IPAddress("#{addr}/#{addr_v["prefixlen"]}") : IPAddress("#{addr}/#{addr_v["netmask"]}"),
           :scope => addr_v["scope"].nil? ? nil : addr_v["scope"].downcase,
           :iface => iface
@@ -49,17 +51,20 @@ Ohai.plugin(:NetworkAddresses) do
     # sort ip addresses by scope, by prefixlen and then by ip address
     # 128 - prefixlen: longest prefixes first
     ipaddresses.sort_by do |v|
-      [ ( scope_prio.index(v[:scope]) or 999999 ),
+      [ ( scope_prio.index(v[:scope]) || 999999 ),
         128 - v[:ipaddress].prefix.to_i,
         ( family == "inet" ? v[:ipaddress].to_u32 : v[:ipaddress].to_u128 )
       ]
     end
   end
 
+  # finds ip address / interface for interface with default route based on
+  # passed in family.  returns [ipaddress, interface]  uses 1st ip if no default
+  # route is found
   def find_ip(family = "inet")
-    ips=sorted_ips(family)
+    ips = sorted_ips(family)
 
-    # return if there isn't any #{family} address !
+    # return if there aren't any #{family} addresses!
     return [ nil, nil ] if ips.empty?
 
     # shortcuts to access default #{family} interface and gateway
@@ -76,8 +81,8 @@ Ohai.plugin(:NetworkAddresses) do
       end
       if gw_if_ips.empty?
         Ohai::Log.warn("[#{family}] no ip address on #{network[int_attr]}")
-      elsif network[gw_attr] and
-          network["interfaces"][network[int_attr]] and
+      elsif network[gw_attr] &&
+          network["interfaces"][network[int_attr]] &&
           network["interfaces"][network[int_attr]]["addresses"]
         if [ "0.0.0.0", "::", /^fe80:/ ].any? { |pat| pat === network[gw_attr] }
           # link level default route
@@ -109,9 +114,10 @@ Ohai.plugin(:NetworkAddresses) do
     [ r[:ipaddress].to_s, r[:iface] ]
   end
 
+  # select mac address of first interface with family of lladdr
   def find_mac_from_iface(iface)
-    r = network["interfaces"][iface]["addresses"].select{|k,v| v["family"]=="lladdr"}
-    r.nil? or r.first.nil? ? nil : r.first.first
+    r = network["interfaces"][iface]["addresses"].select{|k,v| v["family"] == "lladdr"}
+    r.nil? || r.first.nil? ? nil : r.first.first
   end
 
   def network_contains_address(address_to_match, ipaddress, iface)
@@ -125,10 +131,10 @@ Ohai.plugin(:NetworkAddresses) do
     end
   end
 
-  # ipaddress, ip6address and macaddress are set by the #{os}::network plugin.
-  # atm it is expected macaddress is set at the same time as ipaddress
-  # if ipaddress is set and macaddress is nil, that means the interface
-  # ipaddress is bound to has the NOARP flag
+  # ipaddress, ip6address and macaddress are set for each interface by the
+  # #{os}::network plugin. atm it is expected macaddress is set at the same
+  # time as ipaddress. if ipaddress is set and macaddress is nil, that means
+  # the interface ipaddress is bound to has the NOARP flag
 
   collect_data do
     results = {}
@@ -138,9 +144,10 @@ Ohai.plugin(:NetworkAddresses) do
     counters Mash.new unless counters
     counters[:network] = Mash.new unless counters[:network]
 
-    # inet family is treated before inet6
+    # inet family is processed before inet6
     Ohai::Mixin::NetworkConstants::FAMILIES.keys.sort.each do |family|
       r = {}
+      # find the ip/interface with the default route for this family
       ( r["ip"], r["iface"] ) = find_ip(family)
       r["mac"] = find_mac_from_iface(r["iface"]) unless r["iface"].nil?
       # don't overwrite attributes if they've already been set by the "#{os}::network" plugin
@@ -159,7 +166,9 @@ Ohai.plugin(:NetworkAddresses) do
           Ohai::Log.debug("unable to detect ip6address")
         else
           ip6address r["ip"]
-          if r["mac"] and macaddress.nil? and ipaddress.nil?
+          # don't overwrite macaddress set by "#{os}::network" plugin
+          # and also
+          if r["mac"] and macaddress.nil? and ( ipaddress.nil? || ipaddress == "127.0.0.1" )
             Ohai::Log.debug("macaddress set to #{r["mac"]} from the ipv6 setup")
             macaddress r["mac"]
           end
diff --git a/spec/unit/plugins/linux/network_spec.rb b/spec/unit/plugins/linux/network_spec.rb
index de5a89ce..67cc1758 100644
--- a/spec/unit/plugins/linux/network_spec.rb
+++ b/spec/unit/plugins/linux/network_spec.rb
@@ -663,7 +663,7 @@ Destination     Gateway         Genmask         Flags Metric Ref    Use Iface
         expect(plugin['network']['interfaces']['eth0.11']['routes']).to include Mash.new( :destination => "default", :via => "1111:2222:3333:4444::1", :metric => "1024", :family => "inet6")
       end
 
-      describe "when there isn't a source field in route entries " do
+      describe "when there isn't a source field in route entries" do
         before(:each) do
           plugin.run
         end
diff --git a/spec/unit/plugins/network_spec.rb b/spec/unit/plugins/network_spec.rb
index 93f8b36f..8a71af47 100644
--- a/spec/unit/plugins/network_spec.rb
+++ b/spec/unit/plugins/network_spec.rb
@@ -388,10 +388,10 @@ describe Ohai::System, "Network Plugin" do
             expect(@plugin["ip6address"]).to eq("3ffe:1111:3333::1")
           end
 
-          it "doesn't set macaddress, ipv4 setup is valid and has precedence over ipv6" do
+          it "sets mac address to mac of eth1, skipping eth0 due to NOARP" do
             expect(Ohai::Log).not_to receive(:warn).with(/^unable to detect macaddress/)
             @plugin.run
-            expect(@plugin["macaddress"]).to be_nil
+            expect(@plugin["macaddress"]).to eq("00:16:3E:2F:36:80")
           end
 
           it "informs about this setup" do
@@ -827,13 +827,7 @@ describe Ohai::System, "Network Plugin" do
         it "can't detect ipaddress" do
           allow(Ohai::Log).to receive(:warn)
           @plugin.run
-          expect(@plugin["ipaddress"]).to be_nil
-        end
-
-        it "warns about not being able to set {ip,mac}address (ipv4)" do
-          expect(Ohai::Log).to receive(:warn).with(/^unable to detect ipaddress/).once
-          expect(Ohai::Log).to receive(:warn).with(/^unable to detect macaddress/).once
-          @plugin.run
+          expect(@plugin["ipaddress"]).to eq("127.0.0.1")
         end
 
         it "sets {ip6,mac}address" do