Better formatting for config errors from modules (#8874)

The idea is that the parse_config method of extension modules can raise either a ConfigError or a JsonValidationError,
and it will be magically turned into a legible error message. There's a few components to it:

* Separating the "path" and the "message" parts of a ConfigError, so that we can fiddle with the path bit to turn it
   into an absolute path.
* Generally improving the way ConfigErrors get printed.
* Passing in the config path to load_module so that it can wrap any exceptions that get caught appropriately.
This commit is contained in:
Richard van der Hoff 2020-12-08 14:04:35 +00:00 committed by GitHub
parent 36ba73f53d
commit ab7a24cc6b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 159 additions and 36 deletions

1
changelog.d/8874.feature Normal file
View file

@ -0,0 +1 @@
Improve the error messages printed as a result of configuration problems for extension modules.

View file

@ -19,7 +19,7 @@ import gc
import logging
import os
import sys
from typing import Iterable
from typing import Iterable, Iterator
from twisted.application import service
from twisted.internet import defer, reactor
@ -90,7 +90,7 @@ class SynapseHomeServer(HomeServer):
tls = listener_config.tls
site_tag = listener_config.http_options.tag
if site_tag is None:
site_tag = port
site_tag = str(port)
# We always include a health resource.
resources = {"/health": HealthResource()}
@ -107,7 +107,10 @@ class SynapseHomeServer(HomeServer):
logger.debug("Configuring additional resources: %r", additional_resources)
module_api = self.get_module_api()
for path, resmodule in additional_resources.items():
handler_cls, config = load_module(resmodule)
handler_cls, config = load_module(
resmodule,
("listeners", site_tag, "additional_resources", "<%s>" % (path,)),
)
handler = handler_cls(config, module_api)
if IResource.providedBy(handler):
resource = handler
@ -342,7 +345,10 @@ def setup(config_options):
"Synapse Homeserver", config_options
)
except ConfigError as e:
sys.stderr.write("\nERROR: %s\n" % (e,))
sys.stderr.write("\n")
for f in format_config_error(e):
sys.stderr.write(f)
sys.stderr.write("\n")
sys.exit(1)
if not config:
@ -445,6 +451,38 @@ def setup(config_options):
return hs
def format_config_error(e: ConfigError) -> Iterator[str]:
"""
Formats a config error neatly
The idea is to format the immediate error, plus the "causes" of those errors,
hopefully in a way that makes sense to the user. For example:
Error in configuration at 'oidc_config.user_mapping_provider.config.display_name_template':
Failed to parse config for module 'JinjaOidcMappingProvider':
invalid jinja template:
unexpected end of template, expected 'end of print statement'.
Args:
e: the error to be formatted
Returns: An iterator which yields string fragments to be formatted
"""
yield "Error in configuration"
if e.path:
yield " at '%s'" % (".".join(e.path),)
yield ":\n %s" % (e.msg,)
e = e.__cause__
indent = 1
while e:
indent += 1
yield ":\n%s%s" % (" " * indent, str(e))
e = e.__cause__
class SynapseService(service.Service):
"""
A twisted Service class that will start synapse. Used to run synapse

View file

@ -23,7 +23,7 @@ import urllib.parse
from collections import OrderedDict
from hashlib import sha256
from textwrap import dedent
from typing import Any, Callable, List, MutableMapping, Optional
from typing import Any, Callable, Iterable, List, MutableMapping, Optional
import attr
import jinja2
@ -32,7 +32,17 @@ import yaml
class ConfigError(Exception):
pass
"""Represents a problem parsing the configuration
Args:
msg: A textual description of the error.
path: Where appropriate, an indication of where in the configuration
the problem lies.
"""
def __init__(self, msg: str, path: Optional[Iterable[str]] = None):
self.msg = msg
self.path = path
# We split these messages out to allow packages to override with package

View file

@ -1,4 +1,4 @@
from typing import Any, List, Optional
from typing import Any, Iterable, List, Optional
from synapse.config import (
api,
@ -35,7 +35,10 @@ from synapse.config import (
workers,
)
class ConfigError(Exception): ...
class ConfigError(Exception):
def __init__(self, msg: str, path: Optional[Iterable[str]] = None):
self.msg = msg
self.path = path
MISSING_REPORT_STATS_CONFIG_INSTRUCTIONS: str
MISSING_REPORT_STATS_SPIEL: str

View file

@ -38,14 +38,27 @@ def validate_config(
try:
jsonschema.validate(config, json_schema)
except jsonschema.ValidationError as e:
# copy `config_path` before modifying it.
path = list(config_path)
for p in list(e.path):
if isinstance(p, int):
path.append("<item %i>" % p)
else:
path.append(str(p))
raise json_error_to_config_error(e, config_path)
raise ConfigError(
"Unable to parse configuration: %s at %s" % (e.message, ".".join(path))
)
def json_error_to_config_error(
e: jsonschema.ValidationError, config_path: Iterable[str]
) -> ConfigError:
"""Converts a json validation error to a user-readable ConfigError
Args:
e: the exception to be converted
config_path: the path within the config file. This will be used as a basis
for the error message.
Returns:
a ConfigError
"""
# copy `config_path` before modifying it.
path = list(config_path)
for p in list(e.path):
if isinstance(p, int):
path.append("<item %i>" % p)
else:
path.append(str(p))
return ConfigError(e.message, path)

View file

@ -66,7 +66,7 @@ class OIDCConfig(Config):
(
self.oidc_user_mapping_provider_class,
self.oidc_user_mapping_provider_config,
) = load_module(ump_config)
) = load_module(ump_config, ("oidc_config", "user_mapping_provider"))
# Ensure loaded user mapping module has defined all necessary methods
required_methods = [

View file

@ -36,7 +36,7 @@ class PasswordAuthProviderConfig(Config):
providers.append({"module": LDAP_PROVIDER, "config": ldap_config})
providers.extend(config.get("password_providers") or [])
for provider in providers:
for i, provider in enumerate(providers):
mod_name = provider["module"]
# This is for backwards compat when the ldap auth provider resided
@ -45,7 +45,8 @@ class PasswordAuthProviderConfig(Config):
mod_name = LDAP_PROVIDER
(provider_class, provider_config) = load_module(
{"module": mod_name, "config": provider["config"]}
{"module": mod_name, "config": provider["config"]},
("password_providers", "<item %i>" % i),
)
self.password_providers.append((provider_class, provider_config))

View file

@ -142,7 +142,7 @@ class ContentRepositoryConfig(Config):
# them to be started.
self.media_storage_providers = [] # type: List[tuple]
for provider_config in storage_providers:
for i, provider_config in enumerate(storage_providers):
# We special case the module "file_system" so as not to need to
# expose FileStorageProviderBackend
if provider_config["module"] == "file_system":
@ -151,7 +151,9 @@ class ContentRepositoryConfig(Config):
".FileStorageProviderBackend"
)
provider_class, parsed_config = load_module(provider_config)
provider_class, parsed_config = load_module(
provider_config, ("media_storage_providers", "<item %i>" % i)
)
wrapper_config = MediaStorageProviderConfig(
provider_config.get("store_local", False),

View file

@ -180,7 +180,7 @@ class _RoomDirectoryRule:
self._alias_regex = glob_to_regex(alias)
self._room_id_regex = glob_to_regex(room_id)
except Exception as e:
raise ConfigError("Failed to parse glob into regex: %s", e)
raise ConfigError("Failed to parse glob into regex") from e
def matches(self, user_id, room_id, aliases):
"""Tests if this rule matches the given user_id, room_id and aliases.

