Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 12 additions & 1 deletion main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,19 @@ resource "azurerm_kubernetes_cluster" "main" {
oidc_issuer_enabled = var.oidc_issuer_enabled

tags = var.tags
}

lifecycle {
precondition {
condition = (var.client_id != "" && var.client_secret != "") || (var.identity_type != "")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

identity_type should have nullable = false in the variable definition ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

identity_type has a validation block, null won't pass the validation so I think it's ok for this case.

error_message = "Either `client_id` and `client_secret` or `identity_type` must be set."
}
precondition {
# Why don't use var.identity_ids != null && length(var.identity_ids)>0 ? Because bool expression in Terraform is not short circuit so even var.identity_ids is null Terraform will still invoke length function with null and cause error. https://github.com/hashicorp/terraform/issues/24128
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."
}
}
}

resource "azurerm_log_analytics_workspace" "main" {
count = var.enable_log_analytics_workspace ? 1 : 0
Expand Down
4 changes: 2 additions & 2 deletions outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ output "http_application_routing_zone_name" {
value = azurerm_kubernetes_cluster.main.http_application_routing_zone_name != null ? azurerm_kubernetes_cluster.main.http_application_routing_zone_name : ""
}

output "system_assigned_identity" {
value = azurerm_kubernetes_cluster.main.identity
output "cluster_identity" {
value = try(azurerm_kubernetes_cluster.main.identity[0], null)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the try block ? Is there a condition when the cluster has null identity ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we output in the value only the first element, rather than making available the all list ?

If we export the all list the user of the module can use the output with a index and consume all the identities of the cluster in other resources.

This would be also consistent with how we export the kubelet identity, example:
https://github.com/zioproto/istio-aks-example/blob/main/registry.tf#L10

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi, according to the resource schema and identity schema it referenced, the identity in aks resource is a list with maximum length 1. So in this case the identity list has three cases: null, an empty list and a list with exactly one item.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens when you use SystemAssigned, UserAssigned ? Dont you have 2 cluster identities ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we also patch the CHANGLOG.md to indicate that the output name changed, so folks using the module must update their code ?

}

output "kubelet_identity" {
Expand Down
2 changes: 1 addition & 1 deletion test/fixture/outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ output "test_aks_without_monitor_id" {
}

output "test_aks_without_monitor_identity" {
value = module.aks_without_monitor.system_assigned_identity
value = module.aks_without_monitor.cluster_identity
}

output "test_admin_client_key" {
Expand Down
9 changes: 8 additions & 1 deletion variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ variable "client_id" {
description = "(Optional) The Client ID (appId) for the Service Principal used for the AKS deployment"
type = string
default = ""
nullable = false
}

variable "client_secret" {
description = "(Optional) The Client Secret (password) for the Service Principal used for the AKS deployment"
type = string
default = ""
nullable = false
}

variable "admin_username" {
Expand Down Expand Up @@ -328,9 +330,14 @@ variable "ingress_application_gateway_subnet_id" {
}

variable "identity_type" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also identity_type should be nullable = false to make sure the condition in main.tf L158 works all the times.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

identity_type has a validation block, null won't pass the validation so I think it's ok for this case.

description = "(Optional) The type of identity used for the managed cluster. Conflict with `client_id` and `client_secret`. Possible values are `SystemAssigned` and `UserAssigned`. If `UserAssigned` is set, a `user_assigned_identity_id` must be set as well."
description = "(Optional) The type of identity used for the managed cluster. Conflict with `client_id` and `client_secret`. Possible values are `SystemAssigned`, `UserAssigned`, `SystemAssigned, UserAssigned`(to enable both). If `UserAssigned` or `SystemAssigned, UserAssigned` is set, an `identity_ids` must be set as well."
type = string
default = "SystemAssigned"

validation {
condition = var.identity_type == "SystemAssigned" || var.identity_type == "UserAssigned" || var.identity_type == "SystemAssigned, UserAssigned"
error_message = "`identity_type`'s possible values are `SystemAssigned`, `UserAssigned`, `SystemAssigned, UserAssigned`(to enable both)."
}
}

variable "identity_ids" {
Expand Down
2 changes: 1 addition & 1 deletion versions.tf
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ terraform {
}
}

required_version = ">= 1.1.0"
required_version = ">= 1.2"
}