-
Notifications
You must be signed in to change notification settings - Fork 389
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
[datadog_security_monitoring_default_rule] Add support for custom tags in security monitoring default rules #2399
Conversation
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 left two small comments.
@@ -110,6 +115,14 @@ resource "datadog_security_monitoring_default_rule" "acceptance_test" { | |||
options { | |||
decrease_criticality_based_on_env = true | |||
} | |||
|
|||
tags = [ | |||
"iaas:aws", |
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.
nit: Could you fix the indentation (same for the next file)?
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.
Tabs vs spaces 🫠 Seems like gofmt does not run automatically here? Fixed
"tags": { | ||
Type: schema.TypeSet, | ||
Optional: true, | ||
Description: "Tags for generated signals.", |
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.
🎯 suggestion: Could you add the fact that: Tags must contain all OOTB rule tags
.
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.
Added to the description. One thing I realized is that anyone upgrading to this version of our terraform provider will have to import all these tags initially, and they'll also have to import them any time we change the tags on a default rule. I just want to confirm that's expected and there's no better way to do 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.
Good point.
You are right, it could be an issue for rule update.
I think we should append to existing ones (by doing a get before doing the put).
…eld to implement custom tags terraform field
if i.cloudWorkloadSecurityApiV2 == nil { | ||
i.cloudWorkloadSecurityApiV2 = datadogV2.NewCloudWorkloadSecurityApi(i.HttpClient) | ||
// GetCSMThreatsApiV2 get instance of CSMThreatsApi | ||
func (i *ApiInstances) GetCSMThreatsApiV2() *datadogV2.CSMThreatsApi { |
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.
The underlying datadog package had a rename from datadogV2.CloudSecurityWorkloadApi to datadogV2.CSMThreatsApi so I did a find and replace to update the name
@clementgbcn As we discussed, I've updated this PR to use the new default tags field from the datadog api and I've also made two necessary changes to accommodate the package version bump which I've explained in comments above. This is ready to be reviewed again |
/merge |
🚂 MergeQueue This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
/merge -c |
This merge request was unqueued If you need support, contact us on Slack #devflow! |
Adds support for custom tags for default rules. Tested updating the tags in staging to confirm it works as expected.
https://datadoghq.atlassian.net/browse/SEC-12971