Merge pull request #941 from opscode/adamed-OC-8622-aix-cron

OC-8622: Add support for the cron resource for AIX

10 files changed, 342 insertions, 62 deletions

diff --git a/lib/chef/mixin/command.rb b/lib/chef/mixin/command.rb
index 55c028ff5f..719151a6b6 100644
--- a/lib/chef/mixin/command.rb
+++ b/lib/chef/mixin/command.rb
@@ -56,8 +56,17 @@ class Chef
       # === Returns
       # Returns the exit status of args[:command]
       def run_command(args={})         
-        command_output = ""
+        status, stdout, stderr = run_command_and_return_stdout_stderr(args)
         
+        status
+      end
+
+      # works same as above, except that it returns stdout and stderr
+      # requirement => platforms like solaris 9,10 has wierd issues where
+      # even in command failure the exit code is zero, so we need to lookup stderr.
+      def run_command_and_return_stdout_stderr(args={})
+        command_output = ""
+
         args[:ignore_failure] ||= false
         args[:output_on_failure] ||= false
 
@@ -73,10 +82,10 @@ class Chef
         command_output << "STDOUT: #{stdout}"
         command_output << "STDERR: #{stderr}"
         handle_command_failures(status, command_output, args)
-        
-        status
+
+        return status, stdout, stderr
       end
-      
+
       def output_of_command(command, args)
         Chef::Log.debug("Executing #{command}")
         stderr_string, stdout_string, status = "", "", nil
