feat: Support existing log analytics workspace (#211) by viters · Pull Request #213 · Azure/terraform-azurerm-aks

viters · 2022-07-06T19:46:22Z

Fixes #211

Changes proposed in the pull request:

Provides a way to attach existing Log Analytics Workspace to AKS through Container Insights (azurerm_log_analytics_solution).

ghost · 2022-07-06T19:46:36Z

All CLA requirements met.

lonegunmanb

Hello @viters , thanks for submitting this pr. Almost LGTM but only one question.

lonegunmanb · 2022-07-07T09:06:14Z

  resource_group_name   = var.resource_group_name
-  workspace_resource_id = azurerm_log_analytics_workspace.main[0].id
-  workspace_name        = azurerm_log_analytics_workspace.main[0].name
+  workspace_resource_id = var.log_analytics_workspace == null ? azurerm_log_analytics_workspace.main[0].id : var.log_analytics_workspace.id


Hi, do we need add this var.log_analytics_workspace == null criteria to azurerm_log_analytics_solution's count expression, since the log analytics resource is injected by the module caller？

azurerm_log_analytics_solution is ContainerInsights solution connecting newly created AKS and existing (or just created) Workspace. I think that current count expression var.enable_log_analytics_workspace ? 1 : 0 is valid. If caller specifies enable_log_analytics_workspace = true then caller either wants to create a new Workspace or use existing. In other words ContainerInsights solution is created always when enable_log_analytics_workspace = true, but the created solution can be connected to different Workspaces, which is solved later by conditions on workspace_resource_id and workspace_name.

I have a question though, because I checked in azurerm_log_analytics_solution docs that:

In my current solution, existing Workspace var.log_analytics_workspace must be placed in the same RG as var.resource_group_name. Do you think it is valid assumption? It was valid in my use case.

Otherwise I can add resource_group_name field to var.log_analytics_workspace. It would mean that caller can create azurerm_log_analytics_solution in RG different than AKS resource.

azurerm_log_analytics_solution is ContainerInsights solution connecting newly created AKS and existing (or just created) Workspace. I think that current count expression var.enable_log_analytics_workspace ? 1 : 0 is valid. If caller specifies enable_log_analytics_workspace = true then caller either wants to create a new Workspace or use existing. In other words ContainerInsights solution is created always when enable_log_analytics_workspace = true, but the created solution can be connected to different Workspaces, which is solved later by conditions on workspace_resource_id and workspace_name.

My concern is what if the caller has used another module that creates log analytics domain resources? It looks like we need both workspace and solution to make log work, so maybe we should treat them as an inseparable entity?

I have a question though, because I checked in azurerm_log_analytics_solution docs that:

In my current solution, existing Workspace var.log_analytics_workspace must be placed in the same RG as var.resource_group_name. Do you think it is valid assumption? It was valid in my use case.

Otherwise I can add resource_group_name field to var.log_analytics_workspace. It would mean that caller can create azurerm_log_analytics_solution in RG different than AKS resource.

Good question! I don't know whether it's valid to use different resource groups for aks cluster and log analytics workspace.

I've verified that the workspace can be put in different rg, but must be in the same rg with solution, so I prefer we add this var.log_analytics_workspace == null criteria to solution's count expression, leave the choice to the caller.

I do not get why should we add

var.log_analytics_workspace == null criteria to solution's count expression

The idea of that feature is not to use existing azurerm_log_analytics_solution, but to use existing azurerm_log_analytics_workspace. We need to create azurerm_log_analytics_solution irregardless of var.log_analytics_workspace value. I do not understand why var.log_analytics_workspace == null should be a part of criteria to solution's count expression.

I've verified that the workspace can be put in different rg, but must be in the same rg with solution

Awesome! I will add a way to set Workspace/Solution RG then.

Hi @viters my concern is can our module be composited with other modules easily. When you opened this pr I do agree with you because what if the caller want to use another module to manage log analytics related resources? Your pr makes it possible to avoid a duplicated workspace and leaves the choice to the caller.

So what if the caller has a module that manage all log analytics related resources, even in a standalone repo along with its own state backend? There's no way for the caller to avoid bind the workspace to a solution that he doesn't want.

I think you pr's value is giving callers the right of composition, so can we give the caller more? Thanks!

@lonegunmanb
Oh, I got it. Your idea is to add a way to use existing azurerm_log_analytics_solution and skip creation of it as well! I understand now, I will get to it!

@lonegunmanb
As we discussed, I've added var.log_analytics_solution_id and azurerm_log_analytics_solution resource will not be created if var.log_analytics_solution_id != null.

I've used string id instead of bool to avoid breaking changes in future - we do not need the solution ID now, but it may be required in future Azure API somewhere, e.g. in oms_agent block.

…#211)

viters · 2022-07-07T18:21:26Z

@lonegunmanb
I've added a support for Log Analytics Workspace (both existing and newly created) with Container Insights Solution to be created in different RG than AKS.

lonegunmanb · 2022-07-08T01:23:27Z

  name                = var.cluster_log_analytics_workspace_name == null ? "${var.prefix}-workspace" : var.cluster_log_analytics_workspace_name
  location            = coalesce(var.location, data.azurerm_resource_group.main.location)
-  resource_group_name = var.resource_group_name
+  resource_group_name = var.log_analytics_workspace_resource_group_name != null ? var.log_analytics_workspace_resource_group_name : var.resource_group_name


Would coalesce(var.log_analytics_workspace_resource_group_name, var.resource_group_name) here be better?

Yes, I will check how coalesce works and update!

@lonegunmanb Used coalesce as you suggested!

lonegunmanb · 2022-07-08T01:23:45Z

-  resource_group_name   = var.resource_group_name
-  workspace_resource_id = azurerm_log_analytics_workspace.main[0].id
-  workspace_name        = azurerm_log_analytics_workspace.main[0].name
+  resource_group_name   = var.log_analytics_workspace_resource_group_name != null ? var.log_analytics_workspace_resource_group_name : var.resource_group_name


Would coalesce(var.log_analytics_workspace_resource_group_name, var.resource_group_name) here be better?

Yes, I will check how coalesce works and update!

@lonegunmanb Used coalesce as you suggested!

lonegunmanb · 2022-07-08T01:33:11Z

  resource_group_name   = var.resource_group_name
-  workspace_resource_id = azurerm_log_analytics_workspace.main[0].id
-  workspace_name        = azurerm_log_analytics_workspace.main[0].name
+  workspace_resource_id = var.log_analytics_workspace == null ? azurerm_log_analytics_workspace.main[0].id : var.log_analytics_workspace.id


Hi @viters my concern is can our module be composited with other modules easily. When you opened this pr I do agree with you because what if the caller want to use another module to manage log analytics related resources? Your pr makes it possible to avoid a duplicated workspace and leaves the choice to the caller.

So what if the caller has a module that manage all log analytics related resources, even in a standalone repo along with its own state backend? There's no way for the caller to avoid bind the workspace to a solution that he doesn't want.

I think you pr's value is giving callers the right of composition, so can we give the caller more? Thanks!

lonegunmanb

Thanks @viters, LGTM!

feat: Support existing log analytics workspace (Azure#211)

f7b9aa0

lonegunmanb requested changes Jul 7, 2022

View reviewed changes

viters added 3 commits July 7, 2022 20:07

feat: Support existing log analytics workspace in different RG (Azure…

9d63d59

…#211)

feat: Support existing log analytics workspace in different RG (Azure…

48163d8

…#211)

feat: Support log analytics workspace in different RG (Azure#211)

7d7240f

lonegunmanb reviewed Jul 8, 2022

View reviewed changes

feat: Support existing log analytics solution (Azure#211)

143cf6e

lonegunmanb approved these changes Jul 8, 2022

View reviewed changes

lonegunmanb merged commit d207a2a into Azure:master Jul 8, 2022

gzur mentioned this pull request Sep 29, 2022

bugfix: Make the Azure Defender clause robust against a non-existent … #258

Merged

Conversation

viters commented Jul 6, 2022

Uh oh!

ghost commented Jul 6, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viters Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lonegunmanb Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viters Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viters Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viters Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viters commented Jul 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lonegunmanb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghost commented Jul 6, 2022 •

edited by ghost

Loading

viters Jul 7, 2022 •

edited

Loading

lonegunmanb Jul 7, 2022 •

edited

Loading

viters Jul 7, 2022 •

edited

Loading

viters Jul 8, 2022 •

edited

Loading

viters Jul 8, 2022 •

edited

Loading