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

Jump to bottom

resource/aws_elasticsearch_domain DomainEndpointOptions #10430

Merged
merged 17 commits into from
Jan 29, 2020
Merged

resource/aws_elasticsearch_domain DomainEndpointOptions #10430

merged 17 commits into from
Jan 29, 2020

Conversation

barrytam20
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #10424

Release note for CHANGELOG:

resource/aws_elasticsearch_domain: Add `require_https` argument

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

@barrytam20 barrytam20 requested a review from a team October 9, 2019 02:36
@ghost ghost added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/ecs Issues and PRs that pertain to the ecs service. service/elasticsearch Issues and PRs that pertain to the elasticsearch service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 9, 2019
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Oct 9, 2019
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Oct 9, 2019
@barrytam20 barrytam20 changed the title [WIP] resource/aws_elasticsearch_domain require_https resource/aws_elasticsearch_domain require_https Oct 12, 2019
@au78
Copy link

au78 commented Nov 8, 2019

@barrytam20 - do you have an update on when this will completed?

@barrytam20
Copy link
Contributor Author

@au78 i think the code should be ready for review, im working on running the tests.

@barrytam20
Copy link
Contributor Author

im getting the following error for a few of my tests, ill have to investigate whats going on

--- FAIL: TestAccAWSElasticSearchDomain_internetToVpcEndpoint (1464.00s)
    testing.go:615: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=13) "require_https": (string) (len=5) "false"
        }

@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Nov 10, 2019
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Dec 6, 2019
@barrytam20
Copy link
Contributor Author

sorry kind of forgot about this with the holidays and all😅

i ended up including a block for DomainEndpointOptions which includes EnforceHTTPS and TLSSecurityPolicy.

make testacc TESTARGS='-run=TestAccAWSElasticSearchDomain_RequireHTTPS' 
make testacc TESTARGS='-run=TestAccAWSElasticSearchDomain_RequireHTTPS'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSElasticSearchDomain_RequireHTTPS -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSElasticSearchDomain_RequireHTTPS
=== PAUSE TestAccAWSElasticSearchDomain_RequireHTTPS
=== CONT  TestAccAWSElasticSearchDomain_RequireHTTPS
--- PASS: TestAccAWSElasticSearchDomain_RequireHTTPS (603.85s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       605.096s
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap      0.366s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags 0.294s [no tests to run]

@au78
Copy link

au78 commented Dec 6, 2019

thank you @barrytam20 ... it would really help if you can get this released by next week, please.

@barrytam20
Copy link
Contributor Author

@au78 that is really not up to me :) i can see that this PR is moving up the list when sorted by 👍 but i dont think any maintainers have reviewed this just yet

@au78
Copy link

au78 commented Dec 6, 2019

np... see whatever best you can do to move it up the ladder... really waiting for this one so that we can start adopting it without much fuss... ;)

@barrytam20 barrytam20 changed the title resource/aws_elasticsearch_domain require_https resource/aws_elasticsearch_domain DomainEndpointOptions Dec 9, 2019
@khurdz
Copy link

khurdz commented Jan 2, 2020

Hi and best wishes :)
Any idea when this PR would be reviewed and merged please ?

@barrytam20
Copy link
Contributor Author

@bflad is there anything i can do to help get this one reviewed?

@krismassey
Copy link

Hi all, any potential for this to get released? I could really use this feature at the moment

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed service/ecs Issues and PRs that pertain to the ecs service. labels Jan 24, 2020
@bflad bflad self-assigned this Jan 24, 2020
bflad
bflad suggested changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @barrytam20 👋 Thank you for submitting this, it was off to a really good start. Please see the below feedback items and reach out if you have any questions or do not have time to implement them.

aws/resource_aws_elasticsearch_domain.go Outdated
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"require_https": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for being pedantic here, but since the Elasticsearch API Reference denotes this parameter as EnforceHTTPS, we should use similar naming so this matches the AWS CLI, CloudFormation, etc and can better match potential automatic code generation in the future:

