Comments and minor cleanup

Expand network interface mock data for readability && vs. and opscode -> Chef inc. Remove extra spaces in specs Improve debug output Add comments to methods
chef · Dec 22, 2015 · badfc19 · badfc19
1 parent b8d2b39
commit badfc19
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 48 deletions.
diff --git a/lib/ohai/mixin/network_constants.rb b/lib/ohai/mixin/network_constants.rb
@@ -1,6 +1,6 @@
 #
-# Author:: Serdar Sutay (<serdar@opscode.com>)
-# Copyright:: Copyright (c) 2014 Opscode, Inc.
+# Author:: Serdar Sutay (<serdar@chef.io>)
+# Copyright:: Copyright (c) 2014-2015 Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");

diff --git a/lib/ohai/plugins/linux/network.rb b/lib/ohai/plugins/linux/network.rb
@@ -138,6 +138,7 @@ def parse_routes(family, iface)
     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 @@ def ethernet_layer_one(iface)
     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 @@ def parse_ip_addr(iface)
     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 @@ def parse_ip_addr_inet_line(cint, iface, line)
         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 @@ def parse_ip_addr_inet6_line(cint, iface, line)
         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]}")
@@ -416,7 +417,6 @@ def parse_ip_addr_inet6_line(cint, iface, line)
           end
         end
       end
-
     else
 
       begin

diff --git a/lib/ohai/plugins/network.rb b/lib/ohai/plugins/network.rb
@@ -1,14 +1,14 @@
 #
-# Author:: Adam Jacob (<adam@opscode.com>)
-# Copyright:: Copyright (c) 2008 Opscode, Inc.
+# Author:: Adam Jacob (<adam@chef.io>)
+# Copyright:: Copyright (c) 2008-2015 Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
-# 
+#
 #     http://www.apache.org/licenses/LICENSE-2.0
-# 
+#
 # Unless required by applicable law or agreed to in writing, software
 # distributed under the License is distributed on an "AS IS" BASIS,
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -26,19 +26,21 @@
 
   depends "network/interfaces"
 
+  # from interface data create array of hashes with ipaddress, scope, and iface
+  # sorted by scope, prefixlen and then ipaddress where longest prefixes first
   def sorted_ips(family = "inet")
