From c608b5568475b43226c94e1559c757f94c6eb35c Mon Sep 17 00:00:00 2001 From: Dragomir Penev <6687393+dragomirp@users.noreply.github.com> Date: Tue, 6 Feb 2024 16:18:58 +0200 Subject: [PATCH] [DPE-3184] Update secrets implementation (#145) * Initial switch to new secrets implementation * Unit tests * PeerData handler --- .../data_platform_libs/v0/data_interfaces.py | 130 +++++---- lib/charms/postgresql_k8s/v0/postgresql.py | 22 +- poetry.lock | 17 +- pyproject.toml | 13 +- src/charm.py | 227 ++++----------- src/constants.py | 1 - src/relations/backend_database.py | 1 - src/relations/db.py | 13 +- src/relations/peers.py | 14 - src/relations/pgbouncer_provider.py | 2 - tests/unit/relations/test_backend_database.py | 3 - tests/unit/relations/test_peers.py | 28 +- tests/unit/test_charm.py | 267 ++++++++++++++---- 13 files changed, 408 insertions(+), 330 deletions(-) diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index 45d57fefd..c940cc009 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -320,7 +320,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 25 +LIBPATCH = 27 PYDEPS = ["ops>=2.0.0"] @@ -422,15 +422,15 @@ def diff(event: RelationChangedEvent, bucket: Union[Unit, Application]) -> Diff: ) # These are the keys that were added to the databag and triggered this event. - added = new_data.keys() - old_data.keys() # pyright: ignore [reportGeneralTypeIssues] + added = new_data.keys() - old_data.keys() # pyright: ignore [reportAssignmentType] # These are the keys that were removed from the databag and triggered this event. - deleted = old_data.keys() - new_data.keys() # pyright: ignore [reportGeneralTypeIssues] + deleted = old_data.keys() - new_data.keys() # pyright: ignore [reportAssignmentType] # These are the keys that already existed in the databag, # but had their values changed. changed = { key - for key in old_data.keys() & new_data.keys() # pyright: ignore [reportGeneralTypeIssues] - if old_data[key] != new_data[key] # pyright: ignore [reportGeneralTypeIssues] + for key in old_data.keys() & new_data.keys() # pyright: ignore [reportAssignmentType] + if old_data[key] != new_data[key] # pyright: ignore [reportAssignmentType] } # Convert the new_data to a serializable format and save it for a next diff check. set_encoded_field(event.relation, bucket, "data", new_data) @@ -477,12 +477,19 @@ class CachedSecret: The data structure is precisely re-using/simulating as in the actual Secret Storage """ - def __init__(self, charm: CharmBase, label: str, secret_uri: Optional[str] = None): + def __init__( + self, + charm: CharmBase, + component: Union[Application, Unit], + label: str, + secret_uri: Optional[str] = None, + ): self._secret_meta = None self._secret_content = {} self._secret_uri = secret_uri self.label = label self.charm = charm + self.component = component def add_secret(self, content: Dict[str, str], relation: Relation) -> Secret: """Create a new secret.""" @@ -491,7 +498,7 @@ def add_secret(self, content: Dict[str, str], relation: Relation) -> Secret: "Secret is already defined with uri %s", self._secret_uri ) - secret = self.charm.app.add_secret(content, label=self.label) + secret = self.component.add_secret(content, label=self.label) if relation.app != self.charm.app: # If it's not a peer relation, grant is to be applied secret.grant(relation) @@ -523,8 +530,13 @@ def get_content(self) -> Dict[str, str]: except (ValueError, ModelError) as err: # https://bugs.launchpad.net/juju/+bug/2042596 # Only triggered when 'refresh' is set - msg = "ERROR either URI or label should be used for getting an owned secret but not both" - if isinstance(err, ModelError) and msg not in str(err): + known_model_errors = [ + "ERROR either URI or label should be used for getting an owned secret but not both", + "ERROR secret owner cannot use --refresh", + ] + if isinstance(err, ModelError) and not any( + msg in str(err) for msg in known_model_errors + ): raise # Due to: ValueError: Secret owner cannot use refresh=True self._secret_content = self.meta.get_content() @@ -550,14 +562,15 @@ def get_info(self) -> Optional[SecretInfo]: class SecretCache: """A data structure storing CachedSecret objects.""" - def __init__(self, charm): + def __init__(self, charm: CharmBase, component: Union[Application, Unit]): self.charm = charm + self.component = component self._secrets: Dict[str, CachedSecret] = {} def get(self, label: str, uri: Optional[str] = None) -> Optional[CachedSecret]: """Getting a secret from Juju Secret store or cache.""" if not self._secrets.get(label): - secret = CachedSecret(self.charm, label, uri) + secret = CachedSecret(self.charm, self.component, label, uri) if secret.meta: self._secrets[label] = secret return self._secrets.get(label) @@ -567,7 +580,7 @@ def add(self, label: str, content: Dict[str, str], relation: Relation) -> Cached if self._secrets.get(label): raise SecretAlreadyExistsError(f"Secret {label} already exists") - secret = CachedSecret(self.charm, label) + secret = CachedSecret(self.charm, self.component, label) secret.add_secret(content, relation) self._secrets[label] = secret return self._secrets[label] @@ -579,6 +592,8 @@ def add(self, label: str, content: Dict[str, str], relation: Relation) -> Cached class DataRelation(Object, ABC): """Base relation data mainpulation (abstract) class.""" + SCOPE = Scope.APP + # Local map to associate mappings with secrets potentially as a group SECRET_LABEL_MAP = { "username": SecretGroup.USER, @@ -599,8 +614,8 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: self._on_relation_changed_event, ) self._jujuversion = None - self.secrets = SecretCache(self.charm) - self.component = self.local_app + self.component = self.local_app if self.SCOPE == Scope.APP else self.local_unit + self.secrets = SecretCache(self.charm, self.component) @property def relations(self) -> List[Relation]: @@ -808,7 +823,7 @@ def _process_secret_fields( return (result, normal_fields) def _fetch_relation_data_without_secrets( - self, app: Union[Application, Unit], relation: Relation, fields: Optional[List[str]] + self, component: Union[Application, Unit], relation: Relation, fields: Optional[List[str]] ) -> Dict[str, str]: """Fetching databag contents when no secrets are involved. @@ -817,17 +832,19 @@ def _fetch_relation_data_without_secrets( This is used typically when the Provides side wants to read the Requires side's data, or when the Requires side may want to read its own data. """ - if app not in relation.data or not relation.data[app]: + if component not in relation.data or not relation.data[component]: return {} if fields: - return {k: relation.data[app][k] for k in fields if k in relation.data[app]} + return { + k: relation.data[component][k] for k in fields if k in relation.data[component] + } else: - return dict(relation.data[app]) + return dict(relation.data[component]) def _fetch_relation_data_with_secrets( self, - app: Union[Application, Unit], + component: Union[Application, Unit], req_secret_fields: Optional[List[str]], relation: Relation, fields: Optional[List[str]] = None, @@ -843,10 +860,10 @@ def _fetch_relation_data_with_secrets( normal_fields = [] if not fields: - if app not in relation.data or not relation.data[app]: + if component not in relation.data or not relation.data[component]: return {} - all_fields = list(relation.data[app].keys()) + all_fields = list(relation.data[component].keys()) normal_fields = [field for field in all_fields if not self._is_secret_field(field)] # There must have been secrets there @@ -863,30 +880,30 @@ def _fetch_relation_data_with_secrets( # (Typically when Juju3 Requires meets Juju2 Provides) if normal_fields: result.update( - self._fetch_relation_data_without_secrets(app, relation, list(normal_fields)) + self._fetch_relation_data_without_secrets(component, relation, list(normal_fields)) ) return result def _update_relation_data_without_secrets( - self, app: Union[Application, Unit], relation: Relation, data: Dict[str, str] + self, component: Union[Application, Unit], relation: Relation, data: Dict[str, str] ) -> None: """Updating databag contents when no secrets are involved.""" - if app not in relation.data or relation.data[app] is None: + if component not in relation.data or relation.data[component] is None: return if relation: - relation.data[app].update(data) + relation.data[component].update(data) def _delete_relation_data_without_secrets( - self, app: Union[Application, Unit], relation: Relation, fields: List[str] + self, component: Union[Application, Unit], relation: Relation, fields: List[str] ) -> None: """Remove databag fields 'fields' from Relation.""" - if app not in relation.data or relation.data[app] is None: + if component not in relation.data or relation.data[component] is None: return for field in fields: try: - relation.data[app].pop(field) + relation.data[component].pop(field) except KeyError: logger.error( "Non-existing field '%s' was attempted to be removed from the databag (relation ID: %s)", @@ -1311,7 +1328,7 @@ def _register_secret_to_relation( label = self._generate_secret_label(relation_name, relation_id, group) # Fetchin the Secret's meta information ensuring that it's locally getting registered with - CachedSecret(self.charm, label, secret_id).meta + CachedSecret(self.charm, self.component, label, secret_id).meta def _register_secrets_to_relation(self, relation: Relation, params_name_list: List[str]): """Make sure that secrets of the provided list are locally 'registered' from the databag. @@ -1602,7 +1619,8 @@ def _delete_relation_data(self, relation: Relation, fields: List[str]) -> None: current_data.get(relation.id, []) ): logger.error( - "Non-existing secret %s was attempted to be removed.", non_existent + "Non-existing secret %s was attempted to be removed.", + ", ".join(non_existent), ) _, normal_fields = self._process_secret_fields( @@ -1648,9 +1666,10 @@ def fetch_relation_field( class DataPeerUnit(DataPeer): """Unit databag representation.""" + SCOPE = Scope.UNIT + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.component = self.local_unit # General events @@ -1668,12 +1687,8 @@ def extra_user_roles(self) -> Optional[str]: return self.relation.data[self.relation.app].get("extra-user-roles") -class AuthenticationEvent(RelationEvent): - """Base class for authentication fields for events. - - The amount of logic added here is not ideal -- but this was the only way to preserve - the interface when moving to Juju Secrets - """ +class RelationEventWithSecret(RelationEvent): + """Base class for Relation Events that need to handle secrets.""" @property def _secrets(self) -> dict: @@ -1685,18 +1700,6 @@ def _secrets(self) -> dict: self._cached_secrets = {} return self._cached_secrets - @property - def _jujuversion(self) -> JujuVersion: - """Caching jujuversion to avoid a Juju call on each field evaluation. - - DON'T USE the encapsulated helper variable outside of this function - """ - if not hasattr(self, "_cached_jujuversion"): - self._cached_jujuversion = None - if not self._cached_jujuversion: - self._cached_jujuversion = JujuVersion.from_environ() - return self._cached_jujuversion - def _get_secret(self, group) -> Optional[Dict[str, str]]: """Retrieveing secrets.""" if not self.app: @@ -1712,7 +1715,15 @@ def _get_secret(self, group) -> Optional[Dict[str, str]]: @property def secrets_enabled(self): """Is this Juju version allowing for Secrets usage?""" - return self._jujuversion.has_secrets + return JujuVersion.from_environ().has_secrets + + +class AuthenticationEvent(RelationEventWithSecret): + """Base class for authentication fields for events. + + The amount of logic added here is not ideal -- but this was the only way to preserve + the interface when moving to Juju Secrets + """ @property def username(self) -> Optional[str]: @@ -1795,7 +1806,7 @@ class DatabaseProvidesEvents(CharmEvents): database_requested = EventSource(DatabaseRequestedEvent) -class DatabaseRequiresEvent(RelationEvent): +class DatabaseRequiresEvent(RelationEventWithSecret): """Base class for database events.""" @property @@ -1850,6 +1861,11 @@ def uris(self) -> Optional[str]: if not self.relation.app: return None + if self.secrets_enabled: + secret = self._get_secret("user") + if secret: + return secret.get("uris") + return self.relation.data[self.relation.app].get("uris") @property @@ -1893,7 +1909,7 @@ class DatabaseRequiresEvents(CharmEvents): class DatabaseProvides(DataProvides): """Provider-side of the database relations.""" - on = DatabaseProvidesEvents() # pyright: ignore [reportGeneralTypeIssues] + on = DatabaseProvidesEvents() # pyright: ignore [reportAssignmentType] def __init__(self, charm: CharmBase, relation_name: str) -> None: super().__init__(charm, relation_name) @@ -1988,7 +2004,7 @@ def set_version(self, relation_id: int, version: str) -> None: class DatabaseRequires(DataRequires): """Requires-side of the database relation.""" - on = DatabaseRequiresEvents() # pyright: ignore [reportGeneralTypeIssues] + on = DatabaseRequiresEvents() # pyright: ignore [reportAssignmentType] def __init__( self, @@ -2317,7 +2333,7 @@ class KafkaRequiresEvents(CharmEvents): class KafkaProvides(DataProvides): """Provider-side of the Kafka relation.""" - on = KafkaProvidesEvents() # pyright: ignore [reportGeneralTypeIssues] + on = KafkaProvidesEvents() # pyright: ignore [reportAssignmentType] def __init__(self, charm: CharmBase, relation_name: str) -> None: super().__init__(charm, relation_name) @@ -2378,7 +2394,7 @@ def set_zookeeper_uris(self, relation_id: int, zookeeper_uris: str) -> None: class KafkaRequires(DataRequires): """Requires-side of the Kafka relation.""" - on = KafkaRequiresEvents() # pyright: ignore [reportGeneralTypeIssues] + on = KafkaRequiresEvents() # pyright: ignore [reportAssignmentType] def __init__( self, @@ -2515,7 +2531,7 @@ class OpenSearchRequiresEvents(CharmEvents): class OpenSearchProvides(DataProvides): """Provider-side of the OpenSearch relation.""" - on = OpenSearchProvidesEvents() # pyright: ignore[reportGeneralTypeIssues] + on = OpenSearchProvidesEvents() # pyright: ignore[reportAssignmentType] def __init__(self, charm: CharmBase, relation_name: str) -> None: super().__init__(charm, relation_name) @@ -2568,7 +2584,7 @@ def set_version(self, relation_id: int, version: str) -> None: class OpenSearchRequires(DataRequires): """Requires-side of the OpenSearch relation.""" - on = OpenSearchRequiresEvents() # pyright: ignore[reportGeneralTypeIssues] + on = OpenSearchRequiresEvents() # pyright: ignore[reportAssignmentType] def __init__( self, diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 7512ef81f..9b6c0c8f8 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -19,6 +19,7 @@ Any charm using this library should import the `psycopg2` or `psycopg2-binary` dependency. """ import logging +from collections import OrderedDict from typing import Dict, List, Optional, Set, Tuple import psycopg2 @@ -34,10 +35,21 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 21 +LIBPATCH = 22 INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles" +REQUIRED_PLUGINS = { + "address_standardizer": ["postgis"], + "address_standardizer_data_us": ["postgis"], + "jsonb_plperl": ["plperl"], + "postgis_raster": ["postgis"], + "postgis_tiger_geocoder": ["postgis", "fuzzystrmatch"], + "postgis_topology": ["postgis"], +} +DEPENDENCY_PLUGINS = set() +for dependencies in REQUIRED_PLUGINS.values(): + DEPENDENCY_PLUGINS |= set(dependencies) logger = logging.getLogger(__name__) @@ -289,12 +301,18 @@ def enable_disable_extensions(self, extensions: Dict[str, bool], database: str = cursor.execute("SELECT datname FROM pg_database WHERE NOT datistemplate;") databases = {database[0] for database in cursor.fetchall()} + ordered_extensions = OrderedDict() + for plugin in DEPENDENCY_PLUGINS: + ordered_extensions[plugin] = extensions.get(plugin, False) + for extension, enable in extensions.items(): + ordered_extensions[extension] = enable + # Enable/disabled the extension in each database. for database in databases: with self._connect_to_database( database=database ) as connection, connection.cursor() as cursor: - for extension, enable in extensions.items(): + for extension, enable in ordered_extensions.items(): cursor.execute( f"CREATE EXTENSION IF NOT EXISTS {extension};" if enable diff --git a/poetry.lock b/poetry.lock index 8476241ef..46fc203d8 100644 --- a/poetry.lock +++ b/poetry.lock @@ -938,6 +938,20 @@ files = [ {file = "packaging-23.2.tar.gz", hash = "sha256:048fb0e9405036518eaaf48a55953c750c11e1a1b68e0dd1a9d62ed0c092cfc5"}, ] +[[package]] +name = "parameterized" +version = "0.9.0" +description = "Parameterized testing with any Python test framework" +optional = false +python-versions = ">=3.7" +files = [ + {file = "parameterized-0.9.0-py2.py3-none-any.whl", hash = "sha256:4e0758e3d41bea3bbd05ec14fc2c24736723f243b28d702081aef438c9372b1b"}, + {file = "parameterized-0.9.0.tar.gz", hash = "sha256:7fc905272cefa4f364c1a3429cbbe9c0f98b793988efb5bf90aac80f08db09b1"}, +] + +[package.extras] +dev = ["jinja2"] + [[package]] name = "paramiko" version = "2.12.0" @@ -1582,7 +1596,6 @@ files = [ {file = "PyYAML-6.0.1-cp311-cp311-win_amd64.whl", hash = "sha256:bf07ee2fef7014951eeb99f56f39c9bb4af143d8aa3c21b1677805985307da34"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"}, - {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a08c6f0fe150303c1c6b71ebcd7213c2858041a7e01975da3a99aed1e7a378ef"}, {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"}, {file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"}, {file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"}, @@ -2015,4 +2028,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "^3.8.10" -content-hash = "cc59ee68ac427df7ee10e73d0d6a3a394733ca882e3827c8618957e3d80e9a22" +content-hash = "aa5d903b587dddfa36d8a5f13b75704b7feb4add3257e0e332e91f23c82a8721" diff --git a/pyproject.toml b/pyproject.toml index 082157201..329b93aaa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,6 +55,7 @@ coverage = {extras = ["toml"], version = "^7.4.1"} pytest = "^8.0.0" pytest-asyncio = "*" jinja2 = "^3.1.3" +parameterized = "^0.9.0" [tool.poetry.group.integration] optional = true @@ -96,10 +97,12 @@ target-version = ["py38"] [tool.ruff] # preview and explicit preview are enabled for CPY001 preview = true -explicit-preview-rules = true target-version = "py38" src = ["src", "."] line-length = 99 + +[tool.ruff.lint] +explicit-preview-rules = true select = ["A", "E", "W", "F", "C", "N", "D", "I001", "CPY001"] extend-ignore = [ "D203", @@ -118,16 +121,16 @@ extend-ignore = [ # Ignore D107 Missing docstring in __init__ ignore = ["E501", "D107"] -[tool.ruff.per-file-ignores] +[tool.ruff.lint.per-file-ignores] "tests/*" = ["D100", "D101", "D102", "D103", "D104"] -[tool.ruff.flake8-copyright] +[tool.ruff.lint.flake8-copyright] # Check for properly formatted copyright header in each file author = "Canonical Ltd." notice-rgx = "Copyright\\s\\d{4}([-,]\\d{4})*\\s+" -[tool.ruff.mccabe] +[tool.ruff.lint.mccabe] max-complexity = 10 -[tool.ruff.pydocstyle] +[tool.ruff.lint.pydocstyle] convention = "google" diff --git a/src/charm.py b/src/charm.py index 1fa2aa321..26e055199 100755 --- a/src/charm.py +++ b/src/charm.py @@ -10,8 +10,9 @@ import shutil import subprocess from copy import deepcopy -from typing import List, Optional, Union +from typing import 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 @@ -25,8 +26,6 @@ ActiveStatus, BlockedStatus, MaintenanceStatus, - ModelError, - SecretNotFoundError, WaitingStatus, ) @@ -44,23 +43,23 @@ PGB_LOG_DIR, PGBOUNCER_EXECUTABLE, POSTGRESQL_SNAP_NAME, - SECRET_CACHE_LABEL, SECRET_DELETED_LABEL, SECRET_INTERNAL_LABEL, SECRET_KEY_OVERRIDES, - SECRET_LABEL, SNAP_PACKAGES, SNAP_TMP_DIR, UNIT_SCOPE, ) from relations.backend_database import BackendDatabaseRequires from relations.db import DbProvides -from relations.peers import Peers +from relations.peers import AUTH_FILE_DATABAG_KEY, CFG_FILE_DATABAG_KEY, Peers from relations.pgbouncer_provider import PgBouncerProvider from upgrade import PgbouncerUpgrade, get_pgbouncer_dependencies_model logger = logging.getLogger(__name__) +Scopes = Literal[APP_SCOPE, UNIT_SCOPE] + INSTANCE_DIR = "instance_" @@ -72,7 +71,30 @@ class PgBouncerCharm(CharmBase): def __init__(self, *args): super().__init__(*args) - self.secrets = {APP_SCOPE: {}, UNIT_SCOPE: {}} + self.peer_relation_app = DataPeer( + self, + relation_name=PEER_RELATION_NAME, + additional_secret_fields=[ + AUTH_FILE_DATABAG_KEY, + CFG_FILE_DATABAG_KEY, + MONITORING_PASSWORD_KEY, + ], + secret_field_name=SECRET_INTERNAL_LABEL, + deleted_label=SECRET_DELETED_LABEL, + ) + self.peer_relation_unit = DataPeerUnit( + self, + relation_name=PEER_RELATION_NAME, + additional_secret_fields=[ + "key", + "csr", + "cauth", + "cert", + "chain", + ], + secret_field_name=SECRET_INTERNAL_LABEL, + deleted_label=SECRET_DELETED_LABEL, + ) self.framework.observe(self.on.install, self._on_install) self.framework.observe(self.on.remove, self._on_remove) @@ -233,183 +255,56 @@ def _normalize_secret_key(self, key: str) -> str: return new_key - def _scope_obj(self, scope: str): + def _scope_obj(self, scope: Scopes): if scope == APP_SCOPE: - return self.framework.model.app - if scope == UNIT_SCOPE: - return self.framework.model.unit - - def _juju_secrets_get(self, scope: str) -> Optional[bool]: - """Helper function to get Juju secret.""" + return self.app if scope == UNIT_SCOPE: - peer_data = self.peers.unit_databag - else: - peer_data = self.peers.app_databag - - if not peer_data.get(SECRET_INTERNAL_LABEL): - return + return self.unit - if SECRET_CACHE_LABEL not in self.secrets[scope]: - try: - # NOTE: Secret contents are not yet available! - secret = self.model.get_secret(id=peer_data[SECRET_INTERNAL_LABEL]) - except SecretNotFoundError as e: - logging.debug(f"No secret found for ID {peer_data[SECRET_INTERNAL_LABEL]}, {e}") - return - - logging.debug(f"Secret {peer_data[SECRET_INTERNAL_LABEL]} downloaded") - - # We keep the secret object around -- needed when applying modifications - self.secrets[scope][SECRET_LABEL] = secret - - # We retrieve and cache actual secret data for the lifetime of the event scope - try: - self.secrets[scope][SECRET_CACHE_LABEL] = secret.get_content(refresh=True) - except (ValueError, ModelError) as err: - # https://bugs.launchpad.net/juju/+bug/2042596 - # Only triggered when 'refresh' is set - known_model_errors = [ - "ERROR either URI or label should be used for getting an owned secret but not both", - "ERROR secret owner cannot use --refresh", - ] - if isinstance(err, ModelError) and not any( - msg in str(err) for msg in known_model_errors - ): - raise - # Due to: ValueError: Secret owner cannot use refresh=True - self.secrets[scope][SECRET_CACHE_LABEL] = secret.get_content() - - return bool(self.secrets[scope].get(SECRET_CACHE_LABEL)) - - def _juju_secret_get_key(self, scope: str, key: str) -> Optional[str]: - if not key: - return - - key = SECRET_KEY_OVERRIDES.get(key, self._normalize_secret_key(key)) - - if self._juju_secrets_get(scope): - secret_cache = self.secrets[scope].get(SECRET_CACHE_LABEL) - if secret_cache: - secret_data = secret_cache.get(key) - if secret_data and secret_data != SECRET_DELETED_LABEL: - logging.debug(f"Getting secret {scope}:{key}") - return secret_data - logging.debug(f"No value found for secret {scope}:{key}") + def peer_relation_data(self, scope: Scopes) -> DataPeer: + """Returns the peer relation data per scope.""" + if scope == APP_SCOPE: + return self.peer_relation_app + elif scope == UNIT_SCOPE: + return self.peer_relation_unit + + def _translate_field_to_secret_key(self, key: str) -> str: + """Change 'key' to secrets-compatible key field.""" + if not JujuVersion.from_environ().has_secrets: + return key + key = SECRET_KEY_OVERRIDES.get(key, key) + new_key = key.replace("_", "-") + return new_key.strip("-") - def get_secret(self, scope: str, key: str) -> Optional[str]: + def get_secret(self, scope: Scopes, key: str) -> Optional[str]: """Get secret from the secret storage.""" - if scope not in [APP_SCOPE, UNIT_SCOPE]: + if scope not in get_args(Scopes): raise RuntimeError("Unknown secret scope.") - if scope == UNIT_SCOPE: - result = self.peers.unit_databag.get(key, None) - else: - result = self.peers.app_databag.get(key, None) - - # TODO change upgrade to switch to secrets once minor version upgrades is done - if result: - return result - - juju_version = JujuVersion.from_environ() - if juju_version.has_secrets: - return self._juju_secret_get_key(scope, key) - - def _juju_secret_set(self, scope: str, key: str, value: str) -> Optional[str]: - """Helper function setting Juju secret.""" - if scope == UNIT_SCOPE: - peer_data = self.peers.unit_databag - else: - peer_data = self.peers.app_databag - self._juju_secrets_get(scope) + peers = self.model.get_relation(PEER_RELATION_NAME) + secret_key = self._translate_field_to_secret_key(key) + return self.peer_relation_data(scope).fetch_my_relation_field(peers.id, secret_key) - key = SECRET_KEY_OVERRIDES.get(key, self._normalize_secret_key(key)) - - secret = self.secrets[scope].get(SECRET_LABEL) - - # It's not the first secret for the scope, we can reuse the existing one - # that was fetched in the previous call - if secret: - secret_cache = self.secrets[scope][SECRET_CACHE_LABEL] - - if secret_cache.get(key) == value: - logging.debug(f"Key {scope}:{key} has this value defined already") - else: - secret_cache[key] = value - try: - secret.set_content(secret_cache) - except OSError as error: - logging.error( - f"Error in attempt to set {scope}:{key}. " - f"Existing keys were: {list(secret_cache.keys())}. {error}" - ) - return - logging.debug(f"Secret {scope}:{key} was {key} set") - - # We need to create a brand-new secret for this scope - else: - scope_obj = self._scope_obj(scope) - - secret = scope_obj.add_secret({key: value}) - if not secret: - raise RuntimeError(f"Couldn't set secret {scope}:{key}") - - self.secrets[scope][SECRET_LABEL] = secret - self.secrets[scope][SECRET_CACHE_LABEL] = {key: value} - logging.debug(f"Secret {scope}:{key} published (as first). ID: {secret.id}") - peer_data.update({SECRET_INTERNAL_LABEL: secret.id}) - - return self.secrets[scope][SECRET_LABEL].id - - def set_secret(self, scope: str, key: str, value: Optional[str]) -> Optional[str]: + def set_secret(self, scope: Scopes, key: str, value: Optional[str]) -> Optional[str]: """Set secret from the secret storage.""" - if scope not in [APP_SCOPE, UNIT_SCOPE]: + if scope not in get_args(Scopes): raise RuntimeError("Unknown secret scope.") if not value: return self.remove_secret(scope, key) - juju_version = JujuVersion.from_environ() - - if juju_version.has_secrets: - self._juju_secret_set(scope, key, value) - return - if scope == UNIT_SCOPE: - self.peers.unit_databag.update({key: value}) - else: - self.peers.app_databag.update({key: value}) - - def _juju_secret_remove(self, scope: str, key: str) -> None: - """Remove a Juju 3.x secret.""" - self._juju_secrets_get(scope) - - key = SECRET_KEY_OVERRIDES.get(key, self._normalize_secret_key(key)) - - secret = self.secrets[scope].get(SECRET_LABEL) - if not secret: - logging.error(f"Secret {scope}:{key} wasn't deleted: no secrets are available") - return - - secret_cache = self.secrets[scope].get(SECRET_CACHE_LABEL) - if not secret_cache or key not in secret_cache: - logging.error(f"No secret {scope}:{key}") - return + peers = self.model.get_relation(PEER_RELATION_NAME) + secret_key = self._translate_field_to_secret_key(key) + self.peer_relation_data(scope).update_relation_data(peers.id, {secret_key: value}) - secret_cache[key] = SECRET_DELETED_LABEL - secret.set_content(secret_cache) - logging.debug(f"Secret {scope}:{key}") - - def remove_secret(self, scope: str, key: str) -> None: + def remove_secret(self, scope: Scopes, key: str) -> None: """Removing a secret.""" - if scope not in [APP_SCOPE, UNIT_SCOPE]: + if scope not in get_args(Scopes): raise RuntimeError("Unknown secret scope.") - juju_version = JujuVersion.from_environ() - if juju_version.has_secrets: - return self._juju_secret_remove(scope, key) - if scope == UNIT_SCOPE: - del self.peers.unit_databag[key] - else: - del self.peers.app_databag[key] + peers = self.model.get_relation(PEER_RELATION_NAME) + secret_key = self._translate_field_to_secret_key(key) + self.peer_relation_data(scope).delete_relation_data(peers.id, [secret_key]) def _on_start(self, _) -> None: """On Start hook. diff --git a/src/constants.py b/src/constants.py index c03649142..10c04f2b3 100644 --- a/src/constants.py +++ b/src/constants.py @@ -37,7 +37,6 @@ EXTENSIONS_BLOCKING_MESSAGE = "bad relation request - remote app requested extensions, which are unsupported. Please remove this relation." SECRET_LABEL = "secret" -SECRET_CACHE_LABEL = "cache" SECRET_INTERNAL_LABEL = "internal-secret" SECRET_DELETED_LABEL = "None" diff --git a/src/relations/backend_database.py b/src/relations/backend_database.py index 183959dac..2e7f01932 100644 --- a/src/relations/backend_database.py +++ b/src/relations/backend_database.py @@ -213,7 +213,6 @@ def _on_relation_departed(self, event: RelationDepartedEvent): return self.postgres.delete_user(self.auth_user) - self.charm.peers.remove_user(self.auth_user) logger.info("pgbouncer auth user removed") def _on_relation_broken(self, event: RelationBrokenEvent): diff --git a/src/relations/db.py b/src/relations/db.py index e18736395..7a80e443b 100644 --- a/src/relations/db.py +++ b/src/relations/db.py @@ -111,7 +111,7 @@ WaitingStatus, ) -from constants import APP_SCOPE, EXTENSIONS_BLOCKING_MESSAGE, PG +from constants import EXTENSIONS_BLOCKING_MESSAGE, PG logger = logging.getLogger(__name__) @@ -199,6 +199,9 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): If the backend relation is fully initialised and available, we generate the proposed database and create a user on the postgres charm, and add preliminary data to the databag. """ + if not self.charm.unit.is_leader(): + return + if not self._check_backend(): join_event.defer() return @@ -224,11 +227,7 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): return user = self._generate_username(join_event) - if self.charm.unit.is_leader(): - password = pgb.generate_password() - else: - if not (password := self.charm.get_secret(APP_SCOPE, user)): - join_event.defer() + password = pgb.generate_password() self.update_databags( join_event.relation, @@ -262,7 +261,6 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): # set up auth function self.charm.backend.initialise_auth_function([database]) - self.charm.peers.add_user(user, password) # Create user in pgbouncer config cfg = self.charm.read_pgb_config() @@ -481,7 +479,6 @@ def _on_relation_broken(self, broken_event: RelationBrokenEvent): cfg.remove_user(user) self.charm.render_pgb_config(cfg, reload_pgbouncer=True) - self.charm.peers.remove_user(user) try: self.charm.backend.postgres.delete_user(user) diff --git a/src/relations/peers.py b/src/relations/peers.py index 8d87053a9..bb0f52654 100644 --- a/src/relations/peers.py +++ b/src/relations/peers.py @@ -237,17 +237,3 @@ def update_auth_file(self, auth_file: str) -> None: self.charm.set_secret(APP_SCOPE, AUTH_FILE_DATABAG_KEY, auth_file) logger.debug("updated auth file in peer databag") - - def add_user(self, username: str, password: str): - """Adds user to app databag.""" - if not self.charm.unit.is_leader(): - return - - self.charm.set_secret(APP_SCOPE, username, password) - - def remove_user(self, username: str): - """Removes user from app databag.""" - if not self.charm.unit.is_leader(): - return - - self.charm.set_secret(APP_SCOPE, username, None) diff --git a/src/relations/pgbouncer_provider.py b/src/relations/pgbouncer_provider.py index 2a2b5e2de..bd730e1c8 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -129,8 +129,6 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: ) return - self.charm.peers.add_user(user, password) - # Update pgbouncer config cfg = self.charm.read_pgb_config() cfg.add_user(user, admin=True if "SUPERUSER" in extra_user_roles else False) diff --git a/tests/unit/relations/test_backend_database.py b/tests/unit/relations/test_backend_database.py index 6ed68715c..bcddd80e5 100644 --- a/tests/unit/relations/test_backend_database.py +++ b/tests/unit/relations/test_backend_database.py @@ -113,7 +113,6 @@ def test_on_database_created( @patch( "relations.backend_database.BackendDatabaseRequires.postgres", new_callable=PropertyMock ) - @patch("relations.peers.Peers.remove_user") @patch("charm.PgBouncerCharm.update_postgres_endpoints") @patch("charm.PgBouncerCharm.update_client_connection_info") @patch("relations.backend_database.BackendDatabaseRequires.remove_auth_function") @@ -122,7 +121,6 @@ def test_relation_departed( _remove_auth, _update_conn_info, _update_endpoints, - _remove_user, _postgres, _auth_user, ): @@ -138,7 +136,6 @@ def test_relation_departed( _remove_auth.assert_called() _remove_auth.reset_mock() _postgres().delete_user.assert_called() - _remove_user.assert_called() # Check departing when we're just scaling down this depart_event.departing_unit = self.charm.unit diff --git a/tests/unit/relations/test_peers.py b/tests/unit/relations/test_peers.py index 37356e1ea..41cae6e1a 100644 --- a/tests/unit/relations/test_peers.py +++ b/tests/unit/relations/test_peers.py @@ -2,12 +2,12 @@ # See LICENSE file for licensing details. import unittest -from unittest.mock import MagicMock, PropertyMock, patch +from unittest.mock import MagicMock, patch from ops.testing import Harness from charm import PgBouncerCharm -from constants import BACKEND_RELATION_NAME +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 tests.helpers import patch_network_get @@ -24,18 +24,12 @@ def setUp(self): self.app = self.charm.app.name self.unit = self.charm.unit.name - @patch("relations.peers.Peers.app_databag", new_callable=PropertyMock) - @patch("relations.peers.Peers.unit_databag", new_callable=PropertyMock) + self.rel_id = self.harness.add_relation(PEER_RELATION_NAME, self.charm.app.name) + @patch("charm.PgBouncerCharm.render_pgb_config") @patch("charm.PgBouncerCharm.render_auth_file") @patch("charm.PgBouncerCharm.reload_pgbouncer") - def test_on_peers_changed( - self, reload_pgbouncer, render_auth_file, render_pgb_config, unit_databag, app_databag - ): - databag = {} - app_databag.return_value = databag - unit_databag.return_value = databag - + 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) @@ -52,7 +46,12 @@ def test_on_peers_changed( reload_pgbouncer.assert_not_called() # Assert that we're reloading pgb even if we're only changing one thing - databag[CFG_FILE_DATABAG_KEY] = PgbConfig(DEFAULT_CONFIG).render() + 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() @@ -60,7 +59,10 @@ def test_on_peers_changed( render_pgb_config.reset_mock() reload_pgbouncer.reset_mock() - databag[AUTH_FILE_DATABAG_KEY] = '"user" "pass"' + 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() diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 4825f2e7f..97c885461 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -1,17 +1,26 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. +import logging import os import unittest from copy import deepcopy -from unittest.mock import MagicMock, Mock, PropertyMock, call, patch +from unittest.mock import MagicMock, 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 ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus +from ops.model import ( + ActiveStatus, + BlockedStatus, + MaintenanceStatus, + RelationDataTypeError, + WaitingStatus, +) from ops.testing import Harness +from parameterized import parameterized from charm import PgBouncerCharm from constants import ( @@ -20,10 +29,7 @@ PEER_RELATION_NAME, PGB_CONF_DIR, PGB_LOG_DIR, - SECRET_CACHE_LABEL, - SECRET_DELETED_LABEL, SECRET_INTERNAL_LABEL, - SECRET_LABEL, SNAP_PACKAGES, ) from tests.helpers import patch_network_get @@ -46,6 +52,10 @@ def setUp(self): self.rel_id = self.harness.add_relation(PEER_RELATION_NAME, self.charm.app.name) + @pytest.fixture + def use_caplog(self, caplog): + self._caplog = caplog + @patch("builtins.open", unittest.mock.mock_open()) @patch("charm.snap.SnapCache") @patch("charm.PgBouncerCharm._install_snap_packages") @@ -316,8 +326,20 @@ def test_render_pgb_config(self, _render, _reload): self.charm.render_pgb_config(cfg, reload_pgbouncer=True) _reload.assert_called_once() + # + # Secrets + # + + def test_scope_obj(self): + assert self.charm._scope_obj("app") == self.charm.framework.model.app + assert self.charm._scope_obj("unit") == self.charm.framework.model.unit + assert self.charm._scope_obj("test") is None + @patch_network_get(private_address="1.1.1.1") def test_get_secret(self): + # App level changes require leader privileges + with self.harness.hooks_disabled(): + self.harness.set_leader() # Test application scope. assert self.charm.get_secret("app", "password") is None self.harness.update_relation_data( @@ -325,6 +347,9 @@ def test_get_secret(self): ) assert self.charm.get_secret("app", "password") == "test-password" + # Unit level changes don't require leader privileges + with self.harness.hooks_disabled(): + self.harness.set_leader(False) # Test unit scope. assert self.charm.get_secret("unit", "password") is None self.harness.update_relation_data( @@ -332,36 +357,22 @@ def test_get_secret(self): ) assert self.charm.get_secret("unit", "password") == "test-password" - @patch("ops.charm.model.Model.get_secret") - @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) + @parameterized.expand([("app"), ("unit")]) @patch_network_get(private_address="1.1.1.1") - def test_get_secret_juju(self, _, _get_secret): - _get_secret.return_value.get_content.return_value = {"password": "test-password"} - - # clean the caches - if SECRET_INTERNAL_LABEL in self.charm.peers.app_databag: - del self.charm.app_peer_data[SECRET_INTERNAL_LABEL] - self.charm.secrets["app"] = {} - - # Test application scope. - assert self.charm.get_secret("app", "password") is None - self.harness.update_relation_data( - self.rel_id, self.charm.app.name, {SECRET_INTERNAL_LABEL: "secret_key"} - ) - assert self.charm.get_secret("app", "password") == "test-password" - _get_secret.assert_called_once_with(id="secret_key") + @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) + def test_get_secret_secrets(self, scope, _): + with self.harness.hooks_disabled(): + self.harness.set_leader() - _get_secret.reset_mock() - - # Test unit scope. - assert self.charm.get_secret("unit", "password") is None - self.harness.update_relation_data( - self.rel_id, self.charm.unit.name, {SECRET_INTERNAL_LABEL: "secret_key"} - ) - assert self.charm.get_secret("unit", "password") == "test-password" - _get_secret.assert_called_once_with(id="secret_key") + assert self.charm.get_secret(scope, "operator-password") is None + self.charm.set_secret(scope, "operator-password", "test-password") + assert self.charm.get_secret(scope, "operator-password") == "test-password" + @patch_network_get(private_address="1.1.1.1") def test_set_secret(self): + with self.harness.hooks_disabled(): + self.harness.set_leader() + # Test application scope. assert "password" not in self.harness.get_relation_data(self.rel_id, self.charm.app.name) self.charm.set_secret("app", "password", "test-password") @@ -382,35 +393,179 @@ def test_set_secret(self): self.charm.set_secret("unit", "password", None) assert "password" not in self.harness.get_relation_data(self.rel_id, self.charm.unit.name) + with self.assertRaises(RuntimeError): + self.charm.set_secret("test", "password", "test") + + @parameterized.expand([("app", True), ("unit", True), ("unit", False)]) + @patch_network_get(private_address="1.1.1.1") @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) - def test_set_secret_juju(self, _): - secret_mock = Mock() - self.charm.secrets["app"][SECRET_LABEL] = secret_mock - self.charm.secrets["app"][SECRET_CACHE_LABEL] = {} - self.charm.secrets["unit"][SECRET_LABEL] = secret_mock - self.charm.secrets["unit"][SECRET_CACHE_LABEL] = {} + def test_set_reset_new_secret(self, scope, is_leader, _): + """NOTE: currently ops.testing seems to allow for non-leader to set secrets too!""" + # App has to be leader, unit can be eithe + with self.harness.hooks_disabled(): + self.harness.set_leader(is_leader) + # Getting current password + self.harness.charm.set_secret(scope, "new-secret", "bla") + assert self.harness.charm.get_secret(scope, "new-secret") == "bla" + + # Reset new secret + self.harness.charm.set_secret(scope, "new-secret", "blablabla") + assert self.harness.charm.get_secret(scope, "new-secret") == "blablabla" + + # Set another new secret + self.harness.charm.set_secret(scope, "new-secret2", "blablabla") + assert self.harness.charm.get_secret(scope, "new-secret2") == "blablabla" + + @parameterized.expand([("app", True), ("unit", True), ("unit", False)]) + @patch_network_get(private_address="1.1.1.1") + @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) + def test_invalid_secret(self, scope, is_leader, _): + # App has to be leader, unit can be either + with self.harness.hooks_disabled(): + self.harness.set_leader(is_leader) - # Test application scope. - assert "password" not in self.charm.secrets["app"].get(SECRET_CACHE_LABEL, {}) - self.charm.set_secret("app", "password", "test-password") - assert self.charm.secrets["app"][SECRET_CACHE_LABEL]["password"] == "test-password" - secret_mock.set_content.assert_called_once_with( - self.charm.secrets["app"][SECRET_CACHE_LABEL] + with self.assertRaises(RelationDataTypeError): + self.harness.charm.set_secret(scope, "somekey", 1) + + self.harness.charm.set_secret(scope, "somekey", "") + assert self.harness.charm.get_secret(scope, "somekey") is None + + @pytest.mark.usefixtures("use_caplog") + @patch_network_get(private_address="1.1.1.1") + def test_delete_password(self): + """NOTE: currently ops.testing seems to allow for non-leader to remove secrets too!""" + with self.harness.hooks_disabled(): + self.harness.set_leader(True) + self.harness.update_relation_data( + self.rel_id, self.charm.app.name, {"replication": "somepw"} ) - secret_mock.reset_mock() + self.harness.charm.remove_secret("app", "replication") + assert self.harness.charm.get_secret("app", "replication") is None - self.charm.set_secret("app", "password", None) - assert self.charm.secrets["app"][SECRET_CACHE_LABEL]["password"] == SECRET_DELETED_LABEL - secret_mock.set_content.assert_called_once_with( - self.charm.secrets["app"][SECRET_CACHE_LABEL] + with self.harness.hooks_disabled(): + self.harness.set_leader(False) + self.harness.update_relation_data( + self.rel_id, self.charm.unit.name, {"somekey": "somevalue"} ) - secret_mock.reset_mock() + self.harness.charm.remove_secret("unit", "somekey") + assert self.harness.charm.get_secret("unit", "somekey") is None + + with self.harness.hooks_disabled(): + self.harness.set_leader(True) + with self._caplog.at_level(logging.ERROR): + self.harness.charm.remove_secret("app", "replication") + assert ( + "Non-existing field 'replication' was attempted to be removed" in self._caplog.text + ) - # Test unit scope. - assert "password" not in self.charm.secrets["unit"].get(SECRET_CACHE_LABEL, {}) - self.charm.set_secret("unit", "password", "test-password") - assert self.charm.secrets["unit"][SECRET_CACHE_LABEL]["password"] == "test-password" - secret_mock.set_content.assert_called_once_with( - self.charm.secrets["unit"][SECRET_CACHE_LABEL] + self.harness.charm.remove_secret("unit", "somekey") + assert "Non-existing field 'somekey' was attempted to be removed" in self._caplog.text + + self.harness.charm.remove_secret("app", "non-existing-secret") + assert ( + "Non-existing field 'non-existing-secret' was attempted to be removed" + in self._caplog.text + ) + + self.harness.charm.remove_secret("unit", "non-existing-secret") + assert ( + "Non-existing field 'non-existing-secret' was attempted to be removed" + in self._caplog.text + ) + + @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) + @patch_network_get(private_address="1.1.1.1") + @pytest.mark.usefixtures("use_caplog") + def test_delete_existing_password_secrets(self, _): + """NOTE: currently ops.testing seems to allow for non-leader to remove secrets too!""" + with self.harness.hooks_disabled(): + self.harness.set_leader(True) + self.harness.charm.set_secret("app", "operator-password", "somepw") + self.harness.charm.remove_secret("app", "operator-password") + assert self.harness.charm.get_secret("app", "operator-password") is None + + with self.harness.hooks_disabled(): + self.harness.set_leader(False) + self.harness.charm.set_secret("unit", "operator-password", "somesecret") + self.harness.charm.remove_secret("unit", "operator-password") + assert self.harness.charm.get_secret("unit", "operator-password") is None + + with self.harness.hooks_disabled(): + self.harness.set_leader(True) + with self._caplog.at_level(logging.ERROR): + self.harness.charm.remove_secret("app", "operator-password") + assert ( + "Non-existing secret operator-password was attempted to be removed." + in self._caplog.text + ) + + self.harness.charm.remove_secret("unit", "operator-password") + assert ( + "Non-existing secret operator-password was attempted to be removed." + in self._caplog.text + ) + + self.harness.charm.remove_secret("app", "non-existing-secret") + assert ( + "Non-existing field 'non-existing-secret' was attempted to be removed" + in self._caplog.text + ) + + self.harness.charm.remove_secret("unit", "non-existing-secret") + assert ( + "Non-existing field 'non-existing-secret' was attempted to be removed" + in self._caplog.text + ) + + @parameterized.expand([("app", True), ("unit", True), ("unit", False)]) + @patch_network_get(private_address="1.1.1.1") + @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) + def test_migration_from_databag(self, scope, is_leader, _): + """Check if we're moving on to use secrets when live upgrade from databag to Secrets usage.""" + # App has to be leader, unit can be either + with self.harness.hooks_disabled(): + self.harness.set_leader(is_leader) + + # Getting current password + entity = getattr(self.charm, scope) + self.harness.update_relation_data(self.rel_id, entity.name, {"operator-password": "bla"}) + assert self.harness.charm.get_secret(scope, "operator-password") == "bla" + + # Reset new secret + self.harness.charm.set_secret(scope, "operator-password", "blablabla") + assert self.harness.charm.model.get_secret(label=f"pgbouncer.{scope}") + assert self.harness.charm.get_secret(scope, "operator-password") == "blablabla" + assert "operator-password" not in self.harness.get_relation_data( + self.rel_id, getattr(self.charm, scope).name + ) + + @parameterized.expand([("app", True), ("unit", True), ("unit", False)]) + @patch_network_get(private_address="1.1.1.1") + @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) + def test_migration_from_single_secret(self, scope, is_leader, _): + """Check if we're moving on to use secrets when live upgrade from databag to Secrets usage.""" + # App has to be leader, unit can be either + with self.harness.hooks_disabled(): + self.harness.set_leader(is_leader) + + secret = self.harness.charm.app.add_secret({"operator-password": "bla"}) + + # Getting current password + entity = getattr(self.charm, scope) + self.harness.update_relation_data( + self.rel_id, entity.name, {SECRET_INTERNAL_LABEL: secret.id} + ) + assert self.harness.charm.get_secret(scope, "operator-password") == "bla" + + # Reset new secret + # Only the leader can set app secret content. + with self.harness.hooks_disabled(): + self.harness.set_leader(True) + self.harness.charm.set_secret(scope, "operator-password", "blablabla") + with self.harness.hooks_disabled(): + self.harness.set_leader(is_leader) + assert self.harness.charm.model.get_secret(label=f"pgbouncer.{scope}") + assert self.harness.charm.get_secret(scope, "operator-password") == "blablabla" + assert SECRET_INTERNAL_LABEL not in self.harness.get_relation_data( + self.rel_id, getattr(self.charm, scope).name ) - secret_mock.reset_mock()