Merge pull request #7007 from chef/tm/backport_6851

Backport mount provider fixes to 13

8 files changed, 89 insertions, 193 deletions

diff --git a/lib/chef/provider/mount.rb b/lib/chef/provider/mount.rb
index 4d0d273ea9..533dd2514b 100644
--- a/lib/chef/provider/mount.rb
+++ b/lib/chef/provider/mount.rb
@@ -83,7 +83,7 @@ class Chef
       end
 
       def action_enable
-        unless current_resource.enabled && mount_options_unchanged?
+        unless current_resource.enabled && mount_options_unchanged? && device_unchanged?
           converge_by("enable #{current_resource.device}") do
             enable_fs
             Chef::Log.info("#{new_resource} enabled")
@@ -120,6 +120,14 @@ class Chef
         raise Chef::Exceptions::UnsupportedAction, "#{self} does not implement #mount_options_unchanged?"
       end
 
+      # It's entirely plausible that a site might prefer UUIDs or labels, so
+      # we need to be able to update fstab to conform with their wishes
+      # without necessarily needing to remount the device.
+      # See #6851 for more.
+      def device_unchanged?
+        @current_resource.device == @new_resource.device
+      end
+
       #
       # NOTE: for the following methods, this superclass will already have checked if the filesystem is
       # enabled and/or mounted and they will be called in converge_by blocks, so most defensive checking
diff --git a/lib/chef/provider/mount/aix.rb b/lib/chef/provider/mount/aix.rb
index ef760b0ba6..4c2ddbe791 100644
--- a/lib/chef/provider/mount/aix.rb
+++ b/lib/chef/provider/mount/aix.rb
@@ -29,7 +29,7 @@ class Chef
           super
           # options and fstype are set to "defaults" and "auto" respectively in the Mount Resource class. These options are not valid for AIX, override them.
           if @new_resource.options[0] == "defaults"
-            @new_resource.options.clear
+            @new_resource.options([])
           end
           if @new_resource.fstype == "auto"
             @new_resource.send(:clear_fstype)
