Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DPE-1454] Update database ownership #287

Merged
merged 4 commits into from
May 17, 2024
Merged

[DPE-1454] Update database ownership #287

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
906bb6a
Bump libs
dragomirp May 7, 2024
e567b39
Reset db ownership
dragomirp May 8, 2024
fae52ab
Check for multiple relations to a single db
dragomirp May 8, 2024
a78d60f
Fix broken auth removal
dragomirp May 10, 2024
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
37 changes: 20 additions & 17 deletions lib/charms/data_platform_libs/v0/data_interfaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent):

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 34
LIBPATCH = 35

PYDEPS = ["ops>=2.0.0"]

Expand Down Expand Up @@ -3016,7 +3016,7 @@ class KafkaRequiresEvents(CharmEvents):
# Kafka Provides and Requires


class KafkaProvidesData(ProviderData):
class KafkaProviderData(ProviderData):
"""Provider-side of the Kafka relation."""

def __init__(self, model: Model, relation_name: str) -> None:
Expand Down Expand Up @@ -3059,12 +3059,12 @@ def set_zookeeper_uris(self, relation_id: int, zookeeper_uris: str) -> None:
self.update_relation_data(relation_id, {"zookeeper-uris": zookeeper_uris})


class KafkaProvidesEventHandlers(EventHandlers):
class KafkaProviderEventHandlers(EventHandlers):
"""Provider-side of the Kafka relation."""

on = KafkaProvidesEvents() # pyright: ignore [reportAssignmentType]

def __init__(self, charm: CharmBase, relation_data: KafkaProvidesData) -> None:
def __init__(self, charm: CharmBase, relation_data: KafkaProviderData) -> None:
super().__init__(charm, relation_data)
# Just to keep lint quiet, can't resolve inheritance. The same happened in super().__init__() above
self.relation_data = relation_data
Expand All @@ -3086,15 +3086,15 @@ def _on_relation_changed_event(self, event: RelationChangedEvent) -> None:
)


class KafkaProvides(KafkaProvidesData, KafkaProvidesEventHandlers):
class KafkaProvides(KafkaProviderData, KafkaProviderEventHandlers):
"""Provider-side of the Kafka relation."""

def __init__(self, charm: CharmBase, relation_name: str) -> None:
KafkaProvidesData.__init__(self, charm.model, relation_name)
KafkaProvidesEventHandlers.__init__(self, charm, self)
KafkaProviderData.__init__(self, charm.model, relation_name)
KafkaProviderEventHandlers.__init__(self, charm, self)


class KafkaRequiresData(RequirerData):
class KafkaRequirerData(RequirerData):
"""Requirer-side of the Kafka relation."""

