From 9850f68693cfe1c0c7e4821390c72f8d8e9fcbb8 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Thu, 28 Sep 2023 16:15:59 -0300 Subject: [PATCH 01/13] Convert CharmState to dataclass --- src/charm.py | 14 ++-- src/charm_state.py | 102 +++++------------------ src/mjolnir.py | 6 +- src/synapse/api.py | 2 +- src/synapse/workload.py | 21 +++-- tests/unit/conftest.py | 11 --- tests/unit/test_charm.py | 6 +- tests/unit/test_database.py | 26 +++--- tests/unit/test_reset_instance_action.py | 6 +- 9 files changed, 64 insertions(+), 130 deletions(-) diff --git a/src/charm.py b/src/charm.py index 5ae792b5..9513e835 100755 --- a/src/charm.py +++ b/src/charm.py @@ -36,10 +36,14 @@ def __init__(self, *args: typing.Any) -> None: args: class arguments. """ super().__init__(*args) - self.database = DatabaseObserver(self) - self.saml = SAMLObserver(self) + self._database = DatabaseObserver(self) + self._saml = SAMLObserver(self) try: - self._charm_state = CharmState.from_charm(charm=self) + self._charm_state = CharmState.from_charm( + charm=self, + datasource=self._database.get_relation_as_datasource(), + saml_config=self._saml.get_relation_as_saml_conf(), + ) except CharmConfigInvalidError as exc: self.model.unit.status = ops.BlockedStatus(exc.msg) return @@ -65,7 +69,7 @@ def __init__(self, *args: typing.Any) -> None: self._observability = Observability(self) # Mjolnir is a moderation tool for Matrix. # See https://github.com/matrix-org/mjolnir/ for more details about it. - if self._charm_state.enable_mjolnir: + if self._charm_state.synapse_config.enable_mjolnir: self._mjolnir = Mjolnir(self, charm_state=self._charm_state) self.framework.observe(self.on.config_changed, self._on_config_changed) self.framework.observe(self.on.reset_instance_action, self._on_reset_instance_action) @@ -136,7 +140,7 @@ def _on_reset_instance_action(self, event: ActionEvent) -> None: try: self.model.unit.status = ops.MaintenanceStatus("Resetting Synapse instance") self.pebble_service.reset_instance(container) - datasource = self.database.get_relation_as_datasource() + datasource = self._database.get_relation_as_datasource() actions.reset_instance( container=container, charm_state=self._charm_state, datasource=datasource ) diff --git a/src/charm_state.py b/src/charm_state.py index 8aaedeca..abd1c628 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -4,9 +4,12 @@ # See LICENSE file for licensing details. """State of the Charm.""" +import dataclasses import itertools import typing +import ops + # pydantic is causing this no-name-in-module problem from pydantic import ( # pylint: disable=no-name-in-module,import-error BaseModel, @@ -18,10 +21,6 @@ from charm_types import DatasourcePostgreSQL, SAMLConfiguration -if typing.TYPE_CHECKING: - from charm import SynapseCharm - - KNOWN_CHARM_CONFIG = ( "server_name", "report_stats", @@ -86,96 +85,33 @@ def to_yes_or_no(cls, value: str) -> str: return "no" +@dataclasses.dataclass(frozen=True) class CharmState: """State of the Charm. - Attrs: - server_name: server_name config. - report_stats: report_stats config. - public_baseurl: public_baseurl config. - enable_mjolnir: enable_mjolnir config. + Attributes: + synapse_config: synapse configuration. datasource: datasource information. saml_config: saml configuration. """ - def __init__( - self, - *, - synapse_config: SynapseConfig, - datasource: typing.Optional[DatasourcePostgreSQL], - saml_config: typing.Optional[SAMLConfiguration], - ) -> None: - """Construct. - - Args: - synapse_config: The value of the synapse_config charm configuration. - datasource: Datasource information. - saml_config: SAML configuration. - """ - self._synapse_config = synapse_config - self._datasource = datasource - self._saml_config = saml_config - - @property - def server_name(self) -> typing.Optional[str]: - """Return server_name config. - - Returns: - str: server_name config. - """ - return self._synapse_config.server_name - - @property - def report_stats(self) -> typing.Union[str, bool, None]: - """Return report_stats config. - - Returns: - str: report_stats config as yes or no. - """ - return self._synapse_config.report_stats - - @property - def public_baseurl(self) -> typing.Optional[str]: - """Return public_baseurl config. - - Returns: - str: public_baseurl config. - """ - return self._synapse_config.public_baseurl - - @property - def enable_mjolnir(self) -> bool: - """Return enable_mjolnir config. - - Returns: - bool: enable_mjolnir config. - """ - return self._synapse_config.enable_mjolnir - - @property - def datasource(self) -> typing.Union[DatasourcePostgreSQL, None]: - """Return datasource. - - Returns: - datasource or None. - """ - return self._datasource - - @property - def saml_config(self) -> typing.Union[SAMLConfiguration, None]: - """Return SAML configuration. - - Returns: - SAMLConfiguration or None. - """ - return self._saml_config + synapse_config: SynapseConfig + datasource: typing.Optional[DatasourcePostgreSQL] + saml_config: typing.Optional[SAMLConfiguration] @classmethod - def from_charm(cls, charm: "SynapseCharm") -> "CharmState": + def from_charm( + cls, + charm: ops.CharmBase, + datasource: typing.Optional[DatasourcePostgreSQL], + saml_config: typing.Optional[SAMLConfiguration], + ) -> "CharmState": """Initialize a new instance of the CharmState class from the associated charm. Args: charm: The charm instance associated with this state. + datasource: datasource information to be used by Synapse. + saml_config: saml configuration to be used by Synapse. Return: The CharmState instance created by the provided charm. @@ -194,6 +130,6 @@ def from_charm(cls, charm: "SynapseCharm") -> "CharmState": raise CharmConfigInvalidError(f"invalid configuration: {error_field_str}") from exc return cls( synapse_config=valid_synapse_config, - datasource=charm.database.get_relation_as_datasource(), - saml_config=charm.saml.get_relation_as_saml_conf(), + datasource=datasource, + saml_config=saml_config, ) diff --git a/src/mjolnir.py b/src/mjolnir.py index 3d8c098d..8e5e44f4 100644 --- a/src/mjolnir.py +++ b/src/mjolnir.py @@ -112,7 +112,7 @@ def _on_collect_status(self, event: ops.CollectStatusEvent) -> None: Args: event: Collect status event. """ - if not self._charm_state.enable_mjolnir: + if not self._charm_state.synapse_config.enable_mjolnir: return container = self._charm.unit.get_container(synapse.SYNAPSE_CONTAINER_NAME) if not container.can_connect(): @@ -194,7 +194,7 @@ def enable_mjolnir(self) -> None: container, USERNAME, True, - str(self._charm_state.server_name), + str(self._charm_state.synapse_config.server_name), admin_access_token, ) mjolnir_access_token = mjolnir_user.access_token @@ -207,7 +207,7 @@ def enable_mjolnir(self) -> None: # Add the Mjolnir user to the management room synapse.make_room_admin( user=mjolnir_user, - server=str(self._charm_state.server_name), + server=str(self._charm_state.synapse_config.server_name), admin_access_token=admin_access_token, room_id=room_id, ) diff --git a/src/synapse/api.py b/src/synapse/api.py index 77623c66..55e3d9ac 100644 --- a/src/synapse/api.py +++ b/src/synapse/api.py @@ -279,7 +279,7 @@ def override_rate_limit(user: User, admin_access_token: str, charm_state: CharmS admin_access_token: server admin access token to be used. charm_state: Instance of CharmState. """ - server_name = charm_state.server_name + server_name = charm_state.synapse_config.server_name rate_limit_url = ( f"{SYNAPSE_URL}/_synapse/admin/v1/users/" f"@{user.username}:{server_name}/override_ratelimit" diff --git a/src/synapse/workload.py b/src/synapse/workload.py index dc08ef6b..3c33736b 100644 --- a/src/synapse/workload.py +++ b/src/synapse/workload.py @@ -266,9 +266,9 @@ def _create_pysaml2_config(charm_state: CharmState) -> typing.Dict: saml_config = charm_state.saml_config entity_id = ( - charm_state.public_baseurl - if charm_state.public_baseurl is not None - else f"https://{charm_state.server_name}" + charm_state.synapse_config.public_baseurl + if charm_state.synapse_config.public_baseurl is not None + else f"https://{charm_state.synapse_config.server_name}" ) sp_config = { "metadata": { @@ -308,8 +308,8 @@ def enable_saml(container: ops.Container, charm_state: CharmState) -> None: try: config = container.pull(SYNAPSE_CONFIG_PATH).read() current_yaml = yaml.safe_load(config) - if charm_state.public_baseurl is not None: - current_yaml["public_baseurl"] = charm_state.public_baseurl + if charm_state.synapse_config.public_baseurl is not None: + current_yaml["public_baseurl"] = charm_state.synapse_config.public_baseurl # enable x_forwarded to pass expected headers current_listeners = current_yaml["listeners"] updated_listeners = [ @@ -386,8 +386,8 @@ def get_environment(charm_state: CharmState) -> typing.Dict[str, str]: A dictionary representing the Synapse environment variables. """ environment = { - "SYNAPSE_SERVER_NAME": f"{charm_state.server_name}", - "SYNAPSE_REPORT_STATS": f"{charm_state.report_stats}", + "SYNAPSE_SERVER_NAME": f"{charm_state.synapse_config.server_name}", + "SYNAPSE_REPORT_STATS": f"{charm_state.synapse_config.report_stats}", # TLS disabled so the listener is HTTP. HTTPS will be handled by Traefik. # TODO verify support to HTTPS backend before changing this # pylint: disable=fixme "SYNAPSE_NO_TLS": str(True), @@ -416,9 +416,12 @@ def _check_server_name(container: ops.Container, charm_state: CharmState) -> Non configuration file. """ configured_server_name = _get_configuration_field(container=container, fieldname="server_name") - if configured_server_name is not None and configured_server_name != charm_state.server_name: + if ( + configured_server_name is not None + and configured_server_name != charm_state.synapse_config.server_name + ): msg = ( - f"server_name {charm_state.server_name} is different from the existing " + f"server_name {charm_state.synapse_config.server_name} is different from the existing " f"one {configured_server_name}. Please revert the config or run the action " "reset-instance if you want to erase the existing instance and start a new " "one." diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 4a8f375a..1ecaf9cd 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -211,14 +211,3 @@ def container_with_path_error_pass_fixture( remove_path_mock = unittest.mock.MagicMock(side_effect=path_error) monkeypatch.setattr(container_mocked, "remove_path", remove_path_mock) return container_mocked - - -@pytest.fixture(name="erase_database_mocked") -def erase_database_mocked_fixture(monkeypatch: pytest.MonkeyPatch) -> unittest.mock.MagicMock: - """Mock erase_database.""" - database_mocked = unittest.mock.MagicMock() - erase_database_mock = unittest.mock.MagicMock(side_effect=None) - monkeypatch.setattr(database_mocked, "erase_database", erase_database_mock) - monkeypatch.setattr(database_mocked, "get_conn", unittest.mock.MagicMock()) - monkeypatch.setattr(database_mocked, "get_relation_data", unittest.mock.MagicMock()) - return database_mocked diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 9ec052f0..edaca955 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -174,7 +174,7 @@ def test_saml_integration_container_down(saml_configured: Harness) -> None: harness.set_can_connect(harness.model.unit.containers[synapse.SYNAPSE_CONTAINER_NAME], False) relation = harness.charm.framework.model.get_relation("saml", 0) - harness.charm.saml.saml.on.saml_data_available.emit(relation) + harness.charm._saml.saml.on.saml_data_available.emit(relation) assert isinstance(harness.model.unit.status, ops.MaintenanceStatus) assert "Waiting for" in str(harness.model.unit.status) @@ -193,9 +193,9 @@ def test_saml_integration_pebble_error( harness.begin() relation = harness.charm.framework.model.get_relation("saml", 0) enable_saml_mock = MagicMock(side_effect=PebbleServiceError("fail")) - monkeypatch.setattr(harness.charm.saml._pebble_service, "enable_saml", enable_saml_mock) + monkeypatch.setattr(harness.charm._saml._pebble_service, "enable_saml", enable_saml_mock) - harness.charm.saml.saml.on.saml_data_available.emit(relation) + harness.charm._saml.saml.on.saml_data_available.emit(relation) assert isinstance(harness.model.unit.status, ops.BlockedStatus) assert "SAML integration failed" in str(harness.model.unit.status) diff --git a/tests/unit/test_database.py b/tests/unit/test_database.py index cdb2d414..14ac6597 100644 --- a/tests/unit/test_database.py +++ b/tests/unit/test_database.py @@ -38,7 +38,7 @@ def test_erase_database(harness: Harness, monkeypatch: pytest.MonkeyPatch) -> No assert: erase query is executed. """ harness.begin() - datasource = harness.charm.database.get_relation_as_datasource() + datasource = harness.charm._database.get_relation_as_datasource() db_client = DatabaseClient(datasource=datasource) conn_mock = unittest.mock.MagicMock() cursor_mock = conn_mock.cursor.return_value.__enter__.return_value @@ -71,7 +71,7 @@ def test_erase_database_error(harness: Harness, monkeypatch: pytest.MonkeyPatch) assert: exception is raised. """ harness.begin() - datasource = harness.charm.database.get_relation_as_datasource() + datasource = harness.charm._database.get_relation_as_datasource() db_client = DatabaseClient(datasource=datasource) conn_mock = unittest.mock.MagicMock() cursor_mock = conn_mock.cursor.return_value.__enter__.return_value @@ -93,7 +93,7 @@ def test_connect(harness: Harness, monkeypatch: pytest.MonkeyPatch): postgresql_relation = harness.model.relations["database"][0] harness.update_relation_data(postgresql_relation.id, "postgresql", {"password": token_hex(16)}) harness.begin() - datasource = harness.charm.database.get_relation_as_datasource() + datasource = harness.charm._database.get_relation_as_datasource() db_client = DatabaseClient(datasource=datasource) mock_connection = unittest.mock.MagicMock() mock_connection.autocommit = True @@ -118,7 +118,7 @@ def test_connect_error(harness: Harness, monkeypatch: pytest.MonkeyPatch) -> Non assert: exception is raised. """ harness.begin() - datasource = harness.charm.database.get_relation_as_datasource() + datasource = harness.charm._database.get_relation_as_datasource() db_client = DatabaseClient(datasource=datasource) error_msg = "Invalid query" connect_mock = unittest.mock.MagicMock(side_effect=psycopg2.Error(error_msg)) @@ -134,7 +134,7 @@ def test_prepare_database(harness: Harness, monkeypatch: pytest.MonkeyPatch) -> assert: update query is executed. """ harness.begin() - datasource = harness.charm.database.get_relation_as_datasource() + datasource = harness.charm._database.get_relation_as_datasource() db_client = DatabaseClient(datasource=datasource) conn_mock = unittest.mock.MagicMock() cursor_mock = conn_mock.cursor.return_value.__enter__.return_value @@ -160,7 +160,7 @@ def test_prepare_database_error(harness: Harness, monkeypatch: pytest.MonkeyPatc assert: exception is raised. """ harness.begin() - datasource = harness.charm.database.get_relation_as_datasource() + datasource = harness.charm._database.get_relation_as_datasource() db_client = DatabaseClient(datasource=datasource) conn_mock = unittest.mock.MagicMock() cursor_mock = conn_mock.cursor.return_value.__enter__.return_value @@ -195,8 +195,8 @@ def test_relation_as_datasource( port="5432", user="user", ) - assert expected == harness.charm.database.get_relation_as_datasource() - assert harness.charm.app.name == harness.charm.database.get_database_name() + assert expected == harness.charm._database.get_relation_as_datasource() + assert harness.charm.app.name == harness.charm._database.get_database_name() synapse_env = synapse.get_environment(harness.charm._charm_state) assert synapse_env["POSTGRES_DB"] == expected["db"] assert synapse_env["POSTGRES_HOST"] == expected["host"] @@ -215,10 +215,10 @@ def test_relation_as_datasource_error(harness: Harness, monkeypatch: pytest.Monk get_relation_as_datasource_mock = unittest.mock.MagicMock(return_value=None) monkeypatch.setattr( - harness.charm.database, "get_relation_as_datasource", get_relation_as_datasource_mock + harness.charm._database, "get_relation_as_datasource", get_relation_as_datasource_mock ) with pytest.raises(CharmDatabaseRelationNotFoundError): - harness.charm.database.get_database_name() + harness.charm._database.get_database_name() def test_change_config(harness: Harness): @@ -229,7 +229,7 @@ def test_change_config(harness: Harness): """ harness.begin() - harness.charm.database._change_config() + harness.charm._database._change_config() assert isinstance(harness.model.unit.status, ops.ActiveStatus) @@ -245,7 +245,7 @@ def test_change_config_error( harness.begin() harness.set_can_connect(harness.model.unit.containers[synapse.SYNAPSE_CONTAINER_NAME], False) - harness.charm.database._change_config() + harness.charm._database._change_config() assert isinstance(harness.model.unit.status, ops.MaintenanceStatus) @@ -267,6 +267,6 @@ def test_on_database_created(harness: Harness, monkeypatch: pytest.MonkeyPatch): database_observer, "DatabaseClient", unittest.mock.MagicMock(return_value=db_client_mock) ) - harness.charm.database._on_database_created(unittest.mock.MagicMock()) + harness.charm._database._on_database_created(unittest.mock.MagicMock()) db_client_mock.prepare.assert_called_once() diff --git a/tests/unit/test_reset_instance_action.py b/tests/unit/test_reset_instance_action.py index 1d0f95d9..83bcd4ee 100644 --- a/tests/unit/test_reset_instance_action.py +++ b/tests/unit/test_reset_instance_action.py @@ -13,6 +13,7 @@ from ops.testing import Harness import synapse +from database_client import DatabaseClient from .conftest import TEST_SERVER_NAME @@ -84,6 +85,7 @@ def test_reset_instance_action_failed(harness: Harness) -> None: def test_reset_instance_action_path_error_blocked( container_with_path_error_blocked: unittest.mock.MagicMock, harness: Harness, + monkeypatch: pytest.MonkeyPatch, ) -> None: """ arrange: start the Synapse charm, set Synapse container to be ready and set server_name. @@ -96,6 +98,7 @@ def test_reset_instance_action_path_error_blocked( return_value=container_with_path_error_blocked ) event = unittest.mock.MagicMock() + monkeypatch.setattr(DatabaseClient, "erase", unittest.mock.MagicMock()) # Calling to test the action since is not possible calling via harness harness.charm._on_reset_instance_action(event) @@ -109,7 +112,6 @@ def test_reset_instance_action_path_error_pass( container_with_path_error_pass: unittest.mock.MagicMock, harness: Harness, monkeypatch: pytest.MonkeyPatch, - erase_database_mocked: unittest.mock.MagicMock, ) -> None: """ arrange: start the Synapse charm, set Synapse container to be ready and set server_name. @@ -118,7 +120,6 @@ def test_reset_instance_action_path_error_pass( """ harness.begin() harness.set_leader(True) - harness.charm._database = erase_database_mocked content = io.StringIO(f'server_name: "{TEST_SERVER_NAME}"') pull_mock = unittest.mock.MagicMock(return_value=content) monkeypatch.setattr(container_with_path_error_pass, "pull", pull_mock) @@ -126,6 +127,7 @@ def test_reset_instance_action_path_error_pass( return_value=container_with_path_error_pass ) event = unittest.mock.MagicMock() + monkeypatch.setattr(DatabaseClient, "erase", unittest.mock.MagicMock()) # Calling to test the action since is not possible calling via harness harness.charm._on_reset_instance_action(event) From df0e1d8b07a722d19634ebedca4e09839155a9c2 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Thu, 28 Sep 2023 16:42:34 -0300 Subject: [PATCH 02/13] Add SMTP configurations to state --- config.yaml | 51 +++++++++++++++++++++++++--------- src/charm_state.py | 37 ++++++++++++++++++++++++ tests/unit/test_synapse_api.py | 6 ++-- 3 files changed, 79 insertions(+), 15 deletions(-) diff --git a/config.yaml b/config.yaml index 4f280d2c..32cbf1a9 100644 --- a/config.yaml +++ b/config.yaml @@ -2,26 +2,51 @@ # See LICENSE file for licensing details. options: - server_name: + enable_mjolnir: + type: boolean + default: false + description: | + Configures whether to enable Mjolnir - moderation tool for Matrix. + Reference: https://github.com/matrix-org/mjolnir + public_baseurl: type: string description: | - Synapse server name. Must be set to deploy the charm. Corresponds to the - server_name option on Synapse configuration file and sets the - public-facing domain of the server. + The public-facing base URL that clients use to access this Homeserver. + Defaults to https:///. Only used if there is integration with + SAML integrator charm. report_stats: description: | Configures whether to report statistics. default: false type: boolean - public_baseurl: + server_name: type: string description: | - The public-facing base URL that clients use to access this Homeserver. - Defaults to https:///. Only used if there is integration with - SAML integrator charm. - enable_mjolnir: + Synapse server name. Must be set to deploy the charm. Corresponds to the + server_name option on Synapse configuration file and sets the + public-facing domain of the server. + smtp_enable_tls: type: boolean - default: false - description: | - Configures whether to enable Mjolnir - moderation tool for Matrix. - Reference: https://github.com/matrix-org/mjolnir + description: If enabled, STARTTLS will be used to use an encrypted SMTP + connection. + default: true + smtp_host: + type: string + description: The hostname of the SMTP host used for sending emails. + default: '' + smtp_notif_from: + type: string + description: defines the "From" address to use when sending emails. + It must be set if email sending is enabled. Defaults to server_name. + smtp_pass: + type: string + description: The password if the SMTP server requires authentication. + default: '' + smtp_port: + type: int + description: The port of the SMTP server used for sending emails. + default: 25 + smtp_user: + type: string + description: The username if the SMTP server requires authentication. + default: '' diff --git a/src/charm_state.py b/src/charm_state.py index abd1c628..bd0b2dbb 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -26,6 +26,12 @@ "report_stats", "public_baseurl", "enable_mjolnir", + "smtp_enable_tls", + "smtp_host", + "smtp_notif_from", + "smtp_pass", + "smtp_port", + "smtp_user", ) @@ -53,12 +59,24 @@ class SynapseConfig(BaseModel): # pylint: disable=too-few-public-methods report_stats: report_stats config. public_baseurl: public_baseurl config. enable_mjolnir: enable_mjolnir config. + smtp_enable_tls: enable tls while connecting to SMTP server. + smtp_host: SMTP host. + smtp_notif_from: defines the "From" address to use when sending emails. + smtp_pass: password to authenticate to SMTP host. + smtp_port: SMTP port. + smtp_user: username to autehtncate to SMTP host. """ server_name: str | None = Field(..., min_length=2) report_stats: str | None = Field(None) public_baseurl: str | None = Field(None) enable_mjolnir: bool = False + smtp_enable_tls: bool = True + smtp_host: str | None = Field(None) + smtp_notif_from: str | None = Field(None) + smtp_pass: str | None = Field(None) + smtp_port: int | None = Field(None) + smtp_user: str | None = Field(None) class Config: # pylint: disable=too-few-public-methods """Config class. @@ -69,6 +87,25 @@ class Config: # pylint: disable=too-few-public-methods extra = Extra.allow + @validator("smtp_notif_from", pre=True, always=True) + @classmethod + def set_default_smtp_notif_from( + cls, smtp_notif_from: typing.Optional[str], values: dict + ) -> typing.Optional[str]: + """Set server_name as default value to smtp_notif_from. + + Args: + smtp_notif_from: the smtp_notif_from current value. + values: values already defined. + + Returns: + The default value for smtp_notif_from if not defined. + """ + server_name = values.get("server_name") + if smtp_notif_from is None and server_name: + return server_name + return smtp_notif_from + @validator("report_stats") @classmethod def to_yes_or_no(cls, value: str) -> str: diff --git a/tests/unit/test_synapse_api.py b/tests/unit/test_synapse_api.py index f900a8cc..a646ec41 100644 --- a/tests/unit/test_synapse_api.py +++ b/tests/unit/test_synapse_api.py @@ -218,7 +218,8 @@ def test_override_rate_limit_success(monkeypatch: pytest.MonkeyPatch): user = User(username=username, admin=True) admin_access_token = token_hex(16) server = token_hex(16) - synapse_config = SynapseConfig(server_name=server, report_stats="False", public_baseurl="") + # while using Pydantic, mypy ignores default values + synapse_config = SynapseConfig(server_name=server) # type: ignore[call-arg] charm_state = CharmState(synapse_config=synapse_config, datasource=None, saml_config=None) expected_url = ( f"http://localhost:8008/_synapse/admin/v1/users/@any-user:{server}/override_ratelimit" @@ -246,7 +247,8 @@ def test_override_rate_limit_error(monkeypatch: pytest.MonkeyPatch): user = User(username=username, admin=True) admin_access_token = token_hex(16) server = token_hex(16) - synapse_config = SynapseConfig(server_name=server, report_stats="False", public_baseurl="") + # while using Pydantic, mypy ignores default values + synapse_config = SynapseConfig(server_name=server) # type: ignore[call-arg] charm_state = CharmState(synapse_config=synapse_config, datasource=None, saml_config=None) expected_error_msg = "Failed to connect" do_request_mock = mock.MagicMock(side_effect=synapse.APIError(expected_error_msg)) From 33ba97fd54ab18e1abbe84ff346af3cd66f37f7b Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Thu, 28 Sep 2023 17:47:14 -0300 Subject: [PATCH 03/13] Enable SMTP in workload if is set --- src/pebble.py | 5 ++++- src/synapse/__init__.py | 1 + src/synapse/workload.py | 25 +++++++++++++++++++++++++ tests/unit/test_charm.py | 20 -------------------- 4 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/pebble.py b/src/pebble.py index 97467667..d51b7c0a 100644 --- a/src/pebble.py +++ b/src/pebble.py @@ -89,12 +89,15 @@ def change_config(self, container: ops.model.Container) -> None: if self._charm_state.saml_config is not None: logger.debug("pebble.change_config: Enabling SAML") synapse.enable_saml(container=container, charm_state=self._charm_state) + if self._charm_state.synapse_config.smtp_host is not None: + logger.debug("pebble.change_config: Enabling SAML") + synapse.enable_smtp(container=container, charm_state=self._charm_state) self.restart_synapse(container) except (synapse.WorkloadError, ops.pebble.PathError) as exc: raise PebbleServiceError(str(exc)) from exc def enable_saml(self, container: ops.model.Container) -> None: - """Enable SAML. + """Enable SAML while receiving on_saml_data_available event. Args: container: Charm container. diff --git a/src/synapse/__init__.py b/src/synapse/__init__.py index afaa7c8d..128cac8e 100644 --- a/src/synapse/__init__.py +++ b/src/synapse/__init__.py @@ -55,6 +55,7 @@ enable_metrics, enable_saml, enable_serve_server_wellknown, + enable_smtp, execute_migrate_config, get_environment, get_registration_shared_secret, diff --git a/src/synapse/workload.py b/src/synapse/workload.py index 3c33736b..5fa78af5 100644 --- a/src/synapse/workload.py +++ b/src/synapse/workload.py @@ -338,6 +338,31 @@ def enable_saml(container: ops.Container, charm_state: CharmState) -> None: raise EnableSAMLError(str(exc)) from exc +def enable_smtp(container: ops.Container, charm_state: CharmState) -> None: + """Change the Synapse configuration to enable SMTP. + + Args: + container: Container of the charm. + charm_state: Instance of CharmState. + + Raises: + WorkloadError: something went wrong enabling SMTP. + """ + try: + config = container.pull(SYNAPSE_CONFIG_PATH).read() + current_yaml = yaml.safe_load(config) + current_yaml["email"] = {} + current_yaml["email"]["smtp_host"] = charm_state.synapse_config.smtp_host + current_yaml["email"]["smtp_port"] = charm_state.synapse_config.smtp_port + current_yaml["email"]["smtp_user"] = charm_state.synapse_config.smtp_user + current_yaml["email"]["smtp_pass"] = charm_state.synapse_config.smtp_pass + current_yaml["email"]["enable_tls"] = charm_state.synapse_config.smtp_enable_tls + current_yaml["email"]["notif_from"] = charm_state.synapse_config.smtp_notif_from + container.push(SYNAPSE_CONFIG_PATH, yaml.safe_dump(current_yaml)) + except ops.pebble.PathError as exc: + raise WorkloadError(str(exc)) from exc + + def get_registration_shared_secret(container: ops.Container) -> typing.Optional[str]: """Get registration_shared_secret from configuration file. diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index edaca955..f70b6cf5 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -143,26 +143,6 @@ def test_traefik_integration(harness: Harness) -> None: } -def test_saml_integration_container_restart(monkeypatch: pytest.MonkeyPatch) -> None: - """ - arrange: start the Synapse charm, set server_name, mock container and enable_saml. - act: enable saml via pebble_service as the observer does. - assert: The container is restarted. - """ - harness = Harness(SynapseCharm) - harness.update_config({"server_name": TEST_SERVER_NAME}) - harness.begin() - monkeypatch.setattr("synapse.enable_saml", MagicMock) - container = MagicMock() - container_restart = MagicMock() - monkeypatch.setattr(container, "restart", container_restart) - - harness.charm.pebble_service.enable_saml(container) - - container_restart.assert_called_once() - harness.cleanup() - - def test_saml_integration_container_down(saml_configured: Harness) -> None: """ arrange: start the Synapse charm, set server_name, set Synapse container to be down. From 1b523d3ddc7d42e0d0ae749dc09b7ff810300d7e Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Thu, 28 Sep 2023 17:49:32 -0300 Subject: [PATCH 04/13] Update src-docs --- src-docs/api.md | 61 +++++++-------- src-docs/charm.py.md | 11 +-- src-docs/charm_state.py.md | 127 +++++++++---------------------- src-docs/database_observer.py.md | 3 - src-docs/mjolnir.py.md | 13 ++-- src-docs/observability.py.md | 3 - src-docs/pebble.py.md | 29 +++---- src-docs/saml_observer.py.md | 3 - 8 files changed, 85 insertions(+), 165 deletions(-) diff --git a/src-docs/api.md b/src-docs/api.md index f315439c..f908d493 100644 --- a/src-docs/api.md +++ b/src-docs/api.md @@ -7,8 +7,7 @@ Helper module used to manage interactions with Synapse API. **Global Variables** --------------- -- **MJOLNIR_MANAGEMENT_ROOM** -- **MJOLNIR_MEMBERSHIP_ROOM** +- **SYNAPSE_PORT** - **SYNAPSE_URL** - **ADD_USER_ROOM_URL** - **CREATE_ROOM_URL** @@ -16,13 +15,15 @@ Helper module used to manage interactions with Synapse API. - **LIST_ROOMS_URL** - **LIST_USERS_URL** - **LOGIN_URL** +- **MJOLNIR_MANAGEMENT_ROOM** +- **MJOLNIR_MEMBERSHIP_ROOM** - **REGISTER_URL** - **SYNAPSE_VERSION_REGEX** - **VERSION_URL** --- - + ## function `register_user` @@ -60,7 +61,7 @@ Register user. --- - + ## function `get_version` @@ -89,7 +90,7 @@ We're using retry here because after the config change, Synapse is restarted. --- - + ## function `get_access_token` @@ -123,7 +124,7 @@ This is a way to do actions on behalf of a user. --- - + ## function `override_rate_limit` @@ -148,7 +149,7 @@ Override user's rate limit. --- - + ## function `get_room_id` @@ -179,7 +180,7 @@ Get room id. --- - + ## function `deactivate_user` @@ -200,7 +201,7 @@ Deactivate user. --- - + ## function `create_management_room` @@ -230,7 +231,7 @@ Create the management room to be used by Mjolnir. --- - + ## function `make_room_admin` @@ -257,14 +258,14 @@ Make user a room's admin. --- - + ## class `APIError` Exception raised when something fails while calling the API. Attrs: msg (str): Explanation of the error. - + ### method `__init__` @@ -286,12 +287,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `NetworkError` Exception raised when requesting API fails due network issues. - + ### method `__init__` @@ -313,12 +314,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `GetNonceError` Exception raised when getting nonce fails. - + ### method `__init__` @@ -340,12 +341,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `GetVersionError` Exception raised when getting version fails. - + ### method `__init__` @@ -367,12 +368,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `VersionUnexpectedContentError` Exception raised when output of getting version is unexpected. - + ### method `__init__` @@ -394,12 +395,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `GetRoomIDError` Exception raised when getting room id fails. - + ### method `__init__` @@ -421,12 +422,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `GetUserIDError` Exception raised when getting user id fails. - + ### method `__init__` @@ -448,12 +449,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `UserExistsError` Exception raised when checking if user exists fails. - + ### method `__init__` @@ -475,12 +476,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `GetAccessTokenError` Exception raised when getting access token fails. - + ### method `__init__` @@ -502,12 +503,12 @@ Initialize a new instance of the APIError exception. --- - + ## class `RegisterUserError` Exception raised when registering user fails. - + ### method `__init__` diff --git a/src-docs/charm.py.md b/src-docs/charm.py.md index 53da2ad3..b65ba120 100644 --- a/src-docs/charm.py.md +++ b/src-docs/charm.py.md @@ -5,11 +5,6 @@ # module `charm.py` Charm for Synapse on kubernetes. -**Global Variables** ---------------- -- **SYNAPSE_CONTAINER_NAME** -- **SYNAPSE_NGINX_CONTAINER_NAME** -- **SYNAPSE_NGINX_PORT** --- @@ -17,7 +12,7 @@ Charm for Synapse on kubernetes. ## class `SynapseCharm` Charm the service. - + ### function `__init__` @@ -74,7 +69,7 @@ Unit that this execution is responsible for. --- - + ### function `change_config` @@ -86,7 +81,7 @@ Change configuration. --- - + ### function `replan_nginx` diff --git a/src-docs/charm_state.py.md b/src-docs/charm_state.py.md index a3de6b7c..10f6fd0b 100644 --- a/src-docs/charm_state.py.md +++ b/src-docs/charm_state.py.md @@ -17,7 +17,7 @@ Exception raised when a charm configuration is found to be invalid. Attrs: msg (str): Explanation of the error. - + ### function `__init__` @@ -42,143 +42,90 @@ Initialize a new instance of the CharmConfigInvalidError exception. ## class `CharmState` State of the Charm. -Attrs: server_name: server_name config. report_stats: report_stats config. public_baseurl: public_baseurl config. enable_mjolnir: enable_mjolnir config. datasource: datasource information. saml_config: saml configuration. - -### function `__init__` - -```python -__init__( - synapse_config: SynapseConfig, - datasource: Optional[DatasourcePostgreSQL], - saml_config: Optional[SAMLConfiguration] -) → None -``` - -Construct. - - - -**Args:** +**Attributes:** - - `synapse_config`: The value of the synapse_config charm configuration. - - `datasource`: Datasource information. - - `saml_config`: SAML configuration. - - ---- + - `synapse_config`: synapse configuration. + - `datasource`: datasource information. + - `saml_config`: saml configuration. -#### property datasource -Return datasource. - -**Returns:** - datasource or None. - --- -#### property enable_mjolnir - -Return enable_mjolnir config. + +### classmethod `from_charm` +```python +from_charm( + charm: CharmBase, + datasource: Optional[DatasourcePostgreSQL], + saml_config: Optional[SAMLConfiguration] +) → CharmState +``` -**Returns:** - - - `bool`: enable_mjolnir config. - ---- - -#### property public_baseurl - -Return public_baseurl config. +Initialize a new instance of the CharmState class from the associated charm. -**Returns:** +**Args:** - - `str`: public_baseurl config. - ---- - -#### property report_stats + - `charm`: The charm instance associated with this state. + - `datasource`: datasource information to be used by Synapse. + - `saml_config`: saml configuration to be used by Synapse. -Return report_stats config. +Return: The CharmState instance created by the provided charm. -**Returns:** +**Raises:** - - `str`: report_stats config as yes or no. - ---- - -#### property saml_config - -Return SAML configuration. - - + - `CharmConfigInvalidError`: if the charm configuration is invalid. -**Returns:** - SAMLConfiguration or None. --- -#### property server_name - -Return server_name config. - +## class `SynapseConfig` +Represent Synapse builtin configuration values. +Attrs: server_name: server_name config. report_stats: report_stats config. public_baseurl: public_baseurl config. enable_mjolnir: enable_mjolnir config. smtp_enable_tls: enable tls while connecting to SMTP server. smtp_host: SMTP host. smtp_notif_from: defines the "From" address to use when sending emails. smtp_pass: password to authenticate to SMTP host. smtp_port: SMTP port. smtp_user: username to autehtncate to SMTP host. -**Returns:** - - - `str`: server_name config. --- - + -### classmethod `from_charm` +### classmethod `set_default_smtp_notif_from` ```python -from_charm(charm: 'SynapseCharm') → CharmState +set_default_smtp_notif_from( + smtp_notif_from: Optional[str], + values: dict +) → Optional[str] ``` -Initialize a new instance of the CharmState class from the associated charm. +Set server_name as default value to smtp_notif_from. **Args:** - - `charm`: The charm instance associated with this state. - -Return: The CharmState instance created by the provided charm. - - - -**Raises:** - - - `CharmConfigInvalidError`: if the charm configuration is invalid. - - ---- - -## class `SynapseConfig` -Represent Synapse builtin configuration values. - -Attrs: server_name: server_name config. report_stats: report_stats config. public_baseurl: public_baseurl config. enable_mjolnir: enable_mjolnir config. + - `smtp_notif_from`: the smtp_notif_from current value. + - `values`: values already defined. +**Returns:** + The default value for smtp_notif_from if not defined. --- - + ### classmethod `to_yes_or_no` diff --git a/src-docs/database_observer.py.md b/src-docs/database_observer.py.md index dbacc42b..714e6b10 100644 --- a/src-docs/database_observer.py.md +++ b/src-docs/database_observer.py.md @@ -5,9 +5,6 @@ # module `database_observer.py` The Database agent relation observer. -**Global Variables** ---------------- -- **SYNAPSE_CONTAINER_NAME** --- diff --git a/src-docs/mjolnir.py.md b/src-docs/mjolnir.py.md index 011c81d8..54859e8a 100644 --- a/src-docs/mjolnir.py.md +++ b/src-docs/mjolnir.py.md @@ -7,13 +7,10 @@ Provide the Mjolnir class to represent the Mjolnir plugin for Synapse. **Global Variables** --------------- -- **MJOLNIR_MANAGEMENT_ROOM** -- **MJOLNIR_MEMBERSHIP_ROOM** - **MJOLNIR_SERVICE_NAME** - **PEER_RELATION_NAME** - **SECRET_ID** - **SECRET_KEY** -- **SYNAPSE_CONTAINER_NAME** - **USERNAME** @@ -24,7 +21,7 @@ A class representing the Mjolnir plugin for Synapse application. Mjolnir is a moderation tool for Matrix to be used to protect your server from malicious invites, spam messages etc. See https://github.com/matrix-org/mjolnir/ for more details about it. - + ### function `__init__` @@ -52,7 +49,7 @@ Shortcut for more simple access the model. --- - + ### function `create_admin_user` @@ -76,7 +73,7 @@ Create an admin user. --- - + ### function `enable_mjolnir` @@ -100,7 +97,7 @@ The required steps to enable Mjolnir are: --- - + ### function `get_admin_access_token` @@ -117,7 +114,7 @@ Get admin access token. --- - + ### function `get_membership_room_id` diff --git a/src-docs/observability.py.md b/src-docs/observability.py.md index 817341ac..359fa392 100644 --- a/src-docs/observability.py.md +++ b/src-docs/observability.py.md @@ -5,9 +5,6 @@ # module `observability.py` Provide the Observability class to represent the observability stack for Synapse. -**Global Variables** ---------------- -- **PROMETHEUS_TARGET_PORT** --- diff --git a/src-docs/pebble.py.md b/src-docs/pebble.py.md index c38d4edb..a13e9373 100644 --- a/src-docs/pebble.py.md +++ b/src-docs/pebble.py.md @@ -5,17 +5,6 @@ # module `pebble.py` Class to interact with pebble. -**Global Variables** ---------------- -- **CHECK_ALIVE_NAME** -- **CHECK_MJOLNIR_READY_NAME** -- **CHECK_NGINX_READY_NAME** -- **CHECK_READY_NAME** -- **MJOLNIR_CONFIG_PATH** -- **MJOLNIR_SERVICE_NAME** -- **SYNAPSE_COMMAND_PATH** -- **SYNAPSE_CONTAINER_NAME** -- **SYNAPSE_SERVICE_NAME** --- @@ -23,7 +12,7 @@ Class to interact with pebble. ## class `PebbleService` The charm pebble service manager. - + ### function `__init__` @@ -44,7 +33,7 @@ Initialize the pebble service. --- - + ### function `change_config` @@ -68,7 +57,7 @@ Change the configuration. --- - + ### function `enable_saml` @@ -76,7 +65,7 @@ Change the configuration. enable_saml(container: Container) → None ``` -Enable SAML. +Enable SAML while receiving on_saml_data_available event. @@ -92,7 +81,7 @@ Enable SAML. --- - + ### function `replan_mjolnir` @@ -110,7 +99,7 @@ Replan Synapse Mjolnir service. --- - + ### function `replan_nginx` @@ -128,7 +117,7 @@ Replan Synapse NGINX service. --- - + ### function `reset_instance` @@ -152,7 +141,7 @@ Reset instance. --- - + ### function `restart_synapse` @@ -178,7 +167,7 @@ Exception raised when something fails while interacting with Pebble. Attrs: msg (str): Explanation of the error. - + ### function `__init__` diff --git a/src-docs/saml_observer.py.md b/src-docs/saml_observer.py.md index 98830911..07e40c3a 100644 --- a/src-docs/saml_observer.py.md +++ b/src-docs/saml_observer.py.md @@ -5,9 +5,6 @@ # module `saml_observer.py` The SAML integrator relation observer. -**Global Variables** ---------------- -- **SYNAPSE_CONTAINER_NAME** --- From 3969056bec4ef6e64b4c99bf564ffe3c5a846210 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Thu, 28 Sep 2023 17:57:41 -0300 Subject: [PATCH 05/13] Remove log from pebble and fix functions order in workload.py --- src-docs/pebble.py.md | 4 +- src/pebble.py | 1 - src/synapse/workload.py | 198 ++++++++++++++++++++-------------------- 3 files changed, 101 insertions(+), 102 deletions(-) diff --git a/src-docs/pebble.py.md b/src-docs/pebble.py.md index a13e9373..2e685c33 100644 --- a/src-docs/pebble.py.md +++ b/src-docs/pebble.py.md @@ -57,7 +57,7 @@ Change the configuration. --- - + ### function `enable_saml` @@ -117,7 +117,7 @@ Replan Synapse NGINX service. --- - + ### function `reset_instance` diff --git a/src/pebble.py b/src/pebble.py index d51b7c0a..20d70987 100644 --- a/src/pebble.py +++ b/src/pebble.py @@ -90,7 +90,6 @@ def change_config(self, container: ops.model.Container) -> None: logger.debug("pebble.change_config: Enabling SAML") synapse.enable_saml(container=container, charm_state=self._charm_state) if self._charm_state.synapse_config.smtp_host is not None: - logger.debug("pebble.change_config: Enabling SAML") synapse.enable_smtp(container=container, charm_state=self._charm_state) self.restart_synapse(container) except (synapse.WorkloadError, ops.pebble.PathError) as exc: diff --git a/src/synapse/workload.py b/src/synapse/workload.py index 5fa78af5..7adea667 100644 --- a/src/synapse/workload.py +++ b/src/synapse/workload.py @@ -138,6 +138,105 @@ def check_mjolnir_ready() -> ops.pebble.CheckDict: return check.to_dict() +def _get_configuration_field(container: ops.Container, fieldname: str) -> typing.Optional[str]: + """Get configuration field. + + Args: + container: Container of the charm. + fieldname: field to get. + + Raises: + PathError: if somethings goes wrong while reading the configuration file. + + Returns: + configuration field value. + """ + try: + configuration_content = str(container.pull(SYNAPSE_CONFIG_PATH, encoding="utf-8").read()) + value = yaml.safe_load(configuration_content)[fieldname] + return value + 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, + ) + return None + logger.exception( + "exception while reading configuration file %s: %r", + SYNAPSE_CONFIG_PATH, + path_error, + ) + raise + + +def get_registration_shared_secret(container: ops.Container) -> typing.Optional[str]: + """Get registration_shared_secret from configuration file. + + Args: + container: Container of the charm. + + Returns: + registration_shared_secret value. + """ + return _get_configuration_field(container=container, fieldname="registration_shared_secret") + + +def _check_server_name(container: ops.Container, charm_state: CharmState) -> 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. + charm_state: Instance of CharmState. + + Raises: + ServerNameModifiedError: if server_name from state is different than the one in the + configuration file. + """ + configured_server_name = _get_configuration_field(container=container, fieldname="server_name") + if ( + configured_server_name is not None + and configured_server_name != charm_state.synapse_config.server_name + ): + msg = ( + f"server_name {charm_state.synapse_config.server_name} is different from the existing " + f"one {configured_server_name}. Please revert the config or run the action " + "reset-instance if you want 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" + ) + + +def _exec( + container: ops.Container, + command: list[str], + environment: dict[str, str] | None = None, +) -> ExecResult: + """Execute a command inside the Synapse workload container. + + Args: + container: Container of the charm. + command: A list of strings representing the command to be executed. + environment: Environment variables for the command to be executed. + + Returns: + ExecResult: An `ExecResult` object representing the result of the command execution. + """ + exec_process = container.exec(command, environment=environment, working_dir=SYNAPSE_CONFIG_DIR) + try: + stdout, stderr = exec_process.wait_output() + return ExecResult(0, typing.cast(str, stdout), typing.cast(str, stderr)) + except ExecError as exc: + return ExecResult( + exc.exit_code, typing.cast(str, exc.stdout), typing.cast(str, exc.stderr) + ) + + def execute_migrate_config(container: ops.Container, charm_state: CharmState) -> None: """Run the Synapse command migrate_config. @@ -363,18 +462,6 @@ def enable_smtp(container: ops.Container, charm_state: CharmState) -> None: raise WorkloadError(str(exc)) from exc -def get_registration_shared_secret(container: ops.Container) -> typing.Optional[str]: - """Get registration_shared_secret from configuration file. - - Args: - container: Container of the charm. - - Returns: - registration_shared_secret value. - """ - return _get_configuration_field(container=container, fieldname="registration_shared_secret") - - def reset_instance(container: ops.Container) -> None: """Erase data and config server_name. @@ -425,90 +512,3 @@ def get_environment(charm_state: CharmState) -> typing.Dict[str, str]: environment["POSTGRES_USER"] = datasource["user"] environment["POSTGRES_PASSWORD"] = datasource["password"] return environment - - -def _check_server_name(container: ops.Container, charm_state: CharmState) -> 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. - charm_state: Instance of CharmState. - - Raises: - ServerNameModifiedError: if server_name from state is different than the one in the - configuration file. - """ - configured_server_name = _get_configuration_field(container=container, fieldname="server_name") - if ( - configured_server_name is not None - and configured_server_name != charm_state.synapse_config.server_name - ): - msg = ( - f"server_name {charm_state.synapse_config.server_name} is different from the existing " - f"one {configured_server_name}. Please revert the config or run the action " - "reset-instance if you want 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" - ) - - -def _exec( - container: ops.Container, - command: list[str], - environment: dict[str, str] | None = None, -) -> ExecResult: - """Execute a command inside the Synapse workload container. - - Args: - container: Container of the charm. - command: A list of strings representing the command to be executed. - environment: Environment variables for the command to be executed. - - Returns: - ExecResult: An `ExecResult` object representing the result of the command execution. - """ - exec_process = container.exec(command, environment=environment, working_dir=SYNAPSE_CONFIG_DIR) - try: - stdout, stderr = exec_process.wait_output() - return ExecResult(0, typing.cast(str, stdout), typing.cast(str, stderr)) - except ExecError as exc: - return ExecResult( - exc.exit_code, typing.cast(str, exc.stdout), typing.cast(str, exc.stderr) - ) - - -def _get_configuration_field(container: ops.Container, fieldname: str) -> typing.Optional[str]: - """Get configuration field. - - Args: - container: Container of the charm. - fieldname: field to get. - - Raises: - PathError: if somethings goes wrong while reading the configuration file. - - Returns: - configuration field value. - """ - try: - configuration_content = str(container.pull(SYNAPSE_CONFIG_PATH, encoding="utf-8").read()) - value = yaml.safe_load(configuration_content)[fieldname] - return value - 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, - ) - return None - logger.exception( - "exception while reading configuration file %s: %r", - SYNAPSE_CONFIG_PATH, - path_error, - ) - raise From 1f3c4c4940d140e94d0c50b197d9f7224ce203b7 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Fri, 29 Sep 2023 16:12:57 -0300 Subject: [PATCH 06/13] Fix SMTP configuration, add use_existing option to integration tests and cos mark if skip is needed --- src/pebble.py | 2 +- src/synapse/workload.py | 13 ++- tests/conftest.py | 6 ++ tests/integration/conftest.py | 62 ++++++++++++- tests/integration/test_charm.py | 133 ++++++++++++---------------- tests/unit/test_synapse_workload.py | 105 ++++++++++++++++++++++ 6 files changed, 239 insertions(+), 82 deletions(-) diff --git a/src/pebble.py b/src/pebble.py index 20d70987..661f2dcc 100644 --- a/src/pebble.py +++ b/src/pebble.py @@ -89,7 +89,7 @@ def change_config(self, container: ops.model.Container) -> None: if self._charm_state.saml_config is not None: logger.debug("pebble.change_config: Enabling SAML") synapse.enable_saml(container=container, charm_state=self._charm_state) - if self._charm_state.synapse_config.smtp_host is not None: + if self._charm_state.synapse_config.smtp_host: synapse.enable_smtp(container=container, charm_state=self._charm_state) self.restart_synapse(container) except (synapse.WorkloadError, ops.pebble.PathError) as exc: diff --git a/src/synapse/workload.py b/src/synapse/workload.py index 7adea667..e80f0542 100644 --- a/src/synapse/workload.py +++ b/src/synapse/workload.py @@ -451,12 +451,19 @@ def enable_smtp(container: ops.Container, charm_state: CharmState) -> None: config = container.pull(SYNAPSE_CONFIG_PATH).read() current_yaml = yaml.safe_load(config) current_yaml["email"] = {} + # The following three configurations are mandatory for SMTP. current_yaml["email"]["smtp_host"] = charm_state.synapse_config.smtp_host current_yaml["email"]["smtp_port"] = charm_state.synapse_config.smtp_port - current_yaml["email"]["smtp_user"] = charm_state.synapse_config.smtp_user - current_yaml["email"]["smtp_pass"] = charm_state.synapse_config.smtp_pass - current_yaml["email"]["enable_tls"] = charm_state.synapse_config.smtp_enable_tls current_yaml["email"]["notif_from"] = charm_state.synapse_config.smtp_notif_from + if charm_state.synapse_config.smtp_user: + current_yaml["email"]["smtp_user"] = charm_state.synapse_config.smtp_user + if charm_state.synapse_config.smtp_pass: + current_yaml["email"]["smtp_pass"] = charm_state.synapse_config.smtp_pass + if not charm_state.synapse_config.smtp_enable_tls: + # Only set if the user set as false. + # By default, if the server supports TLS, it will be used, + # and the server must present a certificate that is valid for 'smtp_host'. + current_yaml["email"]["enable_tls"] = charm_state.synapse_config.smtp_enable_tls container.push(SYNAPSE_CONFIG_PATH, yaml.safe_dump(current_yaml)) except ops.pebble.PathError as exc: raise WorkloadError(str(exc)) from exc diff --git a/tests/conftest.py b/tests/conftest.py index 5a41c5dc..054f3288 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,3 +20,9 @@ def pytest_addoption(parser: Parser) -> None: SYNAPSE_NGINX_IMAGE_PARAM, action="store", help="Synapse NGINX image to be deployed" ) parser.addoption("--charm-file", action="store", help="Charm file to be deployed") + parser.addoption( + "--use-existing", + action="store_true", + default=False, + help="This parameter will skip deploy of Synapse and PostgreSQL", + ) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 0af5b1b2..a4a07cad 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -6,9 +6,12 @@ import json import typing +from secrets import token_hex import pytest import pytest_asyncio +import requests +from juju.action import Action from juju.application import Application from juju.model import Model from ops.model import ActiveStatus @@ -91,8 +94,12 @@ async def synapse_app_fixture( synapse_charm: str, postgresql_app: Application, postgresql_app_name: str, + pytestconfig: Config, ): """Build and deploy the Synapse charm.""" + use_existing = pytestconfig.getoption("--charm-file") + if use_existing: + return model.applications[synapse_app_name] resources = { "synapse-image": synapse_image, "synapse-nginx-image": synapse_nginx_image, @@ -192,11 +199,12 @@ def postgresql_app_name_app_name_fixture() -> str: @pytest_asyncio.fixture(scope="module", name="postgresql_app") async def postgresql_app_fixture( - ops_test: OpsTest, - model: Model, - postgresql_app_name: str, + ops_test: OpsTest, model: Model, postgresql_app_name: str, pytestconfig: Config ): """Deploy postgresql.""" + use_existing = pytestconfig.getoption("--charm-file") + if use_existing: + return model.applications[postgresql_app_name] async with ops_test.fast_forward(): await model.deploy(postgresql_app_name, channel="14/stable", trust=True) await model.wait_for_idle(status=ACTIVE_STATUS_NAME) @@ -250,3 +258,51 @@ async def deploy_prometheus_fixture( await model.wait_for_idle(raise_on_blocked=True, status=ACTIVE_STATUS_NAME) return app + + +@pytest.fixture(scope="module", name="user_username") +def user_username_fixture() -> typing.Generator[str, None, None]: + """Return the a username to be created for tests.""" + yield token_hex(16) + + +@pytest_asyncio.fixture(scope="module", name="user_password") +async def user_password_fixture( + synapse_app: Application, user_username: str +) -> typing.AsyncGenerator[str, None]: + """Return the a username to be created for tests.""" + action_register_user: Action = await synapse_app.units[0].run_action( # type: ignore + "register-user", username=user_username, admin=True + ) + await action_register_user.wait() + assert action_register_user.status == "completed" + assert action_register_user.results["register-user"] + password = action_register_user.results["user-password"] + assert password + yield password + + +@pytest_asyncio.fixture(scope="module", name="access_token") +async def access_token_fixture( + user_username: str, + user_password: str, + synapse_app: Application, + get_unit_ips: typing.Callable[[str], typing.Awaitable[tuple[str, ...]]], +) -> typing.AsyncGenerator[str, None]: + """Return the access token after login with the username and password.""" + synapse_ip = (await get_unit_ips(synapse_app.name))[0] + # login + sess = requests.session() + res = sess.post( + f"http://{synapse_ip}:8080/_matrix/client/r0/login", + json={ + "identifier": {"type": "m.id.user", "user": user_username}, + "password": user_password, + "type": "m.login.password", + }, + timeout=5, + ) + res.raise_for_status() + access_token = res.json()["access_token"] + assert access_token + yield access_token diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 973eb82f..666de793 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -7,7 +7,6 @@ import logging import re import typing -from secrets import token_hex import pytest import requests @@ -46,6 +45,7 @@ async def test_synapse_is_up( assert "Welcome to the Matrix" in response.text +@pytest.mark.cos @pytest.mark.usefixtures("synapse_app", "prometheus_app") async def test_prometheus_integration( model: Model, @@ -61,7 +61,7 @@ async def test_prometheus_integration( """ await model.add_relation(prometheus_app_name, synapse_app_name) await model.wait_for_idle( - apps=[synapse_app_name, prometheus_app_name], status=ACTIVE_STATUS_NAME + apps=[synapse_app_name, prometheus_app_name], status=ACTIVE_STATUS_NAME, idle_period=5 ) for unit_ip in await get_unit_ips(prometheus_app_name): @@ -69,6 +69,7 @@ async def test_prometheus_integration( assert len(query_targets["data"]["activeTargets"]) +@pytest.mark.cos @pytest.mark.usefixtures("synapse_app", "prometheus_app", "grafana_app") async def test_grafana_integration( model: Model, @@ -164,39 +165,6 @@ async def test_reset_instance_action( assert current_server_name == another_server_name -async def test_register_user_action( - model: Model, - synapse_app: Application, - get_unit_ips: typing.Callable[[str], typing.Awaitable[tuple[str, ...]]], -) -> None: - """ - arrange: a deployed Synapse charm. - act: call the register user action. - assert: the user is registered and the login is successful. - """ - await model.wait_for_idle(status=ACTIVE_STATUS_NAME) - username = "operator" - unit = model.applications[synapse_app.name].units[0] - action_register_user: Action = await synapse_app.units[0].run_action( # type: ignore - "register-user", username=username, admin=True - ) - await action_register_user.wait() - assert action_register_user.status == "completed" - assert action_register_user.results["register-user"] - password = action_register_user.results["user-password"] - assert password - assert unit.workload_status == "active" - data = {"type": "m.login.password", "user": username, "password": password} - for unit_ip in await get_unit_ips(synapse_app.name): - response = requests.post( - f"http://{unit_ip}:{synapse.SYNAPSE_NGINX_PORT}/_matrix/client/r0/login", - json=data, - timeout=5, - ) - assert response.status_code == 200 - assert response.json()["access_token"] - - @pytest.mark.asyncio async def test_workload_version( ops_test: OpsTest, @@ -225,46 +193,18 @@ async def test_workload_version( async def test_synapse_enable_mjolnir( ops_test: OpsTest, synapse_app: Application, + access_token: str, get_unit_ips: typing.Callable[[str], typing.Awaitable[tuple[str, ...]]], ): """ - arrange: build and deploy the Synapse charm, create an user. - act: enable Mjolnir and create the management room. + arrange: build and deploy the Synapse charm, create an user, get the access token, + enable Mjolnir and create the management room. + act: check Mjolnir health point. assert: the Synapse application is active and Mjolnir health point returns a correct response. """ - synapse_ip = (await get_unit_ips(synapse_app.name))[0] - response = requests.get( - f"http://{synapse_ip}:{synapse.SYNAPSE_NGINX_PORT}/_matrix/static/", timeout=5 - ) - assert response.status_code == 200 - assert "Welcome to the Matrix" in response.text - username = token_hex(16) - action_register_user: Action = await synapse_app.units[0].run_action( # type: ignore - "register-user", username=username, admin=True - ) - await action_register_user.wait() - assert action_register_user.status == "completed" - assert action_register_user.results["register-user"] - password = action_register_user.results["user-password"] - await synapse_app.set_config({"enable_mjolnir": "true"}) - await synapse_app.model.wait_for_idle(apps=[synapse_app.name], status="blocked") + await synapse_app.model.wait_for_idle(apps=[synapse_app.name], idle_period=5, status="blocked") synapse_ip = (await get_unit_ips(synapse_app.name))[0] - # login - sess = requests.session() - res = sess.post( - f"http://{synapse_ip}:8080/_matrix/client/r0/login", - json={ - "identifier": {"type": "m.id.user", "user": username}, - "password": password, - "type": "m.login.password", - }, - timeout=5, - ) - res.raise_for_status() - access_token = res.json()["access_token"] - assert access_token - # create the room authorization_token = f"Bearer {access_token}" headers = {"Authorization": authorization_token} room_body = { @@ -275,6 +215,7 @@ async def test_synapse_enable_mjolnir( "room_version": "1", "topic": "moderators", } + sess = requests.session() res = sess.post( f"http://{synapse_ip}:8080/_matrix/client/v3/createRoom", json=room_body, @@ -282,13 +223,14 @@ async def test_synapse_enable_mjolnir( timeout=5, ) res.raise_for_status() - - # wait for idle async with ops_test.fast_forward(): # using fast_forward otherwise would wait for model config update-status-hook-interval - await synapse_app.model.wait_for_idle(apps=[synapse_app.name], status="active") - # check healthz endpoint + await synapse_app.model.wait_for_idle( + idle_period=5, apps=[synapse_app.name], status="active" + ) + res = sess.get(f"http://{synapse_ip}:{synapse.MJOLNIR_HEALTH_PORT}/healthz", timeout=5) + assert res.status_code == 200 @@ -317,7 +259,7 @@ async def test_saml_auth( # pylint: disable=too-many-locals series="jammy", trust=True, ) - await model.wait_for_idle() + await model.wait_for_idle(idle_period=5) saml_helper.prepare_pod(model_name, f"{saml_integrator_app.name}-0") saml_helper.prepare_pod(model_name, f"{synapse_app.name}-0") await saml_integrator_app.set_config( @@ -326,10 +268,10 @@ async def test_saml_auth( # pylint: disable=too-many-locals "metadata_url": f"https://{saml_helper.SAML_HOST}/metadata", } ) - await model.wait_for_idle() + await model.wait_for_idle(idle_period=5) await model.add_relation(saml_integrator_app.name, synapse_app.name) await model.wait_for_idle( - apps=[synapse_app.name, saml_integrator_app.name], status=ACTIVE_STATUS_NAME + apps=[synapse_app.name, saml_integrator_app.name], status=ACTIVE_STATUS_NAME, idle_period=5 ) session = requests.session() @@ -363,3 +305,44 @@ async def test_saml_auth( # pylint: disable=too-many-locals assert logged_in_page.status_code == 200 assert "Continue to your account" in logged_in_page.text + + +async def test_synapse_enable_smtp( + synapse_app: Application, + get_unit_ips: typing.Callable[[str], typing.Awaitable[tuple[str, ...]]], + access_token: str, +): + """ + arrange: build and deploy the Synapse charm, create an user, get the access token + and enable SMTP. + act: try to check if a given email address is not already associated. + assert: the Synapse application is active and the error returned is the one expected. + """ + await synapse_app.set_config({"smtp_host": "127.0.0.1"}) + await synapse_app.model.wait_for_idle( + apps=[synapse_app.name], status=ACTIVE_STATUS_NAME, idle_period=5 + ) + + synapse_ip = (await get_unit_ips(synapse_app.name))[0] + authorization_token = f"Bearer {access_token}" + headers = {"Authorization": authorization_token} + dummy_check = { + "id_server": "id.matrix.org", + "client_secret": "this_is_my_secret_string", + "email": "example@example.com", + "send_attempt": "1", + } + sess = requests.session() + res = sess.post( + f"http://{synapse_ip}:8080/_matrix/client/r0/register/email/requestToken", + json=dummy_check, + headers=headers, + timeout=5, + ) + + assert res.status_code == 500 + # If the configuration change fails, will return something like: + # "Email-based registration has been disabled on this server". + # The expected error confirms that the e-mail is configured but failed since + # is not a real SMTP server. + assert "error was encountered when sending the email" in res.text diff --git a/tests/unit/test_synapse_workload.py b/tests/unit/test_synapse_workload.py index 61047a5c..df6653bc 100644 --- a/tests/unit/test_synapse_workload.py +++ b/tests/unit/test_synapse_workload.py @@ -198,3 +198,108 @@ def test_create_mjolnir_config_success(monkeypatch: pytest.MonkeyPatch): push_mock.assert_called_once_with( synapse.MJOLNIR_CONFIG_PATH, yaml.safe_dump(expected_config), make_dirs=True ) + + +def test_enable_smtp_success(harness: Harness, monkeypatch: pytest.MonkeyPatch): + """ + arrange: set mock container with file. + act: update smtp_host config and call enable_smtp. + assert: new configuration file is pushed and SMTP is enabled. + """ + config_content = """ + listeners: + - type: http + port: 8080 + bind_addresses: + - "::" + """ + text_io_mock = io.StringIO(config_content) + pull_mock = Mock(return_value=text_io_mock) + push_mock = MagicMock() + container_mock = MagicMock() + monkeypatch.setattr(container_mock, "pull", pull_mock) + monkeypatch.setattr(container_mock, "push", push_mock) + + expected_smtp_host = "127.0.0.1" + harness.update_config({"smtp_host": expected_smtp_host}) + harness.begin() + synapse.enable_smtp(container_mock, harness.charm._charm_state) + + assert pull_mock.call_args[0][0] == synapse.SYNAPSE_CONFIG_PATH + assert push_mock.call_args[0][0] == synapse.SYNAPSE_CONFIG_PATH + server_name = harness.charm._charm_state.synapse_config.server_name + expected_config_content = { + "listeners": [ + {"type": "http", "port": 8080, "bind_addresses": ["::"]}, + ], + "email": {"notif_from": server_name, "smtp_host": expected_smtp_host, "smtp_port": 25}, + } + assert push_mock.call_args[0][1] == yaml.safe_dump(expected_config_content) + + +def test_enable_smtp_error(harness: Harness, monkeypatch: pytest.MonkeyPatch): + """ + arrange: set mock container with file. + act: update smtp_host config and call enable_smtp. + assert: raise WorkloadError in case of error. + """ + error_message = "Error pulling file" + path_error = ops.pebble.PathError(kind="fake", message=error_message) + pull_mock = MagicMock(side_effect=path_error) + container_mock = MagicMock() + monkeypatch.setattr(container_mock, "pull", pull_mock) + + with pytest.raises(synapse.WorkloadError, match=error_message): + expected_smtp_host = "127.0.0.1" + harness.update_config({"smtp_host": expected_smtp_host}) + harness.begin() + synapse.enable_smtp(container_mock, harness.charm._charm_state) + + +def test_enable_serve_server_wellknown_success(monkeypatch: pytest.MonkeyPatch): + """ + arrange: set mock container with file. + act: update smtp_host config and call enable_serve_server_wellknown. + assert: new configuration file is pushed and serve_server_wellknown is enabled. + """ + config_content = """ + listeners: + - type: http + port: 8080 + bind_addresses: + - "::" + """ + text_io_mock = io.StringIO(config_content) + pull_mock = Mock(return_value=text_io_mock) + push_mock = MagicMock() + container_mock = MagicMock() + monkeypatch.setattr(container_mock, "pull", pull_mock) + monkeypatch.setattr(container_mock, "push", push_mock) + + synapse.enable_serve_server_wellknown(container_mock) + + assert pull_mock.call_args[0][0] == synapse.SYNAPSE_CONFIG_PATH + assert push_mock.call_args[0][0] == synapse.SYNAPSE_CONFIG_PATH + expected_config_content = { + "listeners": [ + {"type": "http", "port": 8080, "bind_addresses": ["::"]}, + ], + "serve_server_wellknown": True, + } + assert push_mock.call_args[0][1] == yaml.safe_dump(expected_config_content) + + +def test_enable_serve_server_wellknown_error(monkeypatch: pytest.MonkeyPatch): + """ + arrange: set mock container with file. + act: call enable_serve_server_wellknown. + assert: raise WorkloadError. + """ + error_message = "Error pulling file" + path_error = ops.pebble.PathError(kind="fake", message=error_message) + pull_mock = MagicMock(side_effect=path_error) + container_mock = MagicMock() + monkeypatch.setattr(container_mock, "pull", pull_mock) + + with pytest.raises(synapse.WorkloadError, match=error_message): + synapse.enable_serve_server_wellknown(container_mock) From c60f22260619c30f4d146846adb02a576aba19f2 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Fri, 29 Sep 2023 16:36:14 -0300 Subject: [PATCH 07/13] Replace dummy for sample --- tests/integration/test_charm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 666de793..7ddffcf7 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -326,7 +326,7 @@ async def test_synapse_enable_smtp( synapse_ip = (await get_unit_ips(synapse_app.name))[0] authorization_token = f"Bearer {access_token}" headers = {"Authorization": authorization_token} - dummy_check = { + sample_check = { "id_server": "id.matrix.org", "client_secret": "this_is_my_secret_string", "email": "example@example.com", @@ -335,7 +335,7 @@ async def test_synapse_enable_smtp( sess = requests.session() res = sess.post( f"http://{synapse_ip}:8080/_matrix/client/r0/register/email/requestToken", - json=dummy_check, + json=sample_check, headers=headers, timeout=5, ) From 809d1eb95919d734171938451de1d56f79ea0c79 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Fri, 29 Sep 2023 16:49:03 -0300 Subject: [PATCH 08/13] Add unit test --- tests/unit/test_synapse_workload.py | 37 +++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/unit/test_synapse_workload.py b/tests/unit/test_synapse_workload.py index df6653bc..9394a330 100644 --- a/tests/unit/test_synapse_workload.py +++ b/tests/unit/test_synapse_workload.py @@ -303,3 +303,40 @@ def test_enable_serve_server_wellknown_error(monkeypatch: pytest.MonkeyPatch): with pytest.raises(synapse.WorkloadError, match=error_message): synapse.enable_serve_server_wellknown(container_mock) + + +def test_get_registration_shared_secret_success(monkeypatch: pytest.MonkeyPatch): + """ + arrange: set mock container with file. + act: call get_registration_shared_secret. + assert: registration_shared_secret is returned. + """ + expected_secret = token_hex(16) + config_content = f"registration_shared_secret: {expected_secret}" + text_io_mock = io.StringIO(config_content) + pull_mock = Mock(return_value=text_io_mock) + push_mock = MagicMock() + container_mock = MagicMock() + monkeypatch.setattr(container_mock, "pull", pull_mock) + monkeypatch.setattr(container_mock, "push", push_mock) + + received_secret = synapse.get_registration_shared_secret(container_mock) + + assert pull_mock.call_args[0][0] == synapse.SYNAPSE_CONFIG_PATH + assert received_secret == expected_secret + + +def test_get_registration_shared_secret_error(monkeypatch: pytest.MonkeyPatch): + """ + arrange: set mock container with file. + act: call get_registration_shared_secret. + assert: raise WorkloadError. + """ + error_message = "Error pulling file" + path_error = ops.pebble.PathError(kind="fake", message=error_message) + pull_mock = MagicMock(side_effect=path_error) + container_mock = MagicMock() + monkeypatch.setattr(container_mock, "pull", pull_mock) + + with pytest.raises(ops.pebble.PathError, match=error_message): + synapse.get_registration_shared_secret(container_mock) From 1fb5cdd64944d5b0236f81c26b3b33731428b9c6 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Fri, 29 Sep 2023 18:01:36 -0300 Subject: [PATCH 09/13] Fix typo --- tests/integration/conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index a4a07cad..d3d9f669 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -97,7 +97,7 @@ async def synapse_app_fixture( pytestconfig: Config, ): """Build and deploy the Synapse charm.""" - use_existing = pytestconfig.getoption("--charm-file") + use_existing = pytestconfig.getoption("--use-existing", default=False) if use_existing: return model.applications[synapse_app_name] resources = { @@ -202,7 +202,7 @@ async def postgresql_app_fixture( ops_test: OpsTest, model: Model, postgresql_app_name: str, pytestconfig: Config ): """Deploy postgresql.""" - use_existing = pytestconfig.getoption("--charm-file") + use_existing = pytestconfig.getoption("--use-existing", default=False) if use_existing: return model.applications[postgresql_app_name] async with ops_test.fast_forward(): From 49c08a939bf4906a1c180f8d78e6b9085ed731cf Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Mon, 2 Oct 2023 16:17:16 -0300 Subject: [PATCH 10/13] Remove idle_period --- tests/integration/test_charm.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 7224af6d..37ca8a24 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -79,7 +79,7 @@ async def test_prometheus_integration( """ await model.add_relation(prometheus_app_name, synapse_app_name) await model.wait_for_idle( - apps=[synapse_app_name, prometheus_app_name], status=ACTIVE_STATUS_NAME, idle_period=5 + apps=[synapse_app_name, prometheus_app_name], status=ACTIVE_STATUS_NAME ) for unit_ip in await get_unit_ips(prometheus_app_name): @@ -135,6 +135,7 @@ async def test_grafana_integration( assert len(dashboards) +@pytest.mark.nginx @pytest.mark.usefixtures("synapse_app") async def test_nginx_route_integration( model: Model, @@ -221,7 +222,7 @@ async def test_synapse_enable_mjolnir( assert: the Synapse application is active and Mjolnir health point returns a correct response. """ await synapse_app.set_config({"enable_mjolnir": "true"}) - await synapse_app.model.wait_for_idle(apps=[synapse_app.name], idle_period=5, status="blocked") + await synapse_app.model.wait_for_idle(timeout=120, apps=[synapse_app.name], status="blocked") synapse_ip = (await get_unit_ips(synapse_app.name))[0] authorization_token = f"Bearer {access_token}" headers = {"Authorization": authorization_token} @@ -243,15 +244,14 @@ async def test_synapse_enable_mjolnir( res.raise_for_status() async with ops_test.fast_forward(): # using fast_forward otherwise would wait for model config update-status-hook-interval - await synapse_app.model.wait_for_idle( - idle_period=5, apps=[synapse_app.name], status="active" - ) + await synapse_app.model.wait_for_idle(apps=[synapse_app.name], status="active") res = sess.get(f"http://{synapse_ip}:{synapse.MJOLNIR_HEALTH_PORT}/healthz", timeout=5) assert res.status_code == 200 +@pytest.mark.nginx @pytest.mark.asyncio @pytest.mark.usefixtures("nginx_integrator_app") async def test_saml_auth( # pylint: disable=too-many-locals @@ -277,7 +277,7 @@ async def test_saml_auth( # pylint: disable=too-many-locals series="jammy", trust=True, ) - await model.wait_for_idle(idle_period=5) + await model.wait_for_idle() saml_helper.prepare_pod(model_name, f"{saml_integrator_app.name}-0") saml_helper.prepare_pod(model_name, f"{synapse_app.name}-0") await saml_integrator_app.set_config( @@ -286,10 +286,10 @@ async def test_saml_auth( # pylint: disable=too-many-locals "metadata_url": f"https://{saml_helper.SAML_HOST}/metadata", } ) - await model.wait_for_idle(idle_period=5) + await model.wait_for_idle() await model.add_relation(saml_integrator_app.name, synapse_app.name) await model.wait_for_idle( - apps=[synapse_app.name, saml_integrator_app.name], status=ACTIVE_STATUS_NAME, idle_period=5 + apps=[synapse_app.name, saml_integrator_app.name], status=ACTIVE_STATUS_NAME ) session = requests.session() @@ -337,9 +337,7 @@ async def test_synapse_enable_smtp( assert: the Synapse application is active and the error returned is the one expected. """ await synapse_app.set_config({"smtp_host": "127.0.0.1"}) - await synapse_app.model.wait_for_idle( - apps=[synapse_app.name], status=ACTIVE_STATUS_NAME, idle_period=5 - ) + await synapse_app.model.wait_for_idle(apps=[synapse_app.name], status=ACTIVE_STATUS_NAME) synapse_ip = (await get_unit_ips(synapse_app.name))[0] authorization_token = f"Bearer {access_token}" From f3caf3823210e68c09efd2326d2e9b0dd5f60a17 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Mon, 2 Oct 2023 16:44:13 -0300 Subject: [PATCH 11/13] Add test_on_collect_status_no_service test --- tests/unit/test_mjolnir.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/unit/test_mjolnir.py b/tests/unit/test_mjolnir.py index 055e21f0..c9fb83c8 100644 --- a/tests/unit/test_mjolnir.py +++ b/tests/unit/test_mjolnir.py @@ -175,6 +175,27 @@ def test_on_collect_status_service_exists( peer_data_mock.assert_not_called() +def test_on_collect_status_no_service(harness: Harness, monkeypatch: pytest.MonkeyPatch) -> None: + """ + arrange: start the Synapse charm, set server_name, mock get_services to return a empty dict. + act: call _on_collect_status. + assert: no actions is taken because Synapse service is not ready. + """ + harness.update_config({"enable_mjolnir": True}) + harness.begin_with_initial_hooks() + harness.set_leader(True) + container: ops.Container = harness.model.unit.get_container(synapse.SYNAPSE_CONTAINER_NAME) + monkeypatch.setattr(container, "get_services", MagicMock(return_value={})) + peer_data_mock = MagicMock() + monkeypatch.setattr(Mjolnir, "_update_peer_data", peer_data_mock) + + event_mock = MagicMock() + harness.charm._mjolnir._on_collect_status(event_mock) + + peer_data_mock.assert_not_called() + assert isinstance(harness.model.unit.status, ops.MaintenanceStatus) + + def test_on_collect_status_container_off( harness: Harness, monkeypatch: pytest.MonkeyPatch ) -> None: From 3950cfb1c4dc230cd301b49be07db5fd3c23dae9 Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Tue, 3 Oct 2023 08:24:17 -0300 Subject: [PATCH 12/13] Sort variables --- src/charm_state.py | 6 +++--- src/synapse/workload.py | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/charm_state.py b/src/charm_state.py index bd0b2dbb..ac83a930 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -22,10 +22,10 @@ from charm_types import DatasourcePostgreSQL, SAMLConfiguration KNOWN_CHARM_CONFIG = ( - "server_name", - "report_stats", - "public_baseurl", "enable_mjolnir", + "public_baseurl", + "report_stats", + "server_name", "smtp_enable_tls", "smtp_host", "smtp_notif_from", diff --git a/src/synapse/workload.py b/src/synapse/workload.py index e80f0542..5ee4f0c2 100644 --- a/src/synapse/workload.py +++ b/src/synapse/workload.py @@ -153,8 +153,7 @@ def _get_configuration_field(container: ops.Container, fieldname: str) -> typing """ try: configuration_content = str(container.pull(SYNAPSE_CONFIG_PATH, encoding="utf-8").read()) - value = yaml.safe_load(configuration_content)[fieldname] - return value + return yaml.safe_load(configuration_content)[fieldname] except PathError as path_error: if path_error.kind == "not-found": logger.debug( From 024ab4eb04e748c750629c925db31a9daeecc9aa Mon Sep 17 00:00:00 2001 From: Amanda Hager Lopes de Andrade Katz Date: Tue, 3 Oct 2023 10:02:03 -0300 Subject: [PATCH 13/13] Increase idle timeout --- tests/integration/test_charm.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 37ca8a24..8dc92c1a 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -79,7 +79,7 @@ async def test_prometheus_integration( """ await model.add_relation(prometheus_app_name, synapse_app_name) await model.wait_for_idle( - apps=[synapse_app_name, prometheus_app_name], status=ACTIVE_STATUS_NAME + idle_period=30, apps=[synapse_app_name, prometheus_app_name], status=ACTIVE_STATUS_NAME ) for unit_ip in await get_unit_ips(prometheus_app_name): @@ -150,7 +150,7 @@ async def test_nginx_route_integration( """ await model.add_relation(f"{synapse_app_name}", f"{nginx_integrator_app_name}") await nginx_integrator_app.set_config({"service-hostname": synapse_app_name}) - await model.wait_for_idle(status=ACTIVE_STATUS_NAME) + await model.wait_for_idle(idle_period=30, status=ACTIVE_STATUS_NAME) response = requests.get( "http://127.0.0.1/_matrix/static/", headers={"Host": synapse_app_name}, timeout=5 @@ -195,6 +195,7 @@ async def test_workload_version( act: get status from Juju. assert: the workload version is set and match the one given by Synapse API request. """ + await synapse_app.model.wait_for_idle(idle_period=30, apps=[synapse_app.name], status="active") _, status, _ = await ops_test.juju("status", "--format", "json") status = json.loads(status) juju_workload_version = status["applications"][synapse_app.name].get("version", "") @@ -222,7 +223,9 @@ async def test_synapse_enable_mjolnir( assert: the Synapse application is active and Mjolnir health point returns a correct response. """ await synapse_app.set_config({"enable_mjolnir": "true"}) - await synapse_app.model.wait_for_idle(timeout=120, apps=[synapse_app.name], status="blocked") + await synapse_app.model.wait_for_idle( + idle_period=30, timeout=120, apps=[synapse_app.name], status="blocked" + ) synapse_ip = (await get_unit_ips(synapse_app.name))[0] authorization_token = f"Bearer {access_token}" headers = {"Authorization": authorization_token} @@ -244,7 +247,9 @@ async def test_synapse_enable_mjolnir( res.raise_for_status() async with ops_test.fast_forward(): # using fast_forward otherwise would wait for model config update-status-hook-interval - await synapse_app.model.wait_for_idle(apps=[synapse_app.name], status="active") + await synapse_app.model.wait_for_idle( + idle_period=30, apps=[synapse_app.name], status="active" + ) res = sess.get(f"http://{synapse_ip}:{synapse.MJOLNIR_HEALTH_PORT}/healthz", timeout=5) @@ -286,10 +291,12 @@ async def test_saml_auth( # pylint: disable=too-many-locals "metadata_url": f"https://{saml_helper.SAML_HOST}/metadata", } ) - await model.wait_for_idle() + await model.wait_for_idle(idle_period=30) await model.add_relation(saml_integrator_app.name, synapse_app.name) await model.wait_for_idle( - apps=[synapse_app.name, saml_integrator_app.name], status=ACTIVE_STATUS_NAME + idle_period=30, + apps=[synapse_app.name, saml_integrator_app.name], + status=ACTIVE_STATUS_NAME, ) session = requests.session() @@ -337,7 +344,9 @@ async def test_synapse_enable_smtp( assert: the Synapse application is active and the error returned is the one expected. """ await synapse_app.set_config({"smtp_host": "127.0.0.1"}) - await synapse_app.model.wait_for_idle(apps=[synapse_app.name], status=ACTIVE_STATUS_NAME) + await synapse_app.model.wait_for_idle( + idle_period=30, apps=[synapse_app.name], status=ACTIVE_STATUS_NAME + ) synapse_ip = (await get_unit_ips(synapse_app.name))[0] authorization_token = f"Bearer {access_token}"