diff --git a/CHANGELOG.md b/CHANGELOG.md index 577afec..c22651e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +## v1.1.0 - 2023-??-?? - ??? + +* Add context to general configuration and Record validation, e.g. + Some problem at filename.yaml, line 42, column 14. Our custom Yaml Loaders + attach this context information, arbitrary string. Other providers may do so + by creating ContextDict to pass as `data` into Record.new. +* Add --all option to octodns-validate to enable showing all record validation + errors (as warnings) rather than exiting on the first. Exit code is non-zero + when there are any validation errors. + ## v1.0.0 - 2023-07-30 - The One 1.0 marks a point at which we can formally deprecate things that will be diff --git a/README.md b/README.md index 26753d3..714d58d 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ The architecture is pluggable and the tooling is flexible to make it applicable + [Notes](#notes) - [Compatibility and Compliance](#compatibilty-and-compliance) * [`lenient`](#-lenient-) - * [`strict_supports` (Work In Progress)](#-strict-supports---work-in-progress-) + * [`strict_supports`](#-strict-supports-) * [Configuring `strict_supports`](#configuring--strict-supports-) - [Custom Sources and Providers](#custom-sources-and-providers) - [Other Uses](#other-uses) @@ -329,7 +329,7 @@ octoDNS supports automatically generating PTR records from the `A`/`AAAA` record `lenient` mostly focuses on the details of `Record`s and standards compliance. When set to `true` octoDNS will allow allow non-compliant configurations & values where possible. For example CNAME values that don't end with a `.`, label length restrictions, and invalid geo codes on `dynamic` records. When in lenient mode octoDNS will log validation problems at `WARNING` and try and continue with the configuration or source data as it exists. See [Lenience](/docs/records.md#lenience) for more information on the concept and how it can be configured. -### `strict_supports` (Work In Progress) +### `strict_supports` `strict_supports` is a `Provider` level parameter that comes into play when a provider has been asked to create a record that it is unable to support. The simplest case of this would be record type, e.g. `SSHFP` not being supported by `AzureProvider`. If such a record is passed to an `AzureProvider` as a target the provider will take action based on the `strict_supports`. When `true` it will throw an exception saying that it's unable to create the record, when set to `false` it will log at `WARNING` with information about what it's unable to do and how it is attempting to working around it. Other examples of things that cannot be supported would be `dynamic` records on a provider that only supports simple or the lack of support for specific geos in a provider, e.g. Route53Provider does not support `NA-CA-*`. diff --git a/octodns/cmds/validate.py b/octodns/cmds/validate.py index b69f856..8120835 100755 --- a/octodns/cmds/validate.py +++ b/octodns/cmds/validate.py @@ -3,12 +3,23 @@ Octo-DNS Validator ''' -from logging import WARN +from logging import WARNING, getLogger +from sys import exit from octodns.cmds.args import ArgumentParser from octodns.manager import Manager +class FlaggingHandler: + level = WARNING + + def __init__(self): + self.flag = False + + def handle(self, record): + self.flag = True + + def main(): parser = ArgumentParser(description=__doc__.split('\n')[1]) @@ -17,11 +28,23 @@ def main(): required=True, help='The Manager configuration file to use', ) + parser.add_argument( + '--all', + action='store_true', + default=False, + help='Validate records in lenient mode, printing warnings so that all validation issues are shown', + ) - args = parser.parse_args(WARN) + args = parser.parse_args(WARNING) + + flagging = FlaggingHandler() + getLogger('Record').addHandler(flagging) manager = Manager(args.config_file) - manager.validate_configs() + manager.validate_configs(lenient=args.all) + + if flagging.flag: + exit(1) if __name__ == '__main__': diff --git a/octodns/context.py b/octodns/context.py new file mode 100644 index 0000000..eec8ca1 --- /dev/null +++ b/octodns/context.py @@ -0,0 +1,20 @@ +# +# +# + + +class ContextDict(dict): + ''' + This is used by things that call `Record.new` to pass in a `data` + dictionary that includes some context as to where the data came from to be + printed along with exceptions or validations of the record. + + It breaks lots of stuff if we stored the context in an extra key and the + python `dict` object doesn't allow you to set attributes on the object so + this is a very thin wrapper around `dict` that allows us to have a context + attribute. + ''' + + def __init__(self, *args, context=None, **kwargs): + super().__init__(*args, **kwargs) + self.context = context diff --git a/octodns/manager.py b/octodns/manager.py index 53158af..40912dc 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -190,9 +190,11 @@ class Manager(object): except KeyError: self.log.exception('Invalid provider class') raise ManagerException( - f'Provider {provider_name} is missing class' + f'Provider {provider_name} is missing class, {provider_config.context}' ) - _class, module, version = self._get_named_class('provider', _class) + _class, module, version = self._get_named_class( + 'provider', _class, provider_config.context + ) kwargs = self._build_kwargs(provider_config) try: providers[provider_name] = _class(provider_name, **kwargs) @@ -205,7 +207,7 @@ class Manager(object): except TypeError: self.log.exception('Invalid provider config') raise ManagerException( - 'Incorrect provider config for ' + provider_name + f'Incorrect provider config for {provider_name}, {provider_config.context}' ) return providers @@ -218,9 +220,11 @@ class Manager(object): except KeyError: self.log.exception('Invalid processor class') raise ManagerException( - f'Processor {processor_name} is missing class' + f'Processor {processor_name} is missing class, {processor_config.context}' ) - _class, module, version = self._get_named_class('processor', _class) + _class, module, version = self._get_named_class( + 'processor', _class, processor_config.context + ) kwargs = self._build_kwargs(processor_config) try: processors[processor_name] = _class(processor_name, **kwargs) @@ -233,22 +237,23 @@ class Manager(object): except TypeError: self.log.exception('Invalid processor config') raise ManagerException( - 'Incorrect processor config for ' + processor_name + f'Incorrect processor config for {processor_name}, {processor_config.context}' ) return processors def _config_plan_outputs(self, plan_outputs_config): plan_outputs = {} for plan_output_name, plan_output_config in plan_outputs_config.items(): + context = getattr(plan_output_config, 'context', '') try: _class = plan_output_config.pop('class') except KeyError: self.log.exception('Invalid plan_output class') raise ManagerException( - f'plan_output {plan_output_name} is missing class' + f'plan_output {plan_output_name} is missing class, {context}' ) _class, module, version = self._get_named_class( - 'plan_output', _class + 'plan_output', _class, context ) kwargs = self._build_kwargs(plan_output_config) try: @@ -266,8 +271,9 @@ class Manager(object): except TypeError: self.log.exception('Invalid plan_output config') raise ManagerException( - 'Incorrect plan_output config for ' + plan_output_name + f'Incorrect plan_output config for {plan_output_name}, {context}' ) + return plan_outputs def _try_version(self, module_name, module=None, version=None): @@ -298,7 +304,7 @@ class Manager(object): version = self._try_version(current) return module, version or 'n/a' - def _get_named_class(self, _type, _class): + def _get_named_class(self, _type, _class, context): try: module_name, class_name = _class.rsplit('.', 1) module, version = self._import_module(module_name) @@ -306,7 +312,9 @@ class Manager(object): self.log.exception( '_get_{}_class: Unable to import module %s', _class ) - raise ManagerException(f'Unknown {_type} class: {_class}') + raise ManagerException( + f'Unknown {_type} class: {_class}, {context}' + ) try: return getattr(module, class_name), module_name, version @@ -316,7 +324,9 @@ class Manager(object): class_name, module, ) - raise ManagerException(f'Unknown {_type} class: {_class}') + raise ManagerException( + f'Unknown {_type} class: {_class}, {context}' + ) def _build_kwargs(self, source): # Build up the arguments we need to pass to the provider @@ -330,8 +340,7 @@ class Manager(object): except KeyError: self.log.exception('Invalid provider config') raise ManagerException( - 'Incorrect provider config, ' - 'missing env var ' + env_var + f'Incorrect provider config, missing env var {env_var}, {source.context}' ) except AttributeError: pass @@ -863,7 +872,7 @@ class Manager(object): plan = Plan(zone, zone, [], False) target.apply(plan) - def validate_configs(self): + def validate_configs(self, lenient=False): # TODO: this code can probably be shared with stuff in sync zones = self.config['zones'] @@ -896,7 +905,6 @@ class Manager(object): source_zone = source_zone continue - lenient = config.get('lenient', False) try: sources = config['sources'] except KeyError: @@ -917,6 +925,7 @@ class Manager(object): f'Zone {decoded_zone_name}, unknown source: ' + source ) + lenient = lenient or config.get('lenient', False) for source in sources: if isinstance(source, YamlProvider): source.populate(zone, lenient=lenient) diff --git a/octodns/record/base.py b/octodns/record/base.py index 23b9a5c..ffea1b6 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -46,14 +46,21 @@ class Record(EqualityTupleMixin): reasons.append('invalid record, whitespace is not allowed') fqdn = f'{name}.{zone.name}' if name else zone.name + context = getattr(data, 'context', None) try: _type = data['type'] except KeyError: - raise Exception(f'Invalid record {idna_decode(fqdn)}, missing type') + msg = f'Invalid record {idna_decode(fqdn)}, missing type' + if context: + msg += f', {context}' + raise Exception(msg) try: _class = cls._CLASSES[_type] except KeyError: - raise Exception(f'Unknown record type: "{_type}"') + msg = f'Unknown record type: "{_type}"' + if context: + msg += f', {context}' + raise Exception(msg) reasons.extend(_class.validate(name, fqdn, data)) try: lenient |= data['octodns']['lenient'] @@ -61,9 +68,11 @@ class Record(EqualityTupleMixin): pass if reasons: if lenient: - cls.log.warning(ValidationError.build_message(fqdn, reasons)) + cls.log.warning( + ValidationError.build_message(fqdn, reasons, context) + ) else: - raise ValidationError(fqdn, reasons) + raise ValidationError(fqdn, reasons, context) return _class(zone, name, data, source=source) @classmethod diff --git a/octodns/record/exception.py b/octodns/record/exception.py index 34aea0b..105a43d 100644 --- a/octodns/record/exception.py +++ b/octodns/record/exception.py @@ -11,11 +11,16 @@ class RecordException(Exception): class ValidationError(RecordException): @classmethod - def build_message(cls, fqdn, reasons): + def build_message(cls, fqdn, reasons, context=None): reasons = '\n - '.join(reasons) - return f'Invalid record "{idna_decode(fqdn)}"\n - {reasons}' + msg = f'Invalid record "{idna_decode(fqdn)}"' + if context: + msg += f', {context}' + msg += f'\n - {reasons}' + return msg - def __init__(self, fqdn, reasons): - super().__init__(self.build_message(fqdn, reasons)) + def __init__(self, fqdn, reasons, context=None): + super().__init__(self.build_message(fqdn, reasons, context)) self.fqdn = fqdn self.reasons = reasons + self.context = context diff --git a/octodns/yaml.py b/octodns/yaml.py index 3021fba..09433d9 100644 --- a/octodns/yaml.py +++ b/octodns/yaml.py @@ -7,16 +7,35 @@ from yaml import SafeDumper, SafeLoader, dump, load from yaml.constructor import ConstructorError from yaml.representer import SafeRepresenter +from .context import ContextDict + _natsort_key = natsort_keygen() +class ContextLoader(SafeLoader): + def _pairs(self, node): + self.flatten_mapping(node) + pairs = self.construct_pairs(node) + start_mark = node.start_mark + context = f'{start_mark.name}, line {start_mark.line+1}, column {start_mark.column+1}' + return ContextDict(pairs, context=context), pairs, context + + def _construct(self, node): + return self._pairs(node)[0] + + +ContextLoader.add_constructor( + ContextLoader.DEFAULT_MAPPING_TAG, ContextLoader._construct +) + + # Found http://stackoverflow.com/a/21912744 which guided me on how to hook in # here -class SortEnforcingLoader(SafeLoader): +class SortEnforcingLoader(ContextLoader): def _construct(self, node): - self.flatten_mapping(node) - ret = self.construct_pairs(node) - keys = [d[0] for d in ret] + ret, pairs, context = self._pairs(node) + + keys = [d[0] for d in pairs] keys_sorted = sorted(keys, key=_natsort_key) for key in keys: expected = keys_sorted.pop(0) @@ -25,9 +44,10 @@ class SortEnforcingLoader(SafeLoader): None, None, 'keys out of order: ' - f'expected {expected} got {key} at ' + str(node.start_mark), + f'expected {expected} got {key} at {context}', ) - return dict(ret) + + return ret SortEnforcingLoader.add_constructor( @@ -36,7 +56,7 @@ SortEnforcingLoader.add_constructor( def safe_load(stream, enforce_order=True): - return load(stream, SortEnforcingLoader if enforce_order else SafeLoader) + return load(stream, SortEnforcingLoader if enforce_order else ContextLoader) class SortingDumper(SafeDumper): diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 464472f..8998610 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -104,13 +104,15 @@ class TestManager(TestCase): with self.assertRaises(ManagerException) as ctx: name = 'bad-plan-output-missing-class.yaml' Manager(get_config_filename(name)).sync() - self.assertEqual('plan_output bad is missing class', str(ctx.exception)) + self.assertTrue( + 'plan_output bad is missing class' in str(ctx.exception) + ) def test_bad_plan_output_config(self): with self.assertRaises(ManagerException) as ctx: Manager(get_config_filename('bad-plan-output-config.yaml')).sync() - self.assertEqual( - 'Incorrect plan_output config for bad', str(ctx.exception) + self.assertTrue( + 'Incorrect plan_output config for bad' in str(ctx.exception) ) def test_source_only_as_a_target(self): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index becc802..29561c3 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -22,6 +22,7 @@ from octodns.record import ( ValidationError, ValuesMixin, ) +from octodns.yaml import ContextDict from octodns.zone import Zone @@ -572,3 +573,58 @@ class TestRecordValidation(TestCase): }, lenient=True, ) + + def test_validation_context(self): + # fails validation, no context + with self.assertRaises(ValidationError) as ctx: + Record.new( + self.zone, 'www', {'type': 'A', 'ttl': -1, 'value': '1.2.3.4'} + ) + self.assertFalse(', line' in str(ctx.exception)) + + # fails validation, with context + with self.assertRaises(ValidationError) as ctx: + Record.new( + self.zone, + 'www', + ContextDict( + {'type': 'A', 'ttl': -1, 'value': '1.2.3.4'}, + context='needle', + ), + ) + self.assertTrue('needle' in str(ctx.exception)) + + def test_invalid_type_context(self): + # fails validation, no context + with self.assertRaises(Exception) as ctx: + Record.new( + self.zone, 'www', {'type': 'X', 'ttl': 42, 'value': '1.2.3.4'} + ) + self.assertFalse(', line' in str(ctx.exception)) + + # fails validation, with context + with self.assertRaises(Exception) as ctx: + Record.new( + self.zone, + 'www', + ContextDict( + {'type': 'X', 'ttl': 42, 'value': '1.2.3.4'}, + context='needle', + ), + ) + self.assertTrue('needle' in str(ctx.exception)) + + def test_missing_type_context(self): + # fails validation, no context + with self.assertRaises(Exception) as ctx: + Record.new(self.zone, 'www', {'ttl': 42, 'value': '1.2.3.4'}) + self.assertFalse(', line' in str(ctx.exception)) + + # fails validation, with context + with self.assertRaises(Exception) as ctx: + Record.new( + self.zone, + 'www', + ContextDict({'ttl': 42, 'value': '1.2.3.4'}, context='needle'), + ) + self.assertTrue('needle' in str(ctx.exception))