New Resource - `sentinel_automation_rule` #11502

magodo · 2021-04-28T08:18:47Z

This PR implements a new sentinel resource called Automation Rule. This fixes: #10960.

Reference

Test Result

💤 TF_ACC=1 go test -v -timeout=2h ./azurerm/internal/services/sentinel -run="TestAccSentinelAutomationRule_"
2021/04/28 13:32:52 [DEBUG] not using binary driver name, it's no longer needed
2021/04/28 13:32:52 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccSentinelAutomationRule_basic
=== PAUSE TestAccSentinelAutomationRule_basic
=== RUN   TestAccSentinelAutomationRule_complete
=== PAUSE TestAccSentinelAutomationRule_complete
=== RUN   TestAccSentinelAutomationRule_update
=== PAUSE TestAccSentinelAutomationRule_update
=== RUN   TestAccSentinelAutomationRule_requiresImport
=== PAUSE TestAccSentinelAutomationRule_requiresImport
=== CONT  TestAccSentinelAutomationRule_basic
=== CONT  TestAccSentinelAutomationRule_requiresImport
=== CONT  TestAccSentinelAutomationRule_complete
=== CONT  TestAccSentinelAutomationRule_update
--- PASS: TestAccSentinelAutomationRule_basic (178.34s)
--- PASS: TestAccSentinelAutomationRule_complete (181.05s)
--- PASS: TestAccSentinelAutomationRule_requiresImport (267.14s)
--- PASS: TestAccSentinelAutomationRule_update (377.57s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/sentinel    377.733s

magodo · 2021-04-28T09:14:25Z

The test for action_playbook is blocked by #11505, without which we are not able to create a logic app trigger for sentinel incident.

katbyte · 2021-04-28T20:08:10Z

@magodo - can we fix logic app trigger so we can test this before merge?

kaovd · 2021-04-29T11:45:05Z

From the following tests I did it seems a few things need to be tweaked:

There seems to be some general issues when defining both action_incident and action_playbook, if you define these both it will cause an internal server error. By looking at the requests it appears something is breaking when putting these together, hence the null.

See results of defining Manual3 playbook with the following params (had to zoom out here):

Azure:
{
  "properties": {
    "displayName": "Manual3",
    "order": 6,
    "triggeringLogic": {
      "isEnabled": true,
      "expirationTimeUtc": null,
      "triggersOn": "Incidents",
      "triggersWhen": "Created",
      "conditions": [
        {
          "conditionType": "Property",
          "conditionProperties": {
            "propertyName": "IncidentTitle",
            "operator": "Contains",
            "propertyValues": [
              "a",
              "b"
            ]
          }
        }
      ]
    },
    "actions": [
      {
        "order": 1,
        "actionType": "ModifyProperties",
        "actionConfiguration": {
          "status": "Closed",
          "classification": "BenignPositive",
          "classificationReason": "SuspiciousButExpected"
        }
      },
      {
        "order": 2,
        "actionType": "RunPlaybook",
        "actionConfiguration": {
          "triggerUri": null,
          "logicAppResourceId": "/subscriptions/{redacted}/resourceGroups/sentinel/providers/Microsoft.Logic/workflows/Test",
          "tenantId": "{redacted}"
        }
      }
    ]
  }
}

Terraform:
{
  "properties": {
    "actions": [
      {
        "actionConfiguration": {
          "logicAppResourceId": "/subscriptions/{redacted}/resourceGroups/sentinel/providers/Microsoft.Logic/workflows/Test",
          "tenantId": "{redacted}"
        },
        "actionType": "RunPlaybook",
        "order": 1
      },
      null
    ],
    "displayName": "automation_rule3",
    "order": 10,
    "triggeringLogic": {
      "isEnabled": true,
      "triggersOn": "Incidents",
      "triggersWhen": "Created",
      "conditions": [
        {
          "conditionProperties": {
            "propertyName": "IncidentTitle",
            "operator": "Contains",
            "propertyValues": [
              "a",
              "b"
            ]
          },
          "conditionType": "Property"
        }
      ]
    }
  }
}

Possibly the distiniction between action_incident and action_playbook is doing this? Surely would make more sense to have playbook definition inside action_incident rather than seperating the two.
From analysing the code it seems it breaks at

	copy(out, actionIncident)
	copy(out, actionPlaybook)

In expandAutomationRuleActions, seems the copy function is just taking the later result in this instance and nulls the second part of the object? I'm a complete Golang newbie here in fairness.

If a Azure Sentinel has not been given the permission to access a given RG where a logic app is, i.e Azure Sentinel automation Contributor, the apply will hang and not bail. Similarlly if user does not have permissions either - Will fail after 5m on 500 Error although might be a good idea to check this if possible?
While it does not break the API, we might want to validate the order options when it comes to the actions i.e playbook and incident actions. Seems fine to clobber orders of automation rules themselves but in the portal if you clobber the actions in terraform and reapply them in azure they randomly put themself into a 1,2 etc order

kaovd · 2021-04-29T12:06:15Z

The test for action_playbook is blocked by #11505, without which we are not able to create a logic app trigger for sentinel incident.

~~Apologies but what exactly is the issue here? The issue you linked isnt too clear as its closed and references to a more general issue.~~
Should be able to work around this by deploying a logic app with an arm template to the test resource group, whereby you now know what the resource Id will end up being. Same case with the sentinel connection just making sure to set the subscription in the template. While I think you may still have to authorize it this shouldn't break the test as all we need to do is connect a logic app with the given trigger.

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go

kaovd

Potential Test change for action_playbook

azurerm/internal/services/sentinel/sentinel_automation_rule_resource_test.go

magodo · 2021-04-30T03:35:02Z

@kaovd Thank you so much for testing this and finding the bugs! Also many thanks to providing the template here to work around the lack of web connection in Terraform!

azurerm/internal/services/sentinel/sentinel_automation_rule_resource_test.go

kaovd · 2021-05-05T07:08:56Z

Reping @katbyte

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go

kaovd · 2021-05-05T18:39:58Z

Just noticed that enabled wasnt an option, added it in see above recent comment @magodo

magodo · 2021-05-06T01:22:23Z

I did this intentionally. I didn't see too much value that users will want to define multiple disabled automation rules in the config. Whilst each way is reasonable, tbh.

kaovd · 2021-05-06T06:14:25Z

I did this intentionally. I didn't see too much value that users will want to define multiple disabled automation rules in the config. Whilst each way is reasonable, tbh.

Ah, surely wouldnt hurt to add in? As a user I just want the ability to toggle off rules while doing change management before deleting them fully. Can always define an impossible condition but this is a bit neater.

kaovd · 2021-05-09T18:25:32Z

Maybe move this from blocked to https://github.com/terraform-providers/terraform-provider-azurerm/milestone/126 as its not blocked anymore? Providing we can make that, just pending on that review.

magodo · 2021-05-10T03:09:42Z

@katbyte Would you please take another look at this PR?

kaovd

Latest AzureRm master pulled that SDK in (#12263), Enums are correct now on the latest rebase. This would fix it for the merge

> $ TF_TRACE=1 TF_ACC=1 go test -v -timeout=2h ./azurerm/internal/services/sentinel -run="TestAccSentinelAutomationRule_basic"                               [±sentinel_automation_rule ●▴]
2021/06/20 11:24:07 [DEBUG] not using binary driver name, it's no longer needed
2021/06/20 11:24:07 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccSentinelAutomationRule_basic
=== PAUSE TestAccSentinelAutomationRule_basic
=== CONT  TestAccSentinelAutomationRule_basic
--- PASS: TestAccSentinelAutomationRule_basic (263.33s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/sentinel    264.023s

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go

magodo · 2021-06-21T07:35:01Z

@kaovd I've merged with the master branch and made the changes. Thank you!

kaovd · 2021-06-21T07:42:10Z

@kaovd I've merged with the master branch and made the changes. Thank you!

Great! Still waiting on that review though :/

magodo · 2021-06-21T07:45:09Z

@kaovd I'll reach to the teams and see anything we can do to make it merged.

katbyte

Thanks for the PR @magodo - sorry its taken so long to review but i have a couple of concerns. Mainly that it appears we need some new resources so support all the added properties without using arm templates in the test. as well it needs a merge master and the build fixed:

[Step 4/5] in directory: /opt/teamcity-agent/work/a73be106926a7472/azurerm/internal/services/sentinel
[04:39:46]	[Step 4/5] sentinel_automation_rule_resource.go:22:2: cannot find package "." in:
[04:39:46]	[Step 4/5] 	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-sdk/helper/schema
[04:39:46]	[Step 4/5] sentinel_automation_rule_resource.go:15:2: cannot find package "." in:
[04:39:46]	[Step 4/5] 	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-sdk/helper/validation
[04:39:46]	[Step 4/5] Process exited with code 1

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go

katbyte · 2021-06-29T20:02:30Z

azurerm/internal/services/sentinel/sentinel_automation_rule_resource_test.go

+  principal_id         = data.azuread_service_principal.securityinsights.object_id
+}
+
+resource "azurerm_template_deployment" "testconnection" {


is there a reason we are not implementing a resource for this? we try our best to never do ARM deployments in acctests

This is currently tracked by #1691, which is blocked by lacks of Go SDK support.

katbyte · 2021-06-29T20:02:37Z

azurerm/internal/services/sentinel/sentinel_automation_rule_resource_test.go

+  TEMPLATE
+}
+
+resource "azurerm_template_deployment" "testlogicapp" {


This piece of template is to provision a azurerm_logic_app_workflow with azurerm_logic_app_trigger_custom embedded. It looks like we can do the same by using these existing resources as below:

resource "azurerm_logic_app_workflow" "test" { name = "testworkflow1" location = azurerm_resource_group.test.location resource_group_name = azurerm_resource_group.test.name } resource "azurerm_logic_app_trigger_custom" "test" { name = "When_Azure_Sentinel_incident_creation_rule_was_triggered" logic_app_id = azurerm_logic_app_workflow.test.id body = <<BODY {og "type": "ApiConnectionWebhook", "inputs": { "body": { "callback_url": "@{listCallbackUrl()}" }, "host": { "connection": { "name": "/subscriptions/${data.azurerm_client_config.current.subscription_id}/resourceGroups/${azurerm_resource_group.test.name}/providers/Microsoft.Web/connections/azuresentinel" } }, "path": "/incident-creation" } } BODY depends_on = [azurerm_template_deployment.testconnection] }

However, this ends up with a logic app workflow non-functional (which is greyed out in the logic app designer interface, and has a red mark complaining Connector not found). By comparing the template (logic app code), the difference might be the format, where the logic app might ask for the certain parameter to be declared and defined. Then it means we will need customized parameter definition for the azurerm_logc_app_workflow resource, which should be resolved by #12314.

The non-functional one

code view

{ "definition": { "$schema": "https://pluginsdk.management.azure.com/providers/Microsoft.Logic/schemas/2016-06-01/workflowdefinition.json#", "actions": {}, "contentVersion": "1.0.0.0", "triggers": { "When_Azure_Sentinel_incident_creation_rule_was_triggered": { "inputs": { "body": { "callback_url": "@{listCallbackUrl()}" }, "host": { "connection": { "name": "/subscriptions/****/resourceGroups/acctestRG-sentinel-999/providers/Microsoft.Web/connections/azuresentinel" } }, "path": "/incident-creation" }, "type": "ApiConnectionWebhook" } } }, "parameters": {} }

THe functional one

code view

{ "definition": { "$schema": "https://schema.management.azure.com/providers/Microsoft.Logic/schemas/2016-06-01/workflowdefinition.json#", "contentVersion": "1.0.0.0", "outputs": {}, "parameters": { "$connections": { "defaultValue": {}, "type": "Object" } }, "triggers": { "When_Azure_Sentinel_incident_creation_rule_was_triggered": { "inputs": { "body": { "callback_url": "@{listCallbackUrl()}" }, "host": { "connection": { "name": "@parameters('$connections')['azuresentinel']['connectionId']" } }, "path": "/incident-creation" }, "type": "ApiConnectionWebhook" } } }, "parameters": { "$connections": { "value": { "azuresentinel": { "connectionId": "/subscriptions/****/resourceGroups/acctestRG-sentinel-888/providers/Microsoft.Web/connections/azuresentinel", "connectionName": "azuresentinel", "id": "/subscriptions/****/providers/Microsoft.Web/locations/westeurope/managedApis/azuresentinel" } } } } }

Correct above findings, the reason why the former doesn't work is because of the setting inside the triggers.When***.inputs.host.connection, it should be set into @parameters('$connections')['azuresentinel']['connectionId'], rather than the evaluated value, I have no idea why thoug 😂

katbyte · 2021-06-29T20:03:55Z

azurerm/internal/services/sentinel/sentinel_automation_rule_resource_test.go

+
+  action_playbook {
+    order        = 5
+    logic_app_id = "/subscriptions/${data.azurerm_client_config.current.subscription_id}/resourceGroups/${azurerm_resource_group.test.name}/providers/Microsoft.Logic/workflows/Test"


given we don't yet have resources to support this maybe we remove these properties until they exist?

Now that it works actually, which means the functionality of this property (block) has no problem. Also some customers (e.g. @kaovd) exactly want this.

Agree with this, while current workaround may use an arm template to test this, it works fully. Having this feature is an absolute game changer for everyone with sentinel deployments. I dont see why we should throw this out the window just because of how tests are implemented

kaovd · 2021-07-26T09:16:01Z

Any further updates post the changes you requested @katbyte ? Just want to know if this is acceptable as a PR as everything is working - if the rm template in the tests is going to completely block this from a standards point then can just fork this functionality instead

luke-iseger91 · 2021-10-06T15:02:17Z

I am eagerly awaiting this new resource. Is there anything left to resolve for this PR? @magodo @katbyte

kaovd · 2021-10-06T17:41:17Z

Can we just remove the logic app trigger so we can do without templates in the tests if thats going to be the conditon rather than having the whole resource blocked, can probably just use a script prov on create / destroy to deal with the rest for now as seems ms are going to take a lot of pushing to action bugfixes on logic api upstream @magodo @katbyte

magodo · 2021-10-08T01:54:48Z

@katbyte I've removed the arm template usage from the test cases, which only affects one property of the automation rule resourc, the action_playbook.

katbyte

Aside from 1 comment looks good now @magodo

katbyte · 2021-10-25T20:48:36Z

internal/services/sentinel/sentinel_automation_rule_resource.go

+	_, err = client.CreateOrUpdate(ctx, id.ResourceGroup, OperationalInsightsResourceProvider, id.WorkspaceName, id.Name, params)
+	if err != nil {


Suggested change

_, err = client.CreateOrUpdate(ctx, id.ResourceGroup, OperationalInsightsResourceProvider, id.WorkspaceName, id.Name, params)

if err != nil {

if _, err = client.CreateOrUpdate(ctx, id.ResourceGroup, OperationalInsightsResourceProvider, id.WorkspaceName, id.Name, params); err != nil {

kaovd · 2021-10-26T10:36:02Z

@katbyte Thank you !!!!

github-actions · 2021-10-29T00:46:52Z

This functionality has been released in v2.83.0 of the Terraform 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!

github-actions · 2021-11-28T02:03:46Z

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 contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ghost added size/XXL documentation labels Apr 28, 2021

magodo added new-resource preview service/sentinel labels Apr 28, 2021

magodo mentioned this pull request Apr 28, 2021

Support for azurerm_sentinel_alert_rule_action #11366

Closed

katbyte added the waiting-response label Apr 28, 2021

katbyte added this to the Blocked milestone Apr 28, 2021

ghost removed the waiting-response label Apr 29, 2021

kaovd suggested changes Apr 29, 2021

View reviewed changes

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go Outdated Show resolved Hide resolved

kaovd suggested changes Apr 29, 2021

View reviewed changes

azurerm/internal/services/sentinel/sentinel_automation_rule_resource_test.go Outdated Show resolved Hide resolved

magodo marked this pull request as draft April 30, 2021 08:01

kaovd reviewed Apr 30, 2021

View reviewed changes

azurerm/internal/services/sentinel/sentinel_automation_rule_resource_test.go Outdated Show resolved Hide resolved

magodo marked this pull request as ready for review May 2, 2021 03:22

magodo requested a review from katbyte May 2, 2021 03:22

kaovd reviewed May 5, 2021

View reviewed changes

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go Outdated Show resolved Hide resolved

kaovd reviewed May 5, 2021

View reviewed changes

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go Outdated Show resolved Hide resolved

kaovd approved these changes May 7, 2021

View reviewed changes

katbyte removed this from the Blocked milestone May 11, 2021

kaovd suggested changes Jun 20, 2021

View reviewed changes

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go Outdated Show resolved Hide resolved

azurerm/internal/services/sentinel/sentinel_automation_rule_resource.go Outdated Show resolved Hide resolved

github-actions bot added the size/XL label Jun 21, 2021

katbyte requested changes Jun 29, 2021

View reviewed changes

katbyte added the waiting-response label Jun 29, 2021

magodo removed the waiting-response label Jun 30, 2021

magodo added 8 commits August 11, 2021 09:14

staged

7d035cc

New Resource: sentinel_automation_rule

3029705

fix CI

29a1378

refine the enum after bumping up the SDK version

18b9102

add "run playbook" test

bed6459

read SecurityInsight SP from ad provider

5f57bdf

expose enabled property

1ec7fe8

adopt to the new project layout && adopt to using pluginsdk

4b76bd7

magodo force-pushed the sentinel_automation_rule branch from 996a899 to 4b76bd7 Compare August 11, 2021 01:31

remove the arm template usage from the test cases

b82643e

katbyte approved these changes Oct 25, 2021

View reviewed changes

katbyte added this to the v2.83.0 milestone Oct 25, 2021

katbyte merged commit 27ca46d into hashicorp:main Oct 25, 2021

katbyte added a commit that referenced this pull request Oct 25, 2021

CHANGELOG.md for #11502

58f819a

github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2021

		_, err = client.CreateOrUpdate(ctx, id.ResourceGroup, OperationalInsightsResourceProvider, id.WorkspaceName, id.Name, params)
		if err != nil {

	_, err = client.CreateOrUpdate(ctx, id.ResourceGroup, OperationalInsightsResourceProvider, id.WorkspaceName, id.Name, params)
	if err != nil {
	if _, err = client.CreateOrUpdate(ctx, id.ResourceGroup, OperationalInsightsResourceProvider, id.WorkspaceName, id.Name, params); err != nil {

New Resource - sentinel_automation_rule #11502

New Resource - sentinel_automation_rule #11502

Conversation

magodo commented Apr 28, 2021

Reference

Test Result

magodo commented Apr 28, 2021

katbyte commented Apr 28, 2021

kaovd commented Apr 29, 2021 • edited Loading

kaovd commented Apr 29, 2021 • edited Loading

kaovd left a comment

Choose a reason for hiding this comment

magodo commented Apr 30, 2021

kaovd commented May 5, 2021

kaovd commented May 5, 2021

magodo commented May 6, 2021

kaovd commented May 6, 2021

kaovd commented May 9, 2021

magodo commented May 10, 2021

kaovd left a comment

Choose a reason for hiding this comment

magodo commented Jun 21, 2021

kaovd commented Jun 21, 2021

magodo commented Jun 21, 2021

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

kaovd commented Jul 26, 2021

luke-iseger91 commented Oct 6, 2021

kaovd commented Oct 6, 2021

magodo commented Oct 8, 2021

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaovd commented Oct 26, 2021

github-actions bot commented Oct 29, 2021

github-actions bot commented Nov 28, 2021

New Resource - `sentinel_automation_rule` #11502

New Resource - `sentinel_automation_rule` #11502

kaovd commented Apr 29, 2021 •

edited

Loading

kaovd commented Apr 29, 2021 •

edited

Loading