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/loki_k8s/v0/loki_push_api.py b/lib/charms/loki_k8s/v0/loki_push_api.py index 01d7dc161..0e4a2667a 100644 --- a/lib/charms/loki_k8s/v0/loki_push_api.py +++ b/lib/charms/loki_k8s/v0/loki_push_api.py @@ -480,7 +480,7 @@ def _alert_rules_error(self, event): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 25 +LIBPATCH = 26 logger = logging.getLogger(__name__) @@ -2115,7 +2115,21 @@ def _download_and_push_promtail_to_workload(self, promtail_info: dict) -> None: - "zipsha": sha256 sum of zip file of promtail binary - "binsha": sha256 sum of unpacked promtail binary """ - with request.urlopen(promtail_info["url"]) as r: + # Check for Juju proxy variables and fall back to standard ones if not set + proxies: Optional[Dict[str, str]] = {} + if proxies and os.environ.get("JUJU_CHARM_HTTP_PROXY"): + proxies.update({"http": os.environ["JUJU_CHARM_HTTP_PROXY"]}) + if proxies and os.environ.get("JUJU_CHARM_HTTPS_PROXY"): + proxies.update({"https": os.environ["JUJU_CHARM_HTTPS_PROXY"]}) + if proxies and os.environ.get("JUJU_CHARM_NO_PROXY"): + proxies.update({"no_proxy": os.environ["JUJU_CHARM_NO_PROXY"]}) + else: + proxies = None + + proxy_handler = request.ProxyHandler(proxies) + opener = request.build_opener(proxy_handler) + + with opener.open(promtail_info["url"]) as r: file_bytes = r.read() file_path = os.path.join(BINARY_DIR, promtail_info["filename"] + ".gz") with open(file_path, "wb") as f: diff --git a/lib/charms/tls_certificates_interface/v2/tls_certificates.py b/lib/charms/tls_certificates_interface/v2/tls_certificates.py index 08c5cb500..ff234ff46 100644 --- a/lib/charms/tls_certificates_interface/v2/tls_certificates.py +++ b/lib/charms/tls_certificates_interface/v2/tls_certificates.py @@ -286,7 +286,6 @@ def _on_all_certificates_invalidated(self, event: AllCertificatesInvalidatedEven from cryptography.hazmat.primitives import hashes, serialization from cryptography.hazmat.primitives.asymmetric import rsa from cryptography.hazmat.primitives.serialization import pkcs12 -from cryptography.x509.extensions import Extension, ExtensionNotFound from jsonschema import exceptions, validate # type: ignore[import-untyped] from ops.charm import ( CharmBase, @@ -308,7 +307,7 @@ def _on_all_certificates_invalidated(self, event: AllCertificatesInvalidatedEven # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 22 +LIBPATCH = 23 PYDEPS = ["cryptography", "jsonschema"] @@ -1676,7 +1675,7 @@ def get_assigned_certificates(self) -> List[Dict[str, str]]: """ final_list = [] for csr in self.get_certificate_signing_requests(fulfilled_only=True): - assert type(csr["certificate_signing_request"]) == str + assert isinstance(csr["certificate_signing_request"], str) if cert := self._find_certificate_in_relation_data(csr["certificate_signing_request"]): final_list.append(cert) return final_list @@ -1699,7 +1698,7 @@ def get_expiring_certificates(self) -> List[Dict[str, str]]: """ final_list = [] for csr in self.get_certificate_signing_requests(fulfilled_only=True): - assert type(csr["certificate_signing_request"]) == str + assert isinstance(csr["certificate_signing_request"], str) if cert := self._find_certificate_in_relation_data(csr["certificate_signing_request"]): expiry_time = _get_certificate_expiry_time(cert["certificate"]) if not expiry_time: @@ -1719,11 +1718,12 @@ def get_certificate_signing_requests( """Gets the list of CSR's that were sent to the provider. You can choose to get only the CSR's that have a certificate assigned or only the CSR's - that don't. + that don't. Args: fulfilled_only (bool): This option will discard CSRs that don't have certificates yet. unfulfilled_only (bool): This option will discard CSRs that have certificates signed. + Returns: List of CSR dictionaries. For example: [ @@ -1733,10 +1733,9 @@ def get_certificate_signing_requests( } ] """ - final_list = [] for csr in self._requirer_csrs: - assert type(csr["certificate_signing_request"]) == str + assert isinstance(csr["certificate_signing_request"], str) cert = self._find_certificate_in_relation_data(csr["certificate_signing_request"]) if (unfulfilled_only and cert) or (fulfilled_only and not cert): continue diff --git a/poetry.lock b/poetry.lock index 496eed1d9..180659a20 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 1.6.1 and should not be changed by hand. +# This file is automatically @generated by Poetry 1.7.1 and should not be changed by hand. [[package]] name = "allure-pytest" @@ -1127,6 +1127,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" @@ -1800,7 +1814,6 @@ files = [ {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:69b023b2b4daa7548bcfbd4aa3da05b3a74b772db9e23b982788168117739938"}, {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:81e0b275a9ecc9c0c0c07b4b90ba548307583c125f54d5b6946cfee6360c733d"}, {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:ba336e390cd8e4d1739f42dfe9bb83a3cc2e80f567d8805e11b46f4a943f5515"}, - {file = "PyYAML-6.0.1-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:326c013efe8048858a6d312ddd31d56e468118ad4cdeda36c719bf5bb6192290"}, {file = "PyYAML-6.0.1-cp310-cp310-win32.whl", hash = "sha256:bd4af7373a854424dabd882decdc5579653d7868b8fb26dc7d0e99f823aa5924"}, {file = "PyYAML-6.0.1-cp310-cp310-win_amd64.whl", hash = "sha256:fd1592b3fdf65fff2ad0004b5e363300ef59ced41c2e6b3a99d4089fa8c5435d"}, {file = "PyYAML-6.0.1-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:6965a7bc3cf88e5a1c3bd2e0b5c22f8d677dc88a455344035f03399034eb3007"}, @@ -1808,16 +1821,8 @@ files = [ {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:42f8152b8dbc4fe7d96729ec2b99c7097d656dc1213a3229ca5383f973a5ed6d"}, {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:062582fca9fabdd2c8b54a3ef1c978d786e0f6b3a1510e0ac93ef59e0ddae2bc"}, {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:d2b04aac4d386b172d5b9692e2d2da8de7bfb6c387fa4f801fbf6fb2e6ba4673"}, - {file = "PyYAML-6.0.1-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:e7d73685e87afe9f3b36c799222440d6cf362062f78be1013661b00c5c6f678b"}, {file = "PyYAML-6.0.1-cp311-cp311-win32.whl", hash = "sha256:1635fd110e8d85d55237ab316b5b011de701ea0f29d07611174a1b42f1444741"}, {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"}, - {file = "PyYAML-6.0.1-cp312-cp312-win_amd64.whl", hash = "sha256:0d3304d8c0adc42be59c5f8a4d9e3d7379e6955ad754aa9d6ab7a398b59dd1df"}, {file = "PyYAML-6.0.1-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:50550eb667afee136e9a77d6dc71ae76a44df8b3e51e41b77f6de2932bfe0f47"}, {file = "PyYAML-6.0.1-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:1fe35611261b29bd1de0070f0b2f47cb6ff71fa6595c077e42bd0c419fa27b98"}, {file = "PyYAML-6.0.1-cp36-cp36m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:704219a11b772aea0d8ecd7058d0082713c3562b4e271b849ad7dc4a5c90c13c"}, @@ -1834,7 +1839,6 @@ files = [ {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a0cd17c15d3bb3fa06978b4e8958dcdc6e0174ccea823003a106c7d4d7899ac5"}, {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:28c119d996beec18c05208a8bd78cbe4007878c6dd15091efb73a30e90539696"}, {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:7e07cbde391ba96ab58e532ff4803f79c4129397514e1413a7dc761ccd755735"}, - {file = "PyYAML-6.0.1-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:49a183be227561de579b4a36efbb21b3eab9651dd81b1858589f796549873dd6"}, {file = "PyYAML-6.0.1-cp38-cp38-win32.whl", hash = "sha256:184c5108a2aca3c5b3d3bf9395d50893a7ab82a38004c8f61c258d4428e80206"}, {file = "PyYAML-6.0.1-cp38-cp38-win_amd64.whl", hash = "sha256:1e2722cc9fbb45d9b87631ac70924c11d3a401b2d7f410cc0e3bbf249f2dca62"}, {file = "PyYAML-6.0.1-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:9eb6caa9a297fc2c2fb8862bc5370d0303ddba53ba97e71f08023b6cd73d16a8"}, @@ -1842,7 +1846,6 @@ files = [ {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:5773183b6446b2c99bb77e77595dd486303b4faab2b086e7b17bc6bef28865f6"}, {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:b786eecbdf8499b9ca1d697215862083bd6d2a99965554781d0d8d1ad31e13a0"}, {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:bc1bf2925a1ecd43da378f4db9e4f799775d6367bdb94671027b73b393a7c42c"}, - {file = "PyYAML-6.0.1-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:04ac92ad1925b2cff1db0cfebffb6ffc43457495c9b3c39d3fcae417d7125dc5"}, {file = "PyYAML-6.0.1-cp39-cp39-win32.whl", hash = "sha256:faca3bdcf85b2fc05d06ff3fbc1f83e1391b3e724afa3feba7d13eeab355484c"}, {file = "PyYAML-6.0.1-cp39-cp39-win_amd64.whl", hash = "sha256:510c9deebc5c0225e8c96813043e62b680ba2f9c50a08d3724c7f28a747d1486"}, {file = "PyYAML-6.0.1.tar.gz", hash = "sha256:bfdf460b1736c775f2ba9f6a92bca30bc2095067b8a9d77876d1fad6cc3b4a43"}, @@ -2394,4 +2397,4 @@ testing = ["big-O", "jaraco.functools", "jaraco.itertools", "more-itertools", "p [metadata] lock-version = "2.0" python-versions = "^3.8.10" -content-hash = "7bc0868b292580c35d7b2f0bc37d874be4544bcb6d092d42626b498b0552a06a" +content-hash = "0c3f70f79b9eef846a2963a4edaf66d58856461ae53c7e512618e8ae349f7e13" diff --git a/pyproject.toml b/pyproject.toml index dba427339..b399bec02 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -58,6 +58,7 @@ pytest = "^8.0.0" pytest-asyncio = "*" jinja2 = "^3.1.3" psycopg2-binary = "^2.9.9" +parameterized = "^0.9.0" [tool.poetry.group.integration] optional = true diff --git a/src/charm.py b/src/charm.py index 15951f2bb..9d3f26112 100755 --- a/src/charm.py +++ b/src/charm.py @@ -8,9 +8,10 @@ import logging import os import socket -from typing import Dict, Optional +from typing import Dict, Literal, Optional, get_args import lightkube +from charms.data_platform_libs.v0.data_interfaces import DataPeer, DataPeerUnit from charms.grafana_k8s.v0.grafana_dashboard import GrafanaDashboardProvider from charms.loki_k8s.v0.loki_push_api import LogProxyConsumer from charms.pgbouncer_k8s.v0 import pgb @@ -22,9 +23,8 @@ from ops.charm import CharmBase, ConfigChangedEvent, PebbleReadyEvent from ops.framework import StoredState from ops.main import main -from ops.model import ActiveStatus, BlockedStatus, ModelError, SecretNotFoundError, WaitingStatus +from ops.model import ActiveStatus, BlockedStatus, WaitingStatus from ops.pebble import ConnectionError, Layer, PathError, ServiceStatus -from tenacity import Retrying, stop_after_attempt, wait_fixed from constants import ( APP_SCOPE, @@ -41,11 +41,9 @@ PGB, PGB_DIR, PGB_LOG_DIR, - SECRET_CACHE_LABEL, SECRET_DELETED_LABEL, SECRET_INTERNAL_LABEL, SECRET_KEY_OVERRIDES, - SECRET_LABEL, TLS_CA_FILE, TLS_CERT_FILE, TLS_KEY_FILE, @@ -53,12 +51,14 @@ ) from relations.backend_database import BackendDatabaseRequires from relations.db import DbProvides -from relations.peers import Peers +from relations.peers import CFG_FILE_DATABAG_KEY, Peers from relations.pgbouncer_provider import PgBouncerProvider from upgrade import PgbouncerUpgrade, get_pgbouncer_k8s_dependencies_model logger = logging.getLogger(__name__) +Scopes = Literal[APP_SCOPE, UNIT_SCOPE] + class PgBouncerK8sCharm(CharmBase): """A class implementing charmed PgBouncer.""" @@ -68,7 +68,30 @@ class PgBouncerK8sCharm(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.config_changed, self._on_config_changed) self.framework.observe(self.on.pgbouncer_pebble_ready, self._on_pgbouncer_pebble_ready) @@ -515,186 +538,59 @@ 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 - - if SECRET_CACHE_LABEL not in self.secrets[scope]: - for attempt in Retrying(stop=stop_after_attempt(3), wait=wait_fixed(1), reraise=True): - with attempt: - 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 + return self.unit - 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 _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) -> 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) - - 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}" - ) - logging.debug(f"Secret {scope}:{key} was {key} set") - - # We need to create a brand-new secret for this scope + peers = self.model.get_relation(PEER_RELATION_NAME) + secret_key = self._translate_field_to_secret_key(key) + if scope == APP_SCOPE: + value = self.peer_relation_app.fetch_my_relation_field(peers.id, secret_key) 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}") + value = self.peer_relation_unit.fetch_my_relation_field(peers.id, secret_key) + return value - 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}) + peers = self.model.get_relation(PEER_RELATION_NAME) + secret_key = self._translate_field_to_secret_key(key) + if scope == APP_SCOPE: + self.peer_relation_app.update_relation_data(peers.id, {secret_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 + self.peer_relation_unit.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] + peers = self.model.get_relation(PEER_RELATION_NAME) + secret_key = self._translate_field_to_secret_key(key) + if scope == APP_SCOPE: + self.peer_relation_app.delete_relation_data(peers.id, [secret_key]) else: - del self.peers.app_databag[key] + self.peer_relation_unit.delete_relation_data(peers.id, [secret_key]) def push_tls_files_to_workload(self, update_config: bool = True) -> bool: """Uploads TLS files to the workload container.""" diff --git a/src/constants.py b/src/constants.py index 1d06f6507..758859a7a 100644 --- a/src/constants.py +++ b/src/constants.py @@ -29,7 +29,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/db.py b/src/relations/db.py index 6610db487..f99f28ca3 100644 --- a/src/relations/db.py +++ b/src/relations/db.py @@ -82,7 +82,7 @@ WaitingStatus, ) -from constants import APP_SCOPE, EXTENSIONS_BLOCKING_MESSAGE +from constants import EXTENSIONS_BLOCKING_MESSAGE logger = logging.getLogger(__name__) @@ -209,6 +209,9 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): - If password hasn't been added to the databag by this charm, implying that a user has not been created. """ + if not self.charm.unit.is_leader(): + return + if not self._check_backend(): # We can't relate an app to the backend database without a backend postgres relation join_event.defer() @@ -237,11 +240,7 @@ def _on_relation_joined(self, join_event: RelationJoinedEvent): user = self._generate_username(join_event) if self.charm.unit.is_leader(): - if not (password := self.charm.get_secret(APP_SCOPE, user)): - password = pgb.generate_password() - self.charm.peers.add_user(user, password) - else: - password = self.charm.get_secret(APP_SCOPE, user) + password = pgb.generate_password() if None in [database, password]: # If database isn't available, defer @@ -257,9 +256,6 @@ 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: init_msg = f"initialising database and user for {self.relation_name} relation" @@ -500,7 +496,6 @@ def _on_relation_broken(self, broken_event: RelationBrokenEvent): cfg.remove_user(user) self.charm.render_pgb_config(cfg, reload_pgbouncer=True) if self.charm.unit.is_leader(): - self.charm.peers.remove_user(user) self.charm.backend.postgres.delete_user(user) self._check_for_blocking_relations(broken_event.relation.id) diff --git a/src/relations/peers.py b/src/relations/peers.py index 4d29df504..d71639966 100644 --- a/src/relations/peers.py +++ b/src/relations/peers.py @@ -277,17 +277,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 0493fc55f..54040ecc5 100644 --- a/src/relations/pgbouncer_provider.py +++ b/src/relations/pgbouncer_provider.py @@ -142,8 +142,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/integration/helpers/helpers.py b/tests/integration/helpers/helpers.py index 7965e793c..908dbaab4 100644 --- a/tests/integration/helpers/helpers.py +++ b/tests/integration/helpers/helpers.py @@ -9,6 +9,7 @@ import yaml from charms.pgbouncer_k8s.v0 import pgb +from juju.unit import Unit from pytest_operator.plugin import OpsTest from tenacity import ( RetryError, @@ -324,3 +325,13 @@ async def check_tls(ops_test: OpsTest, relation_id: int, enabled: bool) -> bool: return True except RetryError: return False + + +async def get_leader_unit(ops_test: OpsTest, app: str) -> Optional[Unit]: + leader_unit = None + for unit in ops_test.model.applications[app].units: + if await unit.is_leader_from_status(): + leader_unit = unit + break + + return leader_unit diff --git a/tests/integration/relations/test_db.py b/tests/integration/relations/test_db.py index 50b9ef2c6..07a5caf6f 100644 --- a/tests/integration/relations/test_db.py +++ b/tests/integration/relations/test_db.py @@ -18,6 +18,7 @@ get_app_relation_databag, get_backend_user_pass, get_cfg, + get_leader_unit, get_legacy_relation_username, wait_for_relation_joined_between, wait_for_relation_removed_between, @@ -178,10 +179,9 @@ async def test_extensions_blocking(ops_test: OpsTest) -> None: await ops_test.model.add_relation(f"{PGB}:db", f"{CLIENT_APP_NAME}:db") logger.info("Wait for PGB to block due to extensions") - await ops_test.model.wait_for_idle(apps=[PGB], status="blocked", timeout=1000) - assert ( - ops_test.model.applications[PGB].units[0].workload_status_message - == EXTENSIONS_BLOCKING_MESSAGE + leader_unit = await get_leader_unit(ops_test, PGB) + await ops_test.model.block_until( + lambda: leader_unit.workload_status_message == EXTENSIONS_BLOCKING_MESSAGE, timeout=1000 ) await ops_test.model.applications[PGB].destroy_relation(f"{PGB}:db", f"{CLIENT_APP_NAME}:db") await ops_test.model.wait_for_idle(apps=[PGB], status="active", idle_period=15) diff --git a/tests/unit/relations/test_peers.py b/tests/unit/relations/test_peers.py index 929e54466..fab82d9f6 100644 --- a/tests/unit/relations/test_peers.py +++ b/tests/unit/relations/test_peers.py @@ -2,13 +2,13 @@ # See LICENSE file for licensing details. import unittest -from unittest.mock import MagicMock, PropertyMock, patch +from unittest.mock import MagicMock, patch from charms.pgbouncer_k8s.v0.pgb import DEFAULT_CONFIG, PgbConfig from ops.testing import Harness from charm import PgBouncerK8sCharm -from constants import BACKEND_RELATION_NAME +from constants import BACKEND_RELATION_NAME, PEER_RELATION_NAME from relations.peers import AUTH_FILE_DATABAG_KEY, CFG_FILE_DATABAG_KEY @@ -25,20 +25,15 @@ def setUp(self): self.app = self.charm.app.name self.unit = self.charm.unit.name + self.rel_id = self.harness.add_relation(PEER_RELATION_NAME, self.charm.app.name) + def tearDown(self): self.togggle_monitoring_patch.stop() - @patch("relations.peers.Peers.app_databag", new_callable=PropertyMock) - @patch("relations.peers.Peers.unit_databag", new_callable=PropertyMock) @patch("charm.PgBouncerK8sCharm.render_pgb_config") @patch("charm.PgBouncerK8sCharm.render_auth_file") @patch("charm.PgBouncerK8sCharm.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 - + 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) @@ -55,7 +50,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() @@ -63,7 +63,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/relations/test_pgbouncer_provider.py b/tests/unit/relations/test_pgbouncer_provider.py index a9eb633ed..9156bba6e 100644 --- a/tests/unit/relations/test_pgbouncer_provider.py +++ b/tests/unit/relations/test_pgbouncer_provider.py @@ -93,7 +93,6 @@ def test_on_database_requested( user, _password(), extra_user_roles=event.extra_user_roles ) _pg().create_database.assert_called_with(database, user) - assert self.charm.get_secret("app", user) == _password() _dbp_set_credentials.assert_called_with(rel_id, user, _password()) _dbp_set_version.assert_called_with(rel_id, _pg().get_postgresql_version()) _dbp_set_endpoints.assert_called_with( diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 809e67caa..9c742d342 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -3,12 +3,15 @@ # # Learn more about testing at: https://juju.is/docs/sdk/testing +import logging import unittest -from unittest.mock import Mock, PropertyMock, call, patch +from unittest.mock import PropertyMock, call, patch +import pytest from charms.pgbouncer_k8s.v0.pgb import DEFAULT_CONFIG, PgbConfig -from ops.model import WaitingStatus +from ops.model import RelationDataTypeError, WaitingStatus from ops.testing import Harness +from parameterized import parameterized from charm import PgBouncerK8sCharm from constants import ( @@ -16,10 +19,7 @@ INI_PATH, PEER_RELATION_NAME, PGB, - SECRET_CACHE_LABEL, - SECRET_DELETED_LABEL, SECRET_INTERNAL_LABEL, - SECRET_LABEL, ) @@ -40,6 +40,10 @@ def setUp(self): self.rel_id = self.harness.model.relations[PEER_RELATION_NAME][0].id + @pytest.fixture + def use_caplog(self, caplog): + self._caplog = caplog + @patch("charm.PgBouncerK8sCharm._patch_port") @patch("charm.PgBouncerK8sCharm.read_pgb_config") @patch("charm.PgBouncerK8sCharm.push_file") @@ -301,7 +305,19 @@ def test_push_tls_files_to_workload_disabled_tls( update_config.assert_not_called() push_file.assert_not_called() + # + # 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 + 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( @@ -309,6 +325,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( @@ -316,35 +335,20 @@ def test_get_secret(self): ) assert self.charm.get_secret("unit", "password") == "test-password" - @patch("ops.charm.model.Model.get_secret") + @parameterized.expand([("app"), ("unit")]) @patch("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) - 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"] = {} + def test_get_secret_secrets(self, scope, _): + 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( - 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") - - _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" 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") @@ -365,35 +369,175 @@ 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("charm.JujuVersion.has_secrets", new_callable=PropertyMock, return_value=True) + 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 either + 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("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_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) + + 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") + 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"} + ) + self.harness.charm.remove_secret("app", "replication") + assert self.harness.charm.get_secret("app", "replication") is None - # 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.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 + ) - 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] + 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) + @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("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-k8s.{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 ) - secret_mock.reset_mock() - # 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] + @parameterized.expand([("app", True), ("unit", True), ("unit", False)]) + @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-k8s.{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()