[SA-535]Add support for security monitoring rules #763

greentruff · 2020-11-24T16:09:56Z

Add support for Security Monitoring public API.
See: https://docs.datadoghq.com/api/v2/security-monitoring/

Closes #509

greentruff · 2020-11-25T18:01:57Z

datadog/data_source_datadog_security_monitoring_rules.go

+			"default_only_filter": {
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Description: "Limit the search to default rules",
+			},
+			"user_only_filter": {
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Description: "Limit the search to user rules",
+			},


Two different attributes are used for boolean values to work around GetOk only checking if there was truthy value.

Isn't that because you don't check the value returned? GetOk is weird some time, but I'm not sure it's warranted here. AFAICT we only end up with one boolean, which you should be able to achieve with one field.

I updated the resource to have a default value. Unless I am missing something, in this case though it will not be possible to filter out user rules only. Will we get d.GetOk("default_only_filter") == false, false both in the case where the attribute is not present and in the case where the attribute is default_only_filter = false.

(There are discussions about whether we should remove the datasource so leaving it as is for now).

You're right, because you use nil as a possible value, so there are effectively 3 states, not just 2.

greentruff · 2020-11-25T18:03:28Z

datadog/data_source_datadog_security_monitoring_rules.go

+	return nil
+}
+
+func computeSecMonDataSourceRulesId(nameFilter *string, defaultFilter *bool, tagFilter map[string]bool) string {


This is a costly way to get a hash, I am not sure if this matters

datadog/resource_datadog_security_monitoring_rule.go

therve

Looks great! Some minor question/remarks, plus need clarifications around boolean usage. Need to check why CI is red too.

therve · 2020-11-26T08:50:08Z

datadog/data_source_datadog_security_monitoring_rules.go

+			"default_only_filter": {
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Description: "Limit the search to default rules",
+			},
+			"user_only_filter": {
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Description: "Limit the search to user rules",
+			},


Isn't that because you don't check the value returned? GetOk is weird some time, but I'm not sure it's warranted here. AFAICT we only end up with one boolean, which you should be able to achieve with one field.

datadog/data_source_datadog_security_monitoring_rules_test.go

datadog/resource_datadog_security_monitoring_default_rule.go

docs/resources/security_monitoring_rule.md.md

Bit-Doctor

I've made a first pass at custom security rules.
I'll check default rule resource and data source in a bit.

So far looks good with a few nits.

datadog/resource_datadog_security_monitoring_rule.go

datadog/resource_datadog_security_monitoring_rule_test.go

Bit-Doctor · 2020-11-26T10:58:16Z

datadog/resource_datadog_security_monitoring_rule.go

+				Schema: map[string]*schema.Schema{
+					"name": {
+						Type:        schema.TypeString,
+						Optional:    true,


I think name is required when there's more than one case.

Unfortunately I looked and it seems it's not possible to have that checked dynamically except maybe add a ValidateFunc to the schema.
But I might be better to let the API fail. The only drawback is that the user will know it at apply time.

Might be worth adding a note in the description?

There's a trade-off between client-side validation and server-side validation. I think it easier and more maintenable to delegate most of it to the server, but we have to make sure the error message is descriptive and visible.

btoueg · 2020-11-26T12:42:28Z

docs/resources/security_monitoring_default_rule.md

+# Create a resource for each default rule id and enable the rule
+resource "datadog_security_monitoring_default_rule" "attack_rule" {
+    for_each = toset(data.datadog_security_monitoring_rules.rules.rule_ids)
+    rule_id = each.key
+    enabled = true
+}


IIUC this is the motivation behind datadog_security_rules datasource. It is a peculiar use of datasources AFAICT. For instance, how does it behave if you create datasources that overlaps? Say you have attack_default_rules and specific_framework rules. A rule could belong to both, and would result in multiple datadog_security_monitoring_default_rule. My guess is that in this case, last write wins. but with lots of subtleties. Are we comfortable to recommend such usage?

I would have recommended dealing with default rules with a prior import.

IIUC your edge case is something like this ?

resource "datadog_security_monitoring_default_rule" "rule1" { rule_id = "SAMEID" enabled = true } resource "datadog_security_monitoring_default_rule" "rule2" { rule_id = "SAMEID" enabled = false }

I'm actually unsure what whould Terraform do here, It will probably conflict on the resource ID depending on how you build it.

I am asking @NicolasLevaique what he thinks about this as well

Also I just thought of a resource in the AWS provider that has a similar behavior, with no real create/delete.
So at least this behavior exist.

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/default_vpc
https://github.com/hashicorp/terraform-provider-aws/blob/master/aws/resource_aws_default_vpc.go

Might be worth checking if they handle this edge case with two resource with different params.

Also maybe a bit late but there's setintersection, setunion and setsubtract functions that could have helped user with that issue.

datadog/data_source_datadog_security_monitoring_rules.go

datadog/resource_datadog_security_monitoring_default_rule.go

Make all options required

Fix missing/incorrect fields

- tests are broken

- Remove Exists on default rule, the underlying resource should always exists - Rename file and fix indent

- Fix window management for datasource

- Stable ID for datasource - Only check that rules have been deleted

greentruff · 2020-12-02T17:06:55Z

@therve Can you take another look? After discussing with @NicolasLevaique, I changed the default rule so that it needs to be imported like other resources. I kept the datasource though since it can be useful and I don't know how to test the default rule without it.
There is still the question of the default_only_filter/user_only_filter in the datasource: in this case there are three valid states, no filter or filter by user/default rules. Merging the attributes would mean that we would have to always return either default or user rules. Do you think it is ok to leave both? The alternative is to change the format to something like

filter {
"is_default_rule": true,
}

but this would not be consistent with other datasources in the provider.

therve · 2020-12-02T19:31:41Z

/azp run

azure-pipelines · 2020-12-02T19:31:51Z

Azure Pipelines successfully started running 1 pipeline(s).

therve · 2020-12-03T08:38:01Z

/azp run

azure-pipelines · 2020-12-03T08:38:14Z

Azure Pipelines successfully started running 1 pipeline(s).

therve

A few minor comments, otherwise almost there.

therve · 2020-12-03T10:35:03Z

datadog/resource_datadog_security_monitoring_default_rule.go

+
+	if shouldUpdate {
+		_, _, err := datadogClientV2.SecurityMonitoringApi.UpdateSecurityMonitoringRule(authV2, ruleId).Body(*ruleUpdate).Execute()
+		if err != nil {


You should inline that if with the previous command.

therve · 2020-12-03T10:36:46Z

datadog/resource_datadog_security_monitoring_rule.go

+
+func resourceDatadogSecurityMonitoringRule() *schema.Resource {
+	return &schema.Resource{
+		Exists: resourceDatadogSecurityMonitoringRuleExists,


You should remove the exists, and check for 404 in read instead (and set the id to "" when you get a 404)

Done in the last commit, can you double check how I did it though? I didn't find any other resources which omit exists.

therve · 2020-12-03T10:39:44Z

datadog/resource_datadog_security_monitoring_rule.go

+
+	_, err := datadogClientV2.SecurityMonitoringApi.DeleteSecurityMonitoringRule(authV2, d.Id()).Execute()
+
+	if err != nil {


You can inline that condition too.

therve · 2020-12-03T10:40:43Z

docs/resources/security_monitoring_default_rule.md

+```
+
+```
+terraform import datadog_security_monitoring_default_rule.adefaultrule


You're missing an ID here.

- Remove exists for secmon rule resource - Inline if err != nil checks - Add missing id in documentation

therve · 2020-12-03T13:48:54Z

datadog/resource_datadog_security_monitoring_rule.go

+	if err != nil {
+		if httpResponse != nil && httpResponse.StatusCode == 404 {
+			d.SetId("")
+			return errors.New("security monitoring rule does not exist")


Almost that, just return nil in that case instead of an error. Thanks.

greentruff commented Nov 25, 2020

View reviewed changes

greentruff marked this pull request as ready for review November 25, 2020 18:07

greentruff requested a review from a team as a code owner November 25, 2020 18:07

greentruff force-pushed the atrufanow/add-signal branch from ee4eb0b to 9fafe42 Compare November 25, 2020 18:09

btoueg reviewed Nov 26, 2020

View reviewed changes

datadog/resource_datadog_security_monitoring_rule.go Outdated Show resolved Hide resolved

datadog/resource_datadog_security_monitoring_rule.go Outdated Show resolved Hide resolved

therve requested changes Nov 26, 2020

View reviewed changes

Bit-Doctor reviewed Nov 26, 2020

View reviewed changes

btoueg changed the title ~~Add support for security monitoring rules~~ [SA-535]Add support for security monitoring rules Nov 26, 2020

btoueg reviewed Nov 26, 2020

View reviewed changes

Bit-Doctor reviewed Nov 26, 2020

View reviewed changes

datadog/data_source_datadog_security_monitoring_rules.go Show resolved Hide resolved

Bit-Doctor reviewed Nov 26, 2020

View reviewed changes

datadog/resource_datadog_security_monitoring_default_rule.go Outdated Show resolved Hide resolved

greentruff force-pushed the atrufanow/add-signal branch from 9e58eb2 to b015a85 Compare December 1, 2020 18:24

Alexandre Trufanow added 18 commits December 2, 2020 11:06

Add auto-generated rule response resource

b6b51b4

Implement CRUD for security monitoring rules

a98ecf6

Add test for optional fields

1c2f376

Make all options required

Add importer and test

8c6ea43

Fix missing/incorrect fields

Security Monitoring add support for default rules and listing rules

26dbeeb

- tests are broken

Use datasource for default rules tests

8c212a2

Record cassettes for security monitoring

d9b50cd

Add security monitoring rules docs

866a3ef

Separate enabled and disabled attributes

c755788

Changes after review

6a02d78

- Remove Exists on default rule, the underlying resource should always exists - Rename file and fix indent

Remove disabled attribute for security_monitoring_rule

a032669

Test list length for datasource

fffc38d

Format docs with prettier

d57a9bd

Regenerate security monitoring cassettes

ff415ff

Changes after review

fb97668

Merge enable/disable attributes for security_monitoring_default_rule

11a4c23

Update cassettes on datadoghq.com

cd2a593

- Fix window management for datasource

Fix cassettes

45688e9

- Stable ID for datasource - Only check that rules have been deleted

Default rules are imported externally

fff4df2

greentruff force-pushed the atrufanow/add-signal branch from b015a85 to fff4df2 Compare December 2, 2020 10:06

Update docs after review

63fa02f

therve requested changes Dec 3, 2020

View reviewed changes

Changes after review

250f12d

- Remove exists for secmon rule resource - Inline if err != nil checks - Add missing id in documentation

therve requested changes Dec 3, 2020

View reviewed changes

Return when no rule is found

8baca8f

therve approved these changes Dec 3, 2020

View reviewed changes

therve merged commit 49fa690 into master Dec 3, 2020

therve deleted the atrufanow/add-signal branch December 3, 2020 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SA-535]Add support for security monitoring rules #763

[SA-535]Add support for security monitoring rules #763

greentruff commented Nov 24, 2020 •

edited by therve

Loading

greentruff Nov 25, 2020

therve Nov 26, 2020

greentruff Nov 26, 2020 •

edited

Loading

therve Dec 3, 2020

greentruff Nov 25, 2020

therve left a comment

therve Nov 26, 2020

Bit-Doctor left a comment

Bit-Doctor Nov 26, 2020

btoueg Nov 26, 2020

btoueg Nov 26, 2020

Bit-Doctor Nov 26, 2020

greentruff Nov 26, 2020

Bit-Doctor Nov 26, 2020

Bit-Doctor Dec 1, 2020

greentruff commented Dec 2, 2020

therve commented Dec 2, 2020

azure-pipelines bot commented Dec 2, 2020

therve commented Dec 3, 2020

azure-pipelines bot commented Dec 3, 2020

therve left a comment

therve Dec 3, 2020

therve Dec 3, 2020

greentruff Dec 3, 2020

therve Dec 3, 2020

therve Dec 3, 2020

therve Dec 3, 2020


		_, err := datadogClientV2.SecurityMonitoringApi.DeleteSecurityMonitoringRule(authV2, d.Id()).Execute()

		if err != nil {

[SA-535]Add support for security monitoring rules #763

[SA-535]Add support for security monitoring rules #763

Conversation

greentruff commented Nov 24, 2020 • edited by therve Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greentruff Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bit-Doctor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greentruff commented Dec 2, 2020

therve commented Dec 2, 2020

azure-pipelines bot commented Dec 2, 2020

therve commented Dec 3, 2020

azure-pipelines bot commented Dec 3, 2020

therve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greentruff commented Nov 24, 2020 •

edited by therve

Loading

greentruff Nov 26, 2020 •

edited

Loading