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

Support VPC configuration of aws_elasticsearch_domain resources. #1958

Merged
merged 6 commits into from
Oct 26, 2017
Merged

Support VPC configuration of aws_elasticsearch_domain resources. #1958

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1a2d978
Support VPC configuration of aws_elasticsearch_domain resources.
handlerbot Oct 19, 2017
f57cd53
Elasticsearch VPC support: creation/endpoint bugfixes; VPC acceptance…
handlerbot Oct 21, 2017
54d14e1
Elasticsearch VPC support: PR feedback
handlerbot Oct 21, 2017
4b3dd70
Elasticsearch VPC support: 2nd PR feedback
handlerbot Oct 24, 2017
737d763
Elasticsearch VPC support: 3rd PR feedback.
handlerbot Oct 25, 2017
d4945cc
Simplify error handling + retry on missing service role
radeksimko Oct 26, 2017
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
92 changes: 88 additions & 4 deletions aws/resource_aws_elasticsearch_domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,38 @@ func resourceAwsElasticSearchDomain() *schema.Resource {
},
},
},
"vpc_options": {
Type: schema.TypeList,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"security_group_ids": {
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},
"subnet_ids": {
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},
},
},
},
"vpc_availability_zones": {
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},
"vpc_id": {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason for keeping those two fields outside of vpc_options and drift away from the API?

Copy link
Contributor Author

@handlerbot handlerbot Oct 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The ES-in-VPC API is kind of weird compared to the other AWS APIs, from what I've seen. You don't actually pass in the vpc_id or availability_zones as a parameter to the API, you pass in a list of subnets that you want the ES domain to have endpoints in, and AWS enforces that all of those subnets are in the same VPC, and calculates vpc_id and availability_zones from the subnets, and returns them to you in a slightly different datatype.

Structure VPCOptions has SubnetIds and SecurityGroupIds, and you use that for create and update requests for the domain.

Structure VPCDerivedInfo has VPCId, SubnetIds, AvailabilityZones, and SecurityGroupIds, and you get that back as part of a ElasticsearchDomainStatus struct during create or delete requests.

And then there's a structure VPCDerivedInfoStatus, which contains both a VPCDerivedInfo and an OptionStatus structure (unrelated to VPC), which you get back as part of ElasticsearchDomainConfig if you describe the domain.

And then to make your life even more interesting, the API uses the name VPCOptions to refer to all three of the structures, without even trying to do anything Go-idiomatic like defining an interface that just has the two common fields (which would work for VPCOptions and VPCDerivedInfo, but not for VPCDerivedInfoStatus, because VPCDerivedInfo is a named member of it).

$ grep 'VPCOptions.*structure' vendor/github.com/aws/aws-sdk-go/service/elasticsearchservice/api.go
	VPCOptions *VPCOptions `type:"structure"`
	VPCOptions *VPCDerivedInfoStatus `type:"structure"`
	VPCOptions *VPCDerivedInfo `type:"structure"`
	VPCOptions *VPCOptions `type:"structure"`

So, the whole thing is kind of a mess, but after puzzling all of that out, I figured the least complicated thing was, for the Terraform code, to keep vpc_options equivalent to VPCOptions.

But the end of it is: You can't specify the VPC id or the availability zones via the API, you have to let AWS compute them for you and return them, so they're never options that an operator will specify. And, once they are returned, I wanted to export them as top-level attributes of the resource, as I see other resources do, and I don't see a way to export schema sub-members as top-level attributes, other than defining them twice and exporting them twice?

Feedback encouraged if I am missing a more Terraform-idiomatic way to implement this!

Copy link

Choose a reason for hiding this comment

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

SubnetIds are globally unique. This behavior of submitting just subnets is seen elsewhere (ie ALB) and should be familiar to anyone used to using VPC.

Filtering by VPC is often just client-side to narrow down subnet list.

There are exceptions of course, for resources such as security groups that go into a VPC but not into a subnet.

Copy link
Member

Choose a reason for hiding this comment

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

@handlerbot understood, but I don't think that gives us a reason to drift away from the API. They can both live nested under vpc_options if I understand correctly?

Additionally we should remove Optional from both mentioned fields as the user cannot specify them in the config.

Copy link
Member

@radeksimko radeksimko Oct 24, 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 see a way to export schema sub-members as top-level attributes, other than defining them twice and exporting them twice?

I'm not sure I follow - the user can access nested attributes of any TypeList like this:

aws_elasticsearch_domain.vpc_options.0.vpc_id

the 0th index may feel a bit weird, but that's how most other resources do it, so I'd just stick with it. It's something we plan to address in core/HCL eventually.

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 only found one example of the docs specifying a usage like that, and only in passing (cache_nodes in https://www.terraform.io/docs/providers/aws/r/elasticache_cluster.html). I am not personally convinced, but you folks are the bosses, so. :-)

Type: schema.TypeString,
Optional: true,
Computed: true,
},
"elasticsearch_version": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -230,6 +262,21 @@ func resourceAwsElasticSearchDomainCreate(d *schema.ResourceData, meta interface
}
}

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

