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 4 commits
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
48 changes: 48 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 @@ -13,6 +13,7 @@ import (
"github.com/hashicorp/errwrap"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"strings"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

)

func resourceAwsElasticSearchDomain() *schema.Resource {
Expand Down Expand Up @@ -89,6 +90,27 @@ 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,
Optional: true,
Computed: true,
ForceNew: true,
DiffSuppressFunc: suppressEquivalentKmsKeyIds,
},
},
},
},
"cluster_config": {
Type: schema.TypeList,
Optional: true,
Expand Down Expand Up @@ -291,6 +313,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 Expand Up @@ -461,6 +498,10 @@ func resourceAwsElasticSearchDomainRead(d *schema.ResourceData, meta interface{}
if err != nil {
return err
}
err = d.Set("encrypt_at_rest", flattenESEncryptAtRestOptions(ds.EncryptionAtRestOptions))
if err != nil {
return err
}
err = d.Set("cluster_config", flattenESClusterConfig(ds.ElasticsearchClusterConfig))
if err != nil {
return err
Expand Down Expand Up @@ -674,3 +715,10 @@ func resourceAwsElasticSearchDomainDelete(d *schema.ResourceData, meta interface

return err
}

func suppressEquivalentKmsKeyIds(k, old, new string, d *schema.ResourceData) bool {
// The Elasticsearch API accepts a short KMS key id but always returns the ARN of the key.
// The ARN is of the format 'arn:aws:kms:REGION:ACCOUNT_ID:key/KMS_KEY_ID'.
// These should be treated as equivalent.
return strings.Contains(old, new)
}
104 changes: 104 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,44 @@ func TestAccAWSElasticSearchDomain_policy(t *testing.T) {
})
}

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

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

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

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

func TestAccAWSElasticSearchDomain_tags(t *testing.T) {
var domain elasticsearch.ElasticsearchDomainStatus
var td elasticsearch.ListTagsOutput
Expand Down Expand Up @@ -339,6 +377,16 @@ func testAccCheckESNumberOfInstances(numberOfInstances int, status *elasticsearc
}
}

func testAccCheckESEncrypted(encrypted bool, status *elasticsearch.ElasticsearchDomainStatus) resource.TestCheckFunc {
return func(s *terraform.State) error {
conf := status.EncryptionAtRestOptions
if *conf.Enabled != encrypted {
return fmt.Errorf("Encrypt at rest not set properly. Given: %t, Expected: %t", *conf.Enabled, encrypted)
}
return nil
}
}

func testAccLoadESTags(conf *elasticsearch.ElasticsearchDomainStatus, td *elasticsearch.ListTagsOutput) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).esconn
Expand Down Expand Up @@ -411,6 +459,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 +553,61 @@ data "aws_iam_policy_document" "instance-assume-role-policy" {
`, randESId, randRoleId)
}

func testAccESDomainConfigWithEncryptAtRestDefaultKey(randESId int) string {
return fmt.Sprintf(`

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
}
}
`, randESId)
}

func testAccESDomainConfigWithEncryptAtRestWithKey(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
26 changes: 26 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,32 @@ func expandESEBSOptions(m map[string]interface{}) *elasticsearch.EBSOptions {
return &options
}

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

@radeksimko radeksimko Jan 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

No worries 😸 That's why we do reviews.


if o.Enabled != nil {
m["enabled"] = *o.Enabled
}
if o.KmsKeyId != nil {
m["kms_key_id"] = *o.KmsKeyId
}

return []map[string]interface{}{m}
}

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
6 changes: 6 additions & 0 deletions website/docs/r/elasticsearch_domain.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ The following arguments are supported:
* `access_policies` - (Optional) IAM policy document specifying the access policies for the domain
* `advanced_options` - (Optional) Key-value string pairs to specify advanced configuration options.
* `ebs_options` - (Optional) EBS related options, may be required based on chosen [instance size](https://aws.amazon.com/elasticsearch-service/pricing/). See below.
* `encrypt_at_rest` - (Optional) Encrypt at rest options. Only available for [certain instance types](http://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/aes-supported-instance-types.html). See below.
* `cluster_config` - (Optional) Cluster configuration of the domain, see below.
* `snapshot_options` - (Optional) Snapshot related options, see below.
* `vpc_options` - (Optional) VPC related options, see below. Adding or removing this configuration forces a new resource ([documentation](https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-vpc.html#es-vpc-limitations)).
Expand All @@ -73,6 +74,11 @@ The following arguments are supported:
* `iops` - (Optional) The baseline input/output (I/O) performance of EBS volumes
attached to data nodes. Applicable only for the Provisioned IOPS EBS volume type.

**encrypt_at_rest** supports the following attributes:

* `enabled` - (Required) Whether to enable encryption at rest. If the `encrypt_at_rest` block is not provided then this defaults to `false`.
* `kms_key_id` - (Optional) The KMS key id to encrypt the Elasticsearch domain with. If not specified then it defaults to using the `aws/es` service KMS key.

**cluster_config** supports the following attributes:

* `instance_type` - (Optional) Instance type of data nodes in the cluster.
Expand Down