Add support for DynECT DNS provider #271
Conversation
|
Hey @jsgaard. Thanks for the PR. I haven't had time to look at it just yet, but wanted to let you know I haven't forgotten about it. |
|
Ok, this is going to be a tricky one because it doesn't look like I can sign up for an account anymore since the service is being sunset in favor of Oracle's cloud services. So I can't really run my own tests unless you're willing to temporarily share account credentials. Aside from actual testing, I do have a few suggestions though. First, I'd probably rename this from DynECT to something like DynManaged or DynManagedDNS. I realize the portal uses dynect.com as a domain name, but they call the service Dyn Managed DNS. I'll try and add the rest of my suggestions inline in the code. Bear with me. |
| @@ -0,0 +1,22 @@ | |||
| # How To Use the DynECT DNS Plugin | |||
|
|
|||
| This plugin works against DynECT DNS provider. | |||
There was a problem hiding this comment.
Make the provider name a link to their site somewhere generic. Normally it would be the service's homepage but since that doesn't really exist anymore, perhaps the Getting Started Guide for Managed DNS? https://help.dyn.com/managed-dns-gsg/
|
|
||
| } | ||
|
|
||
| Function Add-DynModule { |
There was a problem hiding this comment.
Normally, I'd try to avoid 3rd party module requirements for something with a standard REST API. But since the service is going away soon, I'm ok with it in this instance. However, the plugin shouldn't be responsible for installing the module if it doesn't exist. Just throw an error if it's not found.
| [Parameter(Mandatory,Position=4)] | ||
| [securestring]$pass, | ||
| [Parameter(Mandatory,Position=5)] | ||
| [string]$customer |
There was a problem hiding this comment.
The credential parameter names are too generic and could potentially conflict with other plugins. Add a provider specific prefix like Dyn, DynManaged, or DM. So they'd become $DynUser, $DynPass, and $DynCustomer for example.
You're also missing the required $ExtraParams parameter from the example plugin. It and the $RecordName and $TxtValue need to exist in all plugins to ensure everything works together.
|
|
||
| Add-DynModule | ||
|
|
||
| If ($user -and $customer -and $pass) { |
There was a problem hiding this comment.
Because all of your parameters are set to Mandatory, you don't need to check that they exist here. PowerShell will enforce this for you.
| Return | ||
| } | ||
|
|
||
| If ($zone -and $RecordName -and $TxtValue) { |
There was a problem hiding this comment.
Because all of your parameters are set to Mandatory, you don't need to check that they exist here. PowerShell will enforce this for you.
| If (Test-DynDnsSession) { | ||
| Write-Verbose "DynECT session is alive" | ||
|
|
||
| If ($zone -and $RecordName) { |
There was a problem hiding this comment.
Because all of your parameters are set to Mandatory, you don't need to check that they exist here. PowerShell will enforce this for you.
| [Parameter(Mandatory,Position=1)] | ||
| [string]$TxtValue, | ||
| [Parameter(Mandatory,Position=2)] | ||
| [string]$zone, |
There was a problem hiding this comment.
Requiring a zone parameter doesn't work for Posh-ACME DNS plugins because plugin parameters are saved account-wide. So if you ever request a cert in a different zone than your original one, it will overwrite the saved value for the original one and you'll break your renewals. Plugins generally derive the zone name from the $RecordName value.
| Write-Verbose "Successfully generated auth token to DynECT" | ||
| } Else { | ||
| Write-Warning "Token could not be generated, connection to DynECT has failed" | ||
| Return |
There was a problem hiding this comment.
Informing the user about what went wrong here is good. But a simple return without actually throwing an error is going to make the module think the TXT record was created successfully. I'd change this to something like:
throw "Dyn connection failed. Token could not be generated."
| If ($zone -and $RecordName -and $TxtValue) { | ||
| Write-Verbose "All arguments for updating DNS has been set" | ||
| Write-Verbose "Trying to add DNS record to DynECT" | ||
| Add-DynDnsRecord -Zone $zone -Node $RecordName -DynDnsRecord (New-DynDnsRecord -Text $TxtValue) -Confirm:$false |
There was a problem hiding this comment.
What happens if the TXT record already exists when you call this? Does the Get-DynDnsZoneChanges call below just return nothing? Just want to make sure.
|
|
||
| If ($zone -and $RecordName) { | ||
| Write-Verbose "Trying to remove DNS record" | ||
| $txtToRemove = Get-DynDnsRecord -Zone $zone -RecordType TXT -Node $RecordName |
There was a problem hiding this comment.
How does Dyn deal with multi-valued TXT records? It's a fairly common case when creating wildcard certs that you need to create 2 TXT records with the same name and different values. Most plugins need to distinguish what to delete based on the $TxtValue rather than just $RecordName. We don't want to delete anything except explicitly what we were asked to delete.
There was a problem hiding this comment.
Thanks for the taking the time to look through this.
I'll make a updated version (Can't give an estimate, got a few other things first) and hopefully answer a few of the questions you have.
Unfortunate I'm unable to share credentials, since this is a company account.
|
Closing this in preparation for the 4.0 release and branch naming changes. Please re-submit if necessary after the 4.0 release. |
No description provided.