Merge pull request #28269 from rallytime/refactor_rabbitmq_user_state

Refactor rabbitmq_user state to use test=True correctly

salt/modules/rabbitmq.py

@@ -4,16 +4,17 @@ Module to provide RabbitMQ compatibility to Salt.
 Todo: A lot, need to add cluster support, logging, and minion configuration
 data.
 '''
+
+# Import Python Libs
 from __future__ import absolute_import
-
-# Import salt libs
-import salt.utils
-
-# Import python libs
 import logging
 import random
 import string
+
+# Import Salt Libs
+import salt.utils
 from salt.ext.six.moves import range
+from salt.exceptions import CommandExecutionError
 
 log = logging.getLogger(__name__)
 
@@ -26,14 +27,15 @@ def __virtual__():
 
 
 def _format_response(response, msg):
+    error = 'RabbitMQ command failed: {0}'.format(response)
     if isinstance(response, dict):
         if response['retcode'] != 0:
-            msg = 'Error'
+            raise CommandExecutionError(error)
         else:
             msg = response['stdout']
     else:
         if 'Error' in response:
-            msg = 'Error'
+            raise CommandExecutionError(error)
     return {
         msg: response
     }

salt/states/rabbitmq_user.py

@@ -8,18 +8,18 @@ Example:
 .. code-block:: yaml
 
     rabbit_user:
-        rabbitmq_user.present:
-            - password: password
-            - force: True
-            - tags:
-                - monitoring
-                - user
-            - perms:
-              - '/':
-                - '.*'
-                - '.*'
-                - '.*'
-            - runas: rabbitmq
+      rabbitmq_user.present:
+        - password: password
+        - force: True
+        - tags:
+          - monitoring
+          - user
+        - perms:
+          - '/':
+          - '.*'
+          - '.*'
+          - '.*'
+        - runas: rabbitmq
 '''
 
 # Import python libs
@@ -28,9 +28,8 @@ import logging
 
 # Import salt libs
 import salt.utils
-
-# Import 3rd-party libs
 import salt.ext.six as six
+from salt.exceptions import CommandExecutionError
 
 log = logging.getLogger(__name__)
 
@@ -42,20 +41,25 @@ def __virtual__():
     return salt.utils.which('rabbitmqctl') is not None
 
 
-def _check_perms_changes(name, newperms, runas=None):
+def _check_perms_changes(name, newperms, runas=None, existing=None):
     '''
-    Whether Rabbitmq user's permissions need to be changed
+    Check whether Rabbitmq user's permissions need to be changed.
     '''
     if not newperms:
         return False
 
-    existing_perms = __salt__['rabbitmq.list_user_permissions'](name, runas=runas)
+    if existing is None:
+        try:
+            existing = __salt__['rabbitmq.list_user_permissions'](name, runas=runas)
+        except CommandExecutionError as err:
+            log.error('Error: {0}'.format(err))
+            return False
 
     perm_need_change = False
     for vhost_perms in newperms:
         for vhost, perms in vhost_perms.iteritems():
-            if vhost in existing_perms:
-                if perms != existing_perms[vhost]:
+            if vhost in existing:
+                if perms != existing[vhost]:
                     perm_need_change = True
             else:
                 perm_need_change = True
@@ -63,14 +67,19 @@ def _check_perms_changes(name, newperms, runas=None):
     return perm_need_change
 
 
-def _check_tags_changes(name, newtags, runas=None):
+def _check_tags_changes(name, new_tags, runas=None):
     '''
     Whether Rabbitmq user's tags need to be changed
     '''
-    if newtags:
-        if isinstance(newtags, str):
-            newtags = newtags.split()
-        return __salt__['rabbitmq.list_users'](runas=runas)[name] - set(newtags)
+    if new_tags:
+        if isinstance(new_tags, str):
+            new_tags = new_tags.split()
+        try:
+            users = __salt__['rabbitmq.list_users'](runas=runas)[name] - set(new_tags)
+        except CommandExecutionError as err:
+            log.error('Error: {0}'.format(err))
+            return []
+        return users
     else:
         return []
 
