From d52b109cdd5ba81947332ce07feac73569895b52 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Thu, 25 Jul 2013 13:50:59 +0100 Subject: [PATCH] No general exception catching while rendering pillar. Refs #6305. Refs #5910. * All errors while processing pillar data are now logged besides being returned. * Removed the general exception catch. All we need to is check for the expected data type. If it ain't, log the error and continue to the next iteration. * Added a mocked test case which besides testing the issue reported on #5910, also tests for the proper includes data format and if salt fails accordingly and even if common data is merged. --- salt/pillar/__init__.py | 46 ++++++++++++----- tests/unit/pillar_test.py | 105 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 14 deletions(-) create mode 100644 tests/unit/pillar_test.py diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 69e82d113f..80734d7439 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -272,27 +272,34 @@ class Pillar(object): errors = [] fn_ = self.client.get_state(sls, env).get('dest', False) if not fn_: - errors.append(('Specified SLS {0} in environment {1} is not' - ' available on the salt master').format(sls, env)) + msg = ('Specified SLS {0!r} in environment {1!r} is not' + ' available on the salt master').format(sls, env) + log.error(msg) + errors.append(msg) state = None try: state = compile_template( fn_, self.rend, self.opts['renderer'], env, sls, **defaults) except Exception as exc: - errors.append(('Rendering SLS {0} failed, render error:\n{1}' - .format(sls, exc))) + msg = 'Rendering SLS {0!r} failed, render error:\n{1}'.format( + sls, exc + ) + log.error(msg) + errors.append(msg) mods.add(sls) nstate = None if state: if not isinstance(state, dict): - errors.append(('SLS {0} does not render to a dictionary' - .format(sls))) + msg = 'SLS {0!r} does not render to a dictionary'.format(sls) + log.error(msg) + errors.append(msg) else: if 'include' in state: if not isinstance(state['include'], list): - err = ('Include Declaration in SLS {0} is not formed ' - 'as a list'.format(sls)) - errors.append(err) + msg = ('Include Declaration in SLS {0!r} is not ' + 'formed as a list'.format(sls)) + log.error(msg) + errors.append(msg) else: for sub_sls in state.pop('include'): if isinstance(sub_sls, dict): @@ -328,13 +335,24 @@ class Pillar(object): mods = set() for sls in pstates: pstate, mods, err = self.render_pstate(sls, env, mods) - if pstate: - try: - pillar.update(pstate) - except Exception: - pass + if err: errors += err + + if pstate is not None: + if not isinstance(pstate, dict): + log.error( + 'The rendered pillar sls file, {0!r} state did ' + 'not return the expected data format. This is ' + 'a sign of a malformed pillar sls file. Returned ' + 'errors: {1}'.format( + sls, + ', '.join(['{0!r}'.format(e) for e in errors]) + ) + ) + continue + pillar.update(pstate) + return pillar, errors def ext_pillar(self, pillar): diff --git a/tests/unit/pillar_test.py b/tests/unit/pillar_test.py new file mode 100644 index 0000000000..ca84972bf0 --- /dev/null +++ b/tests/unit/pillar_test.py @@ -0,0 +1,105 @@ +# -*- coding: utf-8 -*- +''' + tests.unit.pillar_test + ~~~~~~~~~~~~~~~~~~~~~~ + + :codeauthor: :email:`Pedro Algarvio (pedro@algarvio.me)` + :copyright: © 2013 by the SaltStack Team, see AUTHORS for more details. + :license: Apache 2.0, see LICENSE for more details. +''' + +# Import Salt Testing libs +from salttesting import skipIf, TestCase +from salttesting.helpers import ensure_in_syspath +ensure_in_syspath('../') + +# Import salt libs +import salt.pillar + +# Import 3rd-party libs +try: + from mock import MagicMock, patch + HAS_MOCK = True +except ImportError: + HAS_MOCK = False + + +@skipIf(HAS_MOCK is False, 'mock python module is unavailable') +class PillarTestCase(TestCase): + + @patch('salt.pillar.compile_template') + def test_malformed_pillar_sls(self, compile_template): + opts = { + 'renderer': 'json', + 'state_top': '', + 'pillar_roots': [], + 'extension_modules': '' + } + grains = { + 'os': 'Ubuntu', + 'os_family': 'Debian', + 'oscodename': 'raring', + 'osfullname': 'Ubuntu', + 'osrelease': '13.04', + 'kernel': 'Linux' + } + pillar = salt.pillar.Pillar(opts, grains, 'mocked-minion', 'base') + # Mock getting the proper template files + pillar.client.get_state = MagicMock( + return_value={ + 'dest': '/path/to/pillar/files/foo.sls', + 'source': 'salt://foo.sls' + } + ) + + # Template compilation returned a string + compile_template.return_value = 'BAHHH' + self.assertEqual( + pillar.render_pillar({'base': ['foo.sls']}), + ({}, ['SLS \'foo.sls\' does not render to a dictionary']) + ) + + # Template compilation returned a list + compile_template.return_value = ['BAHHH'] + self.assertEqual( + pillar.render_pillar({'base': ['foo.sls']}), + ({}, ['SLS \'foo.sls\' does not render to a dictionary']) + ) + + # Template compilation returned a dictionary, which is what's expected + compile_template.return_value = {'foo': 'bar'} + self.assertEqual( + pillar.render_pillar({'base': ['foo.sls']}), + ({'foo': 'bar'}, []) + ) + + # Test improper includes + compile_template.side_effect = [ + {'foo': 'bar', 'include': 'blah'}, + {'foo2': 'bar2'} + ] + self.assertEqual( + pillar.render_pillar({'base': ['foo.sls']}), + ({'foo': 'bar', 'include': 'blah'}, + ["Include Declaration in SLS 'foo.sls' is not formed as a list"]) + ) + + # Test includes as a list, which is what's expected + compile_template.side_effect = [ + {'foo': 'bar', 'include': ['blah']}, + {'foo2': 'bar2'} + ] + self.assertEqual( + pillar.render_pillar({'base': ['foo.sls']}), + ({'foo': 'bar', 'foo2': 'bar2'}, []) + ) + + # Test includes as a list overriding data + compile_template.side_effect = [ + {'foo': 'bar', 'include': ['blah']}, + {'foo': 'bar2'} + ] + self.assertEqual( + pillar.render_pillar({'base': ['foo.sls']}), + ({'foo': 'bar2'}, []) + )