providers/aws: Add ElastiCache replication group support (fixes #1799)

[saulshanabrook]

This is based off of @inferiorhumanorgans work in #1799. I fixed some import paths and added a primary_endpoint attribute, so it can be used like this:

resource "aws_elasticache_replication_group" "default" {
    replication_group_id = "redis.fun"

    description = "redis fun"
    engine = "redis"
    cache_node_type = "${var.node_type}"
    num_cache_clusters = 2
    automatic_failover = true
    subnet_group_name = "${var.subnet_group_name}"
    security_group_ids  = ["${var.security_group_id}"]
}

resource "aws_route53_record" "default" {
    zone_id = "${var.zone_id}"
    name = "${var.engine}-${var.name}.${var.zone_name}"
    type = "CNAME"
    ttl = "30"
    records = [
        "${aws_elasticache_replication_group.default.primary_endpoint}."
    ]
}

[saulshanabrook]

Looks like the tests failed:

builtin/providers/aws/resource_aws_elasticache_replication_group.go:109: undefined: aws.Boolean
builtin/providers/aws/resource_aws_elasticache_replication_group.go:110: undefined: aws.Long
builtin/providers/aws/resource_aws_elasticache_replication_group.go:182: undefined: aws.Boolean
builtin/providers/aws/resource_aws_elasticache_replication_group.go:188: undefined: aws.Boolean

Weird. It builds and passes tests on my local machine.

[dlsniper]

It might be that you use a different version of the aws sdk than the project, check it out: https://github.com/hashicorp/terraform/blob/master/deps/v0-6-1.json#L42 . Hope this helps.

[saulshanabrook]

@dlsniper Those deps look like they are for the 0.6.1 release. Do you know how I update the deps for the next release?

[dlsniper]

Sorry, that's something I can't know, maybe someone else does? I've noticed some problems when running against the latest AWS sdk as well so I'm in the dark as well.

[joshgarnett]

@saulshanabrook Looking at http://docs.aws.amazon.com/sdk-for-go/api/aws.html I don't see aws.Boolean or aws.Long. It looks like there was a breaking change introduced in the SDK. Take a look at 579ccbe for an example where this was fixed in the other resources.

[saulshanabrook]

@josephholsten That makes sense. OK I am trying to do make updatedeps but it keeps failing:

    imports google.golang.org/api/compute/v0.beta: cannot find package "google.golang.org/api/compute/v0.beta" in any of:
    /usr/local/Cellar/go/1.4.2/libexec/src/google.golang.org/api/compute/v0.beta (from $GOROOT)
    /Users/saul/projects/go/src/google.golang.org/api/compute/v0.beta (from $GOPATH)

[dlsniper]

Seems there's that it has been removed in this commit: googleapis/google-api-go-client@18450f4 (warning, it's a bit big). This looks like the new version: https://godoc.org/google.golang.org/api/compute/v1

[saulshanabrook]

OK I am trying to rebase off of the master branch and then run make updatedeps again to see if that solves it.

Yep it does

[saulshanabrook]

OK updated with new names.

[josephholsten]

@saulshanabrook pretty sure you meant @joshgarnett, but I could definitely use this stuff. Thanks!

[joshgarnett]

@catsby any chance this can get merged soon?

[adubkov]

It works in general. But AZ distribution works weird. It put 2 boxes in same AZ, however when I create cluster witn AWS Console, it put them in different AZ.

Please add PreferredAvailabilityZone parameter.

resource "aws_elasticache_replication_group" "redis" {
    replication_group_id = "${concat(var.epop, \"-redis\")}"
    description = "test-sjc-redis"
    engine = "redis"
    cache_node_type = "cache.m3.medium"
    num_cache_clusters = 2
    automatic_failover = true
    subnet_group_name = "${aws_elasticache_subnet_group.redis.name}"
    security_group_ids = ["${aws_security_group.redis.id}"]
    depends_on = ["aws_elasticache_subnet_group.redis"]
}

Result:

{
    "ReplicationGroups": [
        {
            "Status": "available",
            "Description": "test-sjc-redis",
            "NodeGroups": [
                {
                    "Status": "available",
                    "NodeGroupMembers": [
                        {
                            "CurrentRole": "primary",
                            "PreferredAvailabilityZone": "us-west-1c",
                            "CacheNodeId": "0001",
                            "ReadEndpoint": {
                                "Port": 6379,
                                "Address": "test-sjc-redis-001.lcuh0x.0001.usw1.cache.amazonaws.com"
                            },
                            "CacheClusterId": "test-sjc-redis-001"
                        },
                        {
                            "CurrentRole": "replica",
                            "PreferredAvailabilityZone": "us-west-1c",
                            "CacheNodeId": "0001",
                            "ReadEndpoint": {
                                "Port": 6379,
                                "Address": "test-sjc-redis-002.lcuh0x.0001.usw1.cache.amazonaws.com"
                            },
                            "CacheClusterId": "test-sjc-redis-002"
                        }
                    ],
                    "NodeGroupId": "0001",
                    "PrimaryEndpoint": {
                        "Port": 6379,
                        "Address": "test-sjc-redis.lcuh0x.ng.0001.usw1.cache.amazonaws.com"
                    }
                }
            ],
            "ReplicationGroupId": "test-sjc-redis",
            "AutomaticFailover": "enabled",
            "MemberClusters": [
                "test-sjc-redis-001",
                "test-sjc-redis-002"
            ],
            "PendingModifiedValues": {}
        }
    ]
}

[adubkov]

@saulshanabrook
I made PR with this feature support to your fork.

[saulshanabrook]

