resource/aws_elasticsearch_domain: Add support for encrypt_at_rest #2632

tomelliff · 2017-12-11T18:37:19Z

Adds encrypt at rest on ES domains.

Will close #2591

For now just adds encrypt at rest when creating an ES domain, doesn't yet handle reading encrypt at rest options so will likely cause Terraform to rebuild the domain on the next operation. The AWS console handily creates a service KMS key for ES when you are creating an encrypted ES domain via the console. This resource doesn't currently do that but that functionality could be added.

tomelliff · 2017-12-11T18:53:01Z

As mentioned, I think a second operation will show it wanting to rebuild the ES domain because it hasn't read the encrypt at rest options back from the API right now but the test is passing:

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

and I can see the domain is encrypted before it's deleted by the test:

$ aws es describe-elasticsearch-domains --domain-names tf-test-3881985383712683714 --region us-west-2
{
    "DomainStatusList": [
        {
            "ElasticsearchClusterConfig": {
                "DedicatedMasterEnabled": false, 
                "InstanceCount": 1, 
                "ZoneAwarenessEnabled": false, 
                "InstanceType": "m4.large.elasticsearch"
            }, 
            "Endpoint": "search-tf-test-3881985383712683714-tsewb5p4ajycmoo2jkqwlag4ru.us-west-2.es.amazonaws.com", 
            "Created": true, 
            "Deleted": true, 
            "DomainName": "tf-test-3881985383712683714", 
            "EBSOptions": {
                "VolumeSize": 10, 
                "VolumeType": "gp2", 
                "EBSEnabled": true
            }, 
            "SnapshotOptions": {
                "AutomatedSnapshotStartHour": 0
            }, 
            "DomainId": "123456789101/tf-test-3881985383712683714", 
            "AccessPolicies": "", 
            "Processing": true, 
            "AdvancedOptions": {
                "rest.action.multi.allow_explicit_index": "true"
            }, 
            "EncryptionAtRestOptions": {
                "KmsKeyId": "arn:aws:kms:us-west-2:123456789101:key/d454b888-53a9-4422-93d4-11d919a9b02c", 
                "Enabled": true
            }, 
            "ElasticsearchVersion": "6.0", 
            "ARN": "arn:aws:es:us-west-2:123456789101:domain/tf-test-3881985383712683714"
        }
    ]
}

I'll sort the read part tomorrow and see if I can add a test for it too.

As briefly mentioned in #2548 (comment) should I be checking the attributes of the ES domain are what I expect? This is what I've done elsewhere but this resource doesn't seem to do that and I'm unsure why.

Puneeth-n · 2017-12-11T20:39:47Z

aws/resource_aws_elasticsearch_domain.go

+				Computed: true,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"enabled": {


What purpose does this bool serve here? I think the behavior should be:

If kms_key_id is given, create the ES with the kms_key_id

If kms_key_id is not given, create the ES without the kms_key_id

When things change, delete and create the es domain

IMHO makes the implementation way simpler

I was mirroring the API which takes those as options. I did consider setting it automatically when kms_key_id is set but it would prevent you from having Terraform encrypt it with the service KMS key that you get by default when using the console to create an encrypted domain.

The aws_db_instance resource has them as separate parameters to the resource (rather than an options block) and defaults to using the service KMS key if kms_key_id isn't specified but that's largely because the API looks that way.

Yea true. Does AWS API create a default key if only enable is passed? If so, then having enable makes sense. If not, I am not sure whether kms key creation is within the scope of this resource.

For example, when adding triggers to lambda functions via aws console, aws does a lot of stuff in the background like creating IAM roles and adding policies and stuff. However in Terraform we don't do it and all the resources required to trigger a lambda function must be created.

Yeah looks like it does, added a test that runs without specifying the key id and that works nicely without needing to do anything extra which is neat.

Puneeth-n · 2017-12-12T11:49:33Z

aws/resource_aws_elasticsearch_domain.go

+		options := v.([]interface{})
+
+		if len(options) > 1 {
+			return fmt.Errorf("Only a single encrypt_at_rest block is expected")


I think we should also validate the instance types here since, like you mentioned, not all instance types support encryption at rest. I saw a similar approach in t2.unlimited PR. I thought it was 🆒 :)

I don't think that will run at plan time though so it's just returning a different error to what you'll get from the AWS API on apply time. You already get the following error on apply (and immediately):

Error: Error applying plan: 1 error(s) occurred: * aws_elasticsearch_domain.elasticsearch: 1 error(s) occurred: * aws_elasticsearch_domain.elasticsearch: InvalidTypeException: Encryption at rest is not supported with t2.small.elasticsearch instances status code: 409, request id: 085a21b5-df33-11e7-babb-3f32bfcf1d9b Terraform does not automatically rollback in the face of errors. Instead, your Terraform state file has been partially updated with any resources that successfully completed. Please address the error above and apply again to incrementally change your infrastructure.

From memory the validatefuncs in the schema can't validate different parameters to a resource (so you can't put a validatefunc on the encrypted parameter that checks what the instance_type is set to) so we can't put it there unfortunately to give us a plan time failure.

(Also, hardcoding what instance types support encryption in the provider means that the list has to be re-synchronized if it changes, and people need to wait for a new release of the provider, or you have to give them an override command-line flag... Too much complexity for little benefit, IMO!)

I agree with @Puneeth-n here that we should move validation to the schema here.
@tomelliff you're also right that ValidateFunc can't help in this context, but we can use MaxItems: 1 which should do the job. 😉

That's nice, means I can remove some logic there. Is it worth doing the same to some of the other blocks that are using that other style? Or (if you do want it changing) would you prefer that in a separate PR?

tomelliff · 2017-12-12T12:30:18Z

Updated docs.

Building the provider locally it seems to work as is although I'm wary that there's nothing to read the encryption config from the API right now and trying to work out if we actually need to cover this considering changing this forces a recreation.

Right now when you build an encrypted or unencrypted ES domain the state has the encrypt_at_rest config already so it's enough to know if you change this that it needs to recreate the domain. The only thing I can think of is someone could Terraform an unencrypted ES domain, someone could then delete the existing domain and recreate it as encrypted outside of Terraform and then update Terraform to want an encrypted domain and Terraform would rebuild the ES domain needlessly.

This seems like an odd thing to do but maybe it's worth reading the config from the API just to cover it anyway? Are there any other things that could cause the lack of reading the encrypted config to do bad things?

