Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mongodb_replicaset and mongodb_shard: Use error code instead of error text #55639

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@kongslund
Copy link
Contributor

commented Apr 23, 2019

SUMMARY

Use error code instead of error text when deciding on if a login is needed.

This fixes an issue where listDatabases() failed with the error "pymongo.errors.OperationFailure:
there are no users authenticated" instead of trying to authenticate. Using the error code for
"unauthorized" instead of analysing the error text is a more robust approach. All MongoDB error
codes are documented at https://github.com/mongodb/mongo/blob/master/src/mongo/base/error_codes.err

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • mongodb_replicaset
  • mongodb_shard
ADDITIONAL INFORMATION

I have an existing MongoDB replica set with 3 instances and authentication enabled.

---
- hosts: localhost
  gather_facts: no
  tasks:
    - mongodb_replicaset:
        login_user: admin
        login_password: 123456
        login_database: admin
        login_host: 192.168.1.1
        login_port: 30001
        replica_set: my-rs
        arbiter_at_index: 2
        members:
          - db1:27017
          - db2:27017
          - db-arb:27017

Task output before:

TASK [mongodb_replicaset] ****************************************************************************************************************************************
fatal: [localhost]: FAILED! => changed=false
  module_stderr: |-
    /Users/jonas/.ansible/tmp/ansible-tmp-1556004238.142287-29947431643461/AnsiballZ_mongodb_replicaset.py:18: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
      import imp
    Traceback (most recent call last):
      File "/Users/jonas/.ansible/tmp/ansible-tmp-1556004238.142287-29947431643461/AnsiballZ_mongodb_replicaset.py", line 114, in <module>
        _ansiballz_main()
      File "/Users/jonas/.ansible/tmp/ansible-tmp-1556004238.142287-29947431643461/AnsiballZ_mongodb_replicaset.py", line 106, in _ansiballz_main
        invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
      File "/Users/jonas/.ansible/tmp/ansible-tmp-1556004238.142287-29947431643461/AnsiballZ_mongodb_replicaset.py", line 49, in invoke_module
        imp.load_module('__main__', mod, module, MOD_DESC)
      File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/imp.py", line 234, in load_module
        return load_source(name, filename, file)
      File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/imp.py", line 169, in load_source
        module = _exec(spec, sys.modules[name])
      File "<frozen importlib._bootstrap>", line 630, in _exec
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/var/folders/kc/5lwqpft17g596cb93br8zs940000gp/T/ansible_mongodb_replicaset_payload_fj0wte9w/__main__.py", line 417, in <module>
      File "/var/folders/kc/5lwqpft17g596cb93br8zs940000gp/T/ansible_mongodb_replicaset_payload_fj0wte9w/__main__.py", line 385, in main
      File "/var/folders/kc/5lwqpft17g596cb93br8zs940000gp/T/ansible_mongodb_replicaset_payload_fj0wte9w/__main__.py", line 377, in main
      File "/Users/jonas/git/github/ansible/venv/lib/python3.7/site-packages/pymongo/database.py", line 658, in command
        codec_options, session=session, **kwargs)
      File "/Users/jonas/git/github/ansible/venv/lib/python3.7/site-packages/pymongo/database.py", line 555, in _command
        client=self.__client)
      File "/Users/jonas/git/github/ansible/venv/lib/python3.7/site-packages/pymongo/pool.py", line 584, in command
        user_fields=user_fields)
      File "/Users/jonas/git/github/ansible/venv/lib/python3.7/site-packages/pymongo/network.py", line 158, in command
        parse_write_concern_error=parse_write_concern_error)
      File "/Users/jonas/git/github/ansible/venv/lib/python3.7/site-packages/pymongo/helpers.py", line 155, in _check_command_response
        raise OperationFailure(msg % errmsg, code, response)
    pymongo.errors.OperationFailure: there are no users authenticated
  module_stdout: ''
  msg: |-
    MODULE FAILURE
    See stdout/stderr for the exact error
  rc: 1

Output after the fix was applied:

