From aa58631dcd6c5faf2108787d2f79736596d86f07 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Fri, 20 Mar 2020 13:15:14 -0700 Subject: [PATCH 1/2] Validate dynamic rules do not reuse pools --- octodns/record/__init__.py | 13 +++++-- tests/config/dynamic.tests.yaml | 6 +-- tests/config/split/dynamic.tests./a.yaml | 2 +- tests/config/split/dynamic.tests./aaaa.yaml | 2 +- tests/config/split/dynamic.tests./cname.yaml | 2 +- tests/test_octodns_record.py | 40 ++++++++++++++++++++ 6 files changed, 56 insertions(+), 9 deletions(-) diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index 98c1836..70066fe 100644 --- a/octodns/record/__init__.py +++ b/octodns/record/__init__.py @@ -579,6 +579,7 @@ class _DynamicMixin(object): # TODO: don't allow 'default' as a pool name, reserved # TODO: warn or error on unused pools? + pools_seen = set() for i, rule in enumerate(rules): rule_num = i + 1 try: @@ -590,9 +591,15 @@ class _DynamicMixin(object): if not isinstance(pool, string_types): reasons.append('rule {} invalid pool "{}"' .format(rule_num, pool)) - elif pool not in pools: - reasons.append('rule {} undefined pool "{}"' - .format(rule_num, pool)) + else: + if pool not in pools: + reasons.append('rule {} undefined pool "{}"' + .format(rule_num, pool)) + pools_seen.add(pool) + elif pool in pools_seen: + reasons.append('rule {} invalid, target pool "{}" ' + 'reused'.format(rule_num, pool)) + pools_seen.add(pool) try: geos = rule['geos'] diff --git a/tests/config/dynamic.tests.yaml b/tests/config/dynamic.tests.yaml index f826880..4bd97a7 100644 --- a/tests/config/dynamic.tests.yaml +++ b/tests/config/dynamic.tests.yaml @@ -23,7 +23,7 @@ a: rules: - geos: - EU-GB - pool: iad + pool: lax - geos: - EU pool: ams @@ -60,7 +60,7 @@ aaaa: rules: - geos: - EU-GB - pool: iad + pool: lax - geos: - EU pool: ams @@ -96,7 +96,7 @@ cname: rules: - geos: - EU-GB - pool: iad + pool: lax - geos: - EU pool: ams diff --git a/tests/config/split/dynamic.tests./a.yaml b/tests/config/split/dynamic.tests./a.yaml index f182df6..3027686 100644 --- a/tests/config/split/dynamic.tests./a.yaml +++ b/tests/config/split/dynamic.tests./a.yaml @@ -29,7 +29,7 @@ a: rules: - geos: - EU-GB - pool: iad + pool: lax - geos: - EU pool: ams diff --git a/tests/config/split/dynamic.tests./aaaa.yaml b/tests/config/split/dynamic.tests./aaaa.yaml index 3b1dcb7..a2d8779 100644 --- a/tests/config/split/dynamic.tests./aaaa.yaml +++ b/tests/config/split/dynamic.tests./aaaa.yaml @@ -29,7 +29,7 @@ aaaa: rules: - geos: - EU-GB - pool: iad + pool: lax - geos: - EU pool: ams diff --git a/tests/config/split/dynamic.tests./cname.yaml b/tests/config/split/dynamic.tests./cname.yaml index ff85955..b716ad9 100644 --- a/tests/config/split/dynamic.tests./cname.yaml +++ b/tests/config/split/dynamic.tests./cname.yaml @@ -27,7 +27,7 @@ cname: rules: - geos: - EU-GB - pool: iad + pool: lax - geos: - EU pool: ams diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index f313342..749e686 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -3377,6 +3377,46 @@ class TestDynamicRecords(TestCase): self.assertEquals(['rule 2 duplicate default'], ctx.exception.reasons) + # repeated pool in rules + a_data = { + 'dynamic': { + 'pools': { + 'one': { + 'values': [{ + 'value': '3.3.3.3', + }] + }, + 'two': { + 'values': [{ + 'value': '4.4.4.4', + }, { + 'value': '5.5.5.5', + }] + }, + }, + 'rules': [{ + 'geos': ['EU'], + 'pool': 'two', + }, { + 'geos': ['AF'], + 'pool': 'one', + }, { + 'pool': 'one', + }], + }, + 'ttl': 60, + 'type': 'A', + 'values': [ + '1.1.1.1', + '2.2.2.2', + ], + } + print('\n*** start ***') + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'bad', a_data) + self.assertEquals(['rule 3 invalid, target pool "one" reused'], + ctx.exception.reasons) + def test_dynamic_lenient(self): # Missing pools a_data = { From d35c13685893ec33ec0c04d886b8aeff8bec0656 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Fri, 20 Mar 2020 13:32:37 -0700 Subject: [PATCH 2/2] Warn about unused pools, ones not referenced by a rule --- octodns/record/__init__.py | 11 +++++++++-- tests/test_octodns_record.py | 35 +++++++++++++++++++++++++---------- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index 70066fe..0eb9431 100644 --- a/octodns/record/__init__.py +++ b/octodns/record/__init__.py @@ -507,6 +507,8 @@ class _DynamicMixin(object): except KeyError: pools = {} + pools_exist = set() + pools_seen = set() if not isinstance(pools, dict): reasons.append('pools must be a dict') elif not pools: @@ -522,6 +524,8 @@ class _DynamicMixin(object): reasons.append('pool "{}" is missing values'.format(_id)) continue + pools_exist.add(_id) + for i, value in enumerate(values): value_num = i + 1 try: @@ -578,8 +582,6 @@ class _DynamicMixin(object): seen_default = False # TODO: don't allow 'default' as a pool name, reserved - # TODO: warn or error on unused pools? - pools_seen = set() for i, rule in enumerate(rules): rule_num = i + 1 try: @@ -618,6 +620,11 @@ class _DynamicMixin(object): reasons.extend(GeoCodes.validate(geo, 'rule {} ' .format(rule_num))) + unused = pools_exist - pools_seen + if unused: + unused = '", "'.join(sorted(unused)) + reasons.append('unused pools: "{}"'.format(unused)) + return reasons def __init__(self, zone, name, data, *args, **kwargs): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 749e686..4216c52 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -3075,7 +3075,7 @@ class TestDynamicRecords(TestCase): 'invalid IPv4 address "blip"', ], ctx.exception.reasons) - # missing rules + # missing rules, and unused pools a_data = { 'dynamic': { 'pools': { @@ -3102,7 +3102,10 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(['missing rules'], ctx.exception.reasons) + self.assertEquals([ + 'missing rules', + 'unused pools: "one", "two"', + ], ctx.exception.reasons) # empty rules a_data = { @@ -3132,7 +3135,10 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(['missing rules'], ctx.exception.reasons) + self.assertEquals([ + 'missing rules', + 'unused pools: "one", "two"', + ], ctx.exception.reasons) # rules not a list/tuple a_data = { @@ -3162,7 +3168,10 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(['rules must be a list'], ctx.exception.reasons) + self.assertEquals([ + 'rules must be a list', + 'unused pools: "one", "two"', + ], ctx.exception.reasons) # rule without pool a_data = { @@ -3196,7 +3205,10 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(['rule 1 missing pool'], ctx.exception.reasons) + self.assertEquals([ + 'rule 1 missing pool', + 'unused pools: "two"', + ], ctx.exception.reasons) # rule with non-string pools a_data = { @@ -3231,8 +3243,10 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(['rule 1 invalid pool "[]"'], - ctx.exception.reasons) + self.assertEquals([ + 'rule 1 invalid pool "[]"', + 'unused pools: "two"', + ], ctx.exception.reasons) # rule references non-existent pool a_data = { @@ -3267,8 +3281,10 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(["rule 1 undefined pool \"non-existent\""], - ctx.exception.reasons) + self.assertEquals([ + "rule 1 undefined pool \"non-existent\"", + 'unused pools: "two"', + ], ctx.exception.reasons) # rule with invalid geos a_data = { @@ -3411,7 +3427,6 @@ class TestDynamicRecords(TestCase): '2.2.2.2', ], } - print('\n*** start ***') with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) self.assertEquals(['rule 3 invalid, target pool "one" reused'],