diff --git a/lib/chef/provider/mount/mount.rb b/lib/chef/provider/mount/mount.rb
index c3b1cc0141..1435a75ec3 100644
--- a/lib/chef/provider/mount/mount.rb
+++ b/lib/chef/provider/mount/mount.rb
@@ -57,12 +57,13 @@ class Chef
             case line
             when /^[#\s]/
               next
-            when /^#{device_fstab_regex}\s+#{Regexp.escape(@new_resource.mount_point)}\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)/
+            when /^(#{device_fstab_regex})\s+#{Regexp.escape(@new_resource.mount_point)}\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)/
               enabled = true
-              @current_resource.fstype($1)
-              @current_resource.options($2)
-              @current_resource.dump($3.to_i)
-              @current_resource.pass($4.to_i)
+              @current_resource.device($1)
+              @current_resource.fstype($2)
+              @current_resource.options($3)
+              @current_resource.dump($4.to_i)
+              @current_resource.pass($5.to_i)
               Chef::Log.debug("Found mount #{device_fstab} to #{@new_resource.mount_point} in /etc/fstab")
               next
             when /^[\/\w]+\s+#{Regexp.escape(@new_resource.mount_point)}\s+/
@@ -147,7 +148,7 @@ class Chef
         end
 
         def enable_fs
-          if @current_resource.enabled && mount_options_unchanged?
+          if @current_resource.enabled && mount_options_unchanged? && device_unchanged?
             Chef::Log.debug("#{@new_resource} is already enabled - nothing to do")
             return nil
           end
@@ -253,7 +254,7 @@ class Chef
           if @new_resource.device_type == :device
             device_mount_regex
           else
-            device_fstab
+            Regexp.union(device_fstab, device_mount_regex)
           end
         end
 
diff --git a/lib/chef/provider/mount/solaris.rb b/lib/chef/provider/mount/solaris.rb
index a5a7a327cb..f59a2d2c6a 100644
--- a/lib/chef/provider/mount/solaris.rb
+++ b/lib/chef/provider/mount/solaris.rb
@@ -121,8 +121,8 @@ class Chef
         end
 
         def mount_options_unchanged?
-          new_options = options_remove_noauto(options)
-          current_options = options_remove_noauto(current_resource.nil? ? nil : current_resource.options)
+          new_options = native_options(options)
+          current_options = native_options(current_resource.nil? ? nil : current_resource.options)
 
           current_resource.fsck_device == fsck_device &&
             current_resource.fstype == fstype &&
@@ -168,7 +168,8 @@ class Chef
         def read_vfstab_status
           # Check to see if there is an entry in /etc/vfstab. Last entry for a volume wins.
           enabled = false
-          fstype = options = pass = nil
+          pass = false
+          fstype = options = nil
           ::File.foreach(VFSTAB) do |line|
             case line
             when /^[#\s]/
@@ -220,11 +221,7 @@ class Chef
         end
 
         def vfstab_entry
-          actual_options = unless options.nil?
-                             tempops = options.dup
-                             tempops.delete("noauto")
-                             tempops
-                           end
+          actual_options = native_options(options)
           autostr = mount_at_boot? ? "yes" : "no"
           passstr = pass == 0 ? "-" : pass
           optstr = (actual_options.nil? || actual_options.empty?) ? "-" : actual_options.join(",")
@@ -251,11 +248,15 @@ class Chef
           contents << vfstab_entry
         end
 
-        def options_remove_noauto(temp_options)
-          new_options = []
-          new_options += temp_options.nil? ? [] : temp_options
-          new_options.delete("noauto")
-          new_options
+        def native_options(temp_options)
+          if temp_options == %w{defaults}
+            ["-"]
+          else
+            new_options = []
+            new_options += temp_options.nil? ? [] : temp_options.dup
+            new_options.delete("noauto")
+            new_options
+          end
         end
 
         def device_regex
diff --git a/lib/chef/resource/mount.rb b/lib/chef/resource/mount.rb
index 1a1f8c0565..9d8a4089cf 100644
--- a/lib/chef/resource/mount.rb
+++ b/lib/chef/resource/mount.rb
@@ -24,143 +24,37 @@ class Chef
     # Use the mount resource to manage a mounted file system.
     class Mount < Chef::Resource
 
-      identity_attr :device
-
-      state_attrs :mount_point, :device_type, :fstype, :username, :password, :domain
-
       default_action :mount
       allowed_actions :mount, :umount, :unmount, :remount, :enable, :disable
 
       # this is a poor API please do not re-use this pattern
-      property :supports, Hash, default: lazy { { remount: false } },
-                                coerce: proc { |x| x.is_a?(Array) ? x.each_with_object({}) { |i, m| m[i] = true } : x }
+      property :supports, Hash,
+        default: lazy { { remount: false } },
+        coerce: proc { |x| x.is_a?(Array) ? x.each_with_object({}) { |i, m| m[i] = true } : x }
 
       property :password, String, sensitive: true
 
-      def initialize(name, run_context = nil)
-        super
-        @mount_point = name
-        @device = nil
-        @device_type = :device
-        @fsck_device = "-"
-        @fstype = "auto"
-        @options = ["defaults"]
-        @dump = 0
-        @pass = 2
-        @mounted = false
-        @enabled = false
-        @username = nil
-        @password = nil
-        @domain = nil
-      end
-
-      def mount_point(arg = nil)
-        set_or_return(
-          :mount_point,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def device(arg = nil)
-        set_or_return(
-          :device,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def device_type(arg = nil)
-        real_arg = arg.kind_of?(String) ? arg.to_sym : arg
-        valid_devices = if RUBY_PLATFORM =~ /solaris/i
-                          [ :device ]
-                        else
-                          [ :device, :label, :uuid ]
-                        end
-        set_or_return(
-          :device_type,
-          real_arg,
-          :equal_to => valid_devices
-        )
-      end
-
-      def fsck_device(arg = nil)
-        set_or_return(
-          :fsck_device,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def fstype(arg = nil)
-        set_or_return(
-          :fstype,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
-
-      def options(arg = nil)
-        ret = set_or_return(
-                            :options,
-                            arg,
-                            :kind_of => [ Array, String ]
-                            )
-
-        if ret.is_a? String
-          ret.tr(",", " ").split(/ /)
-        else
-          ret
-        end
-      end
-
-      def dump(arg = nil)
-        set_or_return(
-          :dump,
-          arg,
-          :kind_of => [ Integer, FalseClass ]
-        )
-      end
+      property :mount_point, String, name_property: true
+      property :device, String, identity: true
 
-      def pass(arg = nil)
-        set_or_return(
-          :pass,
-          arg,
-          :kind_of => [ Integer, FalseClass ]
-        )
-      end
-
-      def mounted(arg = nil)
-        set_or_return(
-          :mounted,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      property :device_type, [String, Symbol],
+        coerce: proc { |arg| arg.kind_of?(String) ? arg.to_sym : arg },
+        default: :device,
+        equal_to: RUBY_PLATFORM =~ /solaris/i ? %i{ device } : %i{ device label uuid }
 
-      def enabled(arg = nil)
-        set_or_return(
-          :enabled,
-          arg,
-          :kind_of => [ TrueClass, FalseClass ]
-        )
-      end
+      property :fsck_device, String, default: "-"
+      property :fstype, [String, nil], default: "auto"
 
-      def username(arg = nil)
-        set_or_return(
-          :username,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      property :options, [Array, String, nil],
+        coerce: proc { |arg| arg.kind_of?(String) ? arg.split(",") : arg },
+        default: %w{defaults}
 
-      def domain(arg = nil)
-        set_or_return(
-          :domain,
-          arg,
-          :kind_of => [ String ]
-        )
-      end
+      property :dump, [Integer, FalseClass], default: 0
+      property :pass, [Integer, FalseClass], default: 2
+      property :mounted, [TrueClass, FalseClass], default: false
+      property :enabled, [TrueClass, FalseClass], default: false
+      property :username, String
+      property :domain, String
 
       private
 
diff --git a/spec/unit/provider/mount/mount_spec.rb b/spec/unit/provider/mount/mount_spec.rb
index 20a4fd88dd..ca6cf59d71 100644
--- a/spec/unit/provider/mount/mount_spec.rb
+++ b/spec/unit/provider/mount/mount_spec.rb
@@ -53,13 +53,13 @@ describe Chef::Provider::Mount::Mount do
       expect(@provider.current_resource.device).to eq("/dev/sdz1")
     end
 
-    it "should accecpt device_type :uuid", :not_supported_on_solaris do
+    it "should accept device_type :uuid", :not_supported_on_solaris do
       @status = double(:stdout => "/dev/sdz1\n", :exitstatus => 1)
       @new_resource.device_type :uuid
       @new_resource.device "d21afe51-a0fe-4dc6-9152-ac733763ae0a"
       @stdout_findfs = double("STDOUT", :first => "/dev/sdz1")
       expect(@provider).to receive(:shell_out).with("/sbin/findfs UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a").and_return(@status)
-      @provider.load_current_resource()
+      @provider.load_current_resource
       @provider.mountable?
     end
 
@@ -470,5 +470,34 @@ describe Chef::Provider::Mount::Mount do
         @provider.disable_fs
       end
     end
+
+    # the fstab might contain the mount with the device as a device but the resource has a label.
+    # we should not create two mount lines, but update the existing one
+    context "when the device is described differently" do
+      it "should update the existing line" do
+        @current_resource.enabled(true)
+        status = double(:stdout => "/dev/sdz1\n", :exitstatus => 1)
+        expect(@provider).to receive(:shell_out).with("/sbin/findfs UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a").and_return(status)
+
+        filesystems = [%q{/dev/sdy1 /tmp/foo  ext3  defaults  1 2},
+                      %q{/dev/sdz1 /tmp/foo  ext3  defaults  1 2}].join("\n")
+        fstab = StringIO.new filesystems
+
+        fstab_write = StringIO.new
+
+        allow(::File).to receive(:readlines).with("/etc/fstab").and_return(fstab.readlines)
+        allow(::File).to receive(:open).with("/etc/fstab", "w").and_yield(fstab_write)
+        allow(::File).to receive(:open).with("/etc/fstab", "a").and_yield(fstab_write)
+
+        @new_resource.device_type :uuid
+        @new_resource.device "d21afe51-a0fe-4dc6-9152-ac733763ae0a"
+        @new_resource.dump 1
+
+        @provider.enable_fs
+        expect(fstab_write.string).to match(%r{/dev/sdy1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
+        expect(fstab_write.string).to match(%r{UUID=d21afe51-a0fe-4dc6-9152-ac733763ae0a\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
+        expect(fstab_write.string).not_to match(%r{/dev/sdz1\s+/tmp/foo\s+ext3\s+defaults\s+1\s+2})
+      end
+    end
   end
 end
diff --git a/spec/unit/provider/mount/solaris_spec.rb b/spec/unit/provider/mount/solaris_spec.rb
index 264c8b9b36..2ec9feaf3b 100644
--- a/spec/unit/provider/mount/solaris_spec.rb
+++ b/spec/unit/provider/mount/solaris_spec.rb
@@ -538,7 +538,7 @@ describe Chef::Provider::Mount::Solaris, :unix_only do
   context "after the mount's state has been discovered" do
     describe "mount_fs" do
       it "should mount the filesystem" do
-        expect(provider).to receive(:shell_out!).with("mount -F #{fstype} -o defaults #{device} #{mountpoint}")
+        expect(provider).to receive(:shell_out!).with("mount -F #{fstype} #{device} #{mountpoint}")
         provider.mount_fs()
       end
 
@@ -600,7 +600,7 @@ describe Chef::Provider::Mount::Solaris, :unix_only do
       context "in the typical case" do
         let(:other_mount) { "/dev/dsk/c0t2d0s0       /dev/rdsk/c0t2d0s0      /            ufs     2       yes     -" }
 
-        let(:this_mount) { "/dev/dsk/c0t2d0s7\t/dev/rdsk/c0t2d0s7\t/mnt/foo\tufs\t2\tyes\tdefaults\n" }
+        let(:this_mount) { "/dev/dsk/c0t2d0s7\t/dev/rdsk/c0t2d0s7\t/mnt/foo\tufs\t2\tyes\t-\n" }
 
         let(:vfstab_file_contents) { [other_mount].join("\n") }
 
diff --git a/spec/unit/resource/mount_spec.rb b/spec/unit/resource/mount_spec.rb
index da181b7f8c..29a6bdf1f3 100644
--- a/spec/unit/resource/mount_spec.rb
+++ b/spec/unit/resource/mount_spec.rb
@@ -66,6 +66,10 @@ describe Chef::Resource::Mount do
     expect(resource.fstype).to eql("nfs")
   end
 
+  it "sets fstype to 'auto' by default" do
+    expect(resource.fstype).to eql("auto")
+  end
+
   it "allows you to set the dump attribute" do
     resource.dump 1
     expect(resource.dump).to eql(1)
@@ -169,45 +173,4 @@ describe Chef::Resource::Mount do
     resource.domain("TEST_DOMAIN")
     expect(resource.domain).to eq("TEST_DOMAIN")
   end
-
-  describe "when it has mount point, device type, and fstype" do
-    before do
-      resource.device("charmander")
-      resource.mount_point("123.456")
-      resource.device_type(:device)
-      resource.fstype("ranked")
-    end
-
-    it "describes its state" do
-      state = resource.state_for_resource_reporter
-      expect(state[:mount_point]).to eq("123.456")
-      expect(state[:device_type]).to eql(:device)
-      expect(state[:fstype]).to eq("ranked")
-    end
-
-    it "returns the device as its identity" do
-      expect(resource.identity).to eq("charmander")
-    end
-  end
-
-  describe "when it has username, password and domain" do
-    before do
-      resource.mount_point("T:")
-      resource.device("charmander")
-      resource.username("Administrator")
-      resource.password("Jetstream123!")
-      resource.domain("TEST_DOMAIN")
-    end
-
-    it "describes its state" do
-      state = resource.state_for_resource_reporter
-      expect(state[:mount_point]).to eq("T:")
-      expect(state[:username]).to eq("Administrator")
-      expect(state[:password]).to eq("*sensitive value suppressed*")
-      expect(state[:domain]).to eq("TEST_DOMAIN")
-      expect(state[:device_type]).to eql(:device)
-      expect(state[:fstype]).to eq("auto")
-    end
-
-  end
 end