Support VPC configuration of aws_elasticsearch_domain resources. #1958

handlerbot · 2017-10-19T12:14:33Z

For #1949

handlerbot · 2017-10-19T13:34:29Z

This turns out to need a little more work, so relabelled it as [WIP]; I hope to have it finished up during 2017-10-19 US business time.

… tests.

handlerbot · 2017-10-21T00:27:35Z

OK, bugs are fixed, acceptance tests written, this is ready to go!

radeksimko

Hi @handlerbot
thanks for the PR.
I left you some comments in the code, let me know what you think.

radeksimko · 2017-10-21T12:11:07Z

aws/resource_aws_elasticsearch_domain.go

+				Elem:     &schema.Schema{Type: schema.TypeString},
+				Set:      schema.HashString,
+			},
+			"vpc_id": {


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

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!

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.

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

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.

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

radeksimko · 2017-10-21T12:12:21Z

aws/resource_aws_elasticsearch_domain.go

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


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.

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! :-/

radeksimko · 2017-10-21T12:13:48Z

aws/resource_aws_elasticsearch_domain_test.go

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


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?

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

handlerbot · 2017-10-24T22:56:38Z

Next feedback round in, acceptance tests passing.

Ninir

HI @handlerbot

Just added my 2cents there, small nitpicks from my side 😄

Looks very good, can't wait to see this added! 🚀

Ninir · 2017-10-25T09:26:22Z

aws/resource_aws_elasticsearch_domain_test.go

+}
+
+resource "aws_elasticsearch_domain" "example" {
+  domain_name = "tf-test-in-vpc"


Would it be possible to add randomization to this name? 'would help avoid conflicts when running tests :)

@Ninir Sure, I actually had that (name randomization) earlier and then removed it because I dumbly assumed that you could have duplicate ES domain names within your account as long as they were unique within the VPC, but that turned out to be incorrect, so I've got the restoration of the randomness in my pending change.

Ninir · 2017-10-25T09:26:59Z

aws/resource_aws_elasticsearch_domain_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckESDomainExists("aws_elasticsearch_domain.example", &domain),
+				),
+			},


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

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

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

radeksimko · 2017-10-25T10:01:35Z

Thanks, there is one last thing before we can merge this. The attached test is failing:

=== RUN   TestAccAWSElasticSearchDomain_vpc
--- FAIL: TestAccAWSElasticSearchDomain_vpc (61.01s)
	testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:

		* aws_elasticsearch_domain.example: 1 error(s) occurred:

		* aws_elasticsearch_domain.example: ValidationException: Before you can proceed, you must enable a service-linked role to give Amazon ES permissions to access your VPC.
			status code: 400, request id: 9830a6e4-b965-11e7-a4b8-8330fd83b650
FAIL

I assume the config in the test will need an IAM role with the right policy.
Do you mind taking a look into it?

- If creating an ES domain in VPC, create IAM service-linked role if not already existing. - Randomize domain name during basic create/destroy test for ES in VPC. - Add create/update test for ES in VPC.

handlerbot · 2017-10-25T23:00:41Z

Next update in: automatic IAM service-linked role creation + requested test improvements.

This time for sure! 👍

radeksimko

I just modified your code a bit, mainly to make it safe to create multiple VPC-enabled ES domains in parallel.

Also the usual approach we take on IAM eventual consistency is to retry on particular error codes instead of sleeping. It's a little bit more robust and efficient as it only takes as much time as necessary to create the resource successfully.

Thanks for being reactive here! I hope all the changes I made make sense, but feel free to ask if not.

radeksimko · 2017-10-26T10:06:34Z

aws/resource_aws_elasticsearch_domain.go

@@ -155,6 +187,38 @@ func resourceAwsElasticSearchDomainImport(
 	return []*schema.ResourceData{d}, nil
 }