def __init__(
Expand Down Expand Up @@ -3124,12 +3124,12 @@ def topic(self, value):
self._topic = value


class KafkaRequiresEventHandlers(RequirerEventHandlers):
class KafkaRequirerEventHandlers(RequirerEventHandlers):
"""Requires-side of the Kafka relation."""

on = KafkaRequiresEvents() # pyright: ignore [reportAssignmentType]

def __init__(self, charm: CharmBase, relation_data: KafkaRequiresData) -> None:
def __init__(self, charm: CharmBase, relation_data: KafkaRequirerData) -> None:
super().__init__(charm, relation_data)
# Just to keep lint quiet, can't resolve inheritance. The same happened in super().__init__() above
self.relation_data = relation_data
Expand All @@ -3142,10 +3142,13 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None:
return

# Sets topic, extra user roles, and "consumer-group-prefix" in the relation
relation_data = {
f: getattr(self, f.replace("-", "_"), "")
for f in ["consumer-group-prefix", "extra-user-roles", "topic"]
}
relation_data = {"topic": self.relation_data.topic}

if self.relation_data.extra_user_roles:
relation_data["extra-user-roles"] = self.relation_data.extra_user_roles

if self.relation_data.consumer_group_prefix:
relation_data["consumer-group-prefix"] = self.relation_data.consumer_group_prefix

self.relation_data.update_relation_data(event.relation.id, relation_data)

Expand Down Expand Up @@ -3188,7 +3191,7 @@ def _on_relation_changed_event(self, event: RelationChangedEvent) -> None:
return


class KafkaRequires(KafkaRequiresData, KafkaRequiresEventHandlers):
class KafkaRequires(KafkaRequirerData, KafkaRequirerEventHandlers):
"""Provider-side of the Kafka relation."""

def __init__(
Expand All @@ -3200,7 +3203,7 @@ def __init__(
consumer_group_prefix: Optional[str] = None,
additional_secret_fields: Optional[List[str]] = [],
) -> None:
KafkaRequiresData.__init__(
KafkaRequirerData.__init__(
self,
charm.model,
relation_name,
Expand All @@ -3209,7 +3212,7 @@ def __init__(
consumer_group_prefix,
additional_secret_fields,
)
KafkaRequiresEventHandlers.__init__(self, charm, self)
KafkaRequirerEventHandlers.__init__(self, charm, self)


# Opensearch related events
Expand Down
14 changes: 9 additions & 5 deletions lib/charms/postgresql_k8s/v0/postgresql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 25
LIBPATCH = 26

INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles"

Expand Down Expand Up @@ -476,11 +476,11 @@ def set_up_database(self) -> None:
"""Set up postgres database with the right permissions."""
connection = None
try:
self.create_user(
"admin",
extra_user_roles="pg_read_all_data,pg_write_all_data",
)
with self._connect_to_database() as connection, connection.cursor() as cursor:
cursor.execute("SELECT TRUE FROM pg_roles WHERE rolname='admin';")
if cursor.fetchone() is not None:
return

# Allow access to the postgres database only to the system users.
cursor.execute("REVOKE ALL PRIVILEGES ON DATABASE postgres FROM PUBLIC;")
cursor.execute("REVOKE CREATE ON SCHEMA public FROM PUBLIC;")
Expand All @@ -490,6 +490,10 @@ def set_up_database(self) -> None:
sql.Identifier(user)
)
)
self.create_user(
"admin",
extra_user_roles="pg_read_all_data,pg_write_all_data",
)
cursor.execute("GRANT CONNECT ON DATABASE postgres TO admin;")
except psycopg2.Error as e:
logger.error(f"Failed to set up databases: {e}")
Expand Down
6 changes: 3 additions & 3 deletions lib/charms/prometheus_k8s/v0/prometheus_scrape.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def __init__(self, *args):
- `scrape_timeout`
- `proxy_url`
- `relabel_configs`
- `metrics_relabel_configs`
- `metric_relabel_configs`
- `sample_limit`
- `label_limit`
- `label_name_length_limit`
Expand Down Expand Up @@ -362,7 +362,7 @@ def _on_scrape_targets_changed(self, event):

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 46
LIBPATCH = 47

PYDEPS = ["cosl"]

Expand All @@ -377,7 +377,7 @@ def _on_scrape_targets_changed(self, event):
"scrape_timeout",
"proxy_url",
"relabel_configs",
"metrics_relabel_configs",
"metric_relabel_configs",
"sample_limit",
"label_limit",
"label_name_length_limit",
Expand Down
13 changes: 11 additions & 2 deletions src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import os
import socket
from configparser import ConfigParser
from typing import Any, Dict, Literal, Optional, Tuple, Union, get_args
from typing import Any, Dict, List, Literal, Optional, Tuple, Union, get_args

import lightkube
from charms.data_platform_libs.v0.data_interfaces import DataPeerData, DataPeerUnitData
Expand All @@ -23,7 +23,7 @@
from ops.charm import CharmBase, ConfigChangedEvent, PebbleReadyEvent
from ops.framework import StoredState
from ops.main import main
from ops.model import ActiveStatus, BlockedStatus, WaitingStatus
from ops.model import ActiveStatus, BlockedStatus, Relation, WaitingStatus
from ops.pebble import ConnectionError, Layer, ServiceStatus

from constants import (
Expand Down Expand Up @@ -893,6 +893,15 @@ def _has_blocked_status(self) -> bool:
"""Returns whether the unit is in a blocked state."""
return isinstance(self.unit.status, BlockedStatus)

@property
def client_relations(self) -> List[Relation]:
"""Return the list of established client relations."""
relations = []
for relation_name in ["database", "db", "db-admin"]:
for relation in self.model.relations.get(relation_name, []):
relations.append(relation)
return relations


if __name__ == "__main__":
main(PgBouncerK8sCharm)
13 changes: 9 additions & 4 deletions src/relations/db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,9 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent):
logger.info(init_msg)

self.charm.backend.postgres.create_user(user, password, admin=self.admin)
self.charm.backend.postgres.create_database(database, user)

self.charm.backend.postgres.create_database(
database, user, client_relations=self.charm.client_relations
)
Comment on lines +260 to +262
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from the PG charm. This can potentially cause issues if both PG and PGB are related on the same database, as both will think they own it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it may happen only when we change the PgBouncer extra user roles from SUPERUSER to fewer privileges.

created_msg = f"database and user for {self.relation_name} relation created"
self.charm.unit.status = initial_status
self.charm.update_status()
Expand All @@ -270,6 +271,7 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent):
return

# set up auth function
self.charm.backend.remove_auth_function(dbs=[database])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the auth_function was not removed, create_database() will change ownership and initialise_auth_function() will not recreate it.

self.charm.backend.initialise_auth_function([database])

def _on_relation_changed(self, change_event: RelationChangedEvent):
Expand Down Expand Up @@ -428,11 +430,14 @@ def _on_relation_broken(self, broken_event: RelationBrokenEvent):
self._check_for_blocking_relations(broken_event.relation.id)
return

dbs = self.charm.generate_relation_databases()
dbs.pop(str(broken_event.relation.id), None)
dbs = self.charm.get_relation_databases()
database = dbs.pop(str(broken_event.relation.id), {}).get("name")
Comment on lines +433 to +434
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the db name to remove the auth function.

self.charm.set_relation_databases(dbs)
if self.charm.unit.is_leader():
self.charm.backend.postgres.delete_user(user)
delete_db = database not in dbs.values()
if database and delete_db:
self.charm.backend.remove_auth_function(dbs=[database])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only delete the auth query if no other relations use the same db.


self._check_for_blocking_relations(broken_event.relation.id)

Expand Down
12 changes: 9 additions & 3 deletions src/relations/pgbouncer_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,11 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None:
user, password, extra_user_roles=extra_user_roles
)
logger.debug("creating database")
self.charm.backend.postgres.create_database(database, user)
self.charm.backend.postgres.create_database(
database, user, client_relations=self.charm.client_relations
)
# set up auth function
self.charm.backend.remove_auth_function(dbs=[database])
self.charm.backend.initialise_auth_function(dbs=[database])
except (
PostgreSQLCreateDatabaseError,
Expand Down Expand Up @@ -192,10 +195,13 @@ def _on_relation_broken(self, event: RelationBrokenEvent) -> None:
self.charm.peers.unit_databag.pop(self._depart_flag(event.relation), None)
return

dbs = self.charm.generate_relation_databases()
dbs.pop(str(event.relation.id), None)
dbs = self.charm.get_relation_databases()
database = dbs.pop(str(event.relation.id), {}).get("name")
self.charm.set_relation_databases(dbs)

delete_db = database not in dbs.values()
if database and delete_db:
self.charm.backend.remove_auth_function(dbs=[database])
Copy link
Member

Choose a reason for hiding this comment

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

I think the auth_function is not being correctly created/removed correctly.

By running the following commands, I lost the connection through app1 credentials (data-integrator) when I removed only the relation between app2 (another data-integrator) and pgbouncer-k8s.

juju deploy postgresql-k8s --channel 14/edge --trust
juju deploy ./pgbouncer-k8s_ubuntu-22.04-amd64.charm --resource pgbouncer-image=ghcr.io/canonical/charmed-postgresql@sha256:31cf150b4523481202c1ff9b7b5d7f0b36729edad89d61242d8f1eb56b2912c0 --trust
juju deploy data-integrator app1
juju deploy data-integrator app2

juju relate pgbouncer-k8s postgresql-k8s
juju config app1 database-name=test
juju config app2 database-name=test
juju relate app1 pgbouncer-k8s
juju relate app2 pgbouncer-k8s

juju run postgresql-k8s/leader get-password
psql "host={postgresql-unit-ip} user=operator dbname=test password={password}”
# Run SELECT specific_schema FROM information_schema.routines WHERE routine_name='get_auth'; which outputs only pgbouncer_auth_relation_id_7.

juju run app1/leader get-credentials
psql "host={ip-from-get-credentials} user={user-from-get-credentials} dbname={database-name-from-get-credentials} password={password-from-get-credentials}”
# The connection works.

juju remove-relation app2 pgbouncer-k8s
psql "host={postgresql-unit-ip} user=operator dbname=test password={password}”
# Run SELECT specific_schema FROM information_schema.routines WHERE routine_name='get_auth'; which outputs nothing.

juju run app1/leader get-credentials # To check that the same credentials from before are shown.
psql "host={ip-from-get-credentials} user={user-from-get-credentials} dbname={database-name-from-get-credentials} password={password-from-get-credentials}”
# The connection doesn’t work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dbs.values() are dicts so delete_db was always true. Nice catch.

# Delete the user.
try:
user = f"relation_id_{event.relation.id}"
Expand Down
12 changes: 10 additions & 2 deletions tests/unit/relations/test_db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# See LICENSE file for licensing details.

import unittest
from unittest.mock import Mock, PropertyMock, patch
from unittest.mock import Mock, PropertyMock, patch, sentinel

from charms.pgbouncer_k8s.v0.pgb import parse_dict_to_kv_string
from ops.model import Unit
Expand Down Expand Up @@ -60,6 +60,11 @@ def test_correct_admin_perms_set_in_constructor(self):
assert self.charm.legacy_db_admin_relation.relation_name == "db-admin"
assert self.charm.legacy_db_admin_relation.admin is True

@patch(
"charm.PgBouncerK8sCharm.client_relations",
new_callable=PropertyMock,
return_value=sentinel.client_rels,
)
@patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True)
@patch(
"relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock
Expand All @@ -68,6 +73,7 @@ def test_correct_admin_perms_set_in_constructor(self):
@patch("charms.postgresql_k8s.v0.postgresql.PostgreSQL")
@patch("charms.postgresql_k8s.v0.postgresql.PostgreSQL.create_user")
@patch("charms.postgresql_k8s.v0.postgresql.PostgreSQL.create_database")
@patch("relations.backend_database.BackendDatabaseRequires.remove_auth_function")
@patch("relations.backend_database.BackendDatabaseRequires.initialise_auth_function")
@patch("charm.PgBouncerK8sCharm.set_relation_databases")
@patch("charm.PgBouncerK8sCharm.generate_relation_databases")
Expand All @@ -76,12 +82,14 @@ def test_on_relation_joined(
_gen_rel_dbs,
_set_rel_dbs,
_init_auth,
_remove_auth,
_create_database,
_create_user,
_postgres,
_gen_pw,
_backend_pg,
_check_backend,
_,
):
self.harness.set_leader(True)

Expand Down Expand Up @@ -109,7 +117,7 @@ def test_on_relation_joined(
_set_rel_dbs.assert_called_once_with({"1": {"name": "test_db", "legacy": True}})

_create_user.assert_called_with(user, password, admin=True)
_create_database.assert_called_with(database, user)
_create_database.assert_called_with(database, user, client_relations=sentinel.client_rels)
_init_auth.assert_called_with([database])

for dbag in [relation_data[self.charm.unit], relation_data[self.charm.app]]:
Expand Down
12 changes: 10 additions & 2 deletions tests/unit/relations/test_pgbouncer_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# See LICENSE file for licensing details.

import unittest
from unittest.mock import MagicMock, PropertyMock, patch
from unittest.mock import MagicMock, PropertyMock, patch, sentinel

from ops.testing import Harness

Expand Down Expand Up @@ -35,6 +35,11 @@ def setUp(self):
self.harness.add_relation_unit(self.client_rel_id, "application/0")

@patch("charm.lightkube")
@patch(
"charm.PgBouncerK8sCharm.client_relations",
new_callable=PropertyMock,
return_value=sentinel.client_rels,
)
@patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True)
@patch(
"relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock
Expand Down Expand Up @@ -80,6 +85,7 @@ def test_on_database_requested(
_pg,
_check_backend,
_,
__,
):
self.harness.set_leader()
_gen_rel_dbs.return_value = {}
Expand All @@ -102,7 +108,9 @@ def test_on_database_requested(
_pg().create_user.assert_called_with(
user, _password(), extra_user_roles=event.extra_user_roles
)
_pg().create_database.assert_called_with(database, user)
_pg().create_database.assert_called_with(
database, user, client_relations=sentinel.client_rels
)
_dbp_set_credentials.assert_called_with(rel_id, user, _password())
_dbp_set_version.assert_called_with(rel_id, _pg().get_postgresql_version())
_dbp_set_endpoints.assert_called_with(
Expand Down
Loading