View file

@ -125,7 +125,7 @@ class SAML2Config(Config):
(
self.saml2_user_mapping_provider_class,
self.saml2_user_mapping_provider_config,
) = load_module(ump_dict)
) = load_module(ump_dict, ("saml2_config", "user_mapping_provider"))
# Ensure loaded user mapping module has defined all necessary methods
# Note parse_config() is already checked during the call to load_module

View file

@ -33,13 +33,14 @@ class SpamCheckerConfig(Config):
# spam checker, and thus was simply a dictionary with module
# and config keys. Support this old behaviour by checking
# to see if the option resolves to a dictionary
self.spam_checkers.append(load_module(spam_checkers))
self.spam_checkers.append(load_module(spam_checkers, ("spam_checker",)))
elif isinstance(spam_checkers, list):
for spam_checker in spam_checkers:
for i, spam_checker in enumerate(spam_checkers):
config_path = ("spam_checker", "<item %i>" % i)
if not isinstance(spam_checker, dict):
raise ConfigError("spam_checker syntax is incorrect")
raise ConfigError("expected a mapping", config_path)
self.spam_checkers.append(load_module(spam_checker))
self.spam_checkers.append(load_module(spam_checker, config_path))
else:
raise ConfigError("spam_checker syntax is incorrect")

View file

