Now influxdb_user module can change user password

[ar7z1]

SUMMARY

Previous implementation didn't allow to change user password.

Test to reproduce the problem:

- name: Add user
  influxdb_user: user_name=user user_password=password1 login_username=admin login_password=admin

- name: Change user password
  influxdb_user: user_name=user user_password=password2 login_username=admin login_password=admin
  register: change_password

- name: Check that password changing succeeds with a change
  assert:
    that:
      - change_password.changed == true

In this PR I've added some code to check if we should change user password. The only way to check is try to authenticate. And if authentication failed with error 401 (Unauthorized), we should change user password.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

influxdb_user

ANSIBLE VERSION

ansible 2.4.2.0
  config file = /Users/art/.ansible.cfg
  configured module search path = [u'/Users/art/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/Cellar/ansible/2.4.2.0_2/libexec/lib/python2.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.10 (default, Jul 15 2017, 17:16:57) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.31)]

ADDITIONAL INFORMATION

[ansibot]

cc @kamsz @zhhuta
click here for bot help

[ansibot]

The test ansible-test sanity --test import --python 2.6 [?] failed with the following error:

lib/ansible/modules/database/influxdb/influxdb_user.py:85:0: ImportError: No module named influxdb.exceptions

The test ansible-test sanity --test import --python 2.7 [?] failed with the following error:

lib/ansible/modules/database/influxdb/influxdb_user.py:85:0: ImportError: No module named influxdb.exceptions

The test ansible-test sanity --test import --python 3.5 [?] failed with the following error:

lib/ansible/modules/database/influxdb/influxdb_user.py:85:0: ImportError: No module named 'influxdb'

The test ansible-test sanity --test import --python 3.6 [?] failed with the following error:

lib/ansible/modules/database/influxdb/influxdb_user.py:85:0: ModuleNotFoundError: No module named 'influxdb'

The test ansible-test sanity --test import --python 3.7 [?] failed with the following error:

lib/ansible/modules/database/influxdb/influxdb_user.py:85:0: ModuleNotFoundError: No module named 'influxdb'

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/modules/database/influxdb/influxdb_user.py:87:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/database/influxdb/influxdb_user.py:100:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/database/influxdb/influxdb_user.py:113:1: E302 expected 2 blank lines, found 1

The test ansible-test sanity --test validate-modules [?] failed with the following error:

lib/ansible/modules/database/influxdb/influxdb_user.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'No module named 'influxdb''

click here for bot help

[ar7z1]

switch_user function just sets two properties, it doesn't call server:

self._username = username
self._password = password

We should execute some command on server, that's why we call get_list_users.

[ar7z1]

I've found that there are no tests to check user removal. So I've added them. And I can move them into separate PR if needed. :-)

[Akasurde]

Integration tests are always welcomed. I think we can keep it here unless there is a specific reason not to.

[ansibot]

The test ansible-test sanity --test pep8 [?] failed with the following error:

lib/ansible/modules/database/influxdb/influxdb_user.py:92:1: E302 expected 2 blank lines, found 1

click here for bot help

[zhhuta]

Please try to keep code clean and don't duplicate it.

[zhhuta]

this block is already in module_utils i don't think it is necessary to duplicate code.
86..91 take a look on module_utils/influxdb.py

[ar7z1]

@zhhuta, Thank you for the review!

As you see, I need to catch InfluxDBClientError in the check_user_password function.
This error defined in the influxdb-python module. That's why I need to add import line:

from influxdb.exceptions import InfluxDBClientError

If I add only the import line, then test ansible-test sanity --test import --python XX will fail with the error:

lib/ansible/modules/database/influxdb/influxdb_user.py:85:0: ImportError: No module named influxdb..exceptions

I've already tried. :-)

It is because of this check:

All Python imports in lib/ansible/modules/ and lib/ansible/module_utils/ which are not from the Python standard library must be imported in a try/except ImportError block.

