From 38abbe0889c9a77fbd2d3422b5197140515944bc Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Thu, 21 Oct 2021 15:17:17 +0300 Subject: [PATCH 1/6] Fix synapse.config module "read" command `synapse.config.__main__` has the possibility to read a config item. This can be used to conveniently also validate the config is valid before trying to start Synapse. The "read" command broke in https://github.com/matrix-org/synapse/pull/10916 as it now requires passing in "server.server_name" for example. Also made the read command optional so one can just call this with just the confirm file reference and get a "Config parses OK" if things are ok. Signed-off-by: Jason Robinson --- changelog.d/11145.bugfix | 1 + synapse/config/__main__.py | 28 ++++++++++++++++++---------- 2 files changed, 19 insertions(+), 10 deletions(-) create mode 100644 changelog.d/11145.bugfix diff --git a/changelog.d/11145.bugfix b/changelog.d/11145.bugfix new file mode 100644 index 000000000000..8249fd64976e --- /dev/null +++ b/changelog.d/11145.bugfix @@ -0,0 +1 @@ +Fix synapse.config module "read" command. diff --git a/synapse/config/__main__.py b/synapse/config/__main__.py index b5b6735a8faa..21137c7dcbdc 100644 --- a/synapse/config/__main__.py +++ b/synapse/config/__main__.py @@ -1,4 +1,5 @@ # Copyright 2015, 2016 OpenMarket Ltd +# Copyright 2021 The Matrix.org Foundation C.I.C. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -18,18 +19,25 @@ from synapse.config.homeserver import HomeServerConfig - action = sys.argv[1] + action = sys.argv[1] if len(sys.argv) > 1 and sys.argv[1] == "read" else None + load_config_args = sys.argv[3:] if action else sys.argv[1:] + + try: + config = HomeServerConfig.load_config("", load_config_args) + except ConfigError as e: + sys.stderr.write("\n" + str(e) + "\n") + sys.exit(1) + + print("Config parses OK!") if action == "read": key = sys.argv[2] - try: - config = HomeServerConfig.load_config("", sys.argv[3:]) - except ConfigError as e: - sys.stderr.write("\n" + str(e) + "\n") - sys.exit(1) + key_parts = key.split(".") - print(getattr(config, key)) + value = config + while len(key_parts): + value = getattr(value, key_parts[0]) + key_parts.pop(0) + + print(f"{key}: {value}") sys.exit(0) - else: - sys.stderr.write("Unknown command %r\n" % (action,)) - sys.exit(1) From 757b1a531af2cf5f61dc9b52a6b0475f253d10e3 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Thu, 21 Oct 2021 16:15:45 +0300 Subject: [PATCH 2/6] Update synapse/config/__main__.py Co-authored-by: Brendan Abolivier --- synapse/config/__main__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/config/__main__.py b/synapse/config/__main__.py index 21137c7dcbdc..a97fadcf3024 100644 --- a/synapse/config/__main__.py +++ b/synapse/config/__main__.py @@ -20,6 +20,9 @@ from synapse.config.homeserver import HomeServerConfig action = sys.argv[1] if len(sys.argv) > 1 and sys.argv[1] == "read" else None + # If we're reading a key in the config file, then `sys.argv[1]` will be `read` and `sys.argv[2]` + # will be the key to read. + # We'll want to rework this code if we want to support more actions than just `read`. load_config_args = sys.argv[3:] if action else sys.argv[1:] try: From d53d65e7403ae51269b5432348949095689775f3 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Thu, 21 Oct 2021 20:57:37 +0300 Subject: [PATCH 3/6] Add more informative error on no key found Signed-off-by: Jason Robinson --- synapse/config/__main__.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/synapse/config/__main__.py b/synapse/config/__main__.py index a97fadcf3024..4513c0e03577 100644 --- a/synapse/config/__main__.py +++ b/synapse/config/__main__.py @@ -38,9 +38,15 @@ key_parts = key.split(".") value = config - while len(key_parts): - value = getattr(value, key_parts[0]) - key_parts.pop(0) - - print(f"{key}: {value}") - sys.exit(0) + try: + while len(key_parts): + value = getattr(value, key_parts[0]) + key_parts.pop(0) + + print(f"\n{key}: {value}") + sys.exit(0) + except AttributeError: + print( + f"\nNo '{key}' key could be found in the provided configuration file." + ) + sys.exit(1) From f5952d25860ef69f4941aafdbfc333fdf47afb4d Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Thu, 21 Oct 2021 21:50:50 +0300 Subject: [PATCH 4/6] Add tests for synapse.config.__main__ Few basic tests to parse a config file when no arguments and config key read success/fail. Refactor test_load.py a bit to provide a common test case class for the config tests using a generated config file to parse. Signed-off-by: Jason Robinson --- synapse/config/__main__.py | 21 ++++++------ tests/config/test___main__.py | 31 ++++++++++++++++++ tests/config/test_load.py | 61 +++++++++-------------------------- tests/config/utils.py | 58 +++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 54 deletions(-) create mode 100644 tests/config/test___main__.py create mode 100644 tests/config/utils.py diff --git a/synapse/config/__main__.py b/synapse/config/__main__.py index 4513c0e03577..c555f5f91407 100644 --- a/synapse/config/__main__.py +++ b/synapse/config/__main__.py @@ -12,18 +12,18 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from synapse.config._base import ConfigError +import sys -if __name__ == "__main__": - import sys +from synapse.config._base import ConfigError +from synapse.config.homeserver import HomeServerConfig - from synapse.config.homeserver import HomeServerConfig - action = sys.argv[1] if len(sys.argv) > 1 and sys.argv[1] == "read" else None - # If we're reading a key in the config file, then `sys.argv[1]` will be `read` and `sys.argv[2]` +def main(args): + action = args[1] if len(args) > 1 and args[1] == "read" else None + # If we're reading a key in the config file, then `args[1]` will be `read` and `args[2]` # will be the key to read. # We'll want to rework this code if we want to support more actions than just `read`. - load_config_args = sys.argv[3:] if action else sys.argv[1:] + load_config_args = args[3:] if action else args[1:] try: config = HomeServerConfig.load_config("", load_config_args) @@ -34,7 +34,7 @@ print("Config parses OK!") if action == "read": - key = sys.argv[2] + key = args[2] key_parts = key.split(".") value = config @@ -44,9 +44,12 @@ key_parts.pop(0) print(f"\n{key}: {value}") - sys.exit(0) except AttributeError: print( f"\nNo '{key}' key could be found in the provided configuration file." ) sys.exit(1) + + +if __name__ == "__main__": + main(sys.argv) diff --git a/tests/config/test___main__.py b/tests/config/test___main__.py new file mode 100644 index 000000000000..b1c73d36124f --- /dev/null +++ b/tests/config/test___main__.py @@ -0,0 +1,31 @@ +# Copyright 2021 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from synapse.config.__main__ import main + +from tests.config.utils import ConfigFileTestCase + + +class ConfigMainFileTestCase(ConfigFileTestCase): + def test_executes_without_an_action(self): + self.generate_config() + main(["", "-c", self.config_file]) + + def test_read__error_if_key_not_found(self): + self.generate_config() + with self.assertRaises(SystemExit): + main(["", "read", "foo.bar.hello", "-c", self.config_file]) + + def test_read__passes_if_key_found(self): + self.generate_config() + main(["", "read", "server.server_name", "-c", self.config_file]) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index 59635de205bb..34aee759b636 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -1,4 +1,5 @@ # Copyright 2016 OpenMarket Ltd +# Copyright 2021 The Matrix.org Foundation C.I.C. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -14,21 +15,19 @@ import os.path import shutil import tempfile -from contextlib import redirect_stdout -from io import StringIO import yaml from synapse.config import ConfigError from synapse.config.homeserver import HomeServerConfig -from tests import unittest +from tests.config.utils import ConfigFileTestCase -class ConfigLoadingTestCase(unittest.TestCase): +class ConfigLoadingFileTestCase(ConfigFileTestCase): def setUp(self): self.dir = tempfile.mkdtemp() - self.file = os.path.join(self.dir, "homeserver.yaml") + self.config_file = os.path.join(self.dir, "homeserver.yaml") def tearDown(self): shutil.rmtree(self.dir) @@ -36,18 +35,18 @@ def tearDown(self): def test_load_fails_if_server_name_missing(self): self.generate_config_and_remove_lines_containing("server_name") with self.assertRaises(ConfigError): - HomeServerConfig.load_config("", ["-c", self.file]) + HomeServerConfig.load_config("", ["-c", self.config_file]) with self.assertRaises(ConfigError): - HomeServerConfig.load_or_generate_config("", ["-c", self.file]) + HomeServerConfig.load_or_generate_config("", ["-c", self.config_file]) def test_generates_and_loads_macaroon_secret_key(self): self.generate_config() - with open(self.file) as f: + with open(self.config_file) as f: raw = yaml.safe_load(f) self.assertIn("macaroon_secret_key", raw) - config = HomeServerConfig.load_config("", ["-c", self.file]) + config = HomeServerConfig.load_config("", ["-c", self.config_file]) self.assertTrue( hasattr(config.key, "macaroon_secret_key"), "Want config to have attr macaroon_secret_key", @@ -58,7 +57,7 @@ def test_generates_and_loads_macaroon_secret_key(self): "was: %r" % (config.key.macaroon_secret_key,) ) - config = HomeServerConfig.load_or_generate_config("", ["-c", self.file]) + config = HomeServerConfig.load_or_generate_config("", ["-c", self.config_file]) self.assertTrue( hasattr(config.key, "macaroon_secret_key"), "Want config to have attr macaroon_secret_key", @@ -71,9 +70,9 @@ def test_generates_and_loads_macaroon_secret_key(self): def test_load_succeeds_if_macaroon_secret_key_missing(self): self.generate_config_and_remove_lines_containing("macaroon") - config1 = HomeServerConfig.load_config("", ["-c", self.file]) - config2 = HomeServerConfig.load_config("", ["-c", self.file]) - config3 = HomeServerConfig.load_or_generate_config("", ["-c", self.file]) + config1 = HomeServerConfig.load_config("", ["-c", self.config_file]) + config2 = HomeServerConfig.load_config("", ["-c", self.config_file]) + config3 = HomeServerConfig.load_or_generate_config("", ["-c", self.config_file]) self.assertEqual( config1.key.macaroon_secret_key, config2.key.macaroon_secret_key ) @@ -87,15 +86,15 @@ def test_disable_registration(self): ["enable_registration: true", "disable_registration: true"] ) # Check that disable_registration clobbers enable_registration. - config = HomeServerConfig.load_config("", ["-c", self.file]) + config = HomeServerConfig.load_config("", ["-c", self.config_file]) self.assertFalse(config.registration.enable_registration) - config = HomeServerConfig.load_or_generate_config("", ["-c", self.file]) + config = HomeServerConfig.load_or_generate_config("", ["-c", self.config_file]) self.assertFalse(config.registration.enable_registration) # Check that either config value is clobbered by the command line. config = HomeServerConfig.load_or_generate_config( - "", ["-c", self.file, "--enable-registration"] + "", ["-c", self.config_file, "--enable-registration"] ) self.assertTrue(config.registration.enable_registration) @@ -104,33 +103,5 @@ def test_stats_enabled(self): self.add_lines_to_config(["enable_metrics: true"]) # The default Metrics Flags are off by default. - config = HomeServerConfig.load_config("", ["-c", self.file]) + config = HomeServerConfig.load_config("", ["-c", self.config_file]) self.assertFalse(config.metrics.metrics_flags.known_servers) - - def generate_config(self): - with redirect_stdout(StringIO()): - HomeServerConfig.load_or_generate_config( - "", - [ - "--generate-config", - "-c", - self.file, - "--report-stats=yes", - "-H", - "lemurs.win", - ], - ) - - def generate_config_and_remove_lines_containing(self, needle): - self.generate_config() - - with open(self.file) as f: - contents = f.readlines() - contents = [line for line in contents if needle not in line] - with open(self.file, "w") as f: - f.write("".join(contents)) - - def add_lines_to_config(self, lines): - with open(self.file, "a") as f: - for line in lines: - f.write(line + "\n") diff --git a/tests/config/utils.py b/tests/config/utils.py new file mode 100644 index 000000000000..94c18a052ba4 --- /dev/null +++ b/tests/config/utils.py @@ -0,0 +1,58 @@ +# Copyright 2021 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import os +import shutil +import tempfile +import unittest +from contextlib import redirect_stdout +from io import StringIO + +from synapse.config.homeserver import HomeServerConfig + + +class ConfigFileTestCase(unittest.TestCase): + def setUp(self): + self.dir = tempfile.mkdtemp() + self.config_file = os.path.join(self.dir, "homeserver.yaml") + + def tearDown(self): + shutil.rmtree(self.dir) + + def generate_config(self): + with redirect_stdout(StringIO()): + HomeServerConfig.load_or_generate_config( + "", + [ + "--generate-config", + "-c", + self.config_file, + "--report-stats=yes", + "-H", + "lemurs.win", + ], + ) + + def generate_config_and_remove_lines_containing(self, needle): + self.generate_config() + + with open(self.config_file) as f: + contents = f.readlines() + contents = [line for line in contents if needle not in line] + with open(self.config_file, "w") as f: + f.write("".join(contents)) + + def add_lines_to_config(self, lines): + with open(self.config_file, "a") as f: + for line in lines: + f.write(line + "\n") From bb5477f381c10d19ea51992de1a44f6146519a23 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Fri, 22 Oct 2021 12:21:19 +0300 Subject: [PATCH 5/6] Update changelog.d/11145.bugfix Co-authored-by: Brendan Abolivier --- changelog.d/11145.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11145.bugfix b/changelog.d/11145.bugfix index 8249fd64976e..f369feac4200 100644 --- a/changelog.d/11145.bugfix +++ b/changelog.d/11145.bugfix @@ -1 +1 @@ -Fix synapse.config module "read" command. +Fix a bug introduced in Synapse v1.45.0 breaking the configuration file parsing script. From 94767cb247e3de7960e46327a368125a8ac2bde4 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Fri, 22 Oct 2021 12:21:46 +0300 Subject: [PATCH 6/6] Remove unnecessary methods from ConfigLoadingFileTestCase Signed-off-by: Jason Robinson --- tests/config/test_load.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/config/test_load.py b/tests/config/test_load.py index 34aee759b636..765258c47ad7 100644 --- a/tests/config/test_load.py +++ b/tests/config/test_load.py @@ -12,10 +12,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import os.path -import shutil -import tempfile - import yaml from synapse.config import ConfigError @@ -25,13 +21,6 @@ class ConfigLoadingFileTestCase(ConfigFileTestCase): - def setUp(self): - self.dir = tempfile.mkdtemp() - self.config_file = os.path.join(self.dir, "homeserver.yaml") - - def tearDown(self): - shutil.rmtree(self.dir) - def test_load_fails_if_server_name_missing(self): self.generate_config_and_remove_lines_containing("server_name") with self.assertRaises(ConfigError):