diff --git a/lib/charms/pgbouncer_k8s/v0/pgb.py b/lib/charms/pgbouncer_k8s/v0/pgb.py index 5ee50c782..136d79965 100644 --- a/lib/charms/pgbouncer_k8s/v0/pgb.py +++ b/lib/charms/pgbouncer_k8s/v0/pgb.py @@ -20,17 +20,11 @@ """ -import io import logging -import math -import re import secrets import string -from collections.abc import MutableMapping -from configparser import ConfigParser, ParsingError -from copy import deepcopy from hashlib import md5 -from typing import Dict, Union +from typing import Dict # The unique Charmhub library identifier, never change it LIBID = "113f4a7480c04631bfdf5fe776f760cd" @@ -38,7 +32,7 @@ LIBAPI = 0 # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 8 +LIBPATCH = 9 logger = logging.getLogger(__name__) @@ -46,381 +40,6 @@ PGB_DIR = "/var/lib/postgresql/pgbouncer" INI_PATH = f"{PGB_DIR}/pgbouncer.ini" -DEFAULT_CONFIG = { - "databases": {}, - "pgbouncer": { - "listen_addr": "*", - "listen_port": 6432, - "logfile": f"{PGB_DIR}/pgbouncer.log", - "pidfile": f"{PGB_DIR}/pgbouncer.pid", - "admin_users": set(), - "stats_users": set(), - "auth_type": "md5", - "user": "postgres", - "max_client_conn": "10000", - "ignore_startup_parameters": "extra_float_digits,options", - "server_tls_sslmode": "prefer", - "so_reuseport": "1", - "unix_socket_dir": PGB_DIR, - }, -} - - -class PgbConfig(MutableMapping): - """A mapping that represents the pgbouncer config. - - The PgbConfig class provides an abstraction for the pgbouncer.ini config file. This file - essentially governs how each instance of pgbouncer operates, including the connection - information for each database, user authentication, local file management, and how connections - are pooled in detail. This config also contains a `validate` function that allows a user to - verify their changes will result in a valid pgbouncer config, and a `render` function to render - this object out to a string. - - This config is implemented as a MutableMapping with a few `dict`-style methods for ease of use. - Config can be passed to the constructor as a string, a dict (such as the default variable - DEFAULT_CONFIG), or another PgbConfig object. - - For more pgbouncer config documentation, visit: https://www.pgbouncer.org/config.html - - The following is an example of a pgbouncer.ini config file output by PgbConfig.render(), taken - from a test deployment: - [databases] - discourse-k8s = host=postgresql-k8s-primary.test-db-admin-ipve.svc.cluster.local dbname=discourse-k8s port=5432 - discourse-k8s_standby = host=postgresql-k8s-replicas.test-db-admin-ipve.svc.cluster.local dbname=discourse-k8s port=5432 - discourse-charmers-discourse-k8s = host=postgresql-k8s-primary.test-db-admin-ipve.svc.cluster.local dbname=discourse-charmers-discourse-k8s port=5432 - discourse-charmers-discourse-k8s_standby = host=postgresql-k8s-replicas.test-db-admin-ipve.svc.cluster.local dbname=discourse-charmers-discourse-k8s port=5432 - - [pgbouncer] - listen_addr = * - listen_port = 6432 - logfile = /var/lib/postgresql/pgbouncer/pgbouncer.log - pidfile = /var/lib/postgresql/pgbouncer/pgbouncer.pid - admin_users = relation_1,pgbouncer_k8s_user_2_test_db_admin_ipve,pgbouncer_k8s_user_4_test_db_admin_ipve - stats_users = - auth_type = md5 - user = postgres - max_client_conn = 10000 - ignore_startup_parameters = extra_float_digits,options - server_tls_sslmode = prefer - so_reuseport = 1 - unix_socket_dir = /var/lib/postgresql/pgbouncer - pool_mode = session - max_db_connections = 100 - default_pool_size = 13 - min_pool_size = 7 - reserve_pool_size = 7 - auth_user = pgbouncer_auth_relation_1 - auth_query = SELECT username, password FROM pgbouncer_auth_relation_1.get_auth($1) - - """ # noqa: W505 - - # Define names of ini sections: - # [databases] defines the config options for each database. This section is mandatory. - # [pgbouncer] defines pgbouncer-specific config - # [users] defines config for specific users. - db_section = "databases" - pgb_section = "pgbouncer" - users_section = "users" - user_types = ["admin_users", "stats_users"] - - def __init__( - self, - config: Union[str, dict, "PgbConfig"] = None, - *args, - **kwargs, - ): - """Constructor. - - Args: - config: an existing config. Can be passed in as a string, dict, or PgbConfig object. - pgb.DEFAULT_CONFIG can be used here as a default dict. - args: arguments. - kwargs: keyword arguments. - """ - self.__dict__.update(*args, **kwargs) - - if isinstance(config, str): - self.read_string(config) - elif isinstance(config, dict): - self.read_dict(config) - elif isinstance(config, PgbConfig): - self.read_dict(dict(config)) - - def __delitem__(self, key: str): - """Deletes item from internal mapping.""" - del self.__dict__[key] - - def __getitem__(self, key: str): - """Gets item from internal mapping.""" - return self.__dict__[key] - - def __setitem__(self, key: str, value): - """Set an item in internal mapping.""" - self.__dict__[key] = value - - def __iter__(self): - """Returns an iterable of internal mapping.""" - return iter(self.__dict__) - - def __len__(self): - """Gets number of key-value pairs in internal mapping.""" - return len(self.__dict__) - - def __str__(self): - """String representation of PgbConfig object.""" - return str(self.__dict__) - - def __eq__(self, other_config): - """Checks if self and other_config are equal.""" - return self.__dict__ == other_config.__dict__ - - def keys(self): - """Returns keys of PgbConfig object.""" - return self.__dict__.keys() - - def items(self): - """Returns items of PgbConfig object.""" - return self.__dict__.items() - - def read_dict(self, data: Dict) -> None: - """Populates this object from a dictionary. - - Args: - data: Dict to be read into this object. This dict must follow the pgbouncer config - spec (https://pgbouncer.org/config.html) to pass validation, implementing each - section as its own subdict. Lists should be represented as python lists, not - comma-separated strings. - """ - self.update(deepcopy(data)) - self.validate() - - def read_string(self, data: str) -> None: - """Populates this class from a pgbouncer.ini file, passed in as a string. - - Args: - data: pgbouncer.ini file to be parsed, represented as a string. This string must - follow the pgbouncer config spec (https://pgbouncer.org/config.html) - """ - # Since the parser persists data across reads, we have to create a new one for every read. - parser = ConfigParser() - parser.optionxform = str - parser.read_string(data) - - self.update(deepcopy(dict(parser))) - # Convert Section objects to dictionaries, so they can hold dictionaries themselves. - for section, data in self.items(): - self[section] = dict(data) - - # ConfigParser object creates a DEFAULT section of an .ini file, which we don't need. - del self["DEFAULT"] - - self._parse_complex_variables() - self.validate() - - def _parse_complex_variables(self) -> None: - """Parse complex config variables from string representation into dicts. - - In a pgbouncer.ini file, certain values are represented by more complex data structures, - which are themselves represented as delimited strings. This method parses these strings - into more usable python objects. - - Raises: - PgbConfig.ConfigParsingError: raised when [databases] section is not available - """ - db = PgbConfig.db_section - users = PgbConfig.users_section - pgb = PgbConfig.pgb_section - - try: - for name, cfg_string in self[db].items(): - self[db][name] = parse_kv_string_to_dict(cfg_string) - except KeyError as err: - raise PgbConfig.ConfigParsingError(source=str(err)) - - for name, cfg_string in self.get(users, {}).items(): - self[users][name] = parse_kv_string_to_dict(cfg_string) - - for user_type in PgbConfig.user_types: - users = set(self[pgb].get(user_type, "").split(",")) - if "" in users: - users.remove("") - self[pgb][user_type] = users - - def render(self) -> str: - """Returns a valid pgbouncer.ini file as a string. - - Returns: - str: a string containing a valid pgbouncer.ini file. - """ - self.validate() - - # Create a copy of the config with dicts and lists parsed into valid ini strings - output_dict = deepcopy(dict(self)) - for section, subdict in output_dict.items(): - for option, config_value in subdict.items(): - if isinstance(config_value, dict): - output_dict[section][option] = parse_dict_to_kv_string(config_value) - elif isinstance(config_value, set): - output_dict[section][option] = ",".join(config_value) - else: - output_dict[section][option] = str(config_value) - - # Populate parser object with local data. - parser = ConfigParser() - parser.optionxform = str - parser.read_dict(output_dict) - - # ConfigParser can only write to a file, so write to a StringIO object and then read back - # from it. - with io.StringIO() as string_io: - parser.write(string_io) - string_io.seek(0) - output = string_io.read() - return output - - def validate(self): - """Validates that this object will provide a valid pgbouncer.ini config when rendered. - - Raises: - PgbConfig.ConfigParsingError: - - when necessary config sections [databases] and [pgbouncer] are not present. - - when necessary "logfile" and "pidfile" config values are not present. - """ - db = self.db_section - - # Ensure the config contains the bare minimum it needs to be valid - essentials = { - "databases": [], - "pgbouncer": ["logfile", "pidfile"], - } - if not set(essentials.keys()).issubset(set(self.keys())): - missing_keys = set(essentials.keys()) - (set(self.keys())) - raise PgbConfig.ConfigParsingError( - f"necessary sections not found in config: {missing_keys}" - ) - - if not set(essentials["pgbouncer"]).issubset(set(self["pgbouncer"].keys())): - missing_config_values = set(essentials["pgbouncer"]) - set(self["pgbouncer"].keys()) - raise PgbConfig.ConfigParsingError( - f"necessary pgbouncer config values not found in config: {', '.join(missing_config_values)}" - ) - - # Guarantee db names are valid - for db_id in self[db].keys(): - db_name = self[db][db_id].get("dbname", "") - self._validate_dbname(db_id) - self._validate_dbname(db_name) - - def _validate_dbname(self, string: str): - """Checks string is a valid database name. - - For a database name to be valid, it must contain only alphanumeric characters, hyphens, - and underscores. Any other invalid character must be in double quotes. - - Args: - string: the string to be validated - Raises: - PgbConfig.ConfigParsingError when database names are invalid. This can occur when - database names use the reserved "pgbouncer" database name, and when database names - do not quote invalid characters (anything that isn't alphanumeric, hyphens, or - underscores). - """ - # Check dbnames don't use the reserved "pgbouncer" database name - if string == "pgbouncer": - raise PgbConfig.ConfigParsingError(source=string) - - # Check dbnames are valid characters (alphanumeric and _- ) - search = re.compile(r"[^A-Za-z0-9-_]+").search - filtered_string = "".join(filter(search, string)) - if len(filtered_string) == 0: - # The string only contains the permitted characters - return - - # Check the contents of the string left after removing valid characters are all enclosed - # in double quotes. - quoted_substrings = re.findall(r'"(?:\\.|[^"])*"', filtered_string) - if "".join(quoted_substrings) == filtered_string: - # All substrings of invalid characters are properly quoted - return - - # dbname is invalid, raise parsing error - raise PgbConfig.ConfigParsingError(source=filtered_string) - - def set_max_db_connection_derivatives( - self, max_db_connections: int, pgb_instances: int - ) -> None: - """Calculates and sets config values from the charm config & deployment state. - - The config values that are set include: - - default_pool_size - - min_pool_size - - reserve_pool_size - - Args: - max_db_connections: the maximum number of database connections, given by the user in - the juju config - pgb_instances: the number of pgbouncer instances, which is equal to the number of CPU - cores available on the juju unit. Setting this to zero throws an error. - """ - if pgb_instances < 1: - raise PgbConfig.ConfigParsingError(source="pgb_instances cannot be less than 1 ") - - pgb = PgbConfig.pgb_section - - self[pgb]["max_db_connections"] = str(max_db_connections) - - if max_db_connections == 0: - # Values have to be derived differently if max_db_connections is unlimited. These - # values are set based on the pgbouncer default value for default_pool_size, which is - # used to create values for min_pool_size and reserve_pool_size according to the same - # ratio as below. - self[pgb]["default_pool_size"] = "20" - self[pgb]["min_pool_size"] = "10" - self[pgb]["reserve_pool_size"] = "10" - return - - effective_db_connections = max_db_connections / pgb_instances - self[pgb]["default_pool_size"] = str(math.ceil(effective_db_connections / 2)) - self[pgb]["min_pool_size"] = str(math.ceil(effective_db_connections / 4)) - self[pgb]["reserve_pool_size"] = str(math.ceil(effective_db_connections / 4)) - - def add_user(self, user: str, admin: bool = False, stats: bool = False): - """Adds a user to the config. - - Args: - user: the username for the intended user - admin: whether or not the user has admin permissions - stats: whether or not the user has stats permissions - """ - admin_users = self[PGB].get("admin_users", set()) - if admin: - self[PGB]["admin_users"] = admin_users.union({user}) - - stats_users = self[PGB].get("stats_users", set()) - if stats: - self[PGB]["stats_users"] = stats_users.union({user}) - - def remove_user(self, user: str): - """Removes a user from config. - - Args: - user: the username for the intended user. - """ - if user in self[PGB].get("admin_users", {}): - self[PGB]["admin_users"].remove(user) - if user in self[PGB].get("stats_users", {}): - self[PGB]["stats_users"].remove(user) - - class ConfigParsingError(ParsingError): - """Error raised when parsing config fails.""" - - pass - - class PgbConfigError(Exception): - """Generic Pgbouncer config error.""" - - pass - def parse_kv_string_to_dict(string: str) -> Dict[str, str]: """Parses space-separated key=value pairs into a python dict. diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 574e15778..b0c30b5d5 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -18,6 +18,7 @@ Any charm using this library should import the `psycopg2` or `psycopg2-binary` dependency. """ + import logging from collections import OrderedDict from typing import Dict, List, Optional, Set, Tuple @@ -35,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 = 24 +LIBPATCH = 25 INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles" @@ -358,9 +359,7 @@ def _generate_database_privileges_statements( statements.append( """UPDATE pg_catalog.pg_largeobject_metadata SET lomowner = (SELECT oid FROM pg_roles WHERE rolname = '{}') -WHERE lomowner = (SELECT oid FROM pg_roles WHERE rolname = '{}');""".format( - user, self.user - ) +WHERE lomowner = (SELECT oid FROM pg_roles WHERE rolname = '{}');""".format(user, self.user) ) else: for schema in schemas: @@ -562,18 +561,16 @@ def build_postgresql_parameters( parameters = {} for config, value in config_options.items(): # Filter config option not related to PostgreSQL parameters. - if not config.startswith( - ( - "durability", - "instance", - "logging", - "memory", - "optimizer", - "request", - "response", - "vacuum", - ) - ): + if not config.startswith(( + "durability", + "instance", + "logging", + "memory", + "optimizer", + "request", + "response", + "vacuum", + )): continue parameter = "_".join(config.split("_")[1:]) if parameter in ["date_style", "time_zone"]: @@ -594,8 +591,8 @@ def build_postgresql_parameters( # and the remaining as cache memory. shared_buffers = int(available_memory * 0.25) effective_cache_size = int(available_memory - shared_buffers) - parameters.setdefault("shared_buffers", f"{int(shared_buffers/10**6)}MB") - parameters.update({"effective_cache_size": f"{int(effective_cache_size/10**6)}MB"}) + parameters.setdefault("shared_buffers", f"{int(shared_buffers / 10**6)}MB") + parameters.update({"effective_cache_size": f"{int(effective_cache_size / 10**6)}MB"}) else: # Return default parameters.setdefault("shared_buffers", "128MB") diff --git a/src/charm.py b/src/charm.py index 014273d1d..0945f468b 100755 --- a/src/charm.py +++ b/src/charm.py @@ -4,25 +4,25 @@ """Charmed PgBouncer connection pooler, to run on machine charms.""" +import json import logging +import math import os import platform import pwd import shutil import subprocess -from copy import deepcopy -from typing import List, Literal, Optional, Union, get_args +from configparser import ConfigParser +from typing import Dict, List, Literal, Optional, Union, get_args from charms.data_platform_libs.v0.data_interfaces import DataPeer, DataPeerUnit from charms.grafana_agent.v0.cos_agent import COSAgentProvider from charms.operator_libs_linux.v1 import systemd from charms.operator_libs_linux.v2 import snap -from charms.pgbouncer_k8s.v0 import pgb from charms.postgresql_k8s.v0.postgresql_tls import PostgreSQLTLS from jinja2 import Template from ops import JujuVersion from ops.charm import CharmBase -from ops.framework import StoredState from ops.main import main from ops.model import ( ActiveStatus, @@ -34,10 +34,11 @@ from constants import ( APP_SCOPE, + AUTH_FILE_DATABAG_KEY, AUTH_FILE_NAME, + CFG_FILE_DATABAG_KEY, CLIENT_RELATION_NAME, EXTENSIONS_BLOCKING_MESSAGE, - INI_NAME, MONITORING_PASSWORD_KEY, PEER_RELATION_NAME, PG_USER, @@ -58,7 +59,7 @@ ) from relations.backend_database import BackendDatabaseRequires from relations.db import DbProvides -from relations.peers import AUTH_FILE_DATABAG_KEY, CFG_FILE_DATABAG_KEY, Peers +from relations.peers import Peers from relations.pgbouncer_provider import PgBouncerProvider from upgrade import PgbouncerUpgrade, get_pgbouncer_dependencies_model @@ -72,8 +73,6 @@ class PgBouncerCharm(CharmBase): """A class implementing charmed PgBouncer.""" - _stored = StoredState() - def __init__(self, *args): super().__init__(*args) @@ -142,17 +141,12 @@ def __init__(self, *args): # Charm Lifecycle Hooks # ======================= - def render_utility_files(self, cfg_file=None): + def render_utility_files(self): """Render charm utility services and configuration.""" # Initialise pgbouncer.ini config files from defaults set in charm lib and current config. # We'll add basic configs for now even if this unit isn't a leader, so systemd doesn't # throw a fit. - if not cfg_file: - cfg_file = pgb.DEFAULT_CONFIG - cfg = pgb.PgbConfig(cfg_file) - cfg["pgbouncer"]["listen_addr"] = "127.0.0.1" - cfg["pgbouncer"]["user"] = "snap_daemon" - self.render_pgb_config(cfg) + self.render_pgb_config() # Render pgbouncer service file and reload systemd with open("templates/pgbouncer.service.j2", "r") as file: @@ -291,6 +285,8 @@ def get_secret(self, scope: Scopes, key: str) -> Optional[str]: raise RuntimeError("Unknown secret scope.") peers = self.model.get_relation(PEER_RELATION_NAME) + if not peers: + return None secret_key = self._translate_field_to_secret_key(key) return self.peer_relation_data(scope).fetch_my_relation_field(peers.id, secret_key) @@ -350,26 +346,7 @@ def push_tls_files_to_workload(self, update_config: bool = True) -> bool: return self.update_config() return True - def update_tls_config(self, config, exposed: bool) -> None: - """Sets only the TLS section of a provided configuration.""" - if all(self.tls.get_tls_files()) and exposed: - config["pgbouncer"]["client_tls_key_file"] = ( - f"{PGB_CONF_DIR}/{self.app.name}/{TLS_KEY_FILE}" - ) - config["pgbouncer"]["client_tls_ca_file"] = ( - f"{PGB_CONF_DIR}/{self.app.name}/{TLS_CA_FILE}" - ) - config["pgbouncer"]["client_tls_cert_file"] = ( - f"{PGB_CONF_DIR}/{self.app.name}/{TLS_CERT_FILE}" - ) - config["pgbouncer"]["client_tls_sslmode"] = "prefer" - else: - # cleanup tls keys if present - config["pgbouncer"].pop("client_tls_key_file", None) - config["pgbouncer"].pop("client_tls_cert_file", None) - config["pgbouncer"].pop("client_tls_ca_file", None) - config["pgbouncer"].pop("client_tls_sslmode", None) - + @property def _is_exposed(self) -> bool: # There should be only one client relation for relation in self.model.relations.get(CLIENT_RELATION_NAME, []): @@ -378,17 +355,11 @@ def _is_exposed(self) -> bool: relation.id, "external-node-connectivity" ) ) + return False def update_config(self) -> bool: """Updates PgBouncer config file based on the existence of the TLS files.""" - try: - config = self.read_pgb_config() - except FileNotFoundError as err: - logger.warning(f"update_config: Unable to read config, error: {err}") - return False - - self.update_tls_config(config, self._is_exposed()) - self.render_pgb_config(config, True) + self.render_pgb_config(True) return True @@ -400,16 +371,22 @@ def _on_start(self, _) -> None: # Done first to instantiate the snap's private tmp self.unit.set_workload_version(self.version) + if ( + (auth_file := self.get_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY)) + and self.backend.auth_user + and self.backend.auth_user in auth_file + ): + self.render_auth_file(auth_file) + + if self.backend.postgres: + self.render_prometheus_service() + try: for service in self.pgb_services: logger.info(f"starting {service}") systemd.service_start(service) - if self.backend.postgres: - self.unit.status = self.check_status() - else: - # Wait for backend relation relation if it doesn't exist - self.unit.status = BlockedStatus("waiting for backend database relation") + self.update_status() except systemd.SystemdError as e: logger.error(e) self.unit.status = BlockedStatus("failed to start pgbouncer") @@ -422,177 +399,268 @@ def _on_start(self, _) -> None: self.unit.status = BlockedStatus("failed to start pgbouncer exporter") def _on_leader_elected(self, _): - self.peers.update_connection() + self.peers.update_leader() def _on_update_status(self, _) -> None: - """Update Status hook.""" - self.unit.status = self.check_status() + """Update Status hook. + + Sets BlockedStatus if we have no backend database; if we can't connect to a backend, this + charm serves no purpose. + """ + self.update_status() + + self.peers.update_leader() - def _on_config_changed(self, _) -> None: + def update_status(self): + """Health check to update pgbouncer status based on charm state.""" + if self.unit.status.message == EXTENSIONS_BLOCKING_MESSAGE: + return + + if self.backend.postgres is None: + self.unit.status = BlockedStatus("waiting for backend database relation to initialise") + return + + if not self.backend.ready: + self.unit.status = BlockedStatus("backend database relation not ready") + return + + if self.check_pgb_running(): + self.unit.status = ActiveStatus() + + def _on_config_changed(self, event) -> None: """Config changed handler. Reads charm config values, generates derivative values, writes new pgbouncer config, and restarts pgbouncer to apply changes. """ - cfg = self.read_pgb_config() - old_port = cfg["pgbouncer"]["listen_port"] - if old_port != self.config["listen_port"] and self._is_exposed(): + old_port = self.peers.app_databag.get("current_port") + if old_port != str(self.config["listen_port"]) and self._is_exposed: + if self.unit.is_leader(): + self.peers.app_databag["current_port"] = str(self.config["listen_port"]) # Open port try: if old_port: - self.unit.close_port("tcp", cfg["pgbouncer"]["listen_port"]) + self.unit.close_port("tcp", old_port) self.unit.open_port("tcp", self.config["listen_port"]) except ModelError: logger.exception("failed to open port") - if not self.unit.is_leader(): + # TODO hitting upgrade errors here due to secrets labels failing to set on non-leaders. + # deferring until the leader manages to set the label + try: + self.render_pgb_config(reload_pgbouncer=True) + except ModelError: + logger.warning("Deferring on_config_changed: cannot set secret label") + event.defer() return - - cfg["pgbouncer"]["pool_mode"] = self.config["pool_mode"] - - cfg.set_max_db_connection_derivatives( - self.config["max_db_connections"], - self._cores, - ) - - if cfg["pgbouncer"]["listen_port"] != self.config["listen_port"]: - # This emits relation-changed events to every client relation, so only do it when - # necessary - self.update_client_connection_info(self.config["listen_port"]) - cfg["pgbouncer"]["listen_port"] = self.config["listen_port"] - - self.render_pgb_config(cfg, reload_pgbouncer=True) if self.backend.postgres: self.render_prometheus_service() - def check_status(self) -> Union[ActiveStatus, BlockedStatus, WaitingStatus]: - """Checks status of PgBouncer application. - - Checks whether pgb config is available, backend is ready, and pgbouncer systemd service is - running. - - Returns: - Recommended unit status. Can be active, blocked, or waiting. - """ - try: - self.read_pgb_config() - except FileNotFoundError: - wait_str = "waiting for pgbouncer to start" - logger.warning(wait_str) - return WaitingStatus(wait_str) - - if not self.backend.ready: - # We can't relate an app to the backend database without a backend postgres relation - backend_wait_msg = "waiting for backend database relation to connect" - logger.warning(backend_wait_msg) - return BlockedStatus(backend_wait_msg) - - if self.unit.status.message == EXTENSIONS_BLOCKING_MESSAGE: - return BlockedStatus(EXTENSIONS_BLOCKING_MESSAGE) - + def check_pgb_running(self): + """Checks that pgbouncer service is running, and updates status accordingly.""" prom_service = f"{PGB}-{self.app.name}-prometheus" services = [*self.pgb_services] - if self.backend.postgres: + if self.backend.ready: services.append(prom_service) try: for service in services: if not systemd.service_running(service): - return BlockedStatus(f"{service} is not running") + pgb_not_running = f"PgBouncer service {service} not running" + logger.warning(pgb_not_running) + if self.unit.status.message != EXTENSIONS_BLOCKING_MESSAGE: + self.unit.status = BlockedStatus(pgb_not_running) + return False except systemd.SystemdError as e: logger.error(e) - return BlockedStatus("failed to get pgbouncer status") + if self.unit.status.message != EXTENSIONS_BLOCKING_MESSAGE: + self.unit.status = BlockedStatus("failed to get pgbouncer status") + return False - return ActiveStatus() + return True def reload_pgbouncer(self): """Restarts systemd pgbouncer service.""" + initial_status = self.unit.status self.unit.status = MaintenanceStatus("Reloading Pgbouncer") try: for service in self.pgb_services: systemd.service_restart(service) + self.unit.status = initial_status except systemd.SystemdError as e: logger.error(e) self.unit.status = BlockedStatus("Failed to restart pgbouncer") - self.unit.status = self.check_status() + self.check_pgb_running() # ============================== # PgBouncer-Specific Utilities # ============================== - def read_pgb_config(self) -> pgb.PgbConfig: - """Get config object from pgbouncer.ini file. + def set_relation_databases(self, databases: Dict[int, Dict[str, Union[str, bool]]]) -> None: + """Updates the relation databases.""" + self.peers.app_databag["pgb_dbs_config"] = json.dumps(databases) + + def get_relation_databases(self) -> Dict[str, Dict[str, Union[str, bool]]]: + """Get relation databases.""" + if "pgb_dbs_config" in self.peers.app_databag: + return json.loads(self.peers.app_databag["pgb_dbs_config"]) + # Nothing set in the config peer data trying to regenerate based on old data in case of update. + elif not self.unit.is_leader(): + if cfg := self.get_secret(APP_SCOPE, CFG_FILE_DATABAG_KEY): + try: + parser = ConfigParser() + parser.optionxform = str + parser.read_string(cfg) + old_cfg = dict(parser) + if databases := old_cfg.get("databases"): + databases.pop("DEFAULT", None) + result = {} + i = 1 + for database in dict(databases): + if database.endswith("_standby") or database.endswith("_readonly"): + continue + result[str(i)] = {"name": database, "legacy": False} + i += 1 + return result + except Exception: + logger.exception("Unable to parse legacy config format") + return {} + + def generate_relation_databases(self) -> Dict[str, Dict[str, Union[str, bool]]]: + """Generates a mapping between relation and database and sets it in the app databag.""" + if not self.unit.is_leader(): + return {} - Returns: - PgbConfig object containing pgbouncer config. - """ - with open(f"{PGB_CONF_DIR}/{self.app.name}/{INI_NAME}", "r") as file: - config = pgb.PgbConfig(file.read()) - return config + databases = {} + for relation in self.model.relations.get("db", []): + database = self.legacy_db_relation.get_databags(relation)[0].get("database") + if database: + databases[str(relation.id)] = { + "name": database, + "legacy": True, + } - def render_pgb_config(self, config: pgb.PgbConfig, reload_pgbouncer=False): + for relation in self.model.relations.get("db-admin", []): + database = self.legacy_db_admin_relation.get_databags(relation)[0].get("database") + if database: + databases[str(relation.id)] = { + "name": database, + "legacy": True, + } + + for rel_id, data in self.client_relation.database_provides.fetch_relation_data( + fields=["database"] + ).items(): + database = data.get("database") + if database: + databases[str(rel_id)] = { + "name": database, + "legacy": False, + } + self.set_relation_databases(databases) + return databases + + def _get_relation_config(self) -> [Dict[str, Dict[str, Union[str, bool]]]]: + """Generate pgb config for databases and admin users.""" + if not self.backend.relation or not (databases := self.get_relation_databases()): + return {} + + # In postgres, "endpoints" will only ever have one value. Other databases using the library + # can have more, but that's not planned for the postgres charm. + if not (postgres_endpoint := self.backend.postgres_databag.get("endpoints")): + return {} + host, port = postgres_endpoint.split(":") + + read_only_endpoints = self.backend.get_read_only_endpoints() + r_hosts = ",".join([r_host.split(":")[0] for r_host in read_only_endpoints]) + if r_hosts: + for r_host in read_only_endpoints: + r_port = r_host.split(":")[1] + break + + pgb_dbs = {} + + for database in databases.values(): + name = database["name"] + pgb_dbs[name] = { + "host": host, + "dbname": name, + "port": port, + "auth_user": self.backend.auth_user, + } + if r_hosts: + pgb_dbs[f"{name}_readonly"] = { + "host": r_hosts, + "dbname": name, + "port": r_port, + "auth_user": self.backend.auth_user, + } + return pgb_dbs + + def render_pgb_config(self, reload_pgbouncer=False): """Derives config files for the number of required services from given config. This method takes a primary config and generates one unique config for each intended instance of pgbouncer, implemented as a templated systemd service. """ + initial_status = self.unit.status self.unit.status = MaintenanceStatus("updating PgBouncer config") - self.peers.update_cfg(config) - - # create a copy of the config so the original reference is unchanged. - primary_config = deepcopy(config) - # Render primary config. This config is the only copy that the charm reads from to create # PgbConfig objects, and is modified below to implement individual services. app_conf_dir = f"{PGB_CONF_DIR}/{self.app.name}" app_log_dir = f"{PGB_LOG_DIR}/{self.app.name}" app_temp_dir = f"/tmp/{self.app.name}" - self._render_pgb_config( - pgb.PgbConfig(primary_config), config_path=f"{app_conf_dir}/{INI_NAME}" - ) - - # Modify & render config files for each service instance - for service_id in self.service_ids: - primary_config[PGB]["unix_socket_dir"] = f"{app_temp_dir}/{INSTANCE_DIR}{service_id}" - primary_config[PGB]["logfile"] = ( - f"{app_log_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.log" - ) - primary_config[PGB]["pidfile"] = ( - f"{app_temp_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.pid" - ) - - self._render_pgb_config( - primary_config, - config_path=f"{app_conf_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.ini", - ) - - if reload_pgbouncer: - self.reload_pgbouncer() - def _render_pgb_config( - self, - pgbouncer_ini: pgb.PgbConfig, - reload_pgbouncer: bool = False, - config_path: str = None, - ) -> None: - """Render config object to pgbouncer.ini file. - - Args: - pgbouncer_ini: PgbConfig object containing pgbouncer config. - reload_pgbouncer: A boolean defining whether or not to reload the pgbouncer - application. When config files are updated, pgbouncer must be restarted for the - changes to take effect. However, these config updates can be done in batches, - minimising the amount of necessary restarts. - config_path: intended location for the config. - """ - if config_path is None: - config_path = f"{PGB_CONF_DIR}/{self.app.name}/{INI_NAME}" - self.unit.status = MaintenanceStatus("updating PgBouncer config") - self.render_file(config_path, pgbouncer_ini.render(), 0o700) + max_db_connections = self.config["max_db_connections"] + if max_db_connections == 0: + default_pool_size = 20 + min_pool_size = 10 + reserve_pool_size = 10 + else: + effective_db_connections = max_db_connections / self._cores + default_pool_size = math.ceil(effective_db_connections / 2) + min_pool_size = math.ceil(effective_db_connections / 4) + reserve_pool_size = math.ceil(effective_db_connections / 4) + with open("templates/pgb_config.j2", "r") as file: + template = Template(file.read()) + databases = self._get_relation_config() + enable_tls = all(self.tls.get_tls_files()) and self._is_exposed + if self._is_exposed: + addr = "*" + else: + addr = "127.0.0.1" + # Modify & render config files for each service instance + for service_id in self.service_ids: + self.unit.status = MaintenanceStatus("updating PgBouncer config") + self.render_file( + f"{app_conf_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.ini", + template.render( + databases=databases, + socket_dir=f"{app_temp_dir}/{INSTANCE_DIR}{service_id}", + log_file=f"{app_log_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.log", + pid_file=f"{app_temp_dir}/{INSTANCE_DIR}{service_id}/pgbouncer.pid", + listen_addr=addr, + listen_port=self.config["listen_port"], + pool_mode=self.config["pool_mode"], + max_db_connections=max_db_connections, + default_pool_size=default_pool_size, + min_pool_size=min_pool_size, + reserve_pool_size=reserve_pool_size, + stats_user=self.backend.stats_user, + auth_query=self.backend.auth_query, + auth_file=f"{app_conf_dir}/{AUTH_FILE_NAME}", + enable_tls=enable_tls, + key_file=f"{app_conf_dir}/{TLS_KEY_FILE}", + ca_file=f"{app_conf_dir}/{TLS_CA_FILE}", + cert_file=f"{app_conf_dir}/{TLS_CERT_FILE}", + ), + 0o700, + ) + self.unit.status = initial_status if reload_pgbouncer: self.reload_pgbouncer() @@ -622,11 +690,6 @@ def render_prometheus_service(self): logger.error(e) self.unit.status = BlockedStatus("Failed to restart prometheus exporter") - def read_auth_file(self) -> str: - """Gets the auth file from the pgbouncer container filesystem.""" - with open(f"{PGB_CONF_DIR}/{self.app.name}/{AUTH_FILE_NAME}", "r") as fd: - return fd.read() - def render_auth_file(self, auth_file: str, reload_pgbouncer: bool = False): """Render user list (with encoded passwords) to pgbouncer.ini file. @@ -637,12 +700,13 @@ def render_auth_file(self, auth_file: str, reload_pgbouncer: bool = False): changes to take effect. However, these config updates can be done in batches, minimising the amount of necessary restarts. """ + initial_status = self.unit.status self.unit.status = MaintenanceStatus("updating PgBouncer users") - self.peers.update_auth_file(auth_file) self.render_file( f"{PGB_CONF_DIR}/{self.app.name}/{AUTH_FILE_NAME}", auth_file, perms=0o700 ) + self.unit.status = initial_status if reload_pgbouncer: self.reload_pgbouncer() @@ -719,7 +783,7 @@ def unit_ip(self) -> str: def update_client_connection_info(self, port: Optional[str] = None): """Update ports in backend relations to match updated pgbouncer port.""" # Skip updates if backend.postgres doesn't exist yet. - if not self.backend.postgres: + if not self.backend.postgres or not self.unit.is_leader(): return if port is None: @@ -734,28 +798,6 @@ def update_client_connection_info(self, port: Optional[str] = None): for relation in self.model.relations.get(CLIENT_RELATION_NAME, []): self.client_relation.update_connection_info(relation) - def update_postgres_endpoints(self, reload_pgbouncer): - """Update postgres endpoints in relation config values.""" - # Skip updates if backend.postgres doesn't exist yet. - if not self.backend.postgres or not self.unit.is_leader(): - return - - self.unit.status = MaintenanceStatus("Model changed - updating postgres endpoints") - - for relation in self.model.relations.get("db", []): - self.legacy_db_relation.update_postgres_endpoints(relation, reload_pgbouncer=False) - - for relation in self.model.relations.get("db-admin", []): - self.legacy_db_admin_relation.update_postgres_endpoints( - relation, reload_pgbouncer=False - ) - - for relation in self.model.relations.get(CLIENT_RELATION_NAME, []): - self.client_relation.update_postgres_endpoints(relation, reload_pgbouncer=False) - - if reload_pgbouncer: - self.reload_pgbouncer() - @property def leader_ip(self) -> str: """Gets leader ip.""" diff --git a/src/constants.py b/src/constants.py index 7d8e5fba4..16fa4e9e6 100644 --- a/src/constants.py +++ b/src/constants.py @@ -42,6 +42,8 @@ TLS_CERT_FILE = "cert.pem" MONITORING_PASSWORD_KEY = "monitoring_password" +CFG_FILE_DATABAG_KEY = "cfg_file" +AUTH_FILE_DATABAG_KEY = "auth_file" EXTENSIONS_BLOCKING_MESSAGE = "bad relation request - remote app requested extensions, which are unsupported. Please remove this relation." diff --git a/src/relations/backend_database.py b/src/relations/backend_database.py index de01153fc..42e1715f3 100644 --- a/src/relations/backend_database.py +++ b/src/relations/backend_database.py @@ -47,7 +47,7 @@ """ # noqa: W505 import logging -from typing import Dict, List, Set +from typing import Dict, List, Optional, Set import psycopg2 from charms.data_platform_libs.v0.data_interfaces import ( @@ -59,16 +59,16 @@ from ops.charm import CharmBase, RelationBrokenEvent, RelationDepartedEvent from ops.framework import Object from ops.model import ( - ActiveStatus, Application, BlockedStatus, MaintenanceStatus, Relation, + WaitingStatus, ) from constants import ( APP_SCOPE, - AUTH_FILE_NAME, + AUTH_FILE_DATABAG_KEY, BACKEND_RELATION_NAME, MONITORING_PASSWORD_KEY, PG, @@ -123,11 +123,36 @@ def _on_database_created(self, event: DatabaseCreatedEvent) -> None: Accesses user and password generated by the postgres charm and adds a user. """ - logger.info("initialising postgres and pgbouncer relations") + # When the subordinate is booting up the relation may have already been initialised + # or removed and re-added. + if ( + not (auth_file := self.charm.get_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY)) + or not self.auth_user + or self.auth_user not in auth_file + ): + auth_file = None + if not self.charm.unit.is_leader() or ( + auth_file and self.auth_user and self.auth_user in auth_file + ): + if not auth_file: + logger.debug("_on_database_created deferred: waiting for leader to initialise") + event.defer() + return + self.charm.render_auth_file(auth_file) + self.charm.render_pgb_config(reload_pgbouncer=True) + self.charm.render_prometheus_service() + self.charm.update_status() + return + + logger.info("initialising pgbouncer backend relation") self.charm.unit.status = MaintenanceStatus("Initialising backend-database relation") - if not self.charm.unit.is_leader(): + + if not self.charm.check_pgb_running(): + logger.debug("_on_database_created deferred: PGB not running") + event.defer() return - if self.postgres is None: + + if self.postgres is None or self.relation.data[self.charm.app].get("database") is None: event.defer() logger.error("deferring database-created hook - postgres database not ready") return @@ -142,35 +167,29 @@ def _on_database_created(self, event: DatabaseCreatedEvent) -> None: # Add the monitoring user. if not (monitoring_password := self.charm.get_secret(APP_SCOPE, MONITORING_PASSWORD_KEY)): monitoring_password = pgb.generate_password() - self.charm.set_secret("app", MONITORING_PASSWORD_KEY, monitoring_password) + self.charm.set_secret(APP_SCOPE, MONITORING_PASSWORD_KEY, monitoring_password) hashed_monitoring_password = pgb.get_hashed_password(self.stats_user, monitoring_password) - self.charm.render_auth_file( - f'"{self.auth_user}" "{hashed_password}"\n"{self.stats_user}" "{hashed_monitoring_password}"' - ) + auth_file = f'"{self.auth_user}" "{hashed_password}"\n"{self.stats_user}" "{hashed_monitoring_password}"' + self.charm.set_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY, auth_file) + self.charm.render_auth_file(auth_file) - cfg = self.charm.read_pgb_config() - cfg.add_user(user=self.stats_user, stats=True) - cfg["pgbouncer"]["auth_query"] = ( - f"SELECT username, password FROM {self.auth_user}.get_auth($1)" - ) - cfg["pgbouncer"]["auth_file"] = f"{PGB_CONF_DIR}/{self.charm.app.name}/{AUTH_FILE_NAME}" - self.charm.render_pgb_config(cfg) + self.charm.render_pgb_config(reload_pgbouncer=True) self.charm.render_prometheus_service() - self.charm.update_postgres_endpoints(reload_pgbouncer=True) - - self.charm.unit.status = ActiveStatus("backend-database relation initialised.") + self.charm.update_status() def _on_endpoints_changed(self, _): - self.charm.update_postgres_endpoints(reload_pgbouncer=True) - if self.charm.unit.is_leader(): - self.charm.update_client_connection_info() + self.charm.render_pgb_config(reload_pgbouncer=True) + self.charm.update_client_connection_info() def _on_relation_changed(self, _): - self.charm.update_postgres_endpoints(reload_pgbouncer=True) - if self.charm.unit.is_leader(): - self.charm.update_client_connection_info() + if not self.charm.check_pgb_running(): + logger.debug("_on_relation_changed early exit: PGB not running") + return + + self.charm.render_pgb_config(reload_pgbouncer=True) + self.charm.update_client_connection_info() def _on_relation_departed(self, event: RelationDepartedEvent): """Runs pgbouncer-uninstall.sql and removes auth user. @@ -179,15 +198,19 @@ def _on_relation_departed(self, event: RelationDepartedEvent): the postgres relation-broken hook removes the user needed to remove authentication for the users we create. """ - if self.charm.unit.is_leader(): - self.charm.update_client_connection_info() - self.charm.update_postgres_endpoints(reload_pgbouncer=True) + if self.charm.peers.relation: + self.charm.render_pgb_config(reload_pgbouncer=True) + self.charm.update_client_connection_info() if event.departing_unit == self.charm.unit: - self.charm.peers.unit_databag.update({ - f"{BACKEND_RELATION_NAME}_{event.relation.id}_departing": "true" - }) - logger.error("added relation-departing flag to peer databag") + if self.charm.peers.relation: + # This should only occur when the relation is being removed, not on scale-down + self.charm.peers.unit_databag.update({ + f"{BACKEND_RELATION_NAME}_{event.relation.id}_departing": "true" + }) + logger.warning("added relation-departing flag to peer databag") + else: + logger.warning("peer databag has already departed") return if not self.charm.unit.is_leader() or event.departing_unit.app != self.charm.app: @@ -203,6 +226,7 @@ def _on_relation_departed(self, event: RelationDepartedEvent): try: # TODO de-authorise all databases logger.info("removing auth user") + # Remove auth function before broken-hook, while we can still connect to postgres. self.remove_auth_function([PGB, PG]) except psycopg2.Error: remove_auth_fail_msg = ( @@ -221,30 +245,19 @@ def _on_relation_broken(self, event: RelationBrokenEvent): Removes all traces of this relation from pgbouncer config. """ depart_flag = f"{BACKEND_RELATION_NAME}_{event.relation.id}_departing" - if ( - self.charm.peers.unit_databag.get(depart_flag, False) - or not self.charm.unit.is_leader() - ): + if not self.charm.peers.relation or self.charm.peers.unit_databag.get(depart_flag, False): logging.info("exiting relation-broken hook - nothing to do") return - try: - cfg = self.charm.read_pgb_config() - except FileNotFoundError: - event.defer() - return - - if self.postgres: - cfg.remove_user(self.postgres.user) - cfg["pgbouncer"].pop("stats_users", None) - cfg["pgbouncer"].pop("auth_query", None) - cfg["pgbouncer"].pop("auth_file", None) - self.charm.render_pgb_config(cfg) - - self.charm.delete_file(f"{PGB_CONF_DIR}/userlist.txt") - self.charm.peers.update_auth_file(auth_file=None) + self.charm.delete_file(f"{PGB_CONF_DIR}/{self.charm.app.name}/userlist.txt") + if self.charm.unit.is_leader(): + self.charm.remove_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY) self.charm.remove_exporter_service() + self.charm.render_pgb_config(reload_pgbouncer=True) + self.charm.unit.status = BlockedStatus( + "waiting for backend database relation to initialise" + ) def initialise_auth_function(self, dbs: List[str]): """Runs an SQL script to initialise the auth function. @@ -305,7 +318,8 @@ def postgres(self) -> PostgreSQL: if not self.relation: return None - databag = self.postgres_databag + if not (databag := self.postgres_databag): + return None endpoint = databag.get("endpoints") user = self.database.fetch_relation_field(self.relation.id, "username") password = self.database.fetch_relation_field(self.relation.id, "password") @@ -323,7 +337,7 @@ def postgres(self) -> PostgreSQL: ) @property - def auth_user(self): + def auth_user(self) -> Optional[str]: """Username for auth_user.""" if not self.relation: return None @@ -334,8 +348,17 @@ def auth_user(self): return f"pgbouncer_auth_{username}".replace("-", "_") @property - def stats_user(self): + def auth_query(self) -> str: + """Generate auth query.""" + if not self.relation: + return "" + return f"SELECT username, password FROM {self.auth_user}.get_auth($1)" + + @property + def stats_user(self) -> str: """Username for stats.""" + if not self.relation: + return "" return f"pgbouncer_stats_{self.charm.app.name}".replace("-", "_") @property @@ -355,14 +378,22 @@ def postgres_databag(self) -> Dict: @property def ready(self) -> bool: - """A boolean signifying whether the backend relation is fully initialised & ready.""" + """A boolean signifying whether the backend relation is fully initialised & ready. + + This is a simple binary check to verify that we can send data from this charm to the + backend charm. + """ # Check we have connection information if not self.postgres: + logger.debug("Backend not ready: no connection info") return False - # Check we can authenticate - cfg = self.charm.read_pgb_config() - if "auth_query" not in cfg["pgbouncer"].keys(): + if ( + not (auth_file := self.charm.get_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY)) + or not self.auth_user + or self.auth_user not in auth_file + ): + logger.debug("Backend not ready: no auth file secret set") return False # Check we can actually connect to backend database by running a command. @@ -372,7 +403,7 @@ def ready(self) -> bool: cursor.execute("SELECT version();") conn.close() except (psycopg2.Error, psycopg2.OperationalError): - logger.error("PostgreSQL connection failed") + logger.warning("PostgreSQL connection failed") return False return True @@ -383,3 +414,17 @@ def get_read_only_endpoints(self) -> Set[str]: if not read_only_endpoints: return set() return set(read_only_endpoints.split(",")) + + def check_backend(self) -> bool: + """Verifies backend is ready and updates status. + + Returns: + bool signifying whether backend is ready or not + """ + if not self.ready: + # We can't relate an app to the backend database without a backend postgres relation + wait_str = "waiting for backend-database relation to connect" + logger.warning(wait_str) + self.charm.unit.status = WaitingStatus(wait_str) + return False + return True diff --git a/src/relations/db.py b/src/relations/db.py index 580d25575..90c3029d0 100644 --- a/src/relations/db.py +++ b/src/relations/db.py @@ -102,7 +102,6 @@ ) from ops.framework import Object from ops.model import ( - ActiveStatus, Application, BlockedStatus, MaintenanceStatus, @@ -111,7 +110,7 @@ WaitingStatus, ) -from constants import EXTENSIONS_BLOCKING_MESSAGE, PG +from constants import EXTENSIONS_BLOCKING_MESSAGE logger = logging.getLogger(__name__) @@ -165,6 +164,9 @@ def __init__(self, charm: CharmBase, admin: bool = False): self.charm = charm self.admin = admin + def _depart_flag(self, relation): + return f"{self.relation_name}_{relation.id}_departing" + def _check_extensions(self, extensions: List) -> bool: """Checks if requested extensions are enabled.""" for extension in extensions: @@ -202,7 +204,8 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): if not self.charm.unit.is_leader(): return - if not self._check_backend(): + if not self.charm.backend.check_backend(): + # We can't relate an app to the backend database without a backend postgres relation join_event.defer() return @@ -226,9 +229,18 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): if self._block_on_extensions(join_event.relation, remote_app_databag): return - user = self._generate_username(join_event) + user = self._generate_username(join_event.relation) password = pgb.generate_password() + if None in [database, password]: + # If database isn't available, defer + join_event.defer() + return + + dbs = self.charm.generate_relation_databases() + dbs[str(join_event.relation.id)] = {"name": database, "legacy": True} + self.charm.set_relation_databases(dbs) + self.update_databags( join_event.relation, { @@ -238,11 +250,9 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): }, ) - if not self.charm.unit.is_leader(): - return - # Create user and database in backend postgresql database try: + initial_status = self.charm.unit.status init_msg = f"initialising database and user for {self.relation_name} relation" self.charm.unit.status = MaintenanceStatus(init_msg) logger.info(init_msg) @@ -251,7 +261,8 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): self.charm.backend.postgres.create_database(database, user) created_msg = f"database and user for {self.relation_name} relation created" - self.charm.unit.status = ActiveStatus(created_msg) + self.charm.unit.status = initial_status + self.charm.update_status() logger.info(created_msg) except (PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError): err_msg = f"failed to create database or user for {self.relation_name}" @@ -262,22 +273,18 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): # set up auth function self.charm.backend.initialise_auth_function([database]) - # Create user in pgbouncer config - cfg = self.charm.read_pgb_config() - cfg.add_user(user, admin=self.admin) - self.charm.render_pgb_config(cfg, reload_pgbouncer=True) - def _on_relation_changed(self, change_event: RelationChangedEvent): """Handle db-relation-changed event. Takes information from the pgbouncer db app relation databag and copies it into the pgbouncer.ini config. - This relation will defer if the backend relation isn't fully available, and if the - relation_joined hook isn't completed. + Deferrals: + - If backend relation isn't available + - If relation_joined hook hasn't completed """ - self.charm.unit.status = self.charm.check_status() - if not isinstance(self.charm.unit.status, ActiveStatus): + if not self.charm.backend.check_backend(): + # We can't relate an app to the backend database without a backend postgres relation change_event.defer() return @@ -286,40 +293,39 @@ def _on_relation_changed(self, change_event: RelationChangedEvent): ) # No backup values because if databag isn't populated, this relation isn't initialised. - # This means that the database and user requested in this relation haven't been created, so - # we defer this event until the databag is populated. + # This means that the database and user requested in this relation haven't been created, + # so we defer this event until the databag is populated. databag = self.get_databags(change_event.relation)[0] database = databag.get("database") user = databag.get("user") password = databag.get("password") if None in [database, user, password]: - not_initialised = ( + logger.warning( "relation not fully initialised - deferring until join_event is complete" ) - logger.warning(not_initialised) - self.charm.unit.status = WaitingStatus(not_initialised) change_event.defer() return - self.update_connection_info(change_event.relation, self.charm.config["listen_port"]) - self.update_postgres_endpoints(change_event.relation, reload_pgbouncer=True) - self.update_databags( - change_event.relation, - { - "allowed-subnets": self.get_allowed_subnets(change_event.relation), - "allowed-units": self.get_allowed_units(change_event.relation), - "version": self.charm.backend.postgres.get_postgresql_version(), - "host": self.charm.unit_ip, - "user": user, - "password": password, - "database": database, - "state": self._get_state(), - }, - ) + self.charm.render_pgb_config(reload_pgbouncer=True) + if self.charm.unit.is_leader(): + self.update_connection_info(change_event.relation, self.charm.config["listen_port"]) + self.update_databags( + change_event.relation, + { + "allowed-subnets": self.get_allowed_subnets(change_event.relation), + "allowed-units": self.get_allowed_units(change_event.relation), + "version": self.charm.backend.postgres.get_postgresql_version(), + "host": "localhost", + "user": user, + "password": password, + "database": database, + "state": self._get_state(), + }, + ) def update_connection_info(self, relation: Relation, port: str = None): - """Updates databag connection info.""" + """Updates databag connection information.""" if not port: port = self.charm.config["listen_port"] @@ -349,76 +355,34 @@ def update_connection_info(self, relation: Relation, port: str = None): self.update_databags(relation, connection_updates) - def update_postgres_endpoints(self, relation: Relation, reload_pgbouncer: bool = False): - """Updates postgres replicas. - - Requires the backend relation to be running. - """ - database = self.get_databags(relation)[0].get("database") - if database is None: - logger.warning( - f"{self.relation_name} relation not fully initialised - skipping postgres endpoint update" - ) - # TODO return false - return - - # In postgres, "endpoints" will only ever have one value. Other databases using the library - # can have more, but that's not planned for the postgres charm. - postgres_endpoint = self.charm.backend.postgres_databag.get("endpoints") - - cfg = self.charm.read_pgb_config() - cfg["databases"][database] = { - "host": postgres_endpoint.split(":")[0], - "dbname": database, - "port": postgres_endpoint.split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - - read_only_endpoints = self.charm.backend.get_read_only_endpoints() - if len(read_only_endpoints) > 0: - cfg["databases"][f"{database}_standby"] = { - "host": ",".join([ep.split(":")[0] for ep in read_only_endpoints]), - "dbname": database, - "port": next(iter(read_only_endpoints)).split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - else: - cfg["databases"].pop(f"{database}_standby", None) - - if self.admin: - # Admin relations get access to postgres root db - cfg["databases"][PG] = { - "host": postgres_endpoint.split(":")[0], - "dbname": PG, - "port": postgres_endpoint.split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - - # Write config data to charm filesystem - self.charm.render_pgb_config(cfg, reload_pgbouncer=reload_pgbouncer) - def _on_relation_departed(self, departed_event: RelationDepartedEvent): """Handle db-relation-departed event. Removes relevant information from pgbouncer config when db relation is removed. This function assumes that relation databags are destroyed when the relation itself is removed. """ + # Set a flag to avoid deleting database users when this unit + # is removed and receives relation broken events from related applications. + # This is needed because of https://bugs.launchpad.net/juju/+bug/1979811. + # Neither peer relation data nor stored state are good solutions, + # just a temporary solution. + if departed_event.departing_unit == self.charm.unit: + self.charm.peers.unit_databag.update({ + self._depart_flag(departed_event.relation): "True" + }) + # Just run the rest of the logic for departing of remote units. + return + logger.info("db relation removed - updating config") logger.warning( f"DEPRECATION WARNING - {self.relation_name} is a legacy relation, and will be deprecated in a future release. " ) - self.update_databags( - departed_event.relation, - {"allowed-units": self.get_allowed_units(departed_event.relation)}, - ) - - # If a departed event is dispatched to itself, this relation isn't being scaled down - - # it's being removed - if departed_event.app == self.charm.app and self.charm.unit.is_leader(): - self.charm.peers.app_databag[ - f"{self.relation_name}-{self.relation.id}-relation-breaking" - ] = "true" + if self.charm.unit.is_leader(): + self.update_databags( + departed_event.relation, + {"allowed-units": self.get_allowed_units(departed_event.relation)}, + ) def _on_relation_broken(self, broken_event: RelationBrokenEvent): """Handle db-relation-broken event. @@ -428,25 +392,23 @@ def _on_relation_broken(self, broken_event: RelationBrokenEvent): This doesn't delete any tables so we aren't deleting a user's entire database with one command. + + Deferrals: + - If backend relation doesn't exist + - If relation data has not been fully initialised """ - # Only delete relation data if we're the leader, and we're the last unit to leave. - if not self.charm.unit.is_leader(): - self.charm.update_client_connection_info() - return - break_flag = f"{self.relation_name}-{broken_event.relation.id}-relation-breaking" - if not self.charm.peers.app_databag.get(break_flag, None): - # This relation isn't being removed, so we don't need to do the relation teardown - # steps. - self.update_connection_info(broken_event.relation) + # Run this event only if this unit isn't being removed while the others from this + # application are still alive. This check is needed because of + # https://bugs.launchpad.net/juju/+bug/1979811. Neither peer relation data nor stored state + # are good solutions, just a temporary solution. + if self._depart_flag(broken_event.relation) in self.charm.peers.unit_databag: return - del self.charm.peers.app_databag[break_flag] - databag = self.get_databags(broken_event.relation)[0] user = databag.get("user") database = databag.get("database") - if not self.charm.backend.postgres or None in [user, database]: + if not self.charm.backend.check_backend() or None in [user, database]: # this relation was never created, so wait for it to be initialised before removing # everything. logger.warning( @@ -455,38 +417,34 @@ def _on_relation_broken(self, broken_event: RelationBrokenEvent): broken_event.defer() return - cfg = self.charm.read_pgb_config() - - # check database can be deleted from pgb config, and if so, delete it. Database is kept on - # postgres application because we don't want to delete all user data with one command. - delete_db = True - relations = self.model.relations.get("db", []) + self.model.relations.get("db-admin", []) - for relation in relations: - if relation.id == broken_event.relation.id: - continue - if relation.data[self.charm.unit].get("database") == database: - # There's multiple applications using this database, so don't remove it until - # we can guarantee this is the last one. - delete_db = False - break - - if delete_db: - cfg["databases"].pop(database, None) - cfg["databases"].pop(f"{database}_standby", None) - self.charm.backend.remove_auth_function([database]) - - # TODO delete postgres database from config if there's no admin relations left - - cfg.remove_user(user) - self.charm.render_pgb_config(cfg, reload_pgbouncer=True) - - try: - self.charm.backend.postgres.delete_user(user) - except PostgreSQLDeleteUserError as err: - # We've likely lost connection at this point, and can't do anything about a - # trailing user. - logger.error(f"connection lost to PostgreSQL - unable to delete user {user}.") - logger.error(err) + dbs = self.charm.generate_relation_databases() + dbs.pop(str(broken_event.relation.id), None) + self.charm.set_relation_databases(dbs) + if self.charm.unit.is_leader(): + # check database can be deleted from pgb config, and if so, delete it. Database is kept on + # postgres application because we don't want to delete all user data with one command. + delete_db = True + relations = self.model.relations.get("db", []) + self.model.relations.get( + "db-admin", [] + ) + for relation in relations: + if relation.id == broken_event.relation.id: + continue + if relation.data[self.charm.unit].get("database") == database: + # There's multiple applications using this database, so don't remove it until + # we can guarantee this is the last one. + delete_db = False + break + + if delete_db: + self.charm.backend.remove_auth_function([database]) + + try: + self.charm.backend.postgres.delete_user(user) + except PostgreSQLDeleteUserError: + # We've likely lost connection at this point, and can't do anything about a + # trailing user. + logger.exception(f"connection lost to PostgreSQL - unable to delete user {user}.") def get_databags(self, relation): """Returns a list of writable databags for this unit.""" @@ -497,19 +455,17 @@ def get_databags(self, relation): def update_databags(self, relation, updates: Dict[str, str]): """Updates databag with the given dict.""" - databags = self.get_databags(relation) - # Databag entries can only be strings for key, item in updates.items(): updates[key] = str(item) - for databag in databags: + for databag in self.get_databags(relation): databag.update(updates) - def _generate_username(self, event): + def _generate_username(self, relation): """Generates a unique username for this relation.""" app_name = self.charm.app.name - relation_id = event.relation.id + relation_id = relation.id model_name = self.model.name return f"{app_name}_user_{relation_id}_{model_name}".replace("-", "_") @@ -560,17 +516,3 @@ def get_external_app(self, relation): for entry in relation.data.keys(): if isinstance(entry, Application) and entry != self.charm.app: return entry - - def _check_backend(self) -> bool: - """Verifies backend is ready, sets waiting status if not. - - Returns: - bool signifying whether backend is ready or not - """ - if not self.charm.backend.ready: - # We can't relate an app to the backend database without a backend postgres relation - wait_str = "waiting for backend-database relation to connect" - logger.warning(wait_str) - self.charm.unit.status = WaitingStatus(wait_str) - return False - return True diff --git a/src/relations/peers.py b/src/relations/peers.py index 489dcab0a..68a0602c4 100644 --- a/src/relations/peers.py +++ b/src/relations/peers.py @@ -15,33 +15,7 @@ │ application data │ ╭─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ │ │ │ │ │ │ │ │ auth_file "pgbouncer_auth_relation_3" "md5a430a66f6761df1b5d1d608ed345e44f" │ │ -│ │ │ cfg_file │ │ -│ │ │ cli = host=10.180.162.244 dbname=cli port=5432 auth_user=pgbouncer_auth_relation_3 │ │ -│ │ │ postgres = host=10.180.162.244 dbname=postgres port=5432 auth_user=pgbouncer_auth_relation_3 │ │ -│ │ │ │ │ -│ │ │ │ │ -│ │ │ listen_addr = * │ │ -│ │ │ listen_port = 6432 │ │ -│ │ │ logfile = /var/lib/postgresql/pgbouncer/pgbouncer.log │ │ -│ │ │ pidfile = /var/lib/postgresql/pgbouncer/pgbouncer.pid │ │ -│ │ │ admin_users = relation-3,pgbouncer_user_4_test_db_admin_3una │ │ -│ │ │ stats_users = │ │ -│ │ │ auth_type = md5 │ │ -│ │ │ user = postgres │ │ -│ │ │ max_client_conn = 10000 │ │ -│ │ │ ignore_startup_parameters = extra_float_digits │ │ -│ │ │ server_tls_sslmode = prefer │ │ -│ │ │ so_reuseport = 1 │ │ -│ │ │ unix_socket_dir = /var/lib/postgresql/pgbouncer │ │ -│ │ │ pool_mode = session │ │ -│ │ │ max_db_connections = 100 │ │ -│ │ │ default_pool_size = 13 │ │ -│ │ │ min_pool_size = 7 │ │ -│ │ │ reserve_pool_size = 7 │ │ -│ │ │ auth_query = SELECT username, password FROM pgbouncer_auth_relation_3.get_auth($1) │ │ -│ │ │ auth_file = /var/lib/postgresql/pgbouncer/userlist.txt │ │ -│ │ │ │ │ -│ │ │ │ │ +│ │ │ pgb_dbs_config '{"1": {"name": "db_name", "legacy": false}}' │ │ │ │ │ leader 10.180.162.4 │ │ │ │ │ pgbouncer_user_4_test_db_admin_3una T6NX0iz1ZRHZF5kfYDanKM5z │ │ │ │ ╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ │ @@ -54,16 +28,12 @@ import logging from typing import List, Optional, Set -from charms.pgbouncer_k8s.v0.pgb import PgbConfig -from ops.charm import CharmBase, RelationChangedEvent, RelationCreatedEvent +from ops.charm import CharmBase, HookEvent from ops.framework import Object from ops.model import Relation, Unit -from ops.pebble import ConnectionError -from constants import APP_SCOPE, PEER_RELATION_NAME +from constants import APP_SCOPE, AUTH_FILE_DATABAG_KEY, PEER_RELATION_NAME -CFG_FILE_DATABAG_KEY = "cfg_file" -AUTH_FILE_DATABAG_KEY = "auth_file" ADDRESS_KEY = "private-address" LEADER_ADDRESS_KEY = "leader_ip" @@ -87,8 +57,7 @@ def __init__(self, charm: CharmBase): self.charm = charm - self.framework.observe(charm.on[PEER_RELATION_NAME].relation_created, self._on_created) - self.framework.observe(charm.on[PEER_RELATION_NAME].relation_joined, self._on_changed) + self.framework.observe(charm.on[PEER_RELATION_NAME].relation_joined, self._on_joined) self.framework.observe(charm.on[PEER_RELATION_NAME].relation_changed, self._on_changed) self.framework.observe(charm.on.secret_changed, self._on_changed) self.framework.observe(charm.on.secret_remove, self._on_changed) @@ -148,91 +117,29 @@ def _get_unit_ip(self, unit: Unit) -> Optional[str]: else: return None - def _on_created(self, event: RelationCreatedEvent): - if not self.charm.unit.is_leader(): - return - - try: - cfg = self.charm.read_pgb_config() - self.update_cfg(cfg) - except FileNotFoundError: - # If there's no config, the charm start hook hasn't fired yet, so defer until it's - # available. - event.defer() - return + def _on_joined(self, event: HookEvent): + self._on_changed(event) + if self.charm.unit.is_leader(): + self.charm.client_relation.update_read_only_endpoints() - if self.charm.backend.postgres: - # The backend relation creates the userlist, so only upload userlist to databag if - # backend relation is initialised. If not, it'll be added when that relation first - # writes it to the filesystem, so no need to add it now. - try: - self.update_auth_file(self.charm.read_auth_file()) - except FileNotFoundError: - # Subordinate leader got recreated - if auth_file := self.charm.get_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY): - self.charm.render_auth_file(auth_file) - - def _on_changed(self, event: RelationChangedEvent): + def _on_changed(self, event: HookEvent): """If the current unit is a follower, write updated config and auth files to filesystem.""" self.unit_databag.update({ADDRESS_KEY: self.charm.unit_ip}) - if self.charm.unit.is_leader(): - self.charm.update_client_connection_info() - try: - cfg = self.charm.read_pgb_config() - except FileNotFoundError: - # If there's no config, the charm start hook hasn't fired yet, so defer until it's - # available. - event.defer() - return - - self.app_databag[LEADER_ADDRESS_KEY] = self.charm.unit_ip - return - - if cfg := self.charm.get_secret(APP_SCOPE, CFG_FILE_DATABAG_KEY): - self.charm.render_pgb_config(PgbConfig(cfg)) - + self.update_leader() if auth_file := self.charm.get_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY): self.charm.render_auth_file(auth_file) if self.charm.backend.postgres: self.charm.render_prometheus_service() - if cfg is not None or auth_file is not None: - try: - # raises an error if this is fired before on_pebble_ready. - self.charm.reload_pgbouncer() - except ConnectionError: - event.defer() + self.charm.render_pgb_config(reload_pgbouncer=True) def _on_departed(self, _): - self.update_connection() + self.update_leader() - def update_connection(self): - """Updates available leader in app databag.""" + def update_leader(self): + """Updates leader hostname in peer databag to match this unit if it's the leader.""" if self.charm.unit.is_leader(): self.charm.update_client_connection_info() self.app_databag[LEADER_ADDRESS_KEY] = self.charm.unit_ip - - def update_cfg(self, cfg: PgbConfig) -> None: - """Writes cfg to app databag if leader.""" - if not self.charm.unit.is_leader() or not self.relation: - return - - self.charm.set_secret(APP_SCOPE, CFG_FILE_DATABAG_KEY, cfg.render()) - logger.debug("updated config file in peer databag") - - def get_cfg(self) -> PgbConfig: - """Retrieves the pgbouncer config from the peer databag.""" - if cfg := self.charm.get_secret(APP_SCOPE, CFG_FILE_DATABAG_KEY): - return PgbConfig(cfg) - else: - return None - - def update_auth_file(self, auth_file: str) -> None: - """Writes auth_file to app databag if leader.""" - if not self.charm.unit.is_leader() or not self.relation: - return - - self.charm.set_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY, auth_file) - logger.debug("updated auth file in peer databag") diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index 959c75c11..3a68af877 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -42,7 +42,6 @@ DatabaseRequestedEvent, ) from charms.pgbouncer_k8s.v0 import pgb -from charms.pgbouncer_k8s.v0.pgb import PgbConfig from charms.postgresql_k8s.v0.postgresql import ( PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, @@ -51,7 +50,12 @@ ) from ops.charm import CharmBase, RelationBrokenEvent, RelationDepartedEvent from ops.framework import Object -from ops.model import Application, BlockedStatus, ModelError, Relation, WaitingStatus +from ops.model import ( + Application, + BlockedStatus, + MaintenanceStatus, + ModelError, +) from constants import CLIENT_RELATION_NAME @@ -89,12 +93,21 @@ def __init__(self, charm: CharmBase, relation_name: str = CLIENT_RELATION_NAME) charm.on[self.relation_name].relation_broken, self._on_relation_broken ) + def _depart_flag(self, relation): + return f"{self.relation_name}_{relation.id}_departing" + + def _unit_departing(self, relation): + return self.charm.peers.unit_databag.get(self._depart_flag(relation), None) == "true" + def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: """Handle the client relation-requested event. Generate password and handle user and database creation for the related application. + + Deferrals: + - If backend relation is not fully initialised """ - if not self._check_backend(): + if not self.charm.backend.check_backend(): event.defer() return @@ -110,22 +123,22 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: return # Retrieve the database name and extra user roles using the charm library. - databases = event.database + database = event.database extra_user_roles = event.extra_user_roles or "" rel_id = event.relation.id # Creates the user and the database for this specific relation. user = f"relation_id_{rel_id}" + logger.debug("generating relation user") password = pgb.generate_password() try: self.charm.backend.postgres.create_user( user, password, extra_user_roles=extra_user_roles ) - dblist = databases.split(",") - for database in dblist: - self.charm.backend.postgres.create_database(database, user) + logger.debug("creating database") + self.charm.backend.postgres.create_database(database, user) # set up auth function - self.charm.backend.initialise_auth_function(dbs=dblist) + self.charm.backend.initialise_auth_function(dbs=[database]) except ( PostgreSQLCreateDatabaseError, PostgreSQLCreateUserError, @@ -137,50 +150,49 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: ) return - # Update pgbouncer config - cfg = self.charm.read_pgb_config() - cfg.add_user(user, admin=True if "SUPERUSER" in extra_user_roles else False) - if event.external_node_connectivity: - cfg["pgbouncer"]["listen_addr"] = "*" - self.charm.update_tls_config(cfg, True) - self.update_postgres_endpoints(event.relation, cfg=cfg) - self.charm.render_pgb_config(cfg, reload_pgbouncer=True) + dbs = self.charm.generate_relation_databases() + dbs[str(event.relation.id)] = {"name": database, "legacy": False} + self.charm.set_relation_databases(dbs) # Share the credentials and updated connection info with the client application. self.database_provides.set_credentials(rel_id, user, password) # Set the database name - self.database_provides.set_database(rel_id, databases) + self.database_provides.set_database(rel_id, database) self.update_connection_info(event.relation) def _on_relation_departed(self, event: RelationDepartedEvent) -> None: - """Check if this relation is being removed, and update the peer databag accordingly.""" + """Check if this relation is being removed, and update databags accordingly. + + If the leader is being removed, we check if this unit is departing. This occurs only on + relation deletion, so we set a flag for the relation-broken hook to remove the relation. + When scaling down, we don't set this flag and we just let the newly elected leader take + control of the pgbouncer config. + """ self.update_connection_info(event.relation) + + # This only ever evaluates to true when the relation is being removed - on app scale-down, + # depart events are only sent to the other application in the relation. if event.departing_unit == self.charm.unit: - self.charm.peers.unit_databag.update({ - f"{self.relation_name}_{event.relation.id}_departing": "true" - }) + self.charm.peers.unit_databag.update({self._depart_flag(event.relation): "true"}) def _on_relation_broken(self, event: RelationBrokenEvent) -> None: """Remove the user created for this relation, and revoke connection permissions.""" - if not self._check_backend() or not self.charm.unit.is_leader(): + self.update_connection_info(event.relation) + if not self.charm.backend.check_backend() or not self.charm.unit.is_leader(): return - depart_flag = f"{self.relation_name}_{event.relation.id}_departing" - if self.charm.peers.unit_databag.get(depart_flag, None) == "true": + if self._unit_departing(event.relation): # This unit is being removed, so don't update the relation. - self.charm.peers.unit_databag.pop(depart_flag, None) + self.charm.peers.unit_databag.pop(self._depart_flag(event.relation), None) return - cfg = self.charm.read_pgb_config() - database = self.get_database(event.relation) - cfg["databases"].pop(database, None) - cfg["databases"].pop(f"{database}_readonly", None) - user = f"relation_id_{event.relation.id}" - cfg.remove_user(user) - self.charm.render_pgb_config(cfg, reload_pgbouncer=True) + dbs = self.charm.generate_relation_databases() + dbs.pop(str(event.relation.id), None) + self.charm.set_relation_databases(dbs) # Delete the user. try: + user = f"relation_id_{event.relation.id}" self.charm.backend.postgres.delete_user(user) except PostgreSQLDeleteUserError as e: logger.exception(e) @@ -195,6 +207,12 @@ def update_connection_info(self, relation): exposed = bool( self.database_provides.fetch_relation_field(relation.id, "external-node-connectivity") ) + if not self.charm.unit.is_leader(): + return + initial_status = self.charm.unit.status + self.charm.unit.status = MaintenanceStatus( + f"Updating {self.relation_name} relation connection information" + ) if exposed: rw_endpoint = f"{self.charm.leader_ip}:{self.charm.config['listen_port']}" else: @@ -207,58 +225,13 @@ def update_connection_info(self, relation): self.update_read_only_endpoints() # Set the database version. - if self._check_backend(): + if self.charm.backend.check_backend(): self.database_provides.set_version( relation.id, self.charm.backend.postgres.get_postgresql_version() ) - def update_postgres_endpoints( - self, - relation: Relation, - cfg: PgbConfig = None, - render_cfg: bool = True, - reload_pgbouncer: bool = False, - ): - """Updates postgres replicas.""" - database = self.get_database(relation) - if database is None: - logger.warning("relation not fully initialised - skipping postgres endpoint update") - return - - if not cfg: - cfg = self.charm.read_pgb_config() - render_cfg = True - - # In postgres, "endpoints" will only ever have one value. Other databases using the library - # can have more, but that's not planned for the postgres charm. - postgres_endpoint = self.charm.backend.postgres_databag.get("endpoints") - cfg["databases"][database] = { - "host": postgres_endpoint.split(":")[0], - "dbname": database, - "port": postgres_endpoint.split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - - read_only_endpoints = self.charm.backend.get_read_only_endpoints() - if len(read_only_endpoints) > 0: - # remove ports from endpoints, and convert to comma-separated list. - # NOTE: comma-separated readonly hosts are only valid in pgbouncer v1.17. This will - # enable load balancing once the snap is implemented. - for ep in read_only_endpoints: - break - r_hosts = ",".join([host.split(":")[0] for host in read_only_endpoints]) - cfg["databases"][f"{database}_readonly"] = { - "host": r_hosts, - "dbname": database, - "port": ep.split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - else: - cfg["databases"].pop(f"{database}_readonly", None) - - # Write config data to charm filesystem - if render_cfg: - self.charm.render_pgb_config(cfg, reload_pgbouncer=reload_pgbouncer) + self.charm.unit.status = initial_status + self.charm.update_status() def update_read_only_endpoints(self, event: DatabaseRequestedEvent = None) -> None: """Set the read-only endpoint only if there are replicas.""" @@ -270,13 +243,20 @@ def update_read_only_endpoints(self, event: DatabaseRequestedEvent = None) -> No relations = [event.relation] if event else self.model.relations[self.relation_name] port = self.charm.config["listen_port"] - ips = set(self.charm.peers.units_ips) + ips = self.charm.peers.units_ips ips.discard(self.charm.peers.leader_ip) + ips = list(ips) + ips.sort() for relation in relations: - self.database_provides.set_read_only_endpoints( - relation.id, - ",".join([f"{host}:{port}" for host in ips]), - ) + if bool( + self.database_provides.fetch_relation_field( + relation.id, "external-node-connectivity" + ) + ): + self.database_provides.set_read_only_endpoints( + relation.id, + ",".join([f"{host}:{port}" for host in ips]), + ) def get_database(self, relation): """Gets database name from relation.""" @@ -290,17 +270,3 @@ def get_external_app(self, relation): for entry in relation.data.keys(): if isinstance(entry, Application) and entry != self.charm.app: return entry - - def _check_backend(self) -> bool: - """Verifies backend is ready, defers event if not. - - Returns: - bool signifying whether backend is ready or not - """ - if not self.charm.backend.ready: - # We can't relate an app to the backend database without a backend postgres relation - wait_str = "waiting for backend-database relation to connect" - logger.warning(wait_str) - self.charm.unit.status = WaitingStatus(wait_str) - return False - return True diff --git a/src/upgrade.py b/src/upgrade.py index 49d1a0651..8043c5dd5 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -14,13 +14,12 @@ UpgradeGrantedEvent, ) from charms.operator_libs_linux.v1 import systemd -from ops.model import ActiveStatus, MaintenanceStatus +from ops.model import MaintenanceStatus from pydantic import BaseModel from tenacity import Retrying, stop_after_attempt, wait_fixed from typing_extensions import override -from constants import APP_SCOPE, SNAP_PACKAGES -from relations.peers import CFG_FILE_DATABAG_KEY +from constants import SNAP_PACKAGES DEFAULT_MESSAGE = "Pre-upgrade check failed and cannot safely upgrade" @@ -66,7 +65,7 @@ def log_rollback_instructions(self) -> None: def _cluster_checks(self) -> None: """Check that the cluster is in healthy state.""" - if not isinstance(self.charm.check_status(), ActiveStatus): + if not self.charm.check_pgb_running(): raise ClusterNotReadyError(DEFAULT_MESSAGE, "Not all pgbouncer services are up yet.") if self.charm.backend.postgres and not self.charm.backend.ready: @@ -85,8 +84,10 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None: self.charm._install_snap_packages(packages=SNAP_PACKAGES, refresh=True) self.charm.unit.status = MaintenanceStatus("restarting services") - cfg = self.charm.get_secret(APP_SCOPE, CFG_FILE_DATABAG_KEY) - self.charm.render_utility_files(cfg) + if self.charm.unit.is_leader(): + self.charm.generate_relation_databases() + + self.charm.render_utility_files() self.charm.reload_pgbouncer() if self.charm.backend.postgres: self.charm.render_prometheus_service() @@ -96,7 +97,7 @@ def _on_upgrade_granted(self, event: UpgradeGrantedEvent) -> None: self._cluster_checks() self.set_unit_completed() - self.charm.unit.status = ActiveStatus() + self.charm.update_status() # Ensures leader gets its own relation-changed when it upgrades if self.charm.unit.is_leader(): diff --git a/templates/pgb_config.j2 b/templates/pgb_config.j2 new file mode 100644 index 000000000..4aa9d2ff1 --- /dev/null +++ b/templates/pgb_config.j2 @@ -0,0 +1,31 @@ +[databases] +{% for name, database in databases.items() %} +{{ name }} = host={{ database.host }} dbname={{ database.dbname }} port={{ database.port }} auth_user={{ database.auth_user }} +{% endfor %} + +[pgbouncer] +listen_addr = {{ listen_addr }} +listen_port = {{ listen_port }} +logfile = {{ log_file }} +pidfile = {{ pid_file }} +stats_users = {{ stats_user }} +auth_type = md5 +user = snap_daemon +max_client_conn = 10000 +ignore_startup_parameters = extra_float_digits,options +server_tls_sslmode = prefer +so_reuseport = 1 +unix_socket_dir = {{ socket_dir }} +pool_mode = {{ pool_mode }} +max_db_connections = {{ max_db_connections }} +default_pool_size = {{ default_pool_size }} +min_pool_size = {{ min_pool_size }} +reserve_pool_size = {{ reserve_pool_size }} +auth_query = {{ auth_query }} +auth_file = {{ auth_file }} +{% if enable_tls %} +client_tls_key_file = {{ key_file }} +client_tls_ca_file = {{ ca_file }} +client_tls_cert_file = {{ cert_file }} +client_tls_sslmode = prefer +{% endif %} diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 35c6f9050..29d5bb27c 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -10,14 +10,12 @@ from .helpers.postgresql_helpers import get_leader_unit -@pytest.mark.abort_on_fail @pytest.fixture(scope="module") async def pgb_charm_focal(ops_test: OpsTest): """Build the pgbouncer charm.""" return await ops_test.build_charm(".", bases_index=0) -@pytest.mark.abort_on_fail @pytest.fixture(scope="module") async def pgb_charm_jammy(ops_test: OpsTest): """Build the pgbouncer charm.""" diff --git a/tests/integration/helpers/helpers.py b/tests/integration/helpers/helpers.py index 2a96304e6..09c2f94fd 100644 --- a/tests/integration/helpers/helpers.py +++ b/tests/integration/helpers/helpers.py @@ -5,12 +5,12 @@ import asyncio import json import logging +from configparser import ConfigParser from multiprocessing import ProcessError from pathlib import Path from typing import Dict import yaml -from charms.pgbouncer_k8s.v0 import pgb from juju.unit import Unit from pytest_operator.plugin import OpsTest from tenacity import RetryError, Retrying, stop_after_delay, wait_fixed @@ -26,7 +26,7 @@ WEEBL = "weebl" MAILMAN3 = "mailman3-core" -WAIT_MSG = "waiting for backend database relation to connect" +WAIT_MSG = "waiting for backend database relation to initialise" def get_backend_relation(ops_test: OpsTest): @@ -112,13 +112,24 @@ async def cat_file_from_unit(ops_test: OpsTest, filepath: str, unit_name: str) - return output -async def get_cfg(ops_test: OpsTest, unit_name: str, path: str = None) -> pgb.PgbConfig: +async def get_cfg(ops_test: OpsTest, unit_name: str, path: str = None) -> dict: """Gets pgbouncer config from unit filesystem.""" if path is None: app_name = unit_name.split("/")[0] - path = f"{PGB_CONF_DIR}/{app_name}/{INI_NAME}" - cat = await cat_file_from_unit(ops_test, path, unit_name) - return pgb.PgbConfig(cat) + path = f"{PGB_CONF_DIR}/{app_name}/instance_0/{INI_NAME}" + parser = ConfigParser() + parser.optionxform = str + parser.read_string(await cat_file_from_unit(ops_test, path, unit_name)) + + cfg = dict(parser) + # Convert Section objects to dictionaries, so they can hold dictionaries themselves. + for section, data in cfg.items(): + cfg[section] = dict(data) + + # ConfigParser object creates a DEFAULT section of an .ini file, which we don't need. + del cfg["DEFAULT"] + + return cfg async def get_auth_file(ops_test: OpsTest, unit_name) -> str: diff --git a/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py b/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py index 5759ac1bd..8920d044b 100644 --- a/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py +++ b/tests/integration/relations/pgbouncer_provider/test_pgbouncer_provider.py @@ -20,6 +20,7 @@ get_app_relation_databag, get_backend_relation, get_backend_user_pass, + get_cfg, scale_application, ) from tests.integration.helpers.postgresql_helpers import check_database_users_existence @@ -177,13 +178,17 @@ async def test_database_admin_permissions(ops_test: OpsTest): async def test_no_read_only_endpoint_in_standalone_cluster(ops_test: OpsTest): """Test that there is no read-only endpoint in a standalone cluster.""" await scale_application(ops_test, CLIENT_APP_NAME, 1) - await check_new_relation( - ops_test, - unit_name=ops_test.model.applications[CLIENT_APP_NAME].units[0].name, - relation_id=client_relation.id, - relation_name=FIRST_DATABASE_RELATION_NAME, - dbname=TEST_DBNAME, - ) + cfg = await get_cfg(ops_test, ops_test.model.applications[PGB].units[0].name) + logger.info(cfg) + for unit in ops_test.model.applications[CLIENT_APP_NAME].units: + logger.info(f"Checking connection for {unit.name}") + await check_new_relation( + ops_test, + unit_name=unit.name, + relation_id=client_relation.id, + relation_name=FIRST_DATABASE_RELATION_NAME, + dbname=TEST_DBNAME, + ) unit = ops_test.model.applications[CLIENT_APP_NAME].units[0] databag = await get_app_relation_databag(ops_test, unit.name, client_relation.id) @@ -196,14 +201,17 @@ async def test_no_read_only_endpoint_in_standalone_cluster(ops_test: OpsTest): async def test_no_read_only_endpoint_in_scaled_up_cluster(ops_test: OpsTest): """Test that there is read-only endpoint in a scaled up cluster.""" await scale_application(ops_test, CLIENT_APP_NAME, 2) - await check_new_relation( - ops_test, - unit_name=ops_test.model.applications[CLIENT_APP_NAME].units[0].name, - relation_id=client_relation.id, - relation_name=FIRST_DATABASE_RELATION_NAME, - dbname=TEST_DBNAME, - ) - + cfg = await get_cfg(ops_test, ops_test.model.applications[PGB].units[0].name) + logger.info(cfg) + for unit in ops_test.model.applications[CLIENT_APP_NAME].units: + logger.info(f"Checking connection for {unit.name}") + await check_new_relation( + ops_test, + unit_name=unit.name, + relation_id=client_relation.id, + relation_name=FIRST_DATABASE_RELATION_NAME, + dbname=TEST_DBNAME, + ) unit = ops_test.model.applications[CLIENT_APP_NAME].units[0] databag = await get_app_relation_databag(ops_test, unit.name, client_relation.id) assert not databag.get( @@ -329,60 +337,52 @@ async def test_scaling(ops_test: OpsTest): """Check these relations all work when scaling pgbouncer.""" await scale_application(ops_test, CLIENT_APP_NAME, 1) await ops_test.model.wait_for_idle() - await check_new_relation( - ops_test, - unit_name=ops_test.model.applications[CLIENT_APP_NAME].units[0].name, - relation_id=client_relation.id, - relation_name=FIRST_DATABASE_RELATION_NAME, - dbname=TEST_DBNAME, - ) + cfg = await get_cfg(ops_test, ops_test.model.applications[PGB].units[0].name) + logger.info(cfg) + for unit in ops_test.model.applications[CLIENT_APP_NAME].units: + logger.info(f"Checking connection for {unit.name}") + await check_new_relation( + ops_test, + unit_name=unit.name, + relation_id=client_relation.id, + relation_name=FIRST_DATABASE_RELATION_NAME, + dbname=TEST_DBNAME, + ) await scale_application(ops_test, CLIENT_APP_NAME, 2) await ops_test.model.wait_for_idle() - await check_new_relation( - ops_test, - unit_name=ops_test.model.applications[CLIENT_APP_NAME].units[0].name, - relation_id=client_relation.id, - relation_name=FIRST_DATABASE_RELATION_NAME, - dbname=TEST_DBNAME, - ) - - # Break the relation so that test_relation_broken can be conditionally skipped - await ops_test.model.applications[PGB].remove_relation( - f"{PGB}:database", f"{CLIENT_APP_NAME}:{FIRST_DATABASE_RELATION_NAME}" - ) - async with ops_test.fast_forward(): - await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active", raise_on_blocked=True) + cfg = await get_cfg(ops_test, ops_test.model.applications[PGB].units[0].name) + logger.info(cfg) + for unit in ops_test.model.applications[CLIENT_APP_NAME].units: + logger.info(f"Checking connection for {unit.name}") + await check_new_relation( + ops_test, + unit_name=unit.name, + relation_id=client_relation.id, + relation_name=FIRST_DATABASE_RELATION_NAME, + dbname=TEST_DBNAME, + ) @pytest.mark.group(1) @pytest.mark.unstable async def test_relation_broken(ops_test: OpsTest): """Test that the user is removed when the relation is broken.""" - client_relation = await ops_test.model.add_relation( - f"{CLIENT_APP_NAME}:{FIRST_DATABASE_RELATION_NAME}", PGB - ) - - await ops_test.model.wait_for_idle(status="active", timeout=600) - - client_unit_name = ops_test.model.applications[CLIENT_APP_NAME].units[0].name # Retrieve the relation user. - databag = await get_app_relation_databag(ops_test, client_unit_name, client_relation.id) - relation_user = databag.get("username", None) - logging.info(f"relation user: {relation_user}") - assert relation_user, f"no relation user in client databag: {databag}" + relation_user = await get_application_relation_data( + ops_test, CLIENT_APP_NAME, FIRST_DATABASE_RELATION_NAME, "username" + ) + # Break the relation. backend_rel = get_backend_relation(ops_test) pg_user, pg_pass = await get_backend_user_pass(ops_test, backend_rel) - - # Break the relation. await ops_test.model.applications[PGB].remove_relation( f"{PGB}:database", f"{CLIENT_APP_NAME}:{FIRST_DATABASE_RELATION_NAME}" ) async with ops_test.fast_forward(): await ops_test.model.wait_for_idle(apps=APP_NAMES, status="active", raise_on_blocked=True) + time.sleep(20) - time.sleep(10) # Check that the relation user was removed from the database. await check_database_users_existence( ops_test, [], [relation_user], pg_user=pg_user, pg_user_password=pg_pass diff --git a/tests/integration/relations/test_backend_database.py b/tests/integration/relations/test_backend_database.py index b85915475..f87aeefeb 100644 --- a/tests/integration/relations/test_backend_database.py +++ b/tests/integration/relations/test_backend_database.py @@ -48,7 +48,7 @@ async def test_relate_pgbouncer_to_postgres(ops_test: OpsTest, pgb_charm_jammy): await ops_test.model.wait_for_idle(apps=[PGB], status="active", timeout=600) cfg = await get_cfg(ops_test, f"{PGB}/0") - logger.info(cfg.render()) + logger.info(cfg) pgb_user, pgb_password = await get_backend_user_pass(ops_test, relation) assert cfg["pgbouncer"]["auth_query"] @@ -79,7 +79,7 @@ async def test_relate_pgbouncer_to_postgres(ops_test: OpsTest, pgb_charm_jammy): assert False, "pgbouncer config files failed to update in 3 minutes" cfg = await get_cfg(ops_test, f"{PGB}/0") - logger.info(cfg.render()) + logger.info(cfg) await ops_test.model.remove_application(CLIENT_APP_NAME, block_until_done=True) await ops_test.model.remove_application(PGB, block_until_done=True) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 0971a008d..f98ba9d5a 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -6,7 +6,6 @@ import logging import pytest -from charms.pgbouncer_k8s.v0.pgb import DEFAULT_CONFIG, PgbConfig from pytest_operator.plugin import OpsTest from constants import BACKEND_RELATION_NAME, PGB_CONF_DIR, PGB_LOG_DIR @@ -75,14 +74,10 @@ async def test_change_config(ops_test: OpsTest): # The config changes depending on the amount of cores on the unit, so get that info. cores = await get_unit_cores(ops_test, unit) - expected_cfg = PgbConfig(DEFAULT_CONFIG) - expected_cfg["pgbouncer"]["pool_mode"] = "transaction" - expected_cfg.set_max_db_connection_derivatives(44, cores) - primary_cfg = await get_cfg(ops_test, unit.name) - existing_cfg = PgbConfig(primary_cfg) - assert existing_cfg.render() == primary_cfg.render() + assert primary_cfg["pgbouncer"]["pool_mode"] == "transaction" + assert primary_cfg["pgbouncer"]["max_db_connections"] == "44" # Validating service config files are correctly written is handled by render_pgb_config and its # tests, but we need to make sure they at least exist in the right places. diff --git a/tests/unit/data/test.ini b/tests/unit/data/test.ini deleted file mode 100644 index 7fa118b94..000000000 --- a/tests/unit/data/test.ini +++ /dev/null @@ -1,14 +0,0 @@ -[databases] -test = host=test port=4039 dbname=testdatabase -test2 = host=test2 - -[pgbouncer] -logfile = test/logfile -pidfile = test/pidfile -admin_users = Test -stats_users = test_stats -listen_port = 4545 - -[users] -Test = pool_mode=session max_user_connections=10 - diff --git a/tests/unit/relations/test_backend_database.py b/tests/unit/relations/test_backend_database.py index bcddd80e5..df375b106 100644 --- a/tests/unit/relations/test_backend_database.py +++ b/tests/unit/relations/test_backend_database.py @@ -4,11 +4,12 @@ import unittest from unittest.mock import MagicMock, PropertyMock, call, patch -from charms.pgbouncer_k8s.v0.pgb import DEFAULT_CONFIG, PgbConfig, get_hashed_password +from charms.pgbouncer_k8s.v0.pgb import get_hashed_password +from ops.model import WaitingStatus from ops.testing import Harness from charm import PgBouncerCharm -from constants import BACKEND_RELATION_NAME, PEER_RELATION_NAME, PGB, PGB_CONF_DIR +from constants import BACKEND_RELATION_NAME, PEER_RELATION_NAME, PGB_CONF_DIR from tests.helpers import patch_network_get @@ -29,20 +30,23 @@ def setUp(self): self.harness.add_relation_unit(self.rel_id, "postgres/0") # Define a peer relation - self.peers_rel_id = self.harness.add_relation(PEER_RELATION_NAME, "pgbouncer/0") - self.harness.add_relation_unit(self.peers_rel_id, self.unit) + with patch("charm.PgBouncerCharm.render_pgb_config"): + self.peers_rel_id = self.harness.add_relation(PEER_RELATION_NAME, "pgbouncer/0") + self.harness.add_relation_unit(self.peers_rel_id, self.unit) + @patch("charm.PgBouncerCharm.check_pgb_running", return_value=True) @patch("charm.PgBouncerCharm.get_secret", return_value=None) @patch("charm.PgBouncerCharm.render_prometheus_service") + @patch("relations.peers.Peers.app_databag", new_callable=PropertyMock) @patch( - "relations.backend_database.BackendDatabaseRequires.auth_user", + "relations.backend_database.BackendDatabaseRequires.stats_user", new_callable=PropertyMock, - return_value="user", + return_value="stats_user", ) @patch( - "relations.backend_database.BackendDatabaseRequires.stats_user", + "relations.backend_database.BackendDatabaseRequires.auth_user", new_callable=PropertyMock, - return_value="stats_user", + return_value="user", ) @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock @@ -52,57 +56,113 @@ def setUp(self): ) @patch("charms.pgbouncer_k8s.v0.pgb.generate_password", return_value="pw") @patch("relations.backend_database.BackendDatabaseRequires.initialise_auth_function") - @patch("charm.PgBouncerCharm.render_file") - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=PgbConfig(DEFAULT_CONFIG)) - @patch("charm.PgBouncerCharm.update_postgres_endpoints") + @patch("charm.PgBouncerCharm.render_pgb_config") + @patch("charm.PgBouncerCharm.render_auth_file") def test_on_database_created( self, - _update_endpoints, - _cfg, - _render, + _render_auth_file, + _render_cfg_file, _init_auth, _gen_pw, _relation, _postgres, - _stats_user, _auth_user, + _stats_user, + _app_databag, _render_prometheus_service, _, + __, ): - self.harness.set_leader() + self.harness.set_leader(True) pw = _gen_pw.return_value postgres = _postgres.return_value - _relation = MagicMock() - _relation.data = {} - _relation.data[self.charm.app] = {} - _relation.data[self.charm.app]["database"] = PGB + _relation.return_value.data = {} + _relation.return_value.data[self.charm.app] = {"database": "database"} mock_event = MagicMock() mock_event.username = "mock_user" self.backend._on_database_created(mock_event) hash_pw = get_hashed_password(self.backend.auth_user, pw) - hash_mon_pw = get_hashed_password(self.backend.stats_user, pw) - - postgres.create_user.assert_called_once_with(self.backend.auth_user, hash_pw, admin=True) + postgres.create_user.assert_called_with(self.backend.auth_user, hash_pw, admin=True) _init_auth.assert_has_calls([call([self.backend.database.database, "postgres"])]) - _render.assert_any_call( - f"{PGB_CONF_DIR}/pgbouncer/userlist.txt", - f'"{self.backend.auth_user}" "{hash_pw}"\n"{self.backend.stats_user}" "{hash_mon_pw}"', - perms=0o700, + hash_mon_pw = get_hashed_password(self.backend.stats_user, pw) + _render_auth_file.assert_any_call( + f'"{self.backend.auth_user}" "{hash_pw}"\n"{self.backend.stats_user}" "{hash_mon_pw}"' ) - _render_prometheus_service.assert_called_once_with() - cfg = _cfg.return_value - assert self.backend.stats_user in cfg["pgbouncer"]["stats_users"] - assert ( - cfg["pgbouncer"]["auth_query"] - == f"SELECT username, password FROM {self.backend.auth_user}.get_auth($1)" - ) - assert cfg["pgbouncer"]["auth_file"] == f"{PGB_CONF_DIR}/pgbouncer/userlist.txt" + _render_prometheus_service.assert_called_with() + _render_cfg_file.assert_called_once_with(reload_pgbouncer=True) - _update_endpoints.assert_called_once() + @patch( + "relations.backend_database.BackendDatabaseRequires.auth_user", + new_callable=PropertyMock, + return_value=None, + ) + @patch("charm.PgBouncerCharm.render_prometheus_service") + @patch("charm.PgBouncerCharm.render_auth_file") + @patch("charm.PgBouncerCharm.render_pgb_config") + @patch("charm.PgBouncerCharm.get_secret") + def test_on_database_created_not_leader( + self, _get_secret, _render_pgb, _render_auth, _render_prometheus_service, _auth_user + ): + self.harness.set_leader(False) + + # No secret yet + _get_secret.return_value = None + + mock_event = MagicMock() + mock_event.username = "mock_user" + + self.backend._on_database_created(mock_event) + + assert not _render_auth.called + assert not _render_pgb.called + _get_secret.assert_called_once_with("app", "auth_file") + mock_event.defer.assert_called_once_with() + + # Stale secret + _get_secret.return_value = '"AUTH_USER"' + + self.backend._on_database_created(mock_event) + + assert not _render_auth.called + assert not _render_pgb.called + + # initialised leader + _auth_user.return_value = "AUTH_USER" + + self.backend._on_database_created(mock_event) + + _render_auth.assert_called_once_with('"AUTH_USER"') + _render_prometheus_service.assert_called_with() + _render_pgb.assert_called_once_with(reload_pgbouncer=True) + + @patch("charm.PgBouncerCharm.update_client_connection_info") + @patch("charm.PgBouncerCharm.render_pgb_config") + def test_on_endpoints_changed(self, _render_pgb, _update_client_conn): + self.harness.set_leader() + _update_client_conn.reset_mock() + + self.charm.backend._on_endpoints_changed(MagicMock()) + + _render_pgb.assert_called_once_with(reload_pgbouncer=True) + _update_client_conn.assert_called_once_with() + + @patch("charm.PgBouncerCharm.check_pgb_running", return_value=True) + @patch("charm.PgBouncerCharm.update_client_connection_info") + @patch("charm.PgBouncerCharm.render_pgb_config") + def test_on_relation_changed(self, _render_pgb, _update_client_conn, _): + self.harness.set_leader() + _update_client_conn.reset_mock() + + self.charm.backend._on_relation_changed(MagicMock()) + + _render_pgb.assert_called_once_with(reload_pgbouncer=True) + _update_client_conn.assert_called_once_with() + _render_pgb.reset_mock() + _update_client_conn.reset_mock() @patch_network_get(private_address="1.1.1.1") @patch( @@ -113,45 +173,21 @@ def test_on_database_created( @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) - @patch("charm.PgBouncerCharm.update_postgres_endpoints") - @patch("charm.PgBouncerCharm.update_client_connection_info") - @patch("relations.backend_database.BackendDatabaseRequires.remove_auth_function") - def test_relation_departed( - self, - _remove_auth, - _update_conn_info, - _update_endpoints, - _postgres, - _auth_user, - ): - self.harness.set_leader() + @patch("charm.PgBouncerCharm.render_pgb_config") + def test_relation_departed(self, _render, _postgres, _auth_user): + self.harness.set_leader(True) depart_event = MagicMock() - - depart_event.departing_unit.app = self.charm.app - self.backend._on_relation_departed(depart_event) - _update_endpoints.assert_called() - _update_endpoints.reset_mock() - _update_conn_info.assert_called() - _update_conn_info.reset_mock() - _remove_auth.assert_called() - _remove_auth.reset_mock() - _postgres().delete_user.assert_called() - - # Check departing when we're just scaling down this depart_event.departing_unit = self.charm.unit self.backend._on_relation_departed(depart_event) - _update_endpoints.assert_called() - _update_conn_info.assert_called() - _remove_auth.assert_not_called() + _render.assert_called_once_with(reload_pgbouncer=True) @patch("charm.PgBouncerCharm.remove_exporter_service") @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=PgbConfig(DEFAULT_CONFIG)) @patch("charm.PgBouncerCharm.render_pgb_config") @patch("charm.PgBouncerCharm.delete_file") - def test_relation_broken(self, _delete_file, _render, _cfg, _postgres, _remove_exporter): + def test_relation_broken(self, _delete_file, _render, _postgres, _remove_exporter): event = MagicMock() self.harness.set_leader() self.charm.peers.app_databag[ @@ -160,19 +196,11 @@ def test_relation_broken(self, _delete_file, _render, _cfg, _postgres, _remove_e postgres = _postgres.return_value postgres.user = "test_user" - cfg = _cfg.return_value - cfg.add_user(postgres.user, admin=True) - cfg["pgbouncer"]["stats_users"] = "test" - cfg["pgbouncer"]["auth_query"] = "test" self.backend._on_relation_broken(event) - assert "test_user" not in cfg["pgbouncer"] - assert "stats_users" not in cfg["pgbouncer"] - assert "auth_query" not in cfg["pgbouncer"] - - _render.assert_called_with(cfg) - _delete_file.assert_called_with(f"{PGB_CONF_DIR}/userlist.txt") + _render.assert_called_once_with(reload_pgbouncer=True) + _delete_file.assert_called_with(f"{PGB_CONF_DIR}/pgbouncer/userlist.txt") _remove_exporter.assert_called_once_with() @patch( @@ -196,3 +224,15 @@ def test_initialise_auth_function(self, _postgres, _auth_user): install_script.replace("auth_user", self.backend.auth_user) ) conn.close.assert_called() + + @patch( + "relations.backend_database.BackendDatabaseRequires.ready", + new_callable=PropertyMock, + return_value=True, + ) + def test_check_backend(self, _ready): + assert self.charm.backend.check_backend() + + _ready.return_value = False + assert not self.charm.backend.check_backend() + assert isinstance(self.charm.unit.status, WaitingStatus) diff --git a/tests/unit/relations/test_db.py b/tests/unit/relations/test_db.py index 6fc064290..be1d50171 100644 --- a/tests/unit/relations/test_db.py +++ b/tests/unit/relations/test_db.py @@ -2,9 +2,8 @@ # See LICENSE file for licensing details. import unittest -from unittest.mock import MagicMock, PropertyMock, patch +from unittest.mock import Mock, PropertyMock, patch -from ops.model import ActiveStatus from ops.testing import Harness from charm import PgBouncerCharm @@ -13,13 +12,8 @@ DB_ADMIN_RELATION_NAME, DB_RELATION_NAME, PEER_RELATION_NAME, - PG, -) -from lib.charms.pgbouncer_k8s.v0.pgb import ( - DEFAULT_CONFIG, - PgbConfig, - parse_dict_to_kv_string, ) +from lib.charms.pgbouncer_k8s.v0.pgb import parse_dict_to_kv_string from tests.helpers import patch_network_get @@ -50,8 +44,9 @@ def setUp(self): self.harness.add_relation_unit(self.db_admin_rel_id, "admin_client/0") # Define a peer relation - self.peers_rel_id = self.harness.add_relation(PEER_RELATION_NAME, "pgbouncer") - self.harness.add_relation_unit(self.peers_rel_id, self.unit) + with patch("charm.PgBouncerCharm.render_pgb_config"): + self.peers_rel_id = self.harness.add_relation(PEER_RELATION_NAME, "pgbouncer") + self.harness.add_relation_unit(self.peers_rel_id, self.unit) def test_correct_admin_perms_set_in_constructor(self): assert self.charm.legacy_db_relation.relation_name == "db" @@ -60,32 +55,34 @@ 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("relations.db.DbProvides._check_backend", return_value=True) + @patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True) @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=PgbConfig(DEFAULT_CONFIG)) @patch("charms.pgbouncer_k8s.v0.pgb.generate_password", return_value="test_pass") @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.initialise_auth_function") - @patch("charm.PgBouncerCharm.render_pgb_config") + @patch("charm.PgBouncerCharm.set_relation_databases") + @patch("charm.PgBouncerCharm.generate_relation_databases") def test_on_relation_joined( self, - _render_cfg, + _gen_rel_dbs, + _set_rel_dbs, _init_auth, _create_database, _create_user, _postgres, _gen_pw, - _read_cfg, _backend_pg, _check_backend, ): - self.harness.set_leader() + self.harness.set_leader(True) + + _gen_rel_dbs.return_value = {} - mock_event = MagicMock() + mock_event = Mock() mock_event.app.name = "external_test_app" mock_event.relation.id = 1 @@ -93,6 +90,7 @@ def test_on_relation_joined( user = "pgbouncer_user_1_None" password = _gen_pw.return_value + _set_rel_dbs.reset_mock() relation_data = mock_event.relation.data = {} relation_data[self.charm.unit] = {} relation_data[self.charm.app] = {} @@ -102,13 +100,13 @@ def test_on_relation_joined( _postgres.create_user = _create_user _postgres.create_database = _create_database + _set_rel_dbs.reset_mock() self.db_admin_relation._on_relation_joined(mock_event) + _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) _init_auth.assert_called_with([database]) - assert user in _read_cfg.return_value["pgbouncer"]["admin_users"] - _render_cfg.assert_called_with(_read_cfg.return_value, reload_pgbouncer=True) for dbag in [relation_data[self.charm.unit], relation_data[self.charm.app]]: assert dbag["database"] == database @@ -116,31 +114,33 @@ def test_on_relation_joined( assert dbag["password"] == password # Check admin permissions aren't present when we use db_relation + _set_rel_dbs.reset_mock() self.db_relation._on_relation_joined(mock_event) _create_user.assert_called_with(user, password, admin=False) + _set_rel_dbs.assert_called_once_with({"1": {"name": "test_db", "legacy": True}}) + @patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True) @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) - @patch("charm.PgBouncerCharm.check_status", return_value=ActiveStatus()) @patch("relations.db.DbProvides.get_databags", return_value=[{}]) @patch("relations.db.DbProvides.update_connection_info") - @patch("relations.db.DbProvides.update_postgres_endpoints") @patch("relations.db.DbProvides.update_databags") @patch("relations.db.DbProvides.get_allowed_units") @patch("relations.db.DbProvides.get_allowed_subnets") @patch("relations.db.DbProvides._get_state") + @patch("charm.PgBouncerCharm.render_pgb_config") def test_on_relation_changed( self, + _render_pgb_config, _get_state, _allowed_subnets, _allowed_units, _update_databags, - _update_postgres_endpoints, _update_connection_info, _get_databags, - _check_status, _backend_postgres, + _check_backend, ): self.harness.set_leader(True) @@ -154,32 +154,32 @@ def test_on_relation_changed( } # Call the function - event = MagicMock() + event = Mock() self.db_relation._on_relation_changed(event) _update_connection_info.assert_called_with( event.relation, self.charm.config["listen_port"] ) - _update_postgres_endpoints.assert_called_with(event.relation, reload_pgbouncer=True) _update_databags.assert_called_with( event.relation, { "allowed-subnets": _allowed_subnets.return_value, "allowed-units": _allowed_units.return_value, "version": self.charm.backend.postgres.get_postgresql_version(), - "host": self.charm.unit_ip, + "host": "localhost", "user": user, "password": password, "database": database, "state": _get_state.return_value, }, ) + _render_pgb_config.assert_called_once_with(reload_pgbouncer=True) @patch("relations.db.DbProvides.get_databags", return_value=[{}]) @patch("relations.db.DbProvides.get_external_app") @patch("relations.db.DbProvides.update_databags") def test_update_connection_info(self, _update_databags, _get_external_app, _get_databags): - relation = MagicMock() + relation = Mock() database = "test_db" user = "test_user" password = "test_pw" @@ -200,6 +200,12 @@ def test_update_connection_info(self, _update_databags, _get_external_app, _get_ "fallback_application_name": _get_external_app().name, } + standby_hostnames = self.charm.peers.units_ips - {self.charm.peers.leader_ip} + if len(standby_hostnames) > 0: + standby_hostname = standby_hostnames.pop() + standby_dbconnstr = dict(master_dbconnstr) + standby_dbconnstr.update({"host": standby_hostname, "dbname": f"{database}_standby"}) + self.db_relation.update_connection_info(relation, port) _update_databags.assert_called_with( relation, @@ -210,49 +216,10 @@ def test_update_connection_info(self, _update_databags, _get_external_app, _get_ }, ) - @patch("relations.db.DbProvides.get_databags", return_value=[{}]) - @patch( - "relations.backend_database.BackendDatabaseRequires.postgres_databag", - new_callable=PropertyMock, - return_value={}, - ) - @patch( - "relations.backend_database.BackendDatabaseRequires.get_read_only_endpoints", - return_value=[], - ) - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=PgbConfig(DEFAULT_CONFIG)) - @patch("charm.PgBouncerCharm.render_pgb_config") - def test_update_postgres_endpoints( - self, _render_cfg, _read_cfg, _read_only_endpoint, _pg_databag, _get_databags - ): - database = "test_db" - _get_databags.return_value[0] = {"database": database} - _pg_databag.return_value = {"endpoints": "ip:port"} - cfg = _read_cfg.return_value - - relation = MagicMock() - reload_pgbouncer = False - - self.db_relation.update_postgres_endpoints(relation, reload_pgbouncer) - assert database in cfg["databases"].keys() - assert f"{database}_standby" not in cfg["databases"].keys() - assert PG not in cfg["databases"].keys() - _render_cfg.assert_called_with(cfg, reload_pgbouncer=reload_pgbouncer) - _read_only_endpoint.return_value = ["readonly:endpoint"] - - self.db_relation.update_postgres_endpoints(relation, reload_pgbouncer) - assert database in cfg["databases"].keys() - assert f"{database}_standby" in cfg["databases"].keys() - assert PG not in cfg["databases"].keys() - - self.db_admin_relation.update_postgres_endpoints(relation, reload_pgbouncer) - assert database in cfg["databases"].keys() - assert f"{database}_standby" in cfg["databases"].keys() - assert PG in cfg["databases"].keys() - @patch("relations.db.DbProvides.get_allowed_units", return_value="test_string") def test_on_relation_departed(self, _get_units): - mock_event = MagicMock() + self.harness.set_leader(True) + mock_event = Mock() mock_event.relation.data = { self.charm.app: {"allowed-units": "app"}, self.charm.unit: {"allowed-units": "unit"}, @@ -262,49 +229,50 @@ def test_on_relation_departed(self, _get_units): app_databag = mock_event.relation.data[self.charm.app] unit_databag = mock_event.relation.data[self.charm.unit] - expected_app_databag = {"allowed-units": "app"} + expected_app_databag = {"allowed-units": "test_string"} expected_unit_databag = {"allowed-units": "test_string"} self.assertDictEqual(app_databag, expected_app_databag) self.assertDictEqual(unit_databag, expected_unit_databag) - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=PgbConfig(DEFAULT_CONFIG)) + @patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True) @patch("charms.postgresql_k8s.v0.postgresql.PostgreSQL") @patch("charms.postgresql_k8s.v0.postgresql.PostgreSQL.delete_user") @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) - @patch("charm.PgBouncerCharm.render_pgb_config") @patch("relations.backend_database.BackendDatabaseRequires.remove_auth_function") + @patch("charm.PgBouncerCharm.set_relation_databases") + @patch("charm.PgBouncerCharm.generate_relation_databases") def test_on_relation_broken( - self, _remove_auth, _render_cfg, _backend_postgres, _delete_user, _postgres, _read + self, + _gen_rel_dbs, + _set_rel_dbs, + _remove_auth, + _backend_postgres, + _delete_user, + _postgres, + _check_backend, ): - self.harness.set_leader() - """Test that all traces of the given app are removed from pgb config, including user.""" + _gen_rel_dbs.return_value = {"42": {"name": "test_db", "legacy": True}} + self.harness.set_leader(True) database = "test_db" username = "test_user" _backend_postgres.return_value = _postgres _postgres.delete_user = _delete_user - input_cfg = PgbConfig(DEFAULT_CONFIG) - input_cfg["databases"]["some_other_db"] = {"dbname": "pgb_postgres_standby_0"} - input_cfg["databases"][database] = {"dbname": f"{database}"} - input_cfg["databases"][f"{database}_standby"] = {"dbname": f"{database}"} - _read.return_value = input_cfg - - mock_event = MagicMock() - mock_databag = { + mock_event = Mock() + databag = { "user": username, "database": database, } + mock_event.relation.id = 42 mock_event.relation.data = {} - mock_event.relation.data[self.charm.app] = mock_databag - mock_event.relation.data[self.charm.unit] = dict(mock_databag) + mock_event.relation.data[self.charm.unit] = databag + mock_event.relation.data[self.charm.app] = databag self.charm.peers.app_databag[f"db-{mock_event.relation.id}-relation-breaking"] = "true" self.db_relation._on_relation_broken(mock_event) - _delete_user.assert_called_with(username) - - assert database not in [input_cfg["databases"]] - assert f"{database}_standby" not in [input_cfg["databases"]] + _delete_user.assert_called_once_with(username) + _set_rel_dbs.assert_called_once_with({}) diff --git a/tests/unit/relations/test_peers.py b/tests/unit/relations/test_peers.py index 41cae6e1a..64a8fcebd 100644 --- a/tests/unit/relations/test_peers.py +++ b/tests/unit/relations/test_peers.py @@ -7,9 +7,10 @@ from ops.testing import Harness from charm import PgBouncerCharm -from constants import BACKEND_RELATION_NAME, PEER_RELATION_NAME -from lib.charms.pgbouncer_k8s.v0.pgb import DEFAULT_CONFIG, PgbConfig -from relations.peers import AUTH_FILE_DATABAG_KEY, CFG_FILE_DATABAG_KEY +from constants import ( + BACKEND_RELATION_NAME, + PEER_RELATION_NAME, +) from tests.helpers import patch_network_get @@ -31,39 +32,6 @@ def setUp(self): @patch("charm.PgBouncerCharm.reload_pgbouncer") def test_on_peers_changed(self, reload_pgbouncer, render_auth_file, render_pgb_config): self.harness.add_relation(BACKEND_RELATION_NAME, "postgres") - # We don't want to write anything if we're the leader - self.harness.set_leader(True) self.charm.peers._on_changed(MagicMock()) - render_auth_file.assert_not_called() - render_pgb_config.assert_not_called() - reload_pgbouncer.assert_not_called() - - # Don't write anything if nothing is available to write - self.harness.set_leader(False) - self.charm.peers._on_changed(MagicMock()) - render_pgb_config.assert_not_called() - render_auth_file.assert_not_called() - reload_pgbouncer.assert_not_called() - - # Assert that we're reloading pgb even if we're only changing one thing - with self.harness.hooks_disabled(): - self.harness.update_relation_data( - self.rel_id, - self.charm.app.name, - {CFG_FILE_DATABAG_KEY: PgbConfig(DEFAULT_CONFIG).render()}, - ) - self.charm.peers._on_changed(MagicMock()) - render_pgb_config.assert_called_once() - render_auth_file.assert_not_called() - reload_pgbouncer.assert_called_once() + render_pgb_config.assert_called_once_with(reload_pgbouncer=True) render_pgb_config.reset_mock() - reload_pgbouncer.reset_mock() - - with self.harness.hooks_disabled(): - self.harness.update_relation_data( - self.rel_id, self.charm.app.name, {AUTH_FILE_DATABAG_KEY: '"user" "pass"'} - ) - self.charm.peers._on_changed(MagicMock()) - render_pgb_config.assert_called_once() - render_auth_file.assert_called_once() - reload_pgbouncer.assert_called_once() diff --git a/tests/unit/relations/test_pgbouncer_provider.py b/tests/unit/relations/test_pgbouncer_provider.py index 7bb7d6d31..b9fee6287 100644 --- a/tests/unit/relations/test_pgbouncer_provider.py +++ b/tests/unit/relations/test_pgbouncer_provider.py @@ -8,7 +8,6 @@ from charm import PgBouncerCharm from constants import BACKEND_RELATION_NAME, CLIENT_RELATION_NAME, PEER_RELATION_NAME -from lib.charms.pgbouncer_k8s.v0.pgb import DEFAULT_CONFIG, PgbConfig from tests.helpers import patch_network_get @@ -26,8 +25,9 @@ def setUp(self): self.client_relation = self.charm.client_relation # Define a peer relation - self.peers_rel_id = self.harness.add_relation(PEER_RELATION_NAME, "pgbouncer") - self.harness.add_relation_unit(self.peers_rel_id, self.unit) + with patch("charm.PgBouncerCharm.render_pgb_config"): + self.peers_rel_id = self.harness.add_relation(PEER_RELATION_NAME, "pgbouncer") + self.harness.add_relation_unit(self.peers_rel_id, self.unit) # Define a backend relation self.backend_rel_id = self.harness.add_relation(BACKEND_RELATION_NAME, "postgres") @@ -37,7 +37,7 @@ def setUp(self): self.client_rel_id = self.harness.add_relation(CLIENT_RELATION_NAME, "application") self.harness.add_relation_unit(self.client_rel_id, "application/0") - @patch("relations.pgbouncer_provider.PgBouncerProvider._check_backend") + @patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True) @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) @@ -52,20 +52,22 @@ def setUp(self): return_value="test_auth_user", ) @patch("charms.pgbouncer_k8s.v0.pgb.generate_password", return_value="test_pass") + @patch("relations.pgbouncer_provider.PgBouncerProvider.update_read_only_endpoints") @patch("relations.pgbouncer_provider.PgBouncerProvider.get_database", return_value="test-db") @patch("charms.data_platform_libs.v0.data_interfaces.DatabaseProvides.set_credentials") @patch("charms.data_platform_libs.v0.data_interfaces.DatabaseProvides.set_endpoints") @patch("charms.data_platform_libs.v0.data_interfaces.DatabaseProvides.set_version") - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=PgbConfig(DEFAULT_CONFIG)) - @patch("charm.PgBouncerCharm.render_pgb_config") + @patch("charm.PgBouncerCharm.set_relation_databases") + @patch("charm.PgBouncerCharm.generate_relation_databases") def test_on_database_requested( self, - _render_cfg, - _cfg, + _gen_rel_dbs, + _set_rel_dbs, _dbp_set_version, _dbp_set_endpoints, _dbp_set_credentials, _get_database, + _update_read_only_endpoints, _password, _auth_user, _pg_databag, @@ -73,6 +75,7 @@ def test_on_database_requested( _check_backend, ): self.harness.set_leader() + _gen_rel_dbs.return_value = {} event = MagicMock() rel_id = event.relation.id = 1 @@ -98,89 +101,33 @@ def test_on_database_requested( _dbp_set_endpoints.assert_called_with( rel_id, f"localhost:{self.charm.config['listen_port']}" ) - _render_cfg.assert_called_with(_cfg(), reload_pgbouncer=True) - - # Verify config contains what we want - postgres_endpoint = self.charm.backend.postgres_databag.get("endpoints") - assert _cfg()["databases"][database] == { - "host": postgres_endpoint.split(":")[0], - "dbname": database, - "port": postgres_endpoint.split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - assert not _cfg()["databases"].get(f"{database}_readonly") - - # test cfg with scaled pg - _pg_databag.return_value["read-only-endpoints"] = "r_test:endpoint" - self.client_relation._on_database_requested(event) - postgres_endpoint = self.charm.backend.postgres_databag.get("endpoints") - assert _cfg()["databases"][database] == { - "host": postgres_endpoint.split(":")[0], - "dbname": database, - "port": postgres_endpoint.split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - read_only_endpoints = self.charm.backend.get_read_only_endpoints() - r_hosts = ",".join([host.split(":")[0] for host in read_only_endpoints]) - assert _cfg()["databases"][f"{database}_readonly"] == { - "host": r_hosts, - "dbname": database, - "port": next(iter(read_only_endpoints)).split(":")[1], - "auth_user": self.charm.backend.auth_user, - } - - @patch("relations.pgbouncer_provider.PgBouncerProvider._check_backend") + _set_rel_dbs.assert_called_once_with({"1": {"name": "test-db", "legacy": False}}) + + @patch("relations.backend_database.BackendDatabaseRequires.check_backend", return_value=True) @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=PgbConfig(DEFAULT_CONFIG)) - @patch("charm.PgBouncerCharm.render_pgb_config") - def test_on_relation_broken(self, _render_cfg, _cfg, _pg, _check_backend): + @patch("charm.PgBouncerCharm.set_relation_databases") + @patch("charm.PgBouncerCharm.generate_relation_databases") + def test_on_relation_broken(self, _gen_rel_dbs, _set_rel_dbs, _pg, _check_backend): _pg.return_value.get_postgresql_version.return_value = "10" + _gen_rel_dbs.return_value = {"1": {"name": "test_db", "legacy": False}} self.harness.set_leader() event = MagicMock() rel_id = event.relation.id = 1 - self.charm.peers.app_databag[f"database-{event.relation.id}-relation-breaking"] = "true" external_app = self.charm.client_relation.get_external_app(event.relation) event.relation.data = {external_app: {"database": "test_db"}} - database = event.relation.data[external_app]["database"] user = f"relation_id_{rel_id}" - _cfg.return_value["databases"][database] = { - "host": "host", - "dbname": database, - "port": "1111", - "auth_user": self.charm.backend.auth_user, - } - _cfg.return_value["databases"][f"{database}_readonly"] = { - "host": "host2", - "dbname": database, - "port": "1111", - "auth_user": self.charm.backend.auth_user, - } - _cfg.return_value["pgbouncer"]["admin_users"].add(user) - _cfg.return_value["pgbouncer"]["stats_users"].add(user) - - _check_backend.return_value = True - self.charm.peers.app_databag[f"database-{event.relation.id}-relation-breaking"] = "true" self.client_relation._on_relation_broken(event) - _cfg.assert_called() - assert user not in _cfg()["pgbouncer"]["admin_users"] - assert user not in _cfg()["pgbouncer"]["stats_users"] - assert not _cfg()["databases"].get(database) - assert not _cfg()["databases"].get(f"{database}_readonly") - _render_cfg.assert_called_with(_cfg(), reload_pgbouncer=True) _pg().delete_user.assert_called_with(user) - # Test again without readonly node - _cfg.return_value["databases"][database] = { - "host": "host", - "dbname": database, - "port": "1111", - "auth_user": self.charm.backend.auth_user, - } - self.charm.peers.app_databag[f"database-{event.relation.id}-relation-breaking"] = "true" - self.client_relation._on_relation_broken(event) - assert not _cfg()["databases"].get(database) - assert not _cfg()["databases"].get(f"{database}_readonly") + _set_rel_dbs.assert_called_once_with({}) + + @patch("charms.data_platform_libs.v0.data_interfaces.DatabaseProvides.fetch_relation_field") + @patch("charms.data_platform_libs.v0.data_interfaces.DatabaseProvides.set_read_only_endpoints") + def test_update_read_only_endpoints(self, _set_read_only_endpoints, _fetch_relation_field): + self.harness.set_leader() + self.client_relation.update_read_only_endpoints() + _set_read_only_endpoints.assert_called() diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index bc3d069ba..82d6f59c5 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -2,21 +2,20 @@ # See LICENSE file for licensing details. import logging +import math import os import platform import unittest -from copy import deepcopy -from unittest.mock import MagicMock, PropertyMock, call, patch +from unittest.mock import MagicMock, Mock, PropertyMock, call, patch import ops.testing import pytest from charms.operator_libs_linux.v1 import systemd from charms.operator_libs_linux.v2 import snap -from charms.pgbouncer_k8s.v0 import pgb +from jinja2 import Template from ops.model import ( ActiveStatus, BlockedStatus, - MaintenanceStatus, RelationDataTypeError, WaitingStatus, ) @@ -25,8 +24,8 @@ from charm import PgBouncerCharm from constants import ( + AUTH_FILE_NAME, BACKEND_RELATION_NAME, - INI_NAME, PEER_RELATION_NAME, PGB_CONF_DIR, PGB_LOG_DIR, @@ -37,7 +36,6 @@ DATA_DIR = "tests/unit/data" TEST_VALID_INI = f"{DATA_DIR}/test.ini" -DEFAULT_CFG = pgb.DEFAULT_CONFIG ops.testing.SIMULATE_CAN_CONNECT = True @@ -72,7 +70,7 @@ def test_on_install( self, _reload, _copy, - _render_configs, + _render_config, _render_file, _getpwnam, _chown, @@ -93,10 +91,7 @@ def test_on_install( _chown.assert_any_call(f"{PGB_CONF_DIR}/pgbouncer/instance_{service_id}", 1100, 120) # Check config files are rendered, including correct permissions - initial_cfg = pgb.PgbConfig(DEFAULT_CFG) - initial_cfg["pgbouncer"]["listen_addr"] = "127.0.0.1" - initial_cfg["pgbouncer"]["user"] = "snap_daemon" - _render_configs.assert_called_once_with(initial_cfg) + _render_config.assert_called_once_with() pg_snap.alias.assert_called_once_with("psql") self.assertIsInstance(self.harness.model.unit.status, WaitingStatus) @@ -106,15 +101,15 @@ def test_on_install( new_callable=PropertyMock, return_value=True, ) - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=pgb.PgbConfig(DEFAULT_CFG)) @patch("charm.systemd.service_running") + @patch("charm.PgBouncerCharm.render_prometheus_service") @patch("charms.operator_libs_linux.v1.systemd.service_start", side_effect=systemd.SystemdError) @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock, return_value=None, ) - def test_on_start(self, _has_relation, _start, _, __, ___): + def test_on_start(self, _has_relation, _start, _render_prom_service, _, __): intended_instances = self._cores = os.cpu_count() # Testing charm blocks when systemd is in error self.charm.on.start.emit() @@ -138,79 +133,67 @@ def test_on_start(self, _has_relation, _start, _, __, ___): self.charm.on.start.emit() calls = [call(f"pgbouncer-pgbouncer@{instance}") for instance in range(intended_instances)] _start.assert_has_calls(calls) + _render_prom_service.assert_called_once_with() self.assertIsInstance(self.harness.model.unit.status, ActiveStatus) @patch("charms.operator_libs_linux.v1.systemd.service_restart") - @patch("charm.PgBouncerCharm.check_status", return_value=BlockedStatus()) - def test_reload_pgbouncer(self, _running, _restart): + @patch("charm.PgBouncerCharm.check_pgb_running") + def test_reload_pgbouncer(self, _check_pgb_running, _restart): intended_instances = self._cores = os.cpu_count() self.charm.reload_pgbouncer() calls = [call(f"pgbouncer-pgbouncer@{instance}") for instance in range(intended_instances)] _restart.assert_has_calls(calls) - _running.assert_called_once() + _check_pgb_running.assert_called_once() # Verify that if systemd is in error, the charm enters blocked status. _restart.side_effect = systemd.SystemdError() self.charm.reload_pgbouncer() self.assertIsInstance(self.harness.model.unit.status, BlockedStatus) - @patch("charm.PgBouncerCharm.read_pgb_config", side_effect=FileNotFoundError) @patch("charms.operator_libs_linux.v1.systemd.service_running", return_value=False) @patch( "relations.backend_database.BackendDatabaseRequires.ready", new_callable=PropertyMock, return_value=False, ) - def test_check_status(self, _postgres_ready, _running, _read_cfg): - # check fail on config not available - self.assertIsInstance(self.charm.check_status(), WaitingStatus) - _read_cfg.side_effect = None - + def test_check_pgb_running(self, _postgres_ready, _running): # check fail on postgres not available # Testing charm blocks when the pgbouncer services aren't running - self.assertIsInstance(self.charm.check_status(), BlockedStatus) + assert not self.charm.check_pgb_running() + self.assertIsInstance(self.charm.unit.status, BlockedStatus) _postgres_ready.return_value = True # check fail when services aren't all running - self.assertIsInstance(self.charm.check_status(), BlockedStatus) + assert not self.charm.check_pgb_running() + self.assertIsInstance(self.charm.unit.status, BlockedStatus) calls = [call("pgbouncer-pgbouncer@0")] _running.assert_has_calls(calls) _running.return_value = True # check fail when we can't get service status _running.side_effect = systemd.SystemdError - self.assertIsInstance(self.charm.check_status(), BlockedStatus) + assert not self.charm.check_pgb_running() + self.assertIsInstance(self.charm.unit.status, BlockedStatus) _running.side_effect = None # otherwise check all services and return activestatus intended_instances = self._cores = os.cpu_count() - self.assertIsInstance(self.charm.check_status(), ActiveStatus) + assert self.charm.check_pgb_running() calls = [ call(f"pgbouncer-pgbouncer@{instance}") for instance in range(0, intended_instances) ] _running.assert_has_calls(calls) - @patch("charm.PgBouncerCharm.read_pgb_config", return_value=pgb.PgbConfig(DEFAULT_CFG)) @patch("charm.PgBouncerCharm.render_pgb_config") @patch("relations.peers.Peers.app_databag", new_callable=PropertyMock) @patch_network_get(private_address="1.1.1.1") - def test_on_config_changed(self, _app_databag, _render, _read): + def test_on_config_changed(self, _app_databag, _render): self.harness.add_relation(BACKEND_RELATION_NAME, "postgres") self.harness.set_leader() mock_cores = 1 self.charm._cores = mock_cores max_db_connections = 44 - # Copy config object and modify it as we expect in the hook. - test_config = deepcopy(_read.return_value) - test_config["pgbouncer"]["pool_mode"] = "transaction" - test_config.set_max_db_connection_derivatives( - max_db_connections=max_db_connections, - pgb_instances=mock_cores, - ) - - test_config["pgbouncer"]["listen_port"] = 6464 - # set config to include pool_mode and max_db_connections self.harness.update_config({ "pool_mode": "transaction", @@ -218,10 +201,8 @@ def test_on_config_changed(self, _app_databag, _render, _read): "listen_port": 6464, }) - _read.assert_called_once() # _read.return_value is modified on config update, but the object reference is the same. - _render.assert_called_with(_read.return_value, reload_pgbouncer=True) - self.assertDictEqual(dict(_read.return_value), dict(test_config)) + _render.assert_called_with(reload_pgbouncer=True) @patch("charm.snap.SnapCache") def test_install_snap_packages(self, _snap_cache): @@ -324,50 +305,170 @@ def test_render_file(self, _getpwnam, _chown, _chmod): _getpwnam.assert_called_with("snap_daemon") _chown.assert_called_with(path, uid=1100, gid=120) - def test_read_pgb_config(self): - with open(TEST_VALID_INI, "r") as ini: - test_ini = ini.read() - existing_config = pgb.PgbConfig(test_ini) - - with patch("builtins.open", unittest.mock.mock_open(read_data=test_ini)): - test_config = self.charm.read_pgb_config() - - self.assertEqual(test_ini, test_config.render()) - self.assertEqual(existing_config, test_config) - + @patch( + "relations.backend_database.DatabaseRequires.fetch_relation_field", + return_value="BACKNEND_USER", + ) + @patch( + "charm.BackendDatabaseRequires.relation", new_callable=PropertyMock, return_value=Mock() + ) + @patch( + "charm.BackendDatabaseRequires.postgres_databag", + new_callable=PropertyMock, + return_value={"endpoints": "HOST:PORT", "read-only-endpoints": "HOST2:PORT"}, + ) + @patch("charm.PgBouncerCharm.get_relation_databases") @patch("charm.PgBouncerCharm.reload_pgbouncer") @patch("charm.PgBouncerCharm.render_file") - def test_render_pgb_config(self, _render, _reload): - self.charm.service_ids = [0, 1] - default_cfg = pgb.PgbConfig(DEFAULT_CFG) - cfg_list = [default_cfg.render()] - - for service_id in self.charm.service_ids: - cfg = pgb.PgbConfig(DEFAULT_CFG) - cfg["pgbouncer"]["unix_socket_dir"] = f"/tmp/pgbouncer/instance_{service_id}" - cfg["pgbouncer"]["logfile"] = ( - f"{PGB_LOG_DIR}/pgbouncer/instance_{service_id}/pgbouncer.log" + def test_render_pgb_config( + self, + _render, + _reload, + _get_dbs, + _postgres_databag, + _backend_rel, + _, + ): + _get_dbs.return_value = { + "1": {"name": "first_test", "legacy": True}, + "2": {"name": "second_test", "legacy": False}, + } + + with open("templates/pgb_config.j2") as file: + template = Template(file.read()) + self.charm.render_pgb_config(reload_pgbouncer=True) + _reload.assert_called() + effective_db_connections = 100 / self.charm._cores + default_pool_size = math.ceil(effective_db_connections / 2) + min_pool_size = math.ceil(effective_db_connections / 4) + reserve_pool_size = math.ceil(effective_db_connections / 4) + auth_file = f"{PGB_CONF_DIR}/pgbouncer/{AUTH_FILE_NAME}" + + expected_databases = { + "first_test": { + "host": "HOST", + "dbname": "first_test", + "port": "PORT", + "auth_user": "pgbouncer_auth_BACKNEND_USER", + }, + "first_test_readonly": { + "host": "HOST2", + "dbname": "first_test", + "port": "PORT", + "auth_user": "pgbouncer_auth_BACKNEND_USER", + }, + "second_test": { + "host": "HOST", + "dbname": "second_test", + "port": "PORT", + "auth_user": "pgbouncer_auth_BACKNEND_USER", + }, + "second_test_readonly": { + "host": "HOST2", + "dbname": "second_test", + "port": "PORT", + "auth_user": "pgbouncer_auth_BACKNEND_USER", + }, + } + for i in range(self.charm._cores): + expected_content = template.render( + databases=expected_databases, + socket_dir=f"/tmp/pgbouncer/instance_{i}", + log_file=f"{PGB_LOG_DIR}/pgbouncer/instance_{i}/pgbouncer.log", + pid_file=f"/tmp/pgbouncer/instance_{i}/pgbouncer.pid", + listen_addr="127.0.0.1", + listen_port=6432, + pool_mode="session", + max_db_connections=100, + default_pool_size=default_pool_size, + min_pool_size=min_pool_size, + reserve_pool_size=reserve_pool_size, + stats_user="pgbouncer_stats_pgbouncer", + auth_query="SELECT username, password FROM pgbouncer_auth_BACKNEND_USER.get_auth($1)", + auth_file=auth_file, + enable_tls=False, ) - cfg["pgbouncer"]["pidfile"] = f"/tmp/pgbouncer/instance_{service_id}/pgbouncer.pid" - - cfg_list.append(cfg.render()) - - self.charm.render_pgb_config(default_cfg, reload_pgbouncer=False) - - _render.assert_any_call(f"{PGB_CONF_DIR}/pgbouncer/{INI_NAME}", cfg_list[0], 0o700) - _render.assert_any_call( - f"{PGB_CONF_DIR}/pgbouncer/instance_0/pgbouncer.ini", cfg_list[1], 0o700 - ) - _render.assert_any_call( - f"{PGB_CONF_DIR}/pgbouncer/instance_1/pgbouncer.ini", cfg_list[2], 0o700 - ) + _render.assert_any_call( + f"{PGB_CONF_DIR}/pgbouncer/instance_{i}/pgbouncer.ini", expected_content, 0o700 + ) + _render.reset_mock() + _reload.reset_mock() - _reload.assert_not_called() - # MaintenanceStatus will exit once pgbouncer reloads. - self.assertIsInstance(self.harness.model.unit.status, MaintenanceStatus) + # test constant pool sizes with unlimited connections and no ro endpoints + with self.harness.hooks_disabled(): + self.harness.update_config({ + "max_db_connections": 0, + }) + del expected_databases["first_test_readonly"] + del expected_databases["second_test_readonly"] + + del _postgres_databag.return_value["read-only-endpoints"] + + self.charm.render_pgb_config() + + assert not _reload.called + for i in range(self.charm._cores): + expected_content = template.render( + databases=expected_databases, + socket_dir=f"/tmp/pgbouncer/instance_{i}", + log_file=f"{PGB_LOG_DIR}/pgbouncer/instance_{i}/pgbouncer.log", + pid_file=f"/tmp/pgbouncer/instance_{i}/pgbouncer.pid", + listen_addr="127.0.0.1", + listen_port=6432, + pool_mode="session", + max_db_connections=0, + default_pool_size=20, + min_pool_size=10, + reserve_pool_size=10, + stats_user="pgbouncer_stats_pgbouncer", + auth_query="SELECT username, password FROM pgbouncer_auth_BACKNEND_USER.get_auth($1)", + auth_file=auth_file, + enable_tls=False, + ) + _render.assert_any_call( + f"{PGB_CONF_DIR}/pgbouncer/instance_{i}/pgbouncer.ini", expected_content, 0o700 + ) - self.charm.render_pgb_config(cfg, reload_pgbouncer=True) - _reload.assert_called_once() + @patch("charm.Peers.app_databag", new_callable=PropertyMock, return_value={}) + @patch("charm.PgBouncerCharm.get_secret") + def test_get_relation_databases_legacy_data(self, _get_secret, _): + """Test that legacy data will be parsed if new one is not set.""" + self.harness.set_leader(False) + _get_secret.return_value = """ + [databases] + test_db = host_cfg + test_db_standby = host_cfg + other_db = other_cfg + """ + result = self.charm.get_relation_databases() + assert result == { + "1": {"legacy": False, "name": "test_db"}, + "2": {"legacy": False, "name": "other_db"}, + } + _get_secret.assert_called_once_with("app", "cfg_file") + + # Get empty dict if no config is set + _get_secret.return_value = None + assert self.charm.get_relation_databases() == {} + + # Get empty dict if exception + _get_secret.return_value = 1 + assert self.charm.get_relation_databases() == {} + + # Get empty dict if no databases + _get_secret.return_value = """ + [other] + test_db = host_cfg + test_db_standby = host_cfg + other_db = other_cfg + """ + assert self.charm.get_relation_databases() == {} + + @patch("charm.PgBouncerCharm.get_relation_databases", return_value={"some": "values"}) + def test_generate_relation_databases_not_leader(self, _): + self.harness.set_leader(False) + + assert self.charm.generate_relation_databases() == {} # # Secrets diff --git a/tests/unit/test_upgrade.py b/tests/unit/test_upgrade.py index 9ce7ee659..7e160531a 100644 --- a/tests/unit/test_upgrade.py +++ b/tests/unit/test_upgrade.py @@ -6,7 +6,6 @@ import pytest import tenacity from charms.data_platform_libs.v0.upgrade import ClusterNotReadyError -from ops.model import ActiveStatus, MaintenanceStatus from ops.testing import Harness from charm import PgBouncerCharm @@ -42,9 +41,11 @@ def test_log_rollback_instructions(self, _logger: Mock): @patch("charm.PgBouncerCharm.get_secret") @patch("charm.BackendDatabaseRequires.postgres", return_value=True, new_callable=PropertyMock) + @patch("charm.PgBouncerCharm.update_status") @patch("charm.PgbouncerUpgrade.on_upgrade_changed") @patch("charm.PgbouncerUpgrade.set_unit_completed") @patch("charm.PgbouncerUpgrade._cluster_checks") + @patch("charm.PgBouncerCharm.generate_relation_databases") @patch("charm.PgBouncerCharm.render_prometheus_service") @patch("charm.PgBouncerCharm.reload_pgbouncer") @patch("charm.PgBouncerCharm.render_utility_files") @@ -59,9 +60,11 @@ def test_on_upgrade_granted( _render_utility_files: Mock, _reload_pgbouncer: Mock, _render_prometheus_service: Mock, + _generate_relation_databases: Mock, _cluster_checks: Mock, _set_unit_completed: Mock, _on_upgrade_changed: Mock, + _update_status: Mock, _, __, ): @@ -75,8 +78,11 @@ def test_on_upgrade_granted( _remove_exporter_service.assert_called_once_with() _install_snap_packages.assert_called_once_with(packages=SNAP_PACKAGES, refresh=True) _render_prometheus_service.assert_called_once_with() + _render_utility_files.assert_called_once_with() _cluster_checks.assert_called_once_with() _set_unit_completed.assert_called_once_with() + _update_status.assert_called_once_with() + assert not _generate_relation_databases.called # Test extra call as leader with self.harness.hooks_disabled(): @@ -85,6 +91,7 @@ def test_on_upgrade_granted( self.charm.upgrade._on_upgrade_granted(event) _on_upgrade_changed.assert_called_once_with(event) + _generate_relation_databases.assert_called_once_with() @patch("charm.PgBouncerCharm.get_secret") @patch("upgrade.wait_fixed", return_value=tenacity.wait_fixed(0)) @@ -118,25 +125,25 @@ def test_on_upgrade_granted_error( with pytest.raises(ClusterNotReadyError): self.charm.upgrade._on_upgrade_granted(Mock()) - @patch("charm.PgBouncerCharm.check_status", return_value=ActiveStatus()) - def test_pre_upgrade_check(self, _check_status: Mock): + @patch("charm.PgBouncerCharm.check_pgb_running", return_value=True) + def test_pre_upgrade_check(self, _check_pgb_running: Mock): self.charm.upgrade.pre_upgrade_check() - _check_status.assert_called_once_with() + _check_pgb_running.assert_called_once_with() - @patch("charm.PgBouncerCharm.check_status", return_value=MaintenanceStatus()) - def test_pre_upgrade_check_not_ready(self, _check_status: Mock): + @patch("charm.PgBouncerCharm.check_pgb_running", return_value=False) + def test_pre_upgrade_check_not_ready(self, _check_pgb_running: Mock): with pytest.raises(ClusterNotReadyError): self.charm.upgrade.pre_upgrade_check() - _check_status.assert_called_once_with() + _check_pgb_running.assert_called_once_with() @patch("charm.BackendDatabaseRequires.postgres", return_value=True, new_callable=PropertyMock) @patch("charm.BackendDatabaseRequires.ready", return_value=False, new_callable=PropertyMock) - @patch("charm.PgBouncerCharm.check_status", return_value=ActiveStatus()) - def test_pre_upgrade_check_backend_not_ready(self, _check_status: Mock, _, __): + @patch("charm.PgBouncerCharm.check_pgb_running", return_value=True) + def test_pre_upgrade_check_backend_not_ready(self, _check_pgb_running: Mock, _, __): print(self.charm.backend.ready) with pytest.raises(ClusterNotReadyError): self.charm.upgrade.pre_upgrade_check() - _check_status.assert_called_once_with() + _check_pgb_running.assert_called_once_with()