refine DNS-checking and add automated tests #372
Conversation
- don't try to guess IP addresses but insist on A and AAAA records - try to allow ipv4 or ipv6 only zones - move chatmail.zone generation to jinja so we can have conditionals
…at all records are present
| def check_zonefile(zonefile): | ||
| diff = [] | ||
| """Check expected zone file entries.""" | ||
| required = True | ||
| required_diff = [] | ||
| recommended_diff = [] | ||
|
|
||
| for zf_line in zonefile.splitlines(): | ||
| if "; Recommended" in zf_line: | ||
| required = False | ||
| continue | ||
| if not zf_line.strip() or zf_line.startswith(";"): | ||
| continue | ||
| print(f"dns-checking {zf_line!r}") | ||
| zf_domain, zf_typ, zf_value = zf_line.split(maxsplit=2) | ||
| zf_domain = zf_domain.rstrip(".") | ||
| zf_value = zf_value.strip() | ||
| query_values = query_dns(zf_typ, zf_domain) | ||
| if zf_value in query_values: | ||
| continue | ||
| query_value = query_dns(zf_typ, zf_domain) | ||
| if zf_value != query_value: | ||
| assert zf_typ in ("A", "AAAA", "CNAME", "CAA", "SRV", "MX", "TXT"), zf_line | ||
| if required: | ||
| required_diff.append(zf_line) | ||
| else: | ||
| recommended_diff.append(zf_line) | ||
|
|
||
| return required_diff, recommended_diff |
There was a problem hiding this comment.
Wow, this is beautiful. I still think with horror of my previous spaghetti code
| def test_check_zonefile_all_ok(self, cm_data, mockdns_base): | ||
| zonefile = cm_data.get("zftest.zone") | ||
| parse_zonefile_into_dict(zonefile, mockdns_base) | ||
| required_diff, recommended_diff = remote_funcs.check_zonefile(zonefile) | ||
| assert not required_diff and not recommended_diff | ||
|
|
There was a problem hiding this comment.
With this PR, setting recommended DNS records on with a zonefile is sliding out of the part which is tested by CI, as cmdeploy dns doesn't fail for them anymore. This is probably not a huge problem, but we should notice it consciously.
In the past, the CI set all DNS records on ns.testrun.org, and then ran cmdeploy dns to verify them, and return had to be zero (with three tries). Now I wonder if we would notice if one of the recommended DNS records isn't set as expected anymore, as CI could still be green. Note that there can be complications while setting DNS records, e.g. formatting of DKIM records. OTOH, most of them happen in web interfaces, which are not tested anyway.
There was a problem hiding this comment.
good point. Let's maybe do this is a separate PR -- an option "--all" that gives return code 1 if any dns record does not match.
supersedes #355
There are now phases of DNS checks:
initial DNS entries that must be present for "cmdeploy run"
required DNS entries for operating within the chatmail server network
recommended DNS settings for interoperability and security hardening
"cmdeploy dns" will return 0 as exitcode (all ok) if all required DNS settings are correct but will print warnings for any missing recommended one
We might want to shift the SPF record from recommended into required.