Suggested change
"require_https": {
"enforce_https": {

aws/resource_aws_elasticsearch_domain.go Outdated
"tls_security_policy": {
Type: schema.TypeString,
Optional: true,
Default: "Policy-Min-TLS-1-2-2019-07",
Copy link
Contributor

Choose a reason for hiding this comment

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

The Elasticsearch API Reference denotes that Policy-Min-TLS-1-0-2019-07 is actually the default here (😢 from a security perspective!). We try to not go against API defaults, so in similar situations we have left it up to the API to hopefully update to better defaults in the future and not present operators with a potential difference with existing configurations. These also may continually change over time so we do not constantly want to be playing catch up with API default value. Setting Computed: true instead of setting Default is likely the best path forward here.

We can also introduce plan-time validation for this argument via ValidateFunc. 👍

Suggested change
Default: "Policy-Min-TLS-1-2-2019-07",
Computed: true,
ValidateFunc: validation.StringInSlice([]string{
elasticsearch.TLSSecurityPolicyPolicyMinTls10201907,
elasticsearch.TLSSecurityPolicyPolicyMinTls12201907,
}, false),

aws/resource_aws_elasticsearch_domain.go Outdated
Comment on lines 465 to 483
if v, ok := d.GetOk("domain_endpoint_options"); ok {
options := v.([]interface{})

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

s := options[0].(map[string]interface{})
domainOptions := elasticsearch.DomainEndpointOptions{}
if requireHTTPS, ok := s["require_https"]; ok {
domainOptions.EnforceHTTPS = aws.Bool(requireHTTPS.(bool))
}
if tls, ok := s["tls_security_policy"]; ok {
domainOptions.TLSSecurityPolicy = aws.String(tls.(string))
}
input.SetDomainEndpointOptions(&domainOptions)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This type of logic can usually be simplified by creating an "expand" function (similar to expandESCognitoOptions below): 👍

	if v, ok := d.GetOk("domain_endpoint_options"); ok {
		input.DomainEndpointOptions = expandESDomainEndpointOptions(v.([]interface{}))
	}

// ...

func expandESDomainEndpointOptions(l []interface{}) *elasticsearch.DomainEndpointOptions {
	if len(l) == 0 || l[0] == nil {
		return nil
	}

	m := l[0].(map[string]interface{})
	domainEndpointOptions := &elasticsearch.DomainEndpointOptions{}

	if v, ok := m["enforce_https"].(bool); ok {
		domainEndpointOptions.EnforceHTTPS = aws.Bool(v)
	}

	if v, ok := m["tls_security_policy"].(string); ok {
		domainEndpointOptions.TLSSecurityPolicy = aws.String(v)
	}

	return domainEndpointOptions
}

aws/resource_aws_elasticsearch_domain.go Outdated
Comment on lines 684 to 689
if ds.DomainEndpointOptions != nil {
m := make(map[string]interface{})
m["require_https"] = ds.DomainEndpointOptions.EnforceHTTPS
m["tls_security_policy"] = ds.DomainEndpointOptions.TLSSecurityPolicy
d.Set("domain_endpoint_options", m)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar but opposite concept here with "flatten" functions 👍 Also, when using d.Set() with aggregate types (TypeList, TypeSet, TypeMap), we should perform error checking to prevent issues where the code is not properly able to set the Terraform state.

	if err := d.Set("domain_endpoint_options", flattenESDomainEndpointOptions(ds.DomainEndpointOptions)); err != nil {
		return fmt.Errorf("error setting domain_endpoint_options: %s", err)
	}

// ...

func flattenESDomainEndpointOptions(domainEndpointOptions *elasticsearch.DomainEndpointOptions) []interface{} {
	if domainEndpointOptions == nil {
		return nil
	}

	m := map[string]interface{}{
		"enforce_https":       aws.BoolValue(domainEndpointOptions.EnforceHTTPS),
		"tls_security_policy": aws.StringValue(domainEndpointOptions.TLSSecurityPolicy),
	}

	return []interface{}{m}
}

aws/resource_aws_elasticsearch_domain.go Outdated
Comment on lines 733 to 737
if d.HasChange("require_https") {
input.SetDomainEndpointOptions(&elasticsearch.DomainEndpointOptions{
EnforceHTTPS: aws.Bool(d.Get("require_https").(bool)),
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an older iteration of this feature request 😄

aws/resource_aws_elasticsearch_domain.go Outdated
Comment on lines 739 to 753
if d.HasChange("domain_endpoint_options") {
options := d.Get("domain_endpoint_options").([]interface{})

if len(options) == 1 {
s := options[0].(map[string]interface{})
domainOptions := elasticsearch.DomainEndpointOptions{}
if requireHTTPS, ok := s["require_https"]; ok {
domainOptions.EnforceHTTPS = aws.Bool(requireHTTPS.(bool))
}
if tls, ok := s["tls_security_policy"]; ok {
domainOptions.TLSSecurityPolicy = aws.String(tls.(string))
}
input.SetDomainEndpointOptions(&domainOptions)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can re-use the above mentioned "expand" function here:

	if d.HasChange("domain_endpoint_options") {
		input.DomainEndpointOptions = expandESDomainEndpointOptions(d.Get("domain_endpoint_options").([]interface{}))
	}

aws/resource_aws_elasticsearch_domain_test.go Outdated
Comment on lines 104 to 106
resource.TestCheckResourceAttr(
"aws_elasticsearch_domain.example", "elasticsearch_version", "1.5"),
resource.TestMatchResourceAttr("aws_elasticsearch_domain.example", "kibana_endpoint", regexp.MustCompile(".*es.amazonaws.com/_plugin/kibana/")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: These checks look irrelevant to the testing of domain endpoint options and can be removed. 👍

aws/resource_aws_elasticsearch_domain_test.go
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckESDomainDestroy,
Steps: []resource.TestStep{
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include two additional TestStep that verify that imports and in-place updates work as expected with this new functionality, e.g.

		Steps: []resource.TestStep{
			{
				Config: testAccESDomainConfig_DomainEndpointOptions(ri, true, "Policy-Min-TLS-1-0-2019-07"),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckESDomainExists("aws_elasticsearch_domain.example", &domain),
					testAccCheckESDomainEndpointOptions(true, "Policy-Min-TLS-1-0-2019-07", &domain),
				),
			},
			{
				ResourceName:      "aws_elasticsearch_domain.example",
				ImportState:       true,
				ImportStateId:     resourceId,
				ImportStateVerify: true,
			},
			{
				Config: testAccESDomainConfig_DomainEndpointOptions(ri, true, "Policy-Min-TLS-1-2-2019-07"),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckESDomainExists("aws_elasticsearch_domain.example", &domain),
					testAccCheckESDomainEndpointOptions(true, "Policy-Min-TLS-1-2-2019-07", &domain),
				),
			},
		},

// ...

func testAccESDomainConfig_DomainEndpointOptions(randInt int, enforceHttps bool, tlsSecurityPolicy string) string {
	return fmt.Sprintf(`
resource "aws_elasticsearch_domain" "example" {
  domain_name = "tf-test-%[1]d"

  domain_endpoint_options {
    enforce_https       = %[2]t
    tls_security_policy = %[3]q
  }

  ebs_options {
    ebs_enabled = true
    volume_size = 10
  }
}
`, randInt, enforceHttps, tlsSecurityPolicy)
}

website/docs/r/elasticsearch_domain.html.markdown Outdated
@@ -240,6 +241,11 @@ The following arguments are supported:
* `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.

**domain_endpoint_options** supports the following attributes:

* `require_https` - (Required) wether or not to require HTTPS
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo 😎

Suggested change
* `require_https` - (Required) wether or not to require HTTPS
* `enforce_https` - (Required) Whether or not to require HTTPS

website/docs/r/elasticsearch_domain.html.markdown Outdated
**domain_endpoint_options** supports the following attributes:

* `require_https` - (Required) wether or not to require HTTPS
* `tls_security_policy` - (Optional) the TLS security policy that needs to be applied to the HTTPS endpoint. Defaults to `Policy-Min-TLS-1-2-2019-07`
Copy link
Contributor

Choose a reason for hiding this comment

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

We would prefer not to choose a default in this situation, but should still mention the valid values.

Suggested change
* `tls_security_policy` - (Optional) the TLS security policy that needs to be applied to the HTTPS endpoint. Defaults to `Policy-Min-TLS-1-2-2019-07`
* `tls_security_policy` - (Optional) The name of the TLS security policy that needs to be applied to the HTTPS endpoint. Valid values: `Policy-Min-TLS-1-0-2019-07` and `Policy-Min-TLS-1-2-2019-07`. Terraform will only perform drift detection if a configuration value is provided.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 24, 2020
@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 27, 2020
@barrytam20
Copy link
Contributor Author

thanks @bflad!

ive implemented the changes you requested, and re-ran the tests

make testacc TESTARGS='-run=TestAccAWSElasticSearchDomain_RequireHTTPS'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSElasticSearchDomain_RequireHTTPS -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSElasticSearchDomain_RequireHTTPS
=== PAUSE TestAccAWSElasticSearchDomain_RequireHTTPS
=== CONT  TestAccAWSElasticSearchDomain_RequireHTTPS
--- PASS: TestAccAWSElasticSearchDomain_RequireHTTPS (1166.52s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       1167.762s
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap      0.247s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags 0.134s [no tests to run

@bflad bflad self-requested a review January 28, 2020 21:03
@bflad bflad self-assigned this Jan 28, 2020
bflad
bflad approved these changes Jan 29, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks for those updates, @barrytam20, looks good minus one little documentation thing noted below. 🚀

Output from acceptance testing:

--- PASS: TestAccAWSElasticSearchDomain_duplicate (544.63s)
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_specify_key (681.50s)
--- PASS: TestAccAWSElasticSearchDomain_v23 (712.83s)
--- PASS: TestAccAWSElasticSearchDomain_NodeToNodeEncryption (906.35s)
--- PASS: TestAccAWSElasticSearchDomain_complex (1003.67s)
--- PASS: TestAccAWSElasticSearchDomainPolicy_basic (1044.35s)
--- PASS: TestAccAWSElasticSearchDomain_tags (1069.02s)
--- PASS: TestAccAWSElasticSearchDomain_policy (1187.09s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions (1222.33s)
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_default_key (1366.06s)
--- PASS: TestAccAWSElasticSearchDomain_vpc (1495.79s)
--- PASS: TestAccAWSElasticSearchDomain_RequireHTTPS (1505.39s)
--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsUpdate (1690.98s)
--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsCreateAndRemove (1755.31s)
--- PASS: TestAccAWSElasticSearchDomain_basic (1907.01s)
--- PASS: TestAccAWSElasticSearchDomain_vpc_update (1910.87s)
--- PASS: TestAccAWSElasticSearchDomain_update (2195.83s)
--- PASS: TestAccAWSElasticSearchDomain_internetToVpcEndpoint (2547.44s)
--- PASS: TestAccAWSElasticSearchDomain_update_volume_type (2685.22s)
--- PASS: TestAccAWSElasticSearchDomain_update_version (3011.87s)
--- PASS: TestAccAWSElasticSearchDomain_withDedicatedMaster (3977.83s)
--- PASS: TestAccAWSElasticSearchDomain_ClusterConfig_ZoneAwarenessConfig (5310.68s)

website/docs/r/elasticsearch_domain.html.markdown
@@ -224,6 +224,7 @@ The following arguments are supported:
* `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)).
* `log_publishing_options` - (Optional) Options for publishing slow logs to CloudWatch Logs.
* `elasticsearch_version` - (Optional) The version of Elasticsearch to deploy. Defaults to `1.5`
* `enforce_httpsnt_options` - (Optional) Domain endpoint HTTP(S) related options. See below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this fix got transposed between here and line 246 -- will fix on merge. 👍

@bflad bflad merged commit 1556c29 into hashicorp:master Jan 29, 2020
@bflad bflad added this to the v2.47.0 milestone Jan 29, 2020
bflad added a commit that referenced this pull request Jan 29, 2020
@bflad
Update CHANGELOG for #10430
d1da1e7
@ghost
Copy link

ghost commented Jan 30, 2020

This has been released in version 2.47.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Mar 27, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@venky999 venky999 venky999 requested changes

@bflad bflad bflad approved these changes

@sunil-apty sunil-apty sunil-apty approved these changes

Assignees

@bflad bflad

Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/elasticsearch Issues and PRs that pertain to the elasticsearch service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v2.47.0
Development

Successfully merging this pull request may close these issues.

Support "Require https" on elasticsearch
8 participants
@barrytam20 @au78 @hExPY @sunil-apty @khurdz @krismassey @bflad @venky999