-
Notifications
You must be signed in to change notification settings - Fork 513
Improve identity related variables and output. #197
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens when you use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" { | ||
|
|
@@ -328,9 +330,14 @@ variable "ingress_application_gateway_subnet_id" { | |
| } | ||
|
|
||
| variable "identity_type" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also identity_type should be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| 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" { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,5 +7,5 @@ terraform { | |
| } | ||
| } | ||
|
|
||
| required_version = ">= 1.1.0" | ||
| required_version = ">= 1.2" | ||
| } | ||
There was a problem hiding this comment.
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 = falsein the variable definition ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identity_typehas a validation block,nullwon't pass the validation so I think it's ok for this case.