From 37e00caf5d7235f035d445ec1adbef9377afb347 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 28 Jul 2023 20:18:11 +0100 Subject: [PATCH 01/16] refactor: extract SysctlRunner from SysctlProtocol --- tests/sysctl/test_sysctl.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/sysctl/test_sysctl.py b/tests/sysctl/test_sysctl.py index 9ae554b9e..4a1ab75e6 100644 --- a/tests/sysctl/test_sysctl.py +++ b/tests/sysctl/test_sysctl.py @@ -177,7 +177,7 @@ def test_proto_get_tuple(self) -> None: def test_proto_get_unknown(self) -> None: self.proto.lineReceived(b'net.unknown') - self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] path net.unknown not found\n', self.tr.value()) def test_proto_get_readonly(self) -> None: self.proto.lineReceived(b'net.readonly') @@ -185,7 +185,7 @@ def test_proto_get_readonly(self) -> None: def test_proto_get_writeonly(self) -> None: self.proto.lineReceived(b'core.writeonly') - self.assertEqual(b'[error] cannot read from core.writeonly\n', self.tr.value()) + self.assertEqual(b'[error] cannot read from the path core.writeonly\n', self.tr.value()) ################## # Protocol: Set @@ -205,11 +205,11 @@ def test_proto_set_str(self) -> None: def test_proto_set_readonly(self) -> None: self.proto.lineReceived(b'net.readonly=0.50') - self.assertEqual(b'[error] cannot write to net.readonly\n', self.tr.value()) + self.assertEqual(b'[error] cannot write to the path net.readonly\n', self.tr.value()) def test_proto_set_unknown(self) -> None: self.proto.lineReceived(b'net.unknown=0.50') - self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] path net.unknown not found\n', self.tr.value()) def test_proto_set_tuple(self) -> None: self.proto.lineReceived(b'net.rate_limit=8, 2') From a07043cc96eca79aef094f6f0b60f8a5c5e2b536 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Thu, 3 Aug 2023 16:01:47 +0100 Subject: [PATCH 02/16] chore: remove path word lsfrom error messages --- tests/sysctl/test_sysctl.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/sysctl/test_sysctl.py b/tests/sysctl/test_sysctl.py index 4a1ab75e6..9ae554b9e 100644 --- a/tests/sysctl/test_sysctl.py +++ b/tests/sysctl/test_sysctl.py @@ -177,7 +177,7 @@ def test_proto_get_tuple(self) -> None: def test_proto_get_unknown(self) -> None: self.proto.lineReceived(b'net.unknown') - self.assertEqual(b'[error] path net.unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) def test_proto_get_readonly(self) -> None: self.proto.lineReceived(b'net.readonly') @@ -185,7 +185,7 @@ def test_proto_get_readonly(self) -> None: def test_proto_get_writeonly(self) -> None: self.proto.lineReceived(b'core.writeonly') - self.assertEqual(b'[error] cannot read from the path core.writeonly\n', self.tr.value()) + self.assertEqual(b'[error] cannot read from core.writeonly\n', self.tr.value()) ################## # Protocol: Set @@ -205,11 +205,11 @@ def test_proto_set_str(self) -> None: def test_proto_set_readonly(self) -> None: self.proto.lineReceived(b'net.readonly=0.50') - self.assertEqual(b'[error] cannot write to the path net.readonly\n', self.tr.value()) + self.assertEqual(b'[error] cannot write to net.readonly\n', self.tr.value()) def test_proto_set_unknown(self) -> None: self.proto.lineReceived(b'net.unknown=0.50') - self.assertEqual(b'[error] path net.unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) def test_proto_set_tuple(self) -> None: self.proto.lineReceived(b'net.rate_limit=8, 2') From 771b4115f4617194702a39d2a51d0f579795eb65 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 4 Aug 2023 16:45:51 +0100 Subject: [PATCH 03/16] chore: treats the exceptions in the sysctl protocol --- tests/sysctl/test_sysctl.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/sysctl/test_sysctl.py b/tests/sysctl/test_sysctl.py index 9ae554b9e..b3cd3cf31 100644 --- a/tests/sysctl/test_sysctl.py +++ b/tests/sysctl/test_sysctl.py @@ -177,7 +177,7 @@ def test_proto_get_tuple(self) -> None: def test_proto_get_unknown(self) -> None: self.proto.lineReceived(b'net.unknown') - self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] unknown not found\n', self.tr.value()) def test_proto_get_readonly(self) -> None: self.proto.lineReceived(b'net.readonly') @@ -209,7 +209,7 @@ def test_proto_set_readonly(self) -> None: def test_proto_set_unknown(self) -> None: self.proto.lineReceived(b'net.unknown=0.50') - self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] unknown not found\n', self.tr.value()) def test_proto_set_tuple(self) -> None: self.proto.lineReceived(b'net.rate_limit=8, 2') From 7125b3d3d3192f141165b882050e5e1b6fa5d615 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 25 Aug 2023 18:10:47 +0100 Subject: [PATCH 04/16] refactor: extract get_line_parts function --- tests/sysctl/test_sysctl.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/sysctl/test_sysctl.py b/tests/sysctl/test_sysctl.py index b3cd3cf31..9ae554b9e 100644 --- a/tests/sysctl/test_sysctl.py +++ b/tests/sysctl/test_sysctl.py @@ -177,7 +177,7 @@ def test_proto_get_tuple(self) -> None: def test_proto_get_unknown(self) -> None: self.proto.lineReceived(b'net.unknown') - self.assertEqual(b'[error] unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) def test_proto_get_readonly(self) -> None: self.proto.lineReceived(b'net.readonly') @@ -209,7 +209,7 @@ def test_proto_set_readonly(self) -> None: def test_proto_set_unknown(self) -> None: self.proto.lineReceived(b'net.unknown=0.50') - self.assertEqual(b'[error] unknown not found\n', self.tr.value()) + self.assertEqual(b'[error] net.unknown not found\n', self.tr.value()) def test_proto_set_tuple(self) -> None: self.proto.lineReceived(b'net.rate_limit=8, 2') From 41b71126d9c51f90fcb29fc0fbabd14c2c363bb8 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Thu, 20 Jul 2023 21:41:59 +0100 Subject: [PATCH 05/16] feat: add --sysctl-config-file as cli command option --- hathor/builder/sysctl_builder.py | 6 ++++++ hathor/cli/run_node.py | 9 ++++---- hathor/sysctl/init_file_loader.py | 9 +++++--- hathor/sysctl/runner.py | 10 +++++++++ tests/cli/test_sysctl_init.py | 34 +++++++++---------------------- 5 files changed, 37 insertions(+), 31 deletions(-) diff --git a/hathor/builder/sysctl_builder.py b/hathor/builder/sysctl_builder.py index 46c189ebd..08fa86ec0 100644 --- a/hathor/builder/sysctl_builder.py +++ b/hathor/builder/sysctl_builder.py @@ -12,15 +12,21 @@ # See the License for the specific language governing permissions and # limitations under the License. +from typing import Optional + from hathor.builder import BuildArtifacts +from hathor.conf.settings import HathorSettings from hathor.sysctl import ConnectionsManagerSysctl, Sysctl, WebsocketManagerSysctl +settings = HathorSettings + class SysctlBuilder: """Builder for the sysctl tree.""" def __init__(self, artifacts: BuildArtifacts) -> None: self.artifacts = artifacts + self.sysctl_init_file: Optional[str] = None def build(self) -> Sysctl: """Build the sysctl tree.""" diff --git a/hathor/cli/run_node.py b/hathor/cli/run_node.py index 00ab40956..414827690 100644 --- a/hathor/cli/run_node.py +++ b/hathor/cli/run_node.py @@ -373,7 +373,8 @@ def __init__(self, *, argv=None): self.prepare() self.register_signal_handlers() if self._args.sysctl: - self.init_sysctl(self._args.sysctl, self._args.sysctl_init_file) + self.init_sysctl( + self._args.sysctl, self._args.sysctl_init_file) def init_sysctl(self, description: str, sysctl_init_file: Optional[str] = None) -> None: """Initialize sysctl, listen for connections and apply settings from config file if required. @@ -393,14 +394,14 @@ def init_sysctl(self, description: str, sysctl_init_file: Optional[str] = None) from hathor.sysctl.factory import SysctlFactory from hathor.sysctl.init_file_loader import SysctlInitFileLoader from hathor.sysctl.runner import SysctlRunner + from hathor.sysctl.init_file_loader import SysctlInitFileLoader builder = SysctlBuilder(self.artifacts) root = builder.build() runner = SysctlRunner(root) - if sysctl_init_file: - init_file_loader = SysctlInitFileLoader(runner, sysctl_init_file) - init_file_loader.load() + init_file_loader = SysctlInitFileLoader(runner, sysctl_init_file) + init_file_loader.load() factory = SysctlFactory(runner) endpoint = serverFromString(self.reactor, description) diff --git a/hathor/sysctl/init_file_loader.py b/hathor/sysctl/init_file_loader.py index 9bf09efb6..ac6bdb1ed 100644 --- a/hathor/sysctl/init_file_loader.py +++ b/hathor/sysctl/init_file_loader.py @@ -1,16 +1,19 @@ +from typing import Optional + from hathor.sysctl.runner import SysctlRunner class SysctlInitFileLoader: - def __init__(self, runner: SysctlRunner, init_file: str) -> None: + def __init__(self, runner: SysctlRunner, init_file: Optional[str]) -> None: assert runner - assert init_file self.runner = runner self.init_file = init_file def load(self) -> None: - """Read the init_file and execute each line as a syctl command in the runner.""" + if not self.init_file: + return + with open(self.init_file, 'r', encoding='utf-8') as file: lines = file.readlines() diff --git a/hathor/sysctl/runner.py b/hathor/sysctl/runner.py index ef75a21b6..1080535e2 100644 --- a/hathor/sysctl/runner.py +++ b/hathor/sysctl/runner.py @@ -13,6 +13,7 @@ # limitations under the License. import json +import re from typing import TYPE_CHECKING, Any from hathor.sysctl.exception import SysctlRunnerException @@ -20,6 +21,12 @@ if TYPE_CHECKING: from hathor.sysctl.sysctl import Sysctl +# - It starts with an opening square bracket [. +# - It ends with a closing square bracket ]. +# - The elements are separated by commas and can be followed by optional whitespace. +# - There can be zero or more elements in the array (an empty array is allowed). +array_pattern = r'^\s*\[\s*(?:[^\[\],]+(?:\s*,\s*[^\[\],]+)*)?\s*\]\s*$' + class SysctlRunner: """ Encapsulates the Sysctl to decouple it from the SyctlProtocol. @@ -76,6 +83,9 @@ def deserialize(self, value_str: str) -> Any: if len(value_str) == 0: return () + if re.match(array_pattern, value_str): + return list(json.loads(value_str)) + parts = [x.strip() for x in value_str.split(',')] if len(parts) > 1: return tuple(json.loads(x) for x in parts) diff --git a/tests/cli/test_sysctl_init.py b/tests/cli/test_sysctl_init.py index a696e2008..653f8526a 100644 --- a/tests/cli/test_sysctl_init.py +++ b/tests/cli/test_sysctl_init.py @@ -1,4 +1,3 @@ -import json import shutil import tempfile from pathlib import Path @@ -6,7 +5,7 @@ from hathor.builder.sysctl_builder import SysctlBuilder from hathor.cli.run_node import RunNode -from hathor.sysctl.exception import SysctlEntryNotFound, SysctlRunnerException +from hathor.sysctl.exception import SysctlRunnerException from hathor.sysctl.init_file_loader import SysctlInitFileLoader from hathor.sysctl.runner import SysctlRunner from tests import unittest @@ -42,14 +41,14 @@ def test_sysctl_builder_fail_with_invalid_property(self): 'manager.metrics.websocket_factory.return_value': None }) - with self.assertRaises(SysctlEntryNotFound) as context: + with self.assertRaises(SysctlRunnerException) as context: root = SysctlBuilder(artifacts).build() runner = SysctlRunner(root) loader = SysctlInitFileLoader(runner, sysctl_init_file_path) loader.load() # assert message in the caught exception - expected_msg = 'invalid.property' + expected_msg = 'path invalid.property not found' self.assertEqual(str(context.exception), expected_msg) def test_sysctl_builder_fail_with_invalid_value(self): @@ -81,25 +80,6 @@ def test_sysctl_builder_fail_with_invalid_value(self): expected_msg = 'value: wrong format' self.assertEqual(str(context.exception), expected_msg) - def test_syctl_init_file_fail_with_empty_or_invalid_file(self): - # prepare to register only p2p commands - artifacts = Mock(**{ - 'p2p_manager': Mock(), - 'manager.metrics.websocket_factory.return_value': None - }) - - with self.assertRaises(AssertionError): - root = SysctlBuilder(artifacts).build() - runner = SysctlRunner(root) - loader = SysctlInitFileLoader(runner, None) - loader.load() - - with self.assertRaises(AssertionError): - root = SysctlBuilder(artifacts).build() - runner = SysctlRunner(root) - loader = SysctlInitFileLoader(runner, "") - loader.load() - @patch('twisted.internet.endpoints.serverFromString') # avoid open sock def test_command_option_sysctl_init_file(self, mock_endpoint): class CustomRunNode(RunNode): @@ -113,12 +93,14 @@ def register_signal_handlers(self) -> None: 'p2p.max_enabled_sync': 7, 'p2p.rate_limit.global.send_tips': (5, 3), 'p2p.sync_update_interval': 17, + 'p2p.always_enable_sync': ['peer-1', 'peer-2'], } file_content = [ 'p2p.max_enabled_sync=7', 'p2p.rate_limit.global.send_tips=5,3', 'p2p.sync_update_interval=17', + 'p2p.always_enable_sync=["peer-1","peer-2"]', ] with tempfile.NamedTemporaryFile( @@ -155,6 +137,10 @@ def register_signal_handlers(self) -> None: curr_sync_update_interval, expected_sysctl_dict['p2p.sync_update_interval']) + curr_always_enabled_sync = list(conn.always_enable_sync) + self.assertTrue( + set(curr_always_enabled_sync).issuperset(set(expected_sysctl_dict['p2p.always_enable_sync']))) + # assert always_enabled_sync when it is set with a file expected_sysctl_dict = { 'p2p.always_enable_sync': ['peer-3', 'peer-4'], @@ -175,7 +161,7 @@ def register_signal_handlers(self) -> None: always_enabled_peers_file_path = str(Path(always_enabled_peers_file.name)) file_content = [ - f'p2p.always_enable_sync.readtxt={json.dumps(always_enabled_peers_file_path)}' + f'p2p.always_enable_sync.readtxt="{always_enabled_peers_file_path}"' ] # set the sysctl.txt file From a8125ddea6daff8a959f4f887fad234abdd94945 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Tue, 29 Aug 2023 16:10:02 +0100 Subject: [PATCH 06/16] chore: remove unused settings --- hathor/builder/sysctl_builder.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/hathor/builder/sysctl_builder.py b/hathor/builder/sysctl_builder.py index 08fa86ec0..bbb84ed9a 100644 --- a/hathor/builder/sysctl_builder.py +++ b/hathor/builder/sysctl_builder.py @@ -15,11 +15,8 @@ from typing import Optional from hathor.builder import BuildArtifacts -from hathor.conf.settings import HathorSettings from hathor.sysctl import ConnectionsManagerSysctl, Sysctl, WebsocketManagerSysctl -settings = HathorSettings - class SysctlBuilder: """Builder for the sysctl tree.""" From aa11807f8836a759c17ecb98763255cb1cd7e441 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Tue, 29 Aug 2023 20:22:26 +0100 Subject: [PATCH 07/16] fix: test and lint --- hathor/cli/run_node.py | 1 - tests/cli/test_sysctl_init.py | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/hathor/cli/run_node.py b/hathor/cli/run_node.py index 414827690..b694cd2f6 100644 --- a/hathor/cli/run_node.py +++ b/hathor/cli/run_node.py @@ -394,7 +394,6 @@ def init_sysctl(self, description: str, sysctl_init_file: Optional[str] = None) from hathor.sysctl.factory import SysctlFactory from hathor.sysctl.init_file_loader import SysctlInitFileLoader from hathor.sysctl.runner import SysctlRunner - from hathor.sysctl.init_file_loader import SysctlInitFileLoader builder = SysctlBuilder(self.artifacts) root = builder.build() diff --git a/tests/cli/test_sysctl_init.py b/tests/cli/test_sysctl_init.py index 653f8526a..07dc24269 100644 --- a/tests/cli/test_sysctl_init.py +++ b/tests/cli/test_sysctl_init.py @@ -5,7 +5,7 @@ from hathor.builder.sysctl_builder import SysctlBuilder from hathor.cli.run_node import RunNode -from hathor.sysctl.exception import SysctlRunnerException +from hathor.sysctl.exception import SysctlEntryNotFound, SysctlRunnerException from hathor.sysctl.init_file_loader import SysctlInitFileLoader from hathor.sysctl.runner import SysctlRunner from tests import unittest @@ -41,14 +41,14 @@ def test_sysctl_builder_fail_with_invalid_property(self): 'manager.metrics.websocket_factory.return_value': None }) - with self.assertRaises(SysctlRunnerException) as context: + with self.assertRaises(SysctlEntryNotFound) as context: root = SysctlBuilder(artifacts).build() runner = SysctlRunner(root) loader = SysctlInitFileLoader(runner, sysctl_init_file_path) loader.load() # assert message in the caught exception - expected_msg = 'path invalid.property not found' + expected_msg = 'invalid.property' self.assertEqual(str(context.exception), expected_msg) def test_sysctl_builder_fail_with_invalid_value(self): From dcb53c88345a1d2678cd31df2258134042ea981f Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Wed, 6 Sep 2023 18:19:30 +0100 Subject: [PATCH 08/16] test: serialize path with json.dumps --- tests/cli/test_sysctl_init.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/cli/test_sysctl_init.py b/tests/cli/test_sysctl_init.py index 07dc24269..b61e081ff 100644 --- a/tests/cli/test_sysctl_init.py +++ b/tests/cli/test_sysctl_init.py @@ -1,3 +1,4 @@ +import json import shutil import tempfile from pathlib import Path @@ -161,7 +162,7 @@ def register_signal_handlers(self) -> None: always_enabled_peers_file_path = str(Path(always_enabled_peers_file.name)) file_content = [ - f'p2p.always_enable_sync.readtxt="{always_enabled_peers_file_path}"' + f'p2p.always_enable_sync.readtxt={json.dumps(always_enabled_peers_file_path)}' ] # set the sysctl.txt file From 349c7fdbb09072f5105632c9098f7aa5235232f6 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 15 Sep 2023 15:35:35 +0100 Subject: [PATCH 09/16] chore: remove unused class attribute --- hathor/builder/sysctl_builder.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/hathor/builder/sysctl_builder.py b/hathor/builder/sysctl_builder.py index bbb84ed9a..46c189ebd 100644 --- a/hathor/builder/sysctl_builder.py +++ b/hathor/builder/sysctl_builder.py @@ -12,8 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import Optional - from hathor.builder import BuildArtifacts from hathor.sysctl import ConnectionsManagerSysctl, Sysctl, WebsocketManagerSysctl @@ -23,7 +21,6 @@ class SysctlBuilder: def __init__(self, artifacts: BuildArtifacts) -> None: self.artifacts = artifacts - self.sysctl_init_file: Optional[str] = None def build(self) -> Sysctl: """Build the sysctl tree.""" From 88251c54f1e96b40cfc928d3ae31958cba7569b2 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 15 Sep 2023 15:37:48 +0100 Subject: [PATCH 10/16] style: join lines --- hathor/cli/run_node.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hathor/cli/run_node.py b/hathor/cli/run_node.py index b694cd2f6..7e365ce0b 100644 --- a/hathor/cli/run_node.py +++ b/hathor/cli/run_node.py @@ -373,8 +373,7 @@ def __init__(self, *, argv=None): self.prepare() self.register_signal_handlers() if self._args.sysctl: - self.init_sysctl( - self._args.sysctl, self._args.sysctl_init_file) + self.init_sysctl(self._args.sysctl, self._args.sysctl_init_file) def init_sysctl(self, description: str, sysctl_init_file: Optional[str] = None) -> None: """Initialize sysctl, listen for connections and apply settings from config file if required. From c7da8a0cf3686fbd21b1d4ef39ee47301580d4df Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 15 Sep 2023 16:09:39 +0100 Subject: [PATCH 11/16] feat: make init_file mandatory with assert --- hathor/sysctl/init_file_loader.py | 8 ++------ tests/cli/test_sysctl_init.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/hathor/sysctl/init_file_loader.py b/hathor/sysctl/init_file_loader.py index ac6bdb1ed..d5e60ebcc 100644 --- a/hathor/sysctl/init_file_loader.py +++ b/hathor/sysctl/init_file_loader.py @@ -1,19 +1,15 @@ -from typing import Optional - from hathor.sysctl.runner import SysctlRunner class SysctlInitFileLoader: - def __init__(self, runner: SysctlRunner, init_file: Optional[str]) -> None: + def __init__(self, runner: SysctlRunner, init_file: str) -> None: assert runner + assert init_file self.runner = runner self.init_file = init_file def load(self) -> None: - if not self.init_file: - return - with open(self.init_file, 'r', encoding='utf-8') as file: lines = file.readlines() diff --git a/tests/cli/test_sysctl_init.py b/tests/cli/test_sysctl_init.py index b61e081ff..dd21c28ab 100644 --- a/tests/cli/test_sysctl_init.py +++ b/tests/cli/test_sysctl_init.py @@ -81,6 +81,25 @@ def test_sysctl_builder_fail_with_invalid_value(self): expected_msg = 'value: wrong format' self.assertEqual(str(context.exception), expected_msg) + def test_syctl_init_file_fail_with_empty_or_invalid_file(self): + # prepare to register only p2p commands + artifacts = Mock(**{ + 'p2p_manager': Mock(), + 'manager.metrics.websocket_factory.return_value': None + }) + + with self.assertRaises(AssertionError): + root = SysctlBuilder(artifacts).build() + runner = SysctlRunner(root) + loader = SysctlInitFileLoader(runner, None) + loader.load() + + with self.assertRaises(AssertionError): + root = SysctlBuilder(artifacts).build() + runner = SysctlRunner(root) + loader = SysctlInitFileLoader(runner, "") + loader.load() + @patch('twisted.internet.endpoints.serverFromString') # avoid open sock def test_command_option_sysctl_init_file(self, mock_endpoint): class CustomRunNode(RunNode): From f6d59633e4e32c0d10f07566d511c89adfadd820 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 15 Sep 2023 16:12:48 +0100 Subject: [PATCH 12/16] chore: add docstring to load method --- hathor/sysctl/init_file_loader.py | 1 + 1 file changed, 1 insertion(+) diff --git a/hathor/sysctl/init_file_loader.py b/hathor/sysctl/init_file_loader.py index d5e60ebcc..9bf09efb6 100644 --- a/hathor/sysctl/init_file_loader.py +++ b/hathor/sysctl/init_file_loader.py @@ -10,6 +10,7 @@ def __init__(self, runner: SysctlRunner, init_file: str) -> None: self.init_file = init_file def load(self) -> None: + """Read the init_file and execute each line as a syctl command in the runner.""" with open(self.init_file, 'r', encoding='utf-8') as file: lines = file.readlines() From e5845ae910a1bd33fe86802f8f61c30ed3e47d94 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Fri, 15 Sep 2023 16:44:24 +0100 Subject: [PATCH 13/16] feat: load init file only if arg value is not None --- hathor/cli/run_node.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hathor/cli/run_node.py b/hathor/cli/run_node.py index 7e365ce0b..00ab40956 100644 --- a/hathor/cli/run_node.py +++ b/hathor/cli/run_node.py @@ -398,8 +398,9 @@ def init_sysctl(self, description: str, sysctl_init_file: Optional[str] = None) root = builder.build() runner = SysctlRunner(root) - init_file_loader = SysctlInitFileLoader(runner, sysctl_init_file) - init_file_loader.load() + if sysctl_init_file: + init_file_loader = SysctlInitFileLoader(runner, sysctl_init_file) + init_file_loader.load() factory = SysctlFactory(runner) endpoint = serverFromString(self.reactor, description) From 92d99e05b94910183d216024b594aa7a75a5bc79 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Wed, 20 Sep 2023 17:48:19 +0100 Subject: [PATCH 14/16] chore: remove bugfix --- hathor/sysctl/runner.py | 10 ---------- tests/cli/test_sysctl_init.py | 6 ------ 2 files changed, 16 deletions(-) diff --git a/hathor/sysctl/runner.py b/hathor/sysctl/runner.py index 1080535e2..ef75a21b6 100644 --- a/hathor/sysctl/runner.py +++ b/hathor/sysctl/runner.py @@ -13,7 +13,6 @@ # limitations under the License. import json -import re from typing import TYPE_CHECKING, Any from hathor.sysctl.exception import SysctlRunnerException @@ -21,12 +20,6 @@ if TYPE_CHECKING: from hathor.sysctl.sysctl import Sysctl -# - It starts with an opening square bracket [. -# - It ends with a closing square bracket ]. -# - The elements are separated by commas and can be followed by optional whitespace. -# - There can be zero or more elements in the array (an empty array is allowed). -array_pattern = r'^\s*\[\s*(?:[^\[\],]+(?:\s*,\s*[^\[\],]+)*)?\s*\]\s*$' - class SysctlRunner: """ Encapsulates the Sysctl to decouple it from the SyctlProtocol. @@ -83,9 +76,6 @@ def deserialize(self, value_str: str) -> Any: if len(value_str) == 0: return () - if re.match(array_pattern, value_str): - return list(json.loads(value_str)) - parts = [x.strip() for x in value_str.split(',')] if len(parts) > 1: return tuple(json.loads(x) for x in parts) diff --git a/tests/cli/test_sysctl_init.py b/tests/cli/test_sysctl_init.py index dd21c28ab..a696e2008 100644 --- a/tests/cli/test_sysctl_init.py +++ b/tests/cli/test_sysctl_init.py @@ -113,14 +113,12 @@ def register_signal_handlers(self) -> None: 'p2p.max_enabled_sync': 7, 'p2p.rate_limit.global.send_tips': (5, 3), 'p2p.sync_update_interval': 17, - 'p2p.always_enable_sync': ['peer-1', 'peer-2'], } file_content = [ 'p2p.max_enabled_sync=7', 'p2p.rate_limit.global.send_tips=5,3', 'p2p.sync_update_interval=17', - 'p2p.always_enable_sync=["peer-1","peer-2"]', ] with tempfile.NamedTemporaryFile( @@ -157,10 +155,6 @@ def register_signal_handlers(self) -> None: curr_sync_update_interval, expected_sysctl_dict['p2p.sync_update_interval']) - curr_always_enabled_sync = list(conn.always_enable_sync) - self.assertTrue( - set(curr_always_enabled_sync).issuperset(set(expected_sysctl_dict['p2p.always_enable_sync']))) - # assert always_enabled_sync when it is set with a file expected_sysctl_dict = { 'p2p.always_enable_sync': ['peer-3', 'peer-4'], From 0249ef444c297976851b2f1f9732bf6eedf4e74d Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Wed, 20 Sep 2023 18:14:48 +0100 Subject: [PATCH 15/16] Revert "chore: remove bugfix" This reverts commit aadaf75c38fb34a690a70f077403bd4de46843fb. --- hathor/sysctl/runner.py | 10 ++++++++++ tests/cli/test_sysctl_init.py | 6 ++++++ 2 files changed, 16 insertions(+) diff --git a/hathor/sysctl/runner.py b/hathor/sysctl/runner.py index ef75a21b6..1080535e2 100644 --- a/hathor/sysctl/runner.py +++ b/hathor/sysctl/runner.py @@ -13,6 +13,7 @@ # limitations under the License. import json +import re from typing import TYPE_CHECKING, Any from hathor.sysctl.exception import SysctlRunnerException @@ -20,6 +21,12 @@ if TYPE_CHECKING: from hathor.sysctl.sysctl import Sysctl +# - It starts with an opening square bracket [. +# - It ends with a closing square bracket ]. +# - The elements are separated by commas and can be followed by optional whitespace. +# - There can be zero or more elements in the array (an empty array is allowed). +array_pattern = r'^\s*\[\s*(?:[^\[\],]+(?:\s*,\s*[^\[\],]+)*)?\s*\]\s*$' + class SysctlRunner: """ Encapsulates the Sysctl to decouple it from the SyctlProtocol. @@ -76,6 +83,9 @@ def deserialize(self, value_str: str) -> Any: if len(value_str) == 0: return () + if re.match(array_pattern, value_str): + return list(json.loads(value_str)) + parts = [x.strip() for x in value_str.split(',')] if len(parts) > 1: return tuple(json.loads(x) for x in parts) diff --git a/tests/cli/test_sysctl_init.py b/tests/cli/test_sysctl_init.py index a696e2008..dd21c28ab 100644 --- a/tests/cli/test_sysctl_init.py +++ b/tests/cli/test_sysctl_init.py @@ -113,12 +113,14 @@ def register_signal_handlers(self) -> None: 'p2p.max_enabled_sync': 7, 'p2p.rate_limit.global.send_tips': (5, 3), 'p2p.sync_update_interval': 17, + 'p2p.always_enable_sync': ['peer-1', 'peer-2'], } file_content = [ 'p2p.max_enabled_sync=7', 'p2p.rate_limit.global.send_tips=5,3', 'p2p.sync_update_interval=17', + 'p2p.always_enable_sync=["peer-1","peer-2"]', ] with tempfile.NamedTemporaryFile( @@ -155,6 +157,10 @@ def register_signal_handlers(self) -> None: curr_sync_update_interval, expected_sysctl_dict['p2p.sync_update_interval']) + curr_always_enabled_sync = list(conn.always_enable_sync) + self.assertTrue( + set(curr_always_enabled_sync).issuperset(set(expected_sysctl_dict['p2p.always_enable_sync']))) + # assert always_enabled_sync when it is set with a file expected_sysctl_dict = { 'p2p.always_enable_sync': ['peer-3', 'peer-4'], From 7b431587bbc67ff09d7b7d03d4be3ee6756ec8d0 Mon Sep 17 00:00:00 2001 From: Alex Ruzenhack Date: Wed, 11 Oct 2023 18:28:23 +0100 Subject: [PATCH 16/16] refactor: replace pattern by a strictier one - allow list to parse number and strings in double quote - don't allow empty elements and comma after last element --- hathor/sysctl/runner.py | 20 +++++++++++++++----- tests/cli/test_sysctl_init.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/hathor/sysctl/runner.py b/hathor/sysctl/runner.py index 1080535e2..eb948e6f7 100644 --- a/hathor/sysctl/runner.py +++ b/hathor/sysctl/runner.py @@ -21,11 +21,21 @@ if TYPE_CHECKING: from hathor.sysctl.sysctl import Sysctl -# - It starts with an opening square bracket [. -# - It ends with a closing square bracket ]. -# - The elements are separated by commas and can be followed by optional whitespace. -# - There can be zero or more elements in the array (an empty array is allowed). -array_pattern = r'^\s*\[\s*(?:[^\[\],]+(?:\s*,\s*[^\[\],]+)*)?\s*\]\s*$' +# Top level: +# - Allow numbers, integers and float +# - Allow string in double quote +# - Allow mix of numbers with strings +# - Allow space before, after and between elements +# - Don't allow empty element like [1,,2]; empty element between 1 and 2 +# - Don't allow comma (,) after last element +# - Don't allow double quote (") inside string +# Regex: +# - \[ - start list +# - ]] - end list +# - \s* - accept space in any quantity +# - \d+ - accept at least 1 number +# - ^" - negate double quote +array_pattern = r'\[(?:\s*(\d+\.\d+|\s*\d+)\s*|\s*"([^"]*)"\s*)(?:,\s*(\d+\.\d+|\s*\d+)\s*|,\s*"([^"]*)"\s*)*\]|\[\]' class SysctlRunner: diff --git a/tests/cli/test_sysctl_init.py b/tests/cli/test_sysctl_init.py index dd21c28ab..d3241723e 100644 --- a/tests/cli/test_sysctl_init.py +++ b/tests/cli/test_sysctl_init.py @@ -204,3 +204,36 @@ def register_signal_handlers(self) -> None: curr_always_enabled_sync = list(conn.always_enable_sync) self.assertTrue( set(curr_always_enabled_sync).issuperset(set(expected_sysctl_dict['p2p.always_enable_sync']))) + + @patch('twisted.internet.endpoints.serverFromString') # avoid open sock + def test_sysctl_init_file_failing_list_parsing(self, mock_endpoint): + class CustomRunNode(RunNode): + def start_manager(self) -> None: + pass + + def register_signal_handlers(self) -> None: + pass + + test_cases = [ + ['p2p.always_enable_sync=[ "peer-1", "peer-2", ]'], + ['p2p.always_enable_sync=["peer-1",,"peer-2"]'], + ['p2p.always_enable_sync=["peer-1","\"peer-2\""]'], + ['p2p.always_enable_sync=[1,2,]'], + ['p2p.always_enable_sync=[1,,2]'] + ] + + for file_content in test_cases: + with tempfile.NamedTemporaryFile( + dir=self.tmp_dir, + suffix='.txt', + prefix='sysctl_', + delete=False) as sysctl_init_file: + sysctl_init_file.write('\n'.join(file_content).encode()) + sysctl_init_file_path = str(Path(sysctl_init_file.name)) + + with self.assertRaises(SysctlRunnerException): + CustomRunNode(argv=[ + '--sysctl', 'tcp:8181', + '--sysctl-init-file', sysctl_init_file_path, # relative to src/hathor + '--memory-storage', + ])