From 639ace9ce4d62281fa295b933f237a16c07ef499 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 8 Apr 2019 12:35:58 -0700 Subject: [PATCH 1/2] DynProvider dynamic serve_count=1 to match Route53 Might be possible/make sense to allow it to be configured later, but for now Route53 doesn't support it so we'll go with 1 to make sure that things match up behavior-wise. --- octodns/provider/dyn.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index b63a0c2..8193cde 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -927,9 +927,8 @@ class DynProvider(BaseProvider): # We don't have this pool and thus need to create it records_for = getattr(self, '_dynamic_records_for_{}'.format(_type)) records = records_for(values, record_extras) - record_set = DSFRecordSet(_type, label, - serve_count=min(len(records), 2), - records=records, dsf_monitor_id=monitor_id) + record_set = DSFRecordSet(_type, label, serve_count=1, records=records, + dsf_monitor_id=monitor_id) chain = DSFFailoverChain(label, record_sets=[record_set]) pool = DSFResponsePool(label, rs_chains=[chain]) pool.create(td) From 9f63a508e4222d0e16bc1170477e76c1825bee8f Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 8 Apr 2019 14:22:07 -0700 Subject: [PATCH 2/2] Address TODO about Dyn TD's and subzones to fix bug We hit this bug internally. It generally needed to be fix and was a larger potential problem than expected since it was assuming TD assocaitions rather than looking at them directly. Test changes were a little involved to suss out, otherwise this is a fairly clean and simple fix. --- octodns/provider/dyn.py | 8 +++--- tests/test_octodns_provider_dyn.py | 39 +++++++++++++++++++++++------- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index b63a0c2..c7bf3fd 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -624,10 +624,12 @@ class DynProvider(BaseProvider): zone.name, lenient) td_records = set() for fqdn, types in self.traffic_directors.items(): - # TODO: skip subzones - if not fqdn.endswith(zone.name): - continue for _type, td in types.items(): + # Does this TD belong to the current zone + td_zone = '{}.'.format(td.nodes[0]['zone']) + if td_zone != zone.name: + # Doesn't belong to the current zone, skip it + continue # critical to call rulesets once, each call loads them :-( rulesets = td.rulesets if self._is_traffic_director_dyanmic(td, rulesets): diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index 79d764d..4f224fc 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/tests/test_octodns_provider_dyn.py @@ -980,26 +980,34 @@ class TestDynProviderGeo(TestCase): provider = DynProvider('test', 'cust', 'user', 'pass', traffic_directors_enabled=True) + got = Zone('unit.tests.', []) + zone_name = got.name[:-1] # only traffic director mock.side_effect = [ # get traffic directors self.traffic_directors_response, - # get traffic director + # get the first td's nodes + {'data': [{'fqdn': zone_name, 'zone': zone_name}]}, + # get traffic director, b/c ^ matches self.traffic_director_response, + # get the next td's nodes, not a match + {'data': [{'fqdn': 'other', 'zone': 'other'}]}, # get zone {'data': {}}, # get records {'data': {}}, ] - got = Zone('unit.tests.', []) provider.populate(got) self.assertEquals(1, len(got.records)) self.assertFalse(self.expected_geo.changes(got, provider)) mock.assert_has_calls([ + call('/DSF/', 'GET', {'detail': 'Y'}), + call('/DSFNode/2ERWXQNsb_IKG2YZgYqkPvk0PBM', 'GET', {}), call('/DSF/2ERWXQNsb_IKG2YZgYqkPvk0PBM/', 'GET', {'pending_changes': 'Y'}), + call('/DSFNode/3ERWXQNsb_IKG2YZgYqkPvk0PBM', 'GET', {}), call('/Zone/unit.tests/', 'GET', {}), - call('/AllRecord/unit.tests/unit.tests./', 'GET', {'detail': 'Y'}), + call('/AllRecord/unit.tests/unit.tests./', 'GET', {'detail': 'Y'}) ]) @patch('dyn.core.SessionEngine.execute') @@ -1035,8 +1043,12 @@ class TestDynProviderGeo(TestCase): mock.side_effect = [ # get traffic directors self.traffic_directors_response, - # get traffic director + # grab its nodes, matches + {'data': [{'fqdn': 'unit.tests', 'zone': 'unit.tests'}]}, + # get traffic director b/c match self.traffic_director_response, + # grab next td's nodes, not a match + {'data': [{'fqdn': 'other', 'zone': 'other'}]}, # get zone {'data': {}}, # get records @@ -1047,10 +1059,13 @@ class TestDynProviderGeo(TestCase): self.assertEquals(1, len(got.records)) self.assertFalse(self.expected_geo.changes(got, provider)) mock.assert_has_calls([ + call('/DSF/', 'GET', {'detail': 'Y'}), + call('/DSFNode/2ERWXQNsb_IKG2YZgYqkPvk0PBM', 'GET', {}), call('/DSF/2ERWXQNsb_IKG2YZgYqkPvk0PBM/', 'GET', {'pending_changes': 'Y'}), + call('/DSFNode/3ERWXQNsb_IKG2YZgYqkPvk0PBM', 'GET', {}), call('/Zone/unit.tests/', 'GET', {}), - call('/AllRecord/unit.tests/unit.tests./', 'GET', {'detail': 'Y'}), + call('/AllRecord/unit.tests/unit.tests./', 'GET', {'detail': 'Y'}) ]) @patch('dyn.core.SessionEngine.execute') @@ -1085,8 +1100,10 @@ class TestDynProviderGeo(TestCase): mock.side_effect = [ # get traffic directors self.traffic_directors_response, + {'data': [{'fqdn': 'unit.tests', 'zone': 'unit.tests'}]}, # get traffic director busted_traffic_director_response, + {'data': [{'fqdn': 'other', 'zone': 'other'}]}, # get zone {'data': {}}, # get records @@ -1099,10 +1116,13 @@ class TestDynProviderGeo(TestCase): # so just compare set contents (which does name and type) self.assertEquals(self.expected_geo.records, got.records) mock.assert_has_calls([ + call('/DSF/', 'GET', {'detail': 'Y'}), + call('/DSFNode/2ERWXQNsb_IKG2YZgYqkPvk0PBM', 'GET', {}), call('/DSF/2ERWXQNsb_IKG2YZgYqkPvk0PBM/', 'GET', {'pending_changes': 'Y'}), + call('/DSFNode/3ERWXQNsb_IKG2YZgYqkPvk0PBM', 'GET', {}), call('/Zone/unit.tests/', 'GET', {}), - call('/AllRecord/unit.tests/unit.tests./', 'GET', {'detail': 'Y'}), + call('/AllRecord/unit.tests/unit.tests./', 'GET', {'detail': 'Y'}) ]) @patch('dyn.core.SessionEngine.execute') @@ -1625,11 +1645,12 @@ class DummyRuleset(object): class DummyTrafficDirector(object): - def __init__(self, rulesets=[], response_pools=[], ttl=42): + def __init__(self, zone_name, rulesets=[], response_pools=[], ttl=42): self.label = 'dummy:abcdef1234567890' self.rulesets = rulesets self.all_response_pools = response_pools self.ttl = ttl + self.nodes = [{'zone': zone_name[:-1]}] class TestDynProviderDynamic(TestCase): @@ -1880,9 +1901,9 @@ class TestDynProviderDynamic(TestCase): }, }), ] - td = DummyTrafficDirector(rulesets, [default_response_pool, - pool1_response_pool]) zone = Zone('unit.tests.', []) + td = DummyTrafficDirector(zone.name, rulesets, + [default_response_pool, pool1_response_pool]) record = provider._populate_dynamic_traffic_director(zone, fqdn, 'A', td, rulesets, True)