@@ -97,97 +106,109 @@ def present(name,
     runas
         Name of the user to run the command
     '''
-
-    ret = {'name': name, 'result': True, 'comment': '', 'changes': {}}
-    result = {}
-
-    user_exists = __salt__['rabbitmq.user_exists'](name, runas=runas)
-
-    if user_exists and not any((force, perms, tags)):
-        log.debug('RabbitMQ user %s exists, '
-                  'and force is not set.', name)
-        ret['comment'] = 'User {0} already presents'.format(name)
+    ret = {'name': name, 'result': False, 'comment': '', 'changes': {}}
+    try:
+        user = __salt__['rabbitmq.user_exists'](name, runas=runas)
+    except CommandExecutionError as err:
+        ret['comment'] = 'Error: {0}'.format(err)
         return ret
+
+    if user and not any((force, perms, tags)):
+        log.debug('RabbitMQ user \'{0}\' exists and force is not set.'.format(name))
+        ret['comment'] = 'User \'{0}\' is already present.'.format(name)
+        ret['result'] = True
+        return ret
+
+    if not user:
+        ret['changes'].update({'user':
+                              {'old': '',
+                               'new': name}})
+        if __opts__['test']:
+            ret['result'] = None
+            ret['comment'] = 'User \'{0}\' is set to be created.'.format(name)
+            return ret
+
+        log.debug('RabbitMQ user \'{0}\' doesn\'t exist - Creating.'.format(name))
+        try:
+            __salt__['rabbitmq.add_user'](name, password, runas=runas)
+        except CommandExecutionError as err:
+            ret['comment'] = 'Error: {0}'.format(err)
+            return ret
     else:
-        changes = {'old': '', 'new': ''}
-
-        if not user_exists:
-            if __opts__['test']:
-                ret['result'] = None
-                ret['comment'] = 'User {0} is set to be created'.format(name)
-                return ret
-
-            log.debug("RabbitMQ user %s doesn't exist - Creating", name)
-            result = __salt__['rabbitmq.add_user'](
-                name, password, runas=runas)
-        else:
-            log.debug('RabbitMQ user %s exists', name)
-            if force:
-                if __opts__['test']:
-                    ret['result'] = None
-
-                if password is not None:
-                    if __opts__['test']:
-                        ret['comment'] = ('User {0}\'s password is '
-                                          'set to be updated'.format(name))
+        log.debug('RabbitMQ user \'{0}\' exists'.format(name))
+        if force:
+            if password is not None:
+                if not __opts__['test']:
+                    try:
+                        __salt__['rabbitmq.change_password'](name, password, runas=runas)
+                    except CommandExecutionError as err:
+                        ret['comment'] = 'Error: {0}'.format(err)
                         return ret
-
-                    result = __salt__['rabbitmq.change_password'](
-                        name, password, runas=runas)
-                    changes['new'] = 'Set password.\n'
-                else:
-                    log.debug('Password for %s is not set - Clearing password',
-                              name)
-                    if __opts__['test']:
-                        ret['comment'] = ('User {0}\'s password is '
-                                          'set to be removed'.format(name))
+                ret['changes'].update({'password':
+                                      {'old': '',
+                                       'new': 'Set password.'}})
+            else:
+                if not __opts__['test']:
+                    log.debug('Password for {0} is not set - Clearing password.'.format(name))
+                    try:
+                        __salt__['rabbitmq.clear_password'](name, runas=runas)
+                    except CommandExecutionError as err:
+                        ret['comment'] = 'Error: {0}'.format(err)
                         return ret
+                ret['changes'].update({'password':
+                                      {'old': 'Removed password.',
+                                       'new': ''}})
 
-                    result = __salt__['rabbitmq.clear_password'](
-                        name, runas=runas)
-                    changes['old'] += 'Removed password.\n'
-
-        if _check_tags_changes(name, tags, runas=runas):
-            if __opts__['test']:
-                ret['result'] = None
-                ret['comment'] += ('Tags for user {0} '
-                                   'is set to be changed'.format(name))
+    new_tags = _check_tags_changes(name, tags, runas=runas)
+    if new_tags:
+        if not __opts__['test']:
+            try:
+                __salt__['rabbitmq.set_user_tags'](name, tags, runas=runas)
+            except CommandExecutionError as err:
+                ret['comment'] = 'Error: {0}'.format(err)
                 return ret
-            result.update(__salt__['rabbitmq.set_user_tags'](
-                name, tags, runas=runas)
-            )
-            changes['new'] += 'Set tags: {0}\n'.format(tags)
-
-        if _check_perms_changes(name, perms, runas=runas):
-            if __opts__['test']:
-                ret['result'] = None
-                ret['comment'] += ('Permissions for user {0} '
-                                   'is set to be changed'.format(name))
-                return ret
-            for vhost_perm in perms:
-                for vhost, perm in six.iteritems(vhost_perm):
-                    result.update(__salt__['rabbitmq.set_permissions'](
-                        vhost, name, perm[0], perm[1], perm[2], runas=runas)
-                    )
-                    changes['new'] += (
-                        'Set permissions {0} for vhost {1}'
-                    ).format(perm, vhost)
-
-        if 'Error' in result:
-            ret['result'] = False
-            ret['comment'] = result['Error']
-        elif 'Added' in result:
-            ret['comment'] = result['Added']
-            ret['changes'] = changes
-        elif 'Password Changed' in result:
-            ret['comment'] = result['Password Changed']
-            ret['changes'] = changes
-        elif 'Password Cleared' in result:
-            ret['comment'] = result['Password Cleared']
-            ret['changes'] = changes
-
+        ret['changes'].update({'tags':
+                              {'old': tags,
+                               'new': new_tags}})
+    try:
+        existing_perms = __salt__['rabbitmq.list_user_permissions'](name, runas=runas)[0]
+    except CommandExecutionError as err:
+        ret['comment'] = 'Error: {0}'.format(err)
         return ret
 
+    if _check_perms_changes(name, perms, runas=runas, existing=existing_perms):
+        for vhost_perm in perms:
+            for vhost, perm in six.iteritems(vhost_perm):
+                if not __opts__['test']:
+                    try:
+                        __salt__['rabbitmq.set_permissions'](
+                            vhost, name, perm[0], perm[1], perm[2], runas=runas
+                        )
+                    except CommandExecutionError as err:
+                        ret['comment'] = 'Error: {0}'.format(err)
+                        return ret
+                new_perms = {vhost: perm}
+                if existing_perms != new_perms:
+                    if ret['changes'].get('perms') is None:
+                        ret['changes'].update({'perms':
+                                              {'old': {},
+                                               'new': {}}})
+                    ret['changes']['perms']['old'].update(existing_perms)
+                    ret['changes']['perms']['new'].update(new_perms)
+
+    ret['result'] = True
+    if ret['changes'] == {}:
+        ret['comment'] = '\'{0}\' is already in the desired state.'.format(name)
+        return ret
+
+    if __opts__['test']:
+        ret['result'] = None
+        ret['comment'] = 'Configuration for \'{0}\' will change.'.format(name)
+        return ret
+
+    ret['comment'] = '\'{0}\' was configured.'.format(name)
+    return ret
+
 
 def absent(name,
            runas=None):
@@ -199,23 +220,34 @@ def absent(name,
     runas
         User to run the command
     '''
-    ret = {'name': name, 'result': True, 'comment': '', 'changes': {}}
+    ret = {'name': name, 'result': False, 'comment': '', 'changes': {}}
 
-    user_exists = __salt__['rabbitmq.user_exists'](name, runas=runas)
+    try:
+        user_exists = __salt__['rabbitmq.user_exists'](name, runas=runas)
+    except CommandExecutionError as err:
+        ret['comment'] = 'Error: {0}'.format(err)
+        return ret
 
-    if not user_exists:
-        ret['comment'] = 'User {0} is not present'.format(name)
-    elif __opts__['test']:
-        ret['result'] = None
-        if user_exists:
-            ret['comment'] = 'Removing user {0}'.format(name)
+    if user_exists:
+        if not __opts__['test']:
+            try:
+                __salt__['rabbitmq.delete_user'](name, runas=runas)
+            except CommandExecutionError as err:
+                ret['comment'] = 'Error: {0}'.format(err)
+                return ret
+        ret['changes'].update({'name':
+                              {'old': name,
+                               'new': ''}})
     else:
-        result = __salt__['rabbitmq.delete_user'](name, runas=runas)
-        if 'Error' in result:
-            ret['result'] = False
-            ret['comment'] = result['Error']
-        elif 'Deleted' in result:
-            ret['comment'] = 'Deleted'
-            ret['changes'] = {'new': '', 'old': name}
+        ret['result'] = True
+        ret['comment'] = 'The user \'{0}\' is not present.'.format(name)
+        return ret
 
+    if __opts__['test'] and ret['changes']:
+        ret['result'] = None
+        ret['comment'] = 'The user \'{0}\' will be removed.'.format(name)
+        return ret
+
+    ret['result'] = True
+    ret['comment'] = 'The user \'{0}\' was removed.'.format(name)
     return ret

tests/unit/modules/rabbitmq_test.py

@@ -21,6 +21,7 @@ ensure_in_syspath('../../')
 
 # Import Salt Libs
 from salt.modules import rabbitmq
+from salt.exceptions import CommandExecutionError
 
 # Globals
 rabbitmq.__salt__ = {}
@@ -114,9 +115,7 @@ class RabbitmqTestCase(TestCase):
         with patch.dict(rabbitmq.__salt__, {'cmd.run': mock_run}):
             with patch.object(rabbitmq, 'clear_password',
                               return_value={'Error': 'Error', 'retcode': 1}):
-                self.assertDictEqual(rabbitmq.add_user('saltstack'),
-                                     {'Error': {'Error': 'Error',
-                                                'retcode': 1}})
+                self.assertRaises(CommandExecutionError, rabbitmq.add_user, 'saltstack')
 
     # 'delete_user' function tests: 1
 

tests/unit/states/rabbitmq_user_test.py

@@ -21,7 +21,7 @@ ensure_in_syspath('../../')
 # Import Salt Libs
 from salt.states import rabbitmq_user
 
-rabbitmq_user.__opts__ = {}
+rabbitmq_user.__opts__ = {'test': False}
 rabbitmq_user.__salt__ = {}
 
 
@@ -56,38 +56,40 @@ class RabbitmqUserTestCase(TestCase):
                          'rabbitmq.list_users': mock_dct,
                          'rabbitmq.list_user_permissions': mock_pr,
                          'rabbitmq.set_user_tags': mock_add}):
