mirror of
https://github.com/GNS3/gns3-server.git
synced 2026-08-27 12:30:13 +03:00
feat: add config read-modify-write update and harden file watcher
Config.update_config() applies submitted options to the main configuration file via configparser read-modify-write: unknown options are preserved, null removes an option, the merged view of all files is validated as ServerConfig before anything is written (a bad file would kill the FileWatcher polling loop), and the write is atomic (.tmp + os.replace, mode 0600). Options whose effective value is owned by a later configuration file raise ConfigConflictError instead of writing a no-op. The reload logic is factored into reload_and_notify() and the file watcher callback is exception-guarded so polling never dies.
This commit is contained in:
parent
d3ceb453a6
commit
7d3cbf1023
@ -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.
|
||||
|
||||
@ -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":
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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]
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user