Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ ENHANCEMENTS:

* Add `aci_connector_linux` addon. [#230](https://github.com/Azure/terraform-azurerm-aks/pull/230)
* Restrict Terraform Core version for example cod to `>= 1.2`. [#253](https://github.com/Azure/terraform-azurerm-aks/pull/253)
* Adds support for Ultra Disks by enabling the option. [#245](https://github.com/Azure/terraform-azurerm-aks/pull/245)
* Adds support for Ultra Disks by enabling the option. [#245](https://github.com/Azure/terraform-azurerm-aks/pull/245)
3 changes: 1 addition & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -305,13 +305,12 @@ No modules.
| <a name="input_kubernetes_version"></a> [kubernetes\_version](#input\_kubernetes\_version) | Specify which Kubernetes release to use. The default used is the latest Kubernetes version available in the region | `string` | `null` | no |
| <a name="input_local_account_disabled"></a> [local\_account\_disabled](#input\_local\_account\_disabled) | (Optional) - If `true` local accounts will be disabled. Defaults to `false`. See [the documentation](https://docs.microsoft.com/azure/aks/managed-aad#disable-local-accounts) for more information. | `bool` | `null` | no |
| <a name="input_location"></a> [location](#input\_location) | Location of cluster, if not defined it will be read from the resource-group | `string` | `null` | no |
| <a name="input_log_analytics_solution_id"></a> [log\_analytics\_solution\_id](#input\_log\_analytics\_solution\_id) | (Optional) Existing azurerm\_log\_analytics\_solution ID. Providing ID disables creation of azurerm\_log\_analytics\_solution. | `string` | `null` | no |
| <a name="input_log_analytics_workspace"></a> [log\_analytics\_workspace](#input\_log\_analytics\_workspace) | (Optional) Existing azurerm\_log\_analytics\_workspace to attach azurerm\_log\_analytics\_solution. Providing the config disables creation of azurerm\_log\_analytics\_workspace. | <pre>object({<br> id = string<br> name = string<br> })</pre> | `null` | no |
| <a name="input_log_analytics_workspace_enabled"></a> [log\_analytics\_workspace\_enabled](#input\_log\_analytics\_workspace\_enabled) | Enable the integration of azurerm\_log\_analytics\_workspace and azurerm\_log\_analytics\_solution: https://docs.microsoft.com/en-us/azure/azure-monitor/containers/container-insights-onboard | `bool` | `true` | no |
| <a name="input_log_analytics_workspace_resource_group_name"></a> [log\_analytics\_workspace\_resource\_group\_name](#input\_log\_analytics\_workspace\_resource\_group\_name) | (Optional) Resource group name to create azurerm\_log\_analytics\_solution. | `string` | `null` | no |
| <a name="input_log_analytics_workspace_sku"></a> [log\_analytics\_workspace\_sku](#input\_log\_analytics\_workspace\_sku) | The SKU (pricing level) of the Log Analytics workspace. For new subscriptions the SKU should be set to PerGB2018 | `string` | `"PerGB2018"` | no |
| <a name="input_log_retention_in_days"></a> [log\_retention\_in\_days](#input\_log\_retention\_in\_days) | The retention period for the logs in days | `number` | `30` | no |
| <a name="input_microsoft_defender_enabled"></a> [microsoft\_defender\_enabled](#input\_microsoft\_defender\_enabled) | (Optional) Is Microsoft Defender on the cluster enabled? | `bool` | `false` | no |
| <a name="input_microsoft_defender_enabled"></a> [microsoft\_defender\_enabled](#input\_microsoft\_defender\_enabled) | (Optional) Is Microsoft Defender on the cluster enabled? Requires `var.log_analytics_workspace_enabled` to be `true` to set this variable to `true`. | `bool` | `false` | no |
| <a name="input_net_profile_dns_service_ip"></a> [net\_profile\_dns\_service\_ip](#input\_net\_profile\_dns\_service\_ip) | (Optional) IP address within the Kubernetes service address range that will be used by cluster service discovery (kube-dns). Changing this forces a new resource to be created. | `string` | `null` | no |
| <a name="input_net_profile_docker_bridge_cidr"></a> [net\_profile\_docker\_bridge\_cidr](#input\_net\_profile\_docker\_bridge\_cidr) | (Optional) IP address (in CIDR notation) used as the Docker bridge IP address on nodes. Changing this forces a new resource to be created. | `string` | `null` | no |
| <a name="input_net_profile_outbound_type"></a> [net\_profile\_outbound\_type](#input\_net\_profile\_outbound\_type) | (Optional) The outbound (egress) routing method which should be used for this Kubernetes Cluster. Possible values are loadBalancer and userDefinedRouting. Defaults to loadBalancer. | `string` | `"loadBalancer"` | no |
Expand Down
41 changes: 35 additions & 6 deletions main.tf
Original file line number Diff line number Diff line change
@@ -1,3 +1,27 @@
locals {

# Abstract the decision whether to create an Analytics Workspace or not.
create_analytics_workspace = var.log_analytics_workspace_enabled && var.log_analytics_workspace == null

# Abstract the decision whether to use an Analytics Workspace supplied via vars, provision one ourselves or leave it null.
# This guarantees that local.log_analytics_workspace will contain a valid `id` and `name` IFF log_analytics_workspace_enabled
# is set to `true`.
log_analytics_workspace = var.log_analytics_workspace_enabled ? (
# The Log Analytics Workspace should be enabled:
var.log_analytics_workspace == null ? {
# `log_analytics_workspace_enabled` is `true` but `log_analytics_workspace` was not supplied.
# Create an `azurerm_log_analytics_workspace` resource and use that.
id = azurerm_log_analytics_workspace.main[0].id
name = azurerm_log_analytics_workspace.main[0].name
} : {
# `log_analytics_workspace` is supplied. Let's use that.
id = var.log_analytics_workspace.id
name = var.log_analytics_workspace.name
}
) : null # Finally, the Log Analytics Workspace should be disabled.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would an object with null field be better (the caller don't need to check whether this object is null)?

) : {
  id       = null
  name = null
}

Copy link
Copy Markdown
Contributor Author

@gzur gzur Sep 29, 2022

Choose a reason for hiding this comment

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

But won't the caller just have to check whether local.log_analytics_workspace.id/name is null instead?

I feel that this breaks the Principle of Least Astonishment, since the module will have already decided that there is no log_analytics_workspace - based on the inputs.

In this scenario, I would not expect there to be an object of that type but with attributes as null.
I would expect the object itself to be null.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In Terraform assign null to an argument is equal to omit this argument, in some cases the caller doesn't need to check whether the log analytics workspace id is null or not, if the caller just assigns it to an optional argument, in that case we can save a null check. I personally prefer this "null safe" style, but I think your point also make sense. Please allow me to have more discuss with other people, thanks for your understanding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

did we reach a consensus on:

) : {
  id       = null
  name = null
}

vs.

) : null

?


} # /end locals clause
Comment thread
gzur marked this conversation as resolved.
Outdated

data "azurerm_resource_group" "main" {
name = var.resource_group_name
}
Expand Down Expand Up @@ -150,10 +174,11 @@ resource "azurerm_kubernetes_cluster" "main" {
}
}
dynamic "microsoft_defender" {

Comment thread
gzur marked this conversation as resolved.
for_each = var.microsoft_defender_enabled ? ["microsoft_defender"] : []

content {
log_analytics_workspace_id = coalesce(try(var.log_analytics_workspace.id, null), azurerm_log_analytics_workspace.main[0].id)
log_analytics_workspace_id = local.log_analytics_workspace.id
}
}
network_profile {
Expand All @@ -169,7 +194,7 @@ resource "azurerm_kubernetes_cluster" "main" {
for_each = var.log_analytics_workspace_enabled ? ["oms_agent"] : []

content {
log_analytics_workspace_id = var.log_analytics_workspace == null ? azurerm_log_analytics_workspace.main[0].id : var.log_analytics_workspace.id
log_analytics_workspace_id = local.log_analytics_workspace.id
}
}
dynamic "service_principal" {
Expand All @@ -191,11 +216,15 @@ resource "azurerm_kubernetes_cluster" "main" {
condition = (var.client_id != "" && var.client_secret != "") || (var.identity_type == "SystemAssigned") || (var.identity_ids == null ? false : length(var.identity_ids) > 0)
error_message = "If use identity and `UserAssigned` or `SystemAssigned, UserAssigned` is set, an `identity_ids` must be set as well."
}
precondition {
condition = !var.microsoft_defender_enabled || var.log_analytics_workspace_enabled
Comment thread
gzur marked this conversation as resolved.
Outdated
error_message = "Enabling Microsoft Defender requires that `log_analytics_workspace_enabled` be set to true."
}
}
}

resource "azurerm_log_analytics_workspace" "main" {
count = var.log_analytics_workspace_enabled && var.log_analytics_workspace == null ? 1 : 0
count = local.create_analytics_workspace ? 1 : 0

location = coalesce(var.location, data.azurerm_resource_group.main.location)
name = var.cluster_log_analytics_workspace_name == null ? "${var.prefix}-workspace" : var.cluster_log_analytics_workspace_name
Expand All @@ -206,13 +235,13 @@ resource "azurerm_log_analytics_workspace" "main" {
}

resource "azurerm_log_analytics_solution" "main" {
count = var.log_analytics_workspace_enabled && var.log_analytics_solution_id == null ? 1 : 0
Comment thread
gzur marked this conversation as resolved.
count = local.create_analytics_workspace ? 1 : 0

location = coalesce(var.location, data.azurerm_resource_group.main.location)
resource_group_name = coalesce(var.log_analytics_workspace_resource_group_name, var.resource_group_name)
solution_name = "ContainerInsights"
workspace_name = var.log_analytics_workspace != null ? var.log_analytics_workspace.name : azurerm_log_analytics_workspace.main[0].name
workspace_resource_id = var.log_analytics_workspace != null ? var.log_analytics_workspace.id : azurerm_log_analytics_workspace.main[0].id
workspace_name = local.log_analytics_workspace.name
workspace_resource_id = local.log_analytics_workspace.id
tags = var.tags

plan {
Expand Down
10 changes: 1 addition & 9 deletions variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -227,14 +227,6 @@ variable "location" {
description = "Location of cluster, if not defined it will be read from the resource-group"
default = null
}

variable "log_analytics_solution_id" {
Comment thread
gzur marked this conversation as resolved.
type = string
description = "(Optional) Existing azurerm_log_analytics_solution ID. Providing ID disables creation of azurerm_log_analytics_solution."
default = null
nullable = true
}

variable "log_analytics_workspace" {
type = object({
id = string
Expand Down Expand Up @@ -273,7 +265,7 @@ variable "log_retention_in_days" {

variable "microsoft_defender_enabled" {
type = bool
description = "(Optional) Is Microsoft Defender on the cluster enabled?"
description = "(Optional) Is Microsoft Defender on the cluster enabled? Requires `var.log_analytics_workspace_enabled` to be `true` to set this variable to `true`."
default = false
nullable = false
}
Expand Down