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: Future proof new log type values in order to support the new AUDIT_LOGS log type #15218

Merged
merged 2 commits into from
Sep 24, 2020
Merged

resource/aws_elasticsearch_domain: Future proof new log type values in order to support the new AUDIT_LOGS log type #15218

merged 2 commits into from
Sep 24, 2020

Conversation

reedloden
Copy link
Contributor

@reedloden reedloden commented Sep 18, 2020 •
edited
Loading

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 #15217

Release note for CHANGELOG:

ENHANCEMENTS

* resource/aws_elasticsearch_domain: Add `AUDIT_LOGS` as a possible log type [GH-15218]

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSElasticSearchDomain_LogPublishingOptions'

... [unfortunately, don't have a set up testing environment for this]

@reedloden reedloden requested a review from a team September 18, 2020 06:58
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. documentation Introduces or discusses updates to documentation. service/elasticsearch Issues and PRs that pertain to the elasticsearch service. labels Sep 18, 2020
@reedloden
Copy link
Contributor Author

Note this requires aws/aws-sdk-go v1.34.26 to actually work. #15123

@anGie44 anGie44 removed the needs-triage Waiting for first response or review from a maintainer. label Sep 18, 2020
@anGie44
Copy link
Contributor

anGie44 commented Sep 18, 2020 •
edited
Loading

Hi @reedloden, thank you for creating this PR! With the SDK update we'll have to add one additional update in the "log_type" definition w/in the resource's schema as this field has a validation function in place which checks that the provided input value matches an item in the list of possible log types - currently the list of valid types includes the SDK enums:

elasticsearch.LogTypeIndexSlowLogs,
elasticsearch.LogTypeSearchSlowLogs,
elasticsearch.LogTypeEsApplicationLogs,

we could add the new "AuditLogs" enum available in the AWS Go SDK update or better yet replace the explicit string array seen here with elasticsearchdomain.LogType_Values() provided by the AWS Go SDK so that all possible values are included, making long-term maintenance a bit easier.

@anGie44 anGie44 added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 18, 2020
@bflad
Copy link
Contributor

bflad commented Sep 18, 2020

Just merged in the AWS Go SDK update so updating the validation to elasticsearchdomain.LogType_Values() will enable this now. 👍

@reedloden reedloden changed the title docs: resource/aws_elasticsearch_domain: Add AUDIT_LOGS as possible log type resource/aws_elasticsearch_domain: Add AUDIT_LOGS as possible log type Sep 18, 2020
@reedloden
resource/aws_elasticsearch_domain: Replace enum of possible log types…
a4b3fb0 
… with `LogType_Values()`

This enables support for the new `AUDIT_LOGS` log type.

Fixes hashicorp#15217.
@reedloden
Copy link
Contributor Author

@anGie44 My late night grep mad skillz missed that somehow. My apologies! Good call on just changing this to use the LogType_Values() function (and thanks for the SDK update, @bflad). I have updated my commit. However, I don't have a good working AWS test environment to test this change, unfortunately.

@reedloden reedloden changed the title resource/aws_elasticsearch_domain: Add AUDIT_LOGS as possible log type resource/aws_elasticsearch_domain: Future proof new log type values in order to support the new AUDIT_LOGS log type Sep 18, 2020
@anGie44
Copy link
Contributor

anGie44 commented Sep 18, 2020

No worries @reedloden, changes look great! If you don't mind, I can follow-up with a commit to this PR to add acceptance testing around validating the changes to log_type 👍

@reedloden
Copy link
Contributor Author

@anGie44 Sounds good, and many thanks!

@reedloden
Copy link
Contributor Author

TestAccAWSElasticSearchDomain_LogPublishingOptions already tests the INDEX_SLOW_LOGS log type. Would it make sense to just convert that to a variable and loop through all four possible options to ensure they work?

@anGie44
Copy link
Contributor

anGie44 commented Sep 21, 2020 •
edited
Loading

@reedloden, for this attribute it would be best to create a separate test as different configurations are required for the different logging types. in my initial testing, I found that AUDIT_LOGS type required AdvancedSecurityOptions configured while INDEX_SLOW_LOGS, for example, do not.

Update: updating log_type in configurations affected by #5752 (old log_publishing_options will not be removed as expected :/ )

@DrFaust92 DrFaust92 mentioned this pull request Sep 21, 2020
Closed
@ghost ghost added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/L 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 Sep 22, 2020
@anGie44 anGie44 force-pushed the es-audit-logs branch 5 times, most recently from b459cbb to 8f57522 Compare September 23, 2020 18:03
@anGie44 anGie44 force-pushed the es-audit-logs branch 9 times, most recently from e3b422c to 46f5ed0 Compare September 23, 2020 22:13
anGie44
anGie44 approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

Thanks again @reedloden 🚀
Output of acceptance tests:

--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsCreateAndRemove (1692.96s)
--- PASS: TestAccAWSElasticSearchDomain_duplicate (800.39s)
--- PASS: TestAccAWSElasticSearchDomainPolicy_basic (874.70s)
--- PASS: TestAccAWSElasticSearchDomain_basic (1016.79s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_EsApplicationLogs (1051.29s)
--- PASS: TestAccAWSElasticSearchDomain_vpc (1067.70s)
--- PASS: TestAccAWSElasticSearchDomain_v23 (1157.55s)
--- PASS: TestAccAWSElasticSearchDomain_AdvancedSecurityOptions_UserDB (1286.58s)
--- PASS: TestAccAWSElasticSearchDomain_policy (647.51s)
--- PASS: TestAccAWSElasticSearchDomain_AdvancedSecurityOptions_IAM (1552.30s)
--- PASS: TestAccAWSElasticSearchDomain_AdvancedSecurityOptions_Disabled (1652.79s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_AuditLogs (1860.44s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_SearchSlowLogs (1880.79s)
--- PASS: TestAccAWSElasticSearchDomain_LogPublishingOptions_IndexSlowLogs (1930.89s)
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_specify_key (947.75s)
--- PASS: TestAccAWSElasticSearchDomain_encrypt_at_rest_default_key (1096.99s)
--- PASS: TestAccAWSElasticSearchDomain_tags (931.47s)
--- PASS: TestAccAWSElasticSearchDomain_NodeToNodeEncryption (961.64s)
--- PASS: TestAccAWSElasticSearchDomain_RequireHTTPS (2016.41s)
--- PASS: TestAccAWSElasticSearchDomain_complex (2074.74s)
--- PASS: TestAccAWSElasticSearchDomain_WithVolumeType_Missing (1032.62s)
--- PASS: TestAccAWSElasticSearchDomain_CognitoOptionsUpdate (2528.93s)
--- PASS: TestAccAWSElasticSearchDomain_vpc_update (2616.26s)
--- PASS: TestAccAWSElasticSearchDomain_withDedicatedMaster (3121.48s)
--- PASS: TestAccAWSElasticSearchDomain_update (2003.60s)
--- PASS: TestAccAWSElasticSearchDomain_internetToVpcEndpoint (3686.54s)
--- PASS: TestAccAWSElasticSearchDomain_update_version (3329.99s)
--- PASS: TestAccAWSElasticSearchDomain_update_volume_type (3383.02s)
--- PASS: TestAccAWSElasticSearchDomain_ClusterConfig_ZoneAwarenessConfig (5046.46s)
--- PASS: TestAccAWSElasticSearchDomain_warm (5737.54s)

@anGie44 anGie44 added this to the v3.8.0 milestone Sep 24, 2020
@anGie44 anGie44 merged commit 4355419 into hashicorp:master Sep 24, 2020
anGie44 added a commit that referenced this pull request Sep 24, 2020
@anGie44
Update CHANGELOG for #15218
e916738
@reedloden reedloden deleted the es-audit-logs branch September 24, 2020 19:14
@ghost
Copy link

ghost commented Sep 24, 2020

This has been released in version 3.8.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 Oct 25, 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 as resolved and limited conversation to collaborators Oct 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anGie44 anGie44 anGie44 approved these changes

Assignees
No one assigned
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/L 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
v3.8.0
Development

Successfully merging this pull request may close these issues.

resource/aws_elasticsearch_domain: Support audit logging
3 participants
@reedloden @anGie44 @bflad