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

feat: reduce user interaction in quicksetup #5768

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 69 additions & 35 deletions aiida/manage/external/postgres.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
"""
from typing import TYPE_CHECKING

import click
from pgsu import DEFAULT_DSN as DEFAULT_DBINFO # pylint: disable=no-name-in-module
from pgsu import PGSU, PostgresConnectionMode

Expand Down Expand Up @@ -120,22 +119,44 @@ def drop_dbuser(self, dbuser):
"""
self.execute(_DROP_USER_COMMAND.format(dbuser))

def check_dbuser(self, dbuser):
"""Looks up if a given user already exists, prompts for using or creating a differently named one.
def find_new_dbuser(self, start_from='aiida'):
"""Find database user that does not yet exist.

:param str dbuser: Name of the user to be created or reused.
:returns: tuple (dbuser, created)
Generates names of the form "aiida_1", "aiida_2", etc. until it finds a name that does not yet exist.

:param str start_from: start from this name
:returns: dbuser
"""
dbuser = start_from
i = 0
while self.dbuser_exists(dbuser):
i = i + 1
dbuser = f'{start_from}_{i}'

return dbuser

def can_user_authenticate(self, dbuser, dbpass):
"""Check whether the database user credentials are valid.

Checks whether dbuser has access to the `template1` postgres database
via psycopg2.

:param dbuser: the database user
:param dbpass: the database password
:return: True if the credentials are valid, False otherwise
"""
if not self.interactive:
return dbuser, not self.dbuser_exists(dbuser)
create = True
while create and self.dbuser_exists(dbuser):
echo.echo_warning(f'Database user "{dbuser}" already exists!')
if not click.confirm('Use it? '):
dbuser = click.prompt('New database user name: ', type=str, default=dbuser)
else:
create = False
return dbuser, create
from pgsu import _execute_psyco
import psycopg2
dsn = self.dsn.copy()
dsn['user'] = dbuser
dsn['password'] = dbpass

try:
_execute_psyco('SELECT 1', dsn)
except psycopg2.OperationalError:
return False

return True

### DB functions ###

Expand Down Expand Up @@ -168,35 +189,48 @@ def drop_db(self, dbname):
def copy_db(self, src_db, dest_db, dbuser):
self.execute(_COPY_DB_COMMAND.format(dest_db, src_db, dbuser))

def check_db(self, dbname):
"""Looks up if a database with the name exists, prompts for using or creating a differently named one.
def find_new_db(self, start_from='aiida'):
ltalirz marked this conversation as resolved.
Show resolved Hide resolved
"""Find database name that does not yet exist.

:param str dbname: Name of the database to be created or reused.
:returns: tuple (dbname, created)
Generates names of the form "aiida_1", "aiida_2", etc. until it finds a name that does not yet exist.

:param str start_from: start from this name
:returns: dbname
"""
if not self.interactive:
return dbname, not self.db_exists(dbname)
create = True
while create and self.db_exists(dbname):
echo.echo_warning(f'database {dbname} already exists!')
if not click.confirm('Use it (make sure it is not used by another profile)?'):
dbname = click.prompt('new name', type=str, default=dbname)
else:
create = False
return dbname, create
dbname = start_from
i = 0
while self.db_exists(dbname):
i = i + 1
dbname = f'{start_from}_{i}'

return dbname

def create_dbuser_db_safe(self, dbname, dbuser, dbpass):
"""Create DB and user + grant privileges.

Prompts when reusing existing users / databases.
An existing database user is reused, if it is able to authenticate.
If not, a new username is generated on the fly.

An existing database is not reused (unsafe), a new database name is generated on the fly.

:param str dbname: Name of the database.
:param str dbuser: Name of the user which should own the db.
:param str dbpass: Password the user should be given.
:returns: (dbuser, dbname)
"""
dbuser, create = self.check_dbuser(dbuser=dbuser)
if create:
if not self.dbuser_exists(dbuser):
self.create_dbuser(dbuser=dbuser, dbpass=dbpass)
elif not self.can_user_authenticate(dbuser, dbpass):
echo.echo_warning(f'Database user "{dbuser}" already exists but is unable to authenticate.')
dbuser = self.find_new_dbuser(dbuser)
self.create_dbuser(dbuser=dbuser, dbpass=dbpass)
echo.echo_info(f'Using database user "{dbuser}".')