-            comt = ('User foo already presents')
-            ret.update({'comment': comt})
+            comment = 'User \'foo\' is already present.'
+            ret.update({'comment': comment})
             self.assertDictEqual(rabbitmq_user.present(name), ret)
 
             with patch.dict(rabbitmq_user.__opts__, {'test': True}):
-                comt = ('User foo is set to be created')
-                ret.update({'comment': comt, 'result': None})
+                comment = 'User \'foo\' is set to be created.'
+                changes = {'user': {'new': 'foo', 'old': ''}}
+                ret.update({'comment': comment, 'result': None, 'changes': changes})
                 self.assertDictEqual(rabbitmq_user.present(name), ret)
 
-                comt = ("User foo's password is set to be updated")
-                ret.update({'comment': comt})
+                comment = 'Configuration for \'foo\' will change.'
+                changes = {'password': {'new': 'Set password.', 'old': ''}}
+                ret.update({'comment': comment, 'changes': changes})
                 self.assertDictEqual(rabbitmq_user.present(name,
                                                            password=passwd,
                                                            force=True), ret)
 
-                comt = ("User foo's password is set to be removed")
-                ret.update({'comment': comt})
+                changes = {'password': {'new': '', 'old': 'Removed password.'}}
+                ret.update({'changes': changes})
                 self.assertDictEqual(rabbitmq_user.present(name, force=True),
                                      ret)
 
-                comt = ('Tags for user foo is set to be changed')
-                ret.update({'comment': comt})
+                changes = {'tags': {'new': set(['e', 'r', 's', 'u']), 'old': 'user'}}
+                ret.update({'changes': changes})
                 self.assertDictEqual(rabbitmq_user.present(name, tags=tag), ret)
 
-                comt = ('Permissions for user foo is set to be changed')
-                ret.update({'comment': comt})
+                comment = '\'foo\' is already in the desired state.'
+                ret.update({'changes': {}, 'comment': comment, 'result': True})
                 self.assertDictEqual(rabbitmq_user.present(name, perms=perms),
                                      ret)
 
             with patch.dict(rabbitmq_user.__opts__, {'test': False}):
-                ret.update({'comment': name, 'result': True,
-                            'changes': {'new': 'Set tags: user\n', 'old': ''}})
+                ret.update({'comment': '\'foo\' was configured.', 'result': True,
+                            'changes': {'tags': {'new': set(['e', 'r', 's', 'u']), 'old': 'user'}}})
                 self.assertDictEqual(rabbitmq_user.present(name, tags=tag), ret)
 
     # 'absent' function tests: 1
@@ -101,7 +103,7 @@ class RabbitmqUserTestCase(TestCase):
         ret = {'name': name,
                'changes': {},
                'result': True,
-               'comment': 'User {0} is not present'.format(name)}
+               'comment': 'The user \'foo\' is not present.'}
 
         mock = MagicMock(return_value=False)
         with patch.dict(rabbitmq_user.__salt__, {'rabbitmq.user_exists': mock}):