AnalogJ / lexicon

Manipulate DNS records on various DNS providers in a standardized way.
MIT License
1.48k stars 306 forks source link

Inconsistent content of MX, SRV (etc) data across providers #519

Open doddo opened 4 years ago

doddo commented 4 years ago

Hello!

Whilst browsing through some of the providers, I spot some inconsistencies in how the dns record content is implemented across them.

I am talking specifically about records with the value fields contain more data than just plain value, such as MX and SRV records.

Some of them seem to read the extra data from lexicon config sources, like cloudns:

        if self._get_lexicon_option('ttl'):
            params['ttl'] = self._get_lexicon_option('ttl')
        if self._get_lexicon_option('priority'):
            params['priority'] = self._get_lexicon_option('priority')
        if self._get_provider_option('weight'):
            params['weight'] = self._get_lexicon_option('weight')
        if self._get_provider_option('port'):
            params['port'] = self._get_lexicon_option('port')

while most seem to use lexicon_option only for ttl, like cloudflare, having the extra data in content:

    # Create record. If record already exists with the same content, do nothing'
    def _create_record(self, rtype, name, content):
        data = {'type': rtype, 'name': self._full_name(
            name), 'content': content}
        if self._get_lexicon_option('ttl'):
            data['ttl'] = self._get_lexicon_option('ttl')

The problem is that these providers expect the content to be formatted differently for the _create_record(self, rtype, name, content): method, and therefore it is very hard to handle them transparently.

What I would like to know is which of the two implementations is the correct one. (or at least which one is more correct).

adferrand commented 4 years ago

In theory the first form would be the best, because you can expect bad things happening in general with a given DNS API if you pass all the data for a MX record or a SRV record in a single parameter that is supposed to map to single values records. That said, the first form you mentionned for couldns is still not great, because for instance, weight is not exposed as a proper CLI flag (--weight), and so is only accessible through environment variables or config file. So the API is still not consistent.

I think this goes in the general CLI overhaul we should do, in order to be more consistent towards the variety of DNS records you set, and the unobvious way to update records (you cannot scope an update to record given its content, because --content is used to set the updated content, not searching on the current content of a record).

doddo commented 4 years ago

Maybe but on the other hand each provider needs to handle the content for the record it wants to create and it's pretty easy to split on space and stuff it the way the various DNS api:s likes it. I personally think the second one is more straight forward and might make the cli less awkward. Regardless though, I think that integration doc should specify how to handle such records, as that would make it easier for new providers to handle this in uniform way because as it is now, it is very hard to use these providers programmatically.