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

[BUG] aci_subnet_id is documented as optional but module fails if not specified #5

Closed
mloskot opened this issue Nov 23, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@mloskot
Copy link
Contributor

mloskot commented Nov 23, 2023

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version

1.6.4

AzureRM Provider Version

3.78.0

Affected Resource(s)/Data Source(s)

azurerm_kubernetes_cluster, azapi_resource

Terraform Configuration Files

module "aks" {
  source  = "claranet/aks-light/azurerm"
  version = "~> 7.0.0"

  client_name         = var.owner
  environment         = var.environment
  location            = module.azure_region.location
  location_short      = module.azure_region.location_short
  resource_group_name = module.resource_group.resource_group_name
  stack               = var.stack

  kubernetes_version = var.aks_kubernetes_version
  service_cidr       = var.aks_service_cidr

  nodes_subnet = {
    name                 = module.remote_state_stack_network.result.spoke_aks_system_subnet.subnet_name
    virtual_network_name = module.remote_state_stack_network.result.spoke_virtual_network.virtual_network_name
  }

  private_cluster_enabled = false

  default_node_pool = {
    name                  = "default"
    os_sku                = "AzureLinux"
    os_type               = "Linux"
    vm_size               = "Standard_D2_v3"
  }

  node_pools = []

  logs_destinations_ids = [
    module.remote_state_stack_monitor.result.logs_storage_account.logs_storage_account_id
  ]

  oms_agent = {
    log_analytics_workspace_id = module.remote_state_stack_monitor.result.log_analytics_workspace.log_analytics_workspace_id
  }

Debug Output/Panic Output

│ Error: Invalid function argument
│
│   on .terraform/modules/aks/data.tf line 9, in data "azapi_resource" "subnet_delegation":
│    9:   name      = element(split("/", var.aci_subnet_id), 10)
│     ├────────────────
│     │ while calling split(separator, str)
│     │ var.aci_subnet_id is null
│
│ Invalid value for "str" parameter: argument must not be null.
╵
╷
│ Error: Invalid function argument
│
│   on .terraform/modules/aks/data.tf line 10, in data "azapi_resource" "subnet_delegation":
│   10:   parent_id = trimsuffix(var.aci_subnet_id, format("/subnets/%s", element(split("/", var.aci_subnet_id), 10)))
│     ├────────────────
│     │ while calling split(separator, str)
│     │ var.aci_subnet_id is null
│
│ Invalid value for "str" parameter: argument must not be null.

Expected Behaviour

The README.md says the aci_subnet_id is NOT required.

The variable is also optional as per

variable "aci_subnet_id" {
description = "ID of the Subnet for ACI virtual-nodes."
type = string
default = null
}

I expect to be able to omit the aci_subnet_id variable.

Actual Behaviour

The variable is not optional as per the current implementation, see the debug dump above which says:

while calling split(separator, str)
var.aci_subnet_id is null

Steps to Reproduce

  1. terraform plan will succeeed
  2. terraform apply will fail as in the debug dump

Important Factoids

No response

References

No response

@mloskot mloskot added the bug Something isn't working label Nov 23, 2023
@mloskot mloskot changed the title [BUG] ... [BUG] aci_subnet_id is documented as optional but module fails if not specified Nov 23, 2023
@mloskot
Copy link
Contributor Author

mloskot commented Nov 23, 2023

Workaround failure

I have tried an obvious workaround: create a dummy vnet and subnet with required delegation for ACI, so I can plug in aci_subnet_id = module.bug_aci_subnet.subnet_id into my module.aks definition:

module "bug_network_vnet" {
  source  = "claranet/vnet/azurerm"
  version = "~> 5.2.0"

  environment         = var.environment
  location            = module.azure_region.location
  location_short      = module.azure_region.location_short
  client_name         = var.owner
  stack               = var.stack
  resource_group_name = module.resource_group.resource_group_name

  vnet_cidr = ["10.0.1.0/26"]
}

module "bug_network_route_table" {
  source  = "claranet/route-table/azurerm"
  version = "~> 5.2.0"

