feat(ISD-847): Prevent server_name to be changed

actions.yaml

@@ -0,0 +1,8 @@
+# Copyright 2023 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+reset-instance:
+  description: |
+    Set a new server_name before running this action.
+    Once a server_name is configured, you must start a new instance if you wish a different one.
+    This actions will erase all data and create a instance with the new server_name.

requirements.txt

@@ -1,2 +1,3 @@
+jsonschema >= 4.17
 ops >= 2.2.0
 pydantic >= 1.10

src/charm.py

@@ -10,6 +10,7 @@
 
 import ops
 from charms.traefik_k8s.v1.ingress import IngressPerAppRequirer
+from ops.charm import ActionEvent
 from ops.main import main
 
 from charm_state import CharmState
@@ -20,7 +21,7 @@
     SYNAPSE_PORT,
     SYNAPSE_SERVICE_NAME,
 )
-from exceptions import CharmConfigInvalidError, CommandMigrateConfigError
+from exceptions import CharmConfigInvalidError, CommandMigrateConfigError, ServerNameModifiedError
 from synapse import Synapse
 
 logger = logging.getLogger(__name__)
@@ -36,7 +37,6 @@ def __init__(self, *args: Any) -> None:
             args: class arguments.
         """
         super().__init__(*args)
-        self.framework.observe(self.on.config_changed, self._on_config_changed)
         try:
             self._charm_state = CharmState.from_charm(charm=self)
         except CharmConfigInvalidError as exc:
@@ -52,6 +52,8 @@ def __init__(self, *args: Any) -> None:
             host=f"{self.app.name}-endpoints.{self.model.name}.svc.cluster.local",
             strip_prefix=True,
         )
+        self.framework.observe(self.on.config_changed, self._on_config_changed)
+        self.framework.observe(self.on.reset_instance_action, self._on_reset_instance_action)
 
     def _on_config_changed(self, event: ops.HookEvent) -> None:
         """Handle changed configuration.
@@ -64,11 +66,16 @@ def _on_config_changed(self, event: ops.HookEvent) -> None:
             event.defer()
             self.unit.status = ops.WaitingStatus("Waiting for pebble")
             return
-        self.model.unit.status = ops.MaintenanceStatus("Configuring Synapse")
         try:
+            self.model.unit.status = ops.MaintenanceStatus("Configuring Synapse")
             self._synapse.execute_migrate_config(container)
-        except CommandMigrateConfigError as exc:
-            self.model.unit.status = ops.BlockedStatus(exc.msg)
+        except (
+            CharmConfigInvalidError,
+            CommandMigrateConfigError,
+            ops.pebble.PathError,
+            ServerNameModifiedError,
+        ) as exc:
+            self.model.unit.status = ops.BlockedStatus(str(exc))
             return
         container.add_layer(SYNAPSE_CONTAINER_NAME, self._pebble_layer, combine=True)
         container.replan()
@@ -94,6 +101,35 @@ def _pebble_layer(self) -> Dict:
             },
         }
 
+    def _on_reset_instance_action(self, event: ActionEvent) -> None:
+        """Reset instance and report action result.
+
+        Args:
+            event: Event triggering the reset instance action.
+        """
+        results = {
+            "reset-instance": False,
+        }
+        if not self.model.unit.is_leader():
+            event.fail("Only the juju leader unit can run reset instance action")
+            return
+        container = self.unit.get_container(SYNAPSE_CONTAINER_NAME)
+        if not container.can_connect():
+            event.fail("Failed to connect to container")
+            return
+        self.model.unit.status = ops.MaintenanceStatus("Resetting Synapse instance")
+        try:
+            self._synapse.reset_instance(container)
+            self._synapse.execute_migrate_config(container)
+            results["reset-instance"] = True
+        except (ops.pebble.PathError, CommandMigrateConfigError) as exc:
+            self.model.unit.status = ops.BlockedStatus(str(exc))
+            event.fail(str(exc))
+            return
+        self.model.unit.status = ops.ActiveStatus()
+        # results is a dict and set_results expects _SerializedData
+        event.set_results(results)  # type: ignore[arg-type]
+
 
 if __name__ == "__main__":  # pragma: nocover
     main(SynapseCharm)

src/constants.py

@@ -5,6 +5,8 @@
 
 CHECK_READY_NAME = "synapse-ready"
 COMMAND_MIGRATE_CONFIG = "migrate_config"
+SYNAPSE_CONFIG_DIR = "/data"
+SYNAPSE_CONFIG_PATH = f"{SYNAPSE_CONFIG_DIR}/homeserver.yaml"
 SYNAPSE_COMMAND_PATH = "/start.py"
 SYNAPSE_CONTAINER_NAME = "synapse"
 SYNAPSE_PORT = 8008

src/exceptions.py

