add Dyn provider

[dwradcliffe]

This PR adds a Dyn provider for managing Dyn DNS records. At the moment this only supports CNAME and A records although I plan on expanding that shortly. I'm also waiting on a few PRs to land in the upstream go-dynect library so that I can clean up this code a bit. I wanted to get the PR submitted to get some initial feedback now. I've been using this locally and all CRUD operations are working.

Ref: nesv/go-dynect#5

Todo:

• add other record types
• add docs
• refactor common Dyn code into dynect library

@cbednarski @mitchellh @catsby @radeksimko for review
cc: @dalehamel @thegedge @wvanbergen

[dalehamel]

solves #459

[catsby]

Thanks for contributing this @dwradcliffe ! I didn't see anything major at first glance, but I'll do a more through review once docs are in. Can you also please contribute tests? Let me know if you need some guidance there.

[dwradcliffe]

@catsby Docs have been added, code refactored, tests added. The acceptance tests pass for me too. I tried to follow the pattern from the other providers but if I'm missing something or did something wrong let me know! 😄

[dwradcliffe]

[daveadams]

I'm very interested in seeing this provider be added to terraform as Dyn support is something I've been sorely missing. I've done some testing myself and it satisfies my needs, works well, and the docs are clear. +1

[daveadams]

@dwradcliffe Looks like multiple "dyn_record" resources in a single terraform config can step on each other and cause API 400 errors. At least I assume that's what's happening. I can sometimes run terraform apply over and over to eventually get it to work. I might could use depends_on to sequence the resources in some cases, but my current config looks like this:

resource "dyn_record" "private_dns" {
  count = "${var.host_count}"

  zone = "example.com"
  name = "redis${count.index+1}"
  value = "${element(aws_instance.redis.*.private_ip, count.index)}"
  type = "A"
}

This should produce A records redis1.example.com through redis${var.host_count}.example.com. If host_count is set high enough, even the queries from terraform plan return HTTP 400 errors. I know of no way to get Terraform to not try to parallellize the resources in this instance but I'm hoping it can happen in the module itself.

Thanks!

[apparentlymart]

@daveadams are you saying that the Dyn API starts to reject requests if too many happen concurrently? If so, a similar problem exists with the AWS API but is abstracted away by the underlying AWS library that Terraform uses, which detects the rate limiting error codes and automatically retries.

Could be reasonable to take a similar approach with this one, as long as Dyn's API responses provide enough information to distinguish a rate limit enforcement from other kinds of error.

[dwradcliffe]

@daveadams I hadn't tested multiple records at the same time :( Due to how the Dyn api works, I can totally see how this might break. Adding Dyn DNS records requires 2 api calls, one to add the record to the current session, and one to publish that session to actually create the record. I'm pretty sure you can't have multiple sessions going at the same time. I'm not familiar with the terraform internals enough to know if there's a better way to handle this. @catsby any ideas?

[daveadams]

I'm not much of a Go hacker, so it's taken me a while, but as @dwradcliffe points out, each change actually requires a pair of requests to the Dyn API: one to make the change and one to publish the new zone. Ideally, you'd queue up the changes and run one Publish step at the very end, and so since Terraform tries to run multiple changes in parallel, the requests overlap something like (for two adds):

Create Record 1
Create Record 2
Publish Zone
Publish Zone

Of course the actual order would be unpredictable, and for larger numbers of changes it gets more complicated. Worst of all is that Terraform thinks record1 failed to create, but in reality both record1 and record2 get created, but terraform only knows about and can only destroy one of them (usually record2 in my testing).

So I experimented with using a very naive Mutex approach and that seems to keep things straight. And even when the Dyn API does return an error (which happens from time to time), the state is registered correctly, so a terraform apply will be able to fix it and a terraform destroy does the right thing.

I'm not sure how best to submit a patch to a pull request, but I'll try to paste it in below. The changes are simple: in resource_dyn_record.go, add a global Mutex variable, lock it at the beginning of each of the Create, Update, and Delete functions, and unlock it just before each possible return path.

diff --git a/builtin/providers/dyn/resource_dyn_record.go b/builtin/providers/dyn/resource_dyn_record.go
index 24c2977..e66c014 100644
--- a/builtin/providers/dyn/resource_dyn_record.go
+++ b/builtin/providers/dyn/resource_dyn_record.go
@@ -3,11 +3,14 @@ package dyn
 import (
    "fmt"
    "log"
+   "sync"

    "github.com/hashicorp/terraform/helper/schema"
    "github.com/nesv/go-dynect/dynect"
 )