TASK [mongodb_replicaset] ****************************************************************************************************************************************
ok: [localhost]
Jonas Kongslund
mongodb_replicaset and mongodb_shard: Use error code instead of error…
… text to determine if a login is needed

This fixes an issue where listDatabases() failed with the error "pymongo.errors.OperationFailure:
there are no users authenticated" instead of trying to authenticate. Using the error code for
"unauthorized" instead of analysing the error text is a more robust approach. All MongoDB error
codes are documented at https://github.com/mongodb/mongo/blob/master/src/mongo/base/error_codes.err
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@rhysmeister

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Hi @kongslund

  1. I agree with the use of the error code but I think the flow should be different. I've already had a PR merged where I changed this. Please see here for full details #53605 Perhaps you can repurpose this PR to avoid wiping that out. This code was in part inherited from the mongodb_user/parameter modules. Have a go there too if you're keen.

  2. What version of MongoDB were you running against? I have written some Integration tests that should cover the situation you describe. See here #53900 Either there's something not right with the integration tests or something else is at play.

  3. It's probably not a bad idea to verify the final module pr with versions 3.0+ of MongoDB with the integration tests mentioned above.

Cheers,

Rhys

@ansibot ansibot removed the needs_triage label Apr 24, 2019

@kongslund

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Hi @rhysmeister, thanks for the feedback.

Re 1. I have no strong opinion regarding flow. I'll take a look at it.

Re 2. I'm using the official Docker image mongo:3.6.10. The setup is with 3 instances, of which one is an arbiter. The replica set runs with auth and TLS.

Re 3. I'm new to Ansible's integration test framework. How are you running these integration tests? I managed to run a test for ping using the default Docker test container image but that doesn't work for e.g. mongodb_replicaset.

test/runner/ansible-test integration --docker default -v ping
test/runner/ansible-test integration --docker default -v mongodb_replicaset
@rhysmeister

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

  1. I think it should be...

try: client['admin'].command('listDatabases', 1.0) # if this throws an error we need to authenticate except Exception as excep: if excep.code == MONGO_ERRCODE_UNAUTHORIZED: if login_user is not None and login_password is not None: client.admin.authenticate(login_user, login_password, source=login_database) else: raise excep else: raise excep

That seems to be a flow that make more sense to me. Either way both modules should use the same logic. Perhaps it also makes sense to provide a more friendly excep message if login details are None.

  1. Mmmmm, I'll take a look at that when I get time (probably next week)
  2. I'd reference an old PR there (updated). The correct one is #53900. These tests are not yet merged into the main ansible devel branch. You need to clone my repo and switch to the mongodb_init2 branch.

@ansibot ansibot added the stale_ci label May 3, 2019

@kongslund

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Re 1. How would the expression if excep.code == MONGO_ERRCODE_UNAUTHORIZED behave if the exception is not of type OperationFailure? As far as I can see in the pymongo documentation, it is the only exception type that has a code field containing an error code. Aren't we interested in only catching that particular exception?

Re 3. Tried it out just now. Works great!

@rhysmeister

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

@kongslund Good point. Depending on the flow it might need a "if defined". Best thing to do will be to add a few tests for this. I've added a few to the mongodb_replicaset tests

Do a git pull on the mongodb_init2 branch to get them.

  • name: Test with bad password
  • name: Run test with unknown host

I think it's best here to catch that single exception and let the rest fall through.

@ansibot ansibot removed the stale_ci label May 9, 2019

@kongslund

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I've updated the flow. Did I understand you correctly?

@rhysmeister

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

I think that's an improvement. We should should be using module.fail_json instead of raise excep. There'll also be an uncaught exception if bad authentication details are provided. This could be handled. I could add tests for these situations if you want to take that on too.

@kongslund

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

That's some good ideas for improving the quality of the code. Perhaps we could do those changes in a different PR? I'm hoping to keep this one minimal and focused on fixing the issue described at the beginning.

@rhysmeister

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

These are just suggestions from me. I am not in charge. That said they should be pretty easy to do. We're talking about a couple of module.fail_json outputs and another try catch. Why leave an obvious fault? It's your PR though you are free to proceed as you wish.

Cheers,

R

@ansibot ansibot added the stale_ci label May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.