From d56c485eddfa998033e95a41b2d492aff9693e34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sinclert=20P=C3=A9rez?= Date: Wed, 19 Feb 2025 14:57:43 +0100 Subject: [PATCH] [MISC] Define charm constants (#862) --- lib/charms/postgresql_k8s/v0/postgresql.py | 17 +++++++++----- src/charm.py | 3 ++- src/constants.py | 1 + tests/integration/helpers.py | 4 +++- .../new_relations/test_new_relations_1.py | 21 ++++++++++++------ .../new_relations/test_relations_coherence.py | 8 ++++--- tests/unit/test_postgresql.py | 22 +++++++++++++------ 7 files changed, 51 insertions(+), 25 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index bdfef9afbb..e395d6892f 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -35,7 +35,10 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 42 +LIBPATCH = 43 + +# Groups to distinguish database permissions +PERMISSIONS_GROUP_ADMIN = "admin" INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles" @@ -187,7 +190,7 @@ def create_database( Identifier(database) ) ) - for user_to_grant_access in [user, "admin", *self.system_users]: + for user_to_grant_access in [user, PERMISSIONS_GROUP_ADMIN, *self.system_users]: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( Identifier(database), Identifier(user_to_grant_access) @@ -236,15 +239,17 @@ def create_user( roles = privileges = None if extra_user_roles: extra_user_roles = tuple(extra_user_roles.lower().split(",")) - admin_role = "admin" in extra_user_roles + admin_role = PERMISSIONS_GROUP_ADMIN in extra_user_roles valid_privileges, valid_roles = self.list_valid_privileges_and_roles() roles = [ - role for role in extra_user_roles if role in valid_roles and role != "admin" + role + for role in extra_user_roles + if role in valid_roles and role != PERMISSIONS_GROUP_ADMIN ] privileges = { extra_user_role for extra_user_role in extra_user_roles - if extra_user_role not in roles and extra_user_role != "admin" + if extra_user_role not in roles and extra_user_role != PERMISSIONS_GROUP_ADMIN } invalid_privileges = [ privilege for privilege in privileges if privilege not in valid_privileges @@ -566,7 +571,7 @@ def set_up_database(self) -> None: ) ) self.create_user( - "admin", + PERMISSIONS_GROUP_ADMIN, extra_user_roles="pg_read_all_data,pg_write_all_data", ) cursor.execute("GRANT CONNECT ON DATABASE postgres TO admin;") diff --git a/src/charm.py b/src/charm.py index de0e27a07c..1c9fd7fbc5 100755 --- a/src/charm.py +++ b/src/charm.py @@ -86,6 +86,7 @@ from constants import ( APP_SCOPE, BACKUP_USER, + DATABASE_DEFAULT_NAME, METRICS_PORT, MONITORING_PASSWORD_KEY, MONITORING_USER, @@ -417,7 +418,7 @@ def postgresql(self) -> PostgreSQL: current_host=self.endpoint, user=USER, password=self.get_secret(APP_SCOPE, f"{USER}-password"), - database="postgres", + database=DATABASE_DEFAULT_NAME, system_users=SYSTEM_USERS, ) diff --git a/src/constants.py b/src/constants.py index c5b7d60552..ac5abdd39a 100644 --- a/src/constants.py +++ b/src/constants.py @@ -3,6 +3,7 @@ """File containing constants to be used in the charm.""" +DATABASE_DEFAULT_NAME = "postgres" DATABASE_PORT = "5432" PEER = "database-peers" BACKUP_USER = "backup" diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index e359457acc..ef0cbf6692 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -32,6 +32,8 @@ wait_fixed, ) +from constants import DATABASE_DEFAULT_NAME + CHARM_BASE = "ubuntu@22.04" CHARM_SERIES = "jammy" METADATA = yaml.safe_load(Path("./metadata.yaml").read_text()) @@ -325,7 +327,7 @@ async def execute_query_on_unit( unit_address: str, password: str, query: str, - database: str = "postgres", + database: str = DATABASE_DEFAULT_NAME, sslmode: str | None = None, ): """Execute given PostgreSQL query on a unit. diff --git a/tests/integration/new_relations/test_new_relations_1.py b/tests/integration/new_relations/test_new_relations_1.py index 66cfab2f95..4a48ba8feb 100644 --- a/tests/integration/new_relations/test_new_relations_1.py +++ b/tests/integration/new_relations/test_new_relations_1.py @@ -12,6 +12,8 @@ from pytest_operator.plugin import OpsTest from tenacity import Retrying, stop_after_attempt, wait_fixed +from constants import DATABASE_DEFAULT_NAME + from ..helpers import ( CHARM_BASE, check_database_users_existence, @@ -218,7 +220,10 @@ async def test_two_applications_doesnt_share_the_same_relation_data(ops_test: Op (another_application_app_name, f"{APPLICATION_APP_NAME.replace('-', '_')}_database"), ]: connection_string = await build_connection_string( - ops_test, application, FIRST_DATABASE_RELATION_NAME, database="postgres" + ops_test, + application, + FIRST_DATABASE_RELATION_NAME, + database=DATABASE_DEFAULT_NAME, ) with pytest.raises(psycopg2.Error): psycopg2.connect(connection_string) @@ -448,7 +453,7 @@ async def test_admin_role(ops_test: OpsTest): # Check that the user can access all the databases. for database in [ - "postgres", + DATABASE_DEFAULT_NAME, f"{APPLICATION_APP_NAME.replace('-', '_')}_database", "another_application_database", ]: @@ -472,11 +477,11 @@ async def test_admin_role(ops_test: OpsTest): ) assert version == data - # Write some data (it should fail in the "postgres" database). + # Write some data (it should fail in the default database name). random_name = ( f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}" ) - should_fail = database == "postgres" + should_fail = database == DATABASE_DEFAULT_NAME cursor.execute(f"CREATE TABLE {random_name}(data TEXT);") if should_fail: assert False, ( @@ -494,7 +499,7 @@ async def test_admin_role(ops_test: OpsTest): # Test the creation and deletion of databases. connection_string = await build_connection_string( - ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", database="postgres" + ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", database=DATABASE_DEFAULT_NAME ) connection = psycopg2.connect(connection_string) connection.autocommit = True @@ -503,8 +508,10 @@ async def test_admin_role(ops_test: OpsTest): cursor.execute(f"CREATE DATABASE {random_name};") cursor.execute(f"DROP DATABASE {random_name};") try: - cursor.execute("DROP DATABASE postgres;") - assert False, "the admin extra user role was able to drop the `postgres` system database" + cursor.execute(f"DROP DATABASE {DATABASE_DEFAULT_NAME};") + assert False, ( + f"the admin extra user role was able to drop the `{DATABASE_DEFAULT_NAME}` system database" + ) except psycopg2.errors.InsufficientPrivilege: # Ignore the error, as the admin extra user role mustn't be able to drop # the "postgres" system database. diff --git a/tests/integration/new_relations/test_relations_coherence.py b/tests/integration/new_relations/test_relations_coherence.py index b03e355885..9b1dc66830 100644 --- a/tests/integration/new_relations/test_relations_coherence.py +++ b/tests/integration/new_relations/test_relations_coherence.py @@ -9,6 +9,8 @@ import pytest from pytest_operator.plugin import OpsTest +from constants import DATABASE_DEFAULT_NAME + from ..helpers import CHARM_BASE, DATABASE_APP_NAME, build_and_deploy from .helpers import build_connection_string from .test_new_relations_1 import DATA_INTEGRATOR_APP_NAME @@ -120,14 +122,14 @@ async def test_relations(ops_test: OpsTest, charm): for database in [ DATA_INTEGRATOR_APP_NAME.replace("-", "_"), - "postgres", + DATABASE_DEFAULT_NAME, ]: logger.info(f"connecting to the following database: {database}") connection_string = await build_connection_string( ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", database=database ) connection = None - should_fail = database == "postgres" + should_fail = database == DATABASE_DEFAULT_NAME try: with psycopg2.connect( connection_string @@ -136,7 +138,7 @@ async def test_relations(ops_test: OpsTest, charm): data = cursor.fetchone() assert data[0] == "some data" - # Write some data (it should fail in the "postgres" database). + # Write some data (it should fail in the default database name). random_name = f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}" cursor.execute(f"CREATE TABLE {random_name}(data TEXT);") if should_fail: diff --git a/tests/unit/test_postgresql.py b/tests/unit/test_postgresql.py index 565e2a2c21..676c5709f8 100644 --- a/tests/unit/test_postgresql.py +++ b/tests/unit/test_postgresql.py @@ -5,6 +5,7 @@ import psycopg2 import pytest from charms.postgresql_k8s.v0.postgresql import ( + PERMISSIONS_GROUP_ADMIN, PostgreSQLCreateDatabaseError, PostgreSQLGetLastArchivedWALError, ) @@ -12,7 +13,14 @@ from psycopg2.sql import SQL, Composed, Identifier, Literal from charm import PostgresqlOperatorCharm -from constants import PEER +from constants import ( + BACKUP_USER, + MONITORING_USER, + PEER, + REPLICATION_USER, + REWIND_USER, + USER, +) @pytest.fixture(autouse=True) @@ -75,7 +83,7 @@ def test_create_database(harness): SQL("GRANT ALL PRIVILEGES ON DATABASE "), Identifier(database), SQL(" TO "), - Identifier("admin"), + Identifier(PERMISSIONS_GROUP_ADMIN), SQL(";"), ]) ), @@ -84,7 +92,7 @@ def test_create_database(harness): SQL("GRANT ALL PRIVILEGES ON DATABASE "), Identifier(database), SQL(" TO "), - Identifier("backup"), + Identifier(BACKUP_USER), SQL(";"), ]) ), @@ -93,7 +101,7 @@ def test_create_database(harness): SQL("GRANT ALL PRIVILEGES ON DATABASE "), Identifier(database), SQL(" TO "), - Identifier("replication"), + Identifier(REPLICATION_USER), SQL(";"), ]) ), @@ -102,7 +110,7 @@ def test_create_database(harness): SQL("GRANT ALL PRIVILEGES ON DATABASE "), Identifier(database), SQL(" TO "), - Identifier("rewind"), + Identifier(REWIND_USER), SQL(";"), ]) ), @@ -111,7 +119,7 @@ def test_create_database(harness): SQL("GRANT ALL PRIVILEGES ON DATABASE "), Identifier(database), SQL(" TO "), - Identifier("operator"), + Identifier(USER), SQL(";"), ]) ), @@ -120,7 +128,7 @@ def test_create_database(harness): SQL("GRANT ALL PRIVILEGES ON DATABASE "), Identifier(database), SQL(" TO "), - Identifier("monitoring"), + Identifier(MONITORING_USER), SQL(";"), ]) ),