-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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.
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
r/aws_elasticsearch_domain: Add custom endpoint support #16192
r/aws_elasticsearch_domain: Add custom endpoint support #16192
Conversation
Hi @bflad @DrFaust92 , please can anyone from your team revise this PR?. Thanks in advance. |
Looking forward to see this in the terraform provider! Thank you for working on this :) |
Eagerly awaiting this! |
Looking forward for this feature. |
Thank you @matiaszilli |
Waiting for this feature as well :) |
Looking forward for this to be merged |
Similar to other people's comments... Any update on this? Would love to move away from the pre-generated urls for kibana to a custom one via TF. |
Does anyone know if there's someone from AWS, hashicorp, or otherwise that we can @ mention to get some eyes on this? |
I have mentioned three guys above but they did not answer. |
Any news? |
Hey, I dont work for Hashi so i dont comment on their behalf but please see https://github.com/hashicorp/terraform-provider-aws/blob/main/docs/FAQ.md and https://github.com/hashicorp/terraform-provider-aws/blob/main/ROADMAP.md for currently planned activities. Nudging maintainers is not helpful and just makes their(And Mine) notifications noisy. And ofcourse see the Issue template itself:
|
Are there any plans to have this change released soon ? |
@mrtnfchs no way of knowing unless it's in the roadmap, which this is not. That means it's not a priority for the coming few months unfortunately. One thing that can help is sharing this issue with your team / community and getting folks to 👍 it. I believe the hashi team is more likely to evaluate PRs when they're high up in the 👍 reaction list. This one already is, so I'm sure it wouldn't be hard to push it further towards the top. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a drive by review as I was curious but in general looks good to me (as a non maintainer) at a glance with a couple of nits/questions.
Optional: true, | ||
Default: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reasoning for this change? I don't see any explanation in 5217ecb and it looks like an unnecessary change at a glance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, because it doesn't has to be a necessary attribute to set. For example, you might want to enable a custom endpoint, so you would have to just set custom_endpoint_enabled
and custom_endpoint
attributes and no more that that.
StateFunc: func(v interface{}) string { | ||
// AWS Provider aws_acm_certification.domain_validation_options.resource_record_name | ||
// references (and perhaps others) contain a trailing period, requiring a custom StateFunc | ||
// to trim the string to prevent Route53 API error | ||
value := strings.TrimSuffix(v.(string), ".") | ||
return strings.ToLower(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it has been lifted directly from aws/resource_aws_route53_record.go
. Is it strictly needed here because of a similar API error on the ES API? Sadly https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-configuration-api.html#es-configuration-api-datatypes-domainendpointoptions leaves any validation undocumented.
https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-customendpoint.html suggests you can leave a trailing dot at the end but the console may just strip that.
I was also wondering if there was a common validate function that checks it's a valid FQDN (max length in total and per label etc) but I don't see one elsewhere in the code base or in the plugin SDK.
At the least the comment here probably wants updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I have just lifted that code from aws/resource_aws_route53_record.go
. I was trying to find a common validation function but it doesn't exist, it might be becose the different AWS API endpoints require different validations to be done. I agree with you about the comment update, I will do that. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is generally preferable to leave out these types of StateFunc
where possible. If the API has issues with trailing periods, we can introduce validation to try and catch that early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect @bflad , I will get rid of that validation.
StateFunc: func(v interface{}) string { | ||
// AWS Provider aws_acm_certification.domain_validation_options.resource_record_name | ||
// references (and perhaps others) contain a trailing period, requiring a custom StateFunc | ||
// to trim the string to prevent Route53 API error | ||
value := strings.TrimSuffix(v.(string), ".") | ||
return strings.ToLower(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is generally preferable to leave out these types of StateFunc
where possible. If the API has issues with trailing periods, we can introduce validation to try and catch that early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is looking good, just running the full resource acceptance testing now. Leaving these minor items here in case you are updating the pull request already. Thank you, @matiaszilli
Co-authored-by: Brian Flad <bflad417@gmail.com>
Co-authored-by: Brian Flad <bflad417@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, @matiaszilli 🚀
Output from acceptance testing:
--- PASS: TestAccAWSElasticSearchDomain_AdvancedSecurityOptions_Disabled (1110.37s)
--- PASS: TestAccAWSElasticSearchDomain_AdvancedSecurityOptions_IAM (910.89s)
--- PASS: TestAccAWSElasticSearchDomain_AdvancedSecurityOptions_UserDB (2315.72s)
--- PASS: TestAccAWSElasticSearchDomain_basic (828.97s)
--- PASS: TestAccAWSElasticSearchDomain_ClusterConfig_ZoneAwarenessConfig (6476.66s)
--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsCreateAndRemove (1678.06s)
--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsUpdate (2566.28s)
--- PASS: TestAccAWSElasticSearchDomain_complex (1058.50s)
--- PASS: TestAccAWSElasticSearchDomain_CustomEndpoint (2500.69s)
--- PASS: TestAccAWSElasticSearchDomain_duplicate (712.87s)
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_default_key (993.05s)
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_specify_key (1464.85s)
--- PASS: TestAccAWSElasticSearchDomain_internetToVpcEndpoint (2329.21s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_AuditLogs (1158.98s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_EsApplicationLogs (1519.00s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_IndexSlowLogs (858.37s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_SearchSlowLogs (1081.40s)
--- PASS: TestAccAWSElasticSearchDomain_NodeToNodeEncryption (938.28s)
--- PASS: TestAccAWSElasticSearchDomain_policy (1332.27s)
--- PASS: TestAccAWSElasticSearchDomain_RequireHTTPS (1653.56s)
--- PASS: TestAccAWSElasticSearchDomain_tags (1714.35s)
--- PASS: TestAccAWSElasticSearchDomain_update (2329.18s)
--- PASS: TestAccAWSElasticSearchDomain_update_version (4438.77s)
--- PASS: TestAccAWSElasticSearchDomain_update_volume_type (3179.57s)
--- PASS: TestAccAWSElasticSearchDomain_v23 (1237.71s)
--- PASS: TestAccAWSElasticSearchDomain_vpc (930.36s)
--- PASS: TestAccAWSElasticSearchDomain_vpc_update (3231.96s)
--- PASS: TestAccAWSElasticSearchDomain_warm (6666.88s)
--- PASS: TestAccAWSElasticSearchDomain_withDedicatedMaster (3033.83s)
--- PASS: TestAccAWSElasticSearchDomain_WithVolumeType_Missing (840.98s)
This has been released in version 3.35.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! |
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! |
Add support to define a custom endpoint for your Elasticsearch domain and associate an SSL certificate from AWS ACM.
AWS announcement
The following attributes under
domain_endpoint_options
were added:custom_endpoint_enabled
custom_endpoint
custom_endpoint_certificate_arn
Community Note
Closes #16059
Release note for CHANGELOG:
Output from acceptance testing: