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.

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

feat: enable precondition on default_node_pool for autoscaling with node pool type #484

Merged

Conversation

ishuar
Copy link
Contributor

@ishuar ishuar commented Dec 7, 2023

Describe your changes

  • Incorrect precondition on node pools
  • Enable precondition on default_node_pool for autoscaling with node pool type.

Enable autoscaling only when the node_pool type is VirtualMachineScaleSets, otherwise throw an error.


✅ The below workflow is expected.

The Module had a bug with incorrect precondition on resource azurerm_kubernetes_cluster_node_pool at here

precondition {
      condition     = var.agents_type == "VirtualMachineScaleSets"
      error_message = "Multiple Node Pools are only supported when the Kubernetes Cluster is using Virtual Machine Scale Sets."
    }

variable agents_type is not relevant to azurerm_kubernetes_cluster_node_pool , only applicable to default_node_pool in the azurerm_kubernetes_cluster resource.

It might be working because of the default value of agents_type variable set to VirtualMachineScaleSets , ref @ here

but if the value is updated, additional node pools are not possible to create.

If we make following change to multiple_node_pools example. ( adding agents_type = "Iamfake" ). The plan fails.

module "aks" {
  source = "../.."

  prefix              = "prefix-${random_id.prefix.hex}"
  resource_group_name = local.resource_group.name
  os_disk_size_gb     = 60
  sku_tier            = "Standard"
  rbac_aad            = false
  vnet_subnet_id      = azurerm_subnet.test.id
  node_pools          = local.nodes
   agents_type        = "Iamfake"
}
Click to view the plan with updated value
 ╭─ishansharma@MacBook-Pro ~/workspaces/personal/github/terraform-azurerm-aks/examples/multiple_node_pools ‹main●›
╰─$ tf plan

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create
 <= read (data resources)

Terraform planned the following actions, but then encountered a problem:

  # azurerm_resource_group.main[0] will be created
  + resource "azurerm_resource_group" "main" {
      + id       = (known after apply)
      + location = "centralus"
      + name     = (known after apply)
    }

  # azurerm_subnet.test will be created
  + resource "azurerm_subnet" "test" {
      + address_prefixes                               = [
          + "10.52.0.0/24",
        ]
      + enforce_private_link_endpoint_network_policies = true
      + enforce_private_link_service_network_policies  = (known after apply)
      + id                                             = (known after apply)
      + name                                           = (known after apply)
      + private_endpoint_network_policies_enabled      = (known after apply)
      + private_link_service_network_policies_enabled  = (known after apply)
      + resource_group_name                            = (known after apply)
      + virtual_network_name                           = (known after apply)
    }

  # azurerm_virtual_network.test will be created
  + resource "azurerm_virtual_network" "test" {
      + address_space       = [
          + "10.52.0.0/16",
        ]
      + dns_servers         = (known after apply)
      + guid                = (known after apply)
      + id                  = (known after apply)
      + location            = "centralus"
      + name                = (known after apply)
      + resource_group_name = (known after apply)
      + subnet              = (known after apply)
    }

  # random_id.prefix will be created
  + resource "random_id" "prefix" {
      + b64_std     = (known after apply)
      + b64_url     = (known after apply)
      + byte_length = 8
      + dec         = (known after apply)
      + hex         = (known after apply)
      + id          = (known after apply)
    }

  # module.aks.data.azurerm_resource_group.main will be read during apply
  # (config refers to values not yet known)
 <= data "azurerm_resource_group" "main" {
      + id         = (known after apply)
      + location   = (known after apply)
      + managed_by = (known after apply)
      + name       = (known after apply)
      + tags       = (known after apply)
    }

  # module.aks.azapi_update_resource.aks_cluster_post_create will be created
  + resource "azapi_update_resource" "aks_cluster_post_create" {
      + body                    = jsonencode(
            {
              + properties = {
                  + kubernetesVersion = null
                }
            }
        )
      + id                      = (known after apply)
      + ignore_casing           = false
      + ignore_missing_property = true
      + name                    = (known after apply)
      + output                  = (known after apply)
      + parent_id               = (known after apply)
      + resource_id             = (known after apply)
      + type                    = "Microsoft.ContainerService/managedClusters@2023-01-02-preview"
    }

  # module.aks.azurerm_kubernetes_cluster.main will be created
  + resource "azurerm_kubernetes_cluster" "main" {
      + api_server_authorized_ip_ranges     = (known after apply)
      + azure_policy_enabled                = false
      + dns_prefix                          = (known after apply)
      + fqdn                                = (known after apply)
      + http_application_routing_enabled    = false
      + http_application_routing_zone_name  = (known after apply)
      + id                                  = (known after apply)
      + image_cleaner_enabled               = false
      + image_cleaner_interval_hours        = 48
      + kube_admin_config                   = (sensitive value)
      + kube_admin_config_raw               = (sensitive value)
      + kube_config                         = (sensitive value)
      + kube_config_raw                     = (sensitive value)
      + kubernetes_version                  = (known after apply)
      + location                            = (known after apply)
      + name                                = (known after apply)
      + node_resource_group                 = (known after apply)
      + node_resource_group_id              = (known after apply)
      + oidc_issuer_enabled                 = false
      + oidc_issuer_url                     = (known after apply)
      + portal_fqdn                         = (known after apply)
      + private_cluster_enabled             = false
      + private_cluster_public_fqdn_enabled = false
      + private_dns_zone_id                 = (known after apply)
      + private_fqdn                        = (known after apply)
      + public_network_access_enabled       = false
      + resource_group_name                 = (known after apply)
      + role_based_access_control_enabled   = false
      + run_command_enabled                 = true
      + sku_tier                            = "Standard"
      + workload_identity_enabled           = false

      + default_node_pool {
          + enable_auto_scaling    = false
          + enable_host_encryption = false
          + enable_node_public_ip  = false
          + kubelet_disk_type      = (known after apply)
          + max_pods               = (known after apply)
          + name                   = "nodepool"
          + node_count             = 2
          + node_labels            = (known after apply)
          + orchestrator_version   = (known after apply)
          + os_disk_size_gb        = 60
          + os_disk_type           = "Managed"
          + os_sku                 = (known after apply)
          + scale_down_mode        = "Delete"
          + type                   = "Iamfake"
          + ultra_ssd_enabled      = false
          + vm_size                = "Standard_D2s_v3"
          + vnet_subnet_id         = (known after apply)
          + workload_runtime       = (known after apply)
        }

      + identity {
          + principal_id = (known after apply)
          + tenant_id    = (known after apply)
          + type         = "SystemAssigned"
        }

      + network_profile {
          + dns_service_ip     = (known after apply)
          + docker_bridge_cidr = (known after apply)
          + ip_versions        = (known after apply)
          + load_balancer_sku  = "standard"
          + network_mode       = (known after apply)
          + network_plugin     = "kubenet"
          + network_policy     = (known after apply)
          + outbound_type      = "loadBalancer"
          + pod_cidr           = (known after apply)
          + pod_cidrs          = (known after apply)
          + service_cidr       = (known after apply)
          + service_cidrs      = (known after apply)
        }

      + oms_agent {
          + log_analytics_workspace_id = (known after apply)
          + oms_agent_identity         = (known after apply)
        }
    }

  # module.aks.azurerm_log_analytics_solution.main[0] will be created
  + resource "azurerm_log_analytics_solution" "main" {
      + id                    = (known after apply)
      + location              = (known after apply)
      + resource_group_name   = (known after apply)
      + solution_name         = "ContainerInsights"
      + workspace_name        = (known after apply)
      + workspace_resource_id = (known after apply)

      + plan {
          + name      = (known after apply)
          + product   = "OMSGallery/ContainerInsights"
          + publisher = "Microsoft"
        }
    }

  # module.aks.azurerm_log_analytics_workspace.main[0] will be created
  + resource "azurerm_log_analytics_workspace" "main" {
      + allow_resource_only_permissions = true
      + daily_quota_gb                  = -1
      + id                              = (known after apply)
      + internet_ingestion_enabled      = true
      + internet_query_enabled          = true
      + local_authentication_disabled   = false
      + location                        = (known after apply)
      + name                            = (known after apply)
      + primary_shared_key              = (sensitive value)
      + resource_group_name             = (known after apply)
      + retention_in_days               = 30
      + secondary_shared_key            = (sensitive value)
      + sku                             = "PerGB2018"
      + workspace_id                    = (known after apply)
    }

  # module.aks.null_resource.kubernetes_version_keeper will be created
  + resource "null_resource" "kubernetes_version_keeper" {
      + id       = (known after apply)
      + triggers = {
          + "version" = null
        }
    }

  # module.aks.null_resource.pool_name_keeper["worker0"] will be created
  + resource "null_resource" "pool_name_keeper" {
      + id       = (known after apply)
      + triggers = {
          + "pool_name" = (known after apply)
        }
    }

  # module.aks.null_resource.pool_name_keeper["worker1"] will be created
  + resource "null_resource" "pool_name_keeper" {
      + id       = (known after apply)
      + triggers = {
          + "pool_name" = (known after apply)
        }
    }

  # module.aks.null_resource.pool_name_keeper["worker2"] will be created
  + resource "null_resource" "pool_name_keeper" {
      + id       = (known after apply)
      + triggers = {
          + "pool_name" = (known after apply)
        }
    }

