Add Redis AUTH, in-transit and at-rest encryption #2090

nathanielks · 2017-10-27T20:37:24Z

AWS announced support for AUTH, in-transit and at-rest encryption. This aims to add that to terraform!

nathanielks · 2017-10-28T00:38:39Z

This is ready for review! Questions I have for reviewers are:

AWS has additional requirements for in-transit encryption like redis version, node types, etc. Do we need to add validation before attempting to create the resource, or just defer to the API rejecting the request? I opted for that to keep this simple.
When should I put together a PR for docs? Now or after this has been merged?

This is my first code contribution and it's been a great experience. Thank you for writing such good software! The testing helpers and clear patterns made contributing a breeze. I hadn't ever really looked at source till last night and I've got a PR put together today!! Thanks again!

ewbankkit · 2017-10-29T20:54:43Z

@nathanielks Thanks for tackling this and glad you found contributing so straightforward.

You should also update the corresponding documentation page and since #2088 was merged, you can undo the changes you made in the vendor tree and git rebase from the upstream master branch.

For completeness I suggest updating the aws_elasticache_cluster resource and data source for those not using replication groups.

nathanielks · 2017-10-30T22:15:42Z

@ewbankkit hitting a bit of a blocker and not sure how to resolve! The AWS Go SDK has a few different structs it's using for Elasticache. Terraform uses the CreateCacheClusterInput and CreateReplicationGroupInput structs. Transit Encryption and At Rest Encryption are only available on the CacheCluster, CreateReplicationGroupInput, and ReplicationGroup structs. AuthToken is available on CreateCacheClusterInput, however. If the encryption options aren't available on the Input struct, how do we get around that?

nathanielks · 2017-10-30T23:58:51Z

I've tested by adding just the auth token to the CreateCacheClusterInput and the API errors with the following:

* aws_elasticache_cluster.bar: Error creating Elasticache: InvalidParameterValue: This AWS account is not whitelisted for Redis Auth but AuthToken is provided.
	status code: 400, request id: 50ef2190-bdc7-11e7-a938-c3d192d48d

tavisma · 2017-11-01T23:23:30Z

Eagerly awaiting this!

nathanielks · 2017-11-01T23:35:50Z

Maybe @radeksimko or another core committer can recommend how to resolve this issue?

#2090 (comment)

nathanielks · 2017-11-06T20:21:17Z

@ewbankkit Can I get your eyes on #2090 (comment)?

handlerbot · 2017-11-06T20:23:57Z

@nathanielks (not a Terraform committer, just a contributor with a vested interested in encryption support in AWS resources in Terraform...) What do you think about splitting the AUTH work to a separate PR, since the encryption-at-creation options are simpler and working, rather than delaying the entire package until whatever is going on with AUTH can be resolved?

(Thanks very much for working on adding this support, BTW!)

handlerbot · 2017-11-06T20:31:05Z

@nathanielks Also, as much as this sounds like "Why don't you go down to the DMV..." (apologies!) you may want to engage with AWS support on the error you're getting. That could be related to the AWS region or account you're using for testing, or it could be a bug with AWS... I wouldn't necessarily expect the Terraform folks to have any knowledge past what we have re: freshly released AWS features. (Which I am learning the hard way are often buggy in surprising/hilarious ways...)

nathanielks · 2017-11-06T21:09:25Z

@handlerbot

What do you think about splitting the AUTH work to a separate PR

Unfortunately they're tied together. AUTH depends on in-transit encryption (which practically is a good thing, but annoying at the moment!)

you may want to engage with AWS support on the error you're getting

That's a great call! They may also be able to explain why the Go sdk is structured like it is! I'll reach out on their repo as well 😄

nathanielks · 2017-11-06T21:09:49Z

