Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_elasticsearch_domain: Add support for encrypt_at_rest #2632

Merged
merged 7 commits into from
Jan 22, 2018
Merged

resource/aws_elasticsearch_domain: Add support for encrypt_at_rest #2632

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
69a87dc
Add encrypt at rest to ES domains
tomelliff Dec 11, 2017
793f0f1
Allow encryption of ES domain with service KMS key
tomelliff Dec 12, 2017
5d1b545
Add documentation for ES encryption at rest
tomelliff Dec 12, 2017
ca73236
Read KMS key for encrypt at rest from AWS API
tomelliff Dec 12, 2017
a56d511
Fix PR feed back on #2632
tomelliff Jan 21, 2018
60a99c4
Merge remote-tracking branch 'origin/master' into es-encrypt-at-rest
tomelliff Jan 21, 2018
a13f2a9
fixup a56d511fb9ae890601e76c2423762e6b38830003
tomelliff Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions aws/resource_aws_elasticsearch_domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,24 @@ func resourceAwsElasticSearchDomain() *schema.Resource {
},
},
},
"encrypt_at_rest": {
Type: schema.TypeList,
Optional: true,
Computed: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"enabled": {
Copy link
Contributor

@Puneeth-n Puneeth-n Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely! :)

Type: schema.TypeBool,
Required: true,
ForceNew: true,
},
"kms_key_id": {
Type: schema.TypeString,
Required: true,
},
},
},
},
"cluster_config": {
Type: schema.TypeList,
Optional: true,
Expand Down Expand Up @@ -291,6 +309,21 @@ func resourceAwsElasticSearchDomainCreate(d *schema.ResourceData, meta interface
}
}

if v, ok := d.GetOk("encrypt_at_rest"); ok {
options := v.([]interface{})

if len(options) > 1 {
return fmt.Errorf("Only a single encrypt_at_rest block is expected")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🆒 :)

Copy link
Contributor Author

@tomelliff tomelliff Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

} else if len(options) == 1 {
if options[0] == nil {
return fmt.Errorf("At least one field is expected inside encrypt_at_rest")
}

s := options[0].(map[string]interface{})
input.EncryptionAtRestOptions = expandESEncryptAtRestOptions(s)
}
}

if v, ok := d.GetOk("cluster_config"); ok {
config := v.([]interface{})

Expand Down
49 changes: 49 additions & 0 deletions aws/resource_aws_elasticsearch_domain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,24 @@ func TestAccAWSElasticSearchDomain_policy(t *testing.T) {
})
}

func TestAccAWSElasticSearchDomain_encrypt_at_rest(t *testing.T) {
var domain elasticsearch.ElasticsearchDomainStatus

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckESDomainDestroy,
Steps: []resource.TestStep{
{
Config: testAccESDomainConfigWithEncryptAtRest(acctest.RandInt()),
Check: resource.ComposeTestCheckFunc(
testAccCheckESDomainExists("aws_elasticsearch_domain.example", &domain),
),
},
},
})
}

func TestAccAWSElasticSearchDomain_tags(t *testing.T) {
var domain elasticsearch.ElasticsearchDomainStatus
var td elasticsearch.ListTagsOutput
Expand Down Expand Up @@ -411,6 +429,7 @@ func testAccESDomainConfig(randInt int) string {
return fmt.Sprintf(`
resource "aws_elasticsearch_domain" "example" {
domain_name = "tf-test-%d"

ebs_options {
ebs_enabled = true
volume_size = 10
Expand Down Expand Up @@ -504,6 +523,36 @@ data "aws_iam_policy_document" "instance-assume-role-policy" {
`, randESId, randRoleId)
}

func testAccESDomainConfigWithEncryptAtRest(randESId int) string {
return fmt.Sprintf(`
resource "aws_kms_key" "es" {
description = "kms-key-for-tf-test-%d"
deletion_window_in_days = 7
}

resource "aws_elasticsearch_domain" "example" {
domain_name = "tf-test-%d"

elasticsearch_version = "6.0"

# Encrypt at rest requires m4/c4/r4/i2 instances. See http://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/aes-supported-instance-types.html
cluster_config {
instance_type = "m4.large.elasticsearch"
}

ebs_options {
ebs_enabled = true
volume_size = 10
}

encrypt_at_rest {
enabled = true
kms_key_id = "${aws_kms_key.es.key_id}"
}
}
`, randESId, randESId)
}

func testAccESDomainConfig_complex(randInt int) string {
return fmt.Sprintf(`
resource "aws_elasticsearch_domain" "example" {
Expand Down
13 changes: 13 additions & 0 deletions aws/structure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,19 @@ func expandESEBSOptions(m map[string]interface{}) *elasticsearch.EBSOptions {
return &options
}

func expandESEncryptAtRestOptions(m map[string]interface{}) *elasticsearch.EncryptionAtRestOptions {
options := elasticsearch.EncryptionAtRestOptions{}

if v, ok := m["enabled"]; ok {
options.Enabled = aws.Bool(v.(bool))
}
if v, ok := m["kms_key_id"]; ok && v.(string) != "" {
options.KmsKeyId = aws.String(v.(string))
}

return &options
}

func flattenESVPCDerivedInfo(o *elasticsearch.VPCDerivedInfo) []map[string]interface{} {
m := map[string]interface{}{}

Expand Down