Plan: 12 to add, 0 to change, 0 to destroy.
╷
│ Warning: Argument is deprecated
│
│   with azurerm_subnet.test,
│   on main.tf line 31, in resource "azurerm_subnet" "test":31:   enforce_private_link_endpoint_network_policies = true
│
│ `enforce_private_link_endpoint_network_policies` will be removed in favour of the property `private_endpoint_network_policies_enabled` in version 4.0 of the AzureRM Provider
│
│ (and 2 more similar warnings elsewhere)
╵
╷
│ Error: Resource precondition failed
│
│   on ../../main.tf line 743, in resource "azurerm_kubernetes_cluster_node_pool" "node_pool":743:       condition     = var.agents_type == "VirtualMachineScaleSets"
│     ├────────────────
│     │ var.agents_type is "Iamfake"
│
│ Multiple Node Pools are only supported when the Kubernetes Cluster is using Virtual Machine Scale Sets.
╵
╷
│ Error: Resource precondition failed
│
│   on ../../main.tf line 743, in resource "azurerm_kubernetes_cluster_node_pool" "node_pool":743:       condition     = var.agents_type == "VirtualMachineScaleSets"
│     ├────────────────
│     │ var.agents_type is "Iamfake"
│
│ Multiple Node Pools are only supported when the Kubernetes Cluster is using Virtual Machine Scale Sets.
╵
╷
│ Error: Resource precondition failed
│
│   on ../../main.tf line 743, in resource "azurerm_kubernetes_cluster_node_pool" "node_pool":743:       condition     = var.agents_type == "VirtualMachineScaleSets"
│     ├────────────────
│     │ var.agents_type is "Iamfake"
│
│ Multiple Node Pools are only supported when the Kubernetes Cluster is using Virtual Machine Scale Sets.
  • Removed deprecated attribute public_network_access_enabled

As the azure provider config is version = ">= 3.80.0, < 4.0" public_network_access_enabled is deprecated and not passed to azure API, hence can be removed and cause no effect in the last version released on main branch.

Issue number

N/A

#000

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine
github_actions scan results:

Passed checks: 192, Failed checks: 0, Skipped checks: 0

Thanks for your cooperation!

@ishuar
Copy link
Contributor Author

ishuar commented Dec 7, 2023

@ishuar please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

Copy link
Collaborator

@zioproto zioproto left a comment

Choose a reason for hiding this comment

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

Keep 1 change per PR.

I added some comments to the new condition.

The old condition is correct, and should also be kept in place together with the new one you are proposing.

@lonegunmanb we should check with the product team if anyone is still actually using AvailabilitySet instead of VirtualMachineScaleSets. Because most customers are using multiple node pools we could just enforce the use of VirtualMachineScaleSets for the sake of simplicity.

README.md Outdated Show resolved Hide resolved
examples/named_cluster/main.tf Outdated Show resolved Hide resolved
examples/startup/main.tf Outdated Show resolved Hide resolved
examples/with_acr/main.tf Outdated Show resolved Hide resolved
examples/without_monitor/main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@ishuar ishuar force-pushed the fix/incorrect-precondition-on-nodepools branch from 5df4274 to 5ebd001 Compare December 7, 2023 18:52
@ishuar ishuar force-pushed the fix/incorrect-precondition-on-nodepools branch from 5ebd001 to 406f1a3 Compare December 7, 2023 18:56
@ishuar ishuar changed the title fix: Incorrect precondition on nodepools & removed deprecated public_network_access_enabled attribute feat: enable precondition on default_node_pool for autoscaling with node pool type Dec 7, 2023
@ishuar ishuar requested a review from zioproto December 7, 2023 19:00
Copy link
Contributor

github-actions bot commented Dec 8, 2023

Potential Breaking Changes in 406f1a3:
[delete] "Variables.green_field_application_gateway_for_ingress.Name" from 'green_field_application_gateway_for_ingress' to ''
[delete] "Variables.create_role_assignments_for_application_gateway.Name" from 'create_role_assignments_for_application_gateway' to ''
[delete] "Variables.brown_field_application_gateway_for_ingress.Name" from 'brown_field_application_gateway_for_ingress' to ''
[update] "Variables.maintenance_window.Type" from 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), [
]),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})' to 'object({
allowed = optional(list(object({
day = string
hours = set(number)
})), []),
not_allowed = optional(list(object({
end = string
start = string
})), []),
})'

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @ishuar for opening this pr! Almost LGTM but only one comment.

Btw, apology for our latest merged pr but now we have code conflict, would you please rebase your branch with the latest main branch? Thanks!

main.tf Outdated Show resolved Hide resolved
Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @ishuar, LGTM!

@lonegunmanb lonegunmanb merged commit c3d0a9b into Azure:main Dec 18, 2023
4 checks passed
@ishuar ishuar deleted the fix/incorrect-precondition-on-nodepools branch January 12, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants