diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index 98c1836..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,7 +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? for i, rule in enumerate(rules): rule_num = i + 1 try: @@ -590,9 +593,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'] @@ -611,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/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..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 = { @@ -3377,6 +3393,45 @@ 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', + ], + } + 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 = {