That's why I'd copied try...except from module_utils/influxdb.py:

try:
    from influxdb.exceptions import InfluxDBClientError
    HAS_INFLUXDB = True
except ImportError:
    HAS_INFLUXDB = False

I think I can replace this block with

try:
    from influxdb.exceptions import InfluxDBClientError
except ImportError:
    pass

And then I can drop:

if not HAS_INFLUXDB:
    module.fail_json(msg='This module requires influxdb python package.')

What do you think?

[zhhuta]

Lets keep logic striate: you want set password if user already exist:

        if user:
            if check_user_password(module, user_name, user_password):
                module.exit_json(changed=False)
            else:
                set_user_password(module, client, user_name, user_password)
        else:
            create_user(module, client, user_name, user_password, admin)

it's ok but lets look on logic above, you already have

  client = influxdb.connect_to_influxdb()

and when you call check_user_password you create another connection to db inside of this function.

    influxdb = InfluxDb(module)
    client = influxdb.connect_to_influxdb()

Obviously you can pass to this function client object that was init in main()
Next

        client.switch_user(user_name, user_password)
        client.get_list_users()
    except InfluxDBClientError as e:
        if e.code == 401:
            return False

why do you useclient.get_list_users()? it returns a list of users : actually a list-object. I guess you can keep only client.switch_user(user_name, user_password)

Another is about InfluxDBClientError its already part of InfluxDBClient that we import from influxdb in module_utils/influxdb.py and if you go deeper in influxdb-python/influxdb/client.py
Any way let me try to work with your code and figure out how we can make this code more clean

[zhhuta]

take a look on drop_user()

 try:
            client.drop_user(user_name)
        except client.InfluxDBClientError as e:
            module.fail_json(msg=e.content)

[ar7z1]

1. Import

   take a look on drop_user()

   try:
       client.drop_user(user_name)
   except client.InfluxDBClientError as e:
       module.fail_json(msg=e.content)

   @zhhuta Are you sure that this code works? I'm not a python expert, sorry if I say something stupid.
   I've tried to cause InfluxDBClientError in drop_user. I've replaced it by:

   def drop_user(module, client, user_name):
       if not module.check_mode:
           try:
               client.switch_user('test', 'test')
               client.drop_user(user_name)
           except client.InfluxDBClientError as e:
               module.fail_json(msg=e.content)
   
       module.exit_json(changed=True)

   As you can see, I've added client.switch_user('test', 'test'). This line always causes InfluxDBClientError with code 401 (Unauthorized) (there is no 'test' user in my InfluxDB).
   Then I've run the following test:

   - name: Add user
     influxdb_user: user_name=user user_password=password login_username=admin login_password=admin
   
   - name: Remove user
     influxdb_user: user_name=user state=absent login_username=admin login_password=admin

   It failed with the error AttributeError: 'InfluxDBClient' object has no attribute 'InfluxDBClientError':

   An exception occurred during task execution. To see the full traceback, use -vvv. The error was: AttributeError: 'InfluxDBClient' object has no attribute 'InfluxDBClientError'
   fatal: [testhost]: FAILED! => {"changed": false, "failed": true, "module_stderr": "Traceback (most recent call last):\n  File \"/tmp/ansible_sSZKDE/ansible_module_influxdb_user.py\", line 195, in <module>\n    main()\n  File \"/tmp/ansible_sSZKDE/ansible_module_influxdb_user.py\", line 189, in main\n    drop_user(module, client, user_name)\n  File \"/tmp/ansible_sSZKDE/ansible_module_influxdb_user.py\", line 147, in drop_user\n    except client.InfluxDBClientError as e:\nAttributeError: 'InfluxDBClient' object has no attribute 'InfluxDBClientError'\n", "module_stdout": "", "msg": "MODULE FAILURE", "rc": 1}
   

   When I've added import:

   from influxdb.exceptions import InfluxDBClientError
   

   and replaced client.InfluxDBClientError to InfluxDBClientError in drop_user function:

   def drop_user(module, client, user_name):
       if not module.check_mode:
           try:
               client.switch_user('test', 'test')
               client.drop_user(user_name)
           except InfluxDBClientError as e:
               module.fail_json(msg=e.content)
   
       module.exit_json(changed=True)

   the test have failed as expected:

   fatal: [testhost]: FAILED! => {"changed": false, "failed": true, "msg": "{\"error\":\"authorization failed\"}\n"}
   

   That's why I think that we need to explicitly add import.

