Mdd psql user aws fix #23988

michael-dev2rights · 2017-04-25T22:15:40Z

SUMMARY

This is an attempt to fix Ansible Bug #18933 - make postgresql user creation work on Amazon RDS instances. It has worked on our internal branch based off Ansible stable 2.3 and has been cherry picked to devel for upstream inclusion into the project.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

posgresql_user module

ANSIBLE VERSION

  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/mikedd/dev/ansible/ansible-add-modules/library']
  ansible python module location = /home/mikedd/dev/ansible/ansible-add-modules/lib/ansible
  executable location = /home/mikedd/dev/ansible/ansible-add-modules/bin/ansible
  python version = 2.7.12 (default, Jul  1 2016, 15:12:24) [GCC 5.4.0 20160609]

ADDITIONAL INFORMATION

Not yet nearly fully tested.

ansibot · 2017-04-25T22:25:28Z

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

test/sanity/pep8/legacy-files.txt:406:1: A201 Remove "lib/ansible/modules/database/postgresql/postgresql_user.py" since it passes the current rule set

click here for bot help

ansibot · 2017-04-25T22:25:28Z

cc @nerzhul
click here for bot help

gundalow · 2017-04-26T11:47:33Z

You wrote:

Not yet nearly fully tested.
What other testing needs to be done?

michael-dev2rights · 2017-04-26T21:42:39Z

@gundalow

someone should run the existing integration tests; probably after ensuring that they include user creation by this module and database changes via another module using that user.
preferably, someone should write an integration test case which

