[DPE-2882] Addressing issues 108 and 111 (#113)

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}

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)

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

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(