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

Handle single event selector in cloudtrail with non-default read_write_type #5448

Merged
merged 3 commits into from Jun 25, 2020

Conversation

ts-tek
Copy link
Contributor

@ts-tek ts-tek commented Aug 3, 2018

Fixes the residual issue mentioned at end of #3697

Changes proposed in this pull request:

  • Handle defining a single event selector with a non default read_write_type

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSCloudTrail'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSCloudTrail -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCloudTrailServiceAccount_basic
--- PASS: TestAccAWSCloudTrailServiceAccount_basic (15.62s)
=== RUN   TestAccAWSCloudTrail_importBasic
--- PASS: TestAccAWSCloudTrail_importBasic (36.11s)
=== RUN   TestAccAWSCloudTrail
=== RUN   TestAccAWSCloudTrail/Trail
=== RUN   TestAccAWSCloudTrail/Trail/kmsKey
=== RUN   TestAccAWSCloudTrail/Trail/tags
=== RUN   TestAccAWSCloudTrail/Trail/eventSelector
=== RUN   TestAccAWSCloudTrail/Trail/basic
=== RUN   TestAccAWSCloudTrail/Trail/cloudwatch
=== RUN   TestAccAWSCloudTrail/Trail/enableLogging
=== RUN   TestAccAWSCloudTrail/Trail/isMultiRegion
=== RUN   TestAccAWSCloudTrail/Trail/logValidation
--- PASS: TestAccAWSCloudTrail (571.22s)
    --- PASS: TestAccAWSCloudTrail/Trail (571.22s)
        --- PASS: TestAccAWSCloudTrail/Trail/kmsKey (53.50s)
        --- PASS: TestAccAWSCloudTrail/Trail/tags (77.33s)
        --- PASS: TestAccAWSCloudTrail/Trail/eventSelector (91.25s)
        --- PASS: TestAccAWSCloudTrail/Trail/basic (54.42s)
        --- PASS: TestAccAWSCloudTrail/Trail/cloudwatch (79.32s)
        --- PASS: TestAccAWSCloudTrail/Trail/enableLogging (75.47s)
        --- PASS: TestAccAWSCloudTrail/Trail/isMultiRegion (86.50s)
        --- PASS: TestAccAWSCloudTrail/Trail/logValidation (53.43s)