if len(options) > 1 {
return fmt.Errorf("Only a single vpc_options block is expected")
} else if len(options) == 1 {
if options[0] == nil {
return fmt.Errorf("At least one field is expected inside vpc_options")
}

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

log.Printf("[DEBUG] Creating ElasticSearch domain: %s", input)

// IAM Roles can take some time to propagate if set in AccessPolicies and created in the same terraform
Expand Down Expand Up @@ -289,7 +336,7 @@ func waitForElasticSearchDomainCreation(conn *elasticsearch.ElasticsearchService
return resource.NonRetryableError(err)
}

if !*out.DomainStatus.Processing && out.DomainStatus.Endpoint != nil {
if !*out.DomainStatus.Processing && (out.DomainStatus.Endpoint != nil || out.DomainStatus.Endpoints != nil) {
return nil
}

Expand Down Expand Up @@ -332,9 +379,6 @@ func resourceAwsElasticSearchDomainRead(d *schema.ResourceData, meta interface{}
d.Set("domain_id", ds.DomainId)
d.Set("domain_name", ds.DomainName)
d.Set("elasticsearch_version", ds.ElasticsearchVersion)
if ds.Endpoint != nil {
d.Set("endpoint", *ds.Endpoint)
}

err = d.Set("ebs_options", flattenESEBSOptions(ds.EBSOptions))
if err != nil {
Expand All @@ -349,6 +393,35 @@ func resourceAwsElasticSearchDomainRead(d *schema.ResourceData, meta interface{}
"automated_snapshot_start_hour": *ds.SnapshotOptions.AutomatedSnapshotStartHour,
})
}
if ds.VPCOptions != nil {
err = d.Set("vpc_options", flattenESVPCDerivedInfo(ds.VPCOptions))
if err != nil {
return err
}
err = d.Set("vpc_availability_zones", schema.NewSet(schema.HashString, flattenStringList(ds.VPCOptions.AvailabilityZones)))
if err != nil {
return err
}
err = d.Set("vpc_id", *ds.VPCOptions.VPCId)
if err != nil {
return err
}
endpoints := pointersMapToStringList(ds.Endpoints)
err = d.Set("endpoint", endpoints["vpc"])
if err != nil {
return err
}
if ds.Endpoint != nil {
return fmt.Errorf("%q: Elasticsearch domain in VPC expected to have null Endpoint value", d.Id())
}
} else {
if ds.Endpoint != nil {
d.Set("endpoint", *ds.Endpoint)
}
if ds.Endpoints != nil {
return fmt.Errorf("%q: Elasticsearch domain not in VPC expected to have null Endpoints value", d.Id())
}
}

d.Set("arn", ds.ARN)

Expand Down Expand Up @@ -431,6 +504,17 @@ func resourceAwsElasticSearchDomainUpdate(d *schema.ResourceData, meta interface
}
}

if d.HasChange("vpc_options") {
options := d.Get("vpc_options").([]interface{})

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

Choose a reason for hiding this comment

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

We can do the same validation just by adding MaxItems: 1 to the field in the schema which is a bit more idiomatic and more importantly the user will get the validation error during plan, rather than apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I was cribbing/copying/keeping style with the other optional config sections in this provider, I didn't know that MaxItems was even an option! :-/

} else if len(options) == 1 {
s := options[0].(map[string]interface{})
input.VPCOptions = expandESVPCOptions(s)
}
}

_, err := conn.UpdateElasticsearchDomainConfig(&input)
if err != nil {
return err
Expand Down
64 changes: 64 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 @@ -145,6 +145,25 @@ func TestAccAWSElasticSearchDomain_complex(t *testing.T) {
})
}

func TestAccAWSElasticSearchDomain_vpc(t *testing.T) {
var domain elasticsearch.ElasticsearchDomainStatus
ri := acctest.RandInt()

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckESDomainDestroy,
Steps: []resource.TestStep{
{
Config: testAccESDomainConfig_vpc(ri),
Check: resource.ComposeTestCheckFunc(
testAccCheckESDomainExists("aws_elasticsearch_domain.example", &domain),
),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another step, where we would update the VPC configuration, so that we ensure the update works as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ninir Yeah, I can do that, once I sort the larger IAM problem for the test (update re: that coming next).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disregard "larger IAM problem", I figured it out. 🎉

},
})
}

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

Expand Down Expand Up @@ -448,3 +467,48 @@ resource "aws_elasticsearch_domain" "example" {
}
`, randInt)
}

func testAccESDomainConfig_vpc(randInt int) string {
return fmt.Sprintf(`
data "aws_availability_zones" "available" {
state = "available"
}

resource "aws_default_vpc" "default" {}

resource "aws_default_subnet" "first" {
availability_zone = "${data.aws_availability_zones.available.names[0]}"
}

resource "aws_default_subnet" "second" {
availability_zone = "${data.aws_availability_zones.available.names[1]}"
}

resource "aws_security_group" "first" {
vpc_id = "${aws_default_vpc.default.id}"
}
Copy link
Member

Choose a reason for hiding this comment

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

To avoid sharing any resources and allow running multiple tests in parallel do you mind building custom VPC & subnets here, instead of creating default ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done & re-running this test as I write this. It hasn't completed yet, but it brought up the VPC and subnets and SGs, and the ES domain is creating, so I have very high confidence the tear-down will work. :-)


resource "aws_security_group" "second" {
vpc_id = "${aws_default_vpc.default.id}"
}

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

ebs_options {
ebs_enabled = false
}

cluster_config {
instance_count = 2
zone_awareness_enabled = true
instance_type = "r3.large.elasticsearch"
}

vpc_options {
security_group_ids = ["${aws_security_group.first.id}", "${aws_security_group.second.id}"]
subnet_ids = ["${aws_default_subnet.first.id}", "${aws_default_subnet.second.id}"]
}
}
`, randInt)
}
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 @@ -1014,6 +1014,32 @@ func expandESEBSOptions(m map[string]interface{}) *elasticsearch.EBSOptions {
return &options
}

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

if o.SecurityGroupIds != nil {
m["security_group_ids"] = schema.NewSet(schema.HashString, flattenStringList(o.SecurityGroupIds))
}
if o.SubnetIds != nil {
m["subnet_ids"] = schema.NewSet(schema.HashString, flattenStringList(o.SubnetIds))
}

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

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

if v, ok := m["security_group_ids"]; ok {
options.SecurityGroupIds = expandStringList(v.(*schema.Set).List())
}
if v, ok := m["subnet_ids"]; ok {
options.SubnetIds = expandStringList(v.(*schema.Set).List())
}

return &options
}

func expandConfigRecordingGroup(configured []interface{}) *configservice.RecordingGroup {
recordingGroup := configservice.RecordingGroup{}
group := configured[0].(map[string]interface{})
Expand Down
12 changes: 12 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 @@ -59,6 +59,7 @@ The following arguments are supported:
* `ebs_options` - (Optional) EBS related options, may be required based on chosen [instance size](https://aws.amazon.com/elasticsearch-service/pricing/). 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.
* `elasticsearch_version` - (Optional) The version of ElasticSearch to deploy. Defaults to `1.5`
* `tags` - (Optional) A mapping of tags to assign to the resource

Expand All @@ -80,6 +81,15 @@ The following arguments are supported:
* `dedicated_master_count` - (Optional) Number of dedicated master nodes in the cluster
* `zone_awareness_enabled` - (Optional) Indicates whether zone awareness is enabled.

**vpc_options** supports the following attributes:

AWS documentation: [VPC Support for Amazon Elasticsearch Service Domains](https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-vpc.html)

* `security_group_ids` - (Optional) List of VPC Security Group IDs to be applied to the Elasticsearch domain endpoints. If omitted, the default Security Group for the VPC will be used.
* `subnet_ids` - (Required) List of VPC Subnet IDs for the Elasticsearch domain endpoints to be created in.

Security Groups and Subnets referenced in these attributes must all be within the same VPC; this determines what VPC the endpoints are created in.

**snapshot_options** supports the following attribute:

* `automated_snapshot_start_hour` - (Required) Hour during which the service takes an automated daily
Expand All @@ -93,6 +103,8 @@ The following attributes are exported:
* `arn` - Amazon Resource Name (ARN) of the domain.
* `domain_id` - Unique identifier for the domain.
* `endpoint` - Domain-specific endpoint used to submit index, search, and data upload requests.
* `vpc_availability_zones` - If the domain was created inside a VPC, the names of the availability zones the configured `subnet_ids` were created inside.
* `vpc_id` - If the domain was created inside a VPC, the ID of the VPC.

## Import

Expand Down