From 19798e3acfc65bc2c2b90495588fc6a57d9f668e Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 2 Nov 2020 07:26:07 -0800 Subject: [PATCH 1/4] Only allow ALIAS on APEX --- octodns/record/__init__.py | 8 +++++ tests/fixtures/constellix-records.json | 37 --------------------- tests/fixtures/dnsmadeeasy-records.json | 14 -------- tests/test_octodns_provider_constellix.py | 12 ++----- tests/test_octodns_provider_dnsmadeeasy.py | 12 ++----- tests/test_octodns_provider_mythicbeasts.py | 4 +-- tests/test_octodns_record.py | 14 ++++++-- 7 files changed, 28 insertions(+), 73 deletions(-) diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index 08ec2ee..d42b576 100644 --- a/octodns/record/__init__.py +++ b/octodns/record/__init__.py @@ -782,6 +782,14 @@ class AliasRecord(_ValueMixin, Record): _type = 'ALIAS' _value_type = AliasValue + @classmethod + def validate(cls, name, fqdn, data): + reasons = [] + if name != '': + reasons.append('non-root ALIAS not allowed') + reasons.extend(super(AliasRecord, cls).validate(name, fqdn, data)) + return reasons + class CaaValue(EqualityTupleMixin): # https://tools.ietf.org/html/rfc6844#page-5 diff --git a/tests/fixtures/constellix-records.json b/tests/fixtures/constellix-records.json index c1f1fb4..689fd53 100644 --- a/tests/fixtures/constellix-records.json +++ b/tests/fixtures/constellix-records.json @@ -523,43 +523,6 @@ "roundRobinFailover": [], "pools": [], "poolsDetail": [] -}, { - "id": 1808603, - "type": "ANAME", - "recordType": "aname", - "name": "sub", - "recordOption": "roundRobin", - "noAnswer": false, - "note": "", - "ttl": 1800, - "gtdRegion": 1, - "parentId": 123123, - "parent": "domain", - "source": "Domain", - "modifiedTs": 1565153387855, - "value": [{ - "value": "aname.unit.tests.", - "disableFlag": false - }], - "roundRobin": [{ - "value": "aname.unit.tests.", - "disableFlag": false - }], - "geolocation": null, - "recordFailover": { - "disabled": false, - "failoverType": 1, - "failoverTypeStr": "Normal (always lowest level)", - "values": [] - }, - "failover": { - "disabled": false, - "failoverType": 1, - "failoverTypeStr": "Normal (always lowest level)", - "values": [] - }, - "pools": [], - "poolsDetail": [] }, { "id": 1808520, "type": "A", diff --git a/tests/fixtures/dnsmadeeasy-records.json b/tests/fixtures/dnsmadeeasy-records.json index 4d3ba64..aefd6ce 100644 --- a/tests/fixtures/dnsmadeeasy-records.json +++ b/tests/fixtures/dnsmadeeasy-records.json @@ -320,20 +320,6 @@ "name": "", "value": "aname.unit.tests.", "id": 11189895, - "type": "ANAME" - }, { - "failover": false, - "monitor": false, - "sourceId": 123123, - "dynamicDns": false, - "failed": false, - "gtdLocation": "DEFAULT", - "hardLink": false, - "ttl": 1800, - "source": 1, - "name": "sub", - "value": "aname", - "id": 11189896, "type": "ANAME" }, { "failover": false, diff --git a/tests/test_octodns_provider_constellix.py b/tests/test_octodns_provider_constellix.py index c19ae29..bc17b50 100644 --- a/tests/test_octodns_provider_constellix.py +++ b/tests/test_octodns_provider_constellix.py @@ -42,12 +42,6 @@ class TestConstellixProvider(TestCase): 'value': 'aname.unit.tests.' })) - expected.add_record(Record.new(expected, 'sub', { - 'ttl': 1800, - 'type': 'ALIAS', - 'value': 'aname.unit.tests.' - })) - for record in list(expected.records): if record.name == 'sub' and record._type == 'NS': expected._remove_record(record) @@ -107,14 +101,14 @@ class TestConstellixProvider(TestCase): zone = Zone('unit.tests.', []) provider.populate(zone) - self.assertEquals(15, len(zone.records)) + self.assertEquals(14, len(zone.records)) changes = self.expected.changes(zone, provider) self.assertEquals(0, len(changes)) # 2nd populate makes no network calls/all from cache again = Zone('unit.tests.', []) provider.populate(again) - self.assertEquals(15, len(again.records)) + self.assertEquals(14, len(again.records)) # bust the cache del provider._zone_records[zone.name] @@ -169,7 +163,7 @@ class TestConstellixProvider(TestCase): }), ]) - self.assertEquals(18, provider._client._request.call_count) + self.assertEquals(17, provider._client._request.call_count) provider._client._request.reset_mock() diff --git a/tests/test_octodns_provider_dnsmadeeasy.py b/tests/test_octodns_provider_dnsmadeeasy.py index 50fa576..0ad059d 100644 --- a/tests/test_octodns_provider_dnsmadeeasy.py +++ b/tests/test_octodns_provider_dnsmadeeasy.py @@ -44,12 +44,6 @@ class TestDnsMadeEasyProvider(TestCase): 'value': 'aname.unit.tests.' })) - expected.add_record(Record.new(expected, 'sub', { - 'ttl': 1800, - 'type': 'ALIAS', - 'value': 'aname.unit.tests.' - })) - for record in list(expected.records): if record.name == 'sub' and record._type == 'NS': expected._remove_record(record) @@ -108,14 +102,14 @@ class TestDnsMadeEasyProvider(TestCase): zone = Zone('unit.tests.', []) provider.populate(zone) - self.assertEquals(15, len(zone.records)) + self.assertEquals(14, len(zone.records)) changes = self.expected.changes(zone, provider) self.assertEquals(0, len(changes)) # 2nd populate makes no network calls/all from cache again = Zone('unit.tests.', []) provider.populate(again) - self.assertEquals(15, len(again.records)) + self.assertEquals(14, len(again.records)) # bust the cache del provider._zone_records[zone.name] @@ -180,7 +174,7 @@ class TestDnsMadeEasyProvider(TestCase): 'port': 30 }), ]) - self.assertEquals(27, provider._client._request.call_count) + self.assertEquals(26, provider._client._request.call_count) provider._client._request.reset_mock() diff --git a/tests/test_octodns_provider_mythicbeasts.py b/tests/test_octodns_provider_mythicbeasts.py index 960bd65..f78cb0b 100644 --- a/tests/test_octodns_provider_mythicbeasts.py +++ b/tests/test_octodns_provider_mythicbeasts.py @@ -171,7 +171,7 @@ class TestMythicBeastsProvider(TestCase): def test_command_generation(self): zone = Zone('unit.tests.', []) - zone.add_record(Record.new(zone, 'prawf-alias', { + zone.add_record(Record.new(zone, '', { 'ttl': 60, 'type': 'ALIAS', 'value': 'alias.unit.tests.', @@ -228,7 +228,7 @@ class TestMythicBeastsProvider(TestCase): ) expected_commands = [ - 'ADD prawf-alias.unit.tests 60 ANAME alias.unit.tests.', + 'ADD unit.tests 60 ANAME alias.unit.tests.', 'ADD prawf-ns.unit.tests 300 NS alias.unit.tests.', 'ADD prawf-ns.unit.tests 300 NS alias2.unit.tests.', 'ADD prawf-a.unit.tests 60 A 1.2.3.4', diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 524f8f2..ffca1d0 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -1710,6 +1710,16 @@ class TestRecordValidation(TestCase): 'value': 'foo.bar.com.', }) + # root only + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'nope', { + 'type': 'ALIAS', + 'ttl': 600, + 'value': 'foo.bar.com.', + }) + self.assertEquals(['non-root ALIAS not allowed'], + ctx.exception.reasons) + # missing value with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, '', { @@ -1720,7 +1730,7 @@ class TestRecordValidation(TestCase): # missing value with self.assertRaises(ValidationError) as ctx: - Record.new(self.zone, 'www', { + Record.new(self.zone, '', { 'type': 'ALIAS', 'ttl': 600, 'value': None @@ -1729,7 +1739,7 @@ class TestRecordValidation(TestCase): # empty value with self.assertRaises(ValidationError) as ctx: - Record.new(self.zone, 'www', { + Record.new(self.zone, '', { 'type': 'ALIAS', 'ttl': 600, 'value': '' From 364b70048fbf91251438db76bed3148660cf7a83 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 2 Nov 2020 07:27:48 -0800 Subject: [PATCH 2/4] Fix coverage pragma grep --- script/coverage | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/coverage b/script/coverage index 32bdaea..bd6e4c9 100755 --- a/script/coverage +++ b/script/coverage @@ -27,7 +27,7 @@ export DYN_USERNAME= export GOOGLE_APPLICATION_CREDENTIALS= # Don't allow disabling coverage -grep -r -I --line-number "# pragma: nocover" octodns && { +grep -r -I --line-number "# pragma: +no.*cover" octodns && { echo "Code coverage should not be disabled" exit 1 } From b017f90c669d17113ec53db055fbc3ad2ba02aee Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 3 Nov 2020 10:27:31 -0800 Subject: [PATCH 3/4] Add some docs around lenient and its uses --- docs/records.md | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/docs/records.md b/docs/records.md index a28e86f..74c0b1c 100644 --- a/docs/records.md +++ b/docs/records.md @@ -6,15 +6,17 @@ OctoDNS supports the following record types: * `A` * `AAAA` +* `ALIAS` +* `CAA` * `CNAME` * `DNAME` * `MX` * `NAPTR` * `NS` * `PTR` -* `SSHFP` * `SPF` * `SRV` +* `SSHFP` * `TXT` Underlying provider support for each of these varies and some providers have extra requirements or limitations. In cases where a record type is not supported by a provider OctoDNS will ignore it there and continue to manage the record elsewhere. For example `SSHFP` is supported by Dyn, but not Route53. If your source data includes an SSHFP record OctoDNS will keep it in sync on Dyn, but not consider it when evaluating the state of Route53. The best way to find out what types are supported by a provider is to look for its `supports` method. If that method exists the logic will drive which records are supported and which are ignored. If the provider does not implement the method it will fall back to `BaseProvider.supports` which indicates full support. @@ -82,3 +84,37 @@ In the above example each name had a single record, but there are cases where a Each record type has a corresponding set of required data. The easiest way to determine what's required is probably to look at the record object in [`octodns/record/__init__.py`](/octodns/record/__init__.py). You may also utilize `octodns-validate` which will throw errors about what's missing when run. `type` is required for all records. `ttl` is optional. When TTL is not specified the `YamlProvider`'s default will be used. In any situation where an array of `values` can be used you can opt to go with `value` as a single item if there's only one. + +### Lenience + +octoDNS is fairly strict in terms of standards compliance and is opinionated in terms of best practices. Examples of former include SRV record naming requirements and the latter that ALIAS records are constrained to the root of zones. The strictness and support of providers varies so you may encounter existing records that fail validation when you try to dump them or you may even have use cases for which you need to create or preserve records that don't validate. octoDNS's solution to this is the `lenient` flag. + +#### octodns-dump + +If you're trying to import a zone into octoDNS config file using `octodns-dump` which fails due to validation errors you can supply the `--lenient` argument to tell octoDNS that you acknowledge that things aren't lining up with its expectations, but you'd like it to go ahead anyway. This will do its best to populate the zone and dump the results out into an octoDNS zone file and include the non-compliant bits. If you go to use that config file octoDNS will again complain about the validation problems. You can correct them in cases where that makes sense, but if you need to preserve the non-compliant records read on for options. + +#### Record level lenience + +When there are non-compliant records configured in Yaml you can add the following to tell octoDNS to do it's best to proceed with them anyway. If you use `--lenient` above to dump a zone and you'd like to sync it as-is you can mark the problematic records this way. + +```yaml +'not-root': + octodns: + lenient: true + type: ALIAS + values: something.else.com. +``` + +#### Zone level lenience + +If you'd like to enable lenience for a whole zone you can do so with the following, thought it's strongly encouraged to mark things at record level when possible. The most common case where things may need to be done at the zone level is when using something other than `YamlProvider` as a source, e.g. syncing from `Route53Provider` to `Ns1Provider` when there are non-compliant records in the zone in Route53. + +```yaml + non-compliant-zone.com.: + octodns: + lenient: true + sources: + - route53 + targets: + - ns1 +``` From 269e737812247b3763869398342e1fb54c4952a0 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 3 Nov 2020 10:35:55 -0800 Subject: [PATCH 4/4] Add a caveat emptor clause to lenient doc section. --- docs/records.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/records.md b/docs/records.md index 74c0b1c..d287d8a 100644 --- a/docs/records.md +++ b/docs/records.md @@ -89,6 +89,8 @@ Each record type has a corresponding set of required data. The easiest way to de octoDNS is fairly strict in terms of standards compliance and is opinionated in terms of best practices. Examples of former include SRV record naming requirements and the latter that ALIAS records are constrained to the root of zones. The strictness and support of providers varies so you may encounter existing records that fail validation when you try to dump them or you may even have use cases for which you need to create or preserve records that don't validate. octoDNS's solution to this is the `lenient` flag. +It's best to think of the `lenient` flag as "I know what I'm doing and accept any problems I run across." The main reason being is that some providers may allow the non-compliant setup and others may not. The behavior of the non-compliant records may even vary from one provider to another. Caveat emptor. + #### octodns-dump If you're trying to import a zone into octoDNS config file using `octodns-dump` which fails due to validation errors you can supply the `--lenient` argument to tell octoDNS that you acknowledge that things aren't lining up with its expectations, but you'd like it to go ahead anyway. This will do its best to populate the zone and dump the results out into an octoDNS zone file and include the non-compliant bits. If you go to use that config file octoDNS will again complain about the validation problems. You can correct them in cases where that makes sense, but if you need to preserve the non-compliant records read on for options.