azurerm_sql_database - Add sql threat detection policy configuration #1628

TFaga · 2018-07-23T10:55:56Z

This pull request adds the ability to configure sql threat detection policy for an azurerm_sql_database. Addresses issue #1484

…er` resource

…onfiguration of the `azurerm_sql_database` resource

tombuildsstuff

hey @TFaga

Thanks for this PR - I've taken a look through and left some comments in-line - if we can fix those up we should be able to run the tests and see how this looks :)

Thanks!

tombuildsstuff · 2018-07-23T11:22:04Z

azurerm/resource_arm_sql_database_test.go

+		email_addresses            = "%s"
+		storage_account_access_key = "${azurerm_storage_account.test.primary_access_key}"
+    	storage_endpoint           = "${azurerm_storage_account.test.primary_blob_endpoint}"
+		use_server_default         = "%s"


minor can we make the indentation consistent here?

tombuildsstuff · 2018-07-23T11:23:28Z

azurerm/resource_arm_sql_database.go

+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"disabled_alerts": {
+							Type:             schema.TypeString,


going on the description below:

disabled_alerts - (Optional) Specifies the semicolon-separated list of alerts that are disabled, or empty string to disable no alerts. Possible values: Sql_Injection; Sql_Injection_Vulnerability; Access_Anomaly; Usage_Anomaly.

If this is guaranteed to be a list of semicolon-separated strings, it would be a better UX to make this a List/Set of strings, so that users could get a better diff here in future; what do you think?

tombuildsstuff · 2018-07-23T11:58:40Z

azurerm/resource_arm_sql_database.go

+							Type:             schema.TypeString,
+							Optional:         true,
+							DiffSuppressFunc: suppress.CaseDifference,
+							Default:          string(sql.SecurityAlertPolicyUseServerDefaultDisabled),


🤔 would it make sense for this to default to Enabled?

The reason I left it Disabled is because that's the default value returned from the API if no policy is set.

Default values from the API:

{ "properties": { "useServerDefault": "Disabled", "state": "Disabled", "disabledAlerts": "", "emailAddresses": "", "emailAccountAdmins": "Disabled", "storageEndpoint": "", "storageAccountAccessKey": "", "retentionDays": 0 } }

tombuildsstuff · 2018-07-24T12:25:35Z

azurerm/resource_arm_sql_database.go

+						},
+
+						"email_addresses": {
+							Type:     schema.TypeString,


(same here) I think this'd make more sense as a list?

tombuildsstuff · 2018-07-24T12:29:54Z

azurerm/resource_arm_sql_database_test.go

@@ -644,3 +682,51 @@ resource "azurerm_sql_database" "test" {
 }
 `, rInt, location, rInt, rInt, requestedServiceObjectiveName)
 }
+
+func testAccAzureRMSqlDatabase_threatDetectionPolicy(rInt int, location, state, disabledAlerts, emailAccountAdmins, emailAddresses,


can we hard-code the state,disabledAlerts, emailAccountAdmins, emailAddresses, useServerDefault and retentionDays fields here? since they're only used in a single place, there's no need to make them dynamic

tombuildsstuff · 2018-07-24T12:45:10Z

website/docs/r/sql_database.html.markdown

@@ -85,6 +87,17 @@ The following arguments are supported:
 * `authentication_type` - (Required) Specifies the type of authentication used to access the server. Valid values are `SQL` or `ADPassword`.
 * `operation_mode` - (Optional) Specifies the type of import operation being performed. The only allowable value is `Import`.

+`threat_detection_policy` supports  the following:
+
+* `state` - (Required) Specifies the state of the policy. If state is Enabled, storageEndpoint and storageAccountAccessKey are required.


can we update:

Specifies the state of the policy. If state is Enabled, storageEndpoint and storageAccountAccessKey are required.

to be:

The State of the Policy. Possible values are `Enabled`, `Disabled` or `New`.

tombuildsstuff · 2018-07-24T12:45:27Z

website/docs/r/sql_database.html.markdown

@@ -85,6 +87,17 @@ The following arguments are supported:
 * `authentication_type` - (Required) Specifies the type of authentication used to access the server. Valid values are `SQL` or `ADPassword`.
 * `operation_mode` - (Optional) Specifies the type of import operation being performed. The only allowable value is `Import`.

+`threat_detection_policy` supports  the following:


minor can we add --- and then a blank line above this to add a divider?

tombuildsstuff · 2018-07-24T12:58:44Z

website/docs/r/sql_database.html.markdown

+
+* `state` - (Required) Specifies the state of the policy. If state is Enabled, storageEndpoint and storageAccountAccessKey are required.
+* `disabled_alerts` - (Optional) Specifies the semicolon-separated list of alerts that are disabled, or empty string to disable no alerts. Possible values: Sql_Injection; Sql_Injection_Vulnerability; Access_Anomaly; Usage_Anomaly.
+* `email_account_admins` - (Optional) Specifies that the alert is sent to the account administrators.


Specifies that the alert is sent to the account administrators. -> Should the account administrators be emailed when this alert is triggered?

tombuildsstuff · 2018-07-24T13:04:27Z

website/docs/r/sql_database.html.markdown

+* `state` - (Required) Specifies the state of the policy. If state is Enabled, storageEndpoint and storageAccountAccessKey are required.
+* `disabled_alerts` - (Optional) Specifies the semicolon-separated list of alerts that are disabled, or empty string to disable no alerts. Possible values: Sql_Injection; Sql_Injection_Vulnerability; Access_Anomaly; Usage_Anomaly.
+* `email_account_admins` - (Optional) Specifies that the alert is sent to the account administrators.
+* `email_addresses` - (Optional) Specifies the semicolon-separated list of e-mail addresses to which the alert is sent.


Once we've updated this to a list (above) we should be able to make this:

A list of email addresses which alerts should be sent to.

tombuildsstuff · 2018-07-24T13:07:25Z

website/docs/r/sql_database.html.markdown

+* `retention_days` - (Optional) Specifies the number of days to keep in the Threat Detection audit logs.
+* `storage_account_access_key` - (Optional) Specifies the identifier key of the Threat Detection audit storage account. If state is Enabled, storageAccountAccessKey is required.
+* `storage_endpoint` - (Optional) Specifies the blob storage endpoint (e.g. https://MyAccount.blob.core.windows.net). This blob storage will hold all Threat Detection audit logs. If state is Enabled, storageEndpoint is required.
+* `use_server_default` - (Optional) Specifies whether to use the default server policy.


Specifies whether to use the default server policy. ->

Should the default server policy be used? Defaults to `true`

…es in the sql threat detection configuration of the `azurerm_sql_database` resource

TFaga · 2018-07-25T08:52:21Z

@tombuildsstuff Hey, thank you for the quick review. I've pushed a commit to address the changes.

katbyte

Hi @TFaga,

Thank you for the updates, apologies with the delay in giving this a re-review. I have left some mostly minor comments inline with my primary concern being there are no import test and a missing nil check. The rest are fairly stylistic in nature and are not blocking the PR.

katbyte · 2018-08-13T23:40:16Z

azurerm/resource_arm_sql_database.go

@@ -327,6 +435,12 @@ func resourceArmSqlDatabaseCreateUpdate(d *schema.ResourceData, meta interface{}

 	d.SetId(*resp.ID)

+	threatDetectionClient := meta.(*ArmClient).sqlDatabaseThreatDetectionPoliciesClient
+	_, err = threatDetectionClient.CreateOrUpdate(ctx, resourceGroup, serverName, name, *threatDetection)
+	if err != nil {


These two lines could be combined:

if _, err = threatDetectionClient.CreateOrUpdate(ctx, resourceGroup, serverName, name, *threatDetection); err != nil {

katbyte · 2018-08-13T23:42:26Z

azurerm/resource_arm_sql_database.go

@@ -354,6 +468,19 @@ func resourceArmSqlDatabaseRead(d *schema.ResourceData, meta interface{}) error
 		return fmt.Errorf("Error making Read request on Sql Database %s: %+v", name, err)
 	}

+	oldThreatDetection, hasOldThreatDetection := d.GetOk("threat_detection_policy")
+	if hasOldThreatDetection {


Is there a reason we are only reading this in when it has been set in state?

If the user changes this in the portal/we are importing won't those changes now be ignored?

The problem was that the thread detection problem is a separate API call which always returns the default values, causing every plan operation to be the following every time, if the threat_detection_policy was not set in the manifest as it filled out the list in the state with all default values:

~ azurerm_sql_database.test threat_detection_policy.#: "1" => "0"

I've changed this so that the list element is marked computed, which alleviates the problem. Though I'm not sure if this is ok or if there is a better option to handle this case.

katbyte · 2018-08-13T23:45:57Z

azurerm/resource_arm_sql_database.go

@@ -439,6 +566,46 @@ func flattenEncryptionStatus(encryption *[]sql.TransparentDataEncryption) string
 	return ""
 }

+func flattenArmSqlServerThreatDetectionPolicy(policy sql.DatabaseSecurityAlertPolicy, oldThreatDetection []interface{}) []interface{} {
+
+	properties := policy.DatabaseSecurityAlertPolicyProperties


Could we get a nil check here?

Maybe return early:

threatDetectionPolicy := make(map[string]interface{}) properties := policy.DatabaseSecurityAlertPolicyProperties if properties != nil { return []interface{}{threatDetectionPolicy} }

katbyte · 2018-08-13T23:54:39Z

azurerm/resource_arm_sql_database.go

+	threatDetectionPolicy["email_account_admins"] = string(properties.EmailAccountAdmins)
+	threatDetectionPolicy["use_server_default"] = string(properties.UseServerDefault)
+
+	if properties.DisabledAlerts != nil && *properties.DisabledAlerts != "" {


could we simplify this by:

if disabledAlerts := properties.DisabledAlerts; disabledAlerts != nil {

And if strings.Split() gets an empty string it should return an empty slice. Is there any harm in setting disabled_alerts to an empty slice?

This applies to email_addresses.

katbyte · 2018-08-13T23:57:40Z

azurerm/resource_arm_sql_database.go

+	}
+
+	// If storage account access key is in state readd it to the new state, as the API does not return it for security reasons
+	if t, ok := oldThreatDetection[0].(map[string]interface{})["storage_account_access_key"]; ok {


Minor, but this can be done by passing in d and grabbing the property directly. An example can be see here

katbyte · 2018-08-13T23:58:23Z

azurerm/resource_arm_sql_database.go

+		if v, ok := threadDetection["disabled_alerts"]; ok {
+			alerts := v.(*schema.Set).List()
+			expandedAlerts := make([]string, len(alerts))
+			for i := range alerts {


Very minor but this could be

for _, v := range v.(*schema.Set).List() { expandedAlerts[i] = v.(string) }

I've updated to use the value from the current element of the loop, though still needed the index for the expandedAlerts variable.

katbyte · 2018-08-14T00:00:53Z

website/docs/r/sql_database.html.markdown

+* `email_account_admins` - (Optional) Should the account administrators be emailed when this alert is triggered?
+* `email_addresses` - (Optional) A list of email addresses which alerts should be sent to.
+* `retention_days` - (Optional) Specifies the number of days to keep in the Threat Detection audit logs.
+* `storage_account_access_key` - (Optional) Specifies the identifier key of the Threat Detection audit storage account. If state is Enabled, storageAccountAccessKey is required.


Echoing toms comment, could

If state is Enabled, storageAccountAccessKey is required.

become:

Required if state is enabled.

katbyte · 2018-08-14T00:09:18Z

azurerm/resource_arm_sql_database_test.go

+				),
+			},
+			{
+				Config: postConfig,


Could we get an import test step to verify that import works?

{ ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },

…g of the remote state of the `azurerm_sql_database` resource

TFaga · 2018-08-18T17:03:47Z

Hi @katbyte ,

Thanks for the review. Apologies for the late response. I've pushed the changes in the comments.

WodansSon

This LGTM, I left some very minor validation request comments. Other than that this looks great.

WodansSon · 2018-08-20T23:43:37Z

azurerm/resource_arm_sql_database.go

+						"storage_account_access_key": {
+							Type:      schema.TypeString,
+							Optional:  true,
+							Sensitive: true,


Can we add some validation here to ensure something is entered?

ValidateFunc: validation.NoZeroValues,

WodansSon · 2018-08-20T23:44:02Z

azurerm/resource_arm_sql_database.go

+
+						"storage_endpoint": {
+							Type:     schema.TypeString,
+							Optional: true,


Same here with the validation.

…account_access_key` property of the `azurerm_sql_database` resource

TFaga · 2018-08-21T07:29:16Z

Hi @jeffreyCline,

I've pushed the changes in the comments.

katbyte · 2018-09-09T17:20:26Z

@TFaga,

Thank you for the updates! this LGTM 👍

Tests pass:

katbyte

(forgot to approve in last message)

ghost · 2019-03-06T14:38:18Z

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

TFaga added 2 commits July 23, 2018 11:46

Added SQL Threat Detection Policy properties to the `azurerm_sql_serv…

3f98f8d

…er` resource

Minor cleanup of unnecessary conditions in the sql threat detection c…

60c6ccd

…onfiguration of the `azurerm_sql_database` resource

tombuildsstuff suggested changes Jul 24, 2018

View reviewed changes

tombuildsstuff added enhancement waiting-response service/mssql Microsoft SQL Server labels Jul 24, 2018

Updated schema type of string lists to a type set with some minor fix…

2e2d854

…es in the sql threat detection configuration of the `azurerm_sql_database` resource

metacpp requested a review from WodansSon August 3, 2018 00:04

metacpp assigned WodansSon Aug 3, 2018

tombuildsstuff removed the waiting-response label Aug 3, 2018

katbyte requested changes Aug 14, 2018

View reviewed changes

Minor fixes in the sql threat detection list configuration and readin…

68222d2

…g of the remote state of the `azurerm_sql_database` resource

WodansSon requested changes Aug 20, 2018

View reviewed changes

TFaga added 2 commits August 21, 2018 07:42

Added missing validations to the storage_endpoint and the `storage_…

8672d73

…account_access_key` property of the `azurerm_sql_database` resource

Fixed formating of the schema of the the azurerm_sql_database resource

63be1f8

Update resource_arm_sql_database.go

0eab4de

ghost added the size/L label Sep 9, 2018

Merge branch 'master' into iss1484-sql-threat-detection

44ef9c7

go fmt

6ba227a

ghost added the size/L label Sep 9, 2018

katbyte approved these changes Sep 9, 2018

View reviewed changes

katbyte merged commit 9538c6b into hashicorp:master Sep 9, 2018

katbyte added this to the 1.15.0 milestone Sep 9, 2018

katbyte added a commit that referenced this pull request Sep 9, 2018

Update CHANGELOG.md to include #1628

c165188

ghost locked and limited conversation to collaborators Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_sql_database - Add sql threat detection policy configuration #1628

azurerm_sql_database - Add sql threat detection policy configuration #1628

TFaga commented Jul 23, 2018

tombuildsstuff left a comment

tombuildsstuff Jul 23, 2018

tombuildsstuff Jul 23, 2018

tombuildsstuff Jul 23, 2018

TFaga Jul 25, 2018

tombuildsstuff Jul 24, 2018

tombuildsstuff Jul 24, 2018

tombuildsstuff Jul 24, 2018

tombuildsstuff Jul 24, 2018

tombuildsstuff Jul 24, 2018

tombuildsstuff Jul 24, 2018

tombuildsstuff Jul 24, 2018

TFaga commented Jul 25, 2018

katbyte left a comment

katbyte Aug 13, 2018

katbyte Aug 13, 2018

TFaga Aug 18, 2018

katbyte Aug 13, 2018

katbyte Aug 13, 2018

katbyte Aug 13, 2018

katbyte Aug 13, 2018

katbyte Aug 13, 2018

TFaga Aug 18, 2018

katbyte Aug 14, 2018

katbyte Aug 14, 2018

TFaga commented Aug 18, 2018

WodansSon left a comment

WodansSon Aug 20, 2018

WodansSon Aug 20, 2018

TFaga commented Aug 21, 2018

katbyte commented Sep 9, 2018

katbyte left a comment

ghost commented Mar 6, 2019

azurerm_sql_database - Add sql threat detection policy configuration #1628

azurerm_sql_database - Add sql threat detection policy configuration #1628

Conversation

TFaga commented Jul 23, 2018

tombuildsstuff 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

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

TFaga commented Jul 25, 2018

katbyte 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

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

TFaga commented Aug 18, 2018

WodansSon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TFaga commented Aug 21, 2018

katbyte commented Sep 9, 2018

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Mar 6, 2019