New Resource: aws_dax_cluster #2884

gazoakley · 2018-01-06T05:15:20Z

This adds support for managing AWS DynamoDB Accelerator (DAX). It adds a new aws_dax_cluster resource. Addresses #936

gazoakley · 2018-01-06T05:17:36Z

Unit test results:

$ make test TESTARGS='-v -run=TestTagsDax'
==> Checking that code complies with gofmt requirements...
go test -i $(go list ./... |grep -v 'vendor') || exit 1
echo $(go list ./... |grep -v 'vendor') | \
		xargs -t -n4 go test -v -run=TestTagsDax -timeout=30s -parallel=4
go test -v -run=TestTagsDax -timeout=30s -parallel=4 github.com/terraform-providers/terraform-provider-aws github.com/terraform-providers/terraform-provider-aws/aws
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestTagsDaxIgnore
2018/01/06 11:23:01 [DEBUG] Matching ^aws: with aws:cloudformation:logical-id
2018/01/06 11:23:01 [DEBUG] Found AWS specific tag aws:cloudformation:logical-id (val: foo), ignoring.
2018/01/06 11:23:01 [DEBUG] Matching ^aws: with aws:foo:bar
2018/01/06 11:23:01 [DEBUG] Found AWS specific tag aws:foo:bar (val: baz), ignoring.
--- PASS: TestTagsDaxIgnore (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.035s

bflad

@gazoakley this is off to a good start! Thank so much for all the legwork here. I've left some initial comments below so please let me know if you have any questions. Also, if you don't have time to finish up the work here, feel free to let us know as well.

Passes locally for me so far:

make testacc TEST=./aws TESTARGS='-run=TestAccAwsDaxCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsDaxCluster -timeout 120m
=== RUN   TestAccAwsDaxCluster_basic
--- PASS: TestAccAwsDaxCluster_basic (1411.68s)
=== RUN   TestAccAwsDaxCluster_resize
--- PASS: TestAccAwsDaxCluster_resize (1244.04s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2655.771s

bflad · 2018-02-12T20:04:25Z

aws/resource_aws_dax_cluster.go

+				StateFunc: func(val interface{}) string {
+					return strings.ToLower(val.(string))
+				},
+				// DAX follows the same naming convention as ElastiCache clusters


Thank you for the comment here!

bflad · 2018-02-12T20:31:35Z

aws/resource_aws_dax_cluster.go

+		Pending:    pending,
+		Target:     []string{"available"},
+		Refresh:    daxClusterStateRefreshFunc(conn, d.Id(), "available", pending),
+		Timeout:    40 * time.Minute,


Nitpick: I think we'll want to switch these to the schema timeout handlers instead of hardcoding it since they are so long:

# under &schema.Resource Timeouts: &schema.ResourceTimeout{ Create: schema.DefaultTimeout(45 * time.Minute), Delete: schema.DefaultTimeout(45 * time.Minute), Update: schema.DefaultTimeout(90 * time.Minute), }, # example usage d.Timeout(schema.TimeoutCreate) # for adding the standard documentation you can search for ## Timeouts in the markdown files

bflad · 2018-02-12T20:33:57Z

aws/resource_aws_dax_cluster.go

+			ClusterNames: []*string{aws.String(clusterID)},
+		})
+		if err != nil {
+			apierr := err.(awserr.Error)


Nitpick: Can use our helper function and the SDK constant here to simplify this:

if isAwsErr(err, dax.ErrCodeClusterNotFoundFault, "") {

bflad · 2018-02-12T20:35:03Z

aws/resource_aws_dax_cluster.go

+		}
+
+		if c.Status == nil {
+			log.Printf("[DEBUG] DAX Cluster %s has no status attribute set - assume status is deleting", clusterID)


Should we be assuming here? It seems more likely we should be returning an error.

I need to go back and check, but I think when I wrote this the status field wasn't set during deletion. If that's the case I'll drop a comment in.

Yep. Goes through stages of:

"deleting" status: {"Clusters":[{"ActiveNodes":0,"ClusterArn":"arn:aws:dax:us-west-2:799416384551:cache/tf-x2r91tkwdp","ClusterDiscoveryEndpoint":{"Address":"tf-x2r91tkwdp.ydlnd2.clustercfg.dax.usw2.cache.amazonaws.com","Port":8111},"ClusterName":"tf-x2r91tkwdp","IamRoleArn":"arn:aws:iam::799416384551:role/terraform-20180213123722223800000001","NodeType":"dax.r3.large","ParameterGroup":{"NodeIdsToReboot":[],"ParameterApplyStatus":"in-sync","ParameterGroupName":"default.dax1.0"},"PreferredMaintenanceWindow":"fri:09:00-fri:10:00","SecurityGroups":[{"SecurityGroupIdentifier":"sg-3e289a46","Status":"active"}],"Status":"deleting","SubnetGroup":"default","TotalNodes":1}]}

No status: {"Clusters":[{"ClusterArn":"arn:aws:dax:us-west-2:799416384551:cache/tf-x2r91tkwdp","ClusterName":"tf-x2r91tkwdp"}]}

ClusterNotFoundFault: {"__type":"ClusterNotFoundFault","message":"Cluster tf-x2r91tkwdp not found."}

I'll drop in a comment stating we don't get a status towards the end of the deletion process.

Ha nice - every AWS API is special 😅 thanks for all your work here!

bflad · 2018-02-12T20:39:50Z

aws/resource_aws_dax_cluster.go

+		return err
+	}
+
+	if len(res.Clusters) == 1 {


Nickpick: switch this to the opposite condition and return immediately (to save all the later indentation):

if len(res.Clusters) == 0 { log.Printf("[WARN] DAX cluster (%s) not found, removing from state", d.Id()) d.SetId("") return nil }

bflad · 2018-02-12T21:01:30Z

aws/resource_aws_dax_cluster_test.go

+		})
+		if err != nil {
+			// Verify the error is what we want
+			if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "ClusterNotFoundFault" {


isAwsErr(err, dax.ErrCodeClusterNotFoundFault, "") can help here too 😉

bflad · 2018-02-12T21:03:40Z

website/docs/r/dax_cluster.html.markdown

+  Provides an DAX Cluster resource.
+---
+
+# aws\_dax\_cluster


Nitpick: Backslashes are extraneous here 😄

bflad · 2018-02-12T21:04:28Z

website/docs/r/dax_cluster.html.markdown

+```hcl
+resource "aws_dax_cluster" "bar" {
+  cluster_id         = "cluster-example"
+  iam_role_arn       = "arn:aws:iams:us-east-1:012345678999"


Especially since this is an invalid ARN, you can point at a fake data source like ${data.aws_iam_role.example.arn}

bflad · 2018-02-12T21:06:12Z

website/docs/r/dax_cluster.html.markdown

+
+## Attributes Reference
+
+The following attributes are exported:


Please add additional before attributes (we get sometimes have confusion issues with our newer Terraform implementors)

bflad · 2018-02-12T21:09:14Z

website/docs/r/dax_cluster.html.markdown

+DAX Clusters can be imported using the `cluster_id`, e.g.
+
+```
+$ terraform import aws_dax_cluster.my_cluster my_cluster


Please add an acceptance test step to (at least) the _basic test that checks importability:

{ ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },

I've created a separate test in import_aws_dax_cluster_test.go

bflad · 2018-02-13T15:19:39Z

Thanks for your updates, we really appreciate your effort here. Please let me know when this good to check again or if you don't have time for anything. 👍

gazoakley · 2018-02-13T15:20:23Z

@bflad Also added an arn attribute which probably should have been there from the start 😄- can you take another look please?

bflad · 2018-02-13T15:21:13Z

I can get to this later today. Thanks!

gazoakley · 2018-02-13T16:39:19Z

Acceptance tests passing:

$ make testacc TESTARGS='-run=TestAccAWSDAXCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSDAXCluster -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSDAXCluster_importBasic
--- PASS: TestAccAWSDAXCluster_importBasic (707.64s)
=== RUN   TestAccAWSDAXCluster_basic
--- PASS: TestAccAWSDAXCluster_basic (715.58s)
=== RUN   TestAccAWSDAXCluster_resize
--- PASS: TestAccAWSDAXCluster_resize (1349.65s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2772.920s

bflad

@gazoakley this is fantastic -- awesome job and thanks for the hard work! 🚀

 make testacc TEST=./aws TESTARGS='-run=TestAccAWSDAXCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDAXCluster -timeout 120m
=== RUN   TestAccAWSDAXCluster_importBasic
--- PASS: TestAccAWSDAXCluster_importBasic (701.49s)
=== RUN   TestAccAWSDAXCluster_basic
--- PASS: TestAccAWSDAXCluster_basic (1039.20s)
=== RUN   TestAccAWSDAXCluster_resize
--- PASS: TestAccAWSDAXCluster_resize (1300.12s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3040.870s

bflad · 2018-02-27T12:41:05Z

This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-07T17:10:18Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

radeksimko added the new-resource Introduces a new resource. label Jan 7, 2018

bflad added the service/dax Issues and PRs that pertain to the dax service. label Jan 11, 2018

bflad reviewed Feb 12, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2018

gazoakley force-pushed the f-dax-cluster-resource-ii branch from 55b03aa to 927fa14 Compare February 13, 2018 10:12