From 72244ba16153099944f3b651fccfa7f79f3bf50c Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 27 Aug 2022 15:23:52 -0700 Subject: [PATCH 1/4] Provider._process_desired_zone should call .supports, rather than do in SUPPORTS --- octodns/provider/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 78001fa..19a86dc 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -59,7 +59,7 @@ class BaseProvider(BaseSource): ''' for record in desired.records: - if record._type not in self.SUPPORTS: + if not self.supports(record): msg = f'{record._type} records not supported for {record.fqdn}' fallback = 'omitting record' self.supports_warn_or_except(msg, fallback) From 557b80f7847e14b179694af66c77afecb9cc74e0 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 27 Aug 2022 15:40:02 -0700 Subject: [PATCH 2/4] Implement YamlProvider.supports that always says yes --- octodns/provider/yaml.py | 8 ++++++++ tests/test_octodns_provider_yaml.py | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/octodns/provider/yaml.py b/octodns/provider/yaml.py index 9ad0934..ee436f2 100644 --- a/octodns/provider/yaml.py +++ b/octodns/provider/yaml.py @@ -167,6 +167,14 @@ class YamlProvider(BaseProvider): del args['log'] return self.__class__(**args) + def supports(self, record): + # We're overriding this as a performance tweak, namely to avoid calling + # the implementation of the SUPPORTS property to create a set from a + # dict_keys every single time something checked whether we support a + # record, the answer is always yes so that's overkill and we can just + # return True here and be done with it + return True + @property def SUPPORTS_ROOT_NS(self): return self.supports_root_ns diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py index e2f55c2..99af97d 100644 --- a/tests/test_octodns_provider_yaml.py +++ b/tests/test_octodns_provider_yaml.py @@ -217,6 +217,20 @@ class TestYamlProvider(TestCase): str(ctx.exception), ) + def test_supports_everything(self): + source = YamlProvider('test', join(dirname(__file__), 'config')) + + class DummyType(object): + def __init__(self, _type): + self._type = _type + + # No matter what we check it's always supported + self.assertTrue(source.supports(DummyType(None))) + self.assertTrue(source.supports(DummyType(42))) + self.assertTrue(source.supports(DummyType('A'))) + self.assertTrue(source.supports(DummyType(source))) + self.assertTrue(source.supports(DummyType(self))) + class TestSplitYamlProvider(TestCase): def test_list_all_yaml_files(self): From 2e3d325f71285f9812a1ddcffae21b28b648a8a5 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 27 Aug 2022 15:53:56 -0700 Subject: [PATCH 3/4] YamlProvider.SUPPORTS dynamically returns the list of registered types --- octodns/provider/yaml.py | 28 ++++++++-------------------- octodns/record/__init__.py | 4 ++++ tests/test_octodns_provider_yaml.py | 20 ++++++++++++++++++-- tests/test_octodns_record.py | 4 ++++ 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/octodns/provider/yaml.py b/octodns/provider/yaml.py index ee436f2..5044332 100644 --- a/octodns/provider/yaml.py +++ b/octodns/provider/yaml.py @@ -111,26 +111,6 @@ class YamlProvider(BaseProvider): SUPPORTS_DYNAMIC = True SUPPORTS_POOL_VALUE_STATUS = True SUPPORTS_MULTIVALUE_PTR = True - SUPPORTS = set( - ( - 'A', - 'AAAA', - 'ALIAS', - 'CAA', - 'CNAME', - 'DNAME', - 'LOC', - 'MX', - 'NAPTR', - 'NS', - 'PTR', - 'SSHFP', - 'SPF', - 'SRV', - 'TXT', - 'URLFWD', - ) - ) def __init__( self, @@ -167,6 +147,14 @@ class YamlProvider(BaseProvider): del args['log'] return self.__class__(**args) + @property + def SUPPORTS(self): + # The yaml provider supports all record types even those defined by 3rd + # party modules that we know nothing about, thus we dynamically return + # the types list that is registered in Record, everything that's know as + # of the point in time we're asked + return set(Record.registered_types().keys()) + def supports(self, record): # We're overriding this as a performance tweak, namely to avoid calling # the implementation of the SUPPORTS property to create a set from a diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index d7c6be4..6cb6bf1 100644 --- a/octodns/record/__init__.py +++ b/octodns/record/__init__.py @@ -102,6 +102,10 @@ class Record(EqualityTupleMixin): raise RecordException(msg) cls._CLASSES[_type] = _class + @classmethod + def registered_types(cls): + return cls._CLASSES + @classmethod def new(cls, zone, name, data, source=None, lenient=False): name = str(name).lower() diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py index 99af97d..811aae4 100644 --- a/tests/test_octodns_provider_yaml.py +++ b/tests/test_octodns_provider_yaml.py @@ -15,7 +15,7 @@ from unittest import TestCase from yaml import safe_load from yaml.constructor import ConstructorError -from octodns.record import Create +from octodns.record import _NsValue, Create, Record, ValuesMixin from octodns.provider.base import Plan from octodns.provider.yaml import ( _list_all_yaml_files, @@ -217,7 +217,23 @@ class TestYamlProvider(TestCase): str(ctx.exception), ) - def test_supports_everything(self): + def test_SUPPORTS(self): + source = YamlProvider('test', join(dirname(__file__), 'config')) + # make sure the provider supports all the registered types + self.assertEqual(Record.registered_types().keys(), source.SUPPORTS) + + class YamlRecord(ValuesMixin, Record): + _type = 'YAML' + _value_type = _NsValue + + # don't know anything about a yaml type + self.assertTrue('YAML' not in source.SUPPORTS) + # register it + Record.register_type(YamlRecord) + # when asked again we'll now include it in our list of supports + self.assertTrue('YAML' in source.SUPPORTS) + + def test_supports(self): source = YamlProvider('test', join(dirname(__file__), 'config')) class DummyType(object): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 2bbea5d..626f325 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -69,6 +69,8 @@ class TestRecord(TestCase): _type = 'AA' _value_type = _NsValue + self.assertTrue('AA' not in Record.registered_types()) + Record.register_type(AaRecord) aa = Record.new( self.zone, @@ -77,6 +79,8 @@ class TestRecord(TestCase): ) self.assertEqual(AaRecord, aa.__class__) + self.assertTrue('AA' in Record.registered_types()) + def test_lowering(self): record = ARecord( self.zone, 'MiXeDcAsE', {'ttl': 30, 'type': 'A', 'value': '1.2.3.4'} From b664a28488dcec4a7daa5fe671de6d913945ca92 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 27 Aug 2022 15:54:48 -0700 Subject: [PATCH 4/4] Changelog update about Provider._process_desired_zone and YamlProvider.supports --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f36874..78685f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +## v0.9.19 - 2022-??-?? - ??? + +* Addressed shortcomings with YamlProvider.SUPPORTS in that it didn't include + dynamically registered types, was a static list that could have drifted over + time even ignoring 3rd party types. +* Provider._process_desired_zone needed to call Provider.supports rather than + doing it's own `_type in provider.SUPPORTS`. The default behavior in + Source.supports is ^, but it's possible for providers to override that + behavior and do special checking and `_process_desired_zone` wasn't taking + that into account. +* Now that it's used as it needed to be YamlProvider overrides + Provider.supports and just always says Yes so that any dynamically registered + types will be supported. + ## v0.9.18 - 2022-08-14 - Subzone handling * Fixed issue with sub-zone handling introduced in 0.9.18