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

[datadog_security_monitoring_rule] Add Terraform Support for Signal Correlation Rules #1593

Merged
merged 6 commits into from
Oct 17, 2022

Conversation

clementgbcn
Copy link
Contributor

@clementgbcn clementgbcn commented Oct 4, 2022

Description

This PR updates the Terraform provider to support Signal Correlation rules.

It replaces #1591 which was not backward compatible.

It impact the resources/data:

  • resource_datadog_security_monitoring_rule
  • data_source_datadog_security_monitoring_rules

To be backward compatible, the fields which are specific to Signal Correlation rules (i.e., ruleId, defaultRuleId, correlatedByFields, correlatedQueryIndex) are populated in a different sub list Schema called signal_query.
Since query is a List that could have multiple elements, it is not possible to enforce a DistinctsWith.

Since the Go struct SecurityMonitoringRuleResponse does not implement a common interface for SecurityMonitoringStandardRuleResponse and SecurityMonitoringSignalRuleResponse, most of the methods have been specialised in spite of having common code.

Additionally, this PR homogenises the way the payloads are populated by using the Setters instead of directly accessing the attributes in security_monitoring. It addresses remarks from previous PR about wording.

How do these changes impact current model

Up to now, Datadog Security Monitoring has been supporting one common type of rule whose Terraform schema is

resource "datadog_security_monitoring_rule" "log_detection_example" {
  name = "My log detection rule"
  message = "Rule triggered"
  enabled = false
  has_extended_title = true
  query {
	  name = "first"
	  query = "does not really match much"
	  aggregation = "count"
	  group_by_fields = ["host"]
  }
  case {...}
  options {...}
  filter {...}
  tags = [...]
}

This schema is not modified for existing rule type.

On the other hand, signal correlation rules will have a similar schema which will differ by its queries and its type:

resource "datadog_security_monitoring_rule" "signal_correlation_example" {
  name = "My Signal Correlation rule"
  message = "Important attack in progress"
  enabled = false
  has_extended_title = true
  signal_query {
	  name = "first"
	  rule_id = "lc2-ol2-ue0"
	  aggregation = "event_count"
	  correlated_fy_fields = ["host"]
          correlated_query_index = ""
  }
  signal_query {
	  name = "first"
	  rule_id = "fgt-40m-kiv"
	  aggregation = "event_count"
	  group_by_fields = ["host"]
          correlated_query_index = "1"
  }
  case {...}
  options {...}
  filter {...}
  tags = [...]
}

A check was implemented in Terraform to prevent users from having two populated lists (i.e. query and signal_query).

correlated_query_index

In spite of being an index (thus an integer), the Terraform Schema uses a string to represent it because this value can be empty.
Having:

  • correlated_query_index=0: Means correlate on the projected attributes of the first query of the correlated rule
  • correlated_query_index="": Means correlate on the non-projected per query attribute of the correlated rule.
    Since TypeInt object does not support a nil value as a Default value, we decided to use a string which is converted to an integer if it is not empty.

@clementgbcn clementgbcn changed the title [datadog_security_monitoring] Add Terraform Support for Signal Correlation Rules [datadog_security_monitoring_rule] Add Terraform Support for Signal Correlation Rules Oct 4, 2022
@clementgbcn clementgbcn force-pushed the cgc/SEC-3135-Signal-Correlation-Backward branch 3 times, most recently from ad51fdf to 02d8e5b Compare October 4, 2022 15:12
@clementgbcn clementgbcn force-pushed the cgc/SEC-3135-Signal-Correlation-Backward branch 2 times, most recently from c2fcf70 to 89db8dc Compare October 10, 2022 12:52
@clementgbcn clementgbcn marked this pull request as ready for review October 10, 2022 12:53
@clementgbcn clementgbcn requested review from a team as code owners October 10, 2022 12:53
@clementgbcn clementgbcn force-pushed the cgc/SEC-3135-Signal-Correlation-Backward branch from 89db8dc to a39d706 Compare October 10, 2022 15:28
maycmlee
maycmlee previously approved these changes Oct 11, 2022
Copy link
Contributor

@maycmlee maycmlee left a comment