Thanks @blacked, merged. This was his example usage, from his PR:

resource "aws_elasticache_replication_group" "redis" {
    replication_group_id = "${concat(var.epop, \"-redis\")}"
    description = "test-sjc-redis"
    engine = "redis"
    cache_node_type = "cache.m3.medium"
    num_cache_clusters = 2
    automatic_failover = true
    subnet_group_name = "${aws_elasticache_subnet_group.redis.name}"
    security_group_ids = ["${aws_security_group.redis.id}"]

    preferred_cache_cluster_azs = [
        "us-west-1a",
        "us-west-1c",
    ]

    depends_on = [
        "aws_elasticache_subnet_group.redis",
        "aws_security_group.redis"
    ]
}

[catsby]

Hey @saulshanabrook thank you for the code contribution! Would you by chance be willing to add documentation for this new resource while I review the code and try it out?

Thanks!

[catsby]

Am I correct in that this (as it stands) is not meant to create a cluster replication group for an existing ElastiCache Redis Cluster? There's no mention or use of PrimaryClusterId here. It seems that attribute is optional if NumCacheClusters is specified, but I'm wondering if we could add that here and support both cases?

[catsby]

ReplicationGroupStateRefreshFunc should not be exported, right? replicationGroupStateRefreshFunc instead.

Also, you send d.Id() as the id to ReplicationGroupStateRefreshFunc , before setting it with SetId, thus sending "". This causes the function to return false positives in the event that you already have a replication group defined "" will return all, and then you grab the first. In my case, I had two defined, so I got a false positive that my newest one was up and available.

Calling SetId before making the WaitForState() call should fix this 😄

[saulshanabrook]

Good catch, just fixed, thank you!

[saulshanabrook]

@catsby Yeah it looks like there are two ways to create a Replication Group and we should support both.

from the API docs:

PrimaryClusterId
The identifier of the cache cluster that will serve as the primary for this replication group. This cache cluster must already exist and have a status of available.

This parameter is not required if NumCacheClusters is specified.

Also I will add a ReplicationGroupId field to the ElasticacheCluster, which allows you to add individual clusters as read replicas.

[catsby]

Hey @saulshanabrook – sorry for the silence, is this ready for another review? Thanks again!

[saulshanabrook]

@catsby Yep

[saulshanabrook]

@catsby Anything else that I should do on my end?

[joshgarnett]

Any ETA on this?

[thegedge]

Was just about to sit down and whip up a PR to do exactly this, so if there's anything we can do to help move this along to getting merged, let me know.

[saulshanabrook]

@thegedge Awesome, thanks for taking a look at this. PR's against my branch would be really great, so that I can add your changes easier.

[jamesob]

+1. Would like to have this in.

[FlorinAndrei]

This would be incredibly useful, please merge.

[jgross206]

+1 would love this

[apparentlymart]

For various other ElastiCache resources we had issues where the AWS API would accept mixed-case names for various things but then normalize them to lowercase and fail if we try to retrieve them using the original mixed-case names. See for example #3235 and #3120.

To make sure a similar bug isn't lurking here it'd be good to make the replication_group_id in the acceptance test include some uppercase characters. If that makes it fail, then the fix is probably similar to that of the two issues I linked above. (Sorry I don't have my Terraform dev env handy to test it myself right now.)

Separately, I have a terminology question: for several of the other ElastiCache objects the main identifier for the resource is name. In this case it's called replication_group_id. Would it be appropriate to rename the argument to name for consistency with the others, or is "Replication Group Id" how this is referred to in the AWS console UI?

[saulshanabrook]

@apparentlymart That sounds like a good idea, for the case normalization. I am not working actively on this at the moment, but am happy to accept PR's against my branch.

[HarryAtMove]

Would like to be using this, please merge

[catsby]

This should not be in the .gitignore. There's some oddness with go 1.4 and 1.5 but for now this file should just not be included in any changes, more ignored. Sorry for the hassle

[catsby]

I posted a few notes, but I think this needs some follow up. We need to merge/rebase master, and we need to evaluate what we're sending to the API. I got many "invalid parameter combination" errors when trying to specify all/some of the attributes.

[saulshanabrook]

@catsby I am not actively working on this, at this time. If anyone wants these changes integrated, you are welcome to use this work here and propose a new PR onto this repo or my branch.

[dbethke]

+1. I'd like to use this.

[saulshanabrook]

I am going to close this for now, since I am not going to fix it up.

[grayaii]

Don't forget about tags... We're building from @dpetzold branch, so if possible we'll update it there...

[woodhull]

@catsby this seems like a feature that hashicorp should invest in given the original submitter closed the issue... and there are lots of people running a forked version to get the functionality.

[chadgrant]

@catsby Need this also

[aldarund]

+1, need this too

[HarryAtMove]

we could use this as well

[danielnbarbosa]

+1 this would be great to have

[dlsniper]

Can someone please lock this CLOSED issue so that it doesn't generate anymore +1 e-mails? Thank you!

[gposton]

@catsby @dlsniper, @dpetzold and I intend to follow up on the work that @saulshanabrook did here. From the inline comments, it doesn't look like there's much left to do. Am I missing something?

[inferiorhumanorgans]

@catsby any update on this?

[apparentlymart]

I've locked this since the original submitter has said he no longer intends to work on this and so further discussion here isn't productive.

If someone would like to build on this and submit a new PR then that would be much appreciated!

[catsby]

Hey Friends – sorry for the silence here. @stack72 implemented aws_elasticache_replication_group in #8275, check it out! If you find something missing or have other feedback, please open a new issue

Thanks!