Skip to content
Merged
Show file tree
Hide file tree
Changes from 62 commits
Commits
Show all changes
69 commits
Select commit Hold shift + click to select a range
e1a14e0
Implement instance level predefined roles
shayancanonical May 6, 2025
23c1ea0
Merge branch '16/edge' into feature/16_predefined_instance_roles
shayancanonical May 6, 2025
a5c6442
Fix minor bug introduced while rebasing off of 16/edge
shayancanonical May 6, 2025
09c4a71
Add integration test for charmed_read and charmed_dml roles
shayancanonical May 6, 2025
d778bba
Revert all major changes except introduction of predefined roles
shayancanonical May 7, 2025
873132e
Sweep diff and minor bug fixes
shayancanonical May 7, 2025
0dc8d3d
Avoid creating set_user extension
shayancanonical May 7, 2025
1764971
Merge branch '16/edge' into feature/16_predefined_instance_roles
shayancanonical May 7, 2025
fead5cd
Port Carl's fix for broken unit tests
shayancanonical May 7, 2025
2c0f822
Merge branch '16/edge' into feature/16_predefined_instance_roles
shayancanonical May 7, 2025
c848782
Merge branch '16/edge' into feature/16_predefined_instance_roles
shayancanonical May 28, 2025
e49e999
Create set_up_predefined_catalog_roles_function
marceloneppel May 29, 2025
5724320
Fix linting and run function on database creation
marceloneppel May 29, 2025
8da9ab5
Add login hook function
marceloneppel May 29, 2025
e4bc8d2
Escalate relation users
marceloneppel May 29, 2025
8094e4f
Merge branch '16/edge' into feature/16_predefined_instance_roles
shayancanonical May 30, 2025
2f204a9
Add integration test
marceloneppel May 30, 2025
0dd22ed
Fix unit test
marceloneppel May 30, 2025
d453455
Merge remote-tracking branch 'origin/feature/16_predefined_instance_r…
marceloneppel May 30, 2025
2fc6e5c
Check for no write permissions for relation user
marceloneppel May 30, 2025
fea0148
Don't set up catalog roles if they already exist
marceloneppel May 30, 2025
1c5a911
Test database creation permission
marceloneppel May 30, 2025
6a083c5
Improve logs and move cleanup process to the beginning of the test
marceloneppel Jun 2, 2025
16d6783
Wait for relation to be removed and retrieve primary
marceloneppel Jun 2, 2025
fe5ce8f
Handle re-relation
marceloneppel Jun 2, 2025
43493d4
Add test for removing and re-adding relation
marceloneppel Jun 2, 2025
1005f7e
Test roles after database re-creation
marceloneppel Jun 2, 2025
c526616
Test table creation failure for charmed_databases_owner user
marceloneppel Jun 2, 2025
5de77d0
Deduplicate relations retrieval code
marceloneppel Jun 2, 2025
ff5dd37
Check that the relation user can escalate to the database owner user …
marceloneppel Jun 2, 2025
1d170ff
Check escalation back to charmed_databases_owner
marceloneppel Jun 3, 2025
44beefa
Test permissions on newly created database
marceloneppel Jun 3, 2025
883c4ba
Check database owner user permissions in the newly created database
marceloneppel Jun 3, 2025
4eb981b
Reduce duplicated code with check_connected_user helper function
marceloneppel Jun 3, 2025
8b85257
Reduce more duplicated code with check_connected_user helper function
marceloneppel Jun 3, 2025
e669da3
Bump library
marceloneppel Jun 3, 2025
17e7e4a
Fix test_charmed_read_role
marceloneppel Jun 3, 2025
f1d3293
Remove admin and postgres roles
marceloneppel Jun 3, 2025
3aba882
Create DBA role
marceloneppel Jun 4, 2025
e16567d
Bump postgresql charm lib for 16/edge to v1 due to backwards incompat…
shayancanonical Jun 4, 2025
2c15212
Remove admin role test
marceloneppel Jun 4, 2025
f711479
Add DBA user test
marceloneppel Jun 4, 2025
1fed145
Test DBA role in replica
marceloneppel Jun 4, 2025
c8efa82
Grant reset_user function to DBA role
marceloneppel Jun 4, 2025
6cd2a85
Test set_user function for unprivileged users
marceloneppel Jun 4, 2025
e42e220
Reduce duplicate code in check_connected_user helper function
marceloneppel Jun 4, 2025
fbebe06
Merge remote-tracking branch 'origin/feature/16_predefined_instance_r…
marceloneppel Jun 4, 2025
3ae3885
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
marceloneppel Jun 4, 2025
fb566b5
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
marceloneppel Jun 4, 2025
5f2328e
Fix charmed_databases_owner permissions
marceloneppel Jun 4, 2025
3deb576
Fix test_charmed_dba_role
marceloneppel Jun 4, 2025
c78e2ff
Merge branch '16/edge' into feature/16_predefined_instance_roles
shayancanonical Jun 5, 2025
526bf2d
Re-add mistakenly removed patch statements
shayancanonical Jun 5, 2025
fd41021
Merge remote-tracking branch 'origin/feature/16_predefined_instance_r…
marceloneppel Jun 5, 2025
fa1efa7
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
marceloneppel Jun 5, 2025
4084579
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
marceloneppel Jun 5, 2025
f3eceba
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
marceloneppel Jun 5, 2025
9721e91
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
marceloneppel Jun 5, 2025
7f98938
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
marceloneppel Jun 5, 2025
40c2051
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
marceloneppel Jun 6, 2025
d121c1f
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
marceloneppel Jun 6, 2025
71fb74f
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
marceloneppel Jun 6, 2025
dc5b01e
Reset connection to None before creating a new connection
marceloneppel Jun 6, 2025
262dc2b
Merge remote-tracking branch 'origin/feature/16_predefined_dba_role' …
marceloneppel Jun 6, 2025
0848dcf
Remove irrelevant test and increase timeout
marceloneppel Jun 6, 2025
32f4f56
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
marceloneppel Jun 6, 2025
24605d9
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
marceloneppel Jun 7, 2025
4f2f0e2
Merge remote-tracking branch 'origin/feature/16_predefined_roles_clea…
marceloneppel Jun 7, 2025
32663cb
Merge remote-tracking branch 'origin/16/edge' into feature/16_predefi…
marceloneppel Jun 8, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
319 changes: 199 additions & 120 deletions lib/charms/postgresql_k8s/v1/postgresql.py

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -1712,7 +1712,7 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901
return

try:
self.postgresql.create_predefined_roles()
self.postgresql.create_predefined_instance_roles()
Copy link
Member Author

Choose a reason for hiding this comment

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

Just renaming to be clear on which types of predefined roles we are creating at this moment (i.e. the ones introduced by #881).

except PostgreSQLCreatePredefinedRolesError as e:
logger.exception(e)
self.unit.status = BlockedStatus("Failed to create pre-defined roles")
Expand All @@ -1724,8 +1724,6 @@ def _start_primary(self, event: StartEvent) -> None: # noqa: C901
# This event can be run on a replica if the machines are restarted.
# For that case, check whether the postgres user already exits.
users = self.postgresql.list_users()
if "postgres" not in users:
self.postgresql.create_user("postgres", new_password(), admin=True)
# Create the backup user.
if BACKUP_USER not in users:
self.postgresql.create_user(
Expand Down
9 changes: 5 additions & 4 deletions src/relations/postgresql_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,13 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None:
try:
# Creates the user and the database for this specific relation.
user = f"relation-{event.relation.id}"
password = new_password()
self.charm.postgresql.create_user(user, password, extra_user_roles=extra_user_roles)
plugins = self.charm.get_plugins()

self.charm.postgresql.create_database(
database, user, plugins=plugins, client_relations=self.charm.client_relations
self.charm.postgresql.create_database(database, plugins=plugins)

password = new_password()
self.charm.postgresql.create_user(
user, password, extra_user_roles=extra_user_roles, in_role=f"{database}_admin"
)
Comment on lines +115 to 120
Copy link
Member Author

Choose a reason for hiding this comment

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

The order of the calls was changed here because the catalog/database level roles are created along with the database, and we need to pass the {database-name}_admin to the user creation method.


# Share the credentials with the application.
Expand Down
12 changes: 10 additions & 2 deletions templates/patroni.yml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,11 @@ bootstrap:
log_truncate_on_rotation: 'on'
logging_collector: 'on'
wal_level: logical
shared_preload_libraries: 'timescaledb,pgaudit'
shared_preload_libraries: 'timescaledb,pgaudit,set_user'
session_preload_libraries: 'login_hook'
set_user.block_log_statement: 'on'
set_user.exit_on_error: 'on'
set_user.superuser_allowlist: '+charmed_dba'

{%- if restoring_backup %}
method: pgbackrest
Expand Down Expand Up @@ -132,7 +136,11 @@ postgresql:
bin_dir: /snap/charmed-postgresql/current/usr/lib/postgresql/{{ version }}/bin
data_dir: {{ data_path }}
parameters:
shared_preload_libraries: 'timescaledb,pgaudit'
shared_preload_libraries: 'timescaledb,pgaudit,set_user'
session_preload_libraries: 'login_hook'
set_user.block_log_statement: 'on'
set_user.exit_on_error: 'on'
set_user.superuser_allowlist: '+charmed_dba'
{%- if enable_pgbackrest_archiving %}
archive_command: 'pgbackrest {{ pgbackrest_configuration_file }} --stanza={{ stanza }} archive-push %p'
{% else %}
Expand Down
84 changes: 84 additions & 0 deletions tests/integration/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import botocore
import psycopg2
import pytest
import requests
import yaml
from juju.model import Model
Expand All @@ -37,6 +38,7 @@
DATABASE_APP_NAME = METADATA["name"]
STORAGE_PATH = METADATA["storage"]["data"]["location"]
APPLICATION_NAME = "postgresql-test-app"
DATA_INTEGRATOR_APP_NAME = "data-integrator"


class SecretNotFoundError(Exception):
Expand Down Expand Up @@ -737,6 +739,80 @@ def get_unit_address(ops_test: OpsTest, unit_name: str, model: Model = None) ->
return model.units.get(unit_name).public_address


def check_connected_user(
cursor, session_user: str, current_user: str, primary: bool = True
) -> None:
cursor.execute("SELECT session_user,current_user;")
result = cursor.fetchone()
if result is not None:
instance = "primary" if primary else "replica"
assert result[0] == session_user, (
f"The session user should be the {session_user} user in the {instance}"
)
assert result[1] == current_user, (
f"The current user should be the {current_user} user in the {instance}"
)
else:
assert False, "No result returned from the query"


async def check_roles_and_their_permissions(
ops_test: OpsTest, relation_endpoint: str, database_name: str
) -> None:
action = await ops_test.model.units[f"{DATA_INTEGRATOR_APP_NAME}/0"].run_action(
action_name="get-credentials"
)
result = await action.wait()
data_integrator_credentials = result.results
username = data_integrator_credentials[relation_endpoint]["username"]
uris = data_integrator_credentials[relation_endpoint]["uris"]
connection = None
try:
connection = psycopg2.connect(uris)
connection.autocommit = True
with connection.cursor() as cursor:
logger.info(
"Checking that the relation user is automatically escalated to the database owner user"
)
check_connected_user(cursor, username, f"{database_name}_owner")
logger.info("Creating a test table and inserting data")
cursor.execute("CREATE TABLE test_table (id INTEGER);")
logger.info("Inserting data into the test table")
cursor.execute("INSERT INTO test_table(id) VALUES(1);")
logger.info("Reading data from the test table")
cursor.execute("SELECT * FROM test_table;")
result = cursor.fetchall()
assert len(result) == 1, "The database owner user should be able to read the data"

logger.info("Checking that the database owner user can't create a database")
with pytest.raises(psycopg2.errors.InsufficientPrivilege):
cursor.execute(f"CREATE DATABASE {database_name}_2;")

logger.info("Checking that the relation user can't create a table")
cursor.execute("RESET ROLE;")
check_connected_user(cursor, username, username)
with pytest.raises(psycopg2.errors.InsufficientPrivilege):
cursor.execute("CREATE TABLE test_table_2 (id INTEGER);")
finally:
if connection is not None:
connection.close()

connection_string = f"host={data_integrator_credentials[relation_endpoint]['read-only-endpoints'].split(':')[0]} dbname={data_integrator_credentials[relation_endpoint]['database']} user={username} password={data_integrator_credentials[relation_endpoint]['password']}"
connection = None
try:
connection = psycopg2.connect(connection_string)
with connection.cursor() as cursor:
logger.info("Checking that the relation user can read data from the database")
check_connected_user(cursor, username, username, primary=False)
logger.info("Reading data from the test table")
cursor.execute("SELECT * FROM test_table;")
result = cursor.fetchall()
assert len(result) == 1, "The relation user should be able to read the data"
finally:
if connection is not None:
connection.close()


async def check_tls(ops_test: OpsTest, unit_name: str, enabled: bool) -> bool:
"""Returns whether TLS is enabled on the specific PostgreSQL instance.

Expand Down Expand Up @@ -900,6 +976,14 @@ async def primary_changed(ops_test: OpsTest, old_primary: str) -> bool:
return primary != old_primary


def relations(ops_test: OpsTest, provider_app: str, requirer_app: str) -> list:
return [
relation
for relation in ops_test.model.applications[provider_app].relations
if not relation.is_peer and relation.requires.application_name == requirer_app
]
Comment on lines +979 to +984
Copy link
Member Author

Choose a reason for hiding this comment

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

Way to check if the relation still exists (and avoid situations where we removed it and it's still alive when trying to re-relate).



async def restart_machine(ops_test: OpsTest, unit_name: str) -> None:
"""Restart the machine where a unit run on.

Expand Down
91 changes: 1 addition & 90 deletions tests/integration/new_relations/test_new_relations_1.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
# See LICENSE file for licensing details.
import asyncio
import logging
import secrets
import string
from pathlib import Path

import psycopg2
Expand Down Expand Up @@ -484,97 +482,10 @@ async def test_relation_with_no_database_name(ops_test: OpsTest):
await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active", raise_on_blocked=True)


async def test_admin_role(ops_test: OpsTest):
"""Test that the admin role gives access to all the databases."""
all_app_names = [DATA_INTEGRATOR_APP_NAME]
all_app_names.extend(APP_NAMES)
async def test_invalid_extra_user_roles(ops_test: OpsTest):
async with ops_test.fast_forward():
await ops_test.model.deploy(DATA_INTEGRATOR_APP_NAME, base=CHARM_BASE)
await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked")
await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({
"database-name": DATA_INTEGRATOR_APP_NAME.replace("-", "_"),
"extra-user-roles": "admin",
})
await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked")
await ops_test.model.add_relation(DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME)
await ops_test.model.wait_for_idle(apps=all_app_names, status="active")

# Check that the user can access all the databases.
for database in [
DATABASE_DEFAULT_NAME,
f"{APPLICATION_APP_NAME.replace('-', '_')}_database",
"another_application_database",
]:
logger.info(f"connecting to the following database: {database}")
connection_string = await build_connection_string(
ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", database=database
)
connection = None
should_fail = False
try:
with psycopg2.connect(connection_string) as connection, connection.cursor() as cursor:
# Check the version that the application received is the same on the
# database server.
cursor.execute("SELECT version();")
data = cursor.fetchone()[0].split(" ")[1]

# Get the version of the database and compare with the information that
# was retrieved directly from the database.
version = await get_application_relation_data(
ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", "version"
)
assert version == data

# Write some data (it should fail in the default database name).
random_name = (
f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}"
)
should_fail = database == DATABASE_DEFAULT_NAME
cursor.execute(f"CREATE SCHEMA test; CREATE TABLE test.{random_name}(data TEXT);")
if should_fail:
assert False, (
f"failed to run a statement in the following database: {database}"
)
except psycopg2.errors.InsufficientPrivilege as e:
if not should_fail:
logger.exception(e)
assert False, (
f"failed to connect to or run a statement in the following database: {database}"
)
finally:
if connection is not None:
connection.close()

# Test the creation and deletion of databases.
connection_string = await build_connection_string(
ops_test, DATA_INTEGRATOR_APP_NAME, "postgresql", database=DATABASE_DEFAULT_NAME
)
connection = psycopg2.connect(connection_string)
connection.autocommit = True
cursor = connection.cursor()
random_name = f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}"
cursor.execute(f"CREATE DATABASE {random_name};")
cursor.execute(f"DROP DATABASE {random_name};")
try:
cursor.execute(f"DROP DATABASE {DATABASE_DEFAULT_NAME};")
assert False, (
f"the admin extra user role was able to drop the `{DATABASE_DEFAULT_NAME}` system database"
)
except psycopg2.errors.InsufficientPrivilege:
# Ignore the error, as the admin extra user role mustn't be able to drop
# the "postgres" system database.
pass
finally:
connection.close()


async def test_invalid_extra_user_roles(ops_test: OpsTest):
async with ops_test.fast_forward():
# Remove the relation between the database and the first data integrator.
await ops_test.model.applications[DATABASE_APP_NAME].remove_relation(
DATABASE_APP_NAME, DATA_INTEGRATOR_APP_NAME
)
await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active", raise_on_blocked=True)

another_data_integrator_app_name = f"another-{DATA_INTEGRATOR_APP_NAME}"
data_integrator_apps_names = [DATA_INTEGRATOR_APP_NAME, another_data_integrator_app_name]
Expand Down
34 changes: 0 additions & 34 deletions tests/integration/new_relations/test_relations_coherence.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,40 +82,6 @@ async def test_relations(ops_test: OpsTest, charm):
f"{DATABASE_APP_NAME}:database", f"{DATA_INTEGRATOR_APP_NAME}:postgresql"
)

# Re-relation with admin role with checking permission
await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({
"database-name": DATA_INTEGRATOR_APP_NAME.replace("-", "_"),
"extra-user-roles": "admin",
})
await ops_test.model.wait_for_idle(apps=[DATA_INTEGRATOR_APP_NAME], status="blocked")
await ops_test.model.add_relation(DATA_INTEGRATOR_APP_NAME, DATABASE_APP_NAME)
await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active")

connection_string = await build_connection_string(
ops_test,
DATA_INTEGRATOR_APP_NAME,
"postgresql",
database=DATA_INTEGRATOR_APP_NAME.replace("-", "_"),
)
try:
connection = psycopg2.connect(connection_string)
connection.autocommit = True
cursor = connection.cursor()
random_name = (
f"test_{''.join(secrets.choice(string.ascii_lowercase) for _ in range(10))}"
)
cursor.execute(f"CREATE DATABASE {random_name};")
except psycopg2.errors.InsufficientPrivilege:
assert False, (
f"failed connect to {random_name} or run a statement in the following database"
)
finally:
connection.close()

await ops_test.model.applications[DATABASE_APP_NAME].remove_relation(
f"{DATABASE_APP_NAME}:database", f"{DATA_INTEGRATOR_APP_NAME}:postgresql"
)

# Re-relation again with user role and checking write data
await ops_test.model.applications[DATA_INTEGRATOR_APP_NAME].set_config({
"database-name": DATA_INTEGRATOR_APP_NAME.replace("-", "_"),
Expand Down
Loading
Loading