(also, the auth-only tests were to illustrate they'll need in-transit encryption)

handlerbot · 2017-11-06T21:27:57Z

@nathanielks Sure, but: I was envisioning pulling the auth work out of this PR, holding it, getting the encryption PR merged (which should be simple because it's easy setup-time-only parameters), which insures that encryption functionality at least will be available in provider version 1.3.0. Then bring the auth functionality back in a separate PR, on top of the existing and already committed encryption functionality, and with a little luck that'll make 1.3.0 too.

There's no question this is a little self-serving, as I care lots about getting the encryption support in ASAP, and not much for the auth functionality (yet), but I do think this is a good approach for getting the working 2/3 of your work committed and checkpointed, so however long it takes AWS to help you or fix their doc or bug doesn't delay getting the whole thing in. :-)

(Just a thought, thx for reading.)

nathanielks · 2017-11-06T22:20:20Z

@handlerbot Ah! Thanks for clarifying! So the problem isn't AUTH, it's 100% with the structs that the Go SDK provides for us. AUTH in this PR actually works fine, but it's only for Replication Groups as they are the only resource that exposes any encryption parameters. I'm now trying to figure out how to get Encryption set up on basic Clusters per @ewbankkit's request. You can actually get the same functionality with a replication group as you can with a cluster. Enabling encryption forces the creation of a new resource, so we're forced to create a new something. Whether that's a cache cluster or a replication group, I'm not sure it entirely matters. To get a replication group that behaves like a cache cluster, go ahead and use this config:

resource "aws_elasticache_replication_group" "bar" {
  replication_group_id          = "${var.name}"
  replication_group_description = "replication-group-example"
  node_type                     = "cache.t2.micro"
  number_cache_clusters         = "1"
  port                          = 6379
  parameter_group_name          = "default.redis3.2"
  engine_version                = "3.2.4"
}

To that end, @ewbankkit, is it 100% necessary to add encryption support to aws_elasticache_cluster, consider it's a redis-only feature and it's already supported on the replication group resource? Users will need to a create a new something as explained above, so why not force them to use the replication group resource instead of the cache cluster?

nathanielks · 2017-11-06T22:23:24Z

And more fuel in support of using replication groups, the in-transit option is only supported by redis 3.2.4 and above, which is what replication groups require...

nathanielks · 2017-11-06T22:27:23Z

@ewbankkit looking into this further, the encryption options are only supported by Replication Groups in the API as well: https://docs.aws.amazon.com/AmazonElastiCache/latest/APIReference/API_CreateReplicationGroup.html, so we can't create a cache cluster with encryption enabled. It's only replication groups.

ewbankkit · 2017-11-07T12:31:10Z

@nathanielks Yes, my bad. The User Guide seems to indicate that CreateCacheCluster supports those options but the API documentation makes no mention.

myoung34 · 2017-11-07T16:05:18Z

Would it be easier to split this up into 2 pr's, one for auth one for encryption?
I need encryption ASAP and it seems to be more straight forward vs the auth changes

nathanielks · 2017-11-07T20:01:19Z

@ewbankkit do you know how to bump this to get this reviewed? I think I've followed the contribution guidelines and haven't seen any core committers come around.

nathanielks · 2017-11-07T22:05:04Z

Acceptance tests are passing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption -timeout 120m
=== RUN   TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption
--- PASS: TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption (875.50s)
PASS
ok  	github.com/nathanielks/terraform-provider-aws/aws	875.530s

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption -timeout 120m
=== RUN   TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption
--- PASS: TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption (1059.05s)
PASS
ok  	github.com/nathanielks/terraform-provider-aws/aws	1059.087s

nathanielks · 2017-11-07T22:06:19Z

@myoung34 No need to split into two PR's. Encryption can only be added to Replication Groups and that's what this PR does.

ewbankkit · 2017-11-07T23:13:27Z

@nathanielks I suggest patience 😄.

nathanielks · 2017-11-13T20:15:11Z

@radeksimko this is ready for you! Acceptance tests written and passing!

sstarcher · 2017-11-30T19:06:29Z

@radeksimko does any additional work need done to get this merged in?

ChrisMcKee · 2017-11-30T22:11:21Z

It's on their radar

nathanielks · 2017-12-01T15:12:33Z

Good eyes, @ChrisMcKee

radeksimko

Hi @nathanielks
sorry for the delay in reviewing your PR.

I left you a few comments with varying levels of importance. Let me know if you have any questions or you need further clarification.

Thanks for the contribution.

radeksimko · 2017-12-06T16:32:48Z

aws/resource_aws_elasticache_replication_group.go

@@ -165,6 +187,18 @@ func resourceAwsElasticacheReplicationGroupCreate(d *schema.ResourceData, meta i
 		params.SnapshotName = aws.String(v.(string))
 	}

+	if _, ok := d.GetOk("transit_encryption"); ok {


It will sound like a nitpick, but would you mind changing the field name to transit_encryption_enabled? It will better communicate the fact that this is a boolean field and make space for potential future transit_encryption block which may define some further settings of the encryption.

Sure thing!

radeksimko · 2017-12-06T16:33:27Z

aws/resource_aws_elasticache_replication_group.go

+		params.TransitEncryptionEnabled = aws.Bool(d.Get("transit_encryption").(bool))
+	}
+
+	if _, ok := d.GetOk("at_rest_encryption"); ok {


Likewise - would you mind changing it to at_rest_encryption_enabled for the same reasons as the above field?

radeksimko · 2017-12-06T16:38:09Z

aws/resource_aws_elasticache_replication_group.go

@@ -310,6 +344,9 @@ func resourceAwsElasticacheReplicationGroupRead(d *schema.ResourceData, meta int
 		}

 		d.Set("auto_minor_version_upgrade", c.AutoMinorVersionUpgrade)
+		d.Set("at_rest_encryption", c.AtRestEncryptionEnabled)
+		d.Set("transit_encryption", c.TransitEncryptionEnabled)
+		d.Set("auth_token", c.TransitEncryptionEnabled)


Typo ☝️

Because we cannot get the token back from the API (for obvious reasons) I think we should just make it nil if c.AuthTokenEnabled != nil && !*c.AuthTokenEnabled otherwise leave it alone during Read().

ah, great catch!

radeksimko · 2017-12-06T16:39:56Z

aws/resource_aws_elasticache_replication_group.go

@@ -532,3 +569,16 @@ func validateAwsElastiCacheReplicationGroupId(v interface{}, k string) (ws []str
 	}
 	return
 }
+
+func validateAwsElastiCacheReplicationGroupAuthToken(v interface{}, k string) (ws []string, errors []error) {


Nitpick: We tend to keep our validation functions in aws/validators.go + tests in aws/validators_test.go. It would be nice to move it there too, it's not a blocker for this PR though.

I'll move them!

radeksimko · 2017-12-06T16:44:21Z

aws/resource_aws_elasticache_replication_group_test.go

@@ -970,3 +1048,108 @@ resource "aws_elasticache_replication_group" "bar" {
    }
 }`, rInt, rInt, rInt, rInt, rName)
 }
+
+var testAccAWSElasticacheReplicationGroupEnableAtRestEncryptionConfig = fmt.Sprintf(`


Nitpick: It's a common convention to turn these configs into functions and pass the random strings from the body of the test as argument. It makes it easier to build & compare fields which include these random strings and also makes the config reusable (as it doesn't contain a random string generated once during initiation of the variable).