If you don't specify a KMS key then AWS will use the account's service KMS key (which is created for you automatically if you don't already have it).

tomelliff · 2017-12-12T13:38:36Z

Okay, think this is done now. Added code to read the KMS key from the AWS API. I noticed that the API takes a KMS key id in short form but reading the ES domain config on the API always returns the full ARN of the KMS key so I suppress the diff in that case.

tomelliff · 2017-12-12T16:55:56Z

Tests passing locally:

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSElasticSearchDomain_encrypt_at_rest_"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSElasticSearchDomain_encrypt_at_rest_ -timeout 120m
=== RUN   TestAccAWSElasticSearchDomain_encrypt_at_rest_default_key
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_default_key (886.10s)
=== RUN   TestAccAWSElasticSearchDomain_encrypt_at_rest_specify_key
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_specify_key (1084.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1970.795s

Quentin-M · 2017-12-17T23:29:37Z

+1 Thanks a bunch.

evhen14 · 2017-12-27T02:59:18Z

Hey guys, when do you think it's going to be merged?

sonarjames · 2017-12-27T02:59:29Z

Can this be merged now? Would love to make use of this to rebuild my ElasticSearch with encryption

vseguin · 2017-12-27T16:11:31Z

+1 for merge - would use it asap in our terraform

billcrook · 2017-12-28T14:14:36Z

+1 for merge - would use it asap in our terraform

ditto

gundam-zeta · 2018-01-04T17:10:17Z

+1

Sabotaz · 2018-01-04T17:10:21Z

+1

nvkv · 2018-01-05T07:52:04Z

Can't wait to use it

billcrook · 2018-01-05T14:33:23Z

Can't wait to use it

Me either. No, literally - we manually created the cluster until this is ready. Please please please let's get this in!

rerolled · 2018-01-08T22:44:38Z

merge plz

FireballDWF · 2018-01-09T18:06:36Z

+1 merge please

tomelliff · 2018-01-10T10:58:17Z

Just seen the merge conflicts but I'll hang off on fixing them until there's been a review on this if that's okay?

Also, for those not aware but wanting to add their 👍 to this pull request, Github allows you to add a reaction to any specific post by clicking the +(smiley face) in the top right of the post which shows everyone that you want this feature (or disagree with a 👎 or equivalent) without spamming everyone subscribed to the pull request with emails.

MichaelYak · 2018-01-18T09:26:42Z

+1 Can the feature be merged soon?

radeksimko

Hi @tomelliff
sorry for the delay in reviewing this. It's looking pretty good! I just left you 2 comments.

Also do you mind resolving the conflicts in docs?

I'd like to merge this early next week and ship by the end of that week, so I'm willing to take it to the finish line myself if you don't have any time left, just let me know 😉

radeksimko · 2018-01-21T14:41:04Z

aws/structure.go

@@ -1045,6 +1045,32 @@ func expandESEBSOptions(m map[string]interface{}) *elasticsearch.EBSOptions {
 	return &options
 }

+func flattenESEncryptAtRestOptions(o *elasticsearch.EncryptionAtRestOptions) []map[string]interface{} {
+	m := map[string]interface{}{}


Can you also add a nil check here, just to avoid potential crashes? 😉

We can just return empty []map[string]interface{}{} in such case.

Not sure I follow that, what would cause the crash? If o is nil doesn't it already return an empty []map[string]interface{}{} due to the nil checks on o.Enabled and o.KmsKeyId?

If o is nil doesn't it already return an empty []map[string]interface{}{} due to the nil checks on o.Enabled and o.KmsKeyId?

No, it will crash on line 1052 in such case because that condition is trying to access a field of a struct which isn't struct, but nil. In other words that condition is one step ahead.

See https://play.golang.org/p/R07zPiMT92M

Right, sorry, completely missed that for some reason. Fixed now, thanks for the review.

No worries 😸 That's why we do reviews.

radeksimko · 2018-01-21T14:42:48Z

aws/resource_aws_elasticsearch_domain.go

@@ -13,6 +13,7 @@ import (
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform/helper/resource"
 	"github.com/hashicorp/terraform/helper/schema"
+	"strings"


Nitpick: Do you mind moving this import above? We tend to follow the convention of grouping stdlib imports and keeping them at the top per https://github.com/golang/go/wiki/CodeReviewComments#imports

Aha, didn't spot that gofmt and/or Goland's import optimiser doesn't do this for you. Seems odd that goimports seems to add newlines between stdlib stuff when adding it and then gofmt won't rectify that either which is annoying considering gofmt is so good otherwise.

Fixed merge conflict Conflicts: aws/resource_aws_elasticsearch_domain.go

tomelliff · 2018-01-22T07:45:56Z

Are you planning on squashing this PR or would you prefer me to rebase things to clean up the commit history and the merge?

radeksimko · 2018-01-22T11:52:18Z

Are you planning on squashing this PR or would you prefer me to rebase things to clean up the commit history and the merge?

I'm going to do that as part of merging this PR, so no worries.

tomelliff · 2018-01-22T17:23:07Z

Thanks for the reviews and the merge :)

bflad · 2018-01-29T19:59:32Z

This has been released in terraform-provider-aws version 1.8.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

tehmaspc · 2018-01-29T20:01:28Z

danke!

rawatProgrammer · 2018-02-03T13:14:15Z

@tomelliff above pasted AWS cloud formation is not working. Highlighted field KMS encryption is also not in AWS documentation .
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-elasticsearch-domain.html#cfn-elasticsearch-domain-vpcoptions

Do we have other option to solve this cloud formation issue ?

tomelliff · 2018-02-05T20:18:10Z

What Cloudformation issue? This added support for it to Terraform, not Cloudformation.

ghost · 2020-04-08T17:23:50Z

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!

tomelliff mentioned this pull request Dec 11, 2017

Downsize ES domains to save money #2548

Merged

Puneeth-n reviewed Dec 11, 2017

View reviewed changes

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

Puneeth-n reviewed Dec 12, 2017

View reviewed changes

tomelliff changed the title ~~[WIP] Add encrypt at rest to ES domains~~ Add encrypt at rest to ES domains Dec 12, 2017

tomelliff added 2 commits December 12, 2017 12:33

Allow encryption of ES domain with service KMS key

793f0f1

If you don't specify a KMS key then AWS will use the account's service KMS key (which is created for you automatically if you don't already have it).

Add documentation for ES encryption at rest

5d1b545

tomelliff force-pushed the es-encrypt-at-rest branch from 703daea to 5d1b545 Compare December 12, 2017 12:34

Read KMS key for encrypt at rest from AWS API

ca73236

radeksimko added the service/elasticsearch Issues and PRs that pertain to the elasticsearch service. label Jan 12, 2018

radeksimko added this to the v1.8.0 milestone Jan 18, 2018

radeksimko self-assigned this Jan 18, 2018

radeksimko reviewed Jan 21, 2018

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 21, 2018

radeksimko mentioned this pull request Jan 21, 2018

resource aws_elasticsearch_domain missing encryption at rest #3074

Closed

tomelliff added 3 commits January 21, 2018 19:39

Fix PR feed back on hashicorp#2632

a56d511

Merge remote-tracking branch 'origin/master' into es-encrypt-at-rest

60a99c4

Fixed merge conflict Conflicts: aws/resource_aws_elasticsearch_domain.go

fixup a56d511

a13f2a9

radeksimko changed the title ~~Add encrypt at rest to ES domains~~ resource/aws_elasticsearch_domain: Add support for encrypt_at_rest Jan 22, 2018

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 22, 2018

radeksimko merged commit 924b18d into hashicorp:master Jan 22, 2018

tomelliff deleted the es-encrypt-at-rest branch January 22, 2018 17:22

bflad mentioned this pull request Jan 24, 2018

aws_elasticsearch_domain: Extend ebs_options by "Enable encryption at rest" #3121

Closed

ghost locked and limited conversation to collaborators Apr 8, 2020

resource/aws_elasticsearch_domain: Add support for encrypt_at_rest #2632

resource/aws_elasticsearch_domain: Add support for encrypt_at_rest #2632

Conversation

tomelliff commented Dec 11, 2017 • edited Loading

tomelliff commented Dec 11, 2017

Puneeth-n Dec 11, 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

tomelliff Dec 12, 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

tomelliff commented Dec 12, 2017

tomelliff commented Dec 12, 2017 • edited Loading

tomelliff commented Dec 12, 2017

Quentin-M commented Dec 17, 2017

evhen14 commented Dec 27, 2017

sonarjames commented Dec 27, 2017

vseguin commented Dec 27, 2017

billcrook commented Dec 28, 2017

gundam-zeta commented Jan 4, 2018

Sabotaz commented Jan 4, 2018

nvkv commented Jan 5, 2018

billcrook commented Jan 5, 2018

rerolled commented Jan 8, 2018

FireballDWF commented Jan 9, 2018

tomelliff commented Jan 10, 2018

MichaelYak commented Jan 18, 2018

radeksimko left a comment • edited Loading

Choose a reason for hiding this comment

radeksimko Jan 21, 2018 • 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

tomelliff commented Jan 22, 2018

radeksimko commented Jan 22, 2018

tomelliff commented Jan 22, 2018

bflad commented Jan 29, 2018

tehmaspc commented Jan 29, 2018

rawatProgrammer commented Feb 3, 2018

tomelliff commented Feb 5, 2018

ghost commented Apr 8, 2020

tomelliff commented Dec 11, 2017 •

edited

Loading

Puneeth-n Dec 11, 2017 •

edited

Loading

tomelliff Dec 12, 2017 •

edited

Loading

tomelliff commented Dec 12, 2017 •

edited

Loading

radeksimko left a comment •

edited

Loading

radeksimko Jan 21, 2018 •

edited

Loading