diff --git a/gns3server/config.py b/gns3server/config.py index c579a5328..278432358 100644 --- a/gns3server/config.py +++ b/gns3server/config.py @@ -24,6 +24,7 @@ import shutil import secrets import configparser +from enum import Enum from pydantic import ValidationError from .schemas import ServerConfig from .version import __version_info__ @@ -34,6 +35,19 @@ import logging log = logging.getLogger(__name__) +class ConfigConflictError(Exception): + """ + Raised when a configuration option is set in a configuration file that + takes precedence over the main configuration file. + """ + + +# List options written back to the configuration file as semicolon-separated +# values; every other list option is comma-separated (must match the field +# validators splitting them in gns3server.schemas.config). +LIST_OPTION_SEPARATORS = {"additional_images_paths": ";"} + + class Config: """ Configuration file management using configparser. @@ -199,6 +213,13 @@ class Config: """ log.info(f"'{file_path}' has been updated, reloading the config...") + self.reload_and_notify() + + def reload_and_notify(self): + """ + Reload the configuration files and notify registered listeners. + """ + self.read_config() for callback in self._watch_callback: callback() @@ -210,6 +231,107 @@ class Config: self.read_config() + @staticmethod + def _stringify_option(option: str, value) -> str: + """ + Serialize a settings value to its INI string representation. + """ + + if isinstance(value, bool): + return str(value) + if isinstance(value, Enum): + return str(value.value) + if isinstance(value, list): + return LIST_OPTION_SEPARATORS.get(option, ",").join(value) + return str(value) + + def update_config(self, changes: dict) -> list: + """ + Apply setting changes to the main configuration file (read-modify-write). + + Only the submitted options are set or removed, preserving any unknown + options present in the file. The merged configuration is validated + before anything is written to disk, so an invalid change leaves the + file untouched (a file that fails validation would permanently kill + the FileWatcher polling loop when it gets reloaded). + + :param changes: mapping of section name to {option: value}; a value of + None removes the option from the file, restoring its default + :returns: sorted list of changed options as "Section.option" strings + + :raises pydantic.ValidationError: the merged settings are invalid + :raises ConfigConflictError: an option is set in a configuration file + that takes precedence over the main configuration file + :raises OSError: the configuration file could not be written + """ + + if not changes: + return [] + + main_config_file = self._main_config_file + existing_files = [file for file in self._files if os.path.isfile(file)] + + # per-file parsers to find which file wins for an option + # (later files take precedence, mirroring read_config) + per_file_parsers = [] + for file in existing_files: + parser = configparser.ConfigParser(interpolation=None) + parser.read(file, encoding="utf-8") + per_file_parsers.append(parser) + + # view of what gets written: the main configuration file only + write_parser = configparser.ConfigParser(interpolation=None) + if os.path.isfile(main_config_file): + write_parser.read(main_config_file, encoding="utf-8") + + # view of what the server will load: all configuration files merged + merged_parser = configparser.ConfigParser(interpolation=None) + merged_parser.read(existing_files, encoding="utf-8") + + changed = [] + for section, options in changes.items(): + for option, value in options.items(): + winner = None + for file, parser in zip(reversed(existing_files), reversed(per_file_parsers)): + if parser.has_option(section, option): + winner = file + break + if winner is not None and winner != main_config_file: + raise ConfigConflictError( + f"'{section}.{option}' is set in '{winner}' which takes precedence " + f"over the main configuration file '{main_config_file}'" + ) + if value is None: + # explicit null: remove the option to restore its default + if write_parser.has_option(section, option): + write_parser.remove_option(section, option) + if merged_parser.has_option(section, option): + merged_parser.remove_option(section, option) + else: + option_value = self._stringify_option(option, value) + if not write_parser.has_section(section): + write_parser.add_section(section) + write_parser.set(section, option, option_value) + if not merged_parser.has_section(section): + merged_parser.add_section(section) + merged_parser.set(section, option, option_value) + changed.append(f"{section}.{option}") + + # validate the merged settings before touching the file on disk + ServerConfig(**merged_parser._sections) + + directory_name = os.path.dirname(main_config_file) + if directory_name: + os.makedirs(directory_name, exist_ok=True) + tmp_file = main_config_file + ".tmp" + fd = os.open(tmp_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w", encoding="utf-8") as f: + write_parser.write(f) + os.replace(tmp_file, main_config_file) + + self.reload_and_notify() + return sorted(changed) + def get_config_files(self): """ Return the config files in use. diff --git a/gns3server/schemas/config.py b/gns3server/schemas/config.py index 0c712b351..c9c838513 100644 --- a/gns3server/schemas/config.py +++ b/gns3server/schemas/config.py @@ -202,28 +202,36 @@ class ServerSettings(BaseModel): def split_mcp_allowed_hosts(cls, v): if v and isinstance(v, str): return v.split(",") - return list() + if not v: + return list() + return v @field_validator("mcp_allowed_origins", mode="before") @classmethod def split_mcp_allowed_origins(cls, v): if v and isinstance(v, str): return v.split(",") - return list() + if not v: + return list() + return v @field_validator("additional_images_paths", mode="before") @classmethod def split_additional_images_paths(cls, v): - if v: + if v and isinstance(v, str): return v.split(";") - return list() + if not v: + return list() + return v @field_validator("allowed_interfaces", mode="before") @classmethod def split_allowed_interfaces(cls, v): - if v: + if v and isinstance(v, str): return v.split(",") - return list() + if not v: + return list() + return v @model_validator(mode="after") def check_console_port_range(self) -> "ServerSettings": diff --git a/gns3server/utils/file_watcher.py b/gns3server/utils/file_watcher.py index 94b64626d..508fda67d 100644 --- a/gns3server/utils/file_watcher.py +++ b/gns3server/utils/file_watcher.py @@ -19,6 +19,10 @@ import zlib import asyncio import os +import logging + +log = logging.getLogger(__name__) + class FileWatcher: """ @@ -98,7 +102,12 @@ class FileWatcher: except OSError: self._hashed[path] = None if changed: - self._callback(path) + try: + self._callback(path) + except Exception: + # never let a callback exception kill the polling loop + # (the re-schedule below must always run) + log.exception(f"Error in file watcher callback for '{path}'") asyncio.get_event_loop().call_later(self._delay, self._check_config_file_change) @property diff --git a/tests/test_config.py b/tests/test_config.py index f04ebce36..3537ae76c 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -17,9 +17,11 @@ import configparser +import os import pytest from gns3server.config import Config +from gns3server.config import ConfigConflictError from gns3server.config import ServerConfig from pydantic import ValidationError @@ -158,3 +160,120 @@ def test_vmware_settings(settings: dict, exception_expected: bool): ServerConfig(**vmware_settings) else: ServerConfig(**vmware_settings) + + +def test_update_config_writes_ini_types(tmpdir): + + path = str(tmpdir / "server.conf") + with open(path, "w+") as f: + f.write("# a comment\n[Server]\nhost = 127.0.0.1\nfrobnicate = 42\n") + + config = Config(files=[path]) + changed = config.update_config({ + "Server": { + "port": 3081, + "report_errors": False, + "allowed_interfaces": ["eth0", "eth1"], + "default_symbol_theme": "Classic", + "additional_images_paths": ["/path/to/dir1", "/path/to/dir2"], + } + }) + + parsed = configparser.ConfigParser() + parsed.read(path) + assert parsed["Server"]["port"] == "3081" + assert parsed["Server"]["report_errors"] == "False" + assert parsed["Server"]["allowed_interfaces"] == "eth0,eth1" + assert parsed["Server"]["default_symbol_theme"] == "Classic" + assert parsed["Server"]["additional_images_paths"] == "/path/to/dir1;/path/to/dir2" + # options not submitted are left untouched, including unknown ones + assert parsed["Server"]["host"] == "127.0.0.1" + assert parsed["Server"]["frobnicate"] == "42" + + # in-memory settings have been reloaded + assert config.settings.Server.port == 3081 + assert config.settings.Server.report_errors is False + assert config.settings.Server.allowed_interfaces == ["eth0", "eth1"] + assert changed == [ + "Server.additional_images_paths", + "Server.allowed_interfaces", + "Server.default_symbol_theme", + "Server.port", + "Server.report_errors", + ] + + +def test_update_config_null_removes_option(tmpdir): + + path = write_config(tmpdir, {"Server": {"host": "127.0.0.1"}}) + config = Config(files=[path]) + + config.update_config({"Server": {"host": None}}) + + parsed = configparser.ConfigParser() + parsed.read(path) + assert not parsed.has_option("Server", "host") + assert config.settings.Server.host == "0.0.0.0" # default restored + + +def test_update_config_validation_failure_leaves_file_unchanged(tmpdir): + + path = write_config(tmpdir, {"Server": {"console_start_port_range": "5000"}}) + config = Config(files=[path]) + with open(path) as f: + file_content_before = f.read() + + # cross-field violation (console_end_port_range must be > console_start_port_range) + with pytest.raises(ValidationError): + config.update_config({"Server": {"console_start_port_range": 10000, "console_end_port_range": 5000}}) + + with open(path) as f: + assert f.read() == file_content_before + + +def test_update_config_conflict(tmpdir): + + main_path = write_config(tmpdir, {"Server": {"host": "127.0.0.1"}}) + override_path = str(tmpdir / "override.conf") + with open(override_path, "w+") as f: + f.write("[Server]\nhost = 10.0.0.1\n") + + config = Config(files=[main_path, override_path]) + assert config.settings.Server.host == "10.0.0.1" # later file takes precedence + + with pytest.raises(ConfigConflictError): + config.update_config({"Server": {"host": "192.168.1.1"}}) + with pytest.raises(ConfigConflictError): + config.update_config({"Server": {"host": None}}) + + with open(main_path) as f: + assert "host = 127.0.0.1" in f.read() # main file untouched + + +def test_update_config_creates_missing_main_file(tmpdir): + + path = write_config(tmpdir, {"Server": {"host": "127.0.0.1"}}) + config = Config(files=[path]) + os.remove(path) + + config.update_config({"Server": {"port": 3081}}) + + parsed = configparser.ConfigParser() + parsed.read(path) + assert parsed["Server"]["port"] == "3081" + assert not parsed.has_option("Server", "host") # removed file means defaults + + +def test_reload_and_notify(tmpdir): + + path = write_config(tmpdir, {"Server": {"host": "127.0.0.1"}}) + config = Config(files=[path]) + + notified = [] + config.listen_for_config_changes(lambda: notified.append(True)) + + write_config(tmpdir, {"Server": {"host": "192.168.1.2"}}) + config.reload_and_notify() + + assert config.settings.Server.host == "192.168.1.2" + assert notified == [True] diff --git a/tests/utils/test_file_watcher.py b/tests/utils/test_file_watcher.py index 0581cfee6..003d6162c 100644 --- a/tests/utils/test_file_watcher.py +++ b/tests/utils/test_file_watcher.py @@ -66,3 +66,20 @@ async def test_file_watcher_list(tmpdir, strategy): file2.write("b") await asyncio.sleep(0.5) callback.assert_called_with(str(file2)) + + +@pytest.mark.asyncio +async def test_file_watcher_callback_exception_does_not_stop_polling(tmpdir): + + file = tmpdir / "test" + file.write("a") + callback = MagicMock(side_effect=ValueError("callback error")) + FileWatcher(file, callback, delay=0.1) + await asyncio.sleep(0.5) + assert callback.call_count == 0 + file.write("b") + await asyncio.sleep(0.5) + assert callback.call_count == 1 # raised, but must not kill the polling loop + file.write("c") + await asyncio.sleep(0.5) + assert callback.call_count == 2 # polling survived the exception