creates an Amazon RDS with a database set up on it;
creates a user on that database via this module
does something to the database
changes the user's password
does something to the database with the new password (also fails with old??)
repeats the operation that changed the password with the same password (expected state on RDS is changed even though password won't actualy have changed)
does something to the database

michael-dev2rights · 2017-04-26T21:43:45Z

Beyond that it would be nice to have unit test cases but the refactoring I started should be continued with extreme prejudice, including converting all SQL string statements to prepared statements.

nerzhul · 2017-04-27T08:13:29Z

Hello,
Warning if you refactor i have a pending PR on the module which adds IN_ROLE support (i use it at work to create nested developers groups on schemas)

nerzhul · 2017-04-27T08:14:15Z

lib/ansible/modules/database/postgresql/postgresql_user.py

+            # different
+            return True
+
+    if password == current_role_attrs['rolpassword']:


just return password != current_role_attrs['rolpassword']

nerzhul · 2017-04-27T08:14:27Z

lib/ansible/modules/database/postgresql/postgresql_user.py

+    if encrypted == 'ENCRYPTED' and not password.startswith('md5'):
+        try:
+            from passlib.hash import postgres_md5 as pm
+            if pm.encrypt(password, user) == current_role_attrs['rolpassword']:


just return pm.encrypt(password, user) != current_role_attrs['rolpassword']:

nerzhul · 2017-04-27T08:14:39Z

lib/ansible/modules/database/postgresql/postgresql_user.py

+
+    if encrypted == 'ENCRYPTED' and not password.startswith('md5'):
+        try:
+            from passlib.hash import postgres_md5 as pm


please don't import here, import globally

nerzhul · 2017-04-27T08:14:50Z

lib/ansible/modules/database/postgresql/postgresql_user.py

+    # Do we actually need to do anything?
+
+    if encrypted == 'ENCRYPTED' and password.startswith('md5'):
+        if password == current_role_attrs['rolpassword']:


same as others, return directly test

pilou-

Please, update the description of 'no_password_changes'.

pilou- · 2017-04-27T10:43:13Z

lib/ansible/modules/database/postgresql/postgresql_user.py

+    # apparently port is automagically handled below;  comment out to relieve
+    # flake8 error; delete when tests verify
+    #
+    # port = module.params["port"]


Remove the whole unused line (and the comments).

ansibot · 2017-06-13T14:15:13Z

cc @matburt
click here for bot help

ansibot · 2017-06-15T11:19:03Z

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

lib/ansible/modules/database/postgresql/postgresql_user.py:241:1: E303 too many blank lines (4)
lib/ansible/modules/database/postgresql/postgresql_user.py:676:1: E303 too many blank lines (3)

click here for bot help

abadger · 2017-06-30T00:19:54Z

lib/ansible/modules/database/postgresql/postgresql_user.py

+except ImportError:
+    passlib_hash_found = False
+else:
+    passlib_hash_found = True


Just a couple style note, usually we import from stdlib, then from 3rd party libs, and then from ansible libs.

For setting whether passlib is found, you can do it like this to save a line:

try: from passlib.hash import postgres_md5 as pm passlib_hash_found = True except ImportError: passlib_hash_found = False

Since passlib_hash_found is a toplevel constant, style-wise it should be all uppercase:
PASSLIB_HASH_FOUND

abadger · 2017-06-30T00:24:56Z

I found some style things but nothing important.

@nerzhul and @pilou- Does this change look good to you now? I'll merge when you think it's ready.

Also I see that #22613 currently needs to be rebased. Perhaps after this goes in, @nerzhul would rebase that and then @michael-dev2rights would care to review it. And then, when it's ready, one of you can ping me to merge that one too?

pilou- · 2017-07-05T19:09:00Z

@abadger I discussed about this PR with @michael-dev2rights during ansible fest (see), there is a problem (due to a complex rebase): passlib.hash import postgres_md5 must not be reintroduced, passlib.hash import postgres_md5 is sufficient.

pilou- · 2017-07-06T02:24:13Z

I added 2 commits (on a branch based on this one):

the first commit (pilou-@67193cb) adds a test, I checked the test fails on devel and succeed on this branch
the second one (pilou-@ec27231) contains some fixes

@abadger could you update this pull-request adding these two commits and then merge ?

…_authid

…om failed select

…according to suggestions from PR review

TASK [postgresql : Normal user isn't allowed to access pg_authid relation: password comparison will fail, password will be updated] *** An exception occurred during task execution. To see the full traceback, use -vvv. The error was: psycopg2.ProgrammingError: permission denied for relation pg_authid

michael-dev2rights · 2017-07-07T11:12:33Z

@pilou- I merged your changes into this branch. Thanks.

@abadger I think we should merge this PR soon since it's a proper bug fix and now includes useful tests.

@nerzhul any objections?

abadger · 2017-07-07T16:29:13Z

I've merged to devel (for 2.4.0). If @nerzhul pokes me on IRC we can decide whether to backport to 2.3.x as well.

michael-dev2rights mentioned this pull request Apr 25, 2017

postgres_user module doesn't work with AWS RDS databases #18933

Closed

ansibot added test_pull_requests and removed ci_verified Changes made in this PR are causing tests to fail. labels Apr 25, 2017

michael-dev2rights force-pushed the mdd-psql-user-aws-fix branch from a4e8099 to c55cf73 Compare April 25, 2017 23:00

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 25, 2017

gundalow removed the needs_triage Needs a first human triage before being processed. label Apr 26, 2017

nerzhul suggested changes Apr 27, 2017

View reviewed changes

pilou- reviewed Apr 27, 2017

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 27, 2017

ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label May 11, 2017

michael-dev2rights force-pushed the mdd-psql-user-aws-fix branch from 6274d49 to 6bd6e5f Compare June 13, 2017 13:29

michael-dev2rights force-pushed the mdd-psql-user-aws-fix branch from 6bd6e5f to 1c60713 Compare June 15, 2017 10:30

ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 15, 2017

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jun 15, 2017

ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Jun 29, 2017

abadger reviewed Jun 30, 2017

View reviewed changes

ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jul 3, 2017

mikedlr and others added 9 commits July 7, 2017 11:53

postgresql_user - PEP8 and style fixes to make debugging easier

355cff9

postgresql_user - move password changing logic to separate function

0039859

postgresql_user - trap failure in case where there is no access to pg…

450e606

…_authid

postgresql_user module - transaction logic hacks to allow recovery fr…

9e28589

…om failed select

postgresql_user - further PEP8 fixes

0492abe

postgresql_user - Simplify password change logic and improve imports …

ca05598

…according to suggestions from PR review

postgresql_user - Eliminate pep8/blank line errors introduced in merge

211ee55

Don't reintroduce passlib, remove useless query

103b0d9

michael-dev2rights force-pushed the mdd-psql-user-aws-fix branch from 641a32a to 103b0d9 Compare July 7, 2017 10:55

abadger merged commit 3c4db1e into ansible:devel Jul 7, 2017

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018

dagwieers added the postgresql PostgreSQL community label Jan 28, 2019

dagwieers added the database Database category label Feb 13, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mdd psql user aws fix #23988

Mdd psql user aws fix #23988

michael-dev2rights commented Apr 25, 2017

ansibot commented Apr 25, 2017

ansibot commented Apr 25, 2017

gundalow commented Apr 26, 2017

michael-dev2rights commented Apr 26, 2017

michael-dev2rights commented Apr 26, 2017

nerzhul commented Apr 27, 2017

nerzhul Apr 27, 2017

nerzhul Apr 27, 2017

nerzhul Apr 27, 2017

nerzhul Apr 27, 2017

pilou- left a comment

pilou- Apr 27, 2017

ansibot commented Jun 13, 2017

ansibot commented Jun 15, 2017

abadger Jun 30, 2017

abadger commented Jun 30, 2017

pilou- commented Jul 5, 2017 •

edited

Loading

pilou- commented Jul 6, 2017 •

edited

Loading

michael-dev2rights commented Jul 7, 2017

abadger commented Jul 7, 2017

Mdd psql user aws fix #23988

Mdd psql user aws fix #23988

Conversation

michael-dev2rights commented Apr 25, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Apr 25, 2017

ansibot commented Apr 25, 2017

gundalow commented Apr 26, 2017

michael-dev2rights commented Apr 26, 2017

michael-dev2rights commented Apr 26, 2017

nerzhul commented Apr 27, 2017

nerzhul Apr 27, 2017

Choose a reason for hiding this comment

nerzhul Apr 27, 2017

Choose a reason for hiding this comment

nerzhul Apr 27, 2017

Choose a reason for hiding this comment

nerzhul Apr 27, 2017

Choose a reason for hiding this comment

pilou- left a comment

Choose a reason for hiding this comment

pilou- Apr 27, 2017

Choose a reason for hiding this comment

ansibot commented Jun 13, 2017

ansibot commented Jun 15, 2017

abadger Jun 30, 2017

Choose a reason for hiding this comment

abadger commented Jun 30, 2017

pilou- commented Jul 5, 2017 • edited Loading

pilou- commented Jul 6, 2017 • edited Loading

michael-dev2rights commented Jul 7, 2017

abadger commented Jul 7, 2017

pilou- commented Jul 5, 2017 •

edited

Loading

pilou- commented Jul 6, 2017 •

edited

Loading