2. Pass client from main

   and when you call check_user_password you create another connection to db inside of this function.

   influxdb = InfluxDb(module)
   client = influxdb.connect_to_influxdb()
   

   Obviously you can pass to this function client object that was init in main()

   As you know, there are two users in the module:

   1. username + password (with aliases login_username + login_password). This user is used to authenticate against InfluxDB server.
   2. user_name + user_password. This is user to modify.

   When main calls your version of check_user_password:

   def check_user_password(module, client, user_name, user_password):
       try:
           client.switch_user(user_name, user_password)
           client.get_list_users()
       except client.InfluxDBClientError as e:
           if e.code == 401:
               return False
       except ansible.module_utils.urls.ConnectionError as e:
           module.fail_json(msg=str(e))
       return True

   it replaces username and password (user for authentication) in client by user_name and user_password (user to modify) and never rollbacks. Thus if check_user_password returns False, set_user_password(module, client, user_name, user_password) call will never succeed.

   But I understand your concern. What do you think if I change check_user_password by this:

   def check_user_password(module, client, user_name, user_password):
       try:
           client.switch_user(user_name, user_password)
           client.get_list_users()
       except InfluxDBClientError as e:
           if e.code == 401:
               return False
       except ansible.module_utils.urls.ConnectionError as e:
           module.fail_json(msg=str(e))
       finally:
           # restore previous user
           client.switch_user(module.params['username'], module.params['password'])
       return True

3. Use only switch_user call

   why do you use client.get_list_users()? it returns a list of users : actually a list-object. I guess you can keep only client.switch_user(user_name, user_password)

   As I've already written, switch_user function just sets two properties, it doesn't call server:

   def switch_user(self, username, password):
       """Change the client's username.
       :param username: the username to switch to
       :type username: str
       :param password: the password for the username
       :type password: str
       """
       self._username = username
       self._password = password

   get_list_users is just a random function which calls server.

[zhhuta]

Hi Artem.
I miss switch user logic in influxdb/client.py - you are right and i got the logic - i agree on suggested change in 2 and it make sense to keep switch_user.
about InfluxDBClientError i guess i need to dive more deep and get understanding thy dependency doesn't work as it import in influxdb/client.py

from .exceptions import InfluxDBClientError
from .exceptions import InfluxDBServerError```

[zhhuta]

Actually i've figured out and code need to be refactored in next way

from ansible.module_utils.influxdb import *

so you can use exceptions.InfluxDBClientError
but it's not really nice way of import
it's another way
import ansible.module_utils.influxdb as influxdb
so it will bring next changes:
argument_spec = influxdb.InfluxDb.influxdb_argument_spec()

influxdb = inflxudb.InfluxDb(module)

except inflxudb.exceptions.InfluxDBClientError
such refactoring reduce reimport but bring some hard to understand call.

[zhhuta]

same thing about code duplication.
164..166 check module_utils/influxdb.py

[zhhuta]

please take a look on refactored file
https://gist.github.com/zhhuta/7cb08f58678375f2bb7f311cade36d92

[zhhuta]

I've refactored code and tested it
you may include my changes in this commit.
https://gist.github.com/zhhuta/58c3276eb13c923677c8e644414a0501

[ar7z1]

@zhhuta Great work, thank you! I've added your changes in 59c0ac9.

[ansibot]

@ar7z1 This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

[ar7z1]

@resmo Can you look at this PR?

[resmo]

shipit