+func createAwsElasticsearchIAMServiceRoleIfMissing(meta interface{}) error {


I first thought this is a bad idea, until I took time and read http://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-vpc.html#es-enabling-slr 😢

I wish there was an ES service method to do this... but that's not the reality, so good call 👍

handlerbot · 2017-10-26T19:04:05Z

@radeksimko Thank you for the review and the merge. All of your final modifications make sense, thank you for doing them and for making the code more consistent and idiomatic.

And yeah, the dissonance between needing to use the IAM API call to create the service-linked role, and the ES API call to delete it is definitely... weird. VPC support in the API is so new, maybe they'll refine things as they get real-world experience and feedback from it. shrug

Anyway: on to the next thing!

ghost · 2020-04-10T17:47:57Z

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!

Support VPC configuration of aws_elasticsearch_domain resources.

1a2d978

handlerbot mentioned this pull request Oct 19, 2017

VPC access to Elastic Search #1949

Closed

handlerbot changed the title ~~Support VPC configuration of aws_elasticsearch_domain resources.~~ [WIP] Support VPC configuration of aws_elasticsearch_domain resources. Oct 19, 2017

Elasticsearch VPC support: creation/endpoint bugfixes; VPC acceptance…

f57cd53

… tests.

handlerbot changed the title ~~[WIP] Support VPC configuration of aws_elasticsearch_domain resources.~~ Support VPC configuration of aws_elasticsearch_domain resources. Oct 21, 2017

radeksimko reviewed Oct 21, 2017

View reviewed changes

radeksimko added waiting-response Maintainers are waiting on response from community or contributor. enhancement Requests to existing resources that expand the functionality or scope. labels Oct 21, 2017

Elasticsearch VPC support: PR feedback

54d14e1

Ninir mentioned this pull request Oct 24, 2017

Support VPC Options for Elasticsearch #2013

Closed

Elasticsearch VPC support: 2nd PR feedback

4b3dd70

Ninir reviewed Oct 25, 2017

View reviewed changes

Elasticsearch VPC support: 3rd PR feedback.

737d763

- If creating an ES domain in VPC, create IAM service-linked role if not already existing. - Randomize domain name during basic create/destroy test for ES in VPC. - Add create/update test for ES in VPC.

Simplify error handling + retry on missing service role

d4945cc

radeksimko approved these changes Oct 26, 2017

View reviewed changes

radeksimko merged commit 5659df1 into hashicorp:master Oct 26, 2017

handlerbot deleted the elasticsearch-vpc branch October 26, 2017 19:00

ghost locked and limited conversation to collaborators Apr 10, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support VPC configuration of aws_elasticsearch_domain resources. #1958

Support VPC configuration of aws_elasticsearch_domain resources. #1958

handlerbot commented Oct 19, 2017

handlerbot commented Oct 19, 2017

handlerbot commented Oct 21, 2017

radeksimko left a comment

radeksimko Oct 21, 2017

handlerbot Oct 21, 2017 •

edited

Loading

agarstang Oct 24, 2017

radeksimko Oct 24, 2017

radeksimko Oct 24, 2017 •

edited

Loading

handlerbot Oct 24, 2017

radeksimko Oct 21, 2017

handlerbot Oct 21, 2017

radeksimko Oct 21, 2017

handlerbot Oct 21, 2017

handlerbot commented Oct 24, 2017

Ninir left a comment

Ninir Oct 25, 2017

handlerbot Oct 25, 2017

Ninir Oct 25, 2017

handlerbot Oct 25, 2017

handlerbot Oct 25, 2017

radeksimko commented Oct 25, 2017

handlerbot commented Oct 25, 2017

radeksimko left a comment

radeksimko Oct 26, 2017

handlerbot commented Oct 26, 2017

ghost commented Apr 10, 2020

Support VPC configuration of aws_elasticsearch_domain resources. #1958

Support VPC configuration of aws_elasticsearch_domain resources. #1958

Conversation

handlerbot commented Oct 19, 2017

handlerbot commented Oct 19, 2017

handlerbot commented Oct 21, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handlerbot Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko Oct 24, 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

handlerbot commented Oct 24, 2017

Ninir left a 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

radeksimko commented Oct 25, 2017

handlerbot commented Oct 25, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handlerbot commented Oct 26, 2017

ghost commented Apr 10, 2020

handlerbot Oct 21, 2017 •

edited

Loading

radeksimko Oct 24, 2017 •

edited

Loading