ansible-collections / community.rabbitmq

Manage RabbitMQ with Ansible
http://galaxy.ansible.com/community/rabbitmq
Other
31 stars 46 forks source link

rabbitmq_user doesn't support Ansible check mode #107

Open aderixon opened 2 years ago

aderixon commented 2 years ago
SUMMARY

When applied with ansible --check, the rabbitmq_user module always makes any required changes to the users anyway. This is unexpected behaviour and potentially disruptive to the RabbitMQ service.

ISSUE TYPE
COMPONENT NAME

rabbitmq_user

ANSIBLE VERSION
ansible 2.9.23
  config file = /home/test/src/working/ansible/ansible.cfg
  configured module search path = [u'/usr/share/ansible']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Nov 16 2020, 22:23:17) [GCC 4.8.5 20150623 (Red Hat 4.8.5-44)]
COLLECTION VERSION
Collection         Version
------------------ -------
community.rabbitmq 1.1.0  
CONFIGURATION
ANSIBLE_PIPELINING(/home/test/src/working/ansible/ansible.cfg) = True
DEFAULT_MODULE_PATH(/home/test/src/working/ansible/ansible.cfg) = [u'/usr/share/ansible']
HOST_KEY_CHECKING(/home/test/src/working/ansible/ansible.cfg) = False
TRANSFORM_INVALID_GROUP_CHARS(/home/test/src/working/ansible/ansible.cfg) = ignore
OS / ENVIRONMENT

CentOS 7.9

STEPS TO REPRODUCE

Create a new RabbitMQ user using the rabbitmq_user module, but apply the playbook using ansible-playbook --check, which should show what would happen.

- hosts: all
  become: true
  tasks:
  - name: configure rabbitmq users
    community.rabbitmq.rabbitmq_user:
      name: "test-user"
      password: "pebhebEk"
      configure_priv: "^$"
      read_priv: "^$"
      write_priv: "^$"
      state: "present"
$ ansible-playbook -i hosts rabbitmq-test.yml -l mqserver.test.com -K --check -v
$ ansible-playbook -i hosts rabbitmq-test.yml -l mqserver.test.com -K --check -v
EXPECTED RESULTS

Ansible output would consistently show 'changed: [mqserver.test.com] => {"changed": true, ...}' for task but test-user would not actually be created on RabbitMQ host.

ACTUAL RESULTS

test-user is created the first time playbook is applied with --check. A second run outputs 'ok: ... "changed": false', indicating that the user now exists.

ansible(master)] 1023$ ansible-playbook -i hosts rabbitmq-test.yml -l mqserver.test.com -K --check -v
Using /home/test/src/working/ansible/ansible.cfg as config file
BECOME password: 

PLAY [all] *********************************************************************

TASK [Gathering Facts] *********************************************************
ok: [mqserver.test.com]

TASK [configure rabbitmq users] ************************************************
changed: [mqserver.test.com] => {"changed": true, "state": "present", "user": "test-user"}

PLAY RECAP *********************************************************************
mqserver.test.com          : ok=2    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

ansible(master)] 1024$ ansible-playbook -i hosts rabbitmq-test.yml -l mqserver.test.com -K --check -v
Using /home/test/src/working/ansible/ansible.cfg as config file
BECOME password: 

PLAY [all] *********************************************************************

TASK [Gathering Facts] *********************************************************
ok: [mqserver.test.com]

TASK [configure rabbitmq users] ************************************************
ok: [mqserver.test.com] => {"changed": false, "state": "present", "user": "test-user"}

PLAY RECAP *********************************************************************
mqserver.test.com          : ok=2    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

[root@mqserver test]# rabbitmqctl list_users | grep test-user
test-user   []

This is because the code for rabbitmq_user.py does not include support for running in check mode (e.g. compare implementation of _exec() with rabbitmq_vhost.py, which explicitly tests for check_mode being enabled and does not execute rabbitmqctl if so). I have a modified rabbitmq_user implementation that includes check_mode support (based on rabbitmq_vhost.py) if you want a PR.

movergan commented 2 years ago

Any update on that? It's quite a critical bug, one testing user password change can bring production down without knowing it.

csmart commented 2 years ago

Hi @movergan thanks for the report, I will try to have a look into it this week and provide an update.

aderixon commented 2 years ago

rabbitmq_user.py.txt

Attaching my patched version of this module here in case it's useful for anyone else who needs a workaround for now. (Caveat emptor, etc.)

lukasjuhrich commented 2 years ago

Any update on that? It's quite a critical bug, one testing user password change can bring production down without knowing it.

Agreed, this is what took me out of my sunday afternoon. But these things happen.

As it stands, the „smaller“ bug that we're having here is the fact that rabbitmq_user declares supports_check_mode=True, which at the moment is not true: https://github.com/ansible-collections/community.rabbitmq/blob/74f479f855e4dc66faecc4327d8cca01f853faf8/plugins/modules/rabbitmq_user.py#L466-L469

So setting this to False would be helpful already.

As a side note, to fix this issue „properly“, i.e. “supporting check mode“, can mean two different things:

  1. Checking for the presence of the users in the proper configuration
  2. On top of that, checking that authentication with the desired password is possible. This requires a decision beforehand.
lukasjuhrich commented 2 years ago
2. On top of that, checking that authentication with the desired password is possible.
   This requires a decision beforehand.

I just learned about the update_password option, which one would have to take into account.

lukasjuhrich commented 2 years ago

Hi @movergan thanks for the report, I will try to have a look into it this week and provide an update.

@csmart I hope this doesn't come across as impatient, but if time is an issue, would it be possible to set supports_check_mode=False in the meantime? It would be a trivial change but already very helpful.

csmart commented 2 years ago

@lukasjuhrich not at all, you're right. This fell off my radar, so thanks for the reminder.

csmart commented 2 years ago

This has been merged to main, I will arrange a new release soon.

csmart commented 2 years ago

I've released 1.2.2 which should be available on galaxy soon.