Add Gandi LiveDNS driver #1323
Add Gandi LiveDNS driver #1323
Conversation
…andi-livedns-driver
…nable to locate a way to set request_path instead, update minimum TTL to 300, not 30
…e is a full URL, not just a partial one, so break it up and take the last path segment; re-associating a domain is not as originally implemented, works differently for domains vs zones; documentation is wrong about updating a zone, you can only change the name, not the UUID as advertised, and only from zones, not domains, so a zone_uuid is required, which has been added to the _to_zone method when available; update tests to match urls and revised TTL
…res; add exception handling, and convert to well known exceptions for cases of not found or already existing
Codecov Report
@@ Coverage Diff @@
## trunk #1323 +/- ##
==========================================
+ Coverage 86.26% 86.32% +0.05%
==========================================
Files 367 371 +4
Lines 75262 75743 +481
Branches 6882 6922 +40
==========================================
+ Hits 64927 65385 +458
- Misses 7559 7574 +15
- Partials 2776 2784 +8
Continue to review full report at Codecov.
|
libcloud/common/gandi_live.py
Outdated
| else: | ||
| return body | ||
| elif self.status == httplib.NOT_FOUND: | ||
| if (not json_error) and ('cause' in body): |
There was a problem hiding this comment.
Parenthesis are redundant here, but also not a blocker.
libcloud/common/gandi_live.py
Outdated
| message = body | ||
| raise ResourceConflictError(message, self.status) | ||
| else: | ||
| if (not json_error) and ('cause' in body): |
There was a problem hiding this comment.
This logic seems to be repeated 4 times, perhaps it could be refactored inside a separate method (maybe even the existing _get_error method)?
There was a problem hiding this comment.
I originally grabbed most of this from the Google common lib. Fixed this and the previous one - does it matter who clicks the 'resolve conversation' button (if it's used)?
libcloud/dns/drivers/gandi_live.py
Outdated
| raw_zone_data = { | ||
| 'name': zone_name, | ||
| } | ||
| post_zone_data = json.dumps(raw_zone_data) |
There was a problem hiding this comment.
This encoding should happens inside the connection class - please add encode_data to the connection class and remove data encoding inside the driver methods.
For details and context, please see #1315 (comment)
There was a problem hiding this comment.
Ah, didn't realize that was a thing. Fixed.
| self.assertEqual(zone.id, 'example.org') | ||
| self.assertEqual(zone.domain, 'example.org') | ||
|
|
||
| def test_get_zone(self): |
There was a problem hiding this comment.
Nice work on the tests 👍
It would also be good to add some tests for failure scenarios / edge cases (aka record doesn't exist, zone doesn't exist, record validation failure, etc.).
There was a problem hiding this comment.
More tests added that increase coverage.
libcloud/test/dns/test_gandi_live.py
Outdated
| self.assertEqual(record.id, 'A:@') | ||
| self.assertEqual(record.name, '@') | ||
| self.assertEqual(record.type, RecordType.A) | ||
| self.assertEqual(record.data, ['127.0.0.1']) |
There was a problem hiding this comment.
Hm, can single record contain multiple values?
I believe we handle this inside other drivers by creating multiple Record class instances (all of them share the same name, but have a different value which is a string), but I'm not 100% and I would need to double check.
|
Thanks for the contribution. Overall it looks like a great start. There are just some minor things which need to be sorted out. As far as I still think it would be better to return multiple Here is an example of how a similar scenario is handled in the AWS Route 53 driver - https://github.com/apache/libcloud/blob/trunk/libcloud/dns/drivers/route53.py#L447. I know that means more work in the driver, but that's kinda the whole idea behind the abstraction - we should try to expose the same interface everywhere where possible. |
…python structure for driver encoding
Sure. Yeah, that complicates it since the API itself is returning a JSON array there. It might be a bit until I get to adding error tests and putting this more in line with the base Libcloud API. |
… response body on purpose, so continue and use response codes
… drop unusable path, add note to self for future record rework
… values to better match live responses
…her values into Record.extra['other_values']; add ex_ methods to deal with Gandi zones
|
I've made a change to mimic Route53 in part, but this is another conceptual mapping problem - a record in Gandi encompasses all values, whereas in Route53 each individual value constitutes a record, and multiple values are a convenience. So it's possible there to delete one value (record), whereas with Gandi you can only delete all values. If you want to change the number of values associated with a name-type, you basically have to delete and recreate the record instead of updating it (whereas if you could pass in the new set of values directly, you could just call update). This |
|
Thanks for making those changes - I will have a look. And yes, edge cases like that can be challenging. In theory (from end user friendliness and standard libcloud API perspective) in such scenarios, Libcloud driver should handle this transparently - aka if user updates such record, existing one is deleted and new one is created. This is obviously more complex thought and also introduces more (partial) failure scenarios since single method call now results in multiple API calls. |
|
Merged, thanks. |
Add Gandi LiveDNS driver
Gandi released an API for their DNS service completely different from their previous XMLRPC implementation. This contribution adds a driver to cover the new service, per LIBCLOUD-949.
Description
This is a new driver and does not touch on any of the previous Gandi XMLRPC driver code. The implementation is similar to other providers that use HTTP and JSON.
Gandi's documentation is the overall basis for the implementation though there are some minor discrepancies in it that don't match up to usage experience. They also left out any documentation of errors.
One point of possible confusion is that zones and domains are separate classes in this service, which enables reuse of one zone file for multiple domains but becomes somewhat difficult to shoehorn into Libcloud's classes, especially since what's referred to as a Libcloud zone corresponds to a Gandi domain, and nothing really corresponds to a Gandi zone. There is precedence since this is also true of the existing Gandi XMLRPC implementation, which appears to me to have chosen to go with ignoring Gandi zones as much as possible in favor of domains. The new Gandi API does allow for this (actions taken on a domain will apply to the zone and all related domains behind the scenes), but it means things like
delete_zoneandupdate_zonehave no real meaning - one cannot delete a Gandi domain from the API, and it is basically immutable if ignoring Gandi zones - so I didn't implement those methods.Status
Done, ready for review
Checklist (tick everything that applies)