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.

Sign up for GitHub

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

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

Merged
merged 20 commits into from
Sep 30, 2022
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
32472bc
bugfix: Make the Azure Defender clause robust against a non-existent …
gzur Sep 27, 2022
8ac6c72
Merge branch 'master' of github.com:Azure/terraform-azurerm-aks into …
gzur Sep 28, 2022
58caf97
Leverage the terraform `locals` clause to unify analytics workspace l…
gzur Sep 28, 2022
9d06fa5
Ensure the existence of an analytics workspace enabling Microsoft Def…
gzur Sep 28, 2022
5fe6e64
terraform fmt
gzur Sep 28, 2022
7fbe2ed
Update docs to state that Microsoft Defender requires an Analytics Wo…
gzur Sep 28, 2022
eaab22d
Add link to analytics workspace var to defender var docs
gzur Sep 28, 2022
45e04d3
Close parentheses
gzur Sep 28, 2022
cd498f6
Merge branch 'Azure:master' into fix-disabled-log_analytics_workspace
gzur Sep 28, 2022
fb7f810
Respond to comments
gzur Sep 29, 2022
05ff54e
Apply changes made by automation suite
gzur Sep 29, 2022
f10a149
Remove unused variable. I
gzur Sep 29, 2022
38ac6d3
Revert "Remove unused variable."
gzur Sep 29, 2022
f393026
Instruct tflint to ignore unused variable
gzur Sep 29, 2022
63ffe5b
cleanup
gzur Sep 29, 2022
ae9e5d1
Refactor the Defender precondition expression
gzur Sep 29, 2022
4fde09a
Re-introduce `log_analytics_solution_id`
gzur Sep 29, 2022
19e15ed
Merge branch 'master' of github.com:Azure/terraform-azurerm-aks into …
gzur Sep 30, 2022
e2c9204
Re-introduce autogenerated documentation after merge
gzur Sep 30, 2022
e3016f2
Add control local variable for whether to create an analytics solutio…
gzur Sep 30, 2022
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 README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ No modules.
| <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 an Analytics Workspace (see: [`var.log_analytics_workspace_enabled`](#input_log_analytics_workspace_enabled)) | `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
43 changes: 36 additions & 7 deletions main.tf
View file
Open in desktop
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
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
Contributor Author

@gzur gzur Sep 29, 2022 •
edited
Loading

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
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
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
gzur marked this conversation as resolved.
Show resolved Hide resolved

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" {
for_each = var.microsoft_defender_enabled ? ["microsoft_defender"] : []

gzur marked this conversation as resolved.
Show resolved Hide resolved
for_each = (var.microsoft_defender_enabled && var.log_analytics_workspace_enabled) ? ["microsoft_defender"] : []
gzur marked this conversation as resolved.
Show resolved Hide resolved

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 == true && var.log_analytics_workspace_enabled == true) || var.microsoft_defender_enabled == false
gzur marked this conversation as resolved.
Show resolved Hide resolved
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
gzur marked this conversation as resolved.
Show resolved Hide 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
2 changes: 1 addition & 1 deletion variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,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 a Log Analytics Workspace."
gzur marked this conversation as resolved.
Show resolved Hide resolved
default = false
nullable = false
}
Expand Down