-
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
Update GuardDuty resource to add Kubernetes audit log data source #22859
Conversation
|
||
This `kubernetes_audit_logs` block supports the following: | ||
|
||
* `enable` - (Required) If true, enables [Kubernetes Protection](https://docs.aws.amazon.com/guardduty/latest/ug/kubernetes-protection.html). Defaults to `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.
Does this default to true
because thats what happens with s3_logs
or does this default to true
because thats what AWS default for guard duty is?
I ask because the aws default is changing to false relatively soon (this month, I believe).
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.
Frankly that's a great point. I wrote the code just before they put that notice out.
I don't mind which way it works and I'm quite open to opinions.
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 am in no way a contributor here, but I am looking forward to this contribution so thanks for putting it in!
As for the default, IMO it should follow AWS's default. I think it just makes the cognitive load lower if the default in the provider matches the defaults of what AWS has in place.
That being said I believe the s3_logs default already doesnt match aws's default so it also puts it in a weird position where one component matches and the other doesnt. ¯_(ツ)_/¯
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.
IMHO it's still time to do it right for Kubernetes datasource and so set to false by default as AWS decided to do.
In terms of costs it is a better decision to set it to false by default too. ;)
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.
As already shared, AWS decided on the 02/08 that GuardDuty for EKS Protection will remain off by default : https://aws.amazon.com/about-aws/whats-new/2022/01/amazon-guardduty-elastic-kubernetes-service-clusters/
After about 20 days of trial on a bunch of AWS accounts I see the bills rising rapidly and honestly given the different security means I already use on clusters (Policies), I am not totally convinced of the gain. I think that enabling EKS Protection in Guard Duty should be a conscious decision.
@quintok please reconsider setting the default to 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.
I've confirmed with AWS that Kubernetes protection is enabled by default for new GuardDuty configurations, but for GuardDuty configurations that existed before the introduction of the feature, it is disabled by default to prevent billing surprises. Since we aim to match the defaults from the AWS API, we will leave the defaults for both S3 and Kubernetes protection as enabled.
Does this need additional changes for the aws_organization_guardduty_configuration? There is an |
Anything we can do to move this PR forward? I know Hashi has a huge backlog but hoping to get eyes on this soon? Im not sure what the expected turnaround time for a PR like this should be. Just dont want it to go stale, good work on this! |
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 for the PR, @quintok. I've made a couple updates to make the schema match the AWS API better
--- PASS: TestAccGuardDuty_serial (862.62s)
--- PASS: TestAccGuardDuty_serial/Detector (374.61s)
--- PASS: TestAccGuardDuty_serial/Detector/basic (88.24s)
--- PASS: TestAccGuardDuty_serial/Detector/datasources_s3logs (49.11s)
--- PASS: TestAccGuardDuty_serial/Detector/datasources_kubernetes_audit_logs (40.67s)
--- PASS: TestAccGuardDuty_serial/Detector/datasources_all (75.03s)
--- PASS: TestAccGuardDuty_serial/Detector/tags (57.35s)
--- PASS: TestAccGuardDuty_serial/Detector/datasource_basic (43.72s)
--- PASS: TestAccGuardDuty_serial/Detector/datasource_id (20.48s)
--- PASS: TestAccGuardDuty_serial/Filter (151.25s)
--- PASS: TestAccGuardDuty_serial/Filter/basic (41.43s)
--- PASS: TestAccGuardDuty_serial/Filter/update (36.24s)
--- PASS: TestAccGuardDuty_serial/Filter/tags (52.51s)
--- PASS: TestAccGuardDuty_serial/Filter/disappears (21.07s)
--- PASS: TestAccGuardDuty_serial/InviteAccepter (0.00s)
--- SKIP: TestAccGuardDuty_serial/InviteAccepter/basic (0.00s)
--- PASS: TestAccGuardDuty_serial/ThreatIntelSet (129.49s)
--- PASS: TestAccGuardDuty_serial/ThreatIntelSet/tags (72.92s)
--- PASS: TestAccGuardDuty_serial/ThreatIntelSet/basic (56.57s)
--- PASS: TestAccGuardDuty_serial/IPSet (118.93s)
--- PASS: TestAccGuardDuty_serial/IPSet/basic (52.39s)
--- PASS: TestAccGuardDuty_serial/IPSet/tags (66.54s)
--- PASS: TestAccGuardDuty_serial/OrganizationAdminAccount (0.52s)
--- SKIP: TestAccGuardDuty_serial/OrganizationAdminAccount/basic (0.52s)
--- PASS: TestAccGuardDuty_serial/OrganizationConfiguration (1.53s)
--- SKIP: TestAccGuardDuty_serial/OrganizationConfiguration/basic (0.13s)
--- SKIP: TestAccGuardDuty_serial/OrganizationConfiguration/s3Logs (1.40s)
--- PASS: TestAccGuardDuty_serial/Member (26.01s)
--- PASS: TestAccGuardDuty_serial/Member/basic (26.01s)
--- SKIP: TestAccGuardDuty_serial/Member/inviteOnUpdate (0.00s)
--- SKIP: TestAccGuardDuty_serial/Member/inviteDisassociate (0.00s)
--- SKIP: TestAccGuardDuty_serial/Member/invitationMessage (0.00s)
--- PASS: TestAccGuardDuty_serial/PublishingDestination (60.29s)
--- PASS: TestAccGuardDuty_serial/PublishingDestination/basic (32.61s)
--- PASS: TestAccGuardDuty_serial/PublishingDestination/disappears (27.68s)
This functionality has been released in v4.17.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 #22987.
Output from acceptance testing: