Merge pull request #5422 from chef/tm/rescue_5115

Mac: ensure launchd's start_calendar_interval gets valid settings

3 files changed, 129 insertions, 11 deletions

diff --git a/lib/chef/provider/launchd.rb b/lib/chef/provider/launchd.rb
index c58d4bfa34..0ec8d0cfe1 100644
--- a/lib/chef/provider/launchd.rb
+++ b/lib/chef/provider/launchd.rb
@@ -115,7 +115,7 @@ class Chef
         res = Chef::Resource::File.new(@path, run_context)
         res.name(@path) if @path
         res.backup(backup) if backup
-        res.content(content) if content
+        res.content(content) if content?
         res.group(group) if group
         res.mode(mode) if mode
         res.owner(owner) if owner
diff --git a/lib/chef/resource/launchd.rb b/lib/chef/resource/launchd.rb
index e584693dee..27937515ec 100644
--- a/lib/chef/resource/launchd.rb
+++ b/lib/chef/resource/launchd.rb
@@ -29,12 +29,6 @@ class Chef
       default_action :create
       allowed_actions :create, :create_if_missing, :delete, :enable, :disable
 
-      def initialize(name, run_context = nil)
-        super
-        provider = Chef::Provider::Launchd
-        resource_name = :launchd
-      end
-
       property :label, String, default: lazy { name }, identity: true
       property :backup, [Integer, FalseClass]
       property :cookbook, String
@@ -46,6 +40,53 @@ class Chef
       property :source, String
       property :session_type, String
 
+      # StartCalendarInterval has some gotchas so we coerce it to help sanity
+      # check.  According to `man 5 launchd.plist`:
+      #   StartCalendarInterval <dictionary of integers or array of dictionaries of integers>
+      #     ... Missing arguments are considered to be wildcard.
+      # What the man page doesn't state, but what was observed (OSX 10.11.5, launchctrl v3.4.0)
+      # Is that keys that are specified, but invalid, will also be treated as a wildcard
+      # this means that an entry like:
+      #   { "Hour"=>0, "Weekday"=>"6-7"}
+      # will not just run on midnight of Sat and Sun, rather it will run _every_ midnight.
+      property :start_calendar_interval, [Hash, Array], coerce: proc { |type|
+        # Coerce into an array of hashes to make validation easier
+        array = if type.is_a?(Array)
+                  type
+                else
+                  [type]
+                end
+
+        # Check to make sure that our array only has hashes
+        unless array.all? { |obj| obj.is_a?(Hash) }
+          error_msg = "start_calendar_interval must be a single hash or an array of hashes!"
+          raise Chef::Exceptions::ValidationFailed, error_msg
+        end
+
+        # Make sure the hashes don't have any incorrect keys/values
+        array.each do |entry|
+          allowed_keys = %w{Minute Hour Day Weekday Month}
+          unless entry.keys.all? { |key| allowed_keys.include?(key) }
+            failed_keys = entry.keys.reject { |k| allowed_keys.include?(k) }.join(", ")
+            error_msg = "The following key(s): #{failed_keys} are invalid for start_calendar_interval, must be one of: #{allowed_keys.join(", ")}"
+            raise Chef::Exceptions::ValidationFailed, error_msg
+          end
+
+          unless entry.values.all? { |val| val.is_a?(Fixnum) }
+            failed_values = entry.values.reject { |val| val.is_a?(Fixnum) }.join(", ")
+            error_msg = "Invalid value(s) (#{failed_values}) for start_calendar_interval item.  Values must be integers!"
+            raise Chef::Exceptions::ValidationFailed, error_msg
+          end
+        end
+
+        # Don't return array if we only have one entry
+        if array.size == 1
+          array.first
+        else
+          array
+        end
+      }
+
       property :type, String, default: "daemon", coerce: proc { |type|
         type = type ? type.downcase : "daemon"
         types = %w{daemon agent}
@@ -89,7 +130,6 @@ class Chef
       property :standard_error_path, String
       property :standard_in_path, String
       property :standard_out_path, String
-      property :start_calendar_interval, Hash
       property :start_interval, Integer
       property :start_on_mount, [ TrueClass, FalseClass ]
       property :throttle_interval, Integer
diff --git a/spec/unit/provider/launchd_spec.rb b/spec/unit/provider/launchd_spec.rb
index 2893044833..3e45433c62 100644
--- a/spec/unit/provider/launchd_spec.rb
+++ b/spec/unit/provider/launchd_spec.rb
@@ -40,7 +40,7 @@ describe Chef::Provider::Launchd do
 \t<string>/Library/scripts/call_mom.sh</string>
 \t<key>StartCalendarInterval</key>
 \t<dict>
-\t\t<key>Hourly</key>
+\t\t<key>Hour</key>
 \t\t<integer>10</integer>
 \t\t<key>Weekday</key>
 \t\t<integer>7</integer>
@@ -50,13 +50,42 @@ describe Chef::Provider::Launchd do
 </dict>
 </plist>
 XML
+    let(:test_plist_multiple_intervals) { String.new <<-XML }
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+\t<key>Label</key>
+\t<string>call.mom.weekly</string>
+\t<key>Program</key>
+\t<string>/Library/scripts/call_mom.sh</string>
+\t<key>StartCalendarInterval</key>
+\t<array>
+\t\t<dict>
+\t\t\t<key>Hour</key>
+\t\t\t<integer>11</integer>
+\t\t\t<key>Weekday</key>
+\t\t\t<integer>1</integer>
+\t\t</dict>
+\t\t<dict>
+\t\t\t<key>Hour</key>
+\t\t\t<integer>12</integer>
+\t\t\t<key>Weekday</key>
+\t\t\t<integer>2</integer>
+\t\t</dict>
+\t</array>
+\t<key>TimeOut</key>
+\t<integer>300</integer>
+</dict>
+</plist>
+XML
 
     let(:test_hash) do
       {
       "Label" => "call.mom.weekly",
       "Program" => "/Library/scripts/call_mom.sh",
       "StartCalendarInterval" => {
-        "Hourly" => 10,
+        "Hour" => 10,
         "Weekday" => 7,
       },
       "TimeOut" => 300,
@@ -100,12 +129,61 @@ XML
         it "should produce the test_plist from properties" do
           new_resource.program "/Library/scripts/call_mom.sh"
           new_resource.time_out 300
-          new_resource.start_calendar_interval "Hourly" => 10, "Weekday" => 7
+          new_resource.start_calendar_interval "Hour" => 10, "Weekday" => 7
           expect(provider.content?).to be_truthy
           expect(provider.content).to eql(test_plist)
         end
       end
 
+      describe "start_calendar_interval is passed" do
+        it "should allow array of Hashes" do
+          allowed = (1..2).collect do |num|
+            {
+              "Hour"    => 10 + num,
+              "Weekday" => num,
+            }
+          end
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          new_resource.start_calendar_interval allowed
+          expect(provider.content?).to be_truthy
+          expect(provider.content).to eql(test_plist_multiple_intervals)
+        end
+
+        it "should allow all StartCalendarInterval keys" do
+          allowed = {
+            "Minute"  => 1,
+            "Hour"    => 1,
+            "Day"     => 1,
+            "Weekday" => 1,
+            "Month"   => 1,
+          }
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          new_resource.start_calendar_interval allowed
+          expect(provider.content?).to be_truthy
+          %w{Minute Hour Day Weekday Month}.each do |key|
+            expect(provider.content).to include("<key>#{key}</key>")
+          end
+        end
+
+        it "should not allow invalid ShowCalendarInterval keys" do
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          expect do
+            new_resource.start_calendar_interval "Hourly" => 1
+          end.to raise_error(/Hourly are invalid/)
+        end
+
+        it "should not allow non-integer values" do
+          new_resource.program "/Library/scripts/call_mom.sh"
+          new_resource.time_out 300
+          expect do
+            new_resource.start_calendar_interval "Weekday" => "1-2"
+          end.to raise_error(/Invalid value.*\(1-2\)/)
+        end
+      end
+
       describe "hash is passed" do
         it "should produce the test_plist from the hash" do
           new_resource.hash test_hash