Added support for load_balancer_profile#277
Conversation
|
@microsoft-github-policy-service agree |
moved load_balancer_profile and load_balancer_sku under network
| type = bool | ||
| description = "(Optional) A load_balancer_profile block. This can only be specified when load_balancer_sku is set to standard." | ||
| default = null | ||
| nullable = true |
There was a problem hiding this comment.
nit: if the default is null you can omit the nullable = true.
asking @lonegunmanb for confirmation on this style issue
There was a problem hiding this comment.
Agreed, could you change the default to false here?
|
@lonegunmanb can we please start the E2E tests for this PR ? it looks good for testing General Question: what is the best option to test this functionality in the E2E tests ? should we patch an existing one or creating a new one to test just this feature ? Thanks |
lonegunmanb
left a comment
lonegunmanb
left a comment
There was a problem hiding this comment.
Hello @mazilu88, thanks for opening this pr. I've some comments for you, and most of your new added variables are for the locad_balancer_profile block, could we add a locad_balancer_profile_ prefix to all these variables' name? For example, the nat_gateway_profile block also has an argument named idle_timeout_in_minutes, once we want to support this nat_gateway_profile block we might meet a conflict with this pr.
Another thought is whether we should use an object type for load_balancer_profile variable and include all related variables as it's fields. The cons of this object solution is, we cannot write clear descriptions for the object's fields.
We can add prefix to there variables' names, or we can make them an object with the help of Optional Object Type Attributes, and that would require Terraform 1.3+.
| type = bool | ||
| description = "(Optional) A load_balancer_profile block. This can only be specified when load_balancer_sku is set to standard." | ||
| default = null | ||
| nullable = true |
There was a problem hiding this comment.
Agreed, could you change the default to false here?
| default = null | ||
| } | ||
|
|
||
| variable "load_balancer_profile" { |
There was a problem hiding this comment.
Would load_balancer_enabled be better here?
|
|
||
| variable "load_balancer_profile" { | ||
| type = bool | ||
| description = "(Optional) A load_balancer_profile block. This can only be specified when load_balancer_sku is set to standard." |
There was a problem hiding this comment.
This bool variable doesn't seem like a block, could we change a better description here?
Could we also add a precondition block in aks resource to enforce this pre condition?
|
|
||
| variable "load_balancer_sku" { | ||
| type = string | ||
| description = "(Optional) Specifies the SKU of the Load Balancer used for this Kubernetes Cluster. Possible values are basic and standard. Defaults to standard." |
There was a problem hiding this comment.
According to the provider code the load_balancer_sku argument is a ForceNew argument, which means that if we change this variable's value, the aks cluster would be re-created. The official document need to be upgrade, could we append Changing this forces a new kubernetes cluster to be created. to the description?
|
|
||
| validation { | ||
| condition = var.load_balancer_sku == "basic" || var.load_balancer_sku == "standard" | ||
| error_message = "Possible values are basic and standard" |
There was a problem hiding this comment.
Could we change the error message to the following code?:
error_message = "Possible values are `basic` and `standard`"| type = number | ||
| description = "(Optional) Number of desired SNAT port for each VM in the clusters load balancer. Must be between 0 and 64000 inclusive. Defaults to 0" | ||
| default = 0 | ||
| nullable = false |
There was a problem hiding this comment.
According to the provider schema, the outbound_ports_allocated is optional, which means we could set it to null here.
|
|
||
| variable "outbound_ports_allocated" { | ||
| type = number | ||
| description = "(Optional) Number of desired SNAT port for each VM in the clusters load balancer. Must be between 0 and 64000 inclusive. Defaults to 0" |
There was a problem hiding this comment.
We also need to backquote the number as the official document did here.
| default = "standard" | ||
|
|
||
| validation { | ||
| condition = var.load_balancer_sku == "basic" || var.load_balancer_sku == "standard" |
There was a problem hiding this comment.
Would the following code be better?:
condition = contains(["basic", "standard"], var.load_balancer_sku)|
|
||
| variable "idle_timeout_in_minutes" { | ||
| type = number | ||
| description = "(Optional) Desired outbound flow idle timeout in minutes for the cluster load balancer. Must be between 4 and 120 inclusive." |
There was a problem hiding this comment.
Could we backquote the number as official document did here? And I think we should declare the default value in the description too.
| type = number | ||
| description = "(Optional) Desired outbound flow idle timeout in minutes for the cluster load balancer. Must be between 4 and 120 inclusive." | ||
| default = 30 | ||
| nullable = false |
There was a problem hiding this comment.
According to the provider schema, the idle_timeout_in_minutes argument is optional, which means we can set it to null.
|
Hello @lonegunmanb. Thank you for the review. I hope I understood all the recommendations correctly and made the appropriate changes. The only thing I skipped was the precondition. Is it really needed if we check prior to enabling the profile with this? for_each = var.enable_load_balancer_profile == true && var.load_balancer_sku == "standard" ? ["load_balancer_profile"] : [] |
lonegunmanb
left a comment
lonegunmanb
left a comment
There was a problem hiding this comment.
Thanks @mazilu88 for this update, almost LGTM but some tiny suggestions.
| default = false | ||
| } | ||
|
|
||
| variable "enable_load_balancer_profile" { |
There was a problem hiding this comment.
Could we rename this variable to load_balancer_profile_enabled since now in AzureRM provider we're following such naming convention? Now there're many legacy variables with enable_xxx name but we'd like to keep them to keep backward compatible, but for new variables we`d like to follow the latest naming convention. Thanks!
| variable "enable_load_balancer_profile" { | ||
| type = bool | ||
| description = "(Optional) Enable a load_balancer_profile block. This can only be used when load_balancer_sku is set to `standard`." | ||
| default = false |
There was a problem hiding this comment.
I'd like to restrict this variable as nullable = false since I don't think anyone should set this variable to null.
| service_cidr = var.net_profile_service_cidr | ||
|
|
||
| dynamic "load_balancer_profile" { | ||
| for_each = var.enable_load_balancer_profile == true && var.load_balancer_sku == "standard" ? ["load_balancer_profile"] : [] |
There was a problem hiding this comment.
Since the var.enable_load_balancer_profile should not be null, could we simplify the expression to the following code?:
for_each = var.enable_load_balancer_profile && var.load_balancer_sku == "standard" ? ["load_balancer_profile"] : []| type = number | ||
| description = "(Optional) Desired outbound flow idle timeout in minutes for the cluster load balancer. Must be between `4` and `120` inclusive." | ||
| default = null | ||
| nullable = true |
There was a problem hiding this comment.
nullable is default to true so I think we could just omit this expression here.
| type = number | ||
| description = "(Optional) Count of desired managed outbound IPs for the cluster load balancer. Must be between `1` and `100` inclusive" | ||
| default = null | ||
| nullable = true |
There was a problem hiding this comment.
nullable is default to true so I think we could just omit this expression here.
| type = number | ||
| description = "(Optional) The desired number of IPv6 outbound IPs created and managed by Azure for the cluster load balancer. Must be in the range of `1` to `100` (inclusive). The default value is `0` for single-stack and `1` for dual-stack. Note: managed_outbound_ipv6_count requires dual-stack networking. To enable dual-stack networking the Preview Feature Microsoft.ContainerService/AKS-EnableDualStack needs to be enabled and the Resource Provider re-registered, see the documentation for more information. https://docs.microsoft.com/azure/aks/configure-kubenet-dual-stack?tabs=azure-cli%!C(MISSING)kubectl#register-the-aks-enabledualstack-preview-feature" | ||
| default = null | ||
| nullable = true |
There was a problem hiding this comment.
nullable is default to true so I think we could just omit this expression here.
| type = set(string) | ||
| description = "(Optional) The ID of the Public IP Addresses which should be used for outbound communication for the cluster load balancer." | ||
| default = null | ||
| nullable = true |
There was a problem hiding this comment.
nullable is default to true so I think we could just omit this expression here.
| type = set(string) | ||
| description = "(Optional) The ID of the outbound Public IP Address Prefixes which should be used for the cluster load balancer." | ||
| default = null | ||
| nullable = true |
There was a problem hiding this comment.
nullable is default to true so I think we could just omit this expression here.
| variable "load_balancer_profile_outbound_ports_allocated" { | ||
| type = number | ||
| description = "(Optional) Number of desired SNAT port for each VM in the clusters load balancer. Must be between `0` and `64000` inclusive. Defaults to `0`" | ||
| default = null |
There was a problem hiding this comment.
According to the document, I would suggest that we could set the default to 0 here.
@mazilu88 I'd like to have this |
|
I have added the |
lonegunmanb
left a comment
lonegunmanb
left a comment
There was a problem hiding this comment.
Thanks @mazilu88 for the update, we've passed almost all checks, only one description issue, once we've fixed it we're good to merge.
|
|
||
| variable "load_balancer_profile_managed_outbound_ipv6_count" { | ||
| type = number | ||
| description = "(Optional) The desired number of IPv6 outbound IPs created and managed by Azure for the cluster load balancer. Must be in the range of `1` to `100` (inclusive). The default value is `0` for single-stack and `1` for dual-stack. Note: managed_outbound_ipv6_count requires dual-stack networking. To enable dual-stack networking the Preview Feature Microsoft.ContainerService/AKS-EnableDualStack needs to be enabled and the Resource Provider re-registered, see the documentation for more information. https://docs.microsoft.com/azure/aks/configure-kubenet-dual-stack?tabs=azure-cli%!!(MISSING)C(MISSING)kubectl#register-the-aks-enabledualstack-preview-feature" |
There was a problem hiding this comment.
It seems like some typos in the description's tail:
...https://docs.microsoft.com/azure/aks/configure-kubenet-dual-stack?tabs=azure-cli%!!(MISSING)C(MISSING)kubectl#register-the-aks-enabledualstack-preview-feature
|
Thank you again for the review @lonegunmanb. It was a copy-paste issue when rendering the results of tflint on my machine. |
|
Potential Breaking Changes in db53a41: |
|
What is the purpose of commit db53a41 ? @lonegunmanb I have seen this also in other PRs, it seems contributors are trying to address some |
Hi @zioproto , the variable's |
lonegunmanb
left a comment
lonegunmanb
left a comment
There was a problem hiding this comment.
Thanks @mazilu88 , LGTM! 🚀
3 participants
Describe your changes
Added support for load_balancer_profile
Issue number
#274
Checklist before requesting a review
CHANGELOG.mdfileThanks for your cooperation!