@@ -36,3 +36,21 @@ def __init__(self, msg: str):
             msg (str): Explanation of the error.
         """
         self.msg = msg
+
+
+class ServerNameModifiedError(Exception):
+    """Exception raised while checking configuration file.
+
+    Raised if server_name from state is different than the one in the configuration file.
+
+    Attrs:
+        msg (str): Explanation of the error.
+    """
+
+    def __init__(self, msg: str):
+        """Initialize a new instance of the ServerNameModifiedError exception.
+
+        Args:
+            msg (str): Explanation of the error.
+        """
+        self.msg = msg

src/synapse.py

@@ -9,12 +9,20 @@
 import typing
 
 import ops
-from ops.pebble import Check, ExecError
+import yaml
+from ops.pebble import Check, ExecError, PathError
 
 from charm_state import CharmState
 from charm_types import ExecResult
-from constants import CHECK_READY_NAME, COMMAND_MIGRATE_CONFIG, SYNAPSE_COMMAND_PATH, SYNAPSE_PORT
-from exceptions import CommandMigrateConfigError
+from constants import (
+    CHECK_READY_NAME,
+    COMMAND_MIGRATE_CONFIG,
+    SYNAPSE_COMMAND_PATH,
+    SYNAPSE_CONFIG_DIR,
+    SYNAPSE_CONFIG_PATH,
+    SYNAPSE_PORT,
+)
+from exceptions import CommandMigrateConfigError, ServerNameModifiedError
 
 logger = logging.getLogger(__name__)
 
@@ -66,6 +74,7 @@ def execute_migrate_config(self, container: ops.Container) -> None:
         Raises:
             CommandMigrateConfigError: something went wrong running migrate_config.
         """
+        self.check_server_name(container)
         # TODO validate if is possible to use SDK instead of command  # pylint: disable=fixme
         migrate_config_command = [SYNAPSE_COMMAND_PATH, COMMAND_MIGRATE_CONFIG]
         migrate_config_result = self._exec(
@@ -83,6 +92,76 @@ def execute_migrate_config(self, container: ops.Container) -> None:
                 "Migrate config failed, please review your charm configuration"
             )
 
