apache / apache/libcloud

Inconsistency in specifying TTL in DNS records between example and tests

Open
#1,536 4 comments 0 reactions 0 assignees View on GitHub
stale
Dominant language
Python
Stars
2.1k
Forks
931
Avg merge
1d 2h
Merged PRs (30d)
4

Description

## 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](https://libcloud.readthedocs.io/en/stable/dns/examples.html#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](https://libcloud.readthedocs.io/en/stable/dns/examples.html#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](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/dns/base.py#L163) 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](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/test/dns/test_base.py#L38), 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](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/dns/base.py#L549) 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()`](https://github.com/apache/libcloud/blob/6b588346083a510396c63b5d18db3012aa083071/libcloud/dns/base.py#L83) 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.

Contributor guide

Open the contributing guide

Research direction

Read libcloud/dns/base.py and libcloud/test/dns/test_base.py first, then compare the DNS documentation example with the Record constructor, Zone.create_record(), and DNSDriver._get_bind_record_line(). Done means the project has an agreed single TTL representation, consistent documentation and tests, and an explicit backward-compatibility outcome.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, networking
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.