-
Notifications
You must be signed in to change notification settings - Fork 27
[DPE-7500] Create catalog/database level roles #921
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
Conversation
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 16/edge #921 +/- ##
========================================
Coverage 70.23% 70.23%
========================================
Files 16 16
Lines 3723 3723
Branches 541 541
========================================
Hits 2615 2615
Misses 974 974
Partials 134 134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…oles' into feature/16_predefined_catalog_roles Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…oles' into feature/16_predefined_dba_role Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…into feature/16_predefined_roles_cleanup Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…nup' into feature/16_predefined_catalog_roles Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…ned_dba_role Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…into feature/16_predefined_roles_cleanup Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…nup' into feature/16_predefined_catalog_roles Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…ned_dba_role Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…into feature/16_predefined_roles_cleanup Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…nup' into feature/16_predefined_catalog_roles Signed-off-by: Marcelo Henrique Neppel <[email protected]>
connection.autocommit = True | ||
|
||
with connection.cursor() as cursor: | ||
cursor.execute("RESET ROLE;") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed because the relation user is now auto escalated to the {database-name}_owner user
, which is not part of the charmed_read
role (only the relation user is).
|
||
|
||
@pytest.mark.abort_on_fail | ||
async def test_deploy(ops_test: OpsTest, charm) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big logic from this function is mostly to allow retesting in the same model (by resetting to the roles from the beginning of the first test).
def relations(ops_test: OpsTest, provider_app: str, requirer_app: str) -> list: | ||
return [ | ||
relation | ||
for relation in ops_test.model.applications[provider_app].relations | ||
if not relation.is_peer and relation.requires.application_name == requirer_app | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way to check if the relation still exists (and avoid situations where we removed it and it's still alive when trying to re-relate).
|
||
try: | ||
self.postgresql.create_predefined_roles() | ||
self.postgresql.create_predefined_instance_roles() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just renaming to be clear on which types of predefined roles we are creating at this moment (i.e. the ones introduced by #881).
logger.info(f"Resetting the user to the {username} user in the {instance}") | ||
cursor.execute("RESET ROLE;") | ||
check_connected_user(cursor, username, username, primary=read_write_endpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed because the relation user is now auto escalated to the {database-name}_owner
user, which is not part of the charmed_dba
role (only the relation user is).
self.charm.postgresql.create_database(database, plugins=plugins) | ||
|
||
password = new_password() | ||
self.charm.postgresql.create_user( | ||
user, password, extra_user_roles=extra_user_roles, in_role=f"{database}_admin" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of the calls was changed here because the catalog/database level roles are created along with the database, and we need to pass the {database-name}_admin
to the user creation method.
if connection is not None: | ||
connection.close() | ||
|
||
def _generate_database_privileges_statements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, the {database-name}_owner
owns all the objects in the database.
f"CREATE ROLE {ROLE_DBA} NOSUPERUSER CREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;", | ||
f"GRANT execute ON FUNCTION set_user(text) TO {ROLE_DBA};", | ||
f"GRANT execute ON FUNCTION set_user(text, text) TO {ROLE_DBA};", | ||
f"GRANT execute ON FUNCTION set_user_u(text) TO {ROLE_DBA};" | ||
f"GRANT execute ON FUNCTION reset_user() TO {ROLE_DBA};" | ||
f"GRANT execute ON FUNCTION reset_user(text) TO {ROLE_DBA};" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the set_up_predefined_catalog_roles
database function, so the right permissions are set to the charmed_dba
role in every database.
IF current_session_user LIKE 'relation-%' OR current_session_user LIKE 'relation_id_%' THEN | ||
RAISE NOTICE 'Granting % to %', admin_user, current_session_user; | ||
statement := 'GRANT ' || admin_user || ' TO "' || current_session_user || '";'; | ||
EXECUTE statement; | ||
END IF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the code is used to give the {database-name}_admin
role to the relation user, so they can escalate to the {database-name}_owner
user in a database that was created by themselves (through the charmed_databases_owner
extra user role).
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…into feature/16_predefined_roles_cleanup Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…nup' into feature/16_predefined_catalog_roles Signed-off-by: Marcelo Henrique Neppel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is LGTM, but the PostgreSQL roles itself requires extensive testing... I am not filling 100% confident in complex deployments (async, pgbouncer, etc).
Open for testing in 16/edge, LGTM for merging. Thank you for this incredible work done!!!
…ned_roles_cleanup Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…nup' into feature/16_predefined_catalog_roles Signed-off-by: Marcelo Henrique Neppel <[email protected]>
…ned_catalog_roles Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Issue
Catalog/database level roles (
{database-name}_admin
and{database-name}_owner
) are missing to complete what was specified at DA169 - Predefined roles in Postgres.Solution
Create catalog/database level roles described at DA169 - Predefined roles in Postgres.
Create the
charmed_databases_owner
role, which allows the relation users requesting it to be able to create new databases.Enable the login hook (to auto escalate relation users to the
{database-name}_owner
user when they log in).TODO:
lib/charms/postgresql_k8s/v0/postgresql.py
will be moved tolib/charms/postgresql_k8s/v1/postgresql.py
in a next PR.Checklist