Revise /etc/hosts for correctness and clarity (#2863)

* Clean up test data, correct parse error handling
 * Use functional pipeline to avoid need for conditional clauses and clarify the intent of the comment parsing.
 * Extract magic strings to constants
 * Remove code and tests now covered by FileReader

Co-authored-by: Trevor Bramble <tbramble@chef.io>
Co-authored-by: Paul Welch <pwelch@chef.io>

Signed-off-by: Trevor Bramble <tbramble@chef.io>

lib/resources/etc_hosts.rb

@@ -23,53 +23,44 @@ class EtcHosts < Inspec.resource(1)
   include CommentParser
   include FileReader
 
+  DEFAULT_UNIX_PATH    = '/etc/hosts'.freeze
+  DEFAULT_WINDOWS_PATH = 'C:\windows\system32\drivers\etc\hosts'.freeze
+
   def initialize(hosts_path = nil)
-    @conf_path      = hosts_path || default_hosts_file_path
-    @content        = nil
-    @params         = nil
-    read_content
+    content = read_file_content(hosts_path || default_hosts_file_path)
+
+    @params = parse_conf(content.lines)
   end
 
-  filter = FilterTable.create
-  filter.add_accessor(:where)
-        .add_accessor(:entries)
-        .add(:ip_address,     field: 'ip_address')
-        .add(:primary_name,   field: 'primary_name')
-        .add(:all_host_names, field: 'all_host_names')
-
-  filter.connect(self, :params)
+  FilterTable.create
+             .add_accessor(:where)
+             .add_accessor(:entries)
+             .add(:ip_address,     field: 'ip_address')
+             .add(:primary_name,   field: 'primary_name')
+             .add(:all_host_names, field: 'all_host_names')
+             .connect(self, :params)
 
   private
 
   def default_hosts_file_path
-    inspec.os.windows? ? 'C:\windows\system32\drivers\etc\hosts' : '/etc/hosts'
+    inspec.os.windows? ? DEFAULT_WINDOWS_PATH : DEFAULT_UNIX_PATH
   end
 
-  def read_content
-    @content = ''
-    @params  = {}
-    @content = read_file(@conf_path)
-    @params  = parse_conf(@content)
+  def parse_conf(lines)
+    lines.reject(&:empty?).reject(&comment?).map(&parse_data).map(&format_data)
   end
 
-  def parse_conf(content)
-    content.map do |line|
-      data, = parse_comment_line(line, comment_char: '#', standalone_comments: false)
-      parse_line(data) unless data == ''
-    end.compact
+  def comment?
+    parse_options = { comment_char: '#', standalone_comments: false }
+
+    ->(data) { parse_comment_line(data, parse_options).first.empty? }
   end
 
-  def parse_line(line)
-    line_parts = line.split
-    return nil unless line_parts.length >= 2
-    {
-      'ip_address'     => line_parts[0],
-      'primary_name'   => line_parts[1],
-      'all_host_names' => line_parts[1..-1],
-    }
+  def parse_data
+    ->(data) { [data.split[0], data.split[1], data.split[1..-1]] }
   end
 
-  def read_file(conf_path = @conf_path)
-    read_file_content(conf_path).lines
+  def format_data
+    ->(data) { %w{ip_address primary_name all_host_names}.zip(data).to_h }
   end
 end

test/helper.rb

@@ -177,6 +177,7 @@ class MockLoader
       '/etc/postgresql/9.5/main' => mockfile.call('9.5.main'),
       '/var/lib/postgresql/9.5/main' => mockfile.call('var.9.5.main'),
       '/etc/hosts' => mockfile.call('hosts'),
+      '/etc/hosts_empty' => emptyfile.call,
       'C:\windows\system32\drivers\etc\hosts' => mockfile.call('hosts'),
       '/etc/fstab' => mockfile.call('fstab'),
       'fstab_no_home' => mockfile.call('fstab_no_home'),

test/unit/resources/etc_hosts_test.rb

@@ -1,25 +1,36 @@
 # encoding: utf-8
-# author: Matthew Dromazos
 
 require 'helper'
 require 'inspec/resource'
 
 describe 'Inspec::Resources::EtcHosts' do
   let(:resource) { load_resource('etc_hosts') }
+
+  let(:all_v4_hosts) do
+    %W{localhost localhost.localdomain localhost4 localhost4.localdomain4}
+  end
+
+  let(:all_v6_hosts) do
+    %W{localhost localhost.localdomain localhost6 localhost6.localdomain6}
+  end
+
   it 'Verify etc_hosts filtering by `ip_address`'  do
     entries = resource.where { ip_address == '127.0.0.1' }
     _(entries.primary_name).must_equal ['localhost']
-    _(entries.all_host_names).must_equal [['localhost', 'localhost.localdomain', 'localhost4', 'localhost4.localdomain4']]
+    _(entries.all_host_names).must_equal [all_v4_hosts]
   end
 
   it 'Verify etc_hosts filtering by `canonical_hostname`'  do
     entries = resource.where { primary_name == 'localhost' }
     _(entries.ip_address).must_equal ['127.0.0.1', '::1']
-    _(entries.all_host_names).must_equal [['localhost', 'localhost.localdomain', 'localhost4', 'localhost4.localdomain4'],  ['localhost', 'localhost.localdomain', 'localhost6', 'localhost6.localdomain6']]
+    _(entries.all_host_names).must_equal [all_v4_hosts, all_v6_hosts]
   end
 
   it 'Verify etc_hosts filtering by `all_host_names`'  do
-    entries = resource.where { all_host_names == ['localhost', 'localhost.localdomain', 'localhost4', 'localhost4.localdomain4'] }
+    # direct reference all_v4_hosts fail in filter scope
+    expected_hosts = all_v4_hosts
+
+    entries = resource.where { all_host_names == expected_hosts }
     _(entries.ip_address).must_equal ['127.0.0.1']
     _(entries.primary_name).must_equal ['localhost']
   end