diff --git a/lib/chef/platform/provider_mapping.rb b/lib/chef/platform/provider_mapping.rb
index c9706fb220..2911e32284 100644
--- a/lib/chef/platform/provider_mapping.rb
+++ b/lib/chef/platform/provider_mapping.rb
@@ -322,7 +322,8 @@ class Chef
             :default => {
               :group => Chef::Provider::Group::Aix,
               :mount => Chef::Provider::Mount::Aix,
-              :ifconfig => Chef::Provider::Ifconfig::Aix
+              :ifconfig => Chef::Provider::Ifconfig::Aix,
+              :cron => Chef::Provider::Cron::Aix
             }
           },
           :default  => {
diff --git a/lib/chef/provider/cron.rb b/lib/chef/provider/cron.rb
index a7218fea5a..4430e3e18f 100644
--- a/lib/chef/provider/cron.rb
+++ b/lib/chef/provider/cron.rb
@@ -45,6 +45,7 @@ class Chef
         crontab_lines = []
         @current_resource = Chef::Resource::Cron.new(@new_resource.name)
         @current_resource.user(@new_resource.user)
+        @cron_exists = false
         if crontab = read_crontab
           cron_found = false
           crontab.each_line do |line|
@@ -93,14 +94,7 @@ class Chef
         newcron = String.new
         cron_found = false
 
-        newcron << "# Chef Name: #{new_resource.name}\n"
-        [ :mailto, :path, :shell, :home ].each do |v|
-          newcron << "#{v.to_s.upcase}=#{@new_resource.send(v)}\n" if @new_resource.send(v)
-        end
-        @new_resource.environment.each do |name, value|
-          newcron << "#{name}=#{value}\n"
-        end
-        newcron << "#{@new_resource.minute} #{@new_resource.hour} #{@new_resource.day} #{@new_resource.month} #{@new_resource.weekday} #{@new_resource.command}\n"
+        newcron = get_crontab_entry
 
         if @cron_exists
           unless cron_different?
@@ -202,13 +196,33 @@ class Chef
       end
 
       def write_crontab(crontab)
+        write_exception = false
         status = popen4("crontab -u #{@new_resource.user} -", :waitlast => true) do |pid, stdin, stdout, stderr|
-          stdin.write crontab
+          begin
+            stdin.write crontab
+          rescue Errno::EPIPE => e
+            # popen4 could yield while child has already died.
+            write_exception = true
+            Chef::Log.debug("#{e.message}")
+          end
         end
-        if status.exitstatus > 0
+        if status.exitstatus > 0 || write_exception
           raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{status.exitstatus}"
         end
       end
+
+      def get_crontab_entry
+        newcron = ""
+        newcron << "# Chef Name: #{new_resource.name}\n"
+        [ :mailto, :path, :shell, :home ].each do |v|
+          newcron << "#{v.to_s.upcase}=#{@new_resource.send(v)}\n" if @new_resource.send(v)
+        end
+        @new_resource.environment.each do |name, value|
+          newcron << "#{name}=#{value}\n"
+        end
+        newcron << "#{@new_resource.minute} #{@new_resource.hour} #{@new_resource.day} #{@new_resource.month} #{@new_resource.weekday} #{@new_resource.command}\n"
+        newcron
+      end
     end
   end
 end
diff --git a/lib/chef/provider/cron/aix.rb b/lib/chef/provider/cron/aix.rb
new file mode 100644
index 0000000000..473700bf2f
--- /dev/null
+++ b/lib/chef/provider/cron/aix.rb
@@ -0,0 +1,48 @@
+#
+# Author:: Kaustubh Deorukhkar (<kaustubh@clogeny.com>)
+# Copyright:: Copyright (c) 2013 Opscode, 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.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require "chef/provider/cron/unix"
+
+class Chef
+  class Provider
+    class Cron
+      class Aix < Chef::Provider::Cron::Unix
+
+        private
+
+        # For AIX we ignore env vars/[ :mailto, :path, :shell, :home ]
+        def get_crontab_entry
+          if env_vars_are_set?
+            raise Chef::Exceptions::Cron, "Aix cron entry does not support environment variables. Please set them in script and use script in cron."
+          end
+
+          newcron = ""
+          newcron << "# Chef Name: #{new_resource.name}\n"
+          newcron << "#{@new_resource.minute} #{@new_resource.hour} #{@new_resource.day} #{@new_resource.month} #{@new_resource.weekday}"
+
+          newcron << " #{@new_resource.command}\n"
+          newcron
+        end
+
+        def env_vars_are_set?
+          @new_resource.environment.length > 0 || !@new_resource.mailto.nil? || !@new_resource.path.nil? || !@new_resource.shell.nil? || !@new_resource.home.nil?
+        end
+      end
+    end
+  end
+end
diff --git a/lib/chef/provider/cron/solaris.rb b/lib/chef/provider/cron/solaris.rb
index e0811ba0ac..6861820676 100644
--- a/lib/chef/provider/cron/solaris.rb
+++ b/lib/chef/provider/cron/solaris.rb
@@ -1,15 +1,13 @@
 #
-# Author:: Bryan McLellan (btm@loftninjas.org)
-# Author:: Toomas Pelberg (toomasp@gmx.net)
-# Copyright:: Copyright (c) 2009 Bryan McLellan
-# Copyright:: Copyright (c) 2010 Toomas Pelberg
+# Author:: Kaustubh Deorukhkar (<kaustubh@clogeny.com>)
+# Copyright:: Copyright (c) 2013 Opscode, 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
+# 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,
@@ -18,39 +16,7 @@
 # limitations under the License.
 #
 
-require 'chef/log'
-require 'chef/provider'
+require "chef/provider/cron/unix"
 
-class Chef
-  class Provider
-    class Cron
-      class Solaris < Chef::Provider::Cron
-
-        private
-
-        def read_crontab
-          crontab = nil
-          status = popen4("crontab -l #{@new_resource.user}") do |pid, stdin, stdout, stderr|
-            crontab = stdout.read
-          end
-          if status.exitstatus > 1
-            raise Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: #{status.exitstatus}"
-          end
-          crontab
-        end
-
-        def write_crontab(crontab)
-          tempcron = Tempfile.new("chef-cron")
-          tempcron << crontab
-          tempcron.flush
-          tempcron.chmod(0644)
-          status = run_command(:command => "/usr/bin/crontab #{tempcron.path}",:user => @new_resource.user)
-          tempcron.close!
-          if status.exitstatus > 0
-            raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{status.exitstatus}"
-          end
-        end
-      end
-    end
-  end
-end
+# Just to create an alias so 'Chef::Provider::Cron::Solaris' is exposed and accessible to existing consumers of class.
+Chef::Provider::Cron::Solaris = Chef::Provider::Cron::Unix
\ No newline at end of file
diff --git a/lib/chef/provider/cron/unix.rb b/lib/chef/provider/cron/unix.rb
new file mode 100644
index 0000000000..1149f43f76
--- /dev/null
+++ b/lib/chef/provider/cron/unix.rb
@@ -0,0 +1,76 @@
+#
+# Author:: Bryan McLellan (btm@loftninjas.org)
+# Author:: Toomas Pelberg (toomasp@gmx.net)
+# Copyright:: Copyright (c) 2009 Bryan McLellan
+# Copyright:: Copyright (c) 2010 Toomas Pelberg
+# 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.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'chef/log'
+require 'chef/provider'
+
+class Chef
+  class Provider
+    class Cron
+      class Unix < Chef::Provider::Cron
+
+        private
+
+        def read_crontab
+          crontab = nil
+          status = popen4("crontab -l #{@new_resource.user}") do |pid, stdin, stdout, stderr|
+            crontab = stdout.read
+          end
+          if status.exitstatus > 1
+            raise Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: #{status.exitstatus}"
+          end
+          crontab
+        end
+
+        def write_crontab(crontab)
+          tempcron = Tempfile.new("chef-cron")
+          tempcron << crontab
+          tempcron.flush
+          tempcron.chmod(0644)
+          exit_status = 0
+          error_message = ""
+          begin
+            status, stdout, stderr = run_command_and_return_stdout_stderr(:command => "/usr/bin/crontab #{tempcron.path}",:user => @new_resource.user)
+            exit_status = status.exitstatus
+            # solaris9, 10 on some failures for example invalid 'mins' in crontab fails with exit code of zero :(
+            if stderr && stderr.include?("errors detected in input, no crontab file generated")
+              error_message = stderr
+              exit_status = 1
+            end
+          rescue Chef::Exceptions::Exec => e
+            Chef::Log.debug(e.message)
+            exit_status = 1
+            error_message = e.message
+          rescue ArgumentError => e
+            # usually raised on invalid user.
+            Chef::Log.debug(e.message)
+            exit_status = 1
+            error_message = e.message
+          end          
+          tempcron.close!
+          if exit_status > 0
+            raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{exit_status}, message: #{error_message}"
+          end
+        end
+
+      end
+    end
+  end
+end
diff --git a/lib/chef/providers.rb b/lib/chef/providers.rb
index 65d251f687..a87aa9d141 100644
--- a/lib/chef/providers.rb
+++ b/lib/chef/providers.rb
@@ -21,6 +21,7 @@ require 'chef/provider/breakpoint'
 require 'chef/provider/cookbook_file'
 require 'chef/provider/cron'
 require 'chef/provider/cron/solaris'
+require 'chef/provider/cron/aix'
 require 'chef/provider/deploy'
 require 'chef/provider/directory'
 require 'chef/provider/env'
diff --git a/spec/functional/resource/cron_spec.rb b/spec/functional/resource/cron_spec.rb
new file mode 100644
index 0000000000..35be2bbaa6
--- /dev/null
+++ b/spec/functional/resource/cron_spec.rb
@@ -0,0 +1,147 @@
+# encoding: UTF-8
+#
+# Author:: Kaustubh Deorukhkar (<kaustubh@clogeny.com>)
+# Copyright:: Copyright (c) 2013 Opscode, 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.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'functional/resource/base'
+require 'chef/mixin/shell_out'
+
+describe Chef::Resource::Cron, :requires_root, :unix_only do
+
+  include Chef::Mixin::ShellOut
+
+  # Platform specific validation routines.
+  def cron_should_exists(cron_name, command)
+    case ohai[:platform]
+    when "aix", "solaris", "opensolaris", "solaris2", "omnios"
+      expect(shell_out("crontab -l #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(0)
+      expect(shell_out("crontab -l #{new_resource.user} | grep \"#{command}\"").exitstatus).to eq(0)
+    else
+      expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(0)
+      expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{command}\"").exitstatus).to eq(0)
+    end
+  end
+
+  def cron_should_not_exists(cron_name)
+    case ohai[:platform]
+    when "aix", "solaris", "opensolaris", "solaris2", "omnios"
+      expect(shell_out("crontab -l #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(1)
+    else
+      expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(1)
+    end
+  end
+
+  # Actual tests
+  let(:new_resource) do
+    new_resource = Chef::Resource::Cron.new("Chef functional test cron", run_context)
+    new_resource.user  'root'
+    new_resource.minute "30"
+    new_resource.command "/bin/true"
+    new_resource
+  end
+
+  let(:provider) do
+    provider = new_resource.provider_for_action(new_resource.action)
+    provider
+  end
+
+  describe "create action" do
+    after do
+      new_resource.run_action(:delete)
+    end
+
+    it "should create a crontab entry" do
+      new_resource.run_action(:create)
+      cron_should_exists(new_resource.name, new_resource.command)
+    end
+  end
+
+  describe "delete action" do
+    before do
+      new_resource.run_action(:create)
+    end
+
+    it "should delete a crontab entry" do
+      # Note that test cron is created by previous test
+      new_resource.run_action(:delete)
+      
+      cron_should_not_exists(new_resource.name)
+    end
+  end
+
+  exclude_solaris = ["solaris", "opensolaris", "solaris2", "omnios"].include?(ohai[:platform])
+  describe "create action with various attributes", :external => exclude_solaris do
+    def create_and_validate_with_attribute(resource, attribute, value)
+      if ohai[:platform] == 'aix'
+         expect {resource.run_action(:create)}.to raise_error(Chef::Exceptions::Cron, /Aix cron entry does not support environment variables. Please set them in script and use script in cron./)
+      else
+        resource.run_action(:create)
+        # Verify if the cron is created successfully
+        cron_attribute_should_exists(resource.name, attribute, value)
+      end
+    end
+
+    def cron_attribute_should_exists(cron_name, attribute, value)
+      return if ['aix', 'solaris'].include?(ohai[:platform])
+      # Test if the attribute exists on newly created cron
+      cron_should_exists(cron_name, "")
+      expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{attribute.upcase}=#{value}\"").exitstatus).to eq(0)
+    end
+
+    after do
+      new_resource.run_action(:delete)
+    end
+
+    it "should create a crontab entry for mailto attribute" do
+      new_resource.mailto "cheftest@example.com"
+      create_and_validate_with_attribute(new_resource, "mailto", "cheftest@example.com")
+    end
+
+    it "should create a crontab entry for path attribute" do
+      new_resource.path "/usr/local/bin"
+      create_and_validate_with_attribute(new_resource, "path", "/usr/local/bin")
+    end
+
+    it "should create a crontab entry for shell attribute" do
+      new_resource.shell "/bin/bash"
+      create_and_validate_with_attribute(new_resource, "shell", "/bin/bash")
+    end
+
+    it "should create a crontab entry for home attribute" do
+      new_resource.home "/home/opscode"
+      create_and_validate_with_attribute(new_resource, "home", "/home/opscode")
+    end
+  end
+
+  describe "negative tests for create action" do
+    def cron_create_should_raise_exception
+      expect { new_resource.run_action(:create) }.to raise_error(Chef::Exceptions::Cron, /Error updating state of #{new_resource.name}, exit: 1/)
+      cron_should_not_exists(new_resource.name)
+    end
+
+    it "should not create cron with invalid minute" do
+      new_resource.minute "invalid"
+      cron_create_should_raise_exception
+    end
+
+    it "should not create cron with invalid user" do
+      new_resource.user "1-really-really-invalid-user-name"
+      cron_create_should_raise_exception
+    end
+
+  end
+end
\ No newline at end of file
diff --git a/spec/unit/provider/cron/solaris_spec.rb b/spec/unit/provider/cron/unix_spec.rb
index 498d4fb1f5..ffdfa198b6 100644
--- a/spec/unit/provider/cron/solaris_spec.rb
+++ b/spec/unit/provider/cron/unix_spec.rb
@@ -20,7 +20,7 @@
 
 require 'spec_helper'
 
-describe Chef::Provider::Cron::Solaris do
+describe Chef::Provider::Cron::Unix do
   before do
     @node = Chef::Node.new
     @events = Chef::EventDispatch::Dispatcher.new
@@ -30,7 +30,7 @@ describe Chef::Provider::Cron::Solaris do
     @new_resource.minute "30"
     @new_resource.command "/bin/true"
 
-    @provider = Chef::Provider::Cron::Solaris.new(@new_resource, @run_context)
+    @provider = Chef::Provider::Cron::Unix.new(@new_resource, @run_context)
   end
 
   it "should inherit from Chef::Provider:Cron" do
@@ -86,7 +86,7 @@ CRONTAB
   describe "write_crontab" do
     before :each do
       @status = mock("Status", :exitstatus => 0)
-      @provider.stub!(:run_command).and_return(@status)
+      @provider.stub!(:run_command_and_return_stdout_stderr).and_return(@status, String.new, String.new)
       @tempfile = mock("foo", :path => "/tmp/foo", :close => true)
       Tempfile.stub!(:new).and_return(@tempfile)
       @tempfile.should_receive(:flush)
@@ -95,13 +95,13 @@ CRONTAB
     end
 
     it "should call crontab for the user" do
-      @provider.should_receive(:run_command).with(hash_including(:user => @new_resource.user))
+      @provider.should_receive(:run_command_and_return_stdout_stderr).with(hash_including(:user => @new_resource.user))
       @tempfile.should_receive(:<<).with("Foo")
       @provider.send(:write_crontab, "Foo")
     end
 
     it "should call crontab with a file containing the crontab" do
-      @provider.should_receive(:run_command) do |args|
+      @provider.should_receive(:run_command_and_return_stdout_stderr) do |args|
         (args[:command] =~ %r{\A/usr/bin/crontab (/\S+)\z}).should be_true
         $1.should == "/tmp/foo"
         @status
@@ -115,7 +115,7 @@ CRONTAB
       @status.stub!(:exitstatus).and_return(1)
       lambda do
         @provider.send(:write_crontab, "Foo")
-      end.should raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
+      end.should raise_error(Chef::Exceptions::Cron, /Error updating state of #{@new_resource.name}, exit: 1/)
     end
   end
 end
diff --git a/spec/unit/provider/cron_spec.rb b/spec/unit/provider/cron_spec.rb
index 5a848a30e6..f0d800741d 100644
--- a/spec/unit/provider/cron_spec.rb
+++ b/spec/unit/provider/cron_spec.rb
@@ -563,7 +563,7 @@ HOME=/home/foo
         @provider.stub!(:read_crontab).and_return(<<-CRONTAB)
 0 2 * * * /some/other/command
 
-# Chef Name: something else
+# Chef Name: cronhole some stuff
 * 5 * * * /bin/true
 
 # Another comment
@@ -581,6 +581,7 @@ CRONTAB
       end
 
       it "should log nothing changed" do
+        Chef::Log.should_receive(:debug).with("Found cron '#{@new_resource.name}'") 
         Chef::Log.should_receive(:debug).with("Skipping existing cron entry '#{@new_resource.name}'")
         @provider.run_action(:create)
       end
@@ -808,5 +809,22 @@ MAILTO=foo@example.com
         @provider.send(:write_crontab, "Foo")
       end.should raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
     end
+
+    it "should raise an exception if the command die's and parent tries to write" do
+      class WriteErrPipe
+        def write(str)
+          raise Errno::EPIPE, "Test"
+        end
+      end
+      @status.stub!(:exitstatus).and_return(1)
+      @provider.stub!(:popen4).and_yield(1234, WriteErrPipe.new, StringIO.new, StringIO.new).and_return(@status)
+
+      Chef::Log.should_receive(:debug).with("Broken pipe - Test")
+
+      lambda do
+        @provider.send(:write_crontab, "Foo")
+      end.should raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1")
+    end
+
   end
 end