-
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
Add OpenSearch service #23902
Add OpenSearch service #23902
Conversation
Do we want to publish guidance of use of |
What did you have in mind? Good start would be adding something to the opensearch docs pages. Maybe a note saying "these are the main differences." |
"instance_type": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Default: opensearchservice.OpenSearchPartitionInstanceTypeM3MediumSearch, |
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.
There's been a number of issues raised (#22574, #23922, #21787) where folks expect to be able to create ElasticSearch clusters with OpenSearch instance types and vice versa. I'm about to raise a PR against aws_elasticsearch_domain
to add plan-time validation to improve QOL over there. Just wondering if you want to add the following here?
ValidateFunc: validation.StringInSlice(opensearch.OpenSearchPartitionInstanceType_Values(), false),
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.
wouldn't that mean a new version would need to be released whenever AWS adds support for a new instance type?
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 does, although we effectively get such updates for free due to the use of that Values()
function. Updating the provider's version of the AWS SDK will be enough to pull in the updated list of instance types. Such API calls are very common across the various services in the codebase and mean that we don't have to maintain our own list of values and try to keep them in sync.
But, you're right, there's a tradeoff here; We can 1) Offer users the convenience of plan-time validation so they can avoid the potential of partially applied changes (because other resources may get applied before the OpenSearch domain receives an error back from the API - it does the same validation as we are doing here). or 2) Forego that plan-time validation and instead offer users the ability to specify any value at all there, typos included. The provider generally seems to favour 1. Given the ~ 1 week release cycles of the provider such an approach would seem reasonable to me.
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.
A quick drive-by note.
Yes, we get the up-to-date list of valid instance types by upgrading the AWS Go SDK dependency (and doing a release) but practitioners would then have to upgrade to the newer Terraform AWS Provider release.
Leaving no validation allows older provider versions to be able to specify a newer instance type that passes plan
with a possible runtime error from the underlying AWS API during apply
.
We don't constrain the EC2 Instance's instance_type
argument, for example, even though the AWS Go SDK does provide a list of currently valid instance types.
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.
Ah, interesting. In addition to that note I've been doing some experimenting and it looks like the ES/OpenSearch APIs are sufficiently vague here that adding such validation is actually incorrect/too constraining.
For example, whilst aws es list-elasticsearch-instance-types --elasticsearch-version 7.10
only shows .elasticsearch
instance types, the console does (only!) enable you to specify a .search
instance type and this behaviour goes back at least as far as the 5.5 version. The console's list of available versions changes depending on the version of ES/OpenSearch you select in accordance with https://docs.aws.amazon.com/opensearch-service/latest/developerguide/supported-instance-types.html.
So:
- AWS Console (quite rightly) wants to force you to use the newer instance types
- To get the TF provider to accurately reflect the elasticsearch_version->instance_type mapping, the
ValidateFunc
would somehow have to get access to theelasticsearch_version
from the ResourceData which I don't think it can? So it
- Will only be able to do a partial validation (it would prevent typos only; version+instance type mismatches would still be possible)
- Will constrain the list of valid values in a backward-incompatible manner (would only provide
.elasticsearch
types) - Will need practitioners to upgrade to newer releases of the provider
I'll throw my other PR away now 😄
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.
Nice! Will do.
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.
I think we're in agreement then that the validation should go. We can add something later if we decide that would be helpful.
After changes: % make testacc TESTS=TestAccOpenSearchDomain_ PKG=opensearch
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/opensearch/... -v -count 1 -parallel 20 -run='TestAccOpenSearchDomain_' -timeout 180m
--- PASS: TestAccOpenSearchDomain_AdvancedSecurityOptions_disabled (1520.61s)
--- PASS: TestAccOpenSearchDomain_disappears (1560.68s)
--- PASS: TestAccOpenSearchDomain_Policy_basic (1635.74s)
--- PASS: TestAccOpenSearchDomain_basic (1669.98s)
--- PASS: TestAccOpenSearchDomain_Encryption_atRestDefaultKey (1703.32s)
--- PASS: TestAccOpenSearchDomain_Encryption_nodeToNode (1705.76s)
--- PASS: TestAccOpenSearchDomain_Encryption_atRestSpecifyKey (1821.35s)
--- PASS: TestAccOpenSearchDomain_Policy_ignoreEquivalent (1906.86s)
--- PASS: TestAccOpenSearchDomain_AdvancedSecurityOptions_userDB (1943.03s)
--- PASS: TestAccOpenSearchDomain_VolumeType_missing (1974.23s)
--- PASS: TestAccOpenSearchDomain_tags (2124.77s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_indexSlowLogs (2171.63s)
--- PASS: TestAccOpenSearchDomain_AdvancedSecurityOptions_iam (2309.14s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_auditLogs (2578.41s)
--- PASS: TestAccOpenSearchDomain_duplicate (914.78s)
--- PASS: TestAccOpenSearchDomain_autoTuneOptions (1601.56s)
--- PASS: TestAccOpenSearchDomain_CognitoOptions_update (3180.61s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_applicationLogs (1599.12s)
--- PASS: TestAccOpenSearchDomain_CognitoOptions_createAndRemove (3385.20s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_searchSlowLogs (1911.64s)
--- PASS: TestAccOpenSearchDomain_VPC_basic (2048.74s)
--- PASS: TestAccOpenSearchDomain_complex (3954.60s)
--- PASS: TestAccOpenSearchDomain_versionUpdate (4044.33s)
--- PASS: TestAccOpenSearchDomain_VolumeType_update (4296.07s)
--- PASS: TestAccOpenSearchDomain_requireHTTPS (2363.23s)
--- PASS: TestAccOpenSearchDomain_customEndpoint (2551.75s)
--- PASS: TestAccOpenSearchDomain_Cluster_update (3025.19s)
--- PASS: TestAccOpenSearchDomain_Cluster_dedicatedMaster (5285.62s)
--- PASS: TestAccOpenSearchDomain_VPC_update (3885.27s)
--- PASS: TestAccOpenSearchDomain_VPC_internetToVPCEndpoint (4561.20s)
--- PASS: TestAccOpenSearchDomain_Cluster_warm (5475.57s)
--- PASS: TestAccOpenSearchDomain_Cluster_zoneAwareness (6735.12s)
--- PASS: TestAccOpenSearchDomain_v23 (1248.33s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/opensearch 8558.034s |
This functionality has been released in v4.9.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. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
Closes #20853
Closes #21787
Closes #22574
Output from acceptance testing: