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

New Resource: azurerm_log_analytics_solution #952

Merged
merged 10 commits into from
Mar 12, 2018

Conversation

lawrencegripper
Copy link
Contributor

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:

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

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

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!


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

Choose a reason for hiding this comment

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

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),
}

Copy link
Contributor Author

@lawrencegripper lawrencegripper Mar 9, 2018

Choose a reason for hiding this comment

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

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

plans := make([]interface{}, 0)
values := make(map[string]interface{})

values["product"] = *plan.Product
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do

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

Choose a reason for hiding this comment

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

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)
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

also: could we also set the other fields here?

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'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.

},

Schema: map[string]*schema.Schema{
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


"workspace_resource_id": {
Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot, will update.


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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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


* `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`
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 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)


* `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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


The following attributes are exported:

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

Choose a reason for hiding this comment

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

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

"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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

lawrencegripper commented Mar 8, 2018

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

Sent with GitHawk

@lawrencegripper
Copy link
Contributor Author

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

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

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!

return true
}
return false
},
Copy link
Contributor

Choose a reason for hiding this comment

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

minor we can swap this for DiffSuppressFunc: ignoreCaseDiffSuppressFunc,

Copy link
Contributor

Choose a reason for hiding this comment

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

(done)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(done)

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

Choose a reason for hiding this comment

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

(same here)

Copy link
Contributor

Choose a reason for hiding this comment

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

(done)

Delete: resourceArmLogAnalyticsSolutionDelete,
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(done)

@tombuildsstuff
Copy link
Contributor

Tests pass:

screen shot 2018-03-12 at 10 59 02

@tombuildsstuff tombuildsstuff changed the title Add Log Analytics Solution Provider to support "Container Monitoring Solution" New Resource: azurerm_log_analytics_solution Mar 12, 2018
@tombuildsstuff tombuildsstuff added this to the 1.3.0 milestone Mar 12, 2018
@tombuildsstuff tombuildsstuff merged commit 265f47e into hashicorp:master Mar 12, 2018
tombuildsstuff added a commit that referenced this pull request Mar 12, 2018
@lawrencegripper
Copy link
Contributor Author

Not at all, thanks for fixing those up :)

@ghost
Copy link

ghost commented Mar 31, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants