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.

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

Merged
merged 21 commits into from
Mar 30, 2022
Merged

Add OpenSearch service #23902

merged 21 commits into from
Mar 30, 2022

Conversation

YakDriver
Copy link
Member

@YakDriver YakDriver commented Mar 28, 2022

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #20853
Closes #21787
Closes #22574

Output from acceptance testing:

% make testacc TESTS=TestAccOpenSearch PKG=opensearch 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/opensearch/... -v -count 1 -parallel 20 -run='TestAccOpenSearch'  -timeout 180m
--- PASS: TestAccOpenSearchDomain_WithVolumeType_missing (1991.57s)
--- PASS: TestAccOpenSearchDomainSAMLOptions_Update (2116.18s)
--- PASS: TestAccOpenSearchDomainSAMLOptions_Disabled (2132.37s)
--- PASS: TestAccOpenSearchDomain_AutoTuneOptions (2378.68s)
--- PASS: TestAccOpenSearchDomain_nodeToNodeEncryption (2430.16s)
--- PASS: TestAccOpenSearchDomain_tags (2522.18s)
--- PASS: TestAccOpenSearchDomain_disappears (2550.98s)
--- PASS: TestAccOpenSearchDomainDataSource_Data_basic (2577.93s)
--- PASS: TestAccOpenSearchDomain_complex (2881.34s)
--- PASS: TestAccOpenSearchDomain_duplicate (830.88s)
--- PASS: TestAccOpenSearchDomain_AdvancedSecurityOptions_userDB (3061.73s)
--- PASS: TestAccOpenSearchDomain_vpc (3081.83s)
--- PASS: TestAccOpenSearchDomain_update (3732.72s)
--- PASS: TestAccOpenSearchDomain_customEndpoint (3979.54s)
--- PASS: TestAccOpenSearchDomain_internetToVPCEndpoint (4044.85s)
--- PASS: TestAccOpenSearchDomain_requireHTTPS (4068.59s)
--- PASS: TestAccOpenSearchDomainSAMLOptions_basic (1597.47s)
--- PASS: TestAccOpenSearchDomain_VPC_update (4300.98s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_esApplicationLogs (1858.09s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_indexSlowLogs (1629.91s)
--- PASS: TestAccOpenSearchDomain_EncryptAtRestSpecify_key (2121.07s)
--- PASS: TestAccOpenSearchDomainSAMLOptions_disappears_Domain (1631.04s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_searchSlowLogs (2120.66s)
--- PASS: TestAccOpenSearchDomainSAMLOptions_disappears (1659.07s)
--- PASS: TestAccOpenSearchDomain_v23 (2788.70s)
--- PASS: TestAccOpenSearchDomain_basic (5035.70s)
--- PASS: TestAccOpenSearchDomain_AdvancedSecurityOptions_disabled (2056.79s)
--- PASS: TestAccOpenSearchDomain_UpdateVolume_type (5828.40s)
--- PASS: TestAccOpenSearchDomain_AdvancedSecurityOptions_iam (2226.74s)
--- PASS: TestAccOpenSearchDomainPolicy_basic (2042.25s)
--- PASS: TestAccOpenSearchDomain_policyIgnoreEquivalent (1759.29s)
--- PASS: TestAccOpenSearchDomain_Update_version (6087.78s)
--- PASS: TestAccOpenSearchDomain_EncryptAtRestDefault_key (1608.47s)
--- PASS: TestAccOpenSearchDomain_LogPublishingOptions_auditLogs (1844.51s)
--- PASS: TestAccOpenSearchDomain_withDedicatedMaster (4378.54s)
--- PASS: TestAccOpenSearchDomain_policy (2616.41s)
--- PASS: TestAccOpenSearchDomainDataSource_Data_advanced (3031.66s)
--- PASS: TestAccOpenSearchDomain_cognitoOptionsUpdate (3244.39s)
--- PASS: TestAccOpenSearchDomain_cognitoOptionsCreateAndRemove (3341.02s)
--- PASS: TestAccOpenSearchDomain_warm (6060.79s)
--- PASS: TestAccOpenSearchDomain_Cluster_zoneAwareness (6579.97s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/opensearch	10805.894s

@github-actions github-actions bot added client-connections Pertains to the AWS Client and service connections. documentation Introduces or discusses updates to documentation. generators Relates to code generators. provider Pertains to the provider itself, rather than any interaction with AWS. sweeper Pertains to changes to or issues with the sweeper. tags Pertains to resource tagging. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/XL Managed by automation to categorize the size of a PR. labels Mar 28, 2022
@github-actions github-actions bot added github_actions Pull requests that update Github_actions code repository Repository modifications; GitHub Actions, developer docs, issue templates, codeowners, changelog. service/ec2 Issues and PRs that pertain to the ec2 service. labels Mar 28, 2022
@ewbankkit
Copy link
Contributor

Do we want to publish guidance of use of aws_opensearch_domain vs. aws_elasticsearch_domain (and similar)?

@YakDriver YakDriver marked this pull request as draft March 28, 2022 16:50
@github-actions github-actions bot added the service/elasticsearch Issues and PRs that pertain to the elasticsearch service. label Mar 28, 2022
@YakDriver
Copy link
Member Author

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

@YakDriver YakDriver added the service/opensearch Issues and PRs that pertain to the opensearch service. label Mar 28, 2022
@YakDriver YakDriver marked this pull request as ready for review March 29, 2022 14:35
"instance_type": {
Type: schema.TypeString,
Optional: true,
Default: opensearchservice.OpenSearchPartitionInstanceTypeM3MediumSearch,
Copy link
Collaborator

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),

Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Collaborator

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:

  1. AWS Console (quite rightly) wants to force you to use the newer instance types
  2. To get the TF provider to accurately reflect the elasticsearch_version->instance_type mapping, the ValidateFunc would somehow have to get access to the elasticsearch_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 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! Will do.

Copy link
Member Author

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.

@YakDriver
Copy link
Member Author

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

@YakDriver YakDriver added this to the v4.9.0 milestone Mar 30, 2022
@YakDriver YakDriver merged commit 29494dd into main Mar 30, 2022
@YakDriver YakDriver deleted the f-add-opensearch-service branch March 30, 2022 14:05
github-actions bot pushed a commit that referenced this pull request Mar 30, 2022
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

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!

@github-actions
Copy link

github-actions bot commented May 8, 2022

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
client-connections Pertains to the AWS Client and service connections. documentation Introduces or discusses updates to documentation. generators Relates to code generators. github_actions Pull requests that update Github_actions code provider Pertains to the provider itself, rather than any interaction with AWS. repository Repository modifications; GitHub Actions, developer docs, issue templates, codeowners, changelog. service/ec2 Issues and PRs that pertain to the ec2 service. service/elasticsearch Issues and PRs that pertain to the elasticsearch service. service/opensearch Issues and PRs that pertain to the opensearch service. size/XL Managed by automation to categorize the size of a PR. sweeper Pertains to changes to or issues with the sweeper. tags Pertains to resource tagging. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
4 participants