diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index 5d1691d9..714eace4 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 = 23 +LIBPATCH = 24 PYDEPS = ["ops>=2.0.0"] @@ -526,7 +526,16 @@ def get_content(self) -> Dict[str, str]: """Getting cached secret content.""" if not self._secret_content: if self.meta: - self._secret_content = self.meta.get_content() + try: + self._secret_content = self.meta.get_content(refresh=True) + 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): + raise + # Due to: ValueError: Secret owner cannot use refresh=True + self._secret_content = self.meta.get_content() return self._secret_content def set_content(self, content: Dict[str, str]) -> None: @@ -1085,7 +1094,7 @@ def _delete_relation_secret( secret = self._get_relation_secret(relation.id, group) if not secret: - logging.error("Can't update secret for relation %s", str(relation.id)) + logging.error("Can't delete secret for relation %s", str(relation.id)) return False old_content = secret.get_content() @@ -1827,7 +1836,8 @@ def _assign_relation_alias(self, relation_id: int) -> None: # We need to set relation alias also on the application level so, # it will be accessible in show-unit juju command, executed for a consumer application unit - self.update_relation_data(relation_id, {"alias": available_aliases[0]}) + if self.local_unit.is_leader(): + self.update_relation_data(relation_id, {"alias": available_aliases[0]}) def _emit_aliased_event(self, event: RelationChangedEvent, event_name: str) -> None: """Emit an aliased event to a particular relation if it has an alias. @@ -1914,6 +1924,9 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: # Sets both database and extra user roles in the relation # if the roles are provided. Otherwise, sets only the database. + if not self.local_unit.is_leader(): + return + if self.extra_user_roles: self.update_relation_data( event.relation.id, @@ -2173,6 +2186,9 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: """Event emitted when the Kafka relation is created.""" super()._on_relation_created_event(event) + if not self.local_unit.is_leader(): + return + # Sets topic, extra user roles, and "consumer-group-prefix" in the relation relation_data = { f: getattr(self, f.replace("-", "_"), "") @@ -2345,6 +2361,9 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: """Event emitted when the OpenSearch relation is created.""" super()._on_relation_created_event(event) + if not self.local_unit.is_leader(): + return + # Sets both index and extra user roles in the relation if the roles are provided. # Otherwise, sets only the index. data = {"index": self.index} diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index ca4e0d9f..dade673f 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -5,6 +5,7 @@ import json import os import shutil +from datetime import datetime from pathlib import Path import pytest @@ -104,3 +105,33 @@ async def opensearch_charm(ops_test: OpsTest): charm_path = "tests/integration/opensearch-charm" charm = await ops_test.build_charm(charm_path) return charm + + +@pytest.fixture(autouse=True) +async def without_errors(ops_test: OpsTest, request): + """This fixture is to list all those errors that mustn't occur during execution.""" + # To be executed after the tests + now = datetime.now().strftime("%H:%M:%S.%f")[:-3] + yield + whitelist = [] + if "log_errors_allowed" in request.keywords: + for marker in [ + mark for mark in request.node.iter_markers() if mark.name == "log_errors_allowed" + ]: + for arg in marker.args: + whitelist.append(arg) + + # All errors allowed + if not whitelist: + return + + _, dbg_log, _ = await ops_test.juju("debug-log", "--ms", "--replay") + lines = dbg_log.split("\n") + for index, line in enumerate(lines): + logitems = line.split(" ") + if not line or len(logitems) < 3: + continue + if logitems[1] < now: + continue + if logitems[2] == "ERROR": + assert any(white in line for white in whitelist) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index f913ca94..0cd9fb85 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -2,6 +2,7 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. import json +from time import sleep from typing import Dict, List, Optional import yaml @@ -266,3 +267,14 @@ async def get_application_relation_data( f"no relation data could be grabbed on relation with endpoint {relation_name} and alias {relation_alias}" ) return relation_data[0]["application-data"].get(key) + + +async def check_logs(ops_test: OpsTest, strings: str, limit: int = 10) -> bool: + """Check if any of strings may appear in juju debug-log.""" + # juju debug-log may not be flushed yet, thus the "tenacity simulation" + for tries in range(5): + sleep(3) + _, dbg_log, _ = await ops_test.juju("debug-log", "--no-tail", "--replay") + if any(text in dbg_log for text in strings): + return True + return False diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 6ba92005..04bdd197 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -14,6 +14,7 @@ from .helpers import ( build_connection_string, + check_logs, get_application_relation_data, get_juju_secret, get_leader_id, @@ -41,7 +42,7 @@ @pytest.mark.abort_on_fail async def test_deploy_charms(ops_test: OpsTest, application_charm, database_charm): """Deploy both charms (application and database) to use in the tests.""" - # Deploy both charms (1 units for each application to test that later they correctly + # Deploy both charms (2 units for each application to test that later they correctly # set data in the relation application databag using only the leader unit). await asyncio.gather( ops_test.model.deploy( @@ -431,14 +432,7 @@ async def test_provider_get_set_delete_fields(field, value, ops_test: OpsTest): "field,value,relation_field", [ ("new_field", "blah", "new_field"), - pytest.param( - "tls", - "True", - "secret-tls", - marks=pytest.mark.xfail( - reason="https://github.com/canonical/data-platform-libs/issues/108" - ), - ), + ("tls", "True", "secret-tls"), ], ) @pytest.mark.usefixtures("only_with_juju_secrets") @@ -486,7 +480,7 @@ async def test_provider_get_set_delete_fields_secrets( await action.wait() assert action.results.get("value") == value - # Delete normal field + # Delete field action = await ops_test.model.units.get(leader_name).run_action( "delete-relation-field", **{"relation_id": pytest.second_database_relation.id, "field": field}, @@ -519,34 +513,55 @@ async def test_provider_get_set_delete_fields_secrets( await action.wait() assert action.results["return-code"] == 0 - action = await ops_test.model.units.get(leader_name).run_action( - "delete-relation-field", - **{"relation_id": pytest.second_database_relation.id, "field": "tls-ca"}, - ) - await action.wait() - assert action.results["return-code"] == 0 - +@pytest.mark.log_errors_allowed("Can't delete secret for relation") @pytest.mark.usefixtures("only_with_juju_secrets") async def test_provider_deleted_secret_is_removed(ops_test: OpsTest): """The 'tls' field, that was removed in the previous test has it's secret removed.""" + # Add field + field = "tls" + value = "True" + leader_id = await get_leader_id(ops_test, DATABASE_APP_NAME) + leader_name = f"{DATABASE_APP_NAME}/{leader_id}" + action = await ops_test.model.units.get(leader_name).run_action( + "set-relation-field", + **{ + "relation_id": pytest.second_database_relation.id, + "field": field, + "value": value, + }, + ) + await action.wait() + # Get TLS secret pointer secret_uri = await get_application_relation_data( - ops_test, APPLICATION_APP_NAME, SECOND_DATABASE_RELATION_NAME, f"{SECRET_REF_PREFIX}tls" + ops_test, + APPLICATION_APP_NAME, + SECOND_DATABASE_RELATION_NAME, + f"{SECRET_REF_PREFIX}{field}", ) - # The 7 lines below can be removed once the test above is fully passing - leader_id = await get_leader_id(ops_test, DATABASE_APP_NAME) - leader_name = f"{DATABASE_APP_NAME}/{leader_id}" + # Delete field + action = await ops_test.model.units.get(leader_name).run_action( + "delete-relation-field", + **{"relation_id": pytest.second_database_relation.id, "field": field}, + ) + await action.wait() + assert not (await check_logs(ops_test, strings=["Can't delete secret for relation"])) + action = await ops_test.model.units.get(leader_name).run_action( "delete-relation-field", - **{"relation_id": pytest.second_database_relation.id, "field": "tls"}, + **{"relation_id": pytest.second_database_relation.id, "field": field}, ) await action.wait() + assert await check_logs(ops_test, strings=["Can't delete secret for relation"]) assert ( await get_application_relation_data( - ops_test, APPLICATION_APP_NAME, SECOND_DATABASE_RELATION_NAME, "secret-tls" + ops_test, + APPLICATION_APP_NAME, + SECOND_DATABASE_RELATION_NAME, + f"{SECRET_REF_PREFIX}{field}", ) is None ) @@ -625,6 +640,12 @@ async def test_requires_get_set_delete_fields(ops_test: OpsTest): ) +@pytest.mark.log_errors_allowed( + "This operation (update_relation_data()) can only be performed by the leader unit" +) +@pytest.mark.log_errors_allowed( + "This operation (delete_relation_data()) can only be performed by the leader unit" +) async def test_provider_set_delete_fields_leader_only(ops_test: OpsTest): leader_id = await get_leader_id(ops_test, DATABASE_APP_NAME) leader_name = f"{DATABASE_APP_NAME}/{leader_id}" @@ -649,6 +670,12 @@ async def test_provider_set_delete_fields_leader_only(ops_test: OpsTest): }, ) await action.wait() + assert await check_logs( + ops_test, + strings=[ + "This operation (update_relation_data()) can only be performed by the leader unit" + ], + ) assert ( await get_application_relation_data( @@ -662,6 +689,12 @@ async def test_provider_set_delete_fields_leader_only(ops_test: OpsTest): **{"relation_id": pytest.second_database_relation.id, "field": "new_field"}, ) await action.wait() + assert await check_logs( + ops_test, + strings=[ + "This operation (delete_relation_data()) can only be performed by the leader unit" + ], + ) assert ( await get_application_relation_data( @@ -715,6 +748,12 @@ async def test_requires_set_delete_fields(ops_test: OpsTest): ) +@pytest.mark.log_errors_allowed( + "This operation (update_relation_data()) can only be performed by the leader unit" +) +@pytest.mark.log_errors_allowed( + "This operation (delete_relation_data()) can only be performed by the leader unit" +) async def test_requires_set_delete_fields_leader_only(ops_test: OpsTest): leader_id = await get_leader_id(ops_test, APPLICATION_APP_NAME) leader_name = f"{APPLICATION_APP_NAME}/{leader_id}" @@ -739,6 +778,12 @@ async def test_requires_set_delete_fields_leader_only(ops_test: OpsTest): }, ) await action.wait() + assert await check_logs( + ops_test, + strings=[ + "This operation (update_relation_data()) can only be performed by the leader unit" + ], + ) assert ( await get_application_relation_data( @@ -756,6 +801,12 @@ async def test_requires_set_delete_fields_leader_only(ops_test: OpsTest): **{"relation_id": pytest.second_database_relation.id, "field": "new_field-req"}, ) await action.wait() + assert await check_logs( + ops_test, + strings=[ + "This operation (delete_relation_data()) can only be performed by the leader unit" + ], + ) assert ( await get_application_relation_data(