-    raise "bad family #{family}" unless [ "inet", "inet6" ].include? family
+    fail "bad family #{family}" unless %w(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'
       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
@@ -56,10 +58,13 @@ def sorted_ips(family = "inet")
     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
@@ -109,27 +114,28 @@ def find_ip(family = "inet")
     [ 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 = network["interfaces"][iface]["addresses"].select{|k,v| v["family"] == "lladdr"}
     r.nil? or r.first.nil? ? nil : r.first.first
   end
 
+  # address_to_match: String
+  # ipaddress: IPAddress
+  # iface: String
   def network_contains_address(address_to_match, ipaddress, iface)
-    # address_to_match: String
-    # ipaddress: IPAddress
-    # iface: String
     if peer = network["interfaces"][iface]["addresses"][ipaddress.to_s][:peer]
       IPAddress(peer) == IPAddress(address_to_match)
     else
       ipaddress.include? IPAddress(address_to_match)
     end
   end
 
-  # ipaddress, ip6address and macaddress can be set by the #{os}::network plugin
-  # atm it is expected macaddress is set at the same time than 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. IPV4 currently has
+  # precedence over ipv6 interfaces.
   collect_data do
     results = {}
 
@@ -138,10 +144,11 @@ def network_contains_address(address_to_match, ipaddress, iface)
     counters Mash.new unless counters
     counters[:network] = Mash.new unless counters[:network]
 
-    # inet family is treated before inet6
+    # inet family is processed before inet6 to give ipv4 precedence
     Ohai::Mixin::NetworkConstants::FAMILIES.keys.sort.each do |family|
       r = {}
-      ( r["ip"], r["iface"] ) = find_ip(family)
+      # 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
       if family == "inet" and ipaddress.nil?
@@ -159,6 +166,8 @@ def network_contains_address(address_to_match, ipaddress, iface)
           Ohai::Log.debug("unable to detect ip6address")
         else
           ip6address r["ip"]
+          # don't overwrite macaddress set by "#{os}::network" plugin
+          # and also overwrite mac address from ipv4 loopback interface
           if r["mac"] and macaddress.nil? and ipaddress.nil?
             Ohai::Log.debug("macaddress set to #{r["mac"]} from the ipv6 setup")
             macaddress r["mac"]

diff --git a/spec/unit/plugins/linux/network_spec.rb b/spec/unit/plugins/linux/network_spec.rb
@@ -1,7 +1,7 @@
 #
 #  Author:: Caleb Tennis <[email protected]>
 #  Author:: Chris Read <[email protected]>
-#  Copyright:: Copyright (c) 2011 Opscode, Inc.
+#  Copyright:: Copyright (c) 2011-2015 Chef Software, Inc.
 #  License:: Apache License, Version 2.0
 #
 #  Licensed under the Apache License, Version 2.0 (the "License");
@@ -663,7 +663,7 @@
         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
@@ -798,7 +798,7 @@
         end
       end
 
-      describe "when there's a source field in a local route entry " do
+      describe "when there's a source field in a local route entry" do
         let(:linux_ip_route) {
 '10.116.201.0/24 dev eth0  proto kernel  src 10.116.201.76
 192.168.5.0/24 dev eth0  proto kernel  src 192.168.5.1

diff --git a/spec/unit/plugins/network_spec.rb b/spec/unit/plugins/network_spec.rb
@@ -26,8 +26,8 @@ def it_doesnt_fail
   end
 end
 
+# basic sanity check that is called in all describes below
 def it_populates_ipaddress_attributes
-
   source = caller[0]
 
   it "populates ipaddress, macaddress and ip6address" do
@@ -43,11 +43,11 @@ def it_populates_ipaddress_attributes
       raise
     end
   end
-
 end
 
 describe Ohai::System, "Network Plugin" do
 
+  # output of network plugins on particular platforms to mock plugin runs
   basic_data = {
     "freebsd" => {
       "network" => {
@@ -57,7 +57,9 @@ def it_populates_ipaddress_attributes
             "number" => "0",
             "flags" => ["UP", "BROADCAST", "RUNNING", "SIMPLEX", "MULTICAST"],
             "addresses" => {
-              "00:00:24:c9:5e:b8" => {"family" => "lladdr"},
+              "00:00:24:c9:5e:b8" => {
+                "family" => "lladdr"
+              },
               "fe80::200:24ff:fec9:5eb8" => {
                 "family" => "inet6",
                 "zoneid" => "vr0",
@@ -80,7 +82,9 @@ def it_populates_ipaddress_attributes
             "number" => "1",
             "flags" => ["UP", "BROADCAST", "RUNNING", "PROMISC", "SIMPLEX", "MULTICAST"],
             "addresses" => {
-              "00:00:24:c9:5e:b9" => {"family" => "lladdr"},
+              "00:00:24:c9:5e:b9" => {
+                "family" => "lladdr"
+              },
               "fe80::200:24ff:fec9:5eb9" => {
                 "family" => "inet6",
                 "zoneid" => "vr1",
@@ -94,7 +98,9 @@ def it_populates_ipaddress_attributes
             "number" => "2",
             "flags" => ["UP", "BROADCAST", "RUNNING", "PROMISC", "SIMPLEX", "MULTICAST"],
             "addresses" => {
-              "00:00:24:c9:5e:ba" => {"family" => "lladdr"},
+              "00:00:24:c9:5e:ba" => {
+                "family" => "lladdr"
+              },
               "fe80::200:24ff:fec9:5eba" => {
                 "family" => "inet6",
                 "zoneid" => "vr2",
@@ -108,7 +114,9 @@ def it_populates_ipaddress_attributes
             "number" => "3",
             "flags" => ["UP", "BROADCAST", "RUNNING", "PROMISC", "SIMPLEX", "MULTICAST"],
             "addresses" => {
-              "00:00:24:c9:5e:bb" => {"family" => "lladdr"},
+              "00:00:24:c9:5e:bb" => {
+                "family" => "lladdr"
+              },
               "fe80::200:24ff:fec9:5ebb" => {
                 "family" => "inet6",
                 "zoneid" => "vr3",
@@ -128,8 +136,14 @@ def it_populates_ipaddress_attributes
             "number" => "0",
             "flags" => ["UP", "LOOPBACK", "RUNNING", "MULTICAST"],
             "addresses" => {
-              "127.0.0.1" => {"family" => "inet", "netmask" => "255.0.0.0"},
-              "::1" => {"family" => "inet6", "prefixlen" => "128"},
+              "127.0.0.1" => {
+                "family" => "inet",
+                "netmask" => "255.0.0.0"
+              },
+              "::1" => {
+                "family" => "inet6",
+                "prefixlen" => "128"
+              },
               "fe80::1" => {
                 "family" => "inet6",
                 "zoneid" => "lo0",
@@ -143,13 +157,18 @@ def it_populates_ipaddress_attributes
             "number" => "0",
             "flags" => ["LEARNING", "DISCOVER", "AUTOEDGE", "AUTOPTP"],
             "addresses" => {
-              "02:20:6f:d2:c4:00" => {"family"=>"lladdr"},
+              "02:20:6f:d2:c4:00" => {
+                "family"=>"lladdr"
+              },
               "192.168.2.1" => {
                 "family" => "inet",
                 "netmask" => "255.255.255.0",
                 "broadcast" => "192.168.2.255"
               },
-              "2001:470:d:cb4::1" => {"family" => "inet6", "prefixlen" => "64"},
+              "2001:470:d:cb4::1" => {
+                "family" => "inet6",
+                "prefixlen" => "64"
+              },
               "fe80::cafe:babe:dead:beef" => {
                 "family" => "inet6",
                 "zoneid" => "bridge0",
@@ -188,7 +207,7 @@ def it_populates_ipaddress_attributes
     },
     "linux" => {
       "network" => {
-        # pp Hash[node['network']] from  shef to get the network data
+        # pp Hash[node['network']] from chef-shell to get the network data
         # have just removed the neighbour and route entries by hand
         "interfaces" => {
           "lo" => {
@@ -207,7 +226,8 @@ def it_populates_ipaddress_attributes
               }
             },
             "mtu" => "16436",
-            "encapsulation" => "Loopback"
+            "encapsulation" => "Loopback",
+            "state" => "unknown"
           },
           "eth0" => {
             "flags" => ["BROADCAST", "MULTICAST", "UP"],
@@ -218,7 +238,9 @@ def it_populates_ipaddress_attributes
                 "prefixlen" => "64",
                 "family" => "inet6"
               },
-              "00:16:3E:2F:36:79" => {"family" => "lladdr"},
+              "00:16:3E:2F:36:79" => {
+                "family" => "lladdr"
+              },
               "192.168.66.33" => {
                 "scope" => "Global",
                 "netmask" => "255.255.255.0",
@@ -229,7 +251,8 @@ def it_populates_ipaddress_attributes
               "3ffe:1111:2222::33" => {
                 "prefixlen" => "48",
                 "family" => "inet6",
-                "scope" => "Global"
+                "scope" => "Global",
+                "state" => "up"
               }
             },
             "mtu" => "1500",
@@ -245,7 +268,9 @@ def it_populates_ipaddress_attributes
                 "prefixlen" => "64",
                 "family" => "inet6"
               },
-              "00:16:3E:2F:36:80" => {"family" => "lladdr"},
+              "00:16:3E:2F:36:80" => {
+                "family" => "lladdr"
+              },
               "192.168.99.11" => {
                 "scope" => "Global",
                 "netmask" => "255.255.255.0",
@@ -301,14 +326,14 @@ def it_populates_ipaddress_attributes
     }
   }
 
-  describe "with linux" do
+  describe "on linux" do
     before(:each) do
       @plugin = get_plugin("network")
       @plugin["network"] = basic_data["linux"]["network"]
     end
 
     describe "when the linux::network plugin hasn't set any of {ip,ip6,mac}address attributes" do
-      describe "simple setup" do
+      describe "simple network setup" do
         it_populates_ipaddress_attributes
 
         it "detects {ip,ip6,mac}address" do
@@ -393,7 +418,7 @@ def it_populates_ipaddress_attributes
             expect(@plugin["macaddress"]).to eq("00:16:3E:2F:36:80")
             expect(@plugin["ip6address"]).to eq("3ffe:1111:3333::1")
           end
-          
+
           it "warns about this conflict" do
             expect(Ohai::Log).to receive(:debug).with(/^\[inet\] no ipaddress\/mask on eth1/).once
             expect(Ohai::Log).to receive(:debug).with(/^\[inet6\] no ipaddress\/mask on eth1/).once
@@ -789,6 +814,7 @@ def it_populates_ipaddress_attributes
 
     end
 
+    # specs using network plugin data for each mocked OS (freebsd,linux,windows)
     basic_data.keys.sort.each do |os|
       describe "the #{os}::network has already set some of the {ip,mac,ip6}address attributes" do
         before(:each) do