@ -26,7 +26,9 @@ class ThirdPartyRulesConfig(Config):
provider = config.get("third_party_event_rules", None)
if provider is not None:
self.third_party_event_rules = load_module(provider)
self.third_party_event_rules = load_module(
provider, ("third_party_event_rules",)
)
def generate_config_section(self, **kwargs):
return """\

View file

@ -15,28 +15,56 @@
import importlib
import importlib.util
import itertools
from typing import Any, Iterable, Tuple, Type
import jsonschema
from synapse.config._base import ConfigError
from synapse.config._util import json_error_to_config_error
def load_module(provider):
def load_module(provider: dict, config_path: Iterable[str]) -> Tuple[Type, Any]:
""" Loads a synapse module with its config
Take a dict with keys 'module' (the module name) and 'config'
(the config dict).
Args:
provider: a dict with keys 'module' (the module name) and 'config'
(the config dict).
config_path: the path within the config file. This will be used as a basis
for any error message.
Returns
Tuple of (provider class, parsed config object)
"""
modulename = provider.get("module")
if not isinstance(modulename, str):
raise ConfigError(
"expected a string", path=itertools.chain(config_path, ("module",))
)
# We need to import the module, and then pick the class out of
# that, so we split based on the last dot.
module, clz = provider["module"].rsplit(".", 1)
module, clz = modulename.rsplit(".", 1)
module = importlib.import_module(module)
provider_class = getattr(module, clz)
module_config = provider.get("config")
try:
provider_config = provider_class.parse_config(provider.get("config"))
provider_config = provider_class.parse_config(module_config)
except jsonschema.ValidationError as e:
raise json_error_to_config_error(e, itertools.chain(config_path, ("config",)))
except ConfigError as e:
raise _wrap_config_error(
"Failed to parse config for module %r" % (modulename,),
prefix=itertools.chain(config_path, ("config",)),
e=e,
)
except Exception as e:
raise ConfigError("Failed to parse config for %r: %s" % (provider["module"], e))
raise ConfigError(
"Failed to parse config for module %r" % (modulename,),
path=itertools.chain(config_path, ("config",)),
) from e
return provider_class, provider_config
@ -56,3 +84,27 @@ def load_python_module(location: str):
mod = importlib.util.module_from_spec(spec)
spec.loader.exec_module(mod) # type: ignore
return mod
def _wrap_config_error(
msg: str, prefix: Iterable[str], e: ConfigError
) -> "ConfigError":
"""Wrap a relative ConfigError with a new path
This is useful when we have a ConfigError with a relative path due to a problem
parsing part of the config, and we now need to set it in context.
"""
path = prefix
if e.path:
path = itertools.chain(prefix, e.path)
e1 = ConfigError(msg, path)
# ideally we would set the 'cause' of the new exception to the original exception;
# however now that we have merged the path into our own, the stringification of
# e will be incorrect, so instead we create a new exception with just the "msg"
# part.
e1.__cause__ = Exception(e.msg)
e1.__cause__.__cause__ = e.__cause__
return e1