I know we don't follow that convention everywhere yet, but it's been a common theme of many recent test changes. 😉

…n groups

…s.go, as well as tests

…nctions instead of vars

nathanielks · 2017-12-08T02:24:36Z

@radeksimko requested changes have been made, tests still passing!

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption -timeout 120m
=== RUN   TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption
--- PASS: TestAccAWSElasticacheReplicationGroup_enableAuthTokenTransitEncryption (674.39s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       674.431s

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption -timeout 120m
=== RUN   TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption
 --- PASS: TestAccAWSElasticacheReplicationGroup_enableAtRestEncryption (744.07s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       744.118s

radeksimko

Thanks, I have just fixed a last small thing - indentation. As I didn't change anything else I'll assume this change doesn't need further review from anyone else.

git diff -w | wc -l
       0

* Add Data Source: aws_elb * Fix dataSourceAwsElb typo * Fix dataSourceAwsElb Schema name field to not include Computed * Remove dataSourceAwsElb schema defaults for computed fields * Remove dataSourceAwsElb schema defaults for nested computed fields too * Corrected depends_on entry for EIP depends_on = ["aws_internet_gateway.gw"] is the correct syntax * Add sweeper for IAM Server Certificates * test/aws_config_delivery_channel: Add missing dependencies * d/aws_elb r/aws_elb: hashicorp#2004 review comments * Remove enable_deletion_protection from testAccDataSourceAWSELBConfigBasic * Replace unnecessary errwrap.Wrapf with fmt.Errorf * Reduce flattenAwsELbResource to ec2conn and elbconn instead of meta * Properly name TestAccDataSourceAWSELB_basic resources * Use t.Name() for description and TestName tags * d/aws_elb: Fix documentation sidebar ordering after merging master with new d/aws_elasticache_replication_group * Makefile: Add sweep target * Update cognito_user_pool.markdown * Update CHANGELOG.md * r/aws_elasticache_security_group: add import support (hashicorp#2277) * r/aws_elasticache_security_group: add import support * r/aws_elasticache_security_group: hashicorp#2277 review updates * Use d.Id() instead of d.Get("name") on read, which allows using schema.ImportStatePassthrough * d.Set("security_group_names") on read * Set AWS_DEFAULT_REGION to us-east-1 on import testing * Update CHANGELOG.md * documentation: remove antislashes in page titles * Added missing WARN debug lines when reading a non-existing resource * Removed <wbr> from documentation titles * vendor: Bump aws-sdk-go to v.1.12.44 * Add logs for iam server certificate delete conflict (hashicorp#2533) * Query elb API for load balancer arn causing delete conflict - For IAM server certificate. * Use regex for lb name. * Edits for hashicorp#2533 * r/aws_sqs_queue_policy: Support import by queue URL (hashicorp#2544) * Update CHANGELOG.md * r/aws_elasticsearch_domain: Add LogPublishingOption (hashicorp#2285) * WIP * Add enabled * Use cwl policy * Reflect reviews * Update CHANGELOG.md * Add force_destroy field to aws_athena_database (hashicorp#2363) * Add force_destroy field to aws_athena_database. Fixes hashicorp#2362. * Remove unnecessary import. * Code review feedback * Update CHANGELOG.md * Add more example and missing field * New Resource: aws_media_store_container (hashicorp#2448) * New Resource: aws_media_store_container * Reflect reviews * remove policy * Update CHANGELOG.md * Add Redis AUTH, in-transit and at-rest encryption (hashicorp#2090) * add AUTH, at-rest and in-transit encryption to Elasticache replication groups * add _enabled to transit/at_rest encyrption parameters * added one more _enabled * move validateAwsElastiCacheReplicationGroupAuthToken to aws/validators.go, as well as tests * set auth_token to nil during Reads * update Replication Group encryption acceptance tests to use config functions instead of vars * Fix whitespacing (tabs -> spaces) * docs/elasticache_replication_group: Add missing fields * Update CHANGELOG.md * r/aws_dynamodb_table: Ensure ttl is properly read (hashicorp#2452) * r/aws_dynamodb_table: Ensure ttl is properly read * r/aws_dynamodb_table: hashicorp#2452 review updates * Add timeToLiveOutput.TimeToLiveDescription nil check * Simplify logic to d.Set ttl * Update CHANGELOG.md * Bump aws-sdk-go to v.1.12.45 * New Resource: PublicDnsNamespace (hashicorp#2569) * WIP * Add test, docs * Reflect reviews * Modify error handling * Update CHANGELOG.md * New Resource: ServiceDiscovery PrivateDNS Namespace (hashicorp#2589) * New Resource: service_discovery_private_dns_namespace * Reflect reviews * Update CHANGELOG.md

nathanielks · 2017-12-13T20:09:13Z

Hooray!!

ghost · 2020-04-07T17:27:34Z

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!

nathanielks mentioned this pull request Oct 27, 2017

Support ElastiCache/Redis In-Transit and At-Rest Encryption #2087

Closed

nathanielks changed the title ~~[WIP] Add Redis in-transit encryption~~ [WIP] Add Redis AUTH, in-transit and at-rest encryption Oct 28, 2017

nathanielks changed the title ~~[WIP] Add Redis AUTH, in-transit and at-rest encryption~~ Add Redis AUTH, in-transit and at-rest encryption Oct 28, 2017

radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Oct 28, 2017

nathanielks force-pushed the transit-encryption branch from f18c925 to 5096a32 Compare October 30, 2017 22:13

nathanielks force-pushed the transit-encryption branch from 5096a32 to 092f58b Compare October 30, 2017 22:33

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

radeksimko suggested changes Dec 6, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 6, 2017

add AUTH, at-rest and in-transit encryption to Elasticache replicatio…

8b67b88

…n groups

nathanielks force-pushed the transit-encryption branch from 092f58b to 8b67b88 Compare December 8, 2017 00:29

nathanielks added 5 commits December 7, 2017 16:50

add _enabled to transit/at_rest encyrption parameters

18fa49a

added one more _enabled

8c30b57

move validateAwsElastiCacheReplicationGroupAuthToken to aws/validator…

1b2e21d

…s.go, as well as tests

set auth_token to nil during Reads

970e9c8

update Replication Group encryption acceptance tests to use config fu…

ea414bc

…nctions instead of vars

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 11, 2017

Fix whitespacing (tabs -> spaces)

e25fed5

radeksimko approved these changes Dec 11, 2017

View reviewed changes

radeksimko merged commit f4800f6 into hashicorp:master Dec 11, 2017

radeksimko mentioned this pull request Dec 12, 2017

docs/elasticache_replication_group: Add missing fields #2638

Merged

nathanielks deleted the transit-encryption branch December 21, 2017 23:24

nathanielks mentioned this pull request Jan 7, 2018

Add cache option to AWS CodeBuild projects #2860

Merged

nathanielks mentioned this pull request Feb 19, 2018

Looks good! Moved on? Jonty/aws_redis_elasticache_proxy#1

Closed

bflad added this to the v1.6.0 milestone Mar 21, 2018

bflad added the service/elasticache Issues and PRs that pertain to the elasticache service. label Mar 21, 2018

tomdev mentioned this pull request Jun 25, 2018

Support at-rest and in-transit encryption for aws_elasticache_cluster (Redis) #4973

Closed

alkar mentioned this pull request Sep 13, 2018

Change the elasticache module to support redis only ministryofjustice/cloud-platform#377

Closed

5 tasks

ghost locked and limited conversation to collaborators Apr 7, 2020

Add Redis AUTH, in-transit and at-rest encryption #2090

Add Redis AUTH, in-transit and at-rest encryption #2090

Conversation

nathanielks commented Oct 27, 2017 • edited Loading

nathanielks commented Oct 28, 2017

ewbankkit commented Oct 29, 2017

nathanielks commented Oct 30, 2017

nathanielks commented Oct 30, 2017

tavisma commented Nov 1, 2017

nathanielks commented Nov 1, 2017

nathanielks commented Nov 6, 2017

handlerbot commented Nov 6, 2017

handlerbot commented Nov 6, 2017

nathanielks commented Nov 6, 2017

nathanielks commented Nov 6, 2017

handlerbot commented Nov 6, 2017 • edited Loading

nathanielks commented Nov 6, 2017

nathanielks commented Nov 6, 2017

nathanielks commented Nov 6, 2017

ewbankkit commented Nov 7, 2017

myoung34 commented Nov 7, 2017

nathanielks commented Nov 7, 2017

nathanielks commented Nov 7, 2017

nathanielks commented Nov 7, 2017

ewbankkit commented Nov 7, 2017

nathanielks commented Nov 13, 2017

sstarcher commented Nov 30, 2017

ChrisMcKee commented Nov 30, 2017

nathanielks commented Dec 1, 2017

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielks commented Dec 8, 2017

radeksimko left a comment

Choose a reason for hiding this comment

nathanielks commented Dec 13, 2017

ghost commented Apr 7, 2020

nathanielks commented Oct 27, 2017 •

edited

Loading

handlerbot commented Nov 6, 2017 •

edited

Loading

radeksimko Dec 6, 2017 •

edited

Loading