Support VPC Options for Elasticsearch

[atsushi-ishibashi]

#2010

[atsushi-ishibashi]

In my environment, testacc finished with timeout error...
Acctualy, it took a lot but according to GUI console the domain was completely created and error occured while it was deleting...

make testacc TEST=./aws TESTARGS='-run=TestAccAWSElasticSearchDomain_VPCOptions'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSElasticSearchDomain_VPCOptions -timeout 120m
=== RUN   TestAccAWSElasticSearchDomain_VPCOptions
--- FAIL: TestAccAWSElasticSearchDomain_VPCOptions (3954.14s)
	testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_elasticsearch_domain.example: 1 error(s) occurred:
		
		* aws_elasticsearch_domain.example: "arn:aws:es:us-east-1:hogehoge:domain/tf-test-4577410669025848591": Timeout while waiting for the domain to be created
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	3954.203s
make: *** [testacc] Error 1

[Ninir]

Hi @atsushi-ishibashi

Thanks for the work here :)

I allowed myself to make a review of this work to better help you.
Also, if you encounter create issues, you can still increase the Creation/Update timeout :)

Is this work still in progress? it looks ok at first sight :)

[Ninir]

This implementation implies that we can have more than a single vpc_options section.
What do you think of adding a MaxItems option set to 1, so that we can only define a single VPC configuration?

[atsushi-ishibashi]

I agree with all reviews:bow:
And after increasing timeout, I'll run testacc and report the result. Of course, I won't add the timeout change.

[Ninir]

The else is not needed since the first if returns, meaning we can simplify this to:

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

if len(options) == 1 {
...
}

However, I do think we can simplify this a lot.
Since only a single vpc_options section should be configurable as far as I understand the API, checking more than 2 is unnecessary, hence the MaxItems option.

Then, we can also add this line 256:

option, ok := options[0].(map[string]interface{})

if !ok {
    return errors.New("vpc_options is <nil>")
}

if option != nil {
    //...
}

The same kind of logic applied to the update section would then be required. What do you think? 😄

[Ninir]

Nitpick: missing s to attribute

[Ninir]

Could you fix the indentation here so that it matches the rest of the test configuration?

[Ninir]

Can you add randomization to this name also? it will help avoiding conflicts when running acceptance tests on our side 😄

[Ninir]

@atsushi-ishibashi I'm seing that there was another work for VPC options opened earlier: #1958

Perhaps you could help & contribute on that? :)

[atsushi-ishibashi]

@Ninir Oh, I missed it... Sure👍
Btw, when creating ES domain with vpc options, DescribeElasticsearchDomainOutput. DomainStatus.Endpoint seems to be nil so waitForElasticSearchDomainCreation couldn't return nil..

[radeksimko]

#1958 was just merged and will be part of the next release.

Thanks for the spending the time & investing the effort into this PR though.

[darrenhaken]

Any idea when this will go live?

[s7anley]

@darrenhaken v1.2.0 was released about 2 hours ago.

[ghost]

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!