Add getnameinfo as a fall through case for fqdn resolution

lib/ohai/mixin/network_helper.rb

@@ -19,6 +19,7 @@
 #
 
 require "socket" unless defined?(Socket)
+require "resolv" unless defined?(Resolv)
 
 module Ohai
   module Mixin
@@ -37,6 +38,12 @@ def hex_to_dec_netmask(netmask)
         dec
       end
 
+      # Addrinfo#ip*? methods return true on AI_CANONNAME Addrinfo records that match
+      # the ipv* scheme and #ip? always returns true unless a non tcp Addrinfo
+      def ip?(hostname)
+        Resolv::IPv4::Regex.match?(hostname) || Resolv::IPv6::Regex.match?(hostname)
+      end
+
       # This does a forward and reverse lookup on the hostname to return what should be
       # the FQDN for the host determined by name lookup (generally DNS).  If the forward
       # lookup fails this will throw.  If the reverse lookup fails this will return the
@@ -47,7 +54,27 @@ def hex_to_dec_netmask(netmask)
       # server), and the method should return the hostname and not the IP address.
       #
       def canonicalize_hostname(hostname)
-        Addrinfo.getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME).first.canonname
+        ai = Addrinfo
+          .getaddrinfo(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME)
+          .first
+
+        canonname = ai&.canonname
+        # use canonname if it's an FQDN
+        # This API is preferred as it never gives us an IP address for broken DNS
+        # (see https://github.com/chef/ohai/pull/1705)
+        # However, we have found that Windows hosts that are not joined to a domain
+        # can return a non-qualified hostname).
+        # Use a '.' in the canonname as indicator of FQDN
+        return canonname if canonname.include?(".")
+
+        # If we got a non-qualified name, then we do a standard reverse resolve
+        # which, assuming DNS is working, will work around that windows bug
+        # (and maybe others)
+        canonname = ai&.getnameinfo&.first
+        return canonname unless ip?(canonname)
+
+        # if all else fails, return the name we were given as a safety
+        hostname
       end
 
       def canonicalize_hostname_with_retries(hostname)

spec/unit/mixin/network_helper_spec.rb

@@ -29,14 +29,53 @@
   end
 
   describe "canonicalize hostname" do
-    # this is a brittle test deliberately intended to discourage modifying this API
-    # (see the node in the code on the necessity of manual testing)
-    it "uses the correct ruby API" do
-      hostname = "broken.example.org"
-      addrinfo = instance_double(Addrinfo)
-      expect(Addrinfo).to receive(:getaddrinfo).with(hostname, nil, nil, nil, nil, Socket::AI_CANONNAME).and_return([addrinfo])
-      expect(addrinfo).to receive(:canonname).and_return(hostname)
-      expect(mixin.canonicalize_hostname(hostname)).to eql(hostname)
+    [
+      {
+        desc: "canonname == initial hostname returns those",
+        initial_hostname: "fullhostname.example.com",
+        canonname: "fullhostname.example.com",
+        final_hostname: "fullhostname.example.com",
+      },
+      {
+        desc: "canonname(hostname) => fqdn returns fqdn",
+        initial_hostname: "hostnamepart",
+        canonname: "hostnamepart.example.com",
+        final_hostname: "hostnamepart.example.com",
+      },
+      {
+        desc: "hostname only canonname, getnameinfo is tried and succeeds",
+        initial_hostname: "hostnamepart2",
+        canonname: "hostnamepart2",
+        nameinfo: "hostnamepart2.example.com",
+        final_hostname: "hostnamepart2.example.com",
+      },
+      {
+        desc: "hostname only canonname, getnameinfo returns ip => original hostname",
+        initial_hostname: "hostnameip.example.com",
+        canonname: "hostnameip", # totally contrived
+        nameinfo: "192.168.1.1",
+        final_hostname: "hostnameip.example.com",
+      },
+    ].each do |example_hash|
+      # this is a brittle set of tests deliberately intended to discourage modifying
+      # this API (see the note in the code on the necessity of manual testing)
+      example example_hash[:desc] do
+        addrinfo = instance_double(Addrinfo)
+        expect(Addrinfo).to receive(:getaddrinfo)
+          .with(example_hash[:initial_hostname], nil, nil, nil, nil, Socket::AI_CANONNAME)
+          .and_return([addrinfo])
+        expect(addrinfo).to receive(:canonname).and_return(example_hash[:canonname])
+
+        # only expect this call if :nameinfo key is set, otherwise code should not
+        # fall through to getnameinfo
+        if example_hash[:nameinfo]
+          expect(addrinfo).to receive(:getnameinfo).and_return([example_hash[:nameinfo], "0"])
+        end
+
+        # the actual input and output for #canonicalize_hostname method
+        expect(mixin.canonicalize_hostname(example_hash[:initial_hostname]))
+          .to eql(example_hash[:final_hostname])
+      end
     end
   end
 end