Merge pull request #10614 from MsysTechnologiesllc/antima/cifs_mount_not_idempotent_fixes

mount: Fixes for findmount output causing idempotency issues

6 files changed, 29 insertions, 7 deletions

diff --git a/lib/chef/provider/mount.rb b/lib/chef/provider/mount.rb
index 64c7bc3f29..f8c71c6706 100644
--- a/lib/chef/provider/mount.rb
+++ b/lib/chef/provider/mount.rb
@@ -175,13 +175,15 @@ class Chef
 
       # Returns the new_resource device as per device_type
       def device_fstab
+        # Removed "/" from the end of str, because it was causing idempotency issue.
+        device = @new_resource.device.chomp("/")
         case @new_resource.device_type
         when :device
-          @new_resource.device
+          device
         when :label
-          "LABEL=#{@new_resource.device}"
+          "LABEL=#{device}"
         when :uuid
-          "UUID=#{@new_resource.device}"
+          "UUID=#{device}"
         end
       end
     end
diff --git a/lib/chef/provider/mount/linux.rb b/lib/chef/provider/mount/linux.rb
index 3199024f1b..382e37d41a 100644
--- a/lib/chef/provider/mount/linux.rb
+++ b/lib/chef/provider/mount/linux.rb
@@ -53,6 +53,10 @@ class Chef
             when %r{\A#{Regexp.escape(real_mount_point)}\s+([/\w])+\[#{device_mount_regex}\]\s}
               mounted = true
               logger.trace("Bind device #{device_logstring} mounted as #{real_mount_point}")
+            # Permalink for network device mounted to an existing mount point: https://rubular.com/r/JRTXXGFdQtwCD6
+            when /\A#{Regexp.escape(real_mount_point)}\s+#{device_mount_regex}\[/
+              mounted = true
+              logger.trace("Network device #{device_logstring} mounted as #{real_mount_point}")
             end
           end
           @current_resource.mounted(mounted)
diff --git a/lib/chef/provider/mount/mount.rb b/lib/chef/provider/mount/mount.rb
index f6a7519c06..e065ce09e5 100644
--- a/lib/chef/provider/mount/mount.rb
+++ b/lib/chef/provider/mount/mount.rb
@@ -223,7 +223,8 @@ class Chef
               @real_device = device_line.chomp unless device_line.nil?
             end
           end
-          @real_device
+          # Removed "/" from the end of str, because it was causing idempotency issue.
+          @real_device.chomp("/")
         end
 
         def device_logstring
diff --git a/lib/chef/resource/mount.rb b/lib/chef/resource/mount.rb
index ed5d9a0e0b..c23ba9bbee 100644
--- a/lib/chef/resource/mount.rb
+++ b/lib/chef/resource/mount.rb
@@ -41,6 +41,7 @@ class Chef
         sensitive: true
 
       property :mount_point, String, name_property: true,
+               coerce: proc { |arg| arg.chomp("/") }, # Removed "/" from the end of str, because it was causing idempotency issue.
                description: "The directory (or path) in which the device is to be mounted. Defaults to the name of the resource block if not provided."
 
       property :device, String, identity: true,
@@ -65,7 +66,7 @@ class Chef
 
       property :options, [Array, String, nil],
         description: "An array or comma separated list of options for the mount.",
-        coerce: proc { |arg| arg.is_a?(String) ? arg.split(",") : arg },
+        coerce: proc { |arg| mount_options(arg) }, # Please see #mount_options method.
         default: %w{defaults}
 
       property :dump, [Integer, FalseClass],
@@ -94,6 +95,11 @@ class Chef
         @fstype = nil
       end
 
+      # Returns array of string without leading and trailing whitespace.
+      def mount_options(options)
+        (options.is_a?(String) ? options.split(",") : options).collect(&:strip)
+      end
+
     end
   end
 end
diff --git a/spec/unit/provider/mount/linux_spec.rb b/spec/unit/provider/mount/linux_spec.rb
index 1141175780..3e41f895d1 100644
--- a/spec/unit/provider/mount/linux_spec.rb
+++ b/spec/unit/provider/mount/linux_spec.rb
@@ -24,6 +24,7 @@ describe Chef::Provider::Mount::Linux do
   before(:each) do
     allow(::File).to receive(:exists?).with("/dev/sdz1").and_return true
     allow(::File).to receive(:exists?).with("/tmp/foo").and_return true
+    allow(::File).to receive(:exists?).with("//192.168.11.102/Share/backup").and_return true
     allow(::File).to receive(:realpath).with("/dev/sdz1").and_return "/dev/sdz1"
     allow(::File).to receive(:realpath).with("/tmp/foo").and_return "/tmp/foo"
   end
@@ -92,6 +93,15 @@ describe Chef::Provider::Mount::Linux do
       expect(provider.current_resource.mounted).to be_falsey
     end
 
+    it "should set mounted true if network_device? is true and the mount point is found in the mounts list" do
+      new_resource.device "//192.168.11.102/Share/backup"
+      new_resource.fstype "cifs"
+      mount = "/tmp/foo //192.168.11.102/Share/backup[/backup] cifs rw\n"
+      mount << "#{new_resource.mount_point} #{new_resource.device} type #{new_resource.fstype}\n"
+      allow(provider).to receive(:shell_out!).and_return(double(stdout: mount))
+      provider.load_current_resource
+      expect(provider.current_resource.mounted).to be_truthy
+    end
   end
 
 end
diff --git a/spec/unit/resource/mount_spec.rb b/spec/unit/resource/mount_spec.rb
index b58777ccca..933c4f1947 100644
--- a/spec/unit/resource/mount_spec.rb
+++ b/spec/unit/resource/mount_spec.rb
@@ -50,8 +50,7 @@ describe Chef::Resource::Mount do
   end
 
   it "raises error when mount_point property is not set" do
-    resource.mount_point nil
-    expect { resource.mounted("poop") }.to raise_error(ArgumentError)
+    expect { resource.mount_point nil }.to raise_error(Chef::Exceptions::ValidationFailed, "Property mount_point must be one of: String!  You passed nil.")
   end
 
   it "sets fsck_device to '-' by default" do