Choose a reason for hiding this comment

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

👍 for docs

datadog/data_source_datadog_security_monitoring_rules.go Outdated Show resolved Hide resolved
datadog/resource_datadog_security_monitoring_rule.go Outdated Show resolved Hide resolved
@@ -317,35 +426,78 @@ func resourceDatadogSecurityMonitoringRuleCreate(ctx context.Context, d *schema.
return diag.FromErr(err)
}

d.SetId(response.SecurityMonitoringStandardRuleResponse.GetId())
if response.SecurityMonitoringStandardRuleResponse != nil {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: I think it would be better to differentiate based on wether query or signal_query field is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, the next instruction is about getting the id of the response by accessing the non null field of the response. Therefore, it seems to me that this check is mandatory (and then sufficient) for assigning the id.

query := d.Get("query").([]interface{})
signalQuery := d.Get("signal_query").([]interface{})
if len(query) > 0 && len(signalQuery) > 0 {
return fmt.Errorf("query list and signal query list cannot be both populated")
Copy link
Member

Choose a reason for hiding this comment

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

Why not use TF validation schema helpers such as ConflictsWith properties instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried but it was not working since, as stated in the documentations:

// Only absolute attribute paths, ones starting with top level attribute
// names, are supported. Attribute paths cannot be accurately declared
// for TypeList (if MaxItems is greater than 1), TypeMap, or TypeSet
// attributes. To reference an attribute under a single configuration block
// (TypeList with Elem of *Resource and MaxItems of 1), the syntax is
// "parent_block_name.0.child_attribute_name".

In our case, it is a TypeList whose MaxItems is greater than 1.

skarimo
skarimo previously approved these changes Oct 12, 2022
Copy link
Contributor

@pietrodll pietrodll left a comment

Choose a reason for hiding this comment

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

Nice job 👌 There's a typo in the aggregation type description, otherwise only nits 😄

datadog/resource_datadog_security_monitoring_rule.go Outdated Show resolved Hide resolved
datadog/resource_datadog_security_monitoring_rule.go Outdated Show resolved Hide resolved
datadog/resource_datadog_security_monitoring_rule.go Outdated Show resolved Hide resolved
datadog/resource_datadog_security_monitoring_rule.go Outdated Show resolved Hide resolved
@clementgbcn clementgbcn force-pushed the cgc/SEC-3135-Signal-Correlation-Backward branch from a96195e to ba31583 Compare October 13, 2022 11:48
pietrodll
pietrodll previously approved these changes Oct 13, 2022
Copy link
Contributor

@pietrodll pietrodll left a comment

Choose a reason for hiding this comment

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

Very nice 🎉 only a couple of nits

Comment on lines 724 to 732
if v, ok := query["metrics"]; ok && v != nil {
if tfMetrics, ok := v.([]interface{}); ok && len(tfMetrics) > 0 {
metrics := make([]string, len(tfMetrics))
for i, value := range tfMetrics {
metrics[i] = value.(string)
}
payloadQuery.SetMetrics(metrics)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This code seems useless as metrics is not defined on signal_query.

Suggested change
if v, ok := query["metrics"]; ok && v != nil {
if tfMetrics, ok := v.([]interface{}); ok && len(tfMetrics) > 0 {
metrics := make([]string, len(tfMetrics))
for i, value := range tfMetrics {
metrics[i] = value.(string)
}
payloadQuery.SetMetrics(metrics)
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ (in all places)

Optional: true,
Description: "The rule type.",
Default: "log_detection",
},
}
}

// SecurityMonitoringRuleInterface Common Interface to SecurityMonitoringRuleCreateInterface and SecurityMonitoringRuleReadInterface
type SecurityMonitoringRuleInterface interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these interfaces should be private, as they are utilities to factorize code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@pietrodll pietrodll left a comment

Choose a reason for hiding this comment

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

🎉

@skarimo skarimo enabled auto-merge (squash) October 17, 2022 14:14
@skarimo skarimo merged commit 58aa100 into master Oct 17, 2022
@skarimo skarimo deleted the cgc/SEC-3135-Signal-Correlation-Backward branch October 17, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants