NEW PROVIDER: AutoDNS #1336
Merged
NEW PROVIDER: AutoDNS #1336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tlimoncelli
reviewed
Dec 14, 2021
providers/autodns/api.go
Outdated
| }) | ||
|
|
||
| responseData, err := api.request("POST", "zone/_search", request) | ||
|
|
There was a problem hiding this comment.
style: remove blank between verb and error checking.
| return nil, err | ||
| } | ||
|
|
||
| var responseData, _ = api.request("GET", "zone/" + domain + "/" + systemNameServer.Name, nil) |
There was a problem hiding this comment.
Please check the error or include a comment explaining why it can be ignored.
tlimoncelli
reviewed
Dec 14, 2021
|
Looking great so far! When you're ready for a full review, please post the output of the integration test as a comment. |
|
Erhm... |
|
Woops, missed a spot... |
|
The |
tlimoncelli
reviewed
Dec 16, 2021
providers/autodns/api.go
Outdated
| return responseObject.Data[0], nil | ||
| } | ||
|
|
||
| func (api *autoDnsProvider) updateZone(domain string, resourceRecords []*ResourceRecord, nameServers []*models.Nameserver, zoneTTL uint32) { |
There was a problem hiding this comment.
Add an error return value to this function signature. Then it doesn't need to panic, it can return the error.
That should be the complete suite @tlimoncelli, I've removed the It seems to fail on dual provider support, is it possible to annotate missing support for this using capabilities like other records? |
Congrats! You've passed the biggest challenge!
I thought there was but I can't find anything other that I've started #1345 which will skip that test if |
|
Hi @tlimoncelli, It still seems to fail one specific check, can you point me to where that would be? |
|
Look at Line 758 of |
|
I didn't even know about this 😲 besides learning Go this also lets me find out stuff about DNS I didn't know about :) Surprisingly AutoDNS seems to support RFC 7505 through the web-interface so I'll try to figure out which edge case in my code is causing this to fail. |
|
Sounds good! (the amount one can learn about DNS is... infinite :) ) RFC 7505 isn't used by anyone as far as I can tell. Feel free to just add the provider's name to the opt-out list and fix this later. Best, |
|
I think I found the issue, in https://github.com/arnoschoon/dnscontrol/blob/master/providers/autodns/types.go#L16 the property is omitted if empty (which 0 seems to be), in that case AutoDNS defaults to a value of My laptop is currently churning through 1200+ DNS zones so I'll take a crack at fixing it tomorrow. I'd like to fix it rather than skipping a test even if almost no one uses it :) hopefully that helps with other glitches that'll be discovered in the future. |
|
Sounds like a good plan! Tom |
|
Friendly ping? |
|
Hi @tlimoncelli , sorry for the delay, I got caught up in other work and almost forgot about this. Sadly I had to skip that specific test, at least for now. Passing even an empty |
tlimoncelli
changed the title
tlimoncelli
reviewed
Jan 18, 2022
tlimoncelli
approved these changes
Jan 18, 2022
|
I'm reviewing https://stackexchange.github.io/dnscontrol/writing-providers it looks like we skipped most of Step 8. Please update README.md, docs/provider-list.md, and docs/_providers/PROVIDERNAME.md Thanks! |
|
golint reports: |
tlimoncelli
added a commit
that referenced
this pull request
Mar 26, 2022
* Implement AutoDNS provider to manage existing zones * Moved AuditRecords() in to separate file to ease automatic updating * S1011 - Use a single append to concatenate two slices * Set list of available record types as returned by the system * Fixed style, clarify code and add some extra comments * Documented simple configuration and usage example of AutoDNS * Convert MX and SRV record properly from string to actual structs and back * Add support for integration tests of AutoDNS * Return error message from update request instead of invoking panic() * Skip AUTODNS in test for RFC 7505 (null MX) * Update providers/autodns/autoDnsProvider.go Co-authored-by: Tom Limoncelli <tlimoncelli@stackoverflow.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This initial implementation allows you to manage a zone hosted by AutoDNS. A full zone update is performed in case of a modification instead of sending individual change requests.
The desired TTL of the zones' (apex) nameservers is stored as TTL for the SOA record, AutoDNS doesn't let you specify a TTL for these records.
Currently a rather naive approach is used to set the nameservers (which AutoDNS expects in a certain order), for that reason this provider can't be used in conjunction with others.