New Resource: `azurerm_log_analytics_solution` #952

lawrencegripper · 2018-03-08T12:15:09Z

This PR adds support for deploying a Log Analytics Soluion such as the Container Monitoring Solution.

Context:

The aim is to allow a user deploy a monitored k8s cluster in Azure using terraform automating this guide https://docs.microsoft.com/en-us/azure/aks/tutorial-kubernetes-monitor

It would look as follows in terraform:

Create an AKS Kubernetes cluster
Create a Log Analytics workspace
Create a container monitoring solution in the workspace
Use the Kuberentes resource to deploy the daemonset to forward logs to the solution (Adding output to the AKS resource is ongoing by @jjcollinge)

tombuildsstuff

hey @lawrencegripper

Thanks for this PR - I've taken a look through and left some comments in-line, but this is off to a good start; If we can fix up the remaining issues we should be good to run the tests and get this merged :)

Thanks!

tombuildsstuff · 2018-03-08T16:31:11Z

azurerm/resource_arm_log_analytics_solution.go

+
+	if product := plan["product"].(string); len(product) > 0 {
+		expandedPlan.Product = &product
+	}


given the way the annotations in the schema/structs is in the Azure SDK (omitempty, where empty values won't be posted to the API) - we should be safe to just assign these directly (and ignore any empty values) e.g.:

name := plan["name"].(string) publisher := plan["publisher"].(string) promotionCode := plan["promotion_code"].(string) product := plan["product"].(string) expandedPlan := operationsmanagement.SolutionPlan{ Name: utils.String(name), PromotionCode: utils.String(promotionCode) Publisher: utils.String(publisher), Product: utils.String(product), }

Looks much cleaner, I'll update - I was being over cautious as new to the terraform schema idea.

tombuildsstuff · 2018-03-08T16:36:12Z

azurerm/resource_arm_log_analytics_solution.go

+	plans := make([]interface{}, 0)
+	values := make(map[string]interface{})
+
+	values["product"] = *plan.Product


if we need this field, can we also set the name field here?

Sure, will do

tombuildsstuff · 2018-03-08T17:59:26Z

azurerm/resource_arm_log_analytics_solution.go

+	d.Set("name", resp.Name)
+	d.Set("location", resp.Location)
+	d.Set("resource_group_name", resGroup)
+	d.Set("plan", flattenAzureRmLogAnalyticsSolutionPlan(*resp.Plan))


a couple of things here:

given this is a complex type, can we check for errors when setting this?

can we use the value in resp.Properties / check for crashes here, rather than returning above. e.g.

this scenario is possible when importing legacy resources, since the Azure API's return the last known state of the resource, so they don't necessarily match the schema

if props := resp.Prope..; props != nil { if plan := props.Plan; plan != nil { if err := d.Set("plan", flattenAzureRmLogAnalyticsSolutionPlan(*plan)) { return fmt.Errorf("Error flattening `plan`: %+v`, err) } } }

also: could we also set the other fields here?

I've added nil checks and reversed the mapping used to extract "solution_name" and "workspace_name" although I'd gladly accept feedback if there is a nicer way to approach getting these values. I looked at using tags to round trip them but they don't seem to be supported on this resource.

tombuildsstuff · 2018-03-08T18:06:28Z

azurerm/resource_arm_log_analytics_solution.go

+		},
+
+		Schema: map[string]*schema.Schema{
+			"name": {


do we need to expose this field/is it going to be useful to users? if so - is it worth renaming this to something more appropriate like display_name to make it clearer?

Likely it isn't going to be useful due to the weird generated nature. Agree lets remove it.

tombuildsstuff · 2018-03-08T18:07:19Z

azurerm/resource_arm_log_analytics_solution.go

+
+			"workspace_resource_id": {
+				Type:     schema.TypeString,
+				Required: true,


can both workspace_name and workspace_resource_id be changed, or should they be ForceNew?

Good spot, will update.

tombuildsstuff · 2018-03-08T19:00:15Z

website/docs/r/log_analytics_solution.html.markdown

+
+The following arguments are supported:
+
+* `solution_name` - (Required) Specifies the name of the solution to be deployed. See [here for options](https://docs.microsoft.com/en-us/azure/log-analytics/log-analytics-add-solutions). Note: Resource tested with only `Container` solution. Changing this forces a new resource to be created.


Can we remove Note: Resource tested with only Container solution - we can add additional test-cases as needed

tombuildsstuff · 2018-03-08T19:01:58Z

website/docs/r/log_analytics_solution.html.markdown

+
+* `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created.
+
+* `workspace_resource_id` - (Required) The full resource ID of the Log Analytics workspace with which the solution will be linked. For example: `/subscriptions/00000000-0000-0000-0000-00000000/resourcegroups/examplegroupname/providers/Microsoft.OperationalInsights/workspaces/exampleWorkspaceName`


I think we should remove this example - users should really be using Interpolation where possible (e.g. ${azurerm_log_analytics_workspace.test.id} rather than specifying this separately - since it helps with dependency ordering (from a Terraform perspective) and ensures there's no casing inconsistency (from a users perspective)

tombuildsstuff · 2018-03-08T19:04:30Z

website/docs/r/log_analytics_solution.html.markdown

+
+* `plan.product` - (Required) The product name of the solution. For example `OMSGallery/Containers`. Changing this forces a new resource to be created.
+
+* `plan.promotion_code` - (Optional) A promotion code to be used with the solution.


Can we update this to match the convention for blocks e.g.:

* `plan` - A `plan` block as documented below. --- A `plan` block includes: * `publisher` - (Required) The publisher of the solution. For example `Microsoft`. Changing this forces a new resource to be created. * `product` - (Required) The product name of the solution. For example `OMSGallery/Containers`. Changing this forces a new resource to be created. * `promotion_code` - (Optional) The product name of the solution. For example `OMSGallery/Containers`. Changing this forces a new resource to be created.

tombuildsstuff · 2018-03-08T19:04:59Z

website/docs/r/log_analytics_solution.html.markdown

+
+The following attributes are exported:
+
+* `name` and `plan.name` - These are identical and are generated from the `plan.product` and the `workspace_resource_name`.


(as above) do we need to expose these fields / are they useful to users?

tombuildsstuff · 2018-03-08T19:06:48Z

vendor/vendor.json

+			"checksumSHA1": "WMfs+FCE3stapwrAvAS3fjFZlHk=",
+			"path": "github.com/Azure/azure-sdk-for-go/services/operationsmanagement/mgmt/2015-11-01-preview/operationsmanagement",
+			"revision": "21b68149ccf7c16b3f028bb4c7fd0ab458fe308f",
+			"revisionTime": "2018-02-12T16:31:56Z"


can we update this to include the version and versionExact fields as above (e.g. by vendoring the tags rather than the commit SHA). This can be done via:

govendor update github.com/Azure/azure-sdk-for-go/services/operationsmanagement/mgmt/2015-11-01-preview/operationsmanagement@=v12.5.0-beta

lawrencegripper · 2018-03-08T21:50:27Z

Thanks for the review, have some time tomorrow to fix these up.

_{Sent with GitHawk}

lawrencegripper · 2018-03-09T19:52:21Z

@tombuildsstuff Thanks again for the review - hopefully all set now.

tombuildsstuff

hey @lawrencegripper

Thanks for pushing those updates - this is looking really good. I've commented on a few minor things which I'll push a couple of commits to fix (I hope you don't mind!) - but this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2018-03-12T14:25:11Z

azurerm/resource_arm_log_analytics_solution.go

+						return true
+					}
+					return false
+				},


minor we can swap this for DiffSuppressFunc: ignoreCaseDiffSuppressFunc,

tombuildsstuff · 2018-03-12T14:44:46Z

azurerm/resource_arm_log_analytics_solution_test.go

+func testAccAzureRMLogAnalyticsSolution_containerMonitoring(rInt int, location string) string {
+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+	name     = "oms-acctestRG-%d"


can we make this acctestRG (we've got sweepers which clean up any left over resources using this)

tombuildsstuff · 2018-03-12T14:44:54Z

azurerm/resource_arm_log_analytics_solution_test.go

+func testAccAzureRMLogAnalyticsSolution_security(rInt int, location string) string {
+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+	name     = "oms-acctestRG-%d"


(same here)

tombuildsstuff · 2018-03-12T15:30:34Z

azurerm/resource_arm_log_analytics_solution.go

+		Delete: resourceArmLogAnalyticsSolutionDelete,
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,
+		},


we could also do with adding an Import test / some documentation here

tombuildsstuff · 2018-03-12T15:59:20Z

Tests pass:

lawrencegripper · 2018-03-12T17:26:32Z

Not at all, thanks for fixing those up :)

ghost · 2020-03-31T15:23:56Z

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!

lawrencegripper added 4 commits March 7, 2018 17:36

Initial work on Log Analytics solutions provider

9004404

Removed debug code from sdk. Worked around error in SDK return type

c7974e4

Created example and fixed issue with naming on create.

068bf39

Removed logging code

30afb16

lawrencegripper force-pushed the loganalytics_solution branch from 8ca4be7 to 30afb16 Compare March 8, 2018 13:07

Created docs for resource

051e77c

jjcollinge mentioned this pull request Mar 8, 2018

Add support for outputting kube config credentials from AKS #953

Merged

tombuildsstuff added new-resource service/oms labels Mar 8, 2018

tombuildsstuff suggested changes Mar 8, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Mar 8, 2018

Fixed review feedback. Added new basic test & example. Updated docs.

9db4725

lawrencegripper force-pushed the loganalytics_solution branch from f56a905 to 9db4725 Compare March 9, 2018 13:10

tombuildsstuff removed the waiting-response label Mar 12, 2018

tombuildsstuff approved these changes Mar 12, 2018

View reviewed changes

tombuildsstuff added 4 commits March 12, 2018 10:36

Documenting import support for Log Analytics Workspaces & Solutions

fd0d8fe

Adding a sidebar entry for OMS Solutions

912a268

Refactoring to use common functions / fixing the tests names/spacing

6ef4b9c

Adding an import test

fcd5b64

tombuildsstuff changed the title ~~Add Log Analytics Solution Provider to support "Container Monitoring Solution"~~ New Resource: azurerm_log_analytics_solution Mar 12, 2018

tombuildsstuff added this to the 1.3.0 milestone Mar 12, 2018

tombuildsstuff merged commit 265f47e into hashicorp:master Mar 12, 2018

tombuildsstuff added a commit that referenced this pull request Mar 12, 2018

Updating to include #952

b39b0e7

tombuildsstuff added a commit that referenced this pull request Mar 13, 2018

Updating to include #952

2d45bac

lawrencegripper mentioned this pull request Mar 19, 2018

Operations Management client problems Azure/azure-sdk-for-go#1333

Closed

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_log_analytics_solution` #952

New Resource: `azurerm_log_analytics_solution` #952

lawrencegripper commented Mar 8, 2018

tombuildsstuff left a comment

tombuildsstuff Mar 8, 2018

lawrencegripper Mar 9, 2018 •

edited

Loading

tombuildsstuff Mar 8, 2018

lawrencegripper Mar 9, 2018

tombuildsstuff Mar 8, 2018

tombuildsstuff Mar 8, 2018

lawrencegripper Mar 9, 2018

tombuildsstuff Mar 8, 2018

lawrencegripper Mar 9, 2018

tombuildsstuff Mar 8, 2018

lawrencegripper Mar 9, 2018

tombuildsstuff Mar 8, 2018

tombuildsstuff Mar 8, 2018

tombuildsstuff Mar 8, 2018

tombuildsstuff Mar 8, 2018

tombuildsstuff Mar 8, 2018

lawrencegripper commented Mar 8, 2018 •

edited

Loading

lawrencegripper commented Mar 9, 2018

tombuildsstuff left a comment

tombuildsstuff Mar 12, 2018

tombuildsstuff Mar 12, 2018

tombuildsstuff Mar 12, 2018

tombuildsstuff Mar 12, 2018

tombuildsstuff Mar 12, 2018

tombuildsstuff Mar 12, 2018

tombuildsstuff Mar 12, 2018

tombuildsstuff Mar 12, 2018

tombuildsstuff commented Mar 12, 2018

lawrencegripper commented Mar 12, 2018

ghost commented Mar 31, 2020


		The following arguments are supported:

		* `solution_name` - (Required) Specifies the name of the solution to be deployed. See [here for options](https://docs.microsoft.com/en-us/azure/log-analytics/log-analytics-add-solutions). Note: Resource tested with only `Container` solution. Changing this forces a new resource to be created.


		* `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created.

		* `workspace_resource_id` - (Required) The full resource ID of the Log Analytics workspace with which the solution will be linked. For example: `/subscriptions/00000000-0000-0000-0000-00000000/resourcegroups/examplegroupname/providers/Microsoft.OperationalInsights/workspaces/exampleWorkspaceName`


		* `plan.product` - (Required) The product name of the solution. For example `OMSGallery/Containers`. Changing this forces a new resource to be created.

		* `plan.promotion_code` - (Optional) A promotion code to be used with the solution.


		The following attributes are exported:

		* `name` and `plan.name` - These are identical and are generated from the `plan.product` and the `workspace_resource_name`.

New Resource: azurerm_log_analytics_solution #952

New Resource: azurerm_log_analytics_solution #952

Conversation

lawrencegripper commented Mar 8, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrencegripper Mar 9, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrencegripper commented Mar 8, 2018 • edited Loading

lawrencegripper commented Mar 9, 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

tombuildsstuff commented Mar 12, 2018

lawrencegripper commented Mar 12, 2018

ghost commented Mar 31, 2020

New Resource: `azurerm_log_analytics_solution` #952

New Resource: `azurerm_log_analytics_solution` #952

lawrencegripper Mar 9, 2018 •

edited

Loading

lawrencegripper commented Mar 8, 2018 •

edited

Loading