Skip to content

Commit

Permalink
[DPE-1454] Update database ownership (#287)
Browse files Browse the repository at this point in the history
* Bump libs

* Reset db ownership

* Check for multiple relations to a single db

* Fix broken auth removal
  • Loading branch information
dragomirp authored May 17, 2024
1 parent 1ed7eb5 commit 6247dc6
Show file tree
Hide file tree
Showing 8 changed files with 81 additions and 38 deletions.
37 changes: 20 additions & 17 deletions lib/charms/data_platform_libs/v0/data_interfaces.py
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
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
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
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
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
)
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])
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")
self.charm.set_relation_databases(dbs)
if self.charm.unit.is_leader():
self.charm.backend.postgres.delete_user(user)
delete_db = database not in [db["name"] for db in dbs.values()]
if database and delete_db:
self.charm.backend.remove_auth_function(dbs=[database])

self._check_for_blocking_relations(broken_event.relation.id)

Expand Down
12 changes: 9 additions & 3 deletions src/relations/pgbouncer_provider.py
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 [db["name"] for db in dbs.values()]
if database and delete_db:
self.charm.backend.remove_auth_function(dbs=[database])
# 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
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
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

0 comments on commit 6247dc6

Please sign in to comment.