dbname, create = self.check_db(dbname=dbname)
if create:
self.create_db(dbuser, dbname)
if self.db_exists(dbname):
echo.echo_warning(f'Database "{dbname}" already exists.')
dbname = self.find_new_db(dbname)
self.create_db(dbuser=dbuser, dbname=dbname)
echo.echo_info(f'Using database "{dbname}".')

return dbuser, dbname

Expand Down
18 changes: 18 additions & 0 deletions tests/manage/external/test_postgres.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ def test_create_drop_db_user(self):
postgres = self._setup_postgres()
postgres.create_dbuser(self.dbuser, self.dbpass)
self.assertTrue(postgres.dbuser_exists(self.dbuser))

self.assertTrue(postgres.can_user_authenticate(self.dbuser, self.dbpass))
self.assertFalse(postgres.can_user_authenticate('non-existent-db-user', self.dbpass))
# note: connection with wrong password may work, if postgres server is set to trust local connections
# self.assertFalse(postgres.can_user_connect(self.dbuser, 'wrong-password'))

postgres.drop_dbuser(self.dbuser)
self.assertFalse(postgres.dbuser_exists(self.dbuser))

Expand All @@ -60,3 +66,15 @@ def test_create_drop_db(self):
self.assertTrue(postgres.db_exists(self.dbname))
postgres.drop_db(self.dbname)
self.assertFalse(postgres.db_exists(self.dbname))

def test_create_dbuser_db_safe(self):
"""Test creating a database and user in a safe way"""
postgres = self._setup_postgres()
user1, db1 = postgres.create_dbuser_db_safe(self.dbname, self.dbuser, self.dbpass)
assert user1 == self.dbuser
assert db1 == self.dbname

# a second try should reuse the database user but create a new database
user2, db2 = postgres.create_dbuser_db_safe(self.dbname, self.dbuser, self.dbpass)
assert user2 == self.dbuser
assert db2 == f'{self.dbname}_1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add an additional case here where you pass an invalid password to see it creates a new user automatically? Or do we hit the same problem potentially where certain postgres servers trust local connections? Is this the case for our CI setup? If not, I would still add the test (as well as the one you commented out) just so we have more coverage. If it then fails when people run the tests locally and they have their postgres configured that way, that is ok I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I've thought about it but is it really worth adding a test here that may fail depending on the details of the test setup?

The only thing this test would add is to check that psycopg2 throws the same exception when the password is wrong as when the user name is wrong (wrong user name is already tested).
I've checked that manually, and it does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean though is that lines 221-224 of create_dbuser_db_safe are never hit in this test. In the first test, the user doesn't exist, and so the first conditional is hit and it is created. In the second test, the user exists and it can authenticate, so it goes straight to the database creation, skipping the block with self.find_new_user. So is that actually getting covered now by the tests? And my point here is that it is better to have that test covered in our CI and have it potentially fail if some developer runs the test locally and has a weird Postgres configuration. Anyway, aren't you testing this against a complete mock cluster provided by PGTest? So this should never fail, should it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PGTest currently hardcodes the trust authentication method, i.e. when using the temporary postgres cluster the test will always fail
https://github.com/jamesnunn/pgtest/blob/417f0d89e66a9395db694a96122e0d5ac0097d59/pgtest/pgtest.py#L506-L507

I will open an issue for this to remind ourselves, also related to #3762

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so that means adding the test I suggested will fail on GHA? If so, then I guess we can leave it for now, but it would be good to fix in the future so we can increase coverage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, see

POSTGRES_HOST_AUTH_METHOD: trust

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also opened jamesnunn/pgtest#24

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have a look here at what happens today if we require passwords for postgres
#5776