=== RUN   TestAccAWSCloudTrail_include_global_service_events
--- PASS: TestAccAWSCloudTrail_include_global_service_events (33.21s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	656.199s

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Aug 3, 2018
@bflad bflad added bug Addresses a defect in current functionality. service/cloudtrail Issues and PRs that pertain to the cloudtrail service. labels Aug 4, 2018
@tsiq-tek
Copy link

tsiq-tek commented Sep 6, 2018

Just wondering if there was any estimate on when this might get accepted?

@mnikhil-git
Copy link

hi @ts-tek and @tsiq-tek

just curious if there is any news or information as to when this is being planned for merging?

@bflad can you please help check this fix and is good to go ? looks like assignee/review are missing and probably the reason why is it not in the radar..

@aeschright aeschright requested a review from a team June 25, 2019 21:26
@rossmckelvie
Copy link
Contributor

The issue #3697 for this has been closed in preference of this PR, are there plans to continue work on this PR? Happy to test, I apply changes to a terraform project nearly weekly that has a cloudtrail configuration with the default event selectors that are prone to this bug.

@markliederbach
Copy link

Bump for @rossmckelvie. Would really like this solved for the sake of #3697. Else, that issue should be re-opened.

@klauern
Copy link

klauern commented Feb 3, 2020

Related, #11712 would probably be fixed by this as well.

@linuxman79
Copy link

Hi,

I'm also experiencing this issue. I have the following event_selector configuration and it shows constant diffs:

event_selector {
include_management_events = true
read_write_type = "All"
}

Just wondering if anyone has plans to merge the pull request from @ts-tek?

Thanks,
Ben

@lucsmitty
Copy link

Also looking for status on this. Will this also be a terraform version 12+ fix only, or also in terraform 11.x?

@cloudyparts
Copy link

@bflad - this appears to fix multiple open issues #3697 and #11712.

This has been an issue for a long time. Is there anything we can do to push this forward?

@KyMidd
Copy link
Contributor

KyMidd commented May 20, 2020

Agree this would be useful. For others, we worked around this issue with:

lifecycle {
    ignore_changes = [event_selector]
  }

@ghost ghost added the tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. label Jun 25, 2020
Copy link
Member

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

Thank you so much for this @ts-tek 🚀 LGTM

Output from acceptance testing:

--- PASS: TestAccAWSCloudTrail (898.22s)
    --- PASS: TestAccAWSCloudTrail/Trail (898.22s)
        --- PASS: TestAccAWSCloudTrail/Trail/tags (120.93s)
        --- PASS: TestAccAWSCloudTrail/Trail/cloudwatch (110.30s)
        --- PASS: TestAccAWSCloudTrail/Trail/isMultiRegion (115.70s)
        --- SKIP: TestAccAWSCloudTrail/Trail/isOrganization (1.68s)
        --- PASS: TestAccAWSCloudTrail/Trail/kmsKey (52.65s)
        --- PASS: TestAccAWSCloudTrail/Trail/eventSelector (148.11s)
        --- PASS: TestAccAWSCloudTrail/Trail/basic (86.62s)
        --- PASS: TestAccAWSCloudTrail/Trail/enableLogging (120.31s)
        --- PASS: TestAccAWSCloudTrail/Trail/includeGlobalServiceEvents (54.87s)
        --- PASS: TestAccAWSCloudTrail/Trail/logValidation (87.03s)

@bflad bflad merged commit d83a033 into hashicorp:master Jun 25, 2020
bflad added a commit that referenced this pull request Jun 25, 2020
c4po added a commit to c4po/terraform-provider-aws that referenced this pull request Jun 25, 2020
* origin/master: (59 commits)
  Update CHANGELOG for hashicorp#13935
  resource/aws_batch_compute_environment: Remove resource from Terraform state when not found instead of returning error (hashicorp#13935)
  resource/aws_dynamodb_table: Return error instead of panic on empty CreateTable response (hashicorp#13925)
  Update CHANGELOG for hashicorp#13918
  New Data Source: aws_efs_access_points (hashicorp#13918)
  tests/resource/aws_instance: Ensure sweeper has dependencies on resources that manage EC2 Instances (hashicorp#13917)
  Update CHANGELOG for hashicorp#13937
  Update CHANGELOG for hashicorp#5448
  resource/aws_cloudtrail: Handle single event selector in cloudtrail with non-default read_write_type (hashicorp#5448)
  Update CHANGELOG for hashicorp#13892
  correct retry message to match in error handling
  correct import resource name
  accept empty string in volume_type validation
  Update CHANGELOG for hashicorp#4855
  resource/aws_batch_compute_environment: Support fully optional desired_vcpus and wait for updates
  add retry error handling for SLR
  remove unused WebACL resource name in disappears test
  reference current AWS partition in iam role policy stmt
  remove duplicated disappears test step
  Update CHANGELOG for hashicorp#13926
  ...
@ghost
Copy link

ghost commented Jun 26, 2020

This has been released in version 2.68.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!

@rossmckelvie
Copy link
Contributor

Unfortunately this does not seem to fix #3697 for me, which was closed in preference of this PR. workaround must remain even with 2.68.0 installed:

  # These are defaults, and terraform will always think it needs to change https://github.com/terraform-providers/terraform-provider-aws/pull/5448
  event_selector {
    read_write_type           = "All"
    include_management_events = true
  }
  # TODO: remove lifecycle once above linked issue is resolved. lifecycle is a temporary workaround
  lifecycle {
    ignore_changes = [event_selector]
  }

bflad added a commit that referenced this pull request Jul 1, 2020
…ll event_selector configuration blocks

Reference: #5448

Forgot to commit this when merging the referenced PR.

Output from acceptance testing:

```
        --- PASS: TestAccAWSCloudTrail/Trail/eventSelector (148.09s)
```
@KyMidd
Copy link
Contributor

KyMidd commented Jul 14, 2020

Confirm this doesn't fix for us either - still seeing changes on each run for event selector.

@ghost
Copy link

ghost commented Jul 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 and limited conversation to collaborators Jul 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/cloudtrail Issues and PRs that pertain to the cloudtrail 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