search

dnsimple / erldns

DNS server, in Erlang.
MIT License
398 stars 98 forks source link

fix json key ordering #111

Closed dch closed 3 years ago

dch commented 3 years ago

erldns usage of JSON relies on implicit ordering of keys which is the JSON equivalent of C's undefined behaviour.

The first commit dealing with zonefiles is a reasonable approach, but the 2nd commit for DNSSEC keys could be improved - suggestions welcomed!

For that commit, I tried to preserve the existing behaviour that parses all fields in a single pass, but this introduces a risk of an infinite loop around the final clause if any keys are missing/extra (and therefore the previous pattern match fails).

dch commented 3 years ago

rebased off v2.0.0

aeden commented 3 years ago

Thank you @dch.

aeden commented 3 years ago

There's an issue that has arisen from this change in our internal zone loading system. We are investigating now, but if anyone else sees issues loading zones, please 👍 this comment.

dch commented 3 years ago

@aeden I expect this is around

For that commit, I tried to preserve the existing behaviour that parses all fields in a single pass, but this introduces a risk of an infinite loop around the final clause if any keys are missing/extra (and therefore the previous pattern match fails).

Unless you are explicitly pattern-matching there (e.g. for performance reasons during zone loading), we can probably do something better than the current code. Do you have some opinions on what you'd like to do instead?

aeden commented 3 years ago

In our case the issue was around the mechanism we use for zone management, which is not part of the erldns core.

I've updated the erldns zone parser so it handles JSX 3.x.x maps in addition to the JSX 2.x style proplists in https://github.com/dnsimple/erldns/pull/126 - once that's merged it should no longer be an issue. I am still testing in our canary environment though to iron out some final issues.