You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@libcloud.apache.org by GitBox <gi...@apache.org> on 2020/12/29 22:38:52 UTC
[GitHub] [libcloud] macfreek opened a new issue #1536: Inconsistency in specifying TTL in DNS records between example and tests
macfreek opened a new issue #1536:
URL: https://github.com/apache/libcloud/issues/1536
## Summary
There is an inconsistency how custom TTL are stored and accessed individual DNS Records.
Either it is stored or accessed in the `Record.ttl` attribute, or in the `Record.extra` dict. 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.
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.
## 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 with this way forward, removing the `Record.ttl` attribute? If you are concerned about backward compatibility, I'm happy to write a PR for a setter/getter that uses the Records.extra["ttl"] behind the scenes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] stale[bot] commented on issue #1536: Inconsistency in specifying TTL in DNS records between example and tests
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on issue #1536:
URL: https://github.com/apache/libcloud/issues/1536#issuecomment-868936501
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@libcloud.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] macfreek commented on issue #1536: Inconsistency in specifying TTL in DNS records between example and tests
Posted by GitBox <gi...@apache.org>.
macfreek commented on issue #1536:
URL: https://github.com/apache/libcloud/issues/1536#issuecomment-752648458
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.`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on issue #1536: Inconsistency in specifying TTL in DNS records between example and tests
Posted by GitBox <gi...@apache.org>.
Kami commented on issue #1536:
URL: https://github.com/apache/libcloud/issues/1536#issuecomment-752672124
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami edited a comment on issue #1536: Inconsistency in specifying TTL in DNS records between example and tests
Posted by GitBox <gi...@apache.org>.
Kami edited a comment on issue #1536:
URL: https://github.com/apache/libcloud/issues/1536#issuecomment-752412165
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [libcloud] Kami commented on issue #1536: Inconsistency in specifying TTL in DNS records between example and tests
Posted by GitBox <gi...@apache.org>.
Kami commented on issue #1536:
URL: https://github.com/apache/libcloud/issues/1536#issuecomment-752412165
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 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org