  client_name         = var.owner
  environment         = var.environment
  stack               = var.stack
  location            = module.azure_region.location
  location_short      = module.azure_region.location_short
  resource_group_name = module.resource_group.resource_group_name
}

module "bug_aci_subnet" {
  source  = "claranet/subnet/azurerm"
  version = "6.2.0"

  client_name         = var.owner
  environment         = var.environment
  location_short      = module.azure_region.location_short
  resource_group_name = module.resource_group.resource_group_name
  stack               = var.stack

  custom_subnet_name = "BugAciSubnet"

  virtual_network_name = module.remote_state_stack_network.result.spoke_virtual_network.virtual_network_name
  subnet_cidr_list     = ["10.0.1.0/28"]

  subnet_delegation = {
    app-service-plan = [
      {
        name    = "Microsoft.ContainerInstance/containerGroups"
        actions = ["Microsoft.Network/virtualNetworks/subnets/action"]
      }
    ]
  }

  route_table_name = module.bug_network_route_table.route_table_name
}

but this just opens the pandora's box of further requirements:

│ Error: Invalid count argument
│
│   on .terraform/modules/aks/r-rbac.tf line 51, in data "azuread_service_principal" "aci_identity":
│   51:   count = length(var.aci_subnet_id[*])
│
│ The "count" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the
│ -target argument to first apply only the resources that the count depends on.

So, I don't see any usable workaround for this issue, until the bug (regression) is fixed and presumably 7.0.1 is released.

@mloskot
Copy link
Contributor Author

mloskot commented Dec 9, 2023

A workaround is to vendor the module, hopefully temporarily :-) and apply the following changes

image

@jmapro
Copy link
Contributor

jmapro commented Dec 11, 2023

Hello !

Thanks for reporting the issue. I've pushed a fix on the branch AZ-1305-fix-variable-bug. Can you test with our code and let me know if it works for you ?

Thanks.

@mloskot
Copy link
Contributor Author

mloskot commented Dec 11, 2023

@jmapro Thanks for taking the action.

TL;TR: I'm happy to confirm your fix works for me.

In details, I have just tested your fix:

  1. Copy all *.tf files from AZ-1305-fix-variable-bug branch overwriting existing files in my vendored copy of the module
  2. Using the patched module, run Terraform init, plan and apply against existing AKS cluster
  3. No failures observed

To double-check, I switched to the latest version of module available from the Hashicorp registry:

module "aks" {
  source  = "claranet/aks-light/azurerm"
  version = "~> 7.0.1"
...
}

then, re-run Terraform init, plan against my AKS cluster and it failed, as expected:

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Invalid function argument
│
│   on .terraform/modules/aks/data.tf line 9, in data "azapi_resource" "subnet_delegation":
│    9:   name      = element(split("/", var.aci_subnet_id), 10)
│     ├────────────────
│     │ while calling split(separator, str)
│     │ var.aci_subnet_id is null
│
│ Invalid value for "str" parameter: argument must not be null.
╵
╷
│ Error: Invalid function argument
│
│   on .terraform/modules/aks/data.tf line 10, in data "azapi_resource" "subnet_delegation":
│   10:   parent_id = trimsuffix(var.aci_subnet_id, format("/subnets/%s", element(split("/", var.aci_subnet_id), 10)))
│     ├────────────────
│     │ while calling split(separator, str)
│     │ var.aci_subnet_id is null
│
│ Invalid value for "str" parameter: argument must not be null.

@jmapro
Copy link
Contributor

jmapro commented Dec 11, 2023

Thanks for the confirmation. This fix will pass our internal review this week and if everything is good will be released by the end of the week.

@mloskot
Copy link
Contributor Author

mloskot commented Dec 11, 2023

Thank you @jmapro

jmapro added a commit that referenced this issue Dec 15, 2023
Fixed
  * [GITHUB-5](#5 (comment): Fix `aci_subnet_id` variable usage
  * [GITHUB-8](#8): Assign AcrPull role to kubelet identity
Shr3ps added a commit that referenced this issue Dec 15, 2023
Fixed
  * [GITHUB-5](#5: Fix `aci_subnet_id` variable usage
  * [GITHUB-8](#8): Assign AcrPull role to kubelet identity
@jmapro
Copy link
Contributor

jmapro commented Dec 15, 2023

Released in v7.0.2

@jmapro jmapro closed this as completed Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants