From fe8bd8b6023dfbcfd6f28e42bf949f790db64348 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 16 Aug 2022 10:48:45 -0300 Subject: [PATCH 01/25] Change charm database user --- actions.yaml | 5 +++-- src/charm.py | 30 +++++++++++++++--------------- src/constants.py | 1 + tests/integration/helpers.py | 12 ++++++------ tests/integration/test_charm.py | 17 +++++------------ tests/unit/test_charm.py | 26 +++++++++++++------------- 6 files changed, 43 insertions(+), 48 deletions(-) diff --git a/actions.yaml b/actions.yaml index 8e34be595a..5350a2a4b7 100644 --- a/actions.yaml +++ b/actions.yaml @@ -3,5 +3,6 @@ get-primary: description: Get the unit with is the primary/leader in the replication. -get-postgres-password: - description: Get the initial postgres user password for the database. \ No newline at end of file +get-operator-password: + description: Get the operator user password used by charm. + It is internal charm user, SHOULD NOT be used by applications. diff --git a/src/charm.py b/src/charm.py index 0eb61db491..a29761c7a8 100755 --- a/src/charm.py +++ b/src/charm.py @@ -30,7 +30,7 @@ from requests import ConnectionError from tenacity import RetryError -from constants import PEER +from constants import PEER, USER from patroni import NotReadyError, Patroni from relations.db import DbProvides from relations.postgresql_provider import PostgreSQLProvider @@ -59,7 +59,7 @@ def __init__(self, *args): self.framework.observe(self.on.postgresql_pebble_ready, self._on_postgresql_pebble_ready) self.framework.observe(self.on.upgrade_charm, self._on_upgrade_charm) self.framework.observe( - self.on.get_postgres_password_action, self._on_get_postgres_password + self.on.get_operator_password_action, self._on_get_operator_password ) self.framework.observe(self.on.get_primary_action, self._on_get_primary) self.framework.observe(self.on.update_status, self._on_update_status) @@ -74,8 +74,8 @@ def postgresql(self) -> PostgreSQL: """Returns an instance of the object used to interact with the database.""" return PostgreSQL( host=self.primary_endpoint, - user="postgres", - password=self._get_postgres_password(), + user=USER, + password=self._get_operator_password(), database="postgres", ) @@ -250,11 +250,11 @@ def _get_hostname_from_unit(self, member: str) -> str: def _on_leader_elected(self, event: LeaderElectedEvent) -> None: """Handle the leader-elected event.""" data = self._peers.data[self.app] - postgres_password = data.get("postgres-password", None) + operator_password = data.get("operator-password", None) replication_password = data.get("replication-password", None) - if postgres_password is None: - self._peers.data[self.app]["postgres-password"] = new_password() + if operator_password is None: + self._peers.data[self.app]["operator-password"] = new_password() if replication_password is None: self._peers.data[self.app]["replication-password"] = new_password() @@ -387,9 +387,9 @@ def _create_resources(self) -> None: self.unit.status = BlockedStatus(f"failed to create services {e}") return - def _on_get_postgres_password(self, event: ActionEvent) -> None: - """Returns the password for the postgres user as an action response.""" - event.set_results({"postgres-password": self._get_postgres_password()}) + def _on_get_operator_password(self, event: ActionEvent) -> None: + """Returns the password for the operator user as an action response.""" + event.set_results({"operator-password": self._get_operator_password()}) def _on_get_primary(self, event: ActionEvent) -> None: """Get primary instance.""" @@ -501,8 +501,8 @@ def _postgresql_layer(self) -> Layer: "PATRONI_SCOPE": f"patroni-{self._name}", "PATRONI_REPLICATION_USERNAME": "replication", "PATRONI_REPLICATION_PASSWORD": self._replication_password, - "PATRONI_SUPERUSER_USERNAME": "postgres", - "PATRONI_SUPERUSER_PASSWORD": self._get_postgres_password(), + "PATRONI_SUPERUSER_USERNAME": USER, + "PATRONI_SUPERUSER_PASSWORD": self._get_operator_password(), }, } }, @@ -519,10 +519,10 @@ def _peers(self) -> Relation: """ return self.model.get_relation(PEER) - def _get_postgres_password(self) -> str: - """Get postgres user password.""" + def _get_operator_password(self) -> str: + """Get operator user password.""" data = self._peers.data[self.app] - return data.get("postgres-password", None) + return data.get("operator-password", None) @property def _replication_password(self) -> str: diff --git a/src/constants.py b/src/constants.py index 6bc12a5eeb..a7e2f4c531 100644 --- a/src/constants.py +++ b/src/constants.py @@ -5,3 +5,4 @@ DATABASE_PORT = "5432" PEER = "database-peers" +USER = "operator" diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 864833aa3e..65bd52904b 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -30,7 +30,7 @@ async def check_database_users_existence( """ unit = ops_test.model.applications[DATABASE_APP_NAME].units[0] unit_address = await get_unit_address(ops_test, unit.name) - password = await get_postgres_password(ops_test) + password = await get_operator_password(ops_test) # Retrieve all users in the database. output = await execute_query_on_unit( @@ -61,7 +61,7 @@ async def check_database_creation(ops_test: OpsTest, database: str) -> None: ops_test: The ops test framework database: Name of the database that should have been created """ - password = await get_postgres_password(ops_test) + password = await get_operator_password(ops_test) for unit in ops_test.model.applications[DATABASE_APP_NAME].units: unit_address = await get_unit_address(ops_test, unit.name) @@ -196,12 +196,12 @@ def get_application_units(ops_test: OpsTest, application_name: str) -> List[str] ] -async def get_postgres_password(ops_test: OpsTest): - """Retrieve the postgres user password using the action.""" +async def get_operator_password(ops_test: OpsTest): + """Retrieve the operator user password using the action.""" unit = ops_test.model.units.get(f"{DATABASE_APP_NAME}/0") - action = await unit.run_action("get-postgres-password") + action = await unit.run_action("get-operator-password") result = await action.wait() - return result.results["postgres-password"] + return result.results["operator-password"] async def get_primary(ops_test: OpsTest, unit_id=0) -> str: diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 54e5164e04..11a4d15b76 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -18,6 +18,7 @@ convert_records_to_dict, get_application_units, get_cluster_members, + get_operator_password, get_unit_address, scale_application, ) @@ -75,7 +76,7 @@ async def test_database_is_up(ops_test: OpsTest, unit_id: int): @pytest.mark.parametrize("unit_id", UNIT_IDS) async def test_settings_are_correct(ops_test: OpsTest, unit_id: int): - password = await get_postgres_password(ops_test) + password = await get_operator_password(ops_test) # Connect to PostgreSQL. host = await get_unit_address(ops_test, f"{APP_NAME}/{unit_id}") @@ -163,7 +164,7 @@ async def test_scale_down_and_up(ops_test: OpsTest): async def test_persist_data_through_graceful_restart(ops_test: OpsTest): """Test data persists through a graceful restart.""" primary = await get_primary(ops_test) - password = await get_postgres_password(ops_test) + password = await get_operator_password(ops_test) address = await get_unit_address(ops_test, primary) # Write data to primary IP. @@ -191,7 +192,7 @@ async def test_persist_data_through_graceful_restart(ops_test: OpsTest): async def test_persist_data_through_failure(ops_test: OpsTest): """Test data persists through a failure.""" primary = await get_primary(ops_test) - password = await get_postgres_password(ops_test) + password = await get_operator_password(ops_test) address = await get_unit_address(ops_test, primary) # Write data to primary IP. @@ -291,14 +292,6 @@ async def get_primary(ops_test: OpsTest, unit_id=0) -> str: return action.results["primary"] -async def get_postgres_password(ops_test: OpsTest): - """Retrieve the postgres user password using the action.""" - unit = ops_test.model.units.get(f"{APP_NAME}/0") - action = await unit.run_action("get-postgres-password") - result = await action.wait() - return result.results["postgres-password"] - - def db_connect(host: str, password: str): """Returns psycopg2 connection object linked to postgres db in the given host. @@ -307,7 +300,7 @@ def db_connect(host: str, password: str): password: postgres password Returns: - psycopg2 connection object linked to postgres db, under "postgres" user. + psycopg2 connection object linked to postgres db, under "operator" user. """ return psycopg2.connect( f"dbname='postgres' user='postgres' host='{host}' password='{password}' connect_timeout=10" diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index fcb3f47a6c..f5c230b078 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -52,7 +52,7 @@ def test_on_leader_elected(self, _, __, _render_postgresql_conf_file, ___): # Check that a new password was generated on leader election. self.harness.set_leader() - superuser_password = self.charm._peers.data[self.charm.app].get("postgres-password", None) + superuser_password = self.charm._peers.data[self.charm.app].get("operator-password", None) self.assertIsNotNone(superuser_password) replication_password = self.charm._peers.data[self.charm.app].get( @@ -65,7 +65,7 @@ def test_on_leader_elected(self, _, __, _render_postgresql_conf_file, ___): self.harness.set_leader(False) self.harness.set_leader() self.assertEqual( - self.charm._peers.data[self.charm.app].get("postgres-password", None), + self.charm._peers.data[self.charm.app].get("operator-password", None), superuser_password, ) self.assertEqual( @@ -104,13 +104,13 @@ def test_on_postgresql_pebble_ready( self.assertEqual(container.get_service(self._postgresql_service).is_running(), True) _render_patroni_yml_file.assert_called_once() - @patch("charm.PostgresqlOperatorCharm._get_postgres_password") - def test_on_get_postgres_password(self, _get_postgres_password): + @patch("charm.PostgresqlOperatorCharm._get_operator_password") + def test_on_get_operator_password(self, _get_operator_password): mock_event = Mock() - _get_postgres_password.return_value = "test-password" - self.charm._on_get_postgres_password(mock_event) - _get_postgres_password.assert_called_once() - mock_event.set_results.assert_called_once_with({"postgres-password": "test-password"}) + _get_operator_password.return_value = "test-password" + self.charm._on_get_operator_password(mock_event) + _get_operator_password.assert_called_once() + mock_event.set_results.assert_called_once_with({"operator-password": "test-password"}) @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.get_primary") @@ -278,8 +278,8 @@ def test_postgresql_layer(self, _, __, ___, ____): "PATRONI_SCOPE": f"patroni-{self.charm._name}", "PATRONI_REPLICATION_USERNAME": "replication", "PATRONI_REPLICATION_PASSWORD": self.charm._replication_password, - "PATRONI_SUPERUSER_USERNAME": "postgres", - "PATRONI_SUPERUSER_PASSWORD": self.charm._get_postgres_password(), + "PATRONI_SUPERUSER_USERNAME": "operator", + "PATRONI_SUPERUSER_PASSWORD": self.charm._get_operator_password(), }, } }, @@ -290,13 +290,13 @@ def test_postgresql_layer(self, _, __, ___, ____): @patch("charm.Patroni.render_postgresql_conf_file") @patch("charm.PostgresqlOperatorCharm._patch_pod_labels") @patch("charm.PostgresqlOperatorCharm._create_resources") - def test_get_postgres_password(self, _, __, ___, ____): + def test_get_operator_password(self, _, __, ___, ____): # Test for a None password. self.harness.add_relation(self._peer_relation, self.charm.app.name) - self.assertIsNone(self.charm._get_postgres_password()) + self.assertIsNone(self.charm._get_operator_password()) # Then test for a non empty password after leader election and peer data set. self.harness.set_leader() - password = self.charm._get_postgres_password() + password = self.charm._get_operator_password() self.assertIsNotNone(password) self.assertNotEqual(password, "") From dd395f56c765e54df46345f02fca8a5cbebd24ad Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 16 Aug 2022 10:57:19 -0300 Subject: [PATCH 02/25] Fix tests user --- tests/integration/helpers.py | 2 +- tests/integration/test_charm.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 65bd52904b..21a8bc71bf 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -161,7 +161,7 @@ async def execute_query_on_unit( A list of rows that were potentially returned from the query. """ with psycopg2.connect( - f"dbname='{database}' user='postgres' host='{unit_address}' password='{password}' connect_timeout=10" + f"dbname='{database}' user='operator' host='{unit_address}' password='{password}' connect_timeout=10" ) as connection, connection.cursor() as cursor: cursor.execute(query) output = list(itertools.chain(*cursor.fetchall())) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 11a4d15b76..8267c71719 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -82,7 +82,7 @@ async def test_settings_are_correct(ops_test: OpsTest, unit_id: int): host = await get_unit_address(ops_test, f"{APP_NAME}/{unit_id}") logger.info("connecting to the database host: %s", host) with psycopg2.connect( - f"dbname='postgres' user='postgres' host='{host}' password='{password}' connect_timeout=1" + f"dbname='postgres' user='operator' host='{host}' password='{password}' connect_timeout=1" ) as connection, connection.cursor() as cursor: assert connection.status == psycopg2.extensions.STATUS_READY @@ -303,5 +303,5 @@ def db_connect(host: str, password: str): psycopg2 connection object linked to postgres db, under "operator" user. """ return psycopg2.connect( - f"dbname='postgres' user='postgres' host='{host}' password='{password}' connect_timeout=10" + f"dbname='postgres' user='operator' host='{host}' password='{password}' connect_timeout=10" ) From d325da69f9cca215957480cbf5a2a65233d5c679 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 17 Aug 2022 16:27:28 -0300 Subject: [PATCH 03/25] Add password rotation action --- actions.yaml | 10 ++++ lib/charms/postgresql_k8s/v0/postgresql.py | 43 ++++++++++++++-- src/charm.py | 59 ++++++++++++++++++++-- src/constants.py | 2 + 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/actions.yaml b/actions.yaml index 5350a2a4b7..0a44a220b0 100644 --- a/actions.yaml +++ b/actions.yaml @@ -6,3 +6,13 @@ get-primary: get-operator-password: description: Get the operator user password used by charm. It is internal charm user, SHOULD NOT be used by applications. +rotate-users-passwords: + description: Change the operator user password used by charm. + It is internal charm user, SHOULD NOT be used by applications. + params: + user: + type: string + description: The password will be auto-generated if this option is not specified. + password: + type: string + description: The password will be auto-generated if this option is not specified. diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 71ecf51c07..4888e65346 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -19,6 +19,7 @@ Any charm using this library should import the `psycopg2` or `psycopg2-binary` dependency. """ import logging +from typing import Dict import psycopg2 from psycopg2 import sql @@ -31,7 +32,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 1 +LIBPATCH = 2 logger = logging.getLogger(__name__) @@ -53,6 +54,10 @@ class PostgreSQLGetPostgreSQLVersionError(Exception): """Exception raised when retrieving PostgreSQL version fails.""" +class PostgreSQLSetUserPasswordError(Exception): + """Exception raised when setting/updating a user password fails.""" + + class PostgreSQL: """Class to encapsulate all operations related to interacting with PostgreSQL instance.""" @@ -68,12 +73,16 @@ def __init__( self.password = password self.database = database - def _connect_to_database(self, database: str = None) -> psycopg2.extensions.connection: + def _connect_to_database( + self, database: str = None, autocommit: bool = True + ) -> psycopg2.extensions.connection: """Creates a connection to the database. Args: database: database to connect to (defaults to the database provided when the object for this class was created). + autocommit: whether every command issued to the database + should be immediately committed. Returns: psycopg2 connection object. @@ -81,7 +90,8 @@ def _connect_to_database(self, database: str = None) -> psycopg2.extensions.conn connection = psycopg2.connect( f"dbname='{database if database else self.database}' user='{self.user}' host='{self.host}' password='{self.password}' connect_timeout=1" ) - connection.autocommit = True + if autocommit: + connection.autocommit = True return connection def create_database(self, database: str, user: str) -> None: @@ -173,3 +183,30 @@ def get_postgresql_version(self) -> str: except psycopg2.Error as e: logger.error(f"Failed to get PostgreSQL version: {e}") raise PostgreSQLGetPostgreSQLVersionError() + + def rotate_users_passwords(self, users_with_passwords: Dict[str, str]) -> None: + """Rotates one or more users passwords. + + Args: + users_with_passwords: a dict following the format {"user": "password"}. + It can contain multiple users. + + Raises: + PostgreSQLSetUserPasswordError if any user password couldn't be changed. + """ + try: + with self._connect_to_database( + autocommit=False + ) as connection, connection.cursor() as cursor: + for user, password in users_with_passwords.items(): + cursor.execute( + sql.SQL( + "ALTER USER {} WITH ENCRYPTED PASSWORD '" + password + "';" + ).format(sql.Identifier(user)) + ) + except psycopg2.Error as e: + logger.error(f"Failed to rotate user password: {e}") + raise PostgreSQLSetUserPasswordError() + finally: + if connection is not None: + connection.close() diff --git a/src/charm.py b/src/charm.py index a29761c7a8..a21fbe89ed 100755 --- a/src/charm.py +++ b/src/charm.py @@ -7,7 +7,10 @@ import logging from typing import List -from charms.postgresql_k8s.v0.postgresql import PostgreSQL +from charms.postgresql_k8s.v0.postgresql import ( + PostgreSQL, + PostgreSQLSetUserPasswordError, +) from lightkube import ApiError, Client, codecs from lightkube.resources.core_v1 import Pod from ops.charm import ( @@ -30,7 +33,7 @@ from requests import ConnectionError from tenacity import RetryError -from constants import PEER, USER +from constants import PEER, REPLICATION_USER, SYSTEM_USERS, USER from patroni import NotReadyError, Patroni from relations.db import DbProvides from relations.postgresql_provider import PostgreSQLProvider @@ -61,6 +64,9 @@ def __init__(self, *args): self.framework.observe( self.on.get_operator_password_action, self._on_get_operator_password ) + self.framework.observe( + self.on.rotate_users_passwords_action, self._on_rotate_users_passwords + ) self.framework.observe(self.on.get_primary_action, self._on_get_primary) self.framework.observe(self.on.update_status, self._on_update_status) self._storage_path = self.meta.storages["pgdata"].location @@ -391,6 +397,53 @@ def _on_get_operator_password(self, event: ActionEvent) -> None: """Returns the password for the operator user as an action response.""" event.set_results({"operator-password": self._get_operator_password()}) + def _on_rotate_users_passwords(self, event: ActionEvent) -> None: + """Rotate the password for all system users or the specified user.""" + # Only the leader can write the new password into peer relation. + if not self.unit.is_leader(): + event.fail("The action can be run only on the leader unit") + return + + if "user" in event.params: + user = event.params["user"] + + # Fail if the user is not a system user. + # One example is users created through relations. + if user not in SYSTEM_USERS: + event.fail(f"User {user} is not a system user") + return + + # Generate a new password and use it if no password was provided to the action. + users = { + user: event.params["password"] if "password" in event.params else new_password() + } + else: + if "password" in event.params: + event.fail("The same password cannot be set for multiple users") + return + + users = {user: new_password() for user in SYSTEM_USERS} + + try: + self.postgresql.rotate_users_passwords(users) + except PostgreSQLSetUserPasswordError as e: + event.fail(f"Failed to set user password with error {e}") + return + + # Update the password in the peer relation if the operation was successful. + for user, password in users.items(): + self._peers.data[self.app].update({f"{user}-password": password}) + + # for unit in + self._patroni.reload_patroni_configuration() + + # Return the generated password when the user option is given. + if "user" in event.params and "password" not in event.params: + user = event.params["user"] + event.set_results( + {f"{user}-password": self._peers.data[self.app].get(f"{user}-password")} + ) + def _on_get_primary(self, event: ActionEvent) -> None: """Get primary instance.""" try: @@ -499,7 +552,7 @@ def _postgresql_layer(self) -> Layer: "PATRONI_KUBERNETES_USE_ENDPOINTS": "true", "PATRONI_NAME": pod_name, "PATRONI_SCOPE": f"patroni-{self._name}", - "PATRONI_REPLICATION_USERNAME": "replication", + "PATRONI_REPLICATION_USERNAME": REPLICATION_USER, "PATRONI_REPLICATION_PASSWORD": self._replication_password, "PATRONI_SUPERUSER_USERNAME": USER, "PATRONI_SUPERUSER_PASSWORD": self._get_operator_password(), diff --git a/src/constants.py b/src/constants.py index a7e2f4c531..3dec5b8866 100644 --- a/src/constants.py +++ b/src/constants.py @@ -5,4 +5,6 @@ DATABASE_PORT = "5432" PEER = "database-peers" +REPLICATION_USER = "replication" USER = "operator" +SYSTEM_USERS = [REPLICATION_USER, USER] From fc17a5e2ff466eea674771d33f353d3040d410fb Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 23 Aug 2022 10:48:48 -0300 Subject: [PATCH 04/25] Rework secrets management --- src/charm.py | 62 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/src/charm.py b/src/charm.py index bb3317a4a6..1fd83e069b 100755 --- a/src/charm.py +++ b/src/charm.py @@ -5,7 +5,7 @@ """Charmed Kubernetes Operator for the PostgreSQL database.""" import json import logging -from typing import List +from typing import Dict, List, Optional from charms.postgresql_k8s.v0.postgresql import PostgreSQL from lightkube import ApiError, Client, codecs @@ -69,6 +69,48 @@ def __init__(self, *args): self.legacy_db_relation = DbProvides(self, admin=False) self.legacy_db_admin_relation = DbProvides(self, admin=True) + @property + def app_peer_data(self) -> Dict: + """Application peer relation data object.""" + relation = self.model.get_relation(PEER) + if relation is None: + return {} + + return relation.data[self.app] + + @property + def unit_peer_data(self) -> Dict: + """Unit peer relation data object.""" + relation = self.model.get_relation(PEER) + if relation is None: + return {} + + return relation.data[self.unit] + + def _get_secret(self, scope: str, key: str) -> Optional[str]: + """Get secret from the secret storage.""" + if scope == "unit": + return self.unit_peer_data.get(key, None) + elif scope == "app": + return self.app_peer_data.get(key, None) + else: + raise RuntimeError("Unknown secret scope.") + + def _set_secret(self, scope: str, key: str, value: Optional[str]) -> None: + """Get secret from the secret storage.""" + if scope == "unit": + if not value: + del self.unit_peer_data[key] + return + self.unit_peer_data.update({key: value}) + elif scope == "app": + if not value: + del self.app_peer_data[key] + return + self.app_peer_data.update({key: value}) + else: + raise RuntimeError("Unknown secret scope.") + @property def postgresql(self) -> PostgreSQL: """Returns an instance of the object used to interact with the database.""" @@ -249,15 +291,11 @@ def _get_hostname_from_unit(self, member: str) -> str: def _on_leader_elected(self, event: LeaderElectedEvent) -> None: """Handle the leader-elected event.""" - data = self._peers.data[self.app] - operator_password = data.get("operator-password", None) - replication_password = data.get("replication-password", None) - - if operator_password is None: - self._peers.data[self.app]["operator-password"] = new_password() + if self._get_secret("app", "operator-password") is None: + self._set_secret("app", "operator-password", new_password()) - if replication_password is None: - self._peers.data[self.app]["replication-password"] = new_password() + if self._get_secret("app", "replication-password") is None: + self._set_secret("app", "replication-password", new_password()) # Create resources and add labels needed for replication. self._create_resources() @@ -498,14 +536,12 @@ def _peers(self) -> Relation: def _get_operator_password(self) -> str: """Get operator user password.""" - data = self._peers.data[self.app] - return data.get("operator-password", None) + return self._get_secret("app", "operator-password") @property def _replication_password(self) -> str: """Get replication user password.""" - data = self._peers.data[self.app] - return data.get("replication-password", None) + return self._get_secret("app", "replication-password") def _unit_name_to_pod_name(self, unit_name: str) -> str: """Converts unit name to pod name. From 05b707e9c31d5b63fe36dbabb465b646e0aacb74 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 23 Aug 2022 11:38:44 -0300 Subject: [PATCH 05/25] Add unit tests --- tests/unit/test_charm.py | 48 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 286357fa75..45d76104b9 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -31,6 +31,8 @@ def setUp(self): "app_name": self.harness.model.app.name, } + self.rel_id = self.harness.add_relation(self._peer_relation, self.charm.app.name) + @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.render_postgresql_conf_file") def test_on_install( @@ -46,7 +48,6 @@ def test_on_install( @patch("charm.PostgresqlOperatorCharm._create_resources") def test_on_leader_elected(self, _, __, _render_postgresql_conf_file, ___): # Assert that there is no password in the peer relation. - self.harness.add_relation(self._peer_relation, self.charm.app.name) self.assertIsNone(self.charm._peers.data[self.charm.app].get("postgres-password", None)) self.assertIsNone(self.charm._peers.data[self.charm.app].get("replication-password", None)) @@ -86,7 +87,6 @@ def test_on_postgresql_pebble_ready( # Check that the initial plan is empty. plan = self.harness.get_container_pebble_plan(self._postgresql_container) self.assertEqual(plan.to_dict(), {}) - self.harness.add_relation(self._peer_relation, self.charm.app.name) # Get the current and the expected layer from the pebble plan and the _postgresql_layer # method, respectively. @@ -202,7 +202,6 @@ def test_patch_pod_labels(self, _client): @patch("charm.PostgresqlOperatorCharm._create_resources") def test_postgresql_layer(self, _, __, ___, ____): # Test with the already generated password. - self.harness.add_relation(self._peer_relation, self.charm.app.name) self.harness.set_leader() plan = self.charm._postgresql_layer().to_dict() expected = { @@ -238,7 +237,6 @@ def test_postgresql_layer(self, _, __, ___, ____): @patch("charm.PostgresqlOperatorCharm._create_resources") def test_get_operator_password(self, _, __, ___, ____): # Test for a None password. - self.harness.add_relation(self._peer_relation, self.charm.app.name) self.assertIsNone(self.charm._get_operator_password()) # Then test for a non empty password after leader election and peer data set. @@ -246,3 +244,45 @@ def test_get_operator_password(self, _, __, ___, ____): password = self.charm._get_operator_password() self.assertIsNotNone(password) self.assertNotEqual(password, "") + + @patch("charm.Patroni.reload_patroni_configuration") + @patch("charm.Patroni.render_postgresql_conf_file") + @patch("charm.PostgresqlOperatorCharm._create_resources") + def test_get_secret(self, _, __, ___): + self.harness.set_leader() + + # Test application scope. + assert self.charm._get_secret("app", "password") is None + self.harness.update_relation_data( + self.rel_id, self.charm.app.name, {"password": "test-password"} + ) + assert self.charm._get_secret("app", "password") == "test-password" + + # Test unit scope. + assert self.charm._get_secret("unit", "password") is None + self.harness.update_relation_data( + self.rel_id, self.charm.unit.name, {"password": "test-password"} + ) + assert self.charm._get_secret("unit", "password") == "test-password" + + @patch("charm.Patroni.reload_patroni_configuration") + @patch("charm.Patroni.render_postgresql_conf_file") + @patch("charm.PostgresqlOperatorCharm._create_resources") + def test_set_secret(self, _, __, ___): + self.harness.set_leader() + + # Test application scope. + assert "password" not in self.harness.get_relation_data(self.rel_id, self.charm.app.name) + self.charm._set_secret("app", "password", "test-password") + assert ( + self.harness.get_relation_data(self.rel_id, self.charm.app.name)["password"] + == "test-password" + ) + + # Test unit scope. + assert "password" not in self.harness.get_relation_data(self.rel_id, self.charm.unit.name) + self.charm._set_secret("unit", "password", "test-password") + assert ( + self.harness.get_relation_data(self.rel_id, self.charm.unit.name)["password"] + == "test-password" + ) From 36640b2620cefdacc8d3bbea76b0e3e7f894307e Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 23 Aug 2022 11:44:53 -0300 Subject: [PATCH 06/25] Add constants --- src/charm.py | 16 ++++++++-------- src/constants.py | 2 ++ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/charm.py b/src/charm.py index 1fd83e069b..b9e548b896 100755 --- a/src/charm.py +++ b/src/charm.py @@ -30,7 +30,7 @@ from requests import ConnectionError from tenacity import RetryError -from constants import PEER, USER +from constants import PEER, REPLICATION_PASSWORD_KEY, USER, USER_PASSWORD_KEY from patroni import NotReadyError, Patroni from relations.db import DbProvides from relations.postgresql_provider import PostgreSQLProvider @@ -291,11 +291,11 @@ def _get_hostname_from_unit(self, member: str) -> str: def _on_leader_elected(self, event: LeaderElectedEvent) -> None: """Handle the leader-elected event.""" - if self._get_secret("app", "operator-password") is None: - self._set_secret("app", "operator-password", new_password()) + if self._get_secret("app", USER_PASSWORD_KEY) is None: + self._set_secret("app", USER_PASSWORD_KEY, new_password()) - if self._get_secret("app", "replication-password") is None: - self._set_secret("app", "replication-password", new_password()) + if self._get_secret("app", REPLICATION_PASSWORD_KEY) is None: + self._set_secret("app", REPLICATION_PASSWORD_KEY, new_password()) # Create resources and add labels needed for replication. self._create_resources() @@ -427,7 +427,7 @@ def _create_resources(self) -> None: def _on_get_operator_password(self, event: ActionEvent) -> None: """Returns the password for the operator user as an action response.""" - event.set_results({"operator-password": self._get_operator_password()}) + event.set_results({USER_PASSWORD_KEY: self._get_operator_password()}) def _on_get_primary(self, event: ActionEvent) -> None: """Get primary instance.""" @@ -536,12 +536,12 @@ def _peers(self) -> Relation: def _get_operator_password(self) -> str: """Get operator user password.""" - return self._get_secret("app", "operator-password") + return self._get_secret("app", USER_PASSWORD_KEY) @property def _replication_password(self) -> str: """Get replication user password.""" - return self._get_secret("app", "replication-password") + return self._get_secret("app", REPLICATION_PASSWORD_KEY) def _unit_name_to_pod_name(self, unit_name: str) -> str: """Converts unit name to pod name. diff --git a/src/constants.py b/src/constants.py index a7e2f4c531..7ba366022e 100644 --- a/src/constants.py +++ b/src/constants.py @@ -5,4 +5,6 @@ DATABASE_PORT = "5432" PEER = "database-peers" +REPLICATION_PASSWORD_KEY = "replication-password" USER = "operator" +USER_PASSWORD_KEY = "operator-password" From 4735754b5afe6b7d32ca15d524f0c4f3be3aba1d Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 23 Aug 2022 16:22:37 -0300 Subject: [PATCH 07/25] Update part of the code to correct actions --- actions.yaml | 23 +++++++++------- src/charm.py | 47 ++++++++++++++++++-------------- src/constants.py | 1 + tests/integration/helpers.py | 2 +- tests/unit/test_charm.py | 53 ++++++++++++++++++++++++++++-------- 5 files changed, 83 insertions(+), 43 deletions(-) diff --git a/actions.yaml b/actions.yaml index 0a44a220b0..6b536158b7 100644 --- a/actions.yaml +++ b/actions.yaml @@ -3,16 +3,19 @@ get-primary: description: Get the unit with is the primary/leader in the replication. -get-operator-password: - description: Get the operator user password used by charm. - It is internal charm user, SHOULD NOT be used by applications. -rotate-users-passwords: - description: Change the operator user password used by charm. - It is internal charm user, SHOULD NOT be used by applications. +get-password: + description: Change the system user's password, which is used by charm. + It is for internal charm users and SHOULD NOT be used by applications. params: - user: + username: type: string - description: The password will be auto-generated if this option is not specified. - password: + description: The username, the default value 'operator'. + Possible values - operator, replication. +set-password: + description: Change the system user's password, which is used by charm. + It is for internal charm users and SHOULD NOT be used by applications. + params: + username: type: string - description: The password will be auto-generated if this option is not specified. + description: The username, the default value 'operator'. + Possible values - operator, replication. diff --git a/src/charm.py b/src/charm.py index 7af41faaba..ba69a8b4c4 100755 --- a/src/charm.py +++ b/src/charm.py @@ -68,12 +68,8 @@ def __init__(self, *args): self.framework.observe(self.on[PEER].relation_departed, self._on_peer_relation_departed) self.framework.observe(self.on.postgresql_pebble_ready, self._on_postgresql_pebble_ready) self.framework.observe(self.on.upgrade_charm, self._on_upgrade_charm) - self.framework.observe( - self.on.get_operator_password_action, self._on_get_operator_password - ) - self.framework.observe( - self.on.rotate_users_passwords_action, self._on_rotate_users_passwords - ) + self.framework.observe(self.on.get_password_action, self._on_get_password) + self.framework.observe(self.on.set_password_action, self._on_set_password) self.framework.observe(self.on.get_primary_action, self._on_get_primary) self.framework.observe(self.on.update_status, self._on_update_status) self._storage_path = self.meta.storages["pgdata"].location @@ -130,7 +126,7 @@ def postgresql(self) -> PostgreSQL: return PostgreSQL( host=self.primary_endpoint, user=USER, - password=self._get_operator_password(), + password=self._get_password(), database="postgres", ) @@ -438,11 +434,17 @@ def _create_resources(self) -> None: self.unit.status = BlockedStatus(f"failed to create services {e}") return - def _on_get_operator_password(self, event: ActionEvent) -> None: - """Returns the password for the operator user as an action response.""" - event.set_results({USER_PASSWORD_KEY: self._get_operator_password()}) + def _on_get_password(self, event: ActionEvent) -> None: + """Returns the password for a user as an action response. - def _on_rotate_users_passwords(self, event: ActionEvent) -> None: + If no user is provided, the password of the operator user is returned. + """ + user = USER + if "username" in event.params: + user = event.params["username"] + event.set_results({f"{user}-password": self._get_password(user)}) + + def _on_set_password(self, event: ActionEvent) -> None: """Rotate the password for all system users or the specified user.""" # Only the leader can write the new password into peer relation. if not self.unit.is_leader(): @@ -575,9 +577,9 @@ def _postgresql_layer(self) -> Layer: "PATRONI_NAME": pod_name, "PATRONI_SCOPE": f"patroni-{self._name}", "PATRONI_REPLICATION_USERNAME": REPLICATION_USER, - "PATRONI_REPLICATION_PASSWORD": self._replication_password, + "PATRONI_REPLICATION_PASSWORD": self._get_password("replication"), "PATRONI_SUPERUSER_USERNAME": USER, - "PATRONI_SUPERUSER_PASSWORD": self._get_operator_password(), + "PATRONI_SUPERUSER_PASSWORD": self._get_password(), }, } }, @@ -594,14 +596,19 @@ def _peers(self) -> Relation: """ return self.model.get_relation(PEER) - def _get_operator_password(self) -> str: - """Get operator user password.""" - return self._get_secret("app", USER_PASSWORD_KEY) + def _get_password(self, user: str = None) -> str: + """Get operator user password. - @property - def _replication_password(self) -> str: - """Get replication user password.""" - return self._get_secret("app", REPLICATION_PASSWORD_KEY) + Args: + user: the user to retrieve the password. + Defaults to operator user. + + Returns: + the user password. + """ + if user is None: + user = USER + return self._get_secret("app", f"{user}-password") def _unit_name_to_pod_name(self, unit_name: str) -> str: """Converts unit name to pod name. diff --git a/src/constants.py b/src/constants.py index 05a0eb7745..860e98b296 100644 --- a/src/constants.py +++ b/src/constants.py @@ -9,4 +9,5 @@ REPLICATION_PASSWORD_KEY = "replication-password" USER = "operator" USER_PASSWORD_KEY = "operator-password" +# List of system usernames needed for correct work of the charm/workload. SYSTEM_USERS = [REPLICATION_USER, USER] diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 21a8bc71bf..26e5cddf2b 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -199,7 +199,7 @@ def get_application_units(ops_test: OpsTest, application_name: str) -> List[str] async def get_operator_password(ops_test: OpsTest): """Retrieve the operator user password using the action.""" unit = ops_test.model.units.get(f"{DATABASE_APP_NAME}/0") - action = await unit.run_action("get-operator-password") + action = await unit.run_action("get-password") result = await action.wait() return result.results["operator-password"] diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 45d76104b9..068e47e753 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -2,7 +2,7 @@ # See LICENSE file for licensing details. import unittest -from unittest.mock import Mock, patch +from unittest.mock import MagicMock, Mock, patch from lightkube import codecs from lightkube.resources.core_v1 import Pod @@ -104,14 +104,21 @@ def test_on_postgresql_pebble_ready( self.assertEqual(container.get_service(self._postgresql_service).is_running(), True) _render_patroni_yml_file.assert_called_once() - @patch("charm.PostgresqlOperatorCharm._get_operator_password") - def test_on_get_operator_password(self, _get_operator_password): - mock_event = Mock() - _get_operator_password.return_value = "test-password" - self.charm._on_get_operator_password(mock_event) - _get_operator_password.assert_called_once() + @patch("charm.PostgresqlOperatorCharm._get_password") + def test_on_get_password(self, _get_password): + # Test without providing the username option. + mock_event = MagicMock(params={}) + _get_password.return_value = "test-password" + self.charm._on_get_password(mock_event) + _get_password.assert_called_once() mock_event.set_results.assert_called_once_with({"operator-password": "test-password"}) + # Also test providing the username option. + mock_event.reset_mock() + mock_event.params["username"] = "replication" + self.charm._on_get_password(mock_event) + mock_event.set_results.assert_called_once_with({"replication-password": "test-password"}) + @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.get_primary") def test_on_get_primary(self, _get_primary): @@ -130,6 +137,21 @@ def test_fail_to_get_primary(self, _get_primary): _get_primary.assert_called_once() mock_event.set_results.assert_not_called() + # @patch("charm.Patroni.rotate_users_passwords") + # def test_on_rotate_users_passwords(self): + # self.harness.add_relation(self._peer_relation, self.charm.app.name) + # with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: + # mock_event = Mock() + # + # self.harness.set_leader(False) + # self.charm._on_rotate_users_passwords(mock_event) + # # mock_event.set_results.assert_called_once_with + # # ({"operator-password": "test-password"}) + # postgresql_mock.rotate_users_passwords.assert_not_called() + # + # self.harness.set_leader(True) + # postgresql_mock.rotate_users_passwords.assert_called_once() + @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.get_primary") def test_on_update_status( @@ -222,9 +244,9 @@ def test_postgresql_layer(self, _, __, ___, ____): "PATRONI_NAME": "postgresql-k8s-0", "PATRONI_SCOPE": f"patroni-{self.charm._name}", "PATRONI_REPLICATION_USERNAME": "replication", - "PATRONI_REPLICATION_PASSWORD": self.charm._replication_password, + "PATRONI_REPLICATION_PASSWORD": self.charm._get_password("replication"), "PATRONI_SUPERUSER_USERNAME": "operator", - "PATRONI_SUPERUSER_PASSWORD": self.charm._get_operator_password(), + "PATRONI_SUPERUSER_PASSWORD": self.charm._get_password(), }, } }, @@ -235,16 +257,23 @@ def test_postgresql_layer(self, _, __, ___, ____): @patch("charm.Patroni.render_postgresql_conf_file") @patch("charm.PostgresqlOperatorCharm._patch_pod_labels") @patch("charm.PostgresqlOperatorCharm._create_resources") - def test_get_operator_password(self, _, __, ___, ____): + def test_get_password(self, _, __, ___, ____): # Test for a None password. - self.assertIsNone(self.charm._get_operator_password()) + self.assertIsNone(self.charm._get_password()) # Then test for a non empty password after leader election and peer data set. self.harness.set_leader() - password = self.charm._get_operator_password() + password = self.charm._get_password() self.assertIsNotNone(password) self.assertNotEqual(password, "") + # And also test that when requesting the password for another user + # it is different from the password from the default user. + replication_password = self.charm._get_password("replication") + self.assertIsNotNone(replication_password) + self.assertNotEqual(replication_password, "") + self.assertNotEqual(replication_password, password) + @patch("charm.Patroni.reload_patroni_configuration") @patch("charm.Patroni.render_postgresql_conf_file") @patch("charm.PostgresqlOperatorCharm._create_resources") From f7024e4b6c4f5b58de60c9e630433727df6c2498 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 24 Aug 2022 13:46:00 -0300 Subject: [PATCH 08/25] Implement password rotation --- lib/charms/postgresql_k8s/v0/postgresql.py | 41 +++----- src/charm.py | 111 +++++++++++---------- src/patroni.py | 13 ++- templates/patroni.yml.j2 | 5 + tests/unit/test_charm.py | 42 +++----- tests/unit/test_patroni.py | 18 +++- 6 files changed, 121 insertions(+), 109 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 4888e65346..d030c657f5 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -19,7 +19,6 @@ Any charm using this library should import the `psycopg2` or `psycopg2-binary` dependency. """ import logging -from typing import Dict import psycopg2 from psycopg2 import sql @@ -54,8 +53,8 @@ class PostgreSQLGetPostgreSQLVersionError(Exception): """Exception raised when retrieving PostgreSQL version fails.""" -class PostgreSQLSetUserPasswordError(Exception): - """Exception raised when setting/updating a user password fails.""" +class PostgreSQLUpdateUserPasswordError(Exception): + """Exception raised when updating a user password fails.""" class PostgreSQL: @@ -73,16 +72,12 @@ def __init__( self.password = password self.database = database - def _connect_to_database( - self, database: str = None, autocommit: bool = True - ) -> psycopg2.extensions.connection: + def _connect_to_database(self, database: str = None) -> psycopg2.extensions.connection: """Creates a connection to the database. Args: database: database to connect to (defaults to the database provided when the object for this class was created). - autocommit: whether every command issued to the database - should be immediately committed. Returns: psycopg2 connection object. @@ -90,8 +85,7 @@ def _connect_to_database( connection = psycopg2.connect( f"dbname='{database if database else self.database}' user='{self.user}' host='{self.host}' password='{self.password}' connect_timeout=1" ) - if autocommit: - connection.autocommit = True + connection.autocommit = True return connection def create_database(self, database: str, user: str) -> None: @@ -184,29 +178,26 @@ def get_postgresql_version(self) -> str: logger.error(f"Failed to get PostgreSQL version: {e}") raise PostgreSQLGetPostgreSQLVersionError() - def rotate_users_passwords(self, users_with_passwords: Dict[str, str]) -> None: - """Rotates one or more users passwords. + def update_user_password(self, username: str, password: str) -> None: + """Update a user password. Args: - users_with_passwords: a dict following the format {"user": "password"}. - It can contain multiple users. + username: the user to update the password. + password: the new password for the user. Raises: - PostgreSQLSetUserPasswordError if any user password couldn't be changed. + PostgreSQLUpdateUserPasswordError if the password couldn't be changed. """ try: - with self._connect_to_database( - autocommit=False - ) as connection, connection.cursor() as cursor: - for user, password in users_with_passwords.items(): - cursor.execute( - sql.SQL( - "ALTER USER {} WITH ENCRYPTED PASSWORD '" + password + "';" - ).format(sql.Identifier(user)) + with self._connect_to_database() as connection, connection.cursor() as cursor: + cursor.execute( + sql.SQL("ALTER USER {} WITH ENCRYPTED PASSWORD '" + password + "';").format( + sql.Identifier(username) ) + ) except psycopg2.Error as e: - logger.error(f"Failed to rotate user password: {e}") - raise PostgreSQLSetUserPasswordError() + logger.error(f"Failed to update user password: {e}") + raise PostgreSQLUpdateUserPasswordError() finally: if connection is not None: connection.close() diff --git a/src/charm.py b/src/charm.py index ba69a8b4c4..57c52abdcd 100755 --- a/src/charm.py +++ b/src/charm.py @@ -9,7 +9,7 @@ from charms.postgresql_k8s.v0.postgresql import ( PostgreSQL, - PostgreSQLSetUserPasswordError, + PostgreSQLUpdateUserPasswordError, ) from lightkube import ApiError, Client, codecs from lightkube.resources.core_v1 import Pod @@ -126,7 +126,7 @@ def postgresql(self) -> PostgreSQL: return PostgreSQL( host=self.primary_endpoint, user=USER, - password=self._get_password(), + password=self._get_secret("app", f"{USER}-password"), database="postgres", ) @@ -186,6 +186,11 @@ def _on_peer_relation_changed(self, event: RelationChangedEvent) -> None: # one at a time. if self.unit.is_leader(): self._add_members(event) + else: + # Update the Patroni configuration in this unit to use new passwords when they change. + # The config is reloaded later in `update_cluster_members` if the member has already + # started, or it is loaded the first time Patroni starts. + self._patroni.render_patroni_yml_file() # Don't update this member before it's part of the members list. if self._endpoint not in self._endpoints: @@ -439,57 +444,71 @@ def _on_get_password(self, event: ActionEvent) -> None: If no user is provided, the password of the operator user is returned. """ - user = USER + username = USER if "username" in event.params: - user = event.params["username"] - event.set_results({f"{user}-password": self._get_password(user)}) + username = event.params["username"] + if username not in SYSTEM_USERS: + event.fail( + f"The action can be run only for users used by the charm: {SYSTEM_USERS} not {username}" + ) + return + event.set_results( + {f"{username}-password": self._get_secret("app", f"{username}-password")} + ) def _on_set_password(self, event: ActionEvent) -> None: - """Rotate the password for all system users or the specified user.""" - # Only the leader can write the new password into peer relation. + """Set the password for the specified user.""" + # Only leader can write the new password into peer relation. if not self.unit.is_leader(): - event.fail("The action can be run only on the leader unit") + event.fail("The action can be run only on leader unit") return - if "user" in event.params: - user = event.params["user"] + username = USER + if "username" in event.params: + username = event.params["username"] + if username not in SYSTEM_USERS: + event.fail( + f"The action can be run only for users used by the charm: {SYSTEM_USERS} not {username}." + ) + return - # Fail if the user is not a system user. - # One example is users created through relations. - if user not in SYSTEM_USERS: - event.fail(f"User {user} is not a system user") - return + password = new_password() + if "password" in event.params: + password = event.params["password"] - # Generate a new password and use it if no password was provided to the action. - users = { - user: event.params["password"] if "password" in event.params else new_password() - } - else: - if "password" in event.params: - event.fail("The same password cannot be set for multiple users") - return + if password == self._get_secret("app", f"{username}-password"): + event.log("The old and new passwords are equal.") + event.set_results({f"{username}-password": password}) + return - users = {user: new_password() for user in SYSTEM_USERS} + # Ensure all members are ready before trying to reload Patroni + # configuration to avoid errors (like the API not responding in + # one instance because PostgreSQL and/or Patroni are not ready). + if not self._patroni.are_all_members_ready(): + event.fail( + "Failed changing the password: Not all members healthy or finished initial sync." + ) + return + # Update the password in the PostgreSQL instance. try: - self.postgresql.rotate_users_passwords(users) - except PostgreSQLSetUserPasswordError as e: - event.fail(f"Failed to set user password with error {e}") + self.postgresql.update_user_password(username, password) + except PostgreSQLUpdateUserPasswordError as e: + logger.exception(e) + event.fail( + "Failed changing the password: Not all members healthy or finished initial sync." + ) return - # Update the password in the peer relation if the operation was successful. - for user, password in users.items(): - self._peers.data[self.app].update({f"{user}-password": password}) + # Update the password in the secret store. + self._set_secret("app", f"{username}-password", password) - # for unit in + # Update and reload Patroni configuration in this unit to use the new password. + # Other units Patroni configuration will be + self._patroni.render_patroni_yml_file() self._patroni.reload_patroni_configuration() - # Return the generated password when the user option is given. - if "user" in event.params and "password" not in event.params: - user = event.params["user"] - event.set_results( - {f"{user}-password": self._peers.data[self.app].get(f"{user}-password")} - ) + event.set_results({f"{username}-password": password}) def _on_get_primary(self, event: ActionEvent) -> None: """Get primary instance.""" @@ -516,6 +535,8 @@ def _patroni(self): self._namespace, self.app.planned_units(), self._storage_path, + self._get_secret("app", USER_PASSWORD_KEY), + self._get_secret("app", REPLICATION_PASSWORD_KEY), ) @property @@ -577,9 +598,7 @@ def _postgresql_layer(self) -> Layer: "PATRONI_NAME": pod_name, "PATRONI_SCOPE": f"patroni-{self._name}", "PATRONI_REPLICATION_USERNAME": REPLICATION_USER, - "PATRONI_REPLICATION_PASSWORD": self._get_password("replication"), "PATRONI_SUPERUSER_USERNAME": USER, - "PATRONI_SUPERUSER_PASSWORD": self._get_password(), }, } }, @@ -596,20 +615,6 @@ def _peers(self) -> Relation: """ return self.model.get_relation(PEER) - def _get_password(self, user: str = None) -> str: - """Get operator user password. - - Args: - user: the user to retrieve the password. - Defaults to operator user. - - Returns: - the user password. - """ - if user is None: - user = USER - return self._get_secret("app", f"{user}-password") - def _unit_name_to_pod_name(self, unit_name: str) -> str: """Converts unit name to pod name. diff --git a/src/patroni.py b/src/patroni.py index 9010b95b70..df8e5c4a68 100644 --- a/src/patroni.py +++ b/src/patroni.py @@ -32,13 +32,22 @@ class Patroni: """This class handles the communication with Patroni API and configuration files.""" def __init__( - self, endpoint: str, endpoints: List[str], namespace: str, planned_units, storage_path: str + self, + endpoint: str, + endpoints: List[str], + namespace: str, + planned_units, + storage_path: str, + superuser_password: str, + replication_password: str, ): self._endpoint = endpoint self._endpoints = endpoints self._namespace = namespace self._storage_path = storage_path self._planned_units = planned_units + self._superuser_password = superuser_password + self._replication_password = replication_password def get_primary(self, unit_name_pattern=False) -> str: """Get primary instance. @@ -137,6 +146,8 @@ def render_patroni_yml_file(self) -> None: endpoints=self._endpoints, namespace=self._namespace, storage_path=self._storage_path, + superuser_password=self._superuser_password, + replication_password=self._replication_password, ) self._render_file(f"{self._storage_path}/patroni.yml", rendered, 0o644) diff --git a/templates/patroni.yml.j2 b/templates/patroni.yml.j2 index 8d84d9ddaf..54da2d62ef 100644 --- a/templates/patroni.yml.j2 +++ b/templates/patroni.yml.j2 @@ -30,4 +30,9 @@ postgresql: {%- for endpoint in endpoints %} - host replication replication {{ endpoint }}.{{ namespace }}.svc.cluster.local md5 {%- endfor %} + authentication: + replication: + password: {{ replication_password }} + superuser: + password: {{ superuser_password }} use_endpoints: true diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 068e47e753..34cbe8bcd9 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -104,20 +104,29 @@ def test_on_postgresql_pebble_ready( self.assertEqual(container.get_service(self._postgresql_service).is_running(), True) _render_patroni_yml_file.assert_called_once() - @patch("charm.PostgresqlOperatorCharm._get_password") - def test_on_get_password(self, _get_password): + def test_on_get_password(self): + # Set passwords in peer relation data. + self.harness.update_relation_data( + self.rel_id, + self.charm.app.name, + { + "operator-password": "test-password", + "replication-password": "replication-test-password", + }, + ) + # Test without providing the username option. mock_event = MagicMock(params={}) - _get_password.return_value = "test-password" self.charm._on_get_password(mock_event) - _get_password.assert_called_once() mock_event.set_results.assert_called_once_with({"operator-password": "test-password"}) # Also test providing the username option. mock_event.reset_mock() mock_event.params["username"] = "replication" self.charm._on_get_password(mock_event) - mock_event.set_results.assert_called_once_with({"replication-password": "test-password"}) + mock_event.set_results.assert_called_once_with( + {"replication-password": "replication-test-password"} + ) @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.get_primary") @@ -244,36 +253,13 @@ def test_postgresql_layer(self, _, __, ___, ____): "PATRONI_NAME": "postgresql-k8s-0", "PATRONI_SCOPE": f"patroni-{self.charm._name}", "PATRONI_REPLICATION_USERNAME": "replication", - "PATRONI_REPLICATION_PASSWORD": self.charm._get_password("replication"), "PATRONI_SUPERUSER_USERNAME": "operator", - "PATRONI_SUPERUSER_PASSWORD": self.charm._get_password(), }, } }, } self.assertDictEqual(plan, expected) - @patch("charm.Patroni.reload_patroni_configuration") - @patch("charm.Patroni.render_postgresql_conf_file") - @patch("charm.PostgresqlOperatorCharm._patch_pod_labels") - @patch("charm.PostgresqlOperatorCharm._create_resources") - def test_get_password(self, _, __, ___, ____): - # Test for a None password. - self.assertIsNone(self.charm._get_password()) - - # Then test for a non empty password after leader election and peer data set. - self.harness.set_leader() - password = self.charm._get_password() - self.assertIsNotNone(password) - self.assertNotEqual(password, "") - - # And also test that when requesting the password for another user - # it is different from the password from the default user. - replication_password = self.charm._get_password("replication") - self.assertIsNotNone(replication_password) - self.assertNotEqual(replication_password, "") - self.assertNotEqual(replication_password, password) - @patch("charm.Patroni.reload_patroni_configuration") @patch("charm.Patroni.render_postgresql_conf_file") @patch("charm.PostgresqlOperatorCharm._create_resources") diff --git a/tests/unit/test_patroni.py b/tests/unit/test_patroni.py index 5fec3da7b9..f023a10cf6 100644 --- a/tests/unit/test_patroni.py +++ b/tests/unit/test_patroni.py @@ -15,7 +15,13 @@ class TestPatroni(unittest.TestCase): def setUp(self): # Setup Patroni wrapper. self.patroni = Patroni( - "postgresql-k8s-0", "postgresql-k8s-0", "test-model", 1, STORAGE_PATH + "postgresql-k8s-0", + "postgresql-k8s-0", + "test-model", + 1, + STORAGE_PATH, + "superuser-password", + "replication-password", ) @patch("requests.get") @@ -77,6 +83,8 @@ def test_render_patroni_yml_file(self, _render_file): endpoints=self.patroni._endpoints, namespace=self.patroni._namespace, storage_path=self.patroni._storage_path, + superuser_password=self.patroni._superuser_password, + replication_password=self.patroni._replication_password, ) # Setup a mock for the `open` method, set returned data to postgresql.conf template. @@ -128,7 +136,13 @@ def test_render_postgresql_conf_file(self, _render_file): # Also test with multiple planned units (synchronous_commit is turned on). self.patroni = Patroni( - "postgresql-k8s-0", "postgresql-k8s-0", "test-model", 3, STORAGE_PATH + "postgresql-k8s-0", + "postgresql-k8s-0", + "test-model", + 3, + STORAGE_PATH, + "superuser-password", + "replication-password", ) expected_content = template.render( logging_collector="on", From 9c9b21198adfd9b8eebd4ce04ba5f6ae8b8a10aa Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 24 Aug 2022 13:54:54 -0300 Subject: [PATCH 09/25] Improve messages --- src/charm.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/charm.py b/src/charm.py index 57c52abdcd..044fe41ee6 100755 --- a/src/charm.py +++ b/src/charm.py @@ -449,7 +449,8 @@ def _on_get_password(self, event: ActionEvent) -> None: username = event.params["username"] if username not in SYSTEM_USERS: event.fail( - f"The action can be run only for users used by the charm: {SYSTEM_USERS} not {username}" + f"The action can be run only for users used by the charm or Patroni:" + f" {', '.join(SYSTEM_USERS)} not {username}" ) return event.set_results( @@ -468,7 +469,8 @@ def _on_set_password(self, event: ActionEvent) -> None: username = event.params["username"] if username not in SYSTEM_USERS: event.fail( - f"The action can be run only for users used by the charm: {SYSTEM_USERS} not {username}." + f"The action can be run only for users used by the charm:" + f" {', '.join(SYSTEM_USERS)} not {username}" ) return From 4b7a534ec31a394b2067463277c3a62158272abd Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 24 Aug 2022 13:59:35 -0300 Subject: [PATCH 10/25] Improve get password test --- tests/unit/test_charm.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 34cbe8bcd9..1d64cfc0c3 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -105,7 +105,8 @@ def test_on_postgresql_pebble_ready( _render_patroni_yml_file.assert_called_once() def test_on_get_password(self): - # Set passwords in peer relation data. + # Create a mock event and set passwords in peer relation data. + mock_event = MagicMock(params={}) self.harness.update_relation_data( self.rel_id, self.charm.app.name, @@ -115,8 +116,15 @@ def test_on_get_password(self): }, ) + # Test providing an invalid username. + mock_event.params["username"] = "user" + self.charm._on_get_password(mock_event) + mock_event.fail.assert_called_once() + mock_event.set_results.assert_not_called() + # Test without providing the username option. - mock_event = MagicMock(params={}) + mock_event.reset_mock() + del mock_event.params["username"] self.charm._on_get_password(mock_event) mock_event.set_results.assert_called_once_with({"operator-password": "test-password"}) From c92fdbcfad37b2a9c97a81b80d9568faf376466a Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 24 Aug 2022 15:42:47 -0300 Subject: [PATCH 11/25] Fix comment and remove unneeded else block --- src/charm.py | 7 +------ tests/integration/test_password_rotation.py | 0 2 files changed, 1 insertion(+), 6 deletions(-) create mode 100644 tests/integration/test_password_rotation.py diff --git a/src/charm.py b/src/charm.py index 044fe41ee6..7881369163 100755 --- a/src/charm.py +++ b/src/charm.py @@ -186,11 +186,6 @@ def _on_peer_relation_changed(self, event: RelationChangedEvent) -> None: # one at a time. if self.unit.is_leader(): self._add_members(event) - else: - # Update the Patroni configuration in this unit to use new passwords when they change. - # The config is reloaded later in `update_cluster_members` if the member has already - # started, or it is loaded the first time Patroni starts. - self._patroni.render_patroni_yml_file() # Don't update this member before it's part of the members list. if self._endpoint not in self._endpoints: @@ -506,7 +501,7 @@ def _on_set_password(self, event: ActionEvent) -> None: self._set_secret("app", f"{username}-password", password) # Update and reload Patroni configuration in this unit to use the new password. - # Other units Patroni configuration will be + # Other units Patroni configuration will be reloaded in the peer relation changed event. self._patroni.render_patroni_yml_file() self._patroni.reload_patroni_configuration() diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py new file mode 100644 index 0000000000..e69de29bb2 From 0d7ea40bbf8827e7553410794628f6cd193c4921 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 24 Aug 2022 16:32:52 -0300 Subject: [PATCH 12/25] Add set password unit test --- tests/unit/test_charm.py | 85 ++++++++++++++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 16 deletions(-) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 1d64cfc0c3..7278c74e69 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -2,8 +2,9 @@ # See LICENSE file for licensing details. import unittest -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import MagicMock, Mock, PropertyMock, patch +from charms.postgresql_k8s.v0.postgresql import PostgreSQLUpdateUserPasswordError from lightkube import codecs from lightkube.resources.core_v1 import Pod from ops.model import ActiveStatus @@ -136,6 +137,73 @@ def test_on_get_password(self): {"replication-password": "replication-test-password"} ) + @patch("charm.Patroni.reload_patroni_configuration") + @patch("charm.Patroni.render_patroni_yml_file") + @patch("charm.PostgresqlOperatorCharm._set_secret") + @patch("charm.Patroni.are_all_members_ready") + @patch("charm.PostgresqlOperatorCharm._on_leader_elected") + def test_on_set_password( + self, + _, + _are_all_members_ready, + _set_secret, + _render_patroni_yml_file, + _reload_patroni_configuration, + ): + with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: + # Create a mock event. + mock_event = MagicMock(params={}) + + # Set some values for the other mocks. + _are_all_members_ready.side_effect = [False, True, True, True, True] + postgresql_mock.update_user_password = PropertyMock( + side_effect=[PostgreSQLUpdateUserPasswordError, None, None, None] + ) + + # Test trying to set a password through a non leader unit. + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test providing an invalid username. + self.harness.set_leader() + mock_event.reset_mock() + mock_event.params["username"] = "user" + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test without providing the username option but without all cluster members ready. + mock_event.reset_mock() + del mock_event.params["username"] + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test for an error updating when updating the user password in the database. + mock_event.reset_mock() + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test without providing the username option. + self.charm._on_set_password(mock_event) + self.assertEqual(_set_secret.call_args_list[0][0][1], "operator-password") + + # Also test providing the username option. + _set_secret.reset_mock() + mock_event.params["username"] = "replication" + self.charm._on_set_password(mock_event) + self.assertEqual(_set_secret.call_args_list[0][0][1], "replication-password") + + # And test providing both the username and password options. + _set_secret.reset_mock() + mock_event.params["password"] = "replication-test-password" + self.charm._on_set_password(mock_event) + _set_secret.assert_called_once_with( + "app", "replication-password", "replication-test-password" + ) + @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.get_primary") def test_on_get_primary(self, _get_primary): @@ -154,21 +222,6 @@ def test_fail_to_get_primary(self, _get_primary): _get_primary.assert_called_once() mock_event.set_results.assert_not_called() - # @patch("charm.Patroni.rotate_users_passwords") - # def test_on_rotate_users_passwords(self): - # self.harness.add_relation(self._peer_relation, self.charm.app.name) - # with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: - # mock_event = Mock() - # - # self.harness.set_leader(False) - # self.charm._on_rotate_users_passwords(mock_event) - # # mock_event.set_results.assert_called_once_with - # # ({"operator-password": "test-password"}) - # postgresql_mock.rotate_users_passwords.assert_not_called() - # - # self.harness.set_leader(True) - # postgresql_mock.rotate_users_passwords.assert_called_once() - @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.get_primary") def test_on_update_status( From 71f86ac9b01a407d9058e4fcbcf26552e29768a5 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Wed, 24 Aug 2022 16:34:58 -0300 Subject: [PATCH 13/25] Improve unit test --- tests/unit/test_charm.py | 105 ++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 52 deletions(-) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 7278c74e69..f3d54dda6d 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -140,69 +140,70 @@ def test_on_get_password(self): @patch("charm.Patroni.reload_patroni_configuration") @patch("charm.Patroni.render_patroni_yml_file") @patch("charm.PostgresqlOperatorCharm._set_secret") + @patch("charm.PostgresqlOperatorCharm.postgresql") @patch("charm.Patroni.are_all_members_ready") @patch("charm.PostgresqlOperatorCharm._on_leader_elected") def test_on_set_password( self, _, _are_all_members_ready, + _postgresql, _set_secret, _render_patroni_yml_file, _reload_patroni_configuration, ): - with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: - # Create a mock event. - mock_event = MagicMock(params={}) - - # Set some values for the other mocks. - _are_all_members_ready.side_effect = [False, True, True, True, True] - postgresql_mock.update_user_password = PropertyMock( - side_effect=[PostgreSQLUpdateUserPasswordError, None, None, None] - ) + # Create a mock event. + mock_event = MagicMock(params={}) - # Test trying to set a password through a non leader unit. - self.charm._on_set_password(mock_event) - mock_event.fail.assert_called_once() - _set_secret.assert_not_called() - - # Test providing an invalid username. - self.harness.set_leader() - mock_event.reset_mock() - mock_event.params["username"] = "user" - self.charm._on_set_password(mock_event) - mock_event.fail.assert_called_once() - _set_secret.assert_not_called() - - # Test without providing the username option but without all cluster members ready. - mock_event.reset_mock() - del mock_event.params["username"] - self.charm._on_set_password(mock_event) - mock_event.fail.assert_called_once() - _set_secret.assert_not_called() - - # Test for an error updating when updating the user password in the database. - mock_event.reset_mock() - self.charm._on_set_password(mock_event) - mock_event.fail.assert_called_once() - _set_secret.assert_not_called() - - # Test without providing the username option. - self.charm._on_set_password(mock_event) - self.assertEqual(_set_secret.call_args_list[0][0][1], "operator-password") - - # Also test providing the username option. - _set_secret.reset_mock() - mock_event.params["username"] = "replication" - self.charm._on_set_password(mock_event) - self.assertEqual(_set_secret.call_args_list[0][0][1], "replication-password") - - # And test providing both the username and password options. - _set_secret.reset_mock() - mock_event.params["password"] = "replication-test-password" - self.charm._on_set_password(mock_event) - _set_secret.assert_called_once_with( - "app", "replication-password", "replication-test-password" - ) + # Set some values for the other mocks. + _are_all_members_ready.side_effect = [False, True, True, True, True] + _postgresql.update_user_password = PropertyMock( + side_effect=[PostgreSQLUpdateUserPasswordError, None, None, None] + ) + + # Test trying to set a password through a non leader unit. + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test providing an invalid username. + self.harness.set_leader() + mock_event.reset_mock() + mock_event.params["username"] = "user" + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test without providing the username option but without all cluster members ready. + mock_event.reset_mock() + del mock_event.params["username"] + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test for an error updating when updating the user password in the database. + mock_event.reset_mock() + self.charm._on_set_password(mock_event) + mock_event.fail.assert_called_once() + _set_secret.assert_not_called() + + # Test without providing the username option. + self.charm._on_set_password(mock_event) + self.assertEqual(_set_secret.call_args_list[0][0][1], "operator-password") + + # Also test providing the username option. + _set_secret.reset_mock() + mock_event.params["username"] = "replication" + self.charm._on_set_password(mock_event) + self.assertEqual(_set_secret.call_args_list[0][0][1], "replication-password") + + # And test providing both the username and password options. + _set_secret.reset_mock() + mock_event.params["password"] = "replication-test-password" + self.charm._on_set_password(mock_event) + _set_secret.assert_called_once_with( + "app", "replication-password", "replication-test-password" + ) @patch_network_get(private_address="1.1.1.1") @patch("charm.Patroni.get_primary") From a728a2018d151e5f87518414dc46d08fe68bfc90 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 08:24:32 -0300 Subject: [PATCH 14/25] Change helper function --- tests/integration/helpers.py | 12 ++++++------ tests/integration/test_charm.py | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 26e5cddf2b..e001750c47 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -30,7 +30,7 @@ async def check_database_users_existence( """ unit = ops_test.model.applications[DATABASE_APP_NAME].units[0] unit_address = await get_unit_address(ops_test, unit.name) - password = await get_operator_password(ops_test) + password = await get_password(ops_test) # Retrieve all users in the database. output = await execute_query_on_unit( @@ -61,7 +61,7 @@ async def check_database_creation(ops_test: OpsTest, database: str) -> None: ops_test: The ops test framework database: Name of the database that should have been created """ - password = await get_operator_password(ops_test) + password = await get_password(ops_test) for unit in ops_test.model.applications[DATABASE_APP_NAME].units: unit_address = await get_unit_address(ops_test, unit.name) @@ -196,12 +196,12 @@ def get_application_units(ops_test: OpsTest, application_name: str) -> List[str] ] -async def get_operator_password(ops_test: OpsTest): - """Retrieve the operator user password using the action.""" +async def get_password(ops_test: OpsTest, username: str = "operator"): + """Retrieve a user password using the action.""" unit = ops_test.model.units.get(f"{DATABASE_APP_NAME}/0") - action = await unit.run_action("get-password") + action = await unit.run_action("get-password", **{"username": username}) result = await action.wait() - return result.results["operator-password"] + return result.results[f"{username}-password"] async def get_primary(ops_test: OpsTest, unit_id=0) -> str: diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 960176c6ac..107ac8d91f 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -18,7 +18,7 @@ convert_records_to_dict, get_application_units, get_cluster_members, - get_operator_password, + get_password, get_unit_address, scale_application, ) @@ -74,7 +74,7 @@ async def test_database_is_up(ops_test: OpsTest, unit_id: int): @pytest.mark.parametrize("unit_id", UNIT_IDS) async def test_settings_are_correct(ops_test: OpsTest, unit_id: int): - password = await get_operator_password(ops_test) + password = await get_password(ops_test) # Connect to PostgreSQL. host = await get_unit_address(ops_test, f"{APP_NAME}/{unit_id}") @@ -162,7 +162,7 @@ async def test_scale_down_and_up(ops_test: OpsTest): async def test_persist_data_through_graceful_restart(ops_test: OpsTest): """Test data persists through a graceful restart.""" primary = await get_primary(ops_test) - password = await get_operator_password(ops_test) + password = await get_password(ops_test) address = await get_unit_address(ops_test, primary) # Write data to primary IP. @@ -190,7 +190,7 @@ async def test_persist_data_through_graceful_restart(ops_test: OpsTest): async def test_persist_data_through_failure(ops_test: OpsTest): """Test data persists through a failure.""" primary = await get_primary(ops_test) - password = await get_operator_password(ops_test) + password = await get_password(ops_test) address = await get_unit_address(ops_test, primary) # Write data to primary IP. From be497bcfd868585a2c51e1121455850606edcd39 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 08:31:03 -0300 Subject: [PATCH 15/25] Add copyright notice --- tests/integration/test_password_rotation.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index e69de29bb2..bf98b476ec 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -0,0 +1,3 @@ +#!/usr/bin/env python3 +# Copyright 2022 Canonical Ltd. +# See LICENSE file for licensing details. From ef2bd31f3bb53fa4a3888d48bd1bd5f591a7373a Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 09:06:50 -0300 Subject: [PATCH 16/25] Improve username retrieval --- src/charm.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/charm.py b/src/charm.py index 7881369163..01631f98d7 100755 --- a/src/charm.py +++ b/src/charm.py @@ -439,9 +439,7 @@ def _on_get_password(self, event: ActionEvent) -> None: If no user is provided, the password of the operator user is returned. """ - username = USER - if "username" in event.params: - username = event.params["username"] + username = event.params.get("username", USER) if username not in SYSTEM_USERS: event.fail( f"The action can be run only for users used by the charm or Patroni:" @@ -459,9 +457,7 @@ def _on_set_password(self, event: ActionEvent) -> None: event.fail("The action can be run only on leader unit") return - username = USER - if "username" in event.params: - username = event.params["username"] + username = event.params.get("username", USER) if username not in SYSTEM_USERS: event.fail( f"The action can be run only for users used by the charm:" From 8988aa3c1d05c2a5824653c847198f8a4f83637d Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 09:07:22 -0300 Subject: [PATCH 17/25] Add initial code for password rotation test --- tests/integration/helpers.py | 19 +++++++ tests/integration/test_password_rotation.py | 61 +++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index e001750c47..5fa7276c93 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -235,6 +235,17 @@ async def get_unit_address(ops_test: OpsTest, unit_name: str) -> str: return status["applications"][unit_name.split("/")[0]].units[unit_name]["address"] +async def restart_patroni(ops_test: OpsTest, unit_name: str) -> None: + """Restart patroni on a specific unit. + + Args: + ops_test: The ops test framework instance + unit_name: The name of the unit + """ + unit_ip = await get_unit_address(ops_test, unit_name) + requests.post(f"http://{unit_ip}:8008/restart") + + async def scale_application(ops_test: OpsTest, application_name: str, scale: int) -> None: """Scale a given application to a specific unit count. @@ -250,3 +261,11 @@ async def scale_application(ops_test: OpsTest, application_name: str, scale: int timeout=1000, wait_for_exact_units=scale, ) + + +async def set_password(ops_test: OpsTest, unit_name: str, username: str = "operator"): + """Retrieve a user password using the action.""" + unit = ops_test.model.units.get(unit_name) + action = await unit.run_action("set-password", **{"username": username}) + result = await action.wait() + return result.results diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index bf98b476ec..fb77de1619 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -1,3 +1,64 @@ #!/usr/bin/env python3 # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. + +import pytest +from pytest_operator.plugin import OpsTest + +from tests.helpers import METADATA +from tests.integration.helpers import get_password, restart_patroni, set_password + +APP_NAME = METADATA["name"] + + +@pytest.mark.skip_if_deployed +@pytest.mark.abort_on_fail +async def test_deploy_active(ops_test: OpsTest): + """Build the charm and deploy it.""" + charm = await ops_test.build_charm(".") + async with ops_test.fast_forward(): + await ops_test.model.deploy( + charm, + resources={ + "postgresql-image": METADATA["resources"]["postgresql-image"]["upstream-source"] + }, + application_name=APP_NAME, + num_units=3, + trust=True, + ) + await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", timeout=1000) + + +async def test_password_rotation(ops_test: OpsTest): + """Test password rotation action.""" + # Get the initial passwords set for the system users. + superuser_password = await get_password(ops_test) + replication_password = await get_password(ops_test, "replication") + + # Get the leader unit name (because passwords can only be set through it). + leader = None + for unit in ops_test.model.applications[APP_NAME].units: + if await unit.is_leader_from_status(): + leader = unit.name + break + + # Change both passwords. + result = await set_password(ops_test, unit_name=leader) + assert "operator-password" in result.keys() + await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", timeout=1000) + + result = await set_password(ops_test, unit_name=leader, username="replication") + assert "replication-password" in result.keys() + await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", timeout=1000) + + new_superuser_password = await get_password(ops_test) + new_replication_password = await get_password(ops_test, "replication") + + assert superuser_password != new_superuser_password + assert replication_password != new_replication_password + + # Restart Patroni in any non-leader unit and check that + # Patroni and PostgreSQL continue to work. + for unit in ops_test.model.applications[APP_NAME].units: + if not await unit.is_leader_from_status(): + await restart_patroni(ops_test, unit.name) From 7c75e45953b9b268aea06d99a44e2be929e7fb22 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 09:08:04 -0300 Subject: [PATCH 18/25] Fix comment --- tests/integration/test_password_rotation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index fb77de1619..3a05a051e7 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -57,7 +57,7 @@ async def test_password_rotation(ops_test: OpsTest): assert superuser_password != new_superuser_password assert replication_password != new_replication_password - # Restart Patroni in any non-leader unit and check that + # Restart Patroni on any non-leader unit and check that # Patroni and PostgreSQL continue to work. for unit in ops_test.model.applications[APP_NAME].units: if not await unit.is_leader_from_status(): From 34421e5ffaa418896451e59f9068e29a6bfc50d0 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 11:47:51 -0300 Subject: [PATCH 19/25] Add test checks --- tests/integration/helpers.py | 34 ++++++++++++++++++++- tests/integration/test_password_rotation.py | 21 ++++++++++--- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 5fa7276c93..06e0be989e 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -84,6 +84,22 @@ async def check_database_creation(ops_test: OpsTest, database: str) -> None: assert len(output) +async def check_patroni(ops_test: OpsTest, unit_name: str) -> bool: + """Check if Patroni is running correctly on a specific unit. + + Args: + ops_test: The ops test framework instance + unit_name: The name of the unit + + Returns: + whether Patroni is running correctly. + """ + unit_ip = await get_unit_address(ops_test, unit_name) + health_info = requests.get(f"http://{unit_ip}:8008/health") + print(health_info.json()) + return health_info.json()["state"] == "running" + + def convert_records_to_dict(records: List[tuple]) -> dict: """Converts psycopg2 records list to a dict.""" records_dict = {} @@ -204,6 +220,22 @@ async def get_password(ops_test: OpsTest, username: str = "operator"): return result.results[f"{username}-password"] +async def get_postgresql_start_time(ops_test: OpsTest, unit_name: str) -> bool: + """Get PostgreSQL start time. + + Args: + ops_test: The ops test framework instance + unit_name: The name of the unit + + Returns: + PostgreSQL start time. + """ + unit_ip = await get_unit_address(ops_test, unit_name) + health_info = requests.get(f"http://{unit_ip}:8008/health") + print(health_info.json()) + return health_info.json()["postmaster_start_time"] + + async def get_primary(ops_test: OpsTest, unit_id=0) -> str: """Get the primary unit. @@ -236,7 +268,7 @@ async def get_unit_address(ops_test: OpsTest, unit_name: str) -> str: async def restart_patroni(ops_test: OpsTest, unit_name: str) -> None: - """Restart patroni on a specific unit. + """Restart Patroni on a specific unit. Args: ops_test: The ops test framework instance diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index 3a05a051e7..5a6e13346b 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -6,7 +6,12 @@ from pytest_operator.plugin import OpsTest from tests.helpers import METADATA -from tests.integration.helpers import get_password, restart_patroni, set_password +from tests.integration.helpers import ( + check_patroni, + get_password, + restart_patroni, + set_password, +) APP_NAME = METADATA["name"] @@ -59,6 +64,14 @@ async def test_password_rotation(ops_test: OpsTest): # Restart Patroni on any non-leader unit and check that # Patroni and PostgreSQL continue to work. - for unit in ops_test.model.applications[APP_NAME].units: - if not await unit.is_leader_from_status(): - await restart_patroni(ops_test, unit.name) + non_leader_units = [ + unit.name: + for unit in ops_test.model.applications[APP_NAME].units + if not await unit.is_leader_from_status() + ] + + for unit in non_leader_units: + await restart_patroni(ops_test, unit) + + for unit in non_leader_units: + assert await check_patroni(ops_test, unit) From 4bc38b81197fa2f741c73f12e0f6fe4a2e71d155 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 11:56:41 -0300 Subject: [PATCH 20/25] Small fix --- tests/integration/test_password_rotation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index 5a6e13346b..9d24127ebb 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -65,7 +65,7 @@ async def test_password_rotation(ops_test: OpsTest): # Restart Patroni on any non-leader unit and check that # Patroni and PostgreSQL continue to work. non_leader_units = [ - unit.name: + unit.name for unit in ops_test.model.applications[APP_NAME].units if not await unit.is_leader_from_status() ] From 14ec0f0ab7eabc53315d4d6aedba75859ba147af Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 13:48:51 -0300 Subject: [PATCH 21/25] Add retry to helper function --- tests/integration/helpers.py | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 06e0be989e..9d226e3c6b 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -9,16 +9,17 @@ import requests import yaml from pytest_operator.plugin import OpsTest +from tenacity import retry, retry_if_result, wait_exponential, stop_after_attempt METADATA = yaml.safe_load(Path("./metadata.yaml").read_text()) DATABASE_APP_NAME = METADATA["name"] async def check_database_users_existence( - ops_test: OpsTest, - users_that_should_exist: List[str], - users_that_should_not_exist: List[str], - admin: bool = False, + ops_test: OpsTest, + users_that_should_exist: List[str], + users_that_should_not_exist: List[str], + admin: bool = False, ) -> None: """Checks that applications users exist in the database. @@ -84,6 +85,11 @@ async def check_database_creation(ops_test: OpsTest, database: str) -> None: assert len(output) +@retry( + retry=retry_if_result(lambda x: not x), + stop=stop_after_attempt(10), + wait=wait_exponential(multiplier=1, min=2, max=30), +) async def check_patroni(ops_test: OpsTest, unit_name: str) -> bool: """Check if Patroni is running correctly on a specific unit. @@ -110,12 +116,12 @@ def convert_records_to_dict(records: List[tuple]) -> dict: async def deploy_and_relate_application_with_postgresql( - ops_test: OpsTest, - charm: str, - application_name: str, - number_of_units: int, - channel: str = "stable", - relation: str = "db", + ops_test: OpsTest, + charm: str, + application_name: str, + number_of_units: int, + channel: str = "stable", + relation: str = "db", ) -> int: """Helper function to deploy and relate application with PostgreSQL. @@ -160,10 +166,10 @@ async def deploy_and_relate_application_with_postgresql( async def execute_query_on_unit( - unit_address: str, - password: str, - query: str, - database: str = "postgres", + unit_address: str, + password: str, + query: str, + database: str = "postgres", ): """Execute given PostgreSQL query on a unit. @@ -177,7 +183,7 @@ async def execute_query_on_unit( A list of rows that were potentially returned from the query. """ with psycopg2.connect( - f"dbname='{database}' user='operator' host='{unit_address}' password='{password}' connect_timeout=10" + f"dbname='{database}' user='operator' host='{unit_address}' password='{password}' connect_timeout=10" ) as connection, connection.cursor() as cursor: cursor.execute(query) output = list(itertools.chain(*cursor.fetchall())) From 16dce63f64e6bb536c89310a36c6f2a96e7aac0e Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 13:50:42 -0300 Subject: [PATCH 22/25] Fix lint problems --- tests/integration/helpers.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 9d226e3c6b..68a9d6d89d 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -9,17 +9,17 @@ import requests import yaml from pytest_operator.plugin import OpsTest -from tenacity import retry, retry_if_result, wait_exponential, stop_after_attempt +from tenacity import retry, retry_if_result, stop_after_attempt, wait_exponential METADATA = yaml.safe_load(Path("./metadata.yaml").read_text()) DATABASE_APP_NAME = METADATA["name"] async def check_database_users_existence( - ops_test: OpsTest, - users_that_should_exist: List[str], - users_that_should_not_exist: List[str], - admin: bool = False, + ops_test: OpsTest, + users_that_should_exist: List[str], + users_that_should_not_exist: List[str], + admin: bool = False, ) -> None: """Checks that applications users exist in the database. @@ -116,12 +116,12 @@ def convert_records_to_dict(records: List[tuple]) -> dict: async def deploy_and_relate_application_with_postgresql( - ops_test: OpsTest, - charm: str, - application_name: str, - number_of_units: int, - channel: str = "stable", - relation: str = "db", + ops_test: OpsTest, + charm: str, + application_name: str, + number_of_units: int, + channel: str = "stable", + relation: str = "db", ) -> int: """Helper function to deploy and relate application with PostgreSQL. @@ -166,10 +166,10 @@ async def deploy_and_relate_application_with_postgresql( async def execute_query_on_unit( - unit_address: str, - password: str, - query: str, - database: str = "postgres", + unit_address: str, + password: str, + query: str, + database: str = "postgres", ): """Execute given PostgreSQL query on a unit. @@ -183,7 +183,7 @@ async def execute_query_on_unit( A list of rows that were potentially returned from the query. """ with psycopg2.connect( - f"dbname='{database}' user='operator' host='{unit_address}' password='{password}' connect_timeout=10" + f"dbname='{database}' user='operator' host='{unit_address}' password='{password}' connect_timeout=10" ) as connection, connection.cursor() as cursor: cursor.execute(query) output = list(itertools.chain(*cursor.fetchall())) From b6f2d8a72b8850fe579b3f30af57d39531677b0b Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 25 Aug 2022 14:57:20 -0300 Subject: [PATCH 23/25] Improve integration test --- tests/integration/helpers.py | 28 ++++++--------------- tests/integration/test_password_rotation.py | 17 +++++-------- 2 files changed, 14 insertions(+), 31 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 68a9d6d89d..77e5808501 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -2,6 +2,7 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. import itertools +from datetime import datetime from pathlib import Path from typing import List @@ -90,20 +91,23 @@ async def check_database_creation(ops_test: OpsTest, database: str) -> None: stop=stop_after_attempt(10), wait=wait_exponential(multiplier=1, min=2, max=30), ) -async def check_patroni(ops_test: OpsTest, unit_name: str) -> bool: +async def check_patroni(ops_test: OpsTest, unit_name: str, restart_time: float) -> bool: """Check if Patroni is running correctly on a specific unit. Args: ops_test: The ops test framework instance unit_name: The name of the unit + restart_time: Point in time before the unit was restarted. Returns: whether Patroni is running correctly. """ unit_ip = await get_unit_address(ops_test, unit_name) - health_info = requests.get(f"http://{unit_ip}:8008/health") - print(health_info.json()) - return health_info.json()["state"] == "running" + health_info = requests.get(f"http://{unit_ip}:8008/health").json() + postmaster_start_time = datetime.strptime( + health_info["postmaster_start_time"], "%Y-%m-%d %H:%M:%S.%f%z" + ).timestamp() + return postmaster_start_time > restart_time and health_info["state"] == "running" def convert_records_to_dict(records: List[tuple]) -> dict: @@ -226,22 +230,6 @@ async def get_password(ops_test: OpsTest, username: str = "operator"): return result.results[f"{username}-password"] -async def get_postgresql_start_time(ops_test: OpsTest, unit_name: str) -> bool: - """Get PostgreSQL start time. - - Args: - ops_test: The ops test framework instance - unit_name: The name of the unit - - Returns: - PostgreSQL start time. - """ - unit_ip = await get_unit_address(ops_test, unit_name) - health_info = requests.get(f"http://{unit_ip}:8008/health") - print(health_info.json()) - return health_info.json()["postmaster_start_time"] - - async def get_primary(ops_test: OpsTest, unit_id=0) -> str: """Get the primary unit. diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index 9d24127ebb..ff236524cf 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. +import time import pytest from pytest_operator.plugin import OpsTest @@ -64,14 +65,8 @@ async def test_password_rotation(ops_test: OpsTest): # Restart Patroni on any non-leader unit and check that # Patroni and PostgreSQL continue to work. - non_leader_units = [ - unit.name - for unit in ops_test.model.applications[APP_NAME].units - if not await unit.is_leader_from_status() - ] - - for unit in non_leader_units: - await restart_patroni(ops_test, unit) - - for unit in non_leader_units: - assert await check_patroni(ops_test, unit) + restart_time = time.time() + for unit in ops_test.model.applications[APP_NAME].units: + if not await unit.is_leader_from_status(): + await restart_patroni(ops_test, unit.name) + assert await check_patroni(ops_test, unit.name, restart_time) From db8dbd8996b59ed25fb5af3fb3aed640c5fe531e Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Fri, 26 Aug 2022 16:15:15 -0300 Subject: [PATCH 24/25] Add pytest mark --- tests/integration/test_password_rotation.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index ff236524cf..a751f87923 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -17,8 +17,9 @@ APP_NAME = METADATA["name"] -@pytest.mark.skip_if_deployed @pytest.mark.abort_on_fail +@pytest.mark.password_rotation +@pytest.mark.skip_if_deployed async def test_deploy_active(ops_test: OpsTest): """Build the charm and deploy it.""" charm = await ops_test.build_charm(".") @@ -35,6 +36,7 @@ async def test_deploy_active(ops_test: OpsTest): await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", timeout=1000) +@pytest.mark.password_rotation async def test_password_rotation(ops_test: OpsTest): """Test password rotation action.""" # Get the initial passwords set for the system users. From 4be4ed0019e0a64b728a30dd39a79cee4db05783 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Thu, 1 Sep 2022 15:04:36 -0300 Subject: [PATCH 25/25] Add password parameter to the action --- actions.yaml | 3 +++ tests/integration/helpers.py | 11 ++++++++--- tests/integration/test_password_rotation.py | 8 ++++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/actions.yaml b/actions.yaml index 6b536158b7..0517e665ca 100644 --- a/actions.yaml +++ b/actions.yaml @@ -19,3 +19,6 @@ set-password: type: string description: The username, the default value 'operator'. Possible values - operator, replication. + password: + type: string + description: The password will be auto-generated if this option is not specified. diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 0b3be446f3..4cbbfee2e1 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -415,9 +415,14 @@ async def scale_application(ops_test: OpsTest, application_name: str, scale: int ) -async def set_password(ops_test: OpsTest, unit_name: str, username: str = "operator"): - """Retrieve a user password using the action.""" +async def set_password( + ops_test: OpsTest, unit_name: str, username: str = "operator", password: str = None +): + """Set a user password using the action.""" unit = ops_test.model.units.get(unit_name) - action = await unit.run_action("set-password", **{"username": username}) + parameters = {"username": username} + if password is not None: + parameters["password"] = password + action = await unit.run_action("set-password", **parameters) result = await action.wait() return result.results diff --git a/tests/integration/test_password_rotation.py b/tests/integration/test_password_rotation.py index a751f87923..0b5f225950 100644 --- a/tests/integration/test_password_rotation.py +++ b/tests/integration/test_password_rotation.py @@ -55,14 +55,18 @@ async def test_password_rotation(ops_test: OpsTest): assert "operator-password" in result.keys() await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", timeout=1000) - result = await set_password(ops_test, unit_name=leader, username="replication") + # For replication, generate a specific password and pass it to the action. + new_replication_password = "test-password" + result = await set_password( + ops_test, unit_name=leader, username="replication", password=new_replication_password + ) assert "replication-password" in result.keys() await ops_test.model.wait_for_idle(apps=[APP_NAME], status="active", timeout=1000) new_superuser_password = await get_password(ops_test) - new_replication_password = await get_password(ops_test, "replication") assert superuser_password != new_superuser_password + assert new_replication_password == await get_password(ops_test, "replication") assert replication_password != new_replication_password # Restart Patroni on any non-leader unit and check that