+    def check_server_name(self, container: ops.Container) -> None:
+        """Check server_name.
+
+        Check if server_name of the state has been modified in relation to the configuration file.
+
+        Args:
+            container: Container of the charm.
+
+        Raises:
+            PathError: if somethings goes wrong while reading the configuration file.
+            ServerNameModifiedError: if server_name from state is different than the one in the
+                configuration file.
+        """
+        try:
+            configuration_content = str(
+                container.pull(SYNAPSE_CONFIG_PATH, encoding="utf-8").read()
+            )
+            configured_server_name = yaml.safe_load(configuration_content)["server_name"]
+            if (
+                configured_server_name is not None
+                and configured_server_name != self._charm_state.server_name
+            ):
+                msg = (
+                    f"server_name {self._charm_state.server_name} is different from the existing "
+                    f" one {configured_server_name}. Please revert the config or run the action "
+                    "reset-instance if you to erase the existing instance and start a new one."
+                )
+                logger.error(msg)
+                raise ServerNameModifiedError(
+                    "The server_name modification is not allowed, please check the logs"
+                )
+        except PathError as path_error:
+            if path_error.kind == "not-found":
+                logger.debug(
+                    "configuration file %s not found, will be created by config-changed",
+                    SYNAPSE_CONFIG_PATH,
+                )
+            else:
+                logger.error(
+                    "exception while reading configuration file %s: %s",
+                    SYNAPSE_CONFIG_PATH,
+                    path_error,
+                )
+                raise
+
+    def reset_instance(self, container: ops.Container) -> None:
+        """Erase data and config server_name.
+
+        Args:
+            container: Container of the charm.
+
+        Raises:
+            PathError: if somethings goes wrong while erasing the Synapse directory.
+        """
+        logging.debug("Erasing directory %s", SYNAPSE_CONFIG_DIR)
+        try:
+            container.remove_path(SYNAPSE_CONFIG_DIR, recursive=True)
+        except PathError as path_error:
+            # The error "unlinkat //data: device or resource busy" is expected
+            # when removing the entire directory because it's a volume mount.
+            # The files will be removed but SYNAPSE_CONFIG_DIR directory will
+            # remain.
+            if "device or resource busy" in str(path_error):
+                pass
+            else:
+                logger.error(
+                    "exception while erasing directory %s: %s", SYNAPSE_CONFIG_DIR, path_error
+                )
+                raise
+
     def _exec(
         self,
         container: ops.Container,

tests/integration/conftest.py

@@ -8,41 +8,48 @@
 
 import pytest
 import pytest_asyncio
+from juju.application import Application
 from juju.model import Model
 from pytest import Config
 from pytest_operator.plugin import OpsTest
 
 
 @pytest_asyncio.fixture(scope="module", name="server_name")
-async def fixture_server_name() -> str:
+async def server_name_fixture() -> str:
     """Return a server_name."""
     return "my.synapse.local"
 
 
+@pytest_asyncio.fixture(scope="module", name="another_server_name")
+async def another_server_name_fixture() -> str:
+    """Return a server_name."""
+    return "another.synapse.local"
+
+
 @pytest_asyncio.fixture(scope="module", name="model")
-async def fixture_model(ops_test: OpsTest) -> Model:
+async def model_fixture(ops_test: OpsTest) -> Model:
     """Return the current testing juju model."""
     assert ops_test.model
     return ops_test.model
 
 
 @pytest_asyncio.fixture(scope="module", name="synapse_charm")
-async def fixture_synapse_charm(ops_test) -> str:
+async def synapse_charm_fixture(ops_test) -> str:
     """Build the charm"""
     charm = await ops_test.build_charm(".")
     return charm
 
 
 @pytest_asyncio.fixture(scope="module", name="synapse_image")
-def fixture_synapse_image(pytestconfig: Config):
+def synapse_image_fixture(pytestconfig: Config):
     """Get value from parameter synapse-image."""
     synapse_image = pytestconfig.getoption("--synapse-image")
     assert synapse_image, "--synapse-image must be set"
     return synapse_image
 
 
 @pytest_asyncio.fixture(scope="module", name="synapse_app")
-async def fixture_synapse_app(
+async def synapse_app_fixture(
     synapse_image: str,
     model: Model,
     server_name: str,
@@ -66,7 +73,7 @@ async def fixture_synapse_app(
 
 
 @pytest_asyncio.fixture(scope="module", name="get_unit_ips")
-async def fixture_get_unit_ips(ops_test: OpsTest):
+async def get_unit_ips_fixture(ops_test: OpsTest):
     """Return an async function to retrieve unit ip addresses of a certain application."""
 
     async def get_unit_ips(application_name: str):
@@ -102,7 +109,7 @@ def traefik_app_name_fixture() -> str:
 
 
 @pytest_asyncio.fixture(scope="module", name="traefik_app")
-async def fixture_traefik_app(
+async def traefik_app_fixture(
     model: Model,
     synapse_app,  # pylint: disable=unused-argument
     traefik_app_name: str,
@@ -119,5 +126,22 @@ async def fixture_traefik_app(
         },
     )
     await model.wait_for_idle(raise_on_blocked=True)
-
     return app
+
+
+@pytest_asyncio.fixture(scope="function", name="another_synapse_app")
+async def another_synapse_app_fixture(
+    model: Model, synapse_app: Application, server_name: str, another_server_name: str
+):
+    """Change server_name."""
+    # First we guarantee that the first server_name is set
+    # Then change it.
+    await synapse_app.set_config({"server_name": server_name})
+
+    await model.wait_for_idle()
+
+    await synapse_app.set_config({"server_name": another_server_name})
+
+    await model.wait_for_idle()
+
+    yield synapse_app

tests/integration/test_charm.py

@@ -7,6 +7,7 @@
 import typing
 
 import requests
+from juju.action import Action
 from juju.application import Application
 from juju.model import Model
 from ops.model import ActiveStatus
@@ -61,3 +62,41 @@ async def test_with_ingress(
     )
     assert response.status_code == 200
     assert "Welcome to the Matrix" in response.text
+
+
+async def test_server_name_changed(model: Model, another_synapse_app: Application):
+    """
+    arrange: build and deploy the Synapse charm.
+    act: change server_name via juju config.
+    assert: the Synapse application should prevent the change.
+    """
+    unit = model.applications[another_synapse_app.name].units[0]
+    # Status string defined in Juju
+    # https://github.com/juju/juju/blob/2.9/core/status/status.go#L150
+    assert unit.workload_status == "blocked"
+    assert "server_name modification is not allowed" in unit.workload_status_message
+
+
+async def test_reset_instance_action(
+    model: Model, another_synapse_app: Application, another_server_name: str
+):
+    """
+    arrange: a deployed Synapse charm in a blocked state due to a server_name change.
+    act: call the reset_instance action.
+    assert: the old instance is deleted and the new one configured.
+    """
+    unit = model.applications[another_synapse_app.name].units[0]
+    # Status string defined in Juju
+    # https://github.com/juju/juju/blob/2.9/core/status/status.go#L150
+    assert unit.workload_status == "blocked"
+    assert "server_name modification is not allowed" in unit.workload_status_message
+    action_reset_instance: Action = await another_synapse_app.units[0].run_action(  # type: ignore
+        "reset-instance"
+    )
+    await action_reset_instance.wait()
+    assert action_reset_instance.status == "completed"
+    assert action_reset_instance.results["reset-instance"]
+    assert unit.workload_status == "active"
+    config = await model.applications[another_synapse_app.name].get_config()
+    current_server_name = config.get("server_name", {}).get("value")
+    assert current_server_name == another_server_name