+var mutex = &sync.Mutex{}
+
 func resourceDynRecord() *schema.Resource {
    return &schema.Resource{
        Create: resourceDynRecordCreate,
@@ -54,6 +57,7 @@ func resourceDynRecord() *schema.Resource {
 }

 func resourceDynRecordCreate(d *schema.ResourceData, meta interface{}) error {
+   mutex.Lock()
    client := meta.(*dynect.ConvenientClient)

    record := &dynect.Record{
@@ -68,21 +72,25 @@ func resourceDynRecordCreate(d *schema.ResourceData, meta interface{}) error {
    // create the record
    err := client.CreateRecord(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to create Dyn record: %s", err)
    }

    // publish the zone
    err = client.PublishZone(record.Zone)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to publish Dyn zone: %s", err)
    }

    // get the record ID
    err = client.GetRecordID(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("%s", err)
    }
    d.SetId(record.ID)
+   mutex.Unlock()

    return resourceDynRecordRead(d, meta)
 }
@@ -115,6 +123,7 @@ func resourceDynRecordRead(d *schema.ResourceData, meta interface{}) error {
 }

 func resourceDynRecordUpdate(d *schema.ResourceData, meta interface{}) error {
+   mutex.Lock()
    client := meta.(*dynect.ConvenientClient)

    record := &dynect.Record{
@@ -129,26 +138,31 @@ func resourceDynRecordUpdate(d *schema.ResourceData, meta interface{}) error {
    // update the record
    err := client.UpdateRecord(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to update Dyn record: %s", err)
    }

    // publish the zone
    err = client.PublishZone(record.Zone)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to publish Dyn zone: %s", err)
    }

    // get the record ID
    err = client.GetRecordID(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("%s", err)
    }
    d.SetId(record.ID)
+   mutex.Unlock()

    return resourceDynRecordRead(d, meta)
 }

 func resourceDynRecordDelete(d *schema.ResourceData, meta interface{}) error {
+   mutex.Lock()
    client := meta.(*dynect.ConvenientClient)

    record := &dynect.Record{
@@ -164,14 +178,17 @@ func resourceDynRecordDelete(d *schema.ResourceData, meta interface{}) error {
    // delete the record
    err := client.DeleteRecord(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to delete Dyn record: %s", err)
    }

    // publish the zone
    err = client.PublishZone(record.Zone)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to publish Dyn zone: %s", err)
    }
+   mutex.Unlock()

    return nil
 }

[dwradcliffe]

@daveadams Nice work! I'll try to pull this in and test it on my end too.

[thegedge]

One suggestion: remove all of the mutex.Unlock() calls and do a single defer mutex.Unlock() immediately after locking. defer is your resource-freeing friend in Go :)

[daveadams]

Great tip, @thegedge! Glad to say I learned something new and awesome about Go today thanks to you.

I did some more testing with bigger numbers of records, and my patch still falls short. Looks like the Read step will need to use the mutex lock as well. This also (best I can tell) prevents using defer mutex.Unlock() on Create or Update, since they call Read as part of their final successful return statement, and that creates a deadlock right away. And Mutex doesn't allow for testing its state, and it panics if you Unlock() an unlocked mutex.

So here's the latest patch which I've tested with apply/destroy on 30 records:

diff --git a/builtin/providers/dyn/resource_dyn_record.go b/builtin/providers/dyn/resource_dyn_record.go
index 24c2977..2a4d7dd 100644
--- a/builtin/providers/dyn/resource_dyn_record.go
+++ b/builtin/providers/dyn/resource_dyn_record.go
@@ -3,11 +3,14 @@ package dyn
 import (
    "fmt"
    "log"
+   "sync"

    "github.com/hashicorp/terraform/helper/schema"
    "github.com/nesv/go-dynect/dynect"
 )

+var mutex = &sync.Mutex{}
+
 func resourceDynRecord() *schema.Resource {
    return &schema.Resource{
        Create: resourceDynRecordCreate,
@@ -54,6 +57,9 @@ func resourceDynRecord() *schema.Resource {
 }

 func resourceDynRecordCreate(d *schema.ResourceData, meta interface{}) error {
+   mutex.Lock()
+   // can't defer mutex.Unlock() here, alas
+
    client := meta.(*dynect.ConvenientClient)

    record := &dynect.Record{
@@ -68,26 +74,33 @@ func resourceDynRecordCreate(d *schema.ResourceData, meta interface{}) error {
    // create the record
    err := client.CreateRecord(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to create Dyn record: %s", err)
    }

    // publish the zone
    err = client.PublishZone(record.Zone)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to publish Dyn zone: %s", err)
    }

    // get the record ID
    err = client.GetRecordID(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("%s", err)
    }
    d.SetId(record.ID)

+   mutex.Unlock()
    return resourceDynRecordRead(d, meta)
 }

 func resourceDynRecordRead(d *schema.ResourceData, meta interface{}) error {
+   mutex.Lock()
+   defer mutex.Unlock()
+
    client := meta.(*dynect.ConvenientClient)

    record := &dynect.Record{
@@ -115,6 +128,9 @@ func resourceDynRecordRead(d *schema.ResourceData, meta interface{}) error {
 }

 func resourceDynRecordUpdate(d *schema.ResourceData, meta interface{}) error {
+   mutex.Lock()
+   // can't defer mutex.Unlock() here, alas
+
    client := meta.(*dynect.ConvenientClient)

    record := &dynect.Record{
@@ -129,26 +145,33 @@ func resourceDynRecordUpdate(d *schema.ResourceData, meta interface{}) error {
    // update the record
    err := client.UpdateRecord(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to update Dyn record: %s", err)
    }

    // publish the zone
    err = client.PublishZone(record.Zone)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("Failed to publish Dyn zone: %s", err)
    }

    // get the record ID
    err = client.GetRecordID(record)
    if err != nil {
+       mutex.Unlock()
        return fmt.Errorf("%s", err)
    }
    d.SetId(record.ID)

+   mutex.Unlock()
    return resourceDynRecordRead(d, meta)
 }

 func resourceDynRecordDelete(d *schema.ResourceData, meta interface{}) error {
+   mutex.Lock()
+   defer mutex.Unlock()
+
    client := meta.(*dynect.ConvenientClient)

    record := &dynect.Record{

[daveadams]

What do we have to do to get this pulled into the next release? We're anxious to have this built-in.

[daveadams]

Any hope to get this merged? @phinze ?

[phinze]

@daveadams sorry for the delay on this! We'll make sure this gets looked at soon.

[phinze]

Looking good - landing this. Thanks for all your work here @dwradcliffe and @daveadams! 🙇

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.