Remove worker_replication_* deprecated settings, with helpful errors on startup

changelog.d/15860.removal

@@ -0,0 +1 @@
+Remove deprecated `worker_replication_host`, `worker_replication_http_port` and `worker_replication_http_tls` configuration options.

docs/upgrade.md

@@ -87,6 +87,24 @@ process, for example:
     wget https://packages.matrix.org/debian/pool/main/m/matrix-synapse-py3/matrix-synapse-py3_1.3.0+stretch1_amd64.deb
     dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb
     ```
+# Upgrading to v1.88.0
+
+## Removal of `worker_replication_*` settings
+
+As mentioned previously in [Upgrading to v1.84.0](#upgrading-to-v1840), the following deprecated settings
+are being removed in this release of Synapse:
+
+* [`worker_replication_host`](https://matrix-org.github.io/synapse/v1.86/usage/configuration/config_documentation.html#worker_replication_host)
+* [`worker_replication_http_port`](https://matrix-org.github.io/synapse/v1.86/usage/configuration/config_documentation.html#worker_replication_http_port)
+* [`worker_replication_http_tls`](https://matrix-org.github.io/synapse/v1.86/usage/configuration/config_documentation.html#worker_replication_http_tls)
+
+Please ensure that you have migrated to using `main` on your shared configuration's `instance_map`
+(or create one if necessary). This is required if you have ***any*** workers at all;
+administrators of single-process (monolith) installations don't need to do anything.
+
+For an illustrative example, please see [Upgrading to v1.84.0](#upgrading-to-v1840) below.
+
+
 # Upgrading to v1.86.0
 
 ## Minimum supported Rust version

docs/workers.md

@@ -147,7 +147,9 @@ In the config file for each worker, you must specify:
    the main process (`worker_main_http_uri`). This config option is no longer required and is ignored when running Synapse 1.73 and newer.
  * **Synapse 1.83 and older:** The HTTP replication endpoint that the worker should talk to on the main synapse process
    ([`worker_replication_host`](usage/configuration/config_documentation.md#worker_replication_host) and
-   [`worker_replication_http_port`](usage/configuration/config_documentation.md#worker_replication_http_port)). If using Synapse 1.84 and newer, these are not needed if `main` is defined on the [shared configuration](#shared-configuration) `instance_map`
+   [`worker_replication_http_port`](usage/configuration/config_documentation.md#worker_replication_http_port)). If
+   using Synapse 1.84 and newer, these are not needed if `main` is defined on the
+   [shared configuration](#shared-configuration) `instance_map`. **Backwards compatibility fully removed as of Synapse 1.88.0.**
 
 For example:
 

synapse/config/workers.py

@@ -41,11 +41,17 @@
 
 _MISSING_MAIN_PROCESS_INSTANCE_MAP_DATA = """
 Missing data for a worker to connect to main process. Please include '%s' in the
-`instance_map` declared in your shared yaml configuration, or optionally(as a deprecated
-solution) in every worker's yaml as various `worker_replication_*` settings as defined
-in workers documentation here:
+`instance_map` declared in your shared yaml configuration as defined in configuration
+documentation here:
+`https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#instance_map`
+"""
+
+WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE = """
+'%s' is no longer a supported worker setting, please place '%s' onto your shared
+configuration under `main` inside the `instance_map`. See workers documentation here:
 `https://matrix-org.github.io/synapse/latest/workers.html#worker-configuration`
 """
+
 # This allows for a handy knob when it's time to change from 'master' to
 # something with less 'history'
 MAIN_PROCESS_INSTANCE_NAME = "master"
@@ -216,22 +222,37 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
         )
 
         # A map from instance name to host/port of their HTTP replication endpoint.
-        # Check if the main process is declared. Inject it into the map if it's not,
-        # based first on if a 'main' block is declared then on 'worker_replication_*'
-        # data. If both are available, default to instance_map. The main process
-        # itself doesn't need this data as it would never have to talk to itself.
+        # Check if the main process is declared. The main process itself doesn't need
+        # this data as it would never have to talk to itself.
         instance_map: Dict[str, Any] = config.get("instance_map", {})
 
         if self.instance_name is not MAIN_PROCESS_INSTANCE_NAME:
+            # TODO: The next 3 condition blocks can be deleted after some time has
+            #  passed and we're ready to stop checking for these settings.
             # The host used to connect to the main synapse
             main_host = config.get("worker_replication_host", None)
+            if main_host:
+                raise ConfigError(
+                    WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE
+                    % ("worker_replication_host", main_host)
+                )
 
             # The port on the main synapse for HTTP replication endpoint
             main_port = config.get("worker_replication_http_port")
+            if main_port:
+                raise ConfigError(
+                    WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE
+                    % ("worker_replication_http_port", main_port)
+                )
 
             # The tls mode on the main synapse for HTTP replication endpoint.
             # For backward compatibility this defaults to False.
             main_tls = config.get("worker_replication_http_tls", False)
+            if main_tls:
+                raise ConfigError(
+                    WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE
+                    % ("worker_replication_http_tls", main_tls)
+                )
 
             # For now, accept 'main' in the instance_map, but the replication system
             # expects 'master', force that into being until it's changed later.
@@ -241,22 +262,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
                 ]
                 del instance_map[MAIN_PROCESS_INSTANCE_MAP_NAME]
 
-            # This is the backwards compatibility bit that handles the
-            # worker_replication_* bits using setdefault() to not overwrite anything.
-            elif main_host is not None and main_port is not None:
-                instance_map.setdefault(
-                    MAIN_PROCESS_INSTANCE_NAME,
-                    {
-                        "host": main_host,
-                        "port": main_port,
-                        "tls": main_tls,
-                    },
-                )
-
             else:
                 # If we've gotten here, it means that the main process is not on the
-                # instance_map and that not enough worker_replication_* variables
-                # were declared in the worker's yaml.
+                # instance_map.
                 raise ConfigError(
                     _MISSING_MAIN_PROCESS_INSTANCE_MAP_DATA
                     % MAIN_PROCESS_INSTANCE_MAP_NAME

tests/app/test_homeserver_start.py

@@ -25,9 +25,9 @@ def test_wrong_start_caught(self) -> None:
         # Add a blank line as otherwise the next addition ends up on a line with a comment
         self.add_lines_to_config(["  "])
         self.add_lines_to_config(["worker_app: test_worker_app"])
-        self.add_lines_to_config(["worker_replication_host: 127.0.0.1"])
-        self.add_lines_to_config(["worker_replication_http_port: 0"])
-
+        self.add_lines_to_config(["worker_log_config: /data/logconfig.config"])
+        self.add_lines_to_config(["instance_map:"])
+        self.add_lines_to_config(["  main:", "    host: 127.0.0.1", "    port: 1234"])
         # Ensure that starting master process with worker config raises an exception
         with self.assertRaises(ConfigError):
             synapse.app.homeserver.setup(["-c", self.config_file])

tests/config/test_workers.py

@@ -17,7 +17,7 @@
 from immutabledict import immutabledict
 
 from synapse.config import ConfigError
-from synapse.config.workers import InstanceLocationConfig, WorkerConfig
+from synapse.config.workers import WorkerConfig
 
 from tests.unittest import TestCase
 
@@ -323,28 +323,3 @@ def test_worker_duty_configs(self) -> None:
         )
         self.assertTrue(worker2_config.should_notify_appservices)
         self.assertFalse(worker2_config.should_update_user_directory)
-
-    def test_worker_instance_map_compat(self) -> None:
-        """
-        Test that `worker_replication_*` settings are compatibly handled by
-        adding them to the instance map as a `main` entry.
-        """
-
-        worker1_config = self._make_worker_config(
-            worker_app="synapse.app.generic_worker",
-            worker_name="worker1",
-            extras={
-                "notify_appservices_from_worker": "worker2",
-                "update_user_directory_from_worker": "worker1",
-                "worker_replication_host": "127.0.0.42",
-                "worker_replication_http_port": 1979,
-            },
-        )
-        self.assertEqual(
-            worker1_config.instance_map,
-            {
-                "master": InstanceLocationConfig(
-                    host="127.0.0.42", port=1979, tls=False
-                ),
-            },
-        )