From 476799077574e39daa3ba490445441815a66716b Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Thu, 10 Apr 2025 19:30:28 +0000 Subject: [PATCH 01/21] WIP: Implement unpolished instance level predefined roles --- lib/charms/postgresql_k8s/v0/postgresql.py | 137 ++++++++++++--------- src/charm.py | 23 +++- src/constants.py | 4 +- src/relations/postgresql_provider.py | 9 +- src/upgrade.py | 2 +- templates/patroni.yml.j2 | 7 +- 6 files changed, 110 insertions(+), 72 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 9fe1957e4f..a3283f38a8 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -101,6 +101,10 @@ class PostgreSQLUpdateUserPasswordError(Exception): """Exception raised when updating a user password fails.""" +class PostgreSQLCreatePredefinedRolesError(Exception): + """Exception raised when creating predefined roles.""" + + class PostgreSQL: """Class to encapsulate all operations related to interacting with PostgreSQL instance.""" @@ -190,7 +194,7 @@ def create_database( Identifier(database) ) ) - for user_to_grant_access in [user, PERMISSIONS_GROUP_ADMIN, *self.system_users]: + for user_to_grant_access in [user, *self.system_users]: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( Identifier(database), Identifier(user_to_grant_access) @@ -222,67 +226,91 @@ def create_user( self, user: str, password: Optional[str] = None, - admin: bool = False, - extra_user_roles: Optional[List[str]] = None, + roles: Optional[List[str]] = None, ) -> None: """Creates a database user. Args: user: user to be created. password: password to be assigned to the user. - admin: whether the user should have additional admin privileges. - extra_user_roles: additional privileges and/or roles to be assigned to the user. + roles: roles to be assigned to the user. """ - try: - # Separate roles and privileges from the provided extra user roles. - admin_role = False - roles = privileges = None - if 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 != 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 != PERMISSIONS_GROUP_ADMIN - } - invalid_privileges = [ - privilege for privilege in privileges if privilege not in valid_privileges - ] - if len(invalid_privileges) > 0: - logger.error(f"Invalid extra user roles: {', '.join(privileges)}") - raise PostgreSQLCreateUserError(INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE) + try: + # TODO: find where admin role is created + existing_roles = self.list_roles() + invalid_roles = [role for role in roles if role not in existing_roles] + if invalid_roles: + logger.error(f"Invalid roles: {', '.join(invalid_roles)}") + # TODO: rename INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE + raise PostgreSQLCreateUserError(INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE) + with self._connect_to_database() as connection, connection.cursor() as cursor: - # Create or update the user. cursor.execute( SQL("SELECT TRUE FROM pg_roles WHERE rolname={};").format(Literal(user)) ) if cursor.fetchone() is not None: - user_definition = "ALTER ROLE {}" + user_query = "ALTER ROLE {} " else: - user_definition = "CREATE ROLE {}" - user_definition += f"WITH {'NOLOGIN' if user == 'admin' else 'LOGIN'}{' SUPERUSER' if admin else ''} ENCRYPTED PASSWORD '{password}'{'IN ROLE admin CREATEDB' if admin_role else ''}" - if privileges: - user_definition += f" {' '.join(privileges)}" + user_query = "CREATE ROLE {} " + user_query += f"WITH LOGIN ENCRYPTED PASSWORD '{password}' {'IN ROLE ' if roles else ''}" + if roles: + user_query += f"{' '.join(roles)}" cursor.execute(SQL("BEGIN;")) cursor.execute(SQL("SET LOCAL log_statement = 'none';")) - cursor.execute(SQL(f"{user_definition};").format(Identifier(user))) + cursor.execute(SQL(f"{user_query};").format(Identifier(user))) cursor.execute(SQL("COMMIT;")) - - # Add extra user roles to the new user. - if roles: - for role in roles: - cursor.execute( - SQL("GRANT {} TO {};").format(Identifier(role), Identifier(user)) - ) except psycopg2.Error as e: logger.error(f"Failed to create user: {e}") raise PostgreSQLCreateUserError() from e + + def create_predefined_roles(self) -> None: + """Create predefined roles.""" + role_to_queries = { + "charmed_stats": [ + "CREATE ROLE charmed_stats NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_monitor", + ], + "charmed_read": [ + "CREATE ROLE charmed_read NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_read_all_data", + ], + "charmed_dml": [ + "CREATE ROLE charmed_dml NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_write_all_data", + ], + "charmed_replica": [ + "CREATE ROLE charmed_replica NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN REPLICATION", + ], + "charmed_backup": [ + "CREATE ROLE charmed_backup NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN", + "GRANT charmed_stats TO charmed_backup", + "GRANT execute ON FUNCTION pg_backup_start TO charmed_backup", + "GRANT execute ON FUNCTION pg_backup_stop TO charmed_backup", + "GRANT execute ON FUNCTION pg_create_restore_point TO charmed_backup", + "GRANT execute ON FUNCTION pg_switch_wal TO charmed_backup", + ], + "charmed_dba": [ + "CREATE ROLE charmed_dba NOSUPERUSER CREATEDB NOCREATEROLE NOLOGIN NOREPLICATION", + "GRANT execute ON FUNCTION set_user(text) TO charmed_dba", + "GRANT execute ON FUNCTION set_user(text, text) TO charmed_dba", + "GRANT execute ON FUNCTION set_user_u(text) TO charmed_dba", + ], + } + + existing_roles = self.list_roles() + + try: + with self._connect_to_database() as connection, connection.cursor() as cursor: + for role, queries in role_to_queries.items(): + if role in existing_roles: + logger.debug(f"Role {role} already exists") + continue + + logger.info(f"Creating predefined role {role}") + + for query in queries: + cursor.execute(SQL(query)) + except psycopg2.Error as e: + logger.error(f"Failed to create predefined roles: {e}") + raise PostgreSQLCreatePredefinedRolesError() from e def delete_user(self, user: str) -> None: """Deletes a database user. @@ -549,20 +577,15 @@ def list_users(self) -> Set[str]: logger.error(f"Failed to list PostgreSQL database users: {e}") raise PostgreSQLListUsersError() from e - def list_valid_privileges_and_roles(self) -> Tuple[Set[str], Set[str]]: - """Returns two sets with valid privileges and roles. + def list_roles(self) -> Tuple[Set[str], Set[str]]: + """Returns valid roles in the database. Returns: - Tuple containing two sets: the first with valid privileges - and the second with valid roles. + A set containing the existing roles in the database. """ with self._connect_to_database() as connection, connection.cursor() as cursor: cursor.execute("SELECT rolname FROM pg_roles;") - return { - "createdb", - "createrole", - "superuser", - }, {role[0] for role in cursor.fetchall() if role[0]} + return {role[0] for role in cursor.fetchall() if role[0]} def set_up_database(self) -> None: """Set up postgres database with the right permissions.""" @@ -582,11 +605,13 @@ def set_up_database(self) -> None: Identifier(user) ) ) - self.create_user( - PERMISSIONS_GROUP_ADMIN, - extra_user_roles=["pg_read_all_data", "pg_write_all_data"], - ) - cursor.execute("GRANT CONNECT ON DATABASE postgres TO admin;") + + # TODO: determine why the 'admin' user was being created + # self.create_user( + # PERMISSIONS_GROUP_ADMIN, + # extra_user_roles=["pg_read_all_data", "pg_write_all_data"], + # ) + # cursor.execute("GRANT CONNECT ON DATABASE postgres TO admin;") except psycopg2.Error as e: logger.error(f"Failed to set up databases: {e}") raise PostgreSQLDatabasesSetupError() from e diff --git a/src/charm.py b/src/charm.py index e3b64b9068..1045ec9eb6 100755 --- a/src/charm.py +++ b/src/charm.py @@ -1058,6 +1058,7 @@ def _on_leader_elected(self, event: LeaderElectedEvent) -> None: MONITORING_PASSWORD_KEY, RAFT_PASSWORD_KEY, PATRONI_PASSWORD_KEY, + "dba-user-password", ): if self.get_secret(APP_SCOPE, key) is None: self.set_secret(APP_SCOPE, key, new_password()) @@ -1301,23 +1302,34 @@ def _start_primary(self, event: StartEvent) -> None: event.defer() return + self.postgresql.enable_disable_extensions({"set_user": True}, database="postgres") + + # TODO: add exception handling + self.postgresql.create_predefined_roles() + # Create the default postgres database user that is needed for some # applications (not charms) like Landscape Server. try: # This event can be run on a replica if the machines are restarted. # For that case, check whether the postgres user already exits. users = self.postgresql.list_users() - if "postgres" not in users: - self.postgresql.create_user("postgres", new_password(), admin=True) - # Create the backup user. + # TODO: find out where the 'postgres' user is used + # if "postgres" not in users: + # self.postgresql.create_user("postgres", new_password(), admin=True) + # Create the backup user. + if "dba_user" not in users: + self.postgresql.create_user( + "dba_user", self.get_secret(APP_SCOPE, "dba-user-password"), roles=["charmed_dba"] + ) + # TODO: move predefined roles into constants if BACKUP_USER not in users: - self.postgresql.create_user(BACKUP_USER, new_password(), admin=True) + self.postgresql.create_user(BACKUP_USER, new_password(), roles=["charmed_backup"]) if MONITORING_USER not in users: # Create the monitoring user. self.postgresql.create_user( MONITORING_USER, self.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY), - extra_user_roles=["pg_monitor"], + roles=["charmed_stats"], ) except PostgreSQLCreateUserError as e: logger.exception(e) @@ -2109,6 +2121,7 @@ def get_plugins(self) -> list[str]: for plugin in self.config.plugin_keys() if self.config[plugin] ] + plugins.append("set_user") plugins = [PLUGIN_OVERRIDES.get(plugin, plugin) for plugin in plugins] if "spi" in plugins: plugins.remove("spi") diff --git a/src/constants.py b/src/constants.py index 9eb1ac152c..a6bc4ceb56 100644 --- a/src/constants.py +++ b/src/constants.py @@ -32,7 +32,7 @@ SNAP_PACKAGES = [ ( POSTGRESQL_SNAP_NAME, - {"revision": {"aarch64": "160", "x86_64": "161"}}, + {"revision": {"aarch64": "160", "x86_64": "166"}}, ) ] @@ -79,7 +79,7 @@ TRACING_PROTOCOL = "otlp_http" BACKUP_TYPE_OVERRIDES = {"full": "full", "differential": "diff", "incremental": "incr"} -PLUGIN_OVERRIDES = {"audit": "pgaudit", "uuid_ossp": '"uuid-ossp"'} +PLUGIN_OVERRIDES = {"audit": "pgaudit", "uuid_ossp": '"uuid-ossp"', "set_user": '"set_user"'} SPI_MODULE = ["refint", "autoinc", "insert_username", "moddatetime"] diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index c2fa16add0..2759a601c2 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -100,7 +100,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: # Creates the user and the database for this specific relation. user = f"relation-{event.relation.id}" password = new_password() - self.charm.postgresql.create_user(user, password, extra_user_roles=extra_user_roles) + self.charm.postgresql.create_user(user, password, roles=extra_user_roles) plugins = self.charm.get_plugins() self.charm.postgresql.create_database( @@ -294,7 +294,7 @@ def check_for_invalid_extra_user_roles(self, relation_id: int) -> bool: Args: relation_id: current relation to be skipped. """ - valid_privileges, valid_roles = self.charm.postgresql.list_valid_privileges_and_roles() + valid_roles = self.charm.postgresql.list_roles() for relation in self.charm.model.relations.get(self.relation_name, []): if relation.id == relation_id: continue @@ -302,9 +302,6 @@ def check_for_invalid_extra_user_roles(self, relation_id: int) -> bool: extra_user_roles = data.get("extra-user-roles") extra_user_roles = self._sanitize_extra_roles(extra_user_roles) for extra_user_role in extra_user_roles: - if ( - extra_user_role not in valid_privileges - and extra_user_role not in valid_roles - ): + if extra_user_role not in valid_roles: return True return False diff --git a/src/upgrade.py b/src/upgrade.py index d2de69bfde..79db82f88b 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -244,7 +244,7 @@ def _prepare_upgrade_from_legacy(self) -> None: self.charm.postgresql.create_user( MONITORING_USER, self.charm.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY), - extra_user_roles="pg_monitor", + roles=["charmed_stats"], ) self.charm.postgresql.set_up_database() diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index 2d403cd92c..bbafe6cf07 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -102,7 +102,7 @@ bootstrap: log_truncate_on_rotation: 'on' logging_collector: 'on' wal_level: logical - shared_preload_libraries: 'timescaledb,pgaudit' + shared_preload_libraries: 'timescaledb,pgaudit,set_user' {%- if restoring_backup %} method: pgbackrest @@ -135,7 +135,7 @@ postgresql: bin_dir: /snap/charmed-postgresql/current/usr/lib/postgresql/{{ version }}/bin data_dir: {{ data_path }} parameters: - shared_preload_libraries: 'timescaledb,pgaudit' + shared_preload_libraries: 'timescaledb,pgaudit,set_user' {%- if enable_pgbackrest_archiving %} archive_command: 'pgbackrest {{ pgbackrest_configuration_file }} --stanza={{ stanza }} archive-push %p' {% else %} @@ -153,6 +153,9 @@ postgresql: {{key}}: {{value}} {%- endfor -%} {% endif %} + set_user.block_log_statement: on + set_user.exit_on_error: on + set_user.superuser_allowlist: '+dba' pgpass: /tmp/pgpass pg_hba: - local all backup peer map=operator From 563ac521954d78a99f1c9b577509f529e2c97e9c Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 16 Apr 2025 00:04:35 +0000 Subject: [PATCH 02/21] WIP: Working implementation of catalog level roles --- .github/workflows/ci.yaml | 3 +- lib/charms/postgresql_k8s/v0/postgresql.py | 171 +++++---------------- src/charm.py | 45 +++--- src/cluster.py | 13 ++ src/constants.py | 3 +- src/relations/postgresql_provider.py | 13 +- templates/patroni.yml.j2 | 4 +- 7 files changed, 88 insertions(+), 164 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index cdf5a0e2e6..4162199d8b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -55,7 +55,8 @@ jobs: name: Integration test charm needs: - lint - - unit-test + # TODO: re-enable unit-tests to run integration testss + # - unit-test - build uses: ./.github/workflows/integration_test.yaml with: diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index a3283f38a8..bd6848fce7 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -164,63 +164,57 @@ def _connect_to_database( connection.autocommit = True return connection - def create_database( - self, - database: str, - user: str, - plugins: Optional[List[str]] = None, - client_relations: Optional[List[Relation]] = None, - ) -> None: + def create_database(self, database: str,) -> bool: """Creates a new database and grant privileges to a user on it. Args: - database: database to be created. - user: user that will have access to the database. - plugins: extensions to enable in the new database. - client_relations: current established client relations. + database: database to be created + + Returns: + boolean indicating whether a database was created """ - plugins = plugins if plugins else [] - client_relations = client_relations if client_relations else [] try: connection = self._connect_to_database() cursor = connection.cursor() + cursor.execute( SQL("SELECT datname FROM pg_database WHERE datname={};").format(Literal(database)) ) - if cursor.fetchone() is None: - cursor.execute(SQL("CREATE DATABASE {};").format(Identifier(database))) - cursor.execute( - SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format( - Identifier(database) - ) - ) - for user_to_grant_access in [user, *self.system_users]: + + if cursor.fetchone() is not None: + return False + + cursor.execute(SQL("CREATE DATABASE {};").format(Identifier(database))) + cursor.execute(SQL("ALTER DATABASE {} OWNER TO {}").format(Identifier(database), Identifier(f"{database}_owner"))) + cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_admin"))) + cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) + + for user_to_grant_access in self.system_users: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( Identifier(database), Identifier(user_to_grant_access) ) ) - relations_accessing_this_database = 0 - for relation in client_relations: - for data in relation.data.values(): - if data.get("database") == database: - relations_accessing_this_database += 1 - with self._connect_to_database(database=database) as conn, conn.cursor() as curs: - curs.execute( - "SELECT schema_name FROM information_schema.schemata WHERE schema_name NOT LIKE 'pg_%' and schema_name <> 'information_schema';" - ) - schemas = [row[0] for row in curs.fetchall()] - statements = self._generate_database_privileges_statements( - relations_accessing_this_database, schemas, user - ) - for statement in statements: - curs.execute(statement) + + with self._connect_to_database(database=database) as database_connection, database_connection.cursor() as database_cursor: + database_cursor.execute(SQL("""CREATE OR REPLACE FUNCTION {}() RETURNS TEXT AS $$ +BEGIN + -- Restricting search_path when using security definer IS recommended + SET LOCAL search_path = public; + RETURN set_user({}); +END; +$$ LANGUAGE plpgsql security definer; +""").format(Identifier(f"set_user_{database}_owner"), Literal(f"{database}_owner"))) + database_cursor.execute(SQL("ALTER FUNCTION {} OWNER TO {};").format(Identifier(f"set_user_{database}_owner"), Identifier("charmed_dba"))) + database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION {} TO {};").format(Identifier(f"set_user_{database}_owner"), Identifier(f"{database}_admin"))) + + return True except psycopg2.Error as e: logger.error(f"Failed to create database: {e}") raise PostgreSQLCreateDatabaseError() from e - - # Enable preset extensions - self.enable_disable_extensions({plugin: True for plugin in plugins}, database) + finally: + cursor.close() + connection.close() def create_user( self, @@ -235,14 +229,15 @@ def create_user( password: password to be assigned to the user. roles: roles to be assigned to the user. """ + if "admin" in roles: + roles.remove("admin") + roles.append("charmed_dml") try: - # TODO: find where admin role is created existing_roles = self.list_roles() invalid_roles = [role for role in roles if role not in existing_roles] if invalid_roles: logger.error(f"Invalid roles: {', '.join(invalid_roles)}") - # TODO: rename INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE raise PostgreSQLCreateUserError(INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE) with self._connect_to_database() as connection, connection.cursor() as cursor: @@ -323,25 +318,7 @@ def delete_user(self, user: str) -> None: if user not in users: return - # List all databases. try: - with self._connect_to_database() as connection, connection.cursor() as cursor: - cursor.execute("SELECT datname FROM pg_database WHERE datistemplate = false;") - databases = [row[0] for row in cursor.fetchall()] - - # Existing objects need to be reassigned in each database - # before the user can be deleted. - for database in databases: - with self._connect_to_database( - database - ) as connection, connection.cursor() as cursor: - cursor.execute( - SQL("REASSIGN OWNED BY {} TO {};").format( - Identifier(user), Identifier(self.user) - ) - ) - cursor.execute(SQL("DROP OWNED BY {};").format(Identifier(user))) - # Delete the user. with self._connect_to_database() as connection, connection.cursor() as cursor: cursor.execute(SQL("DROP ROLE {};").format(Identifier(user))) @@ -368,7 +345,8 @@ def enable_disable_extensions( else: # Retrieve all the databases. with self._connect_to_database() as connection, connection.cursor() as cursor: - cursor.execute("SELECT datname FROM pg_database WHERE NOT datistemplate;") + # template0 is meant to be unmodifyable + cursor.execute("SELECT datname FROM pg_database WHERE datname <> 'template0';") databases = {database[0] for database in cursor.fetchall()} ordered_extensions = OrderedDict() @@ -399,72 +377,6 @@ def enable_disable_extensions( if connection is not None: connection.close() - def _generate_database_privileges_statements( - self, relations_accessing_this_database: int, schemas: List[str], user: str - ) -> List[Composed]: - """Generates a list of databases privileges statements.""" - statements = [] - if relations_accessing_this_database == 1: - statements.append( - SQL( - """DO $$ -DECLARE r RECORD; -BEGIN - FOR r IN (SELECT statement FROM (SELECT 1 AS index,'ALTER TABLE '|| schemaname || '."' || tablename ||'" OWNER TO {};' AS statement -FROM pg_tables WHERE NOT schemaname IN ('pg_catalog', 'information_schema') -UNION SELECT 2 AS index,'ALTER SEQUENCE '|| sequence_schema || '."' || sequence_name ||'" OWNER TO {};' AS statement -FROM information_schema.sequences WHERE NOT sequence_schema IN ('pg_catalog', 'information_schema') -UNION SELECT 3 AS index,'ALTER FUNCTION '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement -FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'f' -UNION SELECT 4 AS index,'ALTER PROCEDURE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement -FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'p' -UNION SELECT 5 AS index,'ALTER AGGREGATE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement -FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'a' -UNION SELECT 6 AS index,'ALTER VIEW '|| schemaname || '."' || viewname ||'" OWNER TO {};' AS statement -FROM pg_catalog.pg_views WHERE NOT schemaname IN ('pg_catalog', 'information_schema')) AS statements ORDER BY index) LOOP - EXECUTE format(r.statement); - END LOOP; -END; $$;""" - ).format( - Identifier(user), - Identifier(user), - Identifier(user), - Identifier(user), - Identifier(user), - Identifier(user), - ) - ) - statements.append( - SQL( - "UPDATE pg_catalog.pg_largeobject_metadata\n" - "SET lomowner = (SELECT oid FROM pg_roles WHERE rolname = {})\n" - "WHERE lomowner = (SELECT oid FROM pg_roles WHERE rolname = {});" - ).format(Literal(user), Literal(self.user)) - ) - for schema in schemas: - statements.append( - SQL("ALTER SCHEMA {} OWNER TO {};").format( - Identifier(schema), Identifier(user) - ) - ) - else: - for schema in schemas: - schema = Identifier(schema) - statements.extend([ - SQL("GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA {} TO {};").format( - schema, Identifier(user) - ), - SQL("GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA {} TO {};").format( - schema, Identifier(user) - ), - SQL("GRANT ALL PRIVILEGES ON ALL FUNCTIONS IN SCHEMA {} TO {};").format( - schema, Identifier(user) - ), - SQL("GRANT USAGE ON SCHEMA {} TO {};").format(schema, Identifier(user)), - SQL("GRANT CREATE ON SCHEMA {} TO {};").format(schema, Identifier(user)), - ]) - return statements - def get_last_archived_wal(self) -> str: """Get the name of the last archived wal for the current PostgreSQL cluster.""" try: @@ -605,13 +517,6 @@ def set_up_database(self) -> None: Identifier(user) ) ) - - # TODO: determine why the 'admin' user was being created - # self.create_user( - # PERMISSIONS_GROUP_ADMIN, - # extra_user_roles=["pg_read_all_data", "pg_write_all_data"], - # ) - # cursor.execute("GRANT CONNECT ON DATABASE postgres TO admin;") except psycopg2.Error as e: logger.error(f"Failed to set up databases: {e}") raise PostgreSQLDatabasesSetupError() from e diff --git a/src/charm.py b/src/charm.py index 1045ec9eb6..7dc6830e31 100755 --- a/src/charm.py +++ b/src/charm.py @@ -25,6 +25,7 @@ from charms.postgresql_k8s.v0.postgresql import ( REQUIRED_PLUGINS, PostgreSQL, + PostgreSQLCreatePredefinedRolesError, PostgreSQLCreateUserError, PostgreSQLEnableDisableExtensionError, PostgreSQLGetCurrentTimelineError, @@ -1145,7 +1146,8 @@ def enable_disable_extensions(self, database: str | None = None) -> None: logger.debug("Early exit enable_disable_extensions: standby cluster") return original_status = self.unit.status - extensions = {} + # Always want set_user to be enabled + extensions = {"set_user": True} # collect extensions for plugin in self.config.plugin_keys(): enable = self.config[plugin] @@ -1302,10 +1304,20 @@ def _start_primary(self, event: StartEvent) -> None: event.defer() return - self.postgresql.enable_disable_extensions({"set_user": True}, database="postgres") + try: + # Needed to create predefined roles with set_user execution privileges + self.postgresql.enable_disable_extensions({"set_user": True}, database="postgres") + except Exception as e: + logger.exception(e) + self.unit.status = BlockedStatus("Failed to enable set-user extension") + return - # TODO: add exception handling - self.postgresql.create_predefined_roles() + try: + self.postgresql.create_predefined_roles() + except PostgreSQLCreatePredefinedRolesError as e: + logger.exception(e) + self.unit.status = BlockedStatus("Failed to create pre-defined roles") + return # Create the default postgres database user that is needed for some # applications (not charms) like Landscape Server. @@ -1313,13 +1325,13 @@ def _start_primary(self, event: StartEvent) -> None: # This event can be run on a replica if the machines are restarted. # For that case, check whether the postgres user already exits. users = self.postgresql.list_users() - # TODO: find out where the 'postgres' user is used - # if "postgres" not in users: - # self.postgresql.create_user("postgres", new_password(), admin=True) - # Create the backup user. + # Create the dba user. + # TODO: move 'dba_user' and 'dba-user-password' to constants if "dba_user" not in users: self.postgresql.create_user( - "dba_user", self.get_secret(APP_SCOPE, "dba-user-password"), roles=["charmed_dba"] + "dba_user", + self.get_secret(APP_SCOPE, "dba-user-password"), + roles=["charmed_dba"], ) # TODO: move predefined roles into constants if BACKUP_USER not in users: @@ -2114,21 +2126,6 @@ def log_pitr_last_transaction_time(self) -> None: else: logger.error("Can't tell last completed transaction time") - def get_plugins(self) -> list[str]: - """Return a list of installed plugins.""" - plugins = [ - "_".join(plugin.split("_")[1:-1]) - for plugin in self.config.plugin_keys() - if self.config[plugin] - ] - plugins.append("set_user") - plugins = [PLUGIN_OVERRIDES.get(plugin, plugin) for plugin in plugins] - if "spi" in plugins: - plugins.remove("spi") - for ext in SPI_MODULE: - plugins.append(ext) - return plugins - if __name__ == "__main__": main(PostgresqlOperatorCharm) diff --git a/src/cluster.py b/src/cluster.py index 6d227cda26..b57330e7d3 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -640,6 +640,18 @@ def render_patroni_yml_file( # Open the template patroni.yml file. with open("templates/patroni.yml.j2") as file: template = Template(file.read()) + + # TODO: use can_connect_to_postgresql without retry + nosuperuser_target_allowlist = ( + ",".join([ + f"+{role}" + for role in self.charm.postgresql.list_roles() + if not role.startswith("pg_") and role.endswith("_owner") + ]) + if self.charm._is_workload_running + else "" + ) + # Render the template file with the correct values. rendered = template.render( conf_path=PATRONI_CONF_PATH, @@ -678,6 +690,7 @@ def render_patroni_yml_file( extra_replication_endpoints=self.charm.async_replication.get_standby_endpoints(), raft_password=self.raft_password, patroni_password=self.patroni_password, + nosuperuser_target_allowlist=nosuperuser_target_allowlist, ) self.render_file(f"{PATRONI_CONF_PATH}/patroni.yaml", rendered, 0o600) diff --git a/src/constants.py b/src/constants.py index a6bc4ceb56..4b3fe7c996 100644 --- a/src/constants.py +++ b/src/constants.py @@ -24,7 +24,8 @@ PATRONI_SERVICE_NAME = "snap.charmed-postgresql.patroni.service" PATRONI_SERVICE_DEFAULT_PATH = f"/etc/systemd/system/{PATRONI_SERVICE_NAME}" # List of system usernames needed for correct work of the charm/workload. -SYSTEM_USERS = [BACKUP_USER, REPLICATION_USER, REWIND_USER, USER, MONITORING_USER] +# TODO: Add BACKUP_USER and MONITORING_USER +SYSTEM_USERS = [REPLICATION_USER, REWIND_USER, USER] # Snap constants. PGBACKREST_EXECUTABLE = "charmed-postgresql.pgbackrest" diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index 2759a601c2..16639610a9 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -100,13 +100,18 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: # Creates the user and the database for this specific relation. user = f"relation-{event.relation.id}" password = new_password() - self.charm.postgresql.create_user(user, password, roles=extra_user_roles) - plugins = self.charm.get_plugins() - self.charm.postgresql.create_database( - database, user, plugins=plugins, client_relations=self.charm.client_relations + database_created = self.charm.postgresql.create_database(database) + + # TODO: change role to database_admin once we determine how to auto-escalate + # privileges to database_owner upon login + self.charm.postgresql.create_user( + user, password, roles=[*extra_user_roles, f"{database}_owner"] ) + if database_created: + self.charm.update_config() + # Share the credentials with the application. self.database_provides.set_credentials(event.relation.id, user, password) diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index bbafe6cf07..d9e96fbc78 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -102,7 +102,6 @@ bootstrap: log_truncate_on_rotation: 'on' logging_collector: 'on' wal_level: logical - shared_preload_libraries: 'timescaledb,pgaudit,set_user' {%- if restoring_backup %} method: pgbackrest @@ -156,6 +155,9 @@ postgresql: set_user.block_log_statement: on set_user.exit_on_error: on set_user.superuser_allowlist: '+dba' + {%- if nosuperuser_target_allowlist %} + set_user.nosuperuser_target_allowlist: '{{ nosuperuser_target_allowlist }}' + {% endif %} pgpass: /tmp/pgpass pg_hba: - local all backup peer map=operator From ba17595f006fd1438a86aa560d979ae457694e55 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 16 Apr 2025 13:37:59 +0000 Subject: [PATCH 03/21] Ensure postgresql has started before creating set_user + add mistakenly removed database_owner role creation --- lib/charms/postgresql_k8s/v0/postgresql.py | 1 + src/charm.py | 8 +++++++- src/cluster.py | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index bd6848fce7..4b6a02e15c 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -185,6 +185,7 @@ def create_database(self, database: str,) -> bool: return False cursor.execute(SQL("CREATE DATABASE {};").format(Identifier(database))) + cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_owner"))) cursor.execute(SQL("ALTER DATABASE {} OWNER TO {}").format(Identifier(database), Identifier(f"{database}_owner"))) cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_admin"))) cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) diff --git a/src/charm.py b/src/charm.py index 7dc6830e31..4ebd5f7ffa 100755 --- a/src/charm.py +++ b/src/charm.py @@ -1290,7 +1290,7 @@ def _setup_exporter(self) -> None: postgres_snap.restart(services=[MONITORING_SNAP_SERVICE]) self.unit_peer_data.update({"exporter-started": "True"}) - def _start_primary(self, event: StartEvent) -> None: + def _start_primary(self, event: StartEvent) -> None: # noqa: C901 """Bootstrap the cluster.""" # Set some information needed by Patroni to bootstrap the cluster. if not self._patroni.bootstrap_cluster(): @@ -1304,6 +1304,12 @@ def _start_primary(self, event: StartEvent) -> None: event.defer() return + if not self._can_connect_to_postgresql: + logger.debug("Deferring on_start: awaiting for database to start") + self.unit.status = WaitingStatus("awaiting for database to start") + event.defer() + return + try: # Needed to create predefined roles with set_user execution privileges self.postgresql.enable_disable_extensions({"set_user": True}, database="postgres") diff --git a/src/cluster.py b/src/cluster.py index b57330e7d3..77869b9079 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -649,7 +649,7 @@ def render_patroni_yml_file( if not role.startswith("pg_") and role.endswith("_owner") ]) if self.charm._is_workload_running - else "" + else None ) # Render the template file with the correct values. From e03a6195826ea580acb1a6fa1f6a54054ad98f15 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 16 Apr 2025 16:55:15 +0000 Subject: [PATCH 04/21] Address for postgresql not running before patroni template is rendered --- src/cluster.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/cluster.py b/src/cluster.py index 77869b9079..223ca59ec0 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -15,6 +15,7 @@ from typing import Any import psutil +import psycopg2 import requests from charms.operator_libs_linux.v2 import snap from jinja2 import Template @@ -641,16 +642,19 @@ def render_patroni_yml_file( with open("templates/patroni.yml.j2") as file: template = Template(file.read()) - # TODO: use can_connect_to_postgresql without retry - nosuperuser_target_allowlist = ( - ",".join([ - f"+{role}" - for role in self.charm.postgresql.list_roles() - if not role.startswith("pg_") and role.endswith("_owner") - ]) - if self.charm._is_workload_running - else None - ) + try: + nosuperuser_target_allowlist = ( + ",".join([ + f"+{role}" + for role in self.charm.postgresql.list_roles() + if not role.startswith("pg_") and role.endswith("_owner") + ]) + if self.charm._is_workload_running + and self.charm.postgresql.get_postgresql_timezones() + else None + ) + except psycopg2.OperationalError: + nosuperuser_target_allowlist = None # Render the template file with the correct values. rendered = template.render( From 6b29546edb4452f26155ee60651c1a122378017b Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 16 Apr 2025 17:22:56 +0000 Subject: [PATCH 05/21] Simplify postgresql running check in render_patroni_yml --- src/charm.py | 8 +++++++- src/cluster.py | 23 +++++++++-------------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/charm.py b/src/charm.py index d7983da0db..3b48581021 100755 --- a/src/charm.py +++ b/src/charm.py @@ -384,7 +384,7 @@ def primary_endpoint(self) -> str | None: # TODO figure out why peer data is not available if primary_endpoint and len(self._units_ips) == 1 and len(self._peers.units) > 1: logger.warning( - "Possibly incoplete peer data: Will not map primary IP to unit IP" + "Possibly incomplete peer data: Will not map primary IP to unit IP" ) return primary_endpoint logger.debug("primary endpoint early exit: Primary IP not in cached peer list.") @@ -1310,6 +1310,12 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901 event.defer() return + if not self.primary_endpoint: + logger.debug("Deferrring on_start: awaitng start of the primary") + self.unit.status = WaitingStatus("awaiting start of the primary") + event.defer() + return + try: # Needed to create predefined roles with set_user execution privileges self.postgresql.enable_disable_extensions({"set_user": True}, database="postgres") diff --git a/src/cluster.py b/src/cluster.py index 223ca59ec0..e815867dc4 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -15,7 +15,6 @@ from typing import Any import psutil -import psycopg2 import requests from charms.operator_libs_linux.v2 import snap from jinja2 import Template @@ -642,19 +641,15 @@ def render_patroni_yml_file( with open("templates/patroni.yml.j2") as file: template = Template(file.read()) - try: - nosuperuser_target_allowlist = ( - ",".join([ - f"+{role}" - for role in self.charm.postgresql.list_roles() - if not role.startswith("pg_") and role.endswith("_owner") - ]) - if self.charm._is_workload_running - and self.charm.postgresql.get_postgresql_timezones() - else None - ) - except psycopg2.OperationalError: - nosuperuser_target_allowlist = None + nosuperuser_target_allowlist = ( + ",".join([ + f"+{role}" + for role in self.charm.postgresql.list_roles() + if not role.startswith("pg_") and role.endswith("_owner") + ]) + if self.charm.is_cluster_initialised + else None + ) # Render the template file with the correct values. rendered = template.render( From b7a7387b4c71be5ea795e3ee248833c48d8f0871 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 16 Apr 2025 19:47:42 +0000 Subject: [PATCH 06/21] Ensure that test_smoke passes locally --- src/cluster.py | 2 ++ tests/integration/ha_tests/test_smoke.py | 30 ++++++++++++------------ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/cluster.py b/src/cluster.py index e815867dc4..bfb97989e6 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -648,6 +648,8 @@ def render_patroni_yml_file( if not role.startswith("pg_") and role.endswith("_owner") ]) if self.charm.is_cluster_initialised + and self.charm.primary_endpoint is not None + and self.charm._can_connect_to_postgresql else None ) diff --git a/tests/integration/ha_tests/test_smoke.py b/tests/integration/ha_tests/test_smoke.py index b160d38f2b..6e3228856e 100644 --- a/tests/integration/ha_tests/test_smoke.py +++ b/tests/integration/ha_tests/test_smoke.py @@ -11,8 +11,8 @@ from tenacity import Retrying, stop_after_delay, wait_fixed from ..helpers import ( - APPLICATION_NAME, CHARM_BASE, + DATABASE_APP_NAME, ) from .helpers import ( add_unit_with_storage, @@ -26,7 +26,7 @@ ) TEST_DATABASE_NAME = "test_database" -DUP_APPLICATION_NAME = "postgres-test-dup" +DUP_DATABASE_APP_NAME = "postgres-test-dup" logger = logging.getLogger(__name__) @@ -39,7 +39,7 @@ async def test_app_force_removal(ops_test: OpsTest, charm: str): logger.info("deploying charm") await ops_test.model.deploy( charm, - application_name=APPLICATION_NAME, + application_name=DATABASE_APP_NAME, num_units=1, base=CHARM_BASE, storage={"pgdata": {"pool": "lxd-btrfs", "size": 8046}}, @@ -47,10 +47,10 @@ async def test_app_force_removal(ops_test: OpsTest, charm: str): ) logger.info("waiting for idle") - await ops_test.model.wait_for_idle(apps=[APPLICATION_NAME], status="active", timeout=1500) - assert ops_test.model.applications[APPLICATION_NAME].units[0].workload_status == "active" + await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active", timeout=1500) + assert ops_test.model.applications[DATABASE_APP_NAME].units[0].workload_status == "active" - primary_name = ops_test.model.applications[APPLICATION_NAME].units[0].name + primary_name = ops_test.model.applications[DATABASE_APP_NAME].units[0].name logger.info("waiting for postgresql") for attempt in Retrying(stop=stop_after_delay(15 * 3), wait=wait_fixed(3), reraise=True): @@ -61,18 +61,18 @@ async def test_app_force_removal(ops_test: OpsTest, charm: str): storage_id_str = storage_id(ops_test, primary_name) # Check if storage exists after application deployed - logger.info("werifing is storage exists") + logger.info("verifying if storage exists") for attempt in Retrying(stop=stop_after_delay(15 * 3), wait=wait_fixed(3), reraise=True): with attempt: assert await is_storage_exists(ops_test, storage_id_str) # Create test database to check there is no resources conflicts logger.info("creating db") - await create_db(ops_test, APPLICATION_NAME, TEST_DATABASE_NAME) + await create_db(ops_test, DATABASE_APP_NAME, TEST_DATABASE_NAME) # Check that test database is not exists for new unit logger.info("checking db") - assert await check_db(ops_test, APPLICATION_NAME, TEST_DATABASE_NAME) + assert await check_db(ops_test, DATABASE_APP_NAME, TEST_DATABASE_NAME) # Destroy charm logger.info("force removing charm") @@ -98,9 +98,9 @@ async def test_charm_garbage_ignorance(ops_test: OpsTest, charm: str): garbage_storage = await get_any_deatached_storage(ops_test) logger.info("add unit with attached storage") - await add_unit_with_storage(ops_test, APPLICATION_NAME, garbage_storage) + await add_unit_with_storage(ops_test, DATABASE_APP_NAME, garbage_storage) - primary_name = ops_test.model.applications[APPLICATION_NAME].units[0].name + primary_name = ops_test.model.applications[DATABASE_APP_NAME].units[0].name logger.info("waiting for postgresql") for attempt in Retrying(stop=stop_after_delay(15 * 3), wait=wait_fixed(3), reraise=True): @@ -120,7 +120,7 @@ async def test_charm_garbage_ignorance(ops_test: OpsTest, charm: str): # Check that test database exists for new unit logger.info("checking db") - assert await check_db(ops_test, APPLICATION_NAME, TEST_DATABASE_NAME) + assert await check_db(ops_test, DATABASE_APP_NAME, TEST_DATABASE_NAME) logger.info("removing charm") await ops_test.model.destroy_unit(primary_name) @@ -139,7 +139,7 @@ async def test_app_resources_conflicts_v3(ops_test: OpsTest, charm: str): logger.info("deploying duplicate application with attached storage") await ops_test.model.deploy( charm, - application_name=DUP_APPLICATION_NAME, + application_name=DUP_DATABASE_APP_NAME, num_units=1, base=CHARM_BASE, attach_storage=[tag.storage(garbage_storage)], @@ -152,7 +152,7 @@ async def test_app_resources_conflicts_v3(ops_test: OpsTest, charm: str): logger.info("waiting for duplicate application to be blocked") try: await ops_test.model.wait_for_idle( - apps=[DUP_APPLICATION_NAME], timeout=1000, status="blocked" + apps=[DUP_DATABASE_APP_NAME], timeout=1000, status="blocked" ) except asyncio.TimeoutError: logger.info("Application is not in blocked state. Checking logs...") @@ -160,5 +160,5 @@ async def test_app_resources_conflicts_v3(ops_test: OpsTest, charm: str): # Since application have postgresql db in storage from external application it should not be able to connect due to new password logger.info("checking operator password auth") assert not await check_password_auth( - ops_test, ops_test.model.applications[DUP_APPLICATION_NAME].units[0].name + ops_test, ops_test.model.applications[DUP_DATABASE_APP_NAME].units[0].name ) From ee95ae86ee0271bd8fa1240077147c5e9c50046b Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 23 Apr 2025 13:56:29 +0000 Subject: [PATCH 07/21] WIP: Attempt to get more existing integration tests to pass --- lib/charms/postgresql_k8s/v0/postgresql.py | 57 ++++++++++++++++++++-- src/charm.py | 8 +++ src/constants.py | 3 +- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 4b6a02e15c..529821cf6d 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -104,6 +104,9 @@ class PostgreSQLUpdateUserPasswordError(Exception): class PostgreSQLCreatePredefinedRolesError(Exception): """Exception raised when creating predefined roles.""" +class PostgreSQLGrantDatabasePrivilegesToUserError(Exception): + """Exception raised when granting database privileges to user.""" + class PostgreSQL: """Class to encapsulate all operations related to interacting with PostgreSQL instance.""" @@ -164,7 +167,7 @@ def _connect_to_database( connection.autocommit = True return connection - def create_database(self, database: str,) -> bool: + def create_database(self, database: str) -> bool: """Creates a new database and grant privileges to a user on it. Args: @@ -230,9 +233,27 @@ def create_user( password: password to be assigned to the user. roles: roles to be assigned to the user. """ + createdb_enabled, createrole_enabled = False, False + if "pgbouncer" in roles: + createdb_enabled, createrole_enabled = True, True + roles.remove("pgbouncer") + if "admin" in roles: - roles.remove("admin") + createdb_enabled = True roles.append("charmed_dml") + roles.remove("admin") + + if "CREATEDB" in roles: + createdb_enabled = True + roles.remove("CREATEDB") + + if "CREATEROLE" in roles: + createrole_enabled = True + roles.remove("CREATEROLE") + + if "SUPERUSER" in roles: + logger.warning("SUPERUSER privileges not allowed via extra-user-roles") + roles.remove("SUPERUSER") try: existing_roles = self.list_roles() @@ -249,9 +270,10 @@ def create_user( user_query = "ALTER ROLE {} " else: user_query = "CREATE ROLE {} " - user_query += f"WITH LOGIN ENCRYPTED PASSWORD '{password}' {'IN ROLE ' if roles else ''}" + user_query += f"WITH LOGIN {'CREATEDB' if createdb_enabled else ''} {'CREATEROLE' if createrole_enabled else ''} ENCRYPTED PASSWORD '{password}' {'IN ROLE ' if roles else ''}" if roles: - user_query += f"{' '.join(roles)}" + user_query += f"{', '.join(roles)}" + cursor.execute(SQL("BEGIN;")) cursor.execute(SQL("SET LOCAL log_statement = 'none';")) cursor.execute(SQL(f"{user_query};").format(Identifier(user))) @@ -276,7 +298,7 @@ def create_predefined_roles(self) -> None: "CREATE ROLE charmed_replica NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN REPLICATION", ], "charmed_backup": [ - "CREATE ROLE charmed_backup NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN", + "CREATE ROLE charmed_backup NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_checkpoint", "GRANT charmed_stats TO charmed_backup", "GRANT execute ON FUNCTION pg_backup_start TO charmed_backup", "GRANT execute ON FUNCTION pg_backup_stop TO charmed_backup", @@ -304,10 +326,23 @@ def create_predefined_roles(self) -> None: for query in queries: cursor.execute(SQL(query)) + + # TODO: see if we need charmed_replica role at all + # cursor.execute(SQL("ALTER ROLE replication NOREPLICATION;")) + # cursor.execute(SQL("GRANT charmed_replica TO replication;")) except psycopg2.Error as e: logger.error(f"Failed to create predefined roles: {e}") raise PostgreSQLCreatePredefinedRolesError() from e + def grant_database_privileges_to_user(self, user: str, database: str, privileges: list[str]) -> None: + """Grant the specified priviliges on the provided database for the user.""" + try: + with self._connect_to_database() as connection, connection.cursor() as cursor: + cursor.execute(SQL("GRANT {} ON DATABASE {} TO {};").format(Identifier(", ".join(privileges)), Identifier(database), Identifier(user))) + except psycopg2.Error as e: + logger.error(f"Faield to grant privileges to user: {e}") + raise PostgreSQLGrantDatabasePrivilegesToUserError() from e + def delete_user(self, user: str) -> None: """Deletes a database user. @@ -319,7 +354,19 @@ def delete_user(self, user: str) -> None: if user not in users: return + roles = self.list_roles() + try: + with self._connect_to_database() as connection, connection.cursor() as cursor: + cursor.execute(SQL("SELECT datname FROM pg_database WHERE datistemplate = false;")) + databases = [row[0] for row in cursor.fetchall()] + + for database in databases: + with self._connect_to_database(database) as connection, connection.cursor() as cursor: + inheriting_role = f"{database}_owner" if f"{database}_owner" in roles else self.user + cursor.execute(SQL("REASSIGN OWNED BY {} TO {};").format(Identifier(user), Identifier(inheriting_role))) + cursor.execute(SQL("DROP OWNED BY {};").format(Identifier(user))) + # Delete the user. with self._connect_to_database() as connection, connection.cursor() as cursor: cursor.execute(SQL("DROP ROLE {};").format(Identifier(user))) diff --git a/src/charm.py b/src/charm.py index 3b48581021..aff15bd810 100755 --- a/src/charm.py +++ b/src/charm.py @@ -29,6 +29,7 @@ PostgreSQLCreateUserError, PostgreSQLEnableDisableExtensionError, PostgreSQLGetCurrentTimelineError, + PostgreSQLGrantDatabasePrivilegesToUserError, PostgreSQLListUsersError, PostgreSQLUpdateUserPasswordError, ) @@ -1348,6 +1349,9 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901 # TODO: move predefined roles into constants if BACKUP_USER not in users: self.postgresql.create_user(BACKUP_USER, new_password(), roles=["charmed_backup"]) + self.postgresql.grant_database_privileges_to_user( + BACKUP_USER, "postgres", ["connect"] + ) if MONITORING_USER not in users: # Create the monitoring user. self.postgresql.create_user( @@ -1355,6 +1359,10 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901 self.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY), roles=["charmed_stats"], ) + except PostgreSQLGrantDatabasePrivilegesToUserError as e: + logger.exception(e) + self.unit.status = BlockedStatus("Failed to grant database privileges to user") + return except PostgreSQLCreateUserError as e: logger.exception(e) self.unit.status = BlockedStatus("Failed to create postgres user") diff --git a/src/constants.py b/src/constants.py index 4b3fe7c996..514bcfc806 100644 --- a/src/constants.py +++ b/src/constants.py @@ -24,8 +24,7 @@ PATRONI_SERVICE_NAME = "snap.charmed-postgresql.patroni.service" PATRONI_SERVICE_DEFAULT_PATH = f"/etc/systemd/system/{PATRONI_SERVICE_NAME}" # List of system usernames needed for correct work of the charm/workload. -# TODO: Add BACKUP_USER and MONITORING_USER -SYSTEM_USERS = [REPLICATION_USER, REWIND_USER, USER] +SYSTEM_USERS = [REPLICATION_USER, REWIND_USER, USER, MONITORING_USER] # Snap constants. PGBACKREST_EXECUTABLE = "charmed-postgresql.pgbackrest" From b43023813fa8d649953334d342a999b6026ecc16 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Thu, 24 Apr 2025 11:11:58 +0000 Subject: [PATCH 08/21] Accept createdb and createrole extra-user-roles in lowercase --- lib/charms/postgresql_k8s/v0/postgresql.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 6323a6f053..be144d2c47 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -275,6 +275,7 @@ def create_user( password: password to be assigned to the user. roles: roles to be assigned to the user. """ + createdb_enabled, createrole_enabled = False, False if "pgbouncer" in roles: createdb_enabled, createrole_enabled = True, True @@ -289,10 +290,18 @@ def create_user( createdb_enabled = True roles.remove("CREATEDB") + if "createdb" in roles: + createdb_enabled = True + roles.remove("createdb") + if "CREATEROLE" in roles: createrole_enabled = True roles.remove("CREATEROLE") + if "createrole" in roles: + createrole_enabled = True + roles.remove("createrole") + if "SUPERUSER" in roles: logger.warning("SUPERUSER privileges not allowed via extra-user-roles") roles.remove("SUPERUSER") From 5fe02d977be916ec8c572ffa81ad40aa448e772f Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Thu, 24 Apr 2025 13:46:20 +0000 Subject: [PATCH 09/21] Fixes for smoke and password rotation tests --- lib/charms/postgresql_k8s/v0/postgresql.py | 2 +- src/charm.py | 2 +- tests/integration/ha_tests/helpers.py | 21 +++++++++++++++++---- tests/integration/ha_tests/test_smoke.py | 18 ++++++++++-------- 4 files changed, 29 insertions(+), 14 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index be144d2c47..db3408a8b3 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -321,7 +321,7 @@ def create_user( user_query = "ALTER ROLE {} " else: user_query = "CREATE ROLE {} " - user_query += f"WITH LOGIN {'CREATEDB' if createdb_enabled else ''} {'CREATEROLE' if createrole_enabled else ''} ENCRYPTED PASSWORD '{password}' {'IN ROLE ' if roles else ''}" + user_query += f"WITH LOGIN{' CREATEDB' if createdb_enabled else ''}{' CREATEROLE' if createrole_enabled else ''} ENCRYPTED PASSWORD '{password}' {'IN ROLE ' if roles else ''}" if roles: user_query += f"{', '.join(roles)}" diff --git a/src/charm.py b/src/charm.py index 2e73ebb7ee..e225e04cd7 100755 --- a/src/charm.py +++ b/src/charm.py @@ -1588,7 +1588,7 @@ def _update_admin_password(self, admin_secret_id: str) -> None: # only SYSTEM_USERS with changed passwords are processed, all others ignored updated_passwords = self.get_secret_from_id(secret_id=admin_secret_id) for user, password in list(updated_passwords.items()): - if user not in SYSTEM_USERS: + if user not in [*SYSTEM_USERS, BACKUP_USER]: logger.error( f"Can only update system users: {', '.join(SYSTEM_USERS)} not {user}" ) diff --git a/tests/integration/ha_tests/helpers.py b/tests/integration/ha_tests/helpers.py index 0c56054775..c3de49059c 100644 --- a/tests/integration/ha_tests/helpers.py +++ b/tests/integration/ha_tests/helpers.py @@ -1068,11 +1068,24 @@ async def get_any_deatached_storage(ops_test: OpsTest) -> str: async def check_password_auth(ops_test: OpsTest, unit_name: str) -> bool: """Checks if "operator" password is valid for current postgresql db.""" - stdout = await run_command_on_unit( - ops_test, + complete_command = [ + "exec", + "--unit", unit_name, - """grep -E 'password authentication failed for user' /var/snap/charmed-postgresql/common/var/log/postgresql/postgresql*""", - ) + "--", + "grep", + "-E", + "'password'", + "/var/snap/charmed-postgresql/common/var/log/postgresql/postgresql*", + ] + return_code, stdout, _ = await ops_test.juju(*complete_command) + if return_code != 0: + raise Exception( + "Expected command %s to succeed. Instead it failed: %s with code: ", + complete_command, + stdout, + return_code, + ) return 'password authentication failed for user "operator"' not in stdout diff --git a/tests/integration/ha_tests/test_smoke.py b/tests/integration/ha_tests/test_smoke.py index 6e3228856e..2e8b9518dd 100644 --- a/tests/integration/ha_tests/test_smoke.py +++ b/tests/integration/ha_tests/test_smoke.py @@ -149,16 +149,18 @@ async def test_app_resources_conflicts_v3(ops_test: OpsTest, charm: str): # Reducing the update status frequency to speed up the triggering of deferred events. await ops_test.model.set_config({"update-status-hook-interval": "10s"}) - logger.info("waiting for duplicate application to be blocked") + logger.info("waiting for duplicate application to be waiting") try: await ops_test.model.wait_for_idle( - apps=[DUP_DATABASE_APP_NAME], timeout=1000, status="blocked" + apps=[DUP_DATABASE_APP_NAME], timeout=60, idle_period=30, status="waiting" ) except asyncio.TimeoutError: - logger.info("Application is not in blocked state. Checking logs...") + logger.info("Application is not in waiting state. Checking logs...") - # Since application have postgresql db in storage from external application it should not be able to connect due to new password - logger.info("checking operator password auth") - assert not await check_password_auth( - ops_test, ops_test.model.applications[DUP_DATABASE_APP_NAME].units[0].name - ) + for attempt in Retrying(stop=stop_after_delay(60 * 10), wait=wait_fixed(3), reraise=True): + with attempt: + # Since application have postgresql db in storage from external application it should not be able to connect due to new password + logger.info("checking operator password auth") + assert not await check_password_auth( + ops_test, ops_test.model.applications[DUP_DATABASE_APP_NAME].units[0].name + ) From b046ef702ae338265204b172690ad590937cc2c3 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Thu, 24 Apr 2025 23:24:13 +0000 Subject: [PATCH 10/21] Add missing set_user extension + predefined roles creation upon upgrade --- lib/charms/postgresql_k8s/v0/postgresql.py | 14 ++++++-------- src/upgrade.py | 17 +++++++++++++++++ tests/integration/ha_tests/test_upgrade.py | 2 +- .../new_relations/test_new_relations_1.py | 4 +++- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index db3408a8b3..eb930ef646 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -235,7 +235,7 @@ def create_database(self, database: str) -> bool: cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_admin"))) cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) - for user_to_grant_access in self.system_users: + for user_to_grant_access in [*self.system_users, "charmed_instance_admin"]: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( Identifier(database), Identifier(user_to_grant_access) @@ -284,6 +284,7 @@ def create_user( if "admin" in roles: createdb_enabled = True roles.append("charmed_dml") + roles.append("charmed_instance_admin") roles.remove("admin") if "CREATEDB" in roles: @@ -345,9 +346,6 @@ def create_predefined_roles(self) -> None: "charmed_dml": [ "CREATE ROLE charmed_dml NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_write_all_data", ], - "charmed_replica": [ - "CREATE ROLE charmed_replica NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN REPLICATION", - ], "charmed_backup": [ "CREATE ROLE charmed_backup NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_checkpoint", "GRANT charmed_stats TO charmed_backup", @@ -362,6 +360,10 @@ def create_predefined_roles(self) -> None: "GRANT execute ON FUNCTION set_user(text, text) TO charmed_dba", "GRANT execute ON FUNCTION set_user_u(text) TO charmed_dba", ], + "charmed_instance_admin": [ + "CREATE ROLE charmed_instance_admin NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION", + "GRANT connect ON DATABASE postgres TO charmed_instance_admin", + ] } existing_roles = self.list_roles() @@ -377,10 +379,6 @@ def create_predefined_roles(self) -> None: for query in queries: cursor.execute(SQL(query)) - - # TODO: see if we need charmed_replica role at all - # cursor.execute(SQL("ALTER ROLE replication NOREPLICATION;")) - # cursor.execute(SQL("GRANT charmed_replica TO replication;")) except psycopg2.Error as e: logger.error(f"Failed to create predefined roles: {e}") raise PostgreSQLCreatePredefinedRolesError() from e diff --git a/src/upgrade.py b/src/upgrade.py index 8bb7844c3c..0773554893 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -157,6 +157,23 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None: self.set_unit_failed() return + try: + # Needed to create predefined roles with set_user execution privileges + self.charm.postgresql.enable_disable_extensions( + {"set_user": True}, database="postgres" + ) + except Exception as e: + logger.exception(e) + self.set_unit_failed() + return + + try: + self.charm.postgresql.create_predefined_roles() + except Exception as e: + logger.exception(e) + self.set_unit_failed() + return + self.charm._setup_exporter() self.charm.backup.start_stop_pgbackrest_service() diff --git a/tests/integration/ha_tests/test_upgrade.py b/tests/integration/ha_tests/test_upgrade.py index e639b8ff60..7d269e75a7 100644 --- a/tests/integration/ha_tests/test_upgrade.py +++ b/tests/integration/ha_tests/test_upgrade.py @@ -26,7 +26,7 @@ logger = logging.getLogger(__name__) -TIMEOUT = 600 +TIMEOUT = 20 * 60 @pytest.mark.abort_on_fail diff --git a/tests/integration/new_relations/test_new_relations_1.py b/tests/integration/new_relations/test_new_relations_1.py index f9914f5284..e226b67dcd 100644 --- a/tests/integration/new_relations/test_new_relations_1.py +++ b/tests/integration/new_relations/test_new_relations_1.py @@ -462,7 +462,9 @@ async def test_relation_data_is_updated_correctly_when_scaling(ops_test: OpsTest f"{DATABASE_APP_NAME}:database", f"{APPLICATION_APP_NAME}:{FIRST_DATABASE_RELATION_NAME}", ) - await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active", timeout=1000) + await ops_test.model.wait_for_idle( + apps=[DATABASE_APP_NAME], status="active", timeout=1000, idle_period=30 + ) with pytest.raises(psycopg2.OperationalError): psycopg2.connect(primary_connection_string) From 607bdec893a05c5764d787054d3650683627d2b8 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Fri, 25 Apr 2025 21:12:28 +0000 Subject: [PATCH 11/21] Fix failing upgrade tests --- lib/charms/postgresql_k8s/v0/postgresql.py | 13 ++++-- src/charm.py | 4 +- src/upgrade.py | 47 ++++++++++------------ 3 files changed, 33 insertions(+), 31 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index eb930ef646..02ca5d9b14 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -498,6 +498,15 @@ def enable_disable_extensions( self._configure_pgaudit(False) + if "set_user" in ordered_extensions: + for database in databases: + with self._connect_to_database(database=database) as connection, connection.cursor() as cursor: + try: + cursor.execute(SQL("CREATE EXTENSION IF NOT EXISTS set_user;")) + except psycopg2.Error: + logger.exception(f"Unable to create extension set_user in {database}") + del ordered_extensions["set_user"] + # Enable/disabled the extension in each database. for database in databases: with self._connect_to_database( @@ -693,10 +702,6 @@ def set_up_database(self) -> None: connection = None try: with self._connect_to_database() as connection, connection.cursor() as cursor: - cursor.execute("SELECT TRUE FROM pg_roles WHERE rolname='admin';") - if cursor.fetchone() is not None: - return - # Allow access to the postgres database only to the system users. cursor.execute("REVOKE ALL PRIVILEGES ON DATABASE postgres FROM PUBLIC;") cursor.execute("REVOKE CREATE ON SCHEMA public FROM PUBLIC;") diff --git a/src/charm.py b/src/charm.py index e225e04cd7..2f679cd167 100755 --- a/src/charm.py +++ b/src/charm.py @@ -39,7 +39,7 @@ from charms.postgresql_k8s.v0.postgresql_tls import PostgreSQLTLS from charms.rolling_ops.v0.rollingops import RollingOpsManager, RunWithLock from charms.tempo_coordinator_k8s.v0.charm_tracing import trace_charm -from ops import JujuVersion, main +from ops import main from ops.charm import ( ActionEvent, HookEvent, @@ -180,7 +180,7 @@ def __init__(self, *args): deleted_label=SECRET_DELETED_LABEL, ) - juju_version = JujuVersion.from_environ() + juju_version = self.model.juju_version run_cmd = "/usr/bin/juju-exec" if juju_version.major > 2 else "/usr/bin/juju-run" self._observer = ClusterTopologyObserver(self, run_cmd) self._rotate_logs = RotateLogs(self) diff --git a/src/upgrade.py b/src/upgrade.py index 0773554893..af1eaef400 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -152,25 +152,8 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None: self.charm.unit.status = MaintenanceStatus("refreshing the snap") self.charm._install_snap_packages(packages=SNAP_PACKAGES, refresh=True) - if not self.charm._patroni.start_patroni(): - logger.error("failed to start the database") - self.set_unit_failed() - return - - try: - # Needed to create predefined roles with set_user execution privileges - self.charm.postgresql.enable_disable_extensions( - {"set_user": True}, database="postgres" - ) - except Exception as e: - logger.exception(e) - self.set_unit_failed() - return - - try: - self.charm.postgresql.create_predefined_roles() - except Exception as e: - logger.exception(e) + if not self.charm._patroni.restart_patroni(): + logger.error("failed to restart the database") self.set_unit_failed() return @@ -202,18 +185,18 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None: f" Retry {attempt.retry_state.attempt_number}/6" ) raise Exception() - - self.set_unit_completed() - - # Ensures leader gets its own relation-changed when it upgrades - if self.charm.unit.is_leader(): - self.on_upgrade_changed(event) except RetryError: logger.debug( "Defer on_upgrade_granted: member not ready or not joined the cluster yet" ) event.defer() + self.set_unit_completed() + + # Ensures leader gets its own relation-changed when it upgrades + if self.charm.unit.is_leader(): + self.on_upgrade_changed(event) + @override def pre_upgrade_check(self) -> None: """Runs necessary checks validating the cluster is in a healthy state to upgrade. @@ -267,6 +250,20 @@ def _prepare_upgrade_from_legacy(self) -> None: self.charm.postgresql.set_up_database() self._set_up_new_access_roles_for_legacy() + try: + # Needed to create predefined roles with set_user execution privileges + self.charm.postgresql.enable_disable_extensions( + {"set_user": True}, database="postgres" + ) + except Exception as e: + logger.exception(e) + return + + try: + self.charm.postgresql.create_predefined_roles() + except Exception as e: + logger.exception(e) + def _set_up_new_access_roles_for_legacy(self) -> None: """Create missing access groups and their memberships.""" access_groups = self.charm.postgresql.list_access_groups() From e5adea35512e8460fc0efc927c5598a7efa11048 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Mon, 28 Apr 2025 12:04:14 +0000 Subject: [PATCH 12/21] Appropriately ensure database privileges when more than one relation per database + increase upgrade test timeout --- lib/charms/postgresql_k8s/v0/postgresql.py | 87 +++++++++++++++++++ src/relations/postgresql_provider.py | 12 +++ tests/integration/ha_tests/test_upgrade.py | 2 +- .../ha_tests/test_upgrade_from_stable.py | 2 +- 4 files changed, 101 insertions(+), 2 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 02ca5d9b14..4ae55f11f9 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -132,6 +132,10 @@ class PostgreSQLGrantDatabasePrivilegesToUserError(Exception): """Exception raised when granting database privileges to user.""" +class PostgreSQLEnsureUserPrivilegesToDatabaseError(Exception): + """Exception raised when ensuring user privileges to database.""" + + class PostgreSQL: """Class to encapsulate all operations related to interacting with PostgreSQL instance.""" @@ -233,6 +237,7 @@ def create_database(self, database: str) -> bool: cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_owner"))) cursor.execute(SQL("ALTER DATABASE {} OWNER TO {}").format(Identifier(database), Identifier(f"{database}_owner"))) cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_admin"))) + cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) for user_to_grant_access in [*self.system_users, "charmed_instance_admin"]: @@ -262,6 +267,88 @@ def create_database(self, database: str) -> bool: cursor.close() connection.close() + def ensure_user_privileges_to_database(self, database: str, user: str, relations_accessing_this_database: int) -> None: + """Ensures created user has appropriate privileges to the database.""" + + try: + with self._connect_to_database(database=database) as connection, connection.cursor() as cursor: + cursor.execute("SELECT schema_name FROM information_schema.schemata WHERE schema_name NOT LINKE 'pg_%' and schema_name <> 'information_schema';") + schemas = [row[0] for row in cursor.fetchall()] + statements = self._generate_database_privileges_statements(relations_accessing_this_database, schemas, f"{database}_owner", user) + for statement in statements: + cursor.execute(statement) + except psycopg2.Error as e: + logger.error(f"Failed to ensure user privileges to database: {e}") + raise PostgreSQLEnsureUserPrivilegesToDatabaseError() from e + + def _generate_database_privileges_statements(self, relations_accessing_this_database: int, schemas: list[str], owner_user: str, user: str) -> list[Composed]: + """Generates a list of database privileges statements.""" + statements = [] + if relations_accessing_this_database == 1: + # TODO: eliminate this condition if it is possible to auto-escalate + # all db_admin users to db_owner upon login with set_user and login_hook + # (since then, all resources created will be owned by db_owner) + # necessary to transfer ownership from relation user to database owner role + statements.append( + SQL( + """DO $$ +DECLARE r RECORD; +BEGIN + FOR r IN (SELECT statement FROM (SELECT 1 AS index,'ALTER TABLE '|| schemaname || '."' || tablename ||'" OWNER TO {};' AS statement +FROM pg_tables WHERE NOT schemaname IN ('pg_catalog', 'information_schema') +UNION SELECT 2 AS index,'ALTER SEQUENCE '|| sequence_schema || '."' || sequence_name ||'" OWNER TO {};' AS statement +FROM information_schema.sequences WHERE NOT sequence_schema IN ('pg_catalog', 'information_schema') +UNION SELECT 3 AS index,'ALTER FUNCTION '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement +FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'f' +UNION SELECT 4 AS index,'ALTER PROCEDURE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement +FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'p' +UNION SELECT 5 AS index,'ALTER AGGREGATE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement +FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'a' +UNION SELECT 6 AS index,'ALTER VIEW '|| schemaname || '."' || viewname ||'" OWNER TO {};' AS statement +FROM pg_catalog.pg_views WHERE NOT schemaname IN ('pg_catalog', 'information_schema')) AS statements ORDER BY index) LOOP + EXECUTE format(r.statement); + END LOOP; +END; $$;""" + ).format( + Identifier(owner_user), + Identifier(owner_user), + Identifier(owner_user), + Identifier(owner_user), + Identifier(owner_user), + Identifier(owner_user), + ) + ) + statements.append( + SQL( + "UPDATE pg_catalog.pg_largeobject_metadata\n" + "SET lomowner = (SELECT oid FROM pg_roles WHERE rolname = {})\n" + "WHERE lomowner = (SELECT oid FROM pg_roles WHERE rolname = {});" + ).format(Literal(owner_user), Literal(self.owner_user)) + ) + for schema in schemas: + statements.append( + SQL("ALTER SCHEMA {} OWNER TO {};").format( + Identifier(schema), Identifier(owner_user) + ) + ) + else: + for schema in schemas: + schema = Identifier(schema) + statements.extend([ + SQL("GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA {} TO {};").format( + schema, Identifier(user) + ), + SQL("GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA {} TO {};").format( + schema, Identifier(user) + ), + SQL("GRANT ALL PRIVILEGES ON ALL FUNCTIONS IN SCHEMA {} TO {};").format( + schema, Identifier(user) + ), + SQL("GRANT USAGE ON SCHEMA {} TO {};").format(schema, Identifier(user)), + SQL("GRANT CREATE ON SCHEMA {} TO {};").format(schema, Identifier(user)), + ]) + return statements + def create_user( self, user: str, diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index 18161e97aa..1a6ca646c8 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -16,6 +16,7 @@ PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, PostgreSQLDeleteUserError, + PostgreSQLEnsureUserPrivilegesToDatabaseError, PostgreSQLGetPostgreSQLVersionError, PostgreSQLListUsersError, ) @@ -115,6 +116,16 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: user, password, roles=[*extra_user_roles, f"{database}_owner"] ) + relations_accessing_this_database = 0 + for relation in self.charm.client_relations: + for data in relation.data.values(): + if data.get("database") == database: + relations_accessing_this_database += 1 + + self.charm.postgresql.ensure_user_privileges_to_database( + database, user, relations_accessing_this_database + ) + if database_created: self.charm.update_config() @@ -137,6 +148,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, PostgreSQLGetPostgreSQLVersionError, + PostgreSQLEnsureUserPrivilegesToDatabaseError, ) as e: logger.exception(e) self.charm.unit.status = BlockedStatus( diff --git a/tests/integration/ha_tests/test_upgrade.py b/tests/integration/ha_tests/test_upgrade.py index 7d269e75a7..407e87dd8a 100644 --- a/tests/integration/ha_tests/test_upgrade.py +++ b/tests/integration/ha_tests/test_upgrade.py @@ -26,7 +26,7 @@ logger = logging.getLogger(__name__) -TIMEOUT = 20 * 60 +TIMEOUT = 30 * 60 @pytest.mark.abort_on_fail diff --git a/tests/integration/ha_tests/test_upgrade_from_stable.py b/tests/integration/ha_tests/test_upgrade_from_stable.py index 679c9a639f..be54b8fa60 100644 --- a/tests/integration/ha_tests/test_upgrade_from_stable.py +++ b/tests/integration/ha_tests/test_upgrade_from_stable.py @@ -20,7 +20,7 @@ logger = logging.getLogger(__name__) -TIMEOUT = 900 +TIMEOUT = 25 * 60 @pytest.mark.abort_on_fail From 5decfe8613fad97f498a0e1575e2548a0634d4e6 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Mon, 28 Apr 2025 12:48:28 +0000 Subject: [PATCH 13/21] Fix typo in SQL query --- lib/charms/postgresql_k8s/v0/postgresql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 4ae55f11f9..f6fda92fc4 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -272,7 +272,7 @@ def ensure_user_privileges_to_database(self, database: str, user: str, relations try: with self._connect_to_database(database=database) as connection, connection.cursor() as cursor: - cursor.execute("SELECT schema_name FROM information_schema.schemata WHERE schema_name NOT LINKE 'pg_%' and schema_name <> 'information_schema';") + cursor.execute("SELECT schema_name FROM information_schema.schemata WHERE schema_name NOT LIKE 'pg_%' and schema_name <> 'information_schema';") schemas = [row[0] for row in cursor.fetchall()] statements = self._generate_database_privileges_statements(relations_accessing_this_database, schemas, f"{database}_owner", user) for statement in statements: From 160e50d4cc5e27c7eada5067f7d253e9db376483 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Mon, 28 Apr 2025 13:28:31 +0000 Subject: [PATCH 14/21] Fix incorrectly referenced attribute --- lib/charms/postgresql_k8s/v0/postgresql.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index f6fda92fc4..85a25db837 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -287,8 +287,9 @@ def _generate_database_privileges_statements(self, relations_accessing_this_data if relations_accessing_this_database == 1: # TODO: eliminate this condition if it is possible to auto-escalate # all db_admin users to db_owner upon login with set_user and login_hook - # (since then, all resources created will be owned by db_owner) - # necessary to transfer ownership from relation user to database owner role + # (since then, all resources created will be owned by db_owner). + # this section is necessary to transfer ownership from relation user to + # the database owner role statements.append( SQL( """DO $$ @@ -323,7 +324,7 @@ def _generate_database_privileges_statements(self, relations_accessing_this_data "UPDATE pg_catalog.pg_largeobject_metadata\n" "SET lomowner = (SELECT oid FROM pg_roles WHERE rolname = {})\n" "WHERE lomowner = (SELECT oid FROM pg_roles WHERE rolname = {});" - ).format(Literal(owner_user), Literal(self.owner_user)) + ).format(Literal(owner_user), Literal(owner_user)) ) for schema in schemas: statements.append( From 67d3457440a54ac462062e9f6ae614295b1d3f22 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Mon, 28 Apr 2025 23:16:31 +0000 Subject: [PATCH 15/21] Fix failing status reset after invalid roles status message --- lib/charms/postgresql_k8s/v0/postgresql.py | 14 +++----------- src/relations/postgresql_provider.py | 12 +++++++++++- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 85a25db837..6a912502d6 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -375,25 +375,17 @@ def create_user( roles.append("charmed_instance_admin") roles.remove("admin") - if "CREATEDB" in roles: - createdb_enabled = True - roles.remove("CREATEDB") - if "createdb" in roles: createdb_enabled = True roles.remove("createdb") - if "CREATEROLE" in roles: - createrole_enabled = True - roles.remove("CREATEROLE") - if "createrole" in roles: createrole_enabled = True roles.remove("createrole") - if "SUPERUSER" in roles: - logger.warning("SUPERUSER privileges not allowed via extra-user-roles") - roles.remove("SUPERUSER") + if "superuser" in roles: + logger.warning("superuser privileges not allowed via extra-user-roles") + roles.remove("superuser") try: existing_roles = self.list_roles() diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index 1a6ca646c8..bf182fdc73 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -103,6 +103,10 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: extra_user_roles = self._sanitize_extra_roles(event.extra_user_roles) extra_user_roles.append(ACCESS_GROUP_RELATION) + if self.check_for_invalid_extra_user_roles(event.relation): + self.charm.unit.status = BlockedStatus(INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE) + return + try: # Creates the user and the database for this specific relation. user = f"relation-{event.relation.id}" @@ -317,7 +321,13 @@ def check_for_invalid_extra_user_roles(self, relation_id: int) -> bool: Args: relation_id: current relation to be skipped. """ - valid_roles = self.charm.postgresql.list_roles() + valid_roles = [ + *self.charm.postgresql.list_roles(), + "pgbouncer", + "admin", + "createdb", + "createrole", + ] for relation in self.charm.model.relations.get(self.relation_name, []): if relation.id == relation_id: continue From ab7f5867cb93da9151e5482206f42e2976437eb5 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Tue, 29 Apr 2025 19:38:58 +0000 Subject: [PATCH 16/21] Implement DDL role for a created database --- lib/charms/postgresql_k8s/v0/postgresql.py | 91 +++++++++++++++++++++- src/cluster.py | 2 +- src/constants.py | 2 +- src/relations/postgresql_provider.py | 11 ++- templates/patroni.yml.j2 | 2 +- 5 files changed, 102 insertions(+), 6 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 6a912502d6..833012f0eb 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -136,6 +136,10 @@ class PostgreSQLEnsureUserPrivilegesToDatabaseError(Exception): """Exception raised when ensuring user privileges to database.""" +class PostgreSQLSetUpDDLRoleError(Exception): + """Exception raised when setting up DDL role for a database.""" + + class PostgreSQL: """Class to encapsulate all operations related to interacting with PostgreSQL instance.""" @@ -237,6 +241,7 @@ def create_database(self, database: str) -> bool: cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_owner"))) cursor.execute(SQL("ALTER DATABASE {} OWNER TO {}").format(Identifier(database), Identifier(f"{database}_owner"))) cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_admin"))) + cursor.execute(SQL("GRANT CONNECT ON DATABASE {} TO {};").format(Identifier(database), Identifier(f"{database}_admin"))) cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) @@ -256,7 +261,9 @@ def create_database(self, database: str) -> bool: END; $$ LANGUAGE plpgsql security definer; """).format(Identifier(f"set_user_{database}_owner"), Literal(f"{database}_owner"))) - database_cursor.execute(SQL("ALTER FUNCTION {} OWNER TO {};").format(Identifier(f"set_user_{database}_owner"), Identifier("charmed_dba"))) + database_cursor.execute(SQL("ALTER FUNCTION {} OWNER TO charmed_dba;").format(Identifier(f"set_user_{database}_owner"))) + database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION set_user(text) TO charmed_dba;")) + database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION reset_user() TO charmed_dba;")) database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION {} TO {};").format(Identifier(f"set_user_{database}_owner"), Identifier(f"{database}_admin"))) return True @@ -300,7 +307,7 @@ def _generate_database_privileges_statements(self, relations_accessing_this_data UNION SELECT 2 AS index,'ALTER SEQUENCE '|| sequence_schema || '."' || sequence_name ||'" OWNER TO {};' AS statement FROM information_schema.sequences WHERE NOT sequence_schema IN ('pg_catalog', 'information_schema') UNION SELECT 3 AS index,'ALTER FUNCTION '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement -FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'f' +FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'f' AND p.prosecdef IS false UNION SELECT 4 AS index,'ALTER PROCEDURE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'p' UNION SELECT 5 AS index,'ALTER AGGREGATE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement @@ -339,16 +346,94 @@ def _generate_database_privileges_statements(self, relations_accessing_this_data SQL("GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA {} TO {};").format( schema, Identifier(user) ), + SQL("ALTER DEFAULT PRIVILEGES IN SCHEMA {} GRANT ALL ON TABLES TO {};".format(schema, Identifier(user))), SQL("GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA {} TO {};").format( schema, Identifier(user) ), + SQL("ALTER DEFAULT PRIVILEGES IN SCHEMA {} GRANT ALL ON SEQUENCES TO {};".format(schema, Identifier(user))), SQL("GRANT ALL PRIVILEGES ON ALL FUNCTIONS IN SCHEMA {} TO {};").format( schema, Identifier(user) ), + SQL("ALTER DEFAULT PRIVILEGES IN SCHEMA {} GRANT ALL ON FUNCTION TO {};".format(schema, Identifier(user))), SQL("GRANT USAGE ON SCHEMA {} TO {};").format(schema, Identifier(user)), SQL("GRANT CREATE ON SCHEMA {} TO {};").format(schema, Identifier(user)), ]) return statements + + def set_up_ddl_role(self, database: str, ddl_username: str, ddl_password: str) -> None: + """Sets up the DDL role for the provided database.""" + try: + with self._connect_to_database() as connection, connection.cursor() as cursor: + cursor.execute(SQL("BEGIN;")) + cursor.execute(SQL("SET LOCAL log_statement = 'none';")) + cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE LOGIN NOREPLICATION ENCRYPTED PASSWORD {};").format(Identifier(ddl_username), Literal(ddl_password))) + cursor.execute(SQL("GRANT CONNECT, CREATE ON DATABASE {} TO {};").format(Identifier(database), Identifier(ddl_username))) + cursor.execute(SQL("COMMIT;")) + + with self._connect_to_database(database=database) as connection, connection.cursor() as cursor: + cursor.execute(SQL("""CREATE OR REPLACE FUNCTION update_permissions(S TEXT DEFAULT NULL) +RETURNS VOID AS $$ +DECLARE + r RECORD; + query TEXT; + cur REFCURSOR; + db_name TEXT; + ddl_role TEXT; + admin_role TEXT; + obj TEXT; + objs TEXT[] := ARRAY['TABLES', 'SEQUENCES', 'FUNCTIONS']; + db_query TEXT := 'SELECT current_database()'; + sch_query TEXT := 'SELECT nspname FROM pg_namespace WHERE nspname = %L'; + all_sch_query TEXT := 'SELECT nspname FROM pg_namespace WHERE nspname NOT LIKE ''pg_%'' AND nspname <> ''information_schema'' ORDER BY nspname'; + sch_cre TEXT := 'GRANT USAGE, CREATE ON SCHEMA %I TO %I'; + sch_all TEXT := 'GRANT ALL PRIVILEGES ON SCHEMA %I TO %I'; + obj_all TEXT := 'GRANT ALL PRIVILEGES ON ALL %s IN SCHEMA %I TO %I'; + obj_def TEXT := 'ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT ALL ON %s TO %I'; +BEGIN + IF s IS NULL OR s = '' THEN + query := all_sch_query; + ELSE + query := format(sch_query, s); + END IF; + EXECUTE db_query INTO db_name; + ddl_role = db_name || '_ddl'; + admin_role = db_name || '_owner'; + + OPEN cur FOR EXECUTE query; + LOOP + FETCH cur INTO r; + EXIT WHEN NOT FOUND; + RAISE NOTICE 'Setting permissions on schema %', r; + EXECUTE format(sch_cre, r.nspname, ddl_role); + EXECUTE format(sch_all, r.nspname, admin_role); + FOREACH obj IN ARRAY objs LOOP + EXECUTE format(obj_all, obj, r.nspname, admin_role); + EXECUTE format(obj_def, ddl_role, r.nspname, obj, admin_role); + END LOOP; + END LOOP; + CLOSE cur; +EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'An error occured: % %', SQLERRM, SQLSTATE; +END; +$$ LANGUAGE plpgsql security definer; +""")) + + cursor.execute(SQL("""CREATE OR REPLACE FUNCTION update_permissions_on_create_schema() +RETURNS event_trigger AS $$ +BEGIN + PERFORM update_permissions(); +END; +$$ LANGUAGE plpgsql; +""")) + + cursor.execute(SQL("""CREATE EVENT TRIGGER on_create_schema +ON ddl_command_end +WHEN TAG IN ('CREATE SCHEMA') +EXECUTE FUNCTION update_permissions_on_create_schema(); +""")) + except psycopg2.Error as e: + logger.error(f"Failed to create {database} ddl role: {e}") + raise PostgreSQLSetUpDDLRoleError() from e def create_user( self, @@ -439,6 +524,8 @@ def create_predefined_roles(self) -> None: "GRANT execute ON FUNCTION set_user(text) TO charmed_dba", "GRANT execute ON FUNCTION set_user(text, text) TO charmed_dba", "GRANT execute ON FUNCTION set_user_u(text) TO charmed_dba", + "GRANT execute ON FUNCTION reset_user() TO charmed_dba", + "GRANT execute ON FUNCTION reset_user(text) TO charmed_dba", ], "charmed_instance_admin": [ "CREATE ROLE charmed_instance_admin NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION", diff --git a/src/cluster.py b/src/cluster.py index f94cd12c69..c156342e25 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -656,7 +656,7 @@ def render_patroni_yml_file( nosuperuser_target_allowlist = ( ",".join([ - f"+{role}" + role for role in self.charm.postgresql.list_roles() if not role.startswith("pg_") and role.endswith("_owner") ]) diff --git a/src/constants.py b/src/constants.py index e39e20b924..17f962983e 100644 --- a/src/constants.py +++ b/src/constants.py @@ -32,7 +32,7 @@ SNAP_PACKAGES = [ ( POSTGRESQL_SNAP_NAME, - {"revision": {"aarch64": "160", "x86_64": "166"}}, + {"revision": {"aarch64": "178", "x86_64": "180"}}, ) ] diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index bf182fdc73..963f75f751 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -19,6 +19,7 @@ PostgreSQLEnsureUserPrivilegesToDatabaseError, PostgreSQLGetPostgreSQLVersionError, PostgreSQLListUsersError, + PostgreSQLSetUpDDLRoleError, ) from ops.charm import CharmBase, RelationBrokenEvent, RelationChangedEvent from ops.framework import Object @@ -114,10 +115,17 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: database_created = self.charm.postgresql.create_database(database) + if database_created: + ddl_user = f"{database}_ddl" + ddl_password = new_password() + self.charm.postgresql.set_up_ddl_role(database, ddl_user, ddl_password) + + self.charm.set_secret(APP_SCOPE, f"{database}-ddl-password", ddl_password) + # TODO: change role to database_admin once we determine how to auto-escalate # privileges to database_owner upon login self.charm.postgresql.create_user( - user, password, roles=[*extra_user_roles, f"{database}_owner"] + user, password, roles=[*extra_user_roles, f"{database}_admin"] ) relations_accessing_this_database = 0 @@ -153,6 +161,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: PostgreSQLCreateUserError, PostgreSQLGetPostgreSQLVersionError, PostgreSQLEnsureUserPrivilegesToDatabaseError, + PostgreSQLSetUpDDLRoleError, ) as e: logger.exception(e) self.charm.unit.status = BlockedStatus( diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index 5868fce522..3a186c3d38 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -154,7 +154,7 @@ postgresql: {% endif %} set_user.block_log_statement: on set_user.exit_on_error: on - set_user.superuser_allowlist: '+dba' + set_user.superuser_allowlist: 'charmed_dba' {%- if nosuperuser_target_allowlist %} set_user.nosuperuser_target_allowlist: '{{ nosuperuser_target_allowlist }}' {% endif %} From ad22818e88ab1ed4924962489519f785daf33f1a Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 30 Apr 2025 17:38:23 +0000 Subject: [PATCH 17/21] Flesh out login_hook and database owner role assumption + clean up database privileges for users --- lib/charms/postgresql_k8s/v0/postgresql.py | 181 +++++++++------------ src/charm.py | 5 +- src/cluster.py | 12 -- src/relations/postgresql_provider.py | 14 -- templates/patroni.yml.j2 | 4 +- 5 files changed, 80 insertions(+), 136 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 833012f0eb..e4ae1892bd 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -132,10 +132,6 @@ class PostgreSQLGrantDatabasePrivilegesToUserError(Exception): """Exception raised when granting database privileges to user.""" -class PostgreSQLEnsureUserPrivilegesToDatabaseError(Exception): - """Exception raised when ensuring user privileges to database.""" - - class PostgreSQLSetUpDDLRoleError(Exception): """Exception raised when setting up DDL role for a database.""" @@ -240,12 +236,12 @@ def create_database(self, database: str) -> bool: cursor.execute(SQL("CREATE DATABASE {};").format(Identifier(database))) cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_owner"))) cursor.execute(SQL("ALTER DATABASE {} OWNER TO {}").format(Identifier(database), Identifier(f"{database}_owner"))) - cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_admin"))) + cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION NOINHERIT IN ROLE {};").format(Identifier(f"{database}_admin"), Identifier(f"{database}_owner"))) cursor.execute(SQL("GRANT CONNECT ON DATABASE {} TO {};").format(Identifier(database), Identifier(f"{database}_admin"))) cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) - for user_to_grant_access in [*self.system_users, "charmed_instance_admin"]: + for user_to_grant_access in [*self.system_users, "charmed_instance_admin", "charmed_dba"]: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( Identifier(database), Identifier(user_to_grant_access) @@ -253,18 +249,66 @@ def create_database(self, database: str) -> bool: ) with self._connect_to_database(database=database) as database_connection, database_connection.cursor() as database_cursor: - database_cursor.execute(SQL("""CREATE OR REPLACE FUNCTION {}() RETURNS TEXT AS $$ + database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user_u(text) FROM {};").format(Identifier(f"{database}_owner"))) + database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user_u(text) FROM {};").format(Identifier(f"{database}_admin"))) + + database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user(text) FROM {};").format(Identifier(f"{database}_owner"))) + database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user(text) FROM {};").format(Identifier(f"{database}_admin"))) + + database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user(text, text) FROM {};").format(Identifier(f"{database}_owner"))) + database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user(text, text) FROM {};").format(Identifier(f"{database}_admin"))) + + database_cursor.execute(SQL("""CREATE OR REPLACE FUNCTION login_hook.login() RETURNS VOID AS $$ +DECLARE + ex_state TEXT; + ex_message TEXT; + ex_detail TEXT; + ex_hint TEXT; + ex_context TEXT; + cur_user TEXT; + db_admin_role TEXT; + db_name TEXT; + db_owner_role TEXT; + db_query TEXT := 'SELECT current_database()'; + is_user_admin BOOLEAN; + is_user_admin_query TEXT := 'SELECT EXISTS(SELECT * FROM pg_auth_members a, pg_roles b, pg_roles c WHERE a.roleid = b.oid AND a.member = c.oid AND b.rolname = %L and c.rolname = %L)'; + set_role_query TEXT := 'SET ROLE %L'; BEGIN - -- Restricting search_path when using security definer IS recommended - SET LOCAL search_path = public; - RETURN set_user({}); -END; -$$ LANGUAGE plpgsql security definer; -""").format(Identifier(f"set_user_{database}_owner"), Literal(f"{database}_owner"))) - database_cursor.execute(SQL("ALTER FUNCTION {} OWNER TO charmed_dba;").format(Identifier(f"set_user_{database}_owner"))) - database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION set_user(text) TO charmed_dba;")) - database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION reset_user() TO charmed_dba;")) - database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION {} TO {};").format(Identifier(f"set_user_{database}_owner"), Identifier(f"{database}_admin"))) + IF NOT login_hook.is_executing_login_hook() + THEN + RAISE EXCEPTION 'The login_hook.login() function should only be invoked by the login_hook code'; + END IF; + + cur_user := (SELECT current_user); + + EXECUTE db_query INTO db_name; + db_admin_role = db_name || '_admin'; + + EXECUTE format(is_user_admin_query, db_admin_role, cur_user) INTO is_user_admin; + + BEGIN + IF is_user_admin = true THEN + db_owner_role = db_name || '_owner'; + EXECUTE format(set_role_query, db_owner_role); + END IF; + EXCEPTION + WHEN OTHERS THEN + GET STACKED DIAGNOSTICS ex_state = RETURNED_SQLSTATE + , ex_message = MESSAGE_TEXT + , ex_detail = PG_EXCEPTION_DETAIL + , ex_hint = PG_EXCEPTION_HINT + , ex_context = PG_EXCEPTION_CONTEXT; + RAISE LOG e'Error in login_hook.login()\nsqlstate: %\nmessage : %\ndetail : %\nhint : %\ncontext : %' + , ex_state + , ex_message + , ex_detail + , ex_hint + , ex_context; + END; +END +$$ LANGUAGE plpgsql; +""")) + database_cursor.execute(SQL("GRANT EXECUTE ON FUNCTION login_hook.login() TO PUBLIC;")) return True except psycopg2.Error as e: @@ -273,92 +317,6 @@ def create_database(self, database: str) -> bool: finally: cursor.close() connection.close() - - def ensure_user_privileges_to_database(self, database: str, user: str, relations_accessing_this_database: int) -> None: - """Ensures created user has appropriate privileges to the database.""" - - try: - with self._connect_to_database(database=database) as connection, connection.cursor() as cursor: - cursor.execute("SELECT schema_name FROM information_schema.schemata WHERE schema_name NOT LIKE 'pg_%' and schema_name <> 'information_schema';") - schemas = [row[0] for row in cursor.fetchall()] - statements = self._generate_database_privileges_statements(relations_accessing_this_database, schemas, f"{database}_owner", user) - for statement in statements: - cursor.execute(statement) - except psycopg2.Error as e: - logger.error(f"Failed to ensure user privileges to database: {e}") - raise PostgreSQLEnsureUserPrivilegesToDatabaseError() from e - - def _generate_database_privileges_statements(self, relations_accessing_this_database: int, schemas: list[str], owner_user: str, user: str) -> list[Composed]: - """Generates a list of database privileges statements.""" - statements = [] - if relations_accessing_this_database == 1: - # TODO: eliminate this condition if it is possible to auto-escalate - # all db_admin users to db_owner upon login with set_user and login_hook - # (since then, all resources created will be owned by db_owner). - # this section is necessary to transfer ownership from relation user to - # the database owner role - statements.append( - SQL( - """DO $$ -DECLARE r RECORD; -BEGIN - FOR r IN (SELECT statement FROM (SELECT 1 AS index,'ALTER TABLE '|| schemaname || '."' || tablename ||'" OWNER TO {};' AS statement -FROM pg_tables WHERE NOT schemaname IN ('pg_catalog', 'information_schema') -UNION SELECT 2 AS index,'ALTER SEQUENCE '|| sequence_schema || '."' || sequence_name ||'" OWNER TO {};' AS statement -FROM information_schema.sequences WHERE NOT sequence_schema IN ('pg_catalog', 'information_schema') -UNION SELECT 3 AS index,'ALTER FUNCTION '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement -FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'f' AND p.prosecdef IS false -UNION SELECT 4 AS index,'ALTER PROCEDURE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement -FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'p' -UNION SELECT 5 AS index,'ALTER AGGREGATE '|| nsp.nspname || '."' || p.proname ||'"('||pg_get_function_identity_arguments(p.oid)||') OWNER TO {};' AS statement -FROM pg_proc p JOIN pg_namespace nsp ON p.pronamespace = nsp.oid WHERE NOT nsp.nspname IN ('pg_catalog', 'information_schema') AND p.prokind = 'a' -UNION SELECT 6 AS index,'ALTER VIEW '|| schemaname || '."' || viewname ||'" OWNER TO {};' AS statement -FROM pg_catalog.pg_views WHERE NOT schemaname IN ('pg_catalog', 'information_schema')) AS statements ORDER BY index) LOOP - EXECUTE format(r.statement); - END LOOP; -END; $$;""" - ).format( - Identifier(owner_user), - Identifier(owner_user), - Identifier(owner_user), - Identifier(owner_user), - Identifier(owner_user), - Identifier(owner_user), - ) - ) - statements.append( - SQL( - "UPDATE pg_catalog.pg_largeobject_metadata\n" - "SET lomowner = (SELECT oid FROM pg_roles WHERE rolname = {})\n" - "WHERE lomowner = (SELECT oid FROM pg_roles WHERE rolname = {});" - ).format(Literal(owner_user), Literal(owner_user)) - ) - for schema in schemas: - statements.append( - SQL("ALTER SCHEMA {} OWNER TO {};").format( - Identifier(schema), Identifier(owner_user) - ) - ) - else: - for schema in schemas: - schema = Identifier(schema) - statements.extend([ - SQL("GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA {} TO {};").format( - schema, Identifier(user) - ), - SQL("ALTER DEFAULT PRIVILEGES IN SCHEMA {} GRANT ALL ON TABLES TO {};".format(schema, Identifier(user))), - SQL("GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA {} TO {};").format( - schema, Identifier(user) - ), - SQL("ALTER DEFAULT PRIVILEGES IN SCHEMA {} GRANT ALL ON SEQUENCES TO {};".format(schema, Identifier(user))), - SQL("GRANT ALL PRIVILEGES ON ALL FUNCTIONS IN SCHEMA {} TO {};").format( - schema, Identifier(user) - ), - SQL("ALTER DEFAULT PRIVILEGES IN SCHEMA {} GRANT ALL ON FUNCTION TO {};".format(schema, Identifier(user))), - SQL("GRANT USAGE ON SCHEMA {} TO {};").format(schema, Identifier(user)), - SQL("GRANT CREATE ON SCHEMA {} TO {};").format(schema, Identifier(user)), - ]) - return statements def set_up_ddl_role(self, database: str, ddl_username: str, ddl_password: str) -> None: """Sets up the DDL role for the provided database.""" @@ -395,6 +353,7 @@ def set_up_ddl_role(self, database: str, ddl_username: str, ddl_password: str) - ELSE query := format(sch_query, s); END IF; + EXECUTE db_query INTO db_name; ddl_role = db_name || '_ddl'; admin_role = db_name || '_owner'; @@ -524,8 +483,7 @@ def create_predefined_roles(self) -> None: "GRANT execute ON FUNCTION set_user(text) TO charmed_dba", "GRANT execute ON FUNCTION set_user(text, text) TO charmed_dba", "GRANT execute ON FUNCTION set_user_u(text) TO charmed_dba", - "GRANT execute ON FUNCTION reset_user() TO charmed_dba", - "GRANT execute ON FUNCTION reset_user(text) TO charmed_dba", + "GRANT connect ON DATABASE postgres TO charmed_dba", ], "charmed_instance_admin": [ "CREATE ROLE charmed_instance_admin NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION", @@ -674,6 +632,19 @@ def enable_disable_extensions( logger.exception(f"Unable to create extension set_user in {database}") del ordered_extensions["set_user"] + if "login_hook" in ordered_extensions: + for database in databases: + if database == "postgres": + logger.info("Skipping creating extension login_hook in database postgres") + continue + + with self._connect_to_database(database=database) as connection, connection.cursor() as cursor: + try: + cursor.execute(SQL("CREATE EXTENSION IF NOT EXISTS login_hook;")) + except psycopg2.Error: + logger.exception(f"Unable to create extension login_hook in {database}") + del ordered_extensions["login_hook"] + # Enable/disabled the extension in each database. for database in databases: with self._connect_to_database( diff --git a/src/charm.py b/src/charm.py index 2f679cd167..f193a787b1 100755 --- a/src/charm.py +++ b/src/charm.py @@ -1228,8 +1228,8 @@ def enable_disable_extensions(self, database: str | None = None) -> None: logger.debug("Early exit enable_disable_extensions: standby cluster") return original_status = self.unit.status - # Always want set_user to be enabled - extensions = {"set_user": True} + # Always want set_user and login_hook to be enabled + extensions = {"set_user": True, "login_hook": True} # collect extensions for plugin in self.config.plugin_keys(): enable = self.config[plugin] @@ -1588,6 +1588,7 @@ def _update_admin_password(self, admin_secret_id: str) -> None: # only SYSTEM_USERS with changed passwords are processed, all others ignored updated_passwords = self.get_secret_from_id(secret_id=admin_secret_id) for user, password in list(updated_passwords.items()): + # TODO: generalize and add ddl users here if user not in [*SYSTEM_USERS, BACKUP_USER]: logger.error( f"Can only update system users: {', '.join(SYSTEM_USERS)} not {user}" diff --git a/src/cluster.py b/src/cluster.py index c156342e25..d2a2dad49c 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -654,17 +654,6 @@ def render_patroni_yml_file( with open("templates/patroni.yml.j2") as file: template = Template(file.read()) - nosuperuser_target_allowlist = ( - ",".join([ - role - for role in self.charm.postgresql.list_roles() - if not role.startswith("pg_") and role.endswith("_owner") - ]) - if self.charm.is_cluster_initialised - and self.charm.primary_endpoint is not None - and self.charm._can_connect_to_postgresql - else None - ) ldap_params = self.charm.get_ldap_parameters() # Render the template file with the correct values. @@ -707,7 +696,6 @@ def render_patroni_yml_file( raft_password=self.raft_password, ldap_parameters=self._dict_to_hba_string(ldap_params), patroni_password=self.patroni_password, - nosuperuser_target_allowlist=nosuperuser_target_allowlist, ) self.render_file(f"{PATRONI_CONF_PATH}/patroni.yaml", rendered, 0o600) diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index 963f75f751..a47cfc7175 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -16,7 +16,6 @@ PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, PostgreSQLDeleteUserError, - PostgreSQLEnsureUserPrivilegesToDatabaseError, PostgreSQLGetPostgreSQLVersionError, PostgreSQLListUsersError, PostgreSQLSetUpDDLRoleError, @@ -122,22 +121,10 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: self.charm.set_secret(APP_SCOPE, f"{database}-ddl-password", ddl_password) - # TODO: change role to database_admin once we determine how to auto-escalate - # privileges to database_owner upon login self.charm.postgresql.create_user( user, password, roles=[*extra_user_roles, f"{database}_admin"] ) - relations_accessing_this_database = 0 - for relation in self.charm.client_relations: - for data in relation.data.values(): - if data.get("database") == database: - relations_accessing_this_database += 1 - - self.charm.postgresql.ensure_user_privileges_to_database( - database, user, relations_accessing_this_database - ) - if database_created: self.charm.update_config() @@ -160,7 +147,6 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, PostgreSQLGetPostgreSQLVersionError, - PostgreSQLEnsureUserPrivilegesToDatabaseError, PostgreSQLSetUpDDLRoleError, ) as e: logger.exception(e) diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index 3a186c3d38..a0acbdd435 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -135,6 +135,7 @@ postgresql: data_dir: {{ data_path }} parameters: shared_preload_libraries: 'timescaledb,pgaudit,set_user' + session_preload_libraries: 'login_hook' {%- if enable_pgbackrest_archiving %} archive_command: 'pgbackrest {{ pgbackrest_configuration_file }} --stanza={{ stanza }} archive-push %p' {% else %} @@ -155,9 +156,6 @@ postgresql: set_user.block_log_statement: on set_user.exit_on_error: on set_user.superuser_allowlist: 'charmed_dba' - {%- if nosuperuser_target_allowlist %} - set_user.nosuperuser_target_allowlist: '{{ nosuperuser_target_allowlist }}' - {% endif %} pgpass: /tmp/pgpass pg_hba: - local all backup peer map=operator From d5e713090a497de404cbe72b3c9705ab2cec2f6a Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Wed, 30 Apr 2025 20:37:05 +0000 Subject: [PATCH 18/21] Allow database_owner role to createdb/createrole --- lib/charms/postgresql_k8s/v0/postgresql.py | 29 ++++++++++++++++++---- src/relations/postgresql_provider.py | 13 +++++----- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index e4ae1892bd..3cf2425bab 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -337,12 +337,13 @@ def set_up_ddl_role(self, database: str, ddl_username: str, ddl_password: str) - cur REFCURSOR; db_name TEXT; ddl_role TEXT; - admin_role TEXT; + owner_role TEXT; obj TEXT; objs TEXT[] := ARRAY['TABLES', 'SEQUENCES', 'FUNCTIONS']; db_query TEXT := 'SELECT current_database()'; sch_query TEXT := 'SELECT nspname FROM pg_namespace WHERE nspname = %L'; all_sch_query TEXT := 'SELECT nspname FROM pg_namespace WHERE nspname NOT LIKE ''pg_%'' AND nspname <> ''information_schema'' ORDER BY nspname'; + sch_rev TEXT := 'REVOKE ALL ON SCHEMA %I FROM %I'; sch_cre TEXT := 'GRANT USAGE, CREATE ON SCHEMA %I TO %I'; sch_all TEXT := 'GRANT ALL PRIVILEGES ON SCHEMA %I TO %I'; obj_all TEXT := 'GRANT ALL PRIVILEGES ON ALL %s IN SCHEMA %I TO %I'; @@ -356,19 +357,29 @@ def set_up_ddl_role(self, database: str, ddl_username: str, ddl_password: str) - EXECUTE db_query INTO db_name; ddl_role = db_name || '_ddl'; - admin_role = db_name || '_owner'; + owner_role = db_name || '_owner'; OPEN cur FOR EXECUTE query; LOOP FETCH cur INTO r; EXIT WHEN NOT FOUND; RAISE NOTICE 'Setting permissions on schema %', r; + EXECUTE format(sch_rev, r.nspname, ddl_role); EXECUTE format(sch_cre, r.nspname, ddl_role); - EXECUTE format(sch_all, r.nspname, admin_role); + EXECUTE format(sch_all, r.nspname, owner_role); + + EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE ON TABLES FROM PUBLIC', ddl_role, r.nspname); + EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I REVOKE SELECT, UPDATE ON SEQUENCES FROM PUBLIC', ddl_role, r.nspname); + EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC', ddl_role, r.nspname); + FOREACH obj IN ARRAY objs LOOP - EXECUTE format(obj_all, obj, r.nspname, admin_role); - EXECUTE format(obj_def, ddl_role, r.nspname, obj, admin_role); + EXECUTE format(obj_all, obj, r.nspname, owner_role); + EXECUTE format(obj_def, ddl_role, r.nspname, obj, owner_role); END LOOP; + + EXECUTE format('REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE ON ALL TABLES IN SCHEMA %I FROM %I', r.nspname, ddl_role); + EXECUTE format('REVOKE SELECT, UPDATE ON ALL SEQUENCES IN SCHEMA %I FROM %I', r.nspname, ddl_role); + EXECUTE format('REVOKE EXECUTE ON ALL FUNCTIONS IN SCHEMA %I FROM %I', r.nspname, ddl_role); END LOOP; CLOSE cur; EXCEPTION WHEN OTHERS THEN @@ -399,6 +410,7 @@ def create_user( user: str, password: Optional[str] = None, roles: Optional[List[str]] = None, + database: Optional[str] = None, ) -> None: """Creates a database user. @@ -450,6 +462,13 @@ def create_user( if roles: user_query += f"{', '.join(roles)}" + # TODO: revoke createdb and/or createrole attributes when no longer needed (in delete_user) + if createdb_enabled and database: + cursor.execute(SQL("ALTER ROLE {} WITH CREATEDB;").format(Identifier(f"{database}_owner"))) + + if createdb_enabled and database: + cursor.execute(SQL("ALTER ROLE {} WITH CREATEROLE;").format(Identifier(f"{database}_owner"))) + cursor.execute(SQL("BEGIN;")) cursor.execute(SQL("SET LOCAL log_statement = 'none';")) cursor.execute(SQL(f"{user_query};").format(Identifier(user))) diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index a47cfc7175..d94567b78e 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -114,15 +114,16 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: database_created = self.charm.postgresql.create_database(database) - if database_created: - ddl_user = f"{database}_ddl" - ddl_password = new_password() - self.charm.postgresql.set_up_ddl_role(database, ddl_user, ddl_password) + # TODO: possibly delete if DDL role needs to be postponed to a future PR + # if database_created: + # ddl_user = f"{database}_ddl" + # ddl_password = new_password() + # self.charm.postgresql.set_up_ddl_role(database, ddl_user, ddl_password) - self.charm.set_secret(APP_SCOPE, f"{database}-ddl-password", ddl_password) + # self.charm.set_secret(APP_SCOPE, f"{database}-ddl-password", ddl_password) self.charm.postgresql.create_user( - user, password, roles=[*extra_user_roles, f"{database}_admin"] + user, password, roles=[*extra_user_roles, f"{database}_admin"], database=database ) if database_created: From 1d781c943642dc745a90827c0ad2a46751248227 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Thu, 1 May 2025 15:22:14 +0000 Subject: [PATCH 19/21] Allow db_admin roles to SELECT on tables/sequences + EXECUTE on functions created by db_owner --- lib/charms/postgresql_k8s/v0/postgresql.py | 34 ++++++++++++---------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 3cf2425bab..15e7d928ac 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -234,13 +234,14 @@ def create_database(self, database: str) -> bool: return False cursor.execute(SQL("CREATE DATABASE {};").format(Identifier(database))) + cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) + cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION;").format(Identifier(f"{database}_owner"))) cursor.execute(SQL("ALTER DATABASE {} OWNER TO {}").format(Identifier(database), Identifier(f"{database}_owner"))) + cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION NOINHERIT IN ROLE {};").format(Identifier(f"{database}_admin"), Identifier(f"{database}_owner"))) cursor.execute(SQL("GRANT CONNECT ON DATABASE {} TO {};").format(Identifier(database), Identifier(f"{database}_admin"))) - cursor.execute(SQL("REVOKE ALL PRIVILEGES ON DATABASE {} FROM PUBLIC;").format(Identifier(database))) - for user_to_grant_access in [*self.system_users, "charmed_instance_admin", "charmed_dba"]: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( @@ -258,6 +259,10 @@ def create_database(self, database: str) -> bool: database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user(text, text) FROM {};").format(Identifier(f"{database}_owner"))) database_cursor.execute(SQL("REVOKE EXECUTE ON FUNCTION set_user(text, text) FROM {};").format(Identifier(f"{database}_admin"))) + database_cursor.execute(SQL("ALTER DEFAULT PRIVILEGES FOR ROLE {} GRANT SELECT ON TABLES TO {};").format(Identifier(f"{database}_owner"), Identifier(f"{database}_admin"))) + database_cursor.execute(SQL("ALTER DEFAULT PRIVILEGES FOR ROLE {} GRANT EXECUTE ON FUNCTIONS TO {};").format(Identifier(f"{database}_owner"), Identifier(f"{database}_admin"))) + database_cursor.execute(SQL("ALTER DEFAULT PRIVILEGES FOR ROLE {} GRANT SELECT ON SEQUENCES TO {};").format(Identifier(f"{database}_owner"), Identifier(f"{database}_admin"))) + database_cursor.execute(SQL("""CREATE OR REPLACE FUNCTION login_hook.login() RETURNS VOID AS $$ DECLARE ex_state TEXT; @@ -269,10 +274,7 @@ def create_database(self, database: str) -> bool: db_admin_role TEXT; db_name TEXT; db_owner_role TEXT; - db_query TEXT := 'SELECT current_database()'; is_user_admin BOOLEAN; - is_user_admin_query TEXT := 'SELECT EXISTS(SELECT * FROM pg_auth_members a, pg_roles b, pg_roles c WHERE a.roleid = b.oid AND a.member = c.oid AND b.rolname = %L and c.rolname = %L)'; - set_role_query TEXT := 'SET ROLE %L'; BEGIN IF NOT login_hook.is_executing_login_hook() THEN @@ -281,15 +283,15 @@ def create_database(self, database: str) -> bool: cur_user := (SELECT current_user); - EXECUTE db_query INTO db_name; + EXECUTE 'SELECT current_database()' INTO db_name; db_admin_role = db_name || '_admin'; - EXECUTE format(is_user_admin_query, db_admin_role, cur_user) INTO is_user_admin; + EXECUTE format('SELECT EXISTS(SELECT * FROM pg_auth_members a, pg_roles b, pg_roles c WHERE a.roleid = b.oid AND a.member = c.oid AND b.rolname = %L and c.rolname = %L)', db_admin_role, cur_user) INTO is_user_admin; BEGIN - IF is_user_admin = true THEN + IF is_user_admin THEN db_owner_role = db_name || '_owner'; - EXECUTE format(set_role_query, db_owner_role); + EXECUTE format('SET ROLE %L', db_owner_role); END IF; EXCEPTION WHEN OTHERS THEN @@ -458,21 +460,21 @@ def create_user( user_query = "ALTER ROLE {} " else: user_query = "CREATE ROLE {} " - user_query += f"WITH LOGIN{' CREATEDB' if createdb_enabled else ''}{' CREATEROLE' if createrole_enabled else ''} ENCRYPTED PASSWORD '{password}' {'IN ROLE ' if roles else ''}" + user_query += f"WITH LOGIN{' CREATEDB' if createdb_enabled else ''}{' CREATEROLE' if createrole_enabled else ''} ENCRYPTED PASSWORD '{password}'{' IN ROLE ' if roles else ''}" if roles: user_query += f"{', '.join(roles)}" + cursor.execute(SQL("BEGIN;")) + cursor.execute(SQL("SET LOCAL log_statement = 'none';")) + cursor.execute(SQL(f"{user_query};").format(Identifier(user))) + cursor.execute(SQL("COMMIT;")) + # TODO: revoke createdb and/or createrole attributes when no longer needed (in delete_user) if createdb_enabled and database: cursor.execute(SQL("ALTER ROLE {} WITH CREATEDB;").format(Identifier(f"{database}_owner"))) if createdb_enabled and database: cursor.execute(SQL("ALTER ROLE {} WITH CREATEROLE;").format(Identifier(f"{database}_owner"))) - - cursor.execute(SQL("BEGIN;")) - cursor.execute(SQL("SET LOCAL log_statement = 'none';")) - cursor.execute(SQL(f"{user_query};").format(Identifier(user))) - cursor.execute(SQL("COMMIT;")) except psycopg2.Error as e: logger.error(f"Failed to create user: {e}") raise PostgreSQLCreateUserError() from e @@ -833,7 +835,7 @@ def list_users_from_relation(self) -> Set[str]: try: with self._connect_to_database() as connection, connection.cursor() as cursor: cursor.execute( - "SELECT usename FROM pg_catalog.pg_user WHERE usename LIKE 'relation_id_%';" + "SELECT usename FROM pg_catalog.pg_user WHERE usename LIKE 'relation-%';" ) usernames = cursor.fetchall() return {username[0] for username in usernames} From 539b82f31c2f61c3a824a831f3fc224ea3c4ae53 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Thu, 1 May 2025 19:37:59 +0000 Subject: [PATCH 20/21] Update snap (with login_hook) + bump postgresql lib major version + implement integration tests for predefined roles --- .../postgresql_k8s/{v0 => v1}/postgresql.py | 4 +- src/charm.py | 4 +- src/constants.py | 2 +- src/relations/postgresql_provider.py | 10 +- src/upgrade.py | 2 +- tests/integration/helpers.py | 8 +- tests/integration/test_predefined_roles.py | 220 ++++++++++++++++++ .../spread/test_predefined_roles.py/task.yaml | 7 + 8 files changed, 247 insertions(+), 10 deletions(-) rename lib/charms/postgresql_k8s/{v0 => v1}/postgresql.py (99%) create mode 100644 tests/integration/test_predefined_roles.py create mode 100644 tests/spread/test_predefined_roles.py/task.yaml diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v1/postgresql.py similarity index 99% rename from lib/charms/postgresql_k8s/v0/postgresql.py rename to lib/charms/postgresql_k8s/v1/postgresql.py index 15e7d928ac..b2f02d3e87 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v1/postgresql.py @@ -31,11 +31,11 @@ LIBID = "24ee217a54e840a598ff21a079c3e678" # Increment this major API version when introducing breaking changes -LIBAPI = 0 +LIBAPI = 1 # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 51 +LIBPATCH = 0 # Groups to distinguish HBA access ACCESS_GROUP_IDENTITY = "identity_access" diff --git a/src/charm.py b/src/charm.py index f193a787b1..6794dc39e9 100755 --- a/src/charm.py +++ b/src/charm.py @@ -23,7 +23,8 @@ from charms.data_platform_libs.v0.data_models import TypedCharmBase from charms.grafana_agent.v0.cos_agent import COSAgentProvider, charm_tracing_config from charms.operator_libs_linux.v2 import snap -from charms.postgresql_k8s.v0.postgresql import ( +from charms.postgresql_k8s.v0.postgresql_tls import PostgreSQLTLS +from charms.postgresql_k8s.v1.postgresql import ( ACCESS_GROUP_IDENTITY, ACCESS_GROUPS, REQUIRED_PLUGINS, @@ -36,7 +37,6 @@ PostgreSQLListUsersError, PostgreSQLUpdateUserPasswordError, ) -from charms.postgresql_k8s.v0.postgresql_tls import PostgreSQLTLS from charms.rolling_ops.v0.rollingops import RollingOpsManager, RunWithLock from charms.tempo_coordinator_k8s.v0.charm_tracing import trace_charm from ops import main diff --git a/src/constants.py b/src/constants.py index 17f962983e..6342e12238 100644 --- a/src/constants.py +++ b/src/constants.py @@ -32,7 +32,7 @@ SNAP_PACKAGES = [ ( POSTGRESQL_SNAP_NAME, - {"revision": {"aarch64": "178", "x86_64": "180"}}, + {"revision": {"aarch64": "181", "x86_64": "182"}}, ) ] diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index d94567b78e..aad272cc49 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -9,7 +9,7 @@ DatabaseProvides, DatabaseRequestedEvent, ) -from charms.postgresql_k8s.v0.postgresql import ( +from charms.postgresql_k8s.v1.postgresql import ( ACCESS_GROUP_RELATION, ACCESS_GROUPS, INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE, @@ -154,7 +154,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: self.charm.unit.status = BlockedStatus( e.message if issubclass(type(e), PostgreSQLCreateUserError) and e.message is not None - else f"Failed to initialize {self.relation_name} relation" + else f"Failed to initialize relation {self.relation_name}" ) def _on_relation_broken(self, event: RelationBrokenEvent) -> None: @@ -290,6 +290,12 @@ def _update_unit_status(self, relation: Relation) -> None: ) and not self.check_for_invalid_extra_user_roles(relation.id): self.charm.unit.status = ActiveStatus() + if ( + self.charm.is_blocked + and "Failed to initialize relation" in self.charm.unit.status.message + ): + self.charm.unit.status = ActiveStatus() + self._update_unit_status_on_blocking_endpoint_simultaneously() def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: diff --git a/src/upgrade.py b/src/upgrade.py index af1eaef400..0a72d21d1e 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -12,7 +12,7 @@ DependencyModel, UpgradeGrantedEvent, ) -from charms.postgresql_k8s.v0.postgresql import ACCESS_GROUPS +from charms.postgresql_k8s.v1.postgresql import ACCESS_GROUPS from ops.model import MaintenanceStatus, RelationDataContent, WaitingStatus from pydantic import BaseModel from tenacity import RetryError, Retrying, stop_after_attempt, wait_fixed diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index e3fead648b..d123edf474 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -276,18 +276,22 @@ def count_switchovers(ops_test: OpsTest, unit_name: str) -> int: return len(switchover_history_info.json()) -def db_connect(host: str, password: str) -> psycopg2.extensions.connection: +def db_connect( + host: str, password: str, database: str = "postgres", user: str = "operator" +) -> psycopg2.extensions.connection: """Returns psycopg2 connection object linked to postgres db in the given host. Args: host: the IP of the postgres host password: operator user password + database: the database to connect to + user: the user to connect with Returns: psycopg2 connection object linked to postgres db, under "operator" user. """ return psycopg2.connect( - f"dbname='postgres' user='operator' host='{host}' password='{password}' connect_timeout=10" + f"dbname='{database}' user='{user}' host='{host}' password='{password}' connect_timeout=10" ) diff --git a/tests/integration/test_predefined_roles.py b/tests/integration/test_predefined_roles.py new file mode 100644 index 0000000000..88aa620a64 --- /dev/null +++ b/tests/integration/test_predefined_roles.py @@ -0,0 +1,220 @@ +#!/usr/bin/env python3 +# Copyright 2025 Canonical Ltd. +# See LICENSE file for licensing details. + +import asyncio +import logging + +import psycopg2 +import pytest +from pytest_operator.plugin import OpsTest + +from .helpers import ( + CHARM_BASE, + DATABASE_APP_NAME, +) +from .new_relations.helpers import build_connection_string + +logger = logging.getLogger(__name__) + +TIMEOUT = 15 * 60 +DATA_INTEGRATOR_APP_NAME = "data-integrator" + +# NOTE: We are unable to test set_user_u('operator') as dba_user because psycopg2 +# runs every query in a transaction and running set_user() is not supported in transactions. + + +@pytest.mark.abort_on_fail +@pytest.mark.skip_if_deployed +async def test_deploy(ops_test: OpsTest, charm: str): + """Deploy the postgresql charm.""" + async with ops_test.fast_forward("10s"): + await asyncio.gather( + ops_test.model.deploy( + charm, + application_name=DATABASE_APP_NAME, + num_units=2, + base=CHARM_BASE, + config={"profile": "testing"}, + ), + ops_test.model.deploy( + DATA_INTEGRATOR_APP_NAME, + base=CHARM_BASE, + ), + ) + + await ops_test.model.wait_for_idle( + apps=[DATABASE_APP_NAME], status="active", timeout=TIMEOUT + ) + assert ops_test.model.applications[DATABASE_APP_NAME].units[0].workload_status == "active" + + await ops_test.model.wait_for_idle( + apps=[DATA_INTEGRATOR_APP_NAME], status="blocked", timeout=(5 * 60) + ) + + +@pytest.mark.abort_on_fail +async def test_charmed_read_role(ops_test: OpsTest): + """Test the charmed_read predefined role.""" + await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({ + "database-name": "charmed_read_database", + "extra-user-roles": "charmed_read", + }) + await ops_test.model.add_relation(DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME) + await ops_test.model.wait_for_idle( + apps=[DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME], status="active" + ) + + connection_string = await build_connection_string( + ops_test, + DATA_INTEGRATOR_APP_NAME, + "postgresql", + database="charmed_read_database", + ) + + with psycopg2.connect(connection_string) as connection: + connection.autocommit = True + + with connection.cursor() as cursor: + cursor.execute("CREATE TABLE test_table (id SERIAL PRIMARY KEY, data TEXT);") + cursor.execute("INSERT INTO test_table (data) VALUES ('test_data'), ('test_data_2');") + + # reset role from charmed_read_database_owner to relation user first + cursor.execute("RESET ROLE;") + + cursor.execute( + "SELECT table_name FROM information_schema.tables WHERE table_name NOT LIKE 'pg_%' AND table_name NOT LIKE 'sql_%' AND table_type <> 'VIEW';" + ) + tables = [row[0] for row in cursor.fetchall()] + assert tables == ["test_table"], "Unexpected tables in the database" + + cursor.execute("SELECT data FROM test_table;") + data = sorted([row[0] for row in cursor.fetchall()]) + assert data == sorted(["test_data", "test_data_2"]), ( + "Unexpected data in charmed_read_database with charmed_read role" + ) + + with psycopg2.connect(connection_string) as connection: + connection.autocommit = True + + with connection.cursor() as cursor: + try: + # reset role from charmed_read_database_owner to relation user first + cursor.execute("RESET ROLE;") + + cursor.execute("CREATE TABLE test_table_2 (id SERIAL PRIMARY KEY, data TEXT);") + assert False, "Able to write to charmed_read_database with charmed_read role" + except psycopg2.errors.InsufficientPrivilege: + pass + + await ops_test.model.applications[DATABASE_APP_NAME].remove_relation( + f"{DATABASE_APP_NAME}:database", f"{DATA_INTEGRATOR_APP_NAME}:postgresql" + ) + await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked") + + +@pytest.mark.abort_on_fail +async def test_charmed_dml_role(ops_test: OpsTest): + """Test the charmed_dml role.""" + await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({ + "database-name": "charmed_dml_database", + "extra-user-roles": "charmed_dml", + }) + await ops_test.model.add_relation(DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME) + await ops_test.model.wait_for_idle( + apps=[DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME], status="active" + ) + + connection_string = await build_connection_string( + ops_test, + DATA_INTEGRATOR_APP_NAME, + "postgresql", + database="charmed_dml_database", + ) + + with psycopg2.connect(connection_string) as connection: + connection.autocommit = True + + with connection.cursor() as cursor: + cursor.execute("CREATE TABLE test_table (id SERIAL PRIMARY KEY, data TEXT);") + + # reset role from charmed_dml_database_owner to relation user first + cursor.execute("RESET ROLE;") + + cursor.execute("INSERT INTO test_table (data) VALUES ('test_data'), ('test_data_2');") + + cursor.execute("SELECT data FROM test_table;") + data = sorted([row[0] for row in cursor.fetchall()]) + assert data == sorted(["test_data", "test_data_2"]), ( + "Unexpected data in charmed_dml_database with charmed_dml role" + ) + + await ops_test.model.applications[DATABASE_APP_NAME].remove_relation( + f"{DATABASE_APP_NAME}:database", f"{DATA_INTEGRATOR_APP_NAME}:postgresql" + ) + await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked") + + +@pytest.mark.abort_on_fail +async def test_can_read_data_on_replica(ops_test: OpsTest): + """Test to ensure a relation user can read from a replica. + + login_hook does not activate on a replica, and thus db_admin has read privileges + to the tables in the database. This test ensure this privilege exists + """ + await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({ + "database-name": "read_on_replica", + }) + await ops_test.model.add_relation(DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME) + await ops_test.model.wait_for_idle( + apps=[DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME], status="active" + ) + + connection_string = await build_connection_string( + ops_test, + DATA_INTEGRATOR_APP_NAME, + "postgresql", + database="read_on_replica", + ) + + with psycopg2.connect(connection_string) as connection: + connection.autocommit = True + + with connection.cursor() as cursor: + cursor.execute("CREATE TABLE test_table (id SERIAL PRIMARY KEY, data TEXT);") + cursor.execute("INSERT INTO test_table (data) VALUES ('test_data'), ('test_data_2');") + + cursor.execute("SELECT data FROM test_table;") + data = sorted([row[0] for row in cursor.fetchall()]) + assert data == sorted(["test_data", "test_data_2"]), ( + "Unexpected data in read_on_replica database" + ) + + replica_connection_string = await build_connection_string( + ops_test, + DATA_INTEGRATOR_APP_NAME, + "postgresql", + database="read_on_replica", + read_only_endpoint=True, + ) + + with psycopg2.connect(replica_connection_string) as connection: + connection.autocommit = True + + with connection.cursor() as cursor: + cursor.execute("SELECT data FROM test_table;") + data = sorted([row[0] for row in cursor.fetchall()]) + assert data == sorted(["test_data", "test_data_2"]), ( + "Unexpected data in read_on_replica database" + ) + + try: + cursor.execute("CREATE TABLE test_table_2 (id SERIAL PRIMARY KEY, data TEXT);") + assert False, "Able to create a table on a replica" + except psycopg2.errors.ReadOnlySqlTransaction: + pass + + await ops_test.model.applications[DATABASE_APP_NAME].remove_relation( + f"{DATABASE_APP_NAME}:database", f"{DATA_INTEGRATOR_APP_NAME}:postgresql" + ) + await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked") diff --git a/tests/spread/test_predefined_roles.py/task.yaml b/tests/spread/test_predefined_roles.py/task.yaml new file mode 100644 index 0000000000..bbafb2a6bf --- /dev/null +++ b/tests/spread/test_predefined_roles.py/task.yaml @@ -0,0 +1,7 @@ +summary: test_predefined_roles.py +environment: + TEST_MODULE: test_predefined_roles.py +execute: | + tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results" +artifacts: + - allure-results From e63be2c1c0faa473c7becdfbe71d7962c58fcf47 Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Fri, 2 May 2025 11:06:33 +0000 Subject: [PATCH 21/21] Fix unit tests + minor bugfixes and polish --- lib/charms/postgresql_k8s/v1/postgresql.py | 144 +++++---------------- src/backups.py | 4 +- src/charm.py | 31 +++-- src/cluster.py | 2 + src/constants.py | 2 + src/relations/postgresql_provider.py | 8 -- src/upgrade.py | 5 +- templates/patroni.yml.j2 | 2 +- tests/integration/test_predefined_roles.py | 2 +- tests/unit/conftest.py | 4 +- tests/unit/test_backups.py | 4 +- tests/unit/test_charm.py | 67 +++------- tests/unit/test_cluster.py | 1 + tests/unit/test_postgresql_provider.py | 18 +-- tests/unit/test_upgrade.py | 6 +- 15 files changed, 88 insertions(+), 212 deletions(-) diff --git a/lib/charms/postgresql_k8s/v1/postgresql.py b/lib/charms/postgresql_k8s/v1/postgresql.py index b2f02d3e87..01029a5019 100644 --- a/lib/charms/postgresql_k8s/v1/postgresql.py +++ b/lib/charms/postgresql_k8s/v1/postgresql.py @@ -42,6 +42,13 @@ ACCESS_GROUP_INTERNAL = "internal_access" ACCESS_GROUP_RELATION = "relation_access" +ROLE_STATS = "charmed_stats" +ROLE_READ = "charmed_read" +ROLE_DML = "charmed_dml" +ROLE_BACKUP = "charmed_backup" +ROLE_DBA = "charmed_dba" +ROLE_INSTANCE_ADMIN = "charmed_instance_admin" + # List of access groups to filter role assignments by ACCESS_GROUPS = [ ACCESS_GROUP_IDENTITY, @@ -242,7 +249,7 @@ def create_database(self, database: str) -> bool: cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION NOINHERIT IN ROLE {};").format(Identifier(f"{database}_admin"), Identifier(f"{database}_owner"))) cursor.execute(SQL("GRANT CONNECT ON DATABASE {} TO {};").format(Identifier(database), Identifier(f"{database}_admin"))) - for user_to_grant_access in [*self.system_users, "charmed_instance_admin", "charmed_dba"]: + for user_to_grant_access in [*self.system_users, ROLE_INSTANCE_ADMIN, ROLE_DBA]: cursor.execute( SQL("GRANT ALL PRIVILEGES ON DATABASE {} TO {};").format( Identifier(database), Identifier(user_to_grant_access) @@ -319,93 +326,6 @@ def create_database(self, database: str) -> bool: finally: cursor.close() connection.close() - - def set_up_ddl_role(self, database: str, ddl_username: str, ddl_password: str) -> None: - """Sets up the DDL role for the provided database.""" - try: - with self._connect_to_database() as connection, connection.cursor() as cursor: - cursor.execute(SQL("BEGIN;")) - cursor.execute(SQL("SET LOCAL log_statement = 'none';")) - cursor.execute(SQL("CREATE ROLE {} NOSUPERUSER NOCREATEDB NOCREATEROLE LOGIN NOREPLICATION ENCRYPTED PASSWORD {};").format(Identifier(ddl_username), Literal(ddl_password))) - cursor.execute(SQL("GRANT CONNECT, CREATE ON DATABASE {} TO {};").format(Identifier(database), Identifier(ddl_username))) - cursor.execute(SQL("COMMIT;")) - - with self._connect_to_database(database=database) as connection, connection.cursor() as cursor: - cursor.execute(SQL("""CREATE OR REPLACE FUNCTION update_permissions(S TEXT DEFAULT NULL) -RETURNS VOID AS $$ -DECLARE - r RECORD; - query TEXT; - cur REFCURSOR; - db_name TEXT; - ddl_role TEXT; - owner_role TEXT; - obj TEXT; - objs TEXT[] := ARRAY['TABLES', 'SEQUENCES', 'FUNCTIONS']; - db_query TEXT := 'SELECT current_database()'; - sch_query TEXT := 'SELECT nspname FROM pg_namespace WHERE nspname = %L'; - all_sch_query TEXT := 'SELECT nspname FROM pg_namespace WHERE nspname NOT LIKE ''pg_%'' AND nspname <> ''information_schema'' ORDER BY nspname'; - sch_rev TEXT := 'REVOKE ALL ON SCHEMA %I FROM %I'; - sch_cre TEXT := 'GRANT USAGE, CREATE ON SCHEMA %I TO %I'; - sch_all TEXT := 'GRANT ALL PRIVILEGES ON SCHEMA %I TO %I'; - obj_all TEXT := 'GRANT ALL PRIVILEGES ON ALL %s IN SCHEMA %I TO %I'; - obj_def TEXT := 'ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I GRANT ALL ON %s TO %I'; -BEGIN - IF s IS NULL OR s = '' THEN - query := all_sch_query; - ELSE - query := format(sch_query, s); - END IF; - - EXECUTE db_query INTO db_name; - ddl_role = db_name || '_ddl'; - owner_role = db_name || '_owner'; - - OPEN cur FOR EXECUTE query; - LOOP - FETCH cur INTO r; - EXIT WHEN NOT FOUND; - RAISE NOTICE 'Setting permissions on schema %', r; - EXECUTE format(sch_rev, r.nspname, ddl_role); - EXECUTE format(sch_cre, r.nspname, ddl_role); - EXECUTE format(sch_all, r.nspname, owner_role); - - EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE ON TABLES FROM PUBLIC', ddl_role, r.nspname); - EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I REVOKE SELECT, UPDATE ON SEQUENCES FROM PUBLIC', ddl_role, r.nspname); - EXECUTE format('ALTER DEFAULT PRIVILEGES FOR ROLE %I IN SCHEMA %I REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC', ddl_role, r.nspname); - - FOREACH obj IN ARRAY objs LOOP - EXECUTE format(obj_all, obj, r.nspname, owner_role); - EXECUTE format(obj_def, ddl_role, r.nspname, obj, owner_role); - END LOOP; - - EXECUTE format('REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE ON ALL TABLES IN SCHEMA %I FROM %I', r.nspname, ddl_role); - EXECUTE format('REVOKE SELECT, UPDATE ON ALL SEQUENCES IN SCHEMA %I FROM %I', r.nspname, ddl_role); - EXECUTE format('REVOKE EXECUTE ON ALL FUNCTIONS IN SCHEMA %I FROM %I', r.nspname, ddl_role); - END LOOP; - CLOSE cur; -EXCEPTION WHEN OTHERS THEN - RAISE NOTICE 'An error occured: % %', SQLERRM, SQLSTATE; -END; -$$ LANGUAGE plpgsql security definer; -""")) - - cursor.execute(SQL("""CREATE OR REPLACE FUNCTION update_permissions_on_create_schema() -RETURNS event_trigger AS $$ -BEGIN - PERFORM update_permissions(); -END; -$$ LANGUAGE plpgsql; -""")) - - cursor.execute(SQL("""CREATE EVENT TRIGGER on_create_schema -ON ddl_command_end -WHEN TAG IN ('CREATE SCHEMA') -EXECUTE FUNCTION update_permissions_on_create_schema(); -""")) - except psycopg2.Error as e: - logger.error(f"Failed to create {database} ddl role: {e}") - raise PostgreSQLSetUpDDLRoleError() from e def create_user( self, @@ -429,8 +349,8 @@ def create_user( if "admin" in roles: createdb_enabled = True - roles.append("charmed_dml") - roles.append("charmed_instance_admin") + roles.append(ROLE_DML) + roles.append(ROLE_INSTANCE_ADMIN) roles.remove("admin") if "createdb" in roles: @@ -482,33 +402,33 @@ def create_user( def create_predefined_roles(self) -> None: """Create predefined roles.""" role_to_queries = { - "charmed_stats": [ - "CREATE ROLE charmed_stats NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_monitor", + ROLE_STATS: [ + f"CREATE ROLE {ROLE_STATS} NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_monitor", ], - "charmed_read": [ - "CREATE ROLE charmed_read NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_read_all_data", + ROLE_READ: [ + f"CREATE ROLE {ROLE_READ} NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_read_all_data", ], - "charmed_dml": [ - "CREATE ROLE charmed_dml NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_write_all_data", + ROLE_DML: [ + f"CREATE ROLE {ROLE_DML} NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_write_all_data", ], - "charmed_backup": [ - "CREATE ROLE charmed_backup NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_checkpoint", - "GRANT charmed_stats TO charmed_backup", - "GRANT execute ON FUNCTION pg_backup_start TO charmed_backup", - "GRANT execute ON FUNCTION pg_backup_stop TO charmed_backup", - "GRANT execute ON FUNCTION pg_create_restore_point TO charmed_backup", - "GRANT execute ON FUNCTION pg_switch_wal TO charmed_backup", + ROLE_BACKUP: [ + f"CREATE ROLE {ROLE_BACKUP} NOSUPERUSER NOCREATEDB NOCREATEROLE NOREPLICATION NOLOGIN IN ROLE pg_checkpoint", + f"GRANT {ROLE_STATS} TO {ROLE_BACKUP}", + f"GRANT execute ON FUNCTION pg_backup_start TO {ROLE_BACKUP}", + f"GRANT execute ON FUNCTION pg_backup_stop TO {ROLE_BACKUP}", + f"GRANT execute ON FUNCTION pg_create_restore_point TO {ROLE_BACKUP}", + f"GRANT execute ON FUNCTION pg_switch_wal TO {ROLE_BACKUP}", ], - "charmed_dba": [ - "CREATE ROLE charmed_dba NOSUPERUSER CREATEDB NOCREATEROLE NOLOGIN NOREPLICATION", - "GRANT execute ON FUNCTION set_user(text) TO charmed_dba", - "GRANT execute ON FUNCTION set_user(text, text) TO charmed_dba", - "GRANT execute ON FUNCTION set_user_u(text) TO charmed_dba", - "GRANT connect ON DATABASE postgres TO charmed_dba", + ROLE_DBA: [ + 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 connect ON DATABASE postgres TO {ROLE_DBA}", ], - "charmed_instance_admin": [ - "CREATE ROLE charmed_instance_admin NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION", - "GRANT connect ON DATABASE postgres TO charmed_instance_admin", + ROLE_INSTANCE_ADMIN: [ + f"CREATE ROLE {ROLE_INSTANCE_ADMIN} NOSUPERUSER NOCREATEDB NOCREATEROLE NOLOGIN NOREPLICATION", + f"GRANT connect ON DATABASE postgres TO {ROLE_INSTANCE_ADMIN}", ] } diff --git a/src/backups.py b/src/backups.py index 4c414d75e8..dd9ca2a91e 100644 --- a/src/backups.py +++ b/src/backups.py @@ -27,7 +27,6 @@ from jinja2 import Template from ops.charm import ActionEvent, HookEvent from ops.framework import Object -from ops.jujuversion import JujuVersion from ops.model import ActiveStatus, MaintenanceStatus from tenacity import RetryError, Retrying, stop_after_attempt, wait_fixed @@ -882,12 +881,11 @@ def _on_create_backup_action(self, event) -> None: # Test uploading metadata to S3 to test credentials before backup. datetime_backup_requested = datetime.now().strftime("%Y-%m-%dT%H:%M:%SZ") - juju_version = JujuVersion.from_environ() metadata = f"""Date Backup Requested: {datetime_backup_requested} Model Name: {self.model.name} Application Name: {self.model.app.name} Unit Name: {self.charm.unit.name} -Juju Version: {juju_version!s} +Juju Version: {self.charm.model.juju_version!s} """ if not self._upload_content_to_s3( metadata, diff --git a/src/charm.py b/src/charm.py index 6794dc39e9..9eb397baec 100755 --- a/src/charm.py +++ b/src/charm.py @@ -28,6 +28,9 @@ ACCESS_GROUP_IDENTITY, ACCESS_GROUPS, REQUIRED_PLUGINS, + ROLE_BACKUP, + ROLE_DBA, + ROLE_STATS, PostgreSQL, PostgreSQLCreatePredefinedRolesError, PostgreSQLCreateUserError, @@ -80,6 +83,8 @@ BACKUP_USER, DATABASE_DEFAULT_NAME, DATABASE_PORT, + DBA_PASSWORD_KEY, + DBA_USER, METRICS_PORT, MONITORING_PASSWORD_KEY, MONITORING_SNAP_SERVICE, @@ -180,9 +185,7 @@ def __init__(self, *args): deleted_label=SECRET_DELETED_LABEL, ) - juju_version = self.model.juju_version - run_cmd = "/usr/bin/juju-exec" if juju_version.major > 2 else "/usr/bin/juju-run" - self._observer = ClusterTopologyObserver(self, run_cmd) + self._observer = ClusterTopologyObserver(self, "/usr/bin/juju-exec") self._rotate_logs = RotateLogs(self) self.framework.observe(self.on.cluster_topology_change, self._on_cluster_topology_change) self.framework.observe(self.on.install, self._on_install) @@ -1128,7 +1131,7 @@ def _on_leader_elected(self, event: LeaderElectedEvent) -> None: # noqa: C901 MONITORING_PASSWORD_KEY, RAFT_PASSWORD_KEY, PATRONI_PASSWORD_KEY, - "dba-user-password", + DBA_PASSWORD_KEY, ): if self.get_secret(APP_SCOPE, key) is None: if key in system_user_passwords: @@ -1480,16 +1483,15 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901 # For that case, check whether the postgres user already exits. users = self.postgresql.list_users() # Create the dba user. - # TODO: move 'dba_user' and 'dba-user-password' to constants - if "dba_user" not in users: + if DBA_USER not in users: self.postgresql.create_user( - "dba_user", - self.get_secret(APP_SCOPE, "dba-user-password"), - roles=["charmed_dba"], + DBA_USER, + self.get_secret(APP_SCOPE, DBA_PASSWORD_KEY), + roles=[ROLE_DBA], ) # TODO: move predefined roles into constants if BACKUP_USER not in users: - self.postgresql.create_user(BACKUP_USER, new_password(), roles=["charmed_backup"]) + self.postgresql.create_user(BACKUP_USER, new_password(), roles=[ROLE_BACKUP]) self.postgresql.grant_database_privileges_to_user( BACKUP_USER, "postgres", ["connect"] ) @@ -1498,7 +1500,7 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901 self.postgresql.create_user( MONITORING_USER, self.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY), - roles=["charmed_stats"], + roles=[ROLE_STATS], ) except PostgreSQLGrantDatabasePrivilegesToUserError as e: logger.exception(e) @@ -1584,14 +1586,15 @@ def _update_admin_password(self, admin_secret_id: str) -> None: return try: + updateable_users = [*SYSTEM_USERS, BACKUP_USER, DBA_USER] + # get the secret content and check each user configured there # only SYSTEM_USERS with changed passwords are processed, all others ignored updated_passwords = self.get_secret_from_id(secret_id=admin_secret_id) for user, password in list(updated_passwords.items()): - # TODO: generalize and add ddl users here - if user not in [*SYSTEM_USERS, BACKUP_USER]: + if user not in updateable_users: logger.error( - f"Can only update system users: {', '.join(SYSTEM_USERS)} not {user}" + f"Can only update users: {', '.join(updateable_users)} not {user}" ) updated_passwords.pop(user) continue diff --git a/src/cluster.py b/src/cluster.py index d2a2dad49c..83c9a44098 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -17,6 +17,7 @@ import psutil import requests from charms.operator_libs_linux.v2 import snap +from charms.postgresql_k8s.v1.postgresql import ROLE_DBA from jinja2 import Template from ops import BlockedStatus from pysyncobj.utility import TcpUtility, UtilityException @@ -696,6 +697,7 @@ def render_patroni_yml_file( raft_password=self.raft_password, ldap_parameters=self._dict_to_hba_string(ldap_params), patroni_password=self.patroni_password, + dba_role=ROLE_DBA, ) self.render_file(f"{PATRONI_CONF_PATH}/patroni.yaml", rendered, 0o600) diff --git a/src/constants.py b/src/constants.py index 6342e12238..aecef2f35c 100644 --- a/src/constants.py +++ b/src/constants.py @@ -15,6 +15,7 @@ BACKUP_USER = "backup" REPLICATION_USER = "replication" REWIND_USER = "rewind" +DBA_USER = "dba" TLS_KEY_FILE = "key.pem" TLS_CA_FILE = "ca.pem" TLS_CERT_FILE = "cert.pem" @@ -66,6 +67,7 @@ MONITORING_PASSWORD_KEY = "monitoring-password" # noqa: S105 RAFT_PASSWORD_KEY = "raft-password" # noqa: S105 PATRONI_PASSWORD_KEY = "patroni-password" # noqa: S105 +DBA_PASSWORD_KEY = "dba-password" # noqa: S105 SECRET_INTERNAL_LABEL = "internal-secret" # noqa: S105 SECRET_DELETED_LABEL = "None" # noqa: S105 SYSTEM_USERS_PASSWORD_CONFIG = "system-users" # noqa: S105 diff --git a/src/relations/postgresql_provider.py b/src/relations/postgresql_provider.py index aad272cc49..f8345c203d 100644 --- a/src/relations/postgresql_provider.py +++ b/src/relations/postgresql_provider.py @@ -114,14 +114,6 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: database_created = self.charm.postgresql.create_database(database) - # TODO: possibly delete if DDL role needs to be postponed to a future PR - # if database_created: - # ddl_user = f"{database}_ddl" - # ddl_password = new_password() - # self.charm.postgresql.set_up_ddl_role(database, ddl_user, ddl_password) - - # self.charm.set_secret(APP_SCOPE, f"{database}-ddl-password", ddl_password) - self.charm.postgresql.create_user( user, password, roles=[*extra_user_roles, f"{database}_admin"], database=database ) diff --git a/src/upgrade.py b/src/upgrade.py index 0a72d21d1e..a7ade00175 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -12,7 +12,7 @@ DependencyModel, UpgradeGrantedEvent, ) -from charms.postgresql_k8s.v1.postgresql import ACCESS_GROUPS +from charms.postgresql_k8s.v1.postgresql import ACCESS_GROUPS, ROLE_STATS from ops.model import MaintenanceStatus, RelationDataContent, WaitingStatus from pydantic import BaseModel from tenacity import RetryError, Retrying, stop_after_attempt, wait_fixed @@ -190,6 +190,7 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None: "Defer on_upgrade_granted: member not ready or not joined the cluster yet" ) event.defer() + return self.set_unit_completed() @@ -245,7 +246,7 @@ def _prepare_upgrade_from_legacy(self) -> None: self.charm.postgresql.create_user( MONITORING_USER, self.charm.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY), - roles=["charmed_stats"], + roles=[ROLE_STATS], ) self.charm.postgresql.set_up_database() self._set_up_new_access_roles_for_legacy() diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index a0acbdd435..6407133969 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -155,7 +155,7 @@ postgresql: {% endif %} set_user.block_log_statement: on set_user.exit_on_error: on - set_user.superuser_allowlist: 'charmed_dba' + set_user.superuser_allowlist: '{{ dba_role }}' pgpass: /tmp/pgpass pg_hba: - local all backup peer map=operator diff --git a/tests/integration/test_predefined_roles.py b/tests/integration/test_predefined_roles.py index 88aa620a64..87abf072da 100644 --- a/tests/integration/test_predefined_roles.py +++ b/tests/integration/test_predefined_roles.py @@ -20,7 +20,7 @@ TIMEOUT = 15 * 60 DATA_INTEGRATOR_APP_NAME = "data-integrator" -# NOTE: We are unable to test set_user_u('operator') as dba_user because psycopg2 +# NOTE: We are unable to test set_user_u('operator') as dba user because psycopg2 # runs every query in a transaction and running set_user() is not supported in transactions. diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 479919633d..f1e306822e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -8,10 +8,10 @@ # This causes every test defined in this file to run 2 times, each with -# charm.JujuVersion.has_secrets set as True or as False +# ops.JujuVersion.has_secrets set as True or as False @pytest.fixture(autouse=True) def _has_secrets(request, monkeypatch): - monkeypatch.setattr("charm.JujuVersion.has_secrets", PropertyMock(return_value=True)) + monkeypatch.setattr("ops.JujuVersion.has_secrets", PropertyMock(return_value=True)) @pytest.fixture(autouse=True) diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 982302941d..a945c87127 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -1164,7 +1164,6 @@ def test_on_create_backup_action(harness): ) as _is_primary, patch("charm.PostgreSQLBackups._upload_content_to_s3") as _upload_content_to_s3, patch("backups.datetime") as _datetime, - patch("ops.JujuVersion.from_environ") as _from_environ, patch("charm.PostgreSQLBackups._retrieve_s3_parameters") as _retrieve_s3_parameters, patch("charm.PostgreSQLBackups._can_unit_perform_backup") as _can_unit_perform_backup, ): @@ -1199,13 +1198,12 @@ def test_on_create_backup_action(harness): [], ) _datetime.now.return_value.strftime.return_value = "2023-01-01T09:00:00Z" - _from_environ.return_value = "test-juju-version" _upload_content_to_s3.return_value = False expected_metadata = f"""Date Backup Requested: 2023-01-01T09:00:00Z Model Name: {harness.charm.model.name} Application Name: {harness.charm.model.app.name} Unit Name: {harness.charm.unit.name} -Juju Version: test-juju-version +Juju Version: 0.0.0 """ harness.charm.backup._on_create_backup_action(mock_event) _upload_content_to_s3.assert_called_once_with( diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index c426ad1d41..3d8af12ebc 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -10,7 +10,7 @@ import psycopg2 import pytest from charms.operator_libs_linux.v2 import snap -from charms.postgresql_k8s.v0.postgresql import ( +from charms.postgresql_k8s.v1.postgresql import ( PostgreSQLCreateUserError, PostgreSQLEnableDisableExtensionError, ) @@ -601,6 +601,16 @@ def test_on_start(harness): "charm.PostgresqlOperatorCharm._is_storage_attached", side_effect=[False, True, True, True, True, True], ) as _is_storage_attached, + patch( + "charm.PostgresqlOperatorCharm._can_connect_to_postgresql", + new_callable=PropertyMock, + return_value=True, + ), + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + return_value=True, + ), ): _get_postgresql_version.return_value = "16.6" @@ -646,6 +656,9 @@ def test_on_start(harness): _restart_services_after_reboot.reset_mock() _member_started.return_value = True harness.charm.on.start.emit() + _postgresql.enable_disable_extensions.assert_called_once_with( + {"set_user": True}, database="postgres" + ) _postgresql.create_user.assert_called_once() _oversee_users.assert_not_called() _restart_services_after_reboot.assert_called_once() @@ -909,7 +922,7 @@ def test_on_update_status_after_restore_operation(harness): ) as _handle_processes_failures, patch("charm.PostgreSQLBackups.can_use_s3_repository") as _can_use_s3_repository, patch( - "charms.postgresql_k8s.v0.postgresql.PostgreSQL.get_current_timeline" + "charms.postgresql_k8s.v1.postgresql.PostgreSQL.get_current_timeline" ) as _get_current_timeline, patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started, @@ -1739,25 +1752,6 @@ def test_get_available_memory(harness): assert harness.charm.get_available_memory() == 0 -def test_juju_run_exec_divergence(harness): - with ( - patch("charm.ClusterTopologyObserver") as _topology_observer, - patch("charm.JujuVersion") as _juju_version, - ): - # Juju 2 - _juju_version.from_environ.return_value.major = 2 - harness = Harness(PostgresqlOperatorCharm) - harness.begin() - _topology_observer.assert_called_once_with(harness.charm, "/usr/bin/juju-run") - _topology_observer.reset_mock() - - # Juju 3 - _juju_version.from_environ.return_value.major = 3 - harness = Harness(PostgresqlOperatorCharm) - harness.begin() - _topology_observer.assert_called_once_with(harness.charm, "/usr/bin/juju-exec") - - def test_client_relations(harness): # Test when the charm has no relations. assert len(harness.charm.client_relations) == 0 @@ -2532,37 +2526,6 @@ def test_restart_services_after_reboot(harness): _start_stop_pgbackrest_service.assert_called_once() -def test_get_plugins(harness): - with patch("charm.PostgresqlOperatorCharm._on_config_changed"): - # Test when the charm has no plugins enabled. - assert harness.charm.get_plugins() == ["pgaudit"] - - # Test when the charm has some plugins enabled. - harness.update_config({ - "plugin_audit_enable": True, - "plugin_citext_enable": True, - "plugin_spi_enable": True, - }) - assert harness.charm.get_plugins() == [ - "pgaudit", - "citext", - "refint", - "autoinc", - "insert_username", - "moddatetime", - ] - - # Test when the charm has the pgAudit plugin disabled. - harness.update_config({"plugin_audit_enable": False}) - assert harness.charm.get_plugins() == [ - "citext", - "refint", - "autoinc", - "insert_username", - "moddatetime", - ] - - def test_on_promote_to_primary(harness): with ( patch("charm.PostgresqlOperatorCharm._raft_reinitialisation") as _raft_reinitialisation, diff --git a/tests/unit/test_cluster.py b/tests/unit/test_cluster.py index c9b5fe1ee2..6394501e7b 100644 --- a/tests/unit/test_cluster.py +++ b/tests/unit/test_cluster.py @@ -373,6 +373,7 @@ def test_render_patroni_yml_file(peers_ips, patroni): synchronous_node_count=0, raft_password=raft_password, patroni_password=patroni_password, + dba_role="charmed_dba", ) # Setup a mock for the `open` method, set returned data to patroni.yml template. diff --git a/tests/unit/test_postgresql_provider.py b/tests/unit/test_postgresql_provider.py index 13b065d299..d1ba2bf338 100644 --- a/tests/unit/test_postgresql_provider.py +++ b/tests/unit/test_postgresql_provider.py @@ -4,7 +4,7 @@ from unittest.mock import Mock, PropertyMock, patch import pytest -from charms.postgresql_k8s.v0.postgresql import ( +from charms.postgresql_k8s.v1.postgresql import ( ACCESS_GROUP_RELATION, PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, @@ -103,7 +103,7 @@ def test_on_database_requested(harness): side_effect=[None, PostgreSQLCreateUserError, None, None] ) postgresql_mock.create_database = PropertyMock( - side_effect=[None, PostgreSQLCreateDatabaseError, None] + side_effect=[None, None, PostgreSQLCreateDatabaseError, None] ) postgresql_mock.get_postgresql_version = PropertyMock( side_effect=[ @@ -111,6 +111,7 @@ def test_on_database_requested(harness): PostgreSQLGetPostgreSQLVersionError, ] ) + postgresql_mock.list_roles.return_value = [] # Request a database before the database is ready. request_database(harness) @@ -127,19 +128,14 @@ def test_on_database_requested(harness): user = f"relation-{rel_id}" expected_user_roles = [role.lower() for role in EXTRA_USER_ROLES.split(",")] expected_user_roles.append(ACCESS_GROUP_RELATION) + expected_user_roles.append(f"{DATABASE}_admin") postgresql_mock.create_user.assert_called_once_with( user, "test-password", - extra_user_roles=expected_user_roles, - ) - database_relation = harness.model.get_relation(RELATION_NAME) - client_relations = [database_relation] - postgresql_mock.create_database.assert_called_once_with( - DATABASE, - user, - plugins=["pgaudit"], - client_relations=client_relations, + roles=expected_user_roles, + database=DATABASE, ) + postgresql_mock.create_database.assert_called_once_with(DATABASE) postgresql_mock.get_postgresql_version.assert_called_once() _update_endpoints.assert_called_once() diff --git a/tests/unit/test_upgrade.py b/tests/unit/test_upgrade.py index 1baac2da9b..cabae31fad 100644 --- a/tests/unit/test_upgrade.py +++ b/tests/unit/test_upgrade.py @@ -103,7 +103,7 @@ def test_on_upgrade_granted(harness): "charm.PostgreSQLBackups.start_stop_pgbackrest_service" ) as _start_stop_pgbackrest_service, patch("charm.PostgresqlOperatorCharm._setup_exporter") as _setup_exporter, - patch("charm.Patroni.start_patroni") as _start_patroni, + patch("charm.Patroni.restart_patroni") as _restart_patroni, patch("charm.PostgresqlOperatorCharm._install_snap_packages") as _install_snap_packages, patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, patch( @@ -112,7 +112,7 @@ def test_on_upgrade_granted(harness): ): # Test when the charm fails to start Patroni. mock_event = MagicMock() - _start_patroni.return_value = False + _restart_patroni.return_value = False harness.charm.upgrade._on_upgrade_granted(mock_event) _update_config.assert_called_once() _install_snap_packages.assert_called_once_with(packages=SNAP_PACKAGES, refresh=True) @@ -124,7 +124,7 @@ def test_on_upgrade_granted(harness): # Test when the member hasn't started yet. _set_unit_failed.reset_mock() - _start_patroni.return_value = True + _restart_patroni.return_value = True _member_started.return_value = False harness.charm.upgrade._on_upgrade_granted(mock_event) assert _member_started.call_count == 6