Inconsistency in specifying TTL in DNS records between example and tests

[macfreek]

Summary

Right now, the TTL for individual DNS Records is stored either in the Record.ttl attribute, or in the Record.extra dict, or in both.

Some of the code stores or access the Record.ttl value, other code the Record.extra['ttl'] value. I'll give two examples of either.

This inconsistency causes bugs. E.g. if you follow the documentation for Create a record with a custom TTL, and subsequently call libcloud.dns.base.DNSDriver.export_zone_to_bind_format(), it will not output the specified TTL, but the default TTL of the zone.

Detailed Information

First of all, note that this only concerns a custom TTL in a libcloud.dns.base.Record. This is not commonly set. Usually, records used the default TTL as specified in the associated libcloud.dns.base.Zone.

TTL stored in the Record.ttl attribute

In the documentation, at Create a record with a custom TTL, a custom TTL is given as parameter to Record constructor:

ttl = 900
record = zone.create_record(name='www', type=RecordType.A, data='127.0.0.1',
                            ttl=ttl)

In the Record.__init__() method, on line 163 this ttl is stored in the Record.ttl attribute:

self.ttl = ttl

TTL stored in Record.extra["ttl"]

On the other hand, in the libcloud/test/dns/test_base.py unit test, on line 38, the TTL is not set using the ttl parameter, but using the extra parameter.

values = {'id': 3, 'name': 'www', 'type': RecordType.A, 'data': '127.0.0.1',
 'extra': {'ttl': 123}},
# ...
record = Record(**values)

Futhermore, in the DNSDriver. _get_bind_record_line() method, on line 549 the ttl is retrieved using the extra attribute, ignoring the ttl attribute of the Record (it does look at the Zone.ttl though). Finally, this last behaviour is checked in the unit test.

    ttl = record.extra['ttl'] if 'ttl' in record.extra else record.zone.ttl

How to proceed?

My first attempt to fix this issue was in pull request #1535. In there, I changed the DNSDriver. _get_bind_record_line() to use the Record.ttl instead of the Record.extra["ttl"]

I now think that fixing it the other way around it better: deprecating Record.ttl in favour of the Record.extra["ttl"].

I have two main arguments:

1. The sample code is incorrect. It calls zone.create_record() with ttl as a parameter. But Zone.create_record() does not have a ttl parameter! Only the Record.__init__() method does. So this sample code will fail with a TypeError exception.

2. Most code seem to use the Records.extra["ttl"], not the Record.ttl. However, I have not done an extensive check yet. (neither of the libcloud code, nor of code out in the wild that uses libcloud).

Reaching consensus

• Do you concur that it is better to store the TTL at only one place, not two?
• Do you concur that it is best to store it in Records.extra["ttl"], while creating wrapper functions around Record.ttl for backward compatibility?

If so, please have a look at PR #1537.

[Kami]

Thanks for reporting this.

I believe extra["ttl"] was there first, but since specifying "random" attributes in extra dictionary is really a bad and unfriendly API, we decided to "promote" that attribute directly as a top-level Record.ttl object attribute a while back.

The idea was to eventually remove extra["ttl"] support, but we haven't done that yet since it is a breaking changes.

So in short - yes, I totally agree with you, there should only be one way and that should be Record.ttl attribute. Since removing support for extra["ttl"] is a breaking change, it involves those changes:

1. Need to update all DNS drivers and code examples to ensure they correctly utilize record.ttl attribute
2. Explicitly call out this breaking change in CHANGES.rst and docs/upgrade_notes.rst file
3. Release that change with a major release since it's a breaking change
4. Ideally also update extra["ttl"] to emit a deprecation warning for at least one release so users are wanted and have time to migrate

[macfreek]

Hi @Kami,

Thanks for your feedback, and patience -- I'm aware that opening both an issue and a PR is a bit more work for you, and you probably have noted that I have a habit to changing the issue or PR while I dive deeper into the issue. I'll try to add a "WIP" (work in progress) tag if I'm not yet happy with the report or PR. You may want to delay your comments till I remove it, to save yourself some time. (And yes, I may sometimes change it even without that tag, once I discover more quirks.)

I'll have a look if I can make a PR with the suggested change. It might require a significant number of changes in the drivers though.

Also, I'll have to think how and when. How to ensure a (mostly) backwards compatible API, and when to make sure this ends up a minor or major release (not a bug fix release).

A second question is if you would want to consider other (perhaps breaking) changes or clarifications.

A few things that come to mind are:

• Making the priority an attribute as well. (I'm in doubt. It's fairly common, in use for MX and SRV records, but next thing you know, you'll be adding port, weight, service, and protocol too).
• Align the different extra field:
  • In particular zonomi adds a prio field, while all other drivers use priority as the field name. I prefer consistency here.
  • Align the use of description (Google), comment (Rackspace) and notes (Zerigo).
• Clearly specify the use of trailing dots in both name and data that contain a FQDN. Right now, the name does seems to require a trailing dot (e.g. www.example.com.) A data field that requires a FQDN (e.g. MX, NS, PTR or CNAME) do require a trailing dot in DNS itself, but it seems that most API supported here insist that there MUST NOT be a trailing dot.
• Be clear if/how IDN (international domain names) are supported. I kind of suspect that both name and data fields are expected to be punycode-encoded (e.g. xn--bcher-kva.de. instead of bücher.de.).

[Kami]

You are welcome and thank you for your contributions.

I'm +1 for improving it for other record types as well - handling those things via extra dictionary is definitely not a great API.

As far as implementation wise, I would need to think a bit more about it. For one, I don't think it should live directly on the base Record class since those attributes are specific to only some record types.

One option would be to introduce sub-classes for other common record types which take additional arguments / attributes - so things such as RecordMX, RecordPTR, etc. Base Record should only contain attribute which are common to all or majority of the record types and then those sub classes can contain record specific attributes.

This way we could also relatively easily keep it backward compatible for a while - aka first try to read value from passed in record class instance if it's of a sub-type, otherwise fall back to extra dict. It would still require quite some work to update all the drivers though. We can also do it incrementally.

[stale[bot]]

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue.