New Resource: `azurerm_metric_alertrule` #478

dominik-lekse · 2017-10-31T15:23:59Z

This pull request adds the resource azurerm_metric_alertrule to manage Azure Monitor metric-based alert rules.

Azure Monitor alert supports three different conditions as part of an alert rule: ThresholdRuleCondition, LocationThresholdRuleCondition, and ManagementEventRuleCondition (see API documentation). Each of these conditions use a subset of the parameters of the REST resource. This Terraform resource only implements the ThresholdRuleCondition which correspond to metric-based alert rules. This allows for more rigor validation in the Terraform resource.

Open tasks

Implement resource azurerm_metric_alertrule
Implement acceptance tests
Pass acceptance tests
Write documentation
Resolve runtime error issue in AlertRulesClient of azure-sdk-for-go (see [v11.2.2-beta] JSON unmarshalling error in monitor.AlertRuleResource Azure/azure-sdk-for-go#951)

Acceptance tests

make testacc TEST=./azurerm TESTARGS='-run=TestAccAzureRMMetricAlertRule_virtualMachineCpu'
make testacc TEST=./azurerm TESTARGS='-run=TestAccAzureRMMetricAlertRule_importVirtualMachineCpu'
make testacc TEST=./azurerm TESTARGS='-run=TestAccAzureRMMetricAlertRule_sqlDatabaseStorage'

Executed in Azure region West Europe:

=== RUN   TestAccAzureRMMetricAlertRule_importVirtualMachineCpu
--- PASS: TestAccAzureRMMetricAlertRule_importVirtualMachineCpu (467.06s)
=== RUN   TestAccAzureRMMetricAlertRule_virtualMachineCpu
--- PASS: TestAccAzureRMMetricAlertRule_virtualMachineCpu (430.68s)
=== RUN   TestAccAzureRMMetricAlertRule_sqlDatabaseStorage
--- PASS: TestAccAzureRMMetricAlertRule_sqlDatabaseStorage (185.65s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	1083.410s

Issues

Runtime error in `AlertRulesClient` in UnmarshalJSON

.Using the methods provided by the AlertRulesClient raises a runtime error due invalid memory address or nil pointer dereference. The error occurs during marshalling the json response in Get or CreateOrUpdate.

The reason appears to be that the field AlertRule of type AlertRuleResource is generated as an embedded field (vendor/github.com/Azure/azure-sdk-for-go/arm/monitor/models.go:397). This lead to nil being passed to monitor.(*AlertRule).UnmarshalJSON when unmarshalling.

Changing the field AlertRule to an explicitly defined field solves the problem. Either we use the AlertRulesClient in the wrong manner or there is a bug in the Azure Go SDK client generator.

This pull request includes a separate commit with the described fix in the vendor dependencies.

@tombuildsstuff Would be great if you could take a look at this issue.

…itor

…ield to solve nil dereference issues when using the AlertRulesClient in azure-sdk-for-go/arm/monitor

dominik-lekse · 2017-10-31T16:41:59Z

The make vendor-status fails since I modified the vendor dependencies. 🎃

dominik-lekse · 2017-11-20T19:24:06Z

@tombuildsstuff Did you get a chance to look into the runtime error of the AlertRulesClient which affects this pull request?

# Conflicts: # azurerm/config.go # azurerm/provider.go # vendor/vendor.json # website/azurerm.erb

…onitor to v11.2.2-beta

dominik-lekse · 2018-01-07T12:47:45Z

I have rebased the pull request to the latest master and also updated azure-sdk-for-go/arm/monitor to version v11.2.2-beta. However, the issue still exists.

tombuildsstuff · 2018-01-09T10:30:59Z

@dominik-lekse apologies for the delayed review here - I'm going to take a look at this today

On an unrelated note - would you be able to reach out via email when you get a moment? I'm [email protected]

Thanks!

dominik-lekse · 2018-01-11T10:51:17Z

According to @jhendrixMSFT, the JSON unmarshalling error will be fixed in the Azure Go SDK v12. I will pick up this pull request again when the provider has been upgraded to this version.

tombuildsstuff

Hey @dominik-lekse

Thanks for this PR - apologies for the delayed review of this!

I've taken a look through and left some comments in-line but this is off to a good start. Regarding upgrading to SDK v12, I'll be looking into this next - so hopefully we should have this in soon.

Thanks!

tombuildsstuff · 2018-01-09T10:19:23Z

azurerm/resource_arm_metric_alertrule.go

+				Default:  true,
+			},
+
+			"resource": {


I think this'd be clearer as resource_id or target_resource_id?

tombuildsstuff · 2018-01-09T11:09:07Z

website/docs/r/metric_alertrule.html.markdown

+
+`email_action` supports the following:
+
+* `service_owners` - (Optional) If true, the administrators (service and co-administrators) of the subscription are notified when the alert is triggered. Defaults to false.


can we quote the true and false values here to match the other docs?

tombuildsstuff · 2018-01-09T12:33:10Z

website/docs/r/metric_alertrule.html.markdown

+
+* `service_owners` - (Optional) If true, the administrators (service and co-administrators) of the subscription are notified when the alert is triggered. Defaults to false.
+
+* `custom_emails` - (Optional) A list of email addresses to be notified when the alert is triggered. Defaults to an empty list.


Can we remove the defaults to an empty list (since that's not necessarily true with it being computed)

tombuildsstuff · 2018-01-09T12:33:55Z

website/docs/r/metric_alertrule.html.markdown

+
+---
+
+* `email_action` - (Optional) An `email_action` block as defined below.


can we change "(Optional) An email_action block" -> "(Optional) A email_action block" for consistency with the other docs?

tombuildsstuff · 2018-01-09T12:34:54Z

website/docs/r/metric_alertrule.html.markdown

+
+* `description` - (Optional) A verbose description of the alert rule that will be included in the alert email.
+
+* `enabled` - (Optional) If true, the alert rule is enabled. Defaults to true.


can we quote the true and false values here for consistency?

tombuildsstuff · 2018-01-11T11:20:14Z

azurerm/resource_arm_metric_alertrule.go

+
+	alertRule := resp.AlertRule
+
+	if alertRule != nil {


minor we can actually in-line these as if alertRule := resp.AlertRule; alertRule != nil :)

tombuildsstuff · 2018-01-11T11:21:04Z

azurerm/resource_arm_metric_alertrule.go

+				metricDataSource, _ := dataSource.AsRuleMetricDataSource()
+
+				d.Set("resource", *metricDataSource.ResourceURI)
+				d.Set("metric_name", *metricDataSource.MetricName)


d.Set should handle setting the value from a pointer for us, so I don't believe we need the * on most of these values?

tombuildsstuff · 2018-01-11T11:22:54Z

azurerm/resource_arm_metric_alertrule.go

+			if emailAction, ok := ruleAction.AsRuleEmailAction(); ok {
+				email_action := make(map[string]interface{}, 1)
+
+				email_action["service_owners"] = *emailAction.SendToServiceOwners


(alas, that isn't true within a nested object that's assigning to a map)

given these values could potentially be nil for older Metric Alert Rules - could we make these:

if sendToOwners := emailAction.SendToServiceOwners; sendToOwners != nil { email_action["service_owners"] = *sendToOwners }

tombuildsstuff · 2018-01-11T11:24:24Z

azurerm/resource_arm_metric_alertrule.go

+
+	_, err = client.Delete(resGroup, name)
+
+	return err


can we change this to the following to handle displaying connection drops:

resp, err := client.Delete(resGroup, name) if resp != nil { if utils.ResponseWasNotFound(resp) { return nil } return fmt.Errorf("Error deleting Metric Alert Rule %q (resource group %q): %+v", name, resGroup, err) } return nil

tombuildsstuff · 2018-01-11T11:41:00Z

azurerm/validators.go

@@ -74,3 +74,20 @@ func validateStringLength(maxLength int) schema.SchemaValidateFunc {
 		return
 	}
 }
+
+func validateIso8601Duration() schema.SchemaValidateFunc {


could we add a unit test for this validation function?

# Conflicts: # vendor/vendor.json

…/monitor/mgmt/2017-05-01-preview/insights@=v12.2.0-beta

# Conflicts: # azurerm/config.go

dominik-lekse · 2018-01-16T11:22:09Z

Hi @tombuildsstuff, I picked up your review comments and resolved them with the recent changes. Also, I upgraded the SDK dependency to v12 which also solved the JSON unmarshalling error as @jhendrixMSFT promised. From my perspective, this is ready to merge.

Acceptance tests

=== RUN   TestAccAzureRMMetricAlertRule_importVirtualMachineCpu
--- PASS: TestAccAzureRMMetricAlertRule_importVirtualMachineCpu (505.97s)
=== RUN   TestAccAzureRMMetricAlertRule_virtualMachineCpu
--- PASS: TestAccAzureRMMetricAlertRule_virtualMachineCpu (539.93s)
=== RUN   TestAccAzureRMMetricAlertRule_sqlDatabaseStorage
--- PASS: TestAccAzureRMMetricAlertRule_sqlDatabaseStorage (193.84s)
PASS

Unit tests

make testacc TEST=./azurerm TESTARGS='-run=TestValidateIso8601Duration'

tombuildsstuff

Hey @dominik-lekse

Thanks for pushing those updates - I've taken a look through and left a couple of minor comments, but this otherwise LGTM. If we can rename the field service_owners to send_to_service_owners (to match Azure) then this should be good to merge 👍

Thanks!

tombuildsstuff · 2018-01-16T13:48:42Z

azurerm/resource_arm_metric_alertrule.go

+				Computed: true,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"service_owners": {


minor on reflection I think it might be worth making this send_to_service_owners?

tombuildsstuff · 2018-01-16T13:49:24Z

azurerm/resource_arm_metric_alertrule.go

+	location := d.Get("location").(string)
+	tags := d.Get("tags").(map[string]interface{})
+
+	alertRule, _ := expandAzureRmMetricThresholdAlertRule(d)


can we raise this error?

tombuildsstuff · 2018-01-16T13:49:51Z

azurerm/resource_arm_metric_alertrule.go

+		ruleCondition := alertRule.Condition
+
+		if ruleCondition != nil {
+			thresholdRuleCondition, _ := ruleCondition.AsThresholdRuleCondition()


can we raise this error (or remove it if it's not returned)?

The second return parameter of AsThresholdRuleCondition is always "true" according to /Users/lekse/Projects/terraform/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/Azure/azure-sdk-for-go/services/monitor/mgmt/2017-05-01-preview/insights/models.go:2251

tombuildsstuff · 2018-01-16T13:51:12Z

azurerm/resource_arm_metric_alertrule.go

+			thresholdRuleCondition, _ := ruleCondition.AsThresholdRuleCondition()
+
+			d.Set("operator", string(thresholdRuleCondition.Operator))
+			d.Set("threshold", float64(*thresholdRuleCondition.Threshold))


given this is a potential crash point, could we add a check around this? e.g.

if threshold := thresholdRuleCondition.Threshold; threshold != nil { d.Set("threshold", float64(*threshold)) }

tombuildsstuff · 2018-01-16T13:51:28Z

azurerm/resource_arm_metric_alertrule.go

+			dataSource := thresholdRuleCondition.DataSource
+
+			if dataSource != nil {
+				metricDataSource, _ := dataSource.AsRuleMetricDataSource()


(as above) can we raise this error, or remove it if it's not raised?

tombuildsstuff · 2018-01-16T13:51:55Z

azurerm/resource_arm_metric_alertrule.go

+			} else if webhookAction, ok := ruleAction.AsRuleWebhookAction(); ok {
+				webhook_action := make(map[string]interface{}, 1)
+
+				webhook_action["service_uri"] = *webhookAction.ServiceURI


can we add a nil-check around this?

tombuildsstuff · 2018-01-16T13:52:09Z

azurerm/resource_arm_metric_alertrule.go

+				webhook_action["service_uri"] = *webhookAction.ServiceURI
+
+				properties := make(map[string]string, len(*webhookAction.Properties))
+				for k, v := range *webhookAction.Properties {


can we add a nil-check around these lines incase properties is nil?

dominik-lekse · 2018-01-23T16:09:22Z

Hi @tombuildsstuff, the recent commit includes the changes based on your review. I did not rebase to the latest master this time since I assumed there will be more changes to the config.go before this pull request will be merged. Could you please resolve the conflicts in config.go shortly before you merge the pull request?

paultyng

Some minor issues with the import blocks.

paultyng · 2018-01-23T16:21:17Z

azurerm/resource_arm_metric_alertrule.go

+	"github.com/hashicorp/terraform/helper/schema"
+	"github.com/hashicorp/terraform/helper/validation"
+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+	"log"


could you fix the ordering here to match other files:

import ( "fmt" "log" "github...." "other imports" "these should all be alphabetical within their respective group" )

paultyng · 2018-01-23T16:25:58Z

azurerm/resource_arm_metric_alertrule_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/terraform"
+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+	"strconv"


this import should be with "fmt" and "testing"

tombuildsstuff

hey @dominik-lekse

Thanks for pushing those updates - this now LGTM, I'll push a commit to remove the commented out code (which should be no longer needed) and then kick off the test suite

Thanks!

tombuildsstuff · 2018-01-24T07:48:52Z

azurerm/resource_arm_metric_alertrule.go

+				}
+				//else {
+				//	properties = make(map[string]string, 0)
+				//}


I'll push a commit to remove this commented out code, since it's not needed

tombuildsstuff · 2018-01-24T07:48:56Z

azurerm/resource_arm_metric_alertrule.go

+
+				webhook_action["service_uri"] = *webhookAction.ServiceURI
+
+				//var properties map[string]string;


(same here)

tombuildsstuff · 2018-01-24T08:28:24Z

Tests pass:

requested changes have been made

tombuildsstuff · 2018-01-24T08:33:35Z

I've resolved the merge conflicts (by merging in the branch, since pushing a rebase to a fork which you don't have access to has historically broken PR's)

tombuildsstuff · 2018-01-24T08:42:29Z

@dominik-lekse thanks for this contribution - this now LGTM :)

dominik-lekse · 2018-01-24T09:30:26Z

@tombuildsstuff Thanks for merging this pull requests and resolving the conflicts. I left them intentionally since there were sdk migration pull requests pending.

ghost · 2020-03-31T15:39:43Z

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 👉 [email protected]. Thanks!

dominik-lekse added 3 commits October 31, 2017 16:01

Added vendor dependency for github.com/Azure/azure-sdk-for-go/arm/mon…

cf51bdd

…itor

Changed field AlertRule from an implicitly to an explicitly defined f…

307ba31

…ield to solve nil dereference issues when using the AlertRulesClient in azure-sdk-for-go/arm/monitor

Added resource azurerm_metric_alertrule

719ecfa

Added documentation for azurerm_metric_alertrule resource

58f792c

dominik-lekse changed the title ~~WIP: Add resource azurerm_metric_alertrule~~ Add resource azurerm_metric_alertrule Nov 5, 2017

tombuildsstuff added enhancement new-resource upstream labels Nov 6, 2017

tombuildsstuff changed the title ~~Add resource azurerm_metric_alertrule~~ New Resource: azurerm_metric_alertrule Dec 8, 2017

dominik-lekse added 2 commits January 7, 2018 13:31

Merge branch 'master' into public-feature/metric-alertrule

9e8595a

# Conflicts: # azurerm/config.go # azurerm/provider.go # vendor/vendor.json # website/azurerm.erb

Updated vendor dependency for github.com/Azure/azure-sdk-for-go/arm/m…

477f76a

…onitor to v11.2.2-beta

dominik-lekse mentioned this pull request Jan 7, 2018

[v11.2.2-beta] JSON unmarshalling error in monitor.AlertRuleResource Azure/azure-sdk-for-go#951

Closed

tombuildsstuff reviewed Jan 11, 2018

View reviewed changes

dominik-lekse added 6 commits January 15, 2018 17:31

Merge branch 'master' into public-feature/metric-alertrule

91c3d22

# Conflicts: # vendor/vendor.json

Added vendor dependency to github.com/Azure/azure-sdk-for-go/services…

09c1ac8

…/monitor/mgmt/2017-05-01-preview/insights@=v12.2.0-beta

Migrated resource metric alertrule to new monitor SDK

eab63bb

Removed old monitor SDK

5c5b351

Changes to metric_alertrule resource based on review

6feb073

Merge branch 'master' into public-feature/metric-alertrule

d4024f2

# Conflicts: # azurerm/config.go

Fixed example emails and urls in metric_alertrule documentation

7a279de

tombuildsstuff removed the upstream label Jan 16, 2018

tombuildsstuff mentioned this pull request Jan 19, 2018

refactoring appinsights use the new sdk version #718

Merged

tombuildsstuff reviewed Jan 22, 2018

View reviewed changes

tombuildsstuff added this to the 1.0.2 milestone Jan 22, 2018

tombuildsstuff assigned tombuildsstuff and unassigned tombuildsstuff Jan 22, 2018

tombuildsstuff added the waiting-response label Jan 22, 2018

Changes to metric_alertrule resource based on review

869ffaa

paultyng previously requested changes Jan 23, 2018

View reviewed changes

Fixed ordering of importes in metric_alertrule resource.

149f0ff

tombuildsstuff approved these changes Jan 24, 2018

View reviewed changes

Merge branch 'master' into public-feature/metric-alertrule

804b3cd

tombuildsstuff removed the waiting-response label Jan 24, 2018

Merge branch 'master' into public-feature/metric-alertrule

8510427

tombuildsstuff merged commit bbf16e9 into hashicorp:master Jan 24, 2018

tombuildsstuff added a commit that referenced this pull request Jan 24, 2018

Updating to include #478

d5250e2

tombuildsstuff mentioned this pull request Jan 24, 2018

azurerm_metric_alertrule - Refactoring/removing unused code #749

Merged

ghost locked and limited conversation to collaborators Mar 31, 2020


		`email_action` supports the following:

		* `service_owners` - (Optional) If true, the administrators (service and co-administrators) of the subscription are notified when the alert is triggered. Defaults to false.


		* `service_owners` - (Optional) If true, the administrators (service and co-administrators) of the subscription are notified when the alert is triggered. Defaults to false.

		* `custom_emails` - (Optional) A list of email addresses to be notified when the alert is triggered. Defaults to an empty list.


		---

		* `email_action` - (Optional) An `email_action` block as defined below.


		* `description` - (Optional) A verbose description of the alert rule that will be included in the alert email.

		* `enabled` - (Optional) If true, the alert rule is enabled. Defaults to true.


		webhook_action["service_uri"] = *webhookAction.ServiceURI

		//var properties map[string]string;

New Resource: azurerm_metric_alertrule #478

New Resource: azurerm_metric_alertrule #478

Conversation

dominik-lekse commented Oct 31, 2017 • edited Loading

Open tasks

Acceptance tests

Issues

Runtime error in AlertRulesClient in UnmarshalJSON

dominik-lekse commented Oct 31, 2017

dominik-lekse commented Nov 20, 2017

dominik-lekse commented Jan 7, 2018

tombuildsstuff commented Jan 9, 2018

dominik-lekse commented Jan 11, 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

dominik-lekse commented Jan 16, 2018

Acceptance tests

Unit tests

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

dominik-lekse commented Jan 23, 2018

paultyng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

tombuildsstuff commented Jan 24, 2018

tombuildsstuff commented Jan 24, 2018

tombuildsstuff commented Jan 24, 2018

dominik-lekse commented Jan 24, 2018

ghost commented Mar 31, 2020

New Resource: `azurerm_metric_alertrule` #478

New Resource: `azurerm_metric_alertrule` #478

dominik-lekse commented Oct 31, 2017 •

edited

Loading

Runtime error in `AlertRulesClient` in UnmarshalJSON