Merge pull request #40034 from sp1r/fix-pillar-scheduling

Disallow modification of jobs from pillar with schedule execution module

salt/minion.py

@@ -1822,17 +1822,17 @@ class Minion(MinionBase):
         elif func == 'add':
             self.schedule.add_job(schedule, persist)
         elif func == 'modify':
-            self.schedule.modify_job(name, schedule, persist, where)
+            self.schedule.modify_job(name, schedule, persist)
         elif func == 'enable':
             self.schedule.enable_schedule()
         elif func == 'disable':
             self.schedule.disable_schedule()
         elif func == 'enable_job':
-            self.schedule.enable_job(name, persist, where)
+            self.schedule.enable_job(name, persist)
         elif func == 'run_job':
             self.schedule.run_job(name)
         elif func == 'disable_job':
-            self.schedule.disable_job(name, persist, where)
+            self.schedule.disable_job(name, persist)
         elif func == 'reload':
             self.schedule.reload(schedule)
         elif func == 'list':

salt/utils/schedule.py

@@ -424,15 +424,34 @@ class Schedule(object):
 
     def option(self, opt):
         '''
-        Return the schedule data structure
+        Return options merged from config and pillar
         '''
         if 'config.merge' in self.functions:
             return self.functions['config.merge'](opt, {}, omit_master=True)
         return self.opts.get(opt, {})
 
+    def _get_schedule(self,
+                      include_opts=True,
+                      include_pillar=True):
+        '''
+        Return the schedule data structure
+        '''
+        schedule = {}
+        if include_pillar:
+            pillar_schedule = self.opts.get('pillar', {}).get('schedule', {})
+            if not isinstance(pillar_schedule, dict):
+                raise ValueError('Schedule must be of type dict.')
+            schedule.update(pillar_schedule)
+        if include_opts:
+            opts_schedule = self.opts.get('schedule', {})
+            if not isinstance(opts_schedule, dict):
+                raise ValueError('Schedule must be of type dict.')
+            schedule.update(opts_schedule)
+        return schedule
+
     def persist(self):
         '''
-        Persist the modified schedule into <<configdir>>/minion.d/_schedule.conf
+        Persist the modified schedule into <<configdir>>/<<default_include>>/_schedule.conf
         '''
         config_dir = self.opts.get('conf_dir', None)
         if config_dir is None and 'conf_file' in self.opts:
@@ -454,33 +473,27 @@ class Schedule(object):
             with salt.utils.fopen(schedule_conf, 'wb+') as fp_:
                 fp_.write(
                     salt.utils.to_bytes(
-                        yaml.dump({'schedule': self.option('schedule')})
+                        yaml.dump({'schedule': self._get_schedule(include_pillar=False)})
                     )
                 )
         except (IOError, OSError):
             log.error('Failed to persist the updated schedule',
                       exc_info_on_loglevel=logging.DEBUG)
 
-    def delete_job(self, name, persist=True, where=None):
+    def delete_job(self, name, persist=True):
         '''
-        Deletes a job from the scheduler.
+        Deletes a job from the scheduler. Ignore jobs from pillar
         '''
-        if where is None or where != 'pillar':
-            # ensure job exists, then delete it
-            schedule = self.option('schedule')
-            if name in schedule:
-                del schedule[name]
-        else:
-            # If job is in pillar, delete it there too
-            if 'schedule' in self.opts['pillar']:
-                if name in self.opts['pillar']['schedule']:
-                    del self.opts['pillar']['schedule'][name]
-            schedule = self.opts['pillar']['schedule']
-            log.warning('Pillar schedule deleted. Pillar refresh recommended. Run saltutil.refresh_pillar.')
+        # ensure job exists, then delete it
+        if name in self.opts['schedule']:
+            del self.opts['schedule'][name]
+        elif name in self._get_schedule(include_opts=False):
+            log.warning('Cannot delete job {0}, '
+                        'it`s in the pillar!'.format(name))
 
         # Fire the complete event back along with updated list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
-        evt.fire_event({'complete': True, 'schedule': schedule},
+        evt.fire_event({'complete': True, 'schedule': self._get_schedule()},
                        tag='/salt/minion/minion_schedule_delete_complete')
 
         # remove from self.intervals
@@ -490,28 +503,22 @@ class Schedule(object):
         if persist:
             self.persist()
 
-    def delete_job_prefix(self, name, persist=True, where=None):
+    def delete_job_prefix(self, name, persist=True):
         '''
-        Deletes a job from the scheduler.
+        Deletes a job from the scheduler. Ignores jobs from pillar
         '''
-        if where is None or where != 'pillar':
-            # ensure job exists, then delete it
-            schedule = self.option('schedule')
-            for job in list(schedule.keys()):
-                if job.startswith(name):
-                    del schedule[job]
-        else:
-            # If job is in pillar, delete it there too
-            if 'schedule' in self.opts['pillar']:
-                for job in list(self.opts['pillar']['schedule'].keys()):
-                    if job.startswith(name):
-                        del self.opts['pillar']['schedule'][job]
-            schedule = self.opts['pillar']['schedule']
-            log.warning('Pillar schedule deleted. Pillar refresh recommended. Run saltutil.refresh_pillar.')
+        # ensure job exists, then delete it
+        for job in list(self.opts['schedule'].keys()):
+            if job.startswith(name):
+                del self.opts['schedule'][job]
+        for job in self._get_schedule(include_opts=False).keys():
+            if job.startswith(name):
+                log.warning('Cannot delete job {0}, '
+                            'it`s in the pillar!'.format(job))
 
         # Fire the complete event back along with updated list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
-        evt.fire_event({'complete': True, 'schedule': schedule},
+        evt.fire_event({'complete': True, 'schedule': self._get_schedule()},
                        tag='/salt/minion/minion_schedule_delete_complete')
 
         # remove from self.intervals
@@ -544,77 +551,80 @@ class Schedule(object):
 
         new_job = next(six.iterkeys(data))
 
-        schedule = self.option('schedule')
-        if new_job in schedule:
+        if new_job in self._get_schedule(include_opts=False):
+            log.warning('Cannot update job {0}, '
+                        'it`s in the pillar!'.format(new_job))
+
+        elif new_job in self.opts['schedule']:
             log.info('Updating job settings for scheduled '
                      'job: {0}'.format(new_job))
+            self.opts['schedule'].update(data)
+
         else:
             log.info('Added new job {0} to scheduler'.format(new_job))
-
-        schedule.update(data)
+            self.opts['schedule'].update(data)
 
         # Fire the complete event back along with updated list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
-        evt.fire_event({'complete': True, 'schedule': schedule},
+        evt.fire_event({'complete': True, 'schedule': self._get_schedule()},
                        tag='/salt/minion/minion_schedule_add_complete')
 
         if persist:
             self.persist()
 
-    def enable_job(self, name, persist=True, where=None):
+    def enable_job(self, name, persist=True):
         '''
-        Enable a job in the scheduler.
+        Enable a job in the scheduler. Ignores jobs from pillar
         '''
-        if where == 'pillar':
-            self.opts['pillar']['schedule'][name]['enabled'] = True
-            schedule = self.opts['pillar']['schedule']
-        else:
-            schedule = self.option('schedule')
-            schedule[name]['enabled'] = True
+        # ensure job exists, then enable it
+        if name in self.opts['schedule']:
+            self.opts['schedule'][name]['enabled'] = True
+            log.info('Enabling job {0} in scheduler'.format(name))
+        elif name in self._get_schedule(include_opts=False):
+            log.warning('Cannot modify job {0}, '
+                        'it`s in the pillar!'.format(name))
 
         # Fire the complete event back along with updated list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
-        evt.fire_event({'complete': True, 'schedule': schedule},
+        evt.fire_event({'complete': True, 'schedule': self._get_schedule()},
                        tag='/salt/minion/minion_schedule_enabled_job_complete')
 
-        log.info('Enabling job {0} in scheduler'.format(name))
-
         if persist:
             self.persist()
 
-    def disable_job(self, name, persist=True, where=None):
+    def disable_job(self, name, persist=True):
         '''
-        Disable a job in the scheduler.
+        Disable a job in the scheduler. Ignores jobs from pillar
         '''
-        if where == 'pillar':
-            self.opts['pillar']['schedule'][name]['enabled'] = False
-            schedule = self.opts['pillar']['schedule']
-        else:
-            schedule = self.option('schedule')
-            schedule[name]['enabled'] = False
+        # ensure job exists, then disable it
+        if name in self.opts['schedule']:
+            self.opts['schedule'][name]['enabled'] = False
+            log.info('Disabling job {0} in scheduler'.format(name))
+        elif name in self._get_schedule(include_opts=False):
+            log.warning('Cannot modify job {0}, '
+                        'it`s in the pillar!'.format(name))
 
         # Fire the complete event back along with updated list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
-        evt.fire_event({'complete': True, 'schedule': schedule},
+        evt.fire_event({'complete': True, 'schedule': self._get_schedule()},
                        tag='/salt/minion/minion_schedule_disabled_job_complete')
 
-        log.info('Disabling job {0} in scheduler'.format(name))
-
         if persist:
             self.persist()
 
-    def modify_job(self, name, schedule, persist=True, where=None):
+    def modify_job(self, name, schedule, persist=True):
         '''
-        Modify a job in the scheduler.
+        Modify a job in the scheduler. Ignores jobs from pillar
         '''
-        if where == 'pillar':
-            if name in self.opts['pillar']['schedule']:
-                self.delete_job(name, persist, where=where)
-            self.opts['pillar']['schedule'][name] = schedule
-        else:
-            if name in self.option('schedule'):
-                self.delete_job(name, persist, where=where)
-            self.option('schedule')[name] = schedule
+        # ensure job exists, then replace it
+        if name in self.opts['schedule']:
+            self.delete_job(name, persist)
+        elif name in self._get_schedule(include_opts=False):
+            log.warning('Cannot modify job {0}, '
+                        'it`s in the pillar!'.format(name))
+            return
+
+        self.opts['schedule'][name] = schedule
 
         if persist:
             self.persist()
@@ -623,10 +633,7 @@ class Schedule(object):
         '''
         Run a schedule job now
         '''
-        schedule = self.option('schedule')
-        if 'schedule' in self.opts['pillar']:
-            schedule.update(self.opts['pillar']['schedule'])
-        data = schedule[name]
+        data = self._get_schedule().get(name, {})
 
         if 'function' in data:
             func = data['function']
@@ -670,24 +677,22 @@ class Schedule(object):
         '''
         Enable the scheduler.
         '''
-        schedule = self.option('schedule')
-        schedule['enabled'] = True
+        self.opts['schedule']['enabled'] = True
 
         # Fire the complete event back along with updated list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
-        evt.fire_event({'complete': True, 'schedule': schedule},
+        evt.fire_event({'complete': True, 'schedule': self._get_schedule()},
                        tag='/salt/minion/minion_schedule_enabled_complete')
 
     def disable_schedule(self):
         '''
         Disable the scheduler.
         '''
-        schedule = self.option('schedule')
-        schedule['enabled'] = False
+        self.opts['schedule']['enabled'] = False
 
         # Fire the complete event back along with updated list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
-        evt.fire_event({'complete': True, 'schedule': schedule},
+        evt.fire_event({'complete': True, 'schedule': self._get_schedule()},
                        tag='/salt/minion/minion_schedule_disabled_complete')
 
     def reload(self, schedule):
@@ -705,16 +710,12 @@ class Schedule(object):
         '''
         List the current schedule items
         '''
-        schedule = {}
         if where == 'pillar':
-            if 'schedule' in self.opts['pillar']:
-                schedule.update(self.opts['pillar']['schedule'])
+            schedule = self._get_schedule(include_opts=False)
         elif where == 'opts':
-            schedule.update(self.option('schedule'))
+            schedule = self._get_schedule(include_pillar=False)
         else:
-            schedule.update(self.option('schedule'))
-            if 'schedule' in self.opts['pillar']:
-                schedule.update(self.opts['pillar']['schedule'])
+            schedule = self._get_schedule()
 
         # Fire the complete event back along with the list of schedule
         evt = salt.utils.event.get_event('minion', opts=self.opts, listen=False)
@@ -919,9 +920,7 @@ class Schedule(object):
         '''
         Evaluate and execute the schedule
         '''
-        schedule = self.option('schedule')
-        if not isinstance(schedule, dict):
-            raise ValueError('Schedule must be of type dict.')
+        schedule = self._get_schedule()
         if 'enabled' in schedule and not schedule['enabled']:
             return
         for job, data in six.iteritems(schedule):

tests/unit/utils/schedule_test.py

@@ -48,25 +48,25 @@ class ScheduleTestCase(TestCase):
         '''
         Tests ensuring the job exists and deleting it
         '''
-        self.schedule.opts.update({'schedule': {'foo': 'bar'}, 'pillar': ''})
+        self.schedule.opts.update({'schedule': {'foo': 'bar'}, 'pillar': {}})
         self.assertIn('foo', self.schedule.opts['schedule'])
         self.schedule.delete_job('foo')
         self.assertNotIn('foo', self.schedule.opts['schedule'])
 
     def test_delete_job_in_pillar(self):
         '''
-        Tests deleting job in pillar
+        Tests ignoring deletion job from pillar
         '''
-        self.schedule.opts.update({'pillar': {'schedule': {'foo': 'bar'}}, 'schedule': ''})
+        self.schedule.opts.update({'pillar': {'schedule': {'foo': 'bar'}}, 'schedule': {}})
+        self.assertIn('foo', self.schedule.opts['pillar']['schedule'])
+        self.schedule.delete_job('foo')
         self.assertIn('foo', self.schedule.opts['pillar']['schedule'])
-        self.schedule.delete_job('foo', where='pillar')
-        self.assertNotIn('foo', self.schedule.opts['pillar']['schedule'])
 
     def test_delete_job_intervals(self):
         '''
         Tests removing job from intervals
         '''
-        self.schedule.opts.update({'pillar': '', 'schedule': ''})
+        self.schedule.opts.update({'pillar': {}, 'schedule': {}})
         self.schedule.intervals = {'foo': 'bar'}
         self.schedule.delete_job('foo')
         self.assertNotIn('foo', self.schedule.intervals)
@@ -76,7 +76,7 @@ class ScheduleTestCase(TestCase):
         Tests ensuring jobs exists and deleting them by prefix
         '''
         self.schedule.opts.update({'schedule': {'foobar': 'bar', 'foobaz': 'baz', 'fooboo': 'boo'},
-                                   'pillar': ''})
+                                   'pillar': {}})
         ret = copy.deepcopy(self.schedule.opts)
         del ret['schedule']['foobar']
         del ret['schedule']['foobaz']
@@ -85,14 +85,12 @@ class ScheduleTestCase(TestCase):
 
     def test_delete_job_prefix_in_pillar(self):
         '''
-        Tests deleting jobs by prefix in pillar
+        Tests ignoring deletion jobs by prefix from pillar
         '''
         self.schedule.opts.update({'pillar': {'schedule': {'foobar': 'bar', 'foobaz': 'baz', 'fooboo': 'boo'}},
-                                   'schedule': ''})
+                                   'schedule': {}})
         ret = copy.deepcopy(self.schedule.opts)
-        del ret['pillar']['schedule']['foobar']
-        del ret['pillar']['schedule']['foobaz']
-        self.schedule.delete_job_prefix('fooba', where='pillar')
+        self.schedule.delete_job_prefix('fooba')
         self.assertEqual(self.schedule.opts, ret)
 
     # add_job tests
@@ -118,8 +116,10 @@ class ScheduleTestCase(TestCase):
         data = {'foo': {'bar': 'baz'}}
         ret = copy.deepcopy(self.schedule.opts)
         ret.update({'schedule': {'foo': {'bar': 'baz', 'enabled': True},
-                                 'hello': {'world': 'peace', 'enabled': True}}})
-        self.schedule.opts.update({'schedule': {'hello': {'world': 'peace', 'enabled': True}}})
+                                 'hello': {'world': 'peace', 'enabled': True}},
+                    'pillar': {}})
+        self.schedule.opts.update({'schedule': {'hello': {'world': 'peace', 'enabled': True}},
+                                   'pillar': {}})
         Schedule.add_job(self.schedule, data)
         self.assertEqual(self.schedule.opts, ret)
 
@@ -135,11 +135,11 @@ class ScheduleTestCase(TestCase):
 
     def test_enable_job_pillar(self):
         '''
-        Tests enabling a job in pillar
+        Tests ignoring enable a job from pillar
         '''
-        self.schedule.opts.update({'pillar': {'schedule': {'name': {'enabled': 'foo'}}}})
-        Schedule.enable_job(self.schedule, 'name', persist=False, where='pillar')
-        self.assertTrue(self.schedule.opts['pillar']['schedule']['name']['enabled'])
+        self.schedule.opts.update({'pillar': {'schedule': {'name': {'enabled': False}}}})
+        Schedule.enable_job(self.schedule, 'name', persist=False)
+        self.assertFalse(self.schedule.opts['pillar']['schedule']['name']['enabled'])
 
     # disable_job tests
 
@@ -147,17 +147,17 @@ class ScheduleTestCase(TestCase):
         '''
         Tests disabling a job
         '''
-        self.schedule.opts.update({'schedule': {'name': {'enabled': 'foo'}}})
+        self.schedule.opts.update({'schedule': {'name': {'enabled': 'foo'}}, 'pillar': {}})
         Schedule.disable_job(self.schedule, 'name')
         self.assertFalse(self.schedule.opts['schedule']['name']['enabled'])
 
     def test_disable_job_pillar(self):
         '''
-        Tests disabling a job in pillar
+        Tests ignoring disable a job in pillar
         '''
-        self.schedule.opts.update({'pillar': {'schedule': {'name': {'enabled': 'foo'}}}})
-        Schedule.disable_job(self.schedule, 'name', persist=False, where='pillar')
-        self.assertFalse(self.schedule.opts['pillar']['schedule']['name']['enabled'])
+        self.schedule.opts.update({'pillar': {'schedule': {'name': {'enabled': True}}}, 'schedule': {}})
+        Schedule.disable_job(self.schedule, 'name', persist=False)
+        self.assertTrue(self.schedule.opts['pillar']['schedule']['name']['enabled'])
 
     # modify_job tests
 
@@ -165,22 +165,32 @@ class ScheduleTestCase(TestCase):
         '''
         Tests modifying a job in the scheduler
         '''
-        schedule = {'schedule': {'foo': 'bar'}}
+        schedule = {'foo': 'bar'}
+        self.schedule.opts.update({'schedule': {'name': 'baz'}, 'pillar': {}})
         ret = copy.deepcopy(self.schedule.opts)
-        ret.update({'schedule': {'foo': 'bar', 'name': {'schedule': {'foo': 'bar'}}}})
-        self.schedule.opts.update({'schedule': {'foo': 'bar'}})
+        ret.update({'schedule': {'name': {'foo': 'bar'}}})
+        Schedule.modify_job(self.schedule, 'name', schedule)
+        self.assertEqual(self.schedule.opts, ret)
+
+    def test_modify_job_not_exists(self):
+        '''
+        Tests modifying a job in the scheduler if jobs not exists
+        '''
+        schedule = {'foo': 'bar'}
+        self.schedule.opts.update({'schedule': {}, 'pillar': {}})
+        ret = copy.deepcopy(self.schedule.opts)
+        ret.update({'schedule': {'name':  {'foo': 'bar'}}})
         Schedule.modify_job(self.schedule, 'name', schedule)
         self.assertEqual(self.schedule.opts, ret)
 
     def test_modify_job_pillar(self):
         '''
-        Tests modifying a job in the scheduler in pillar
+        Tests ignoring modification of job from pillar
         '''
         schedule = {'foo': 'bar'}
+        self.schedule.opts.update({'schedule': {}, 'pillar': {'schedule': {'name': 'baz'}}})
         ret = copy.deepcopy(self.schedule.opts)
-        ret.update({'pillar': {'schedule': {'name': {'foo': 'bar'}}}})
-        self.schedule.opts.update({'pillar': {'schedule': {'name': {'foo': 'bar'}}}})
-        Schedule.modify_job(self.schedule, 'name', schedule, persist=False, where='pillar')
+        Schedule.modify_job(self.schedule, 'name', schedule, persist=False)
         self.assertEqual(self.schedule.opts, ret)
 
     maxDiff = None
@@ -191,7 +201,7 @@ class ScheduleTestCase(TestCase):
         '''
         Tests enabling the scheduler
         '''
-        self.schedule.opts.update({'schedule': {'enabled': 'foo'}})
+        self.schedule.opts.update({'schedule': {'enabled': 'foo'}, 'pillar': {}})
         Schedule.enable_schedule(self.schedule)
         self.assertTrue(self.schedule.opts['schedule']['enabled'])
 
@@ -201,7 +211,7 @@ class ScheduleTestCase(TestCase):
         '''
         Tests disabling the scheduler
         '''
-        self.schedule.opts.update({'schedule': {'enabled': 'foo'}})
+        self.schedule.opts.update({'schedule': {'enabled': 'foo'}, 'pillar': {}})
         Schedule.disable_schedule(self.schedule)
         self.assertFalse(self.schedule.opts['schedule']['enabled'])
 
@@ -259,11 +269,17 @@ class ScheduleTestCase(TestCase):
 
     def test_eval_schedule_is_not_dict(self):
         '''
-        Tests if the schedule is a dictionary
+        Tests eval if the schedule is not a dictionary
         '''
-        self.schedule.opts.update({'schedule': ''})
+        self.schedule.opts.update({'schedule': '', 'pillar': {'schedule': {}}})
         self.assertRaises(ValueError, Schedule.eval, self.schedule)
 
+    def test_eval_schedule_is_not_dict_in_pillar(self):
+        '''
+        Tests eval if the schedule from pillar is not a dictionary
+        '''
+        self.schedule.opts.update({'schedule': {}, 'pillar': {'schedule': ''}})
+        self.assertRaises(ValueError, Schedule.eval, self.schedule)
 
 if __name__ == '__main__':
     from integration import run_tests