feat: migrate Packer templates to HCL2 #7689
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates Packer templates from JSON to HCL2 format to address JSON deprecation and leverage modern Packer features. The migration consolidates multiple distro-specific JSON templates into a unified HCL2 configuration with dynamic provisioners.
Changes:
- Introduces new HCL2 configuration files (variables.pkr.hcl, source.pkr.hcl, build.pkr.hcl, packer-plugin.pkr.hcl) replacing multiple JSON templates
- Updates build scripts to reference the new buildconfig directory structure
- Refactors Makefile to use a single unified template path instead of multiple distro-specific paths
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| vhdbuilder/packer/buildconfig/variables.pkr.hcl | Defines all Packer variables and local values with environment variable defaults |
| vhdbuilder/packer/buildconfig/source.pkr.hcl | Configures the Azure ARM builder source with image and infrastructure settings |
| vhdbuilder/packer/buildconfig/build.pkr.hcl | Defines the build process with dynamic provisioners for file uploads and shell scripts |
| vhdbuilder/packer/buildconfig/packer-plugin.pkr.hcl | Specifies required Packer plugin versions |
| vhdbuilder/packer/buildconfig/produce-packer-settings.sh | Script for generating Packer settings JSON from environment variables |
| vhdbuilder/packer/buildconfig/produce-packer-settings-functions.sh | Helper functions for settings generation and Azure resource management |
| vhdbuilder/packer/buildconfig/dynamic-provisioners/*.json | JSON files defining file upload/download mappings for different distros |
| packer.mk | Updated Makefile to use unified HCL2 template path instead of multiple JSON templates |
| buildNumber = "${var.build_number}" | ||
| createdBy = "aks-vhd-pipeline" | ||
| image_sku = "${var.img_sku}" | ||
| now = "${var.create_time}" |
There was a problem hiding this comment.
The variable create_time is referenced but never declared. This will cause a Packer error. You need to add a variable declaration for create_time.
| for_each = "${local.ubuntu_file_upload}" | ||
| content { | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "ubuntu" | ||
| } | ||
| } | ||
|
|
||
| // AzureLinux-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.azlinux_file_upload}" | ||
| content { | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "cblmariner" | ||
| } | ||
| } | ||
|
|
||
| // Flatcar-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.flatcar_file_upload}" | ||
| content { | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "flatcar" |
There was a problem hiding this comment.
The when attribute is not valid for dynamic provisioners in Packer. The conditional logic should be implemented using a conditional expression in the for_each itself:
for_each = lower(var.os_sku) == "flatcar" ? local.flatcar_file_upload : []
| for_each = "${local.ubuntu_file_upload}" | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| when = lower(var.os_sku) == "ubuntu" | |
| } | |
| } | |
| // AzureLinux-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = "${local.azlinux_file_upload}" | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| when = lower(var.os_sku) == "cblmariner" | |
| } | |
| } | |
| // Flatcar-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = "${local.flatcar_file_upload}" | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| when = lower(var.os_sku) == "flatcar" | |
| for_each = lower(var.os_sku) == "ubuntu" ? local.ubuntu_file_upload : [] | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| } | |
| } | |
| // AzureLinux-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = lower(var.os_sku) == "cblmariner" ? local.azlinux_file_upload : [] | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| } | |
| } | |
| // Flatcar-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = lower(var.os_sku) == "flatcar" ? local.flatcar_file_upload : [] | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination |
|
|
||
| subscription_id = "${var.subscription_id}" | ||
| user_assigned_managed_identities = "${var.msi_resource_strings}" | ||
| use_azure_cli_auth = "true" |
There was a problem hiding this comment.
The use_azure_cli_auth is set to the string "true" but should be a boolean value true (without quotes) or reference the variable var.use_azure_cli_auth. As defined in variables.pkr.hcl line 125, this variable is of type bool, so it should be referenced as:
use_azure_cli_auth = var.use_azure_cli_auth
| use_azure_cli_auth = "true" | |
| use_azure_cli_auth = var.use_azure_cli_auth |
| inline = "${local.reboot_command}" | ||
| pause_after = "60s" | ||
| skip_clean = true | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| inline = ["/bin/bash -ux /home/packer/install-dependencies.sh"] | ||
| environment_vars = [ | ||
| FEATURE_FLAGS=${var.feature_flags}, | ||
| BUILD_NUMBER=${var.build_number}, | ||
| BUILD_ID=${var.build_id}, | ||
| COMMIT=${var.commit}, | ||
| HYPERV_GENERATION=${var.hyperv_generation}, | ||
| CONTAINER_RUNTIME=${var.container_runtime}, | ||
| TELEPORTD_PLUGIN_DOWNLOAD_URL=${var.teleportd_plugin_download_url}, | ||
| ENABLE_FIPS=${var.enable_fips}, | ||
| IMG_SKU=${var.img_sku}, | ||
| PRIVATE_PACKAGES_URL=${var.private_packages_url}, | ||
| VHD_BUILD_TIMESTAMP=${local.vhd_build_timestamp} | ||
| ] | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| inline = ["sudo /bin/bash /home/packer/generate-disk-usage.sh"] | ||
| } | ||
|
|
||
| dynamic "provisioner" { | ||
| for_each = "${local.midway_file_downloads}" | ||
| content { | ||
| type = "file" | ||
| direction = "download" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| } | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| expect_disconnect = true | ||
| inline = "${local.reboot_command}" |
There was a problem hiding this comment.
Unnecessary string interpolation. The expression "${local.reboot_command}" can be simplified to local.reboot_command since it's already a string value. The same simplification can be applied throughout this file where single values are wrapped in string interpolation without additional text.
| image_version = "${var.captured_sig_version}" | ||
| replication_regions = ["${var.location}"] | ||
| resource_group = "${var.resource_group_name}" | ||
| subscription = "${local.gallery_subscription_id}" |
There was a problem hiding this comment.
The local variable gallery_subscription_id is referenced but never defined. This will cause a Packer error. You need to add a local or variable definition for gallery_subscription_id.
|
|
||
| variable "captured_sig_version" { | ||
| type = string | ||
| default = "${env("$${CAPTURED_SIG_VERSION")}" |
There was a problem hiding this comment.
The variable default value contains a malformed environment variable reference. The syntax "${env("$${CAPTURED_SIG_VERSION")}" has an extra dollar sign and brace. This should be "${env("CAPTURED_SIG_VERSION")}" to correctly reference the environment variable.
| default = "${env("$${CAPTURED_SIG_VERSION")}" | |
| default = "${env("CAPTURED_SIG_VERSION")}" |
| ENABLE_FIPS=${var.enable_fips}, | ||
| IMG_SKU=${var.img_sku}, | ||
| UA_TOKEN=${var.ua_token}, | ||
| VHD_BUILD_TIMESTAMP=${local.vhd_build_timestamp} |
There was a problem hiding this comment.
The local variable vhd_build_timestamp is referenced but never defined in the variables.pkr.hcl file. This will cause a Packer error. You need to add a local or variable definition for vhd_build_timestamp.
|
|
||
| variable "SkipLinuxAzSecPack" { | ||
| type = string | ||
| default = true |
There was a problem hiding this comment.
The default value is set to the boolean true, but the type is declared as string. This type mismatch will cause a Packer error. The default should be "true" (string) or the type should be changed to bool.
| default = true | |
| default = "true" |
| @@ -0,0 +1,265 @@ | |||
| locals { | |||
| // Managed Image settings - empty for ARM64 builds | |||
| managed_image_resource_group_name = lower("${var.architecture}") == "arm64" ? "" : "${var.resource_group_name}" | |||
There was a problem hiding this comment.
The variable resource_group_name is referenced in line 3 but is never declared in this file. This will cause Packer to fail with an undefined variable error. You need to add a variable declaration for resource_group_name.
packer.mk
Outdated
|
|
||
| run-packer: az-login | ||
| @packer init ./vhdbuilder/packer/packer-plugin.pkr.hcl && packer version && ($(MAKE) -f packer.mk init-packer | tee packer-output) && ($(MAKE) -f packer.mk build-packer | tee -a packer-output) | ||
| @packer init ./vhdbuilder/packer/buildconfig/build.pkr.hcl && packer version && ($(MAKE) -f packer.mk init-packer | tee packer-output) && ($(MAKE) -f packer.mk build-packer | tee -a packer-output) |
There was a problem hiding this comment.
The packer init command should target the directory containing all HCL files (buildconfig) rather than just build.pkr.hcl. The correct command should be:
packer init ./vhdbuilder/packer/buildconfig
This ensures all .pkr.hcl files in the directory are properly initialized.
| @packer init ./vhdbuilder/packer/buildconfig/build.pkr.hcl && packer version && ($(MAKE) -f packer.mk init-packer | tee packer-output) && ($(MAKE) -f packer.mk build-packer | tee -a packer-output) | |
| @packer init ./vhdbuilder/packer/buildconfig && packer version && ($(MAKE) -f packer.mk init-packer | tee packer-output) && ($(MAKE) -f packer.mk build-packer | tee -a packer-output) |
| secure_boot_enabled = "${local.secure_boot_enabled}" | ||
| vtpm_enabled = "${local.vtpm_enabled}" | ||
| security_type = "${local.security_type}" | ||
| security_encryption_type = "${local.security_encryption_type}" | ||
|
|
||
| custom_data_file = "${local.custom_data_file}" | ||
|
|
||
| os_disk_size_gb = 30 | ||
| os_type = "Linux" | ||
| polling_duration_timeout = "1h" | ||
| ssh_read_write_timeout = "5m" | ||
|
|
||
| shared_image_gallery_destination { | ||
| specialized = "${local.specialized_image}" |
There was a problem hiding this comment.
In HCL2, boolean and string values in source blocks should not be wrapped in quotes when they're already boolean values. The values for 'secure_boot_enabled', 'vtpm_enabled', and 'specialized' are already booleans from the locals, so the string interpolation syntax "${local.X}" will convert them to strings. Remove the quotes or use direct reference like 'local.secure_boot_enabled' instead of '"${local.secure_boot_enabled}"'.
| SKU_NAME=${var.sku_name}, | ||
| IMAGE_VERSION=${var.image_version}, | ||
| CONTAINER_RUNTIME=${var.container_runtime} |
There was a problem hiding this comment.
The environment variable assignments are missing quotes around the variable references. In HCL2 shell provisioners, environment_vars should be strings in the format "KEY=value". Change these lines to use proper quoting, for example: "SKU_NAME=${var.sku_name}" instead of SKU_NAME=${var.sku_name}.
| SKU_NAME=${var.sku_name}, | |
| IMAGE_VERSION=${var.image_version}, | |
| CONTAINER_RUNTIME=${var.container_runtime} | |
| "SKU_NAME=${var.sku_name}", | |
| "IMAGE_VERSION=${var.image_version}", | |
| "CONTAINER_RUNTIME=${var.container_runtime}" |
| type = string | ||
| default = "${env("OS_SKU")}" | ||
| } | ||
|
|
There was a problem hiding this comment.
The variable 'create_time' is referenced in source.pkr.hcl at line 53, but it is never defined in variables.pkr.hcl. This will cause Packer to fail with an undefined variable error. You need to add a variable declaration for 'create_time'.
| variable "create_time" { | |
| type = string | |
| default = "${env("CREATE_TIME")}" | |
| } |
|
|
||
| provisioner "shell" { | ||
| expect_disconnect = true | ||
| inline = "${local.reboot_command}" |
There was a problem hiding this comment.
The inline command is using string interpolation for a list value. In HCL2, when 'inline' expects a list, you should use array syntax. Change this from 'inline = "${local.reboot_command}"' to 'inline = [local.reboot_command]' (without quotes and with square brackets).
| inline = "${local.reboot_command}" | |
| inline = [local.reboot_command] |
| locals { | ||
| // Managed Image settings - empty for ARM64 builds | ||
| managed_image_resource_group_name = lower("${var.architecture}") == "arm64" ? "" : "${var.resource_group_name}" | ||
| managed_image_name = lower("${var.architecture}") == "arm64" ? "" : "${var.sig_image_name}-${var.captured_sig_version}" | ||
|
|
||
| // Confidential VM settings, if enabled via feature flags | ||
| secure_boot_enabled = can(regex("cvm", lower(var.feature_flags))) ? true : false | ||
| vtpm_enabled = can(regex("cvm", lower(var.feature_flags))) ? true : false | ||
| security_type = can(regex("cvm", lower(var.feature_flags))) ? "ConfidentialVM" : "" | ||
| security_encryption_type = can(regex("cvm", lower(var.feature_flags))) ? "VMGuestStateOnly" : "" | ||
| specialized_image = can(regex("cvm", lower(var.feature_flags))) ? true : false | ||
| cvm_encryption_type = can(regex("cvm", lower(var.feature_flags))) ? "EncryptedVMGuestStateOnlyWithPmk" : "" | ||
|
|
||
| // File uploads for build process | ||
| custom_data_file = lower(var.os_version) == "flatcar" ? var.custom_data_file : "" | ||
| aks_node_controller = lower("${var.architecture}") == "x86_64" ? var.aksnode_controller_amd64 : var.aksnode_controller_arm64 | ||
| common_file_upload = jsondecode(file(var.common_file_upload)).files | ||
| ubuntu_file_upload = jsondecode(file(var.ubuntu_file_upload)).files | ||
| azlinux_file_upload = jsondecode(file(var.azlinux_file_upload)).files | ||
| flatcar_file_upload = jsondecode(file(var.flatcar_file_upload)).files | ||
|
|
||
| // File downloads for artifact creation | ||
| midway_file_downloads = jsondecode(file(var.file_downloads)).midway | ||
| post_build_file_downloads = jsondecode(file(var.file_downloads)).post-build | ||
|
|
||
| // Reboot command based on distro | ||
| reboot_command = lower(var.os_version) == "flatcar" ? ": reboot" : "sudo reboot" | ||
| } |
There was a problem hiding this comment.
The local variable 'vhd_build_timestamp' is referenced in build.pkr.hcl at lines 74 and 98, but it is never defined in the locals block of variables.pkr.hcl. This will cause Packer to fail with an undefined local variable error. You need to add a definition for 'vhd_build_timestamp' in the locals block.
|
|
||
| // Ubuntu-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.ubuntu_file_upload}" |
There was a problem hiding this comment.
The dynamic provisioner blocks use "${local.ubuntu_file_upload}" with unnecessary string interpolation. In HCL2, for_each expects a map or set directly, so this should be written as 'for_each = local.ubuntu_file_upload' without the quotes and interpolation markers.
| for_each = "${local.ubuntu_file_upload}" | |
| for_each = local.ubuntu_file_upload |
| "source": "parts/linux/cloud-init/artifacts/pam-d-common-auth", | ||
| "destination": "/home/packer/pam-d-common-auth" | ||
| }, | ||
| { | ||
| "type": "file", |
There was a problem hiding this comment.
The file 'parts/linux/cloud-init/artifacts/pam-d-common-auth' with destination '/home/packer/pam-d-common-auth' is listed twice in this file (lines 294-297 and 489-492). This duplication will cause the file to be uploaded twice, which is redundant and may cause issues during the build process. Remove one of these duplicate entries.
| "source": "parts/linux/cloud-init/artifacts/pam-d-common-auth", | |
| "destination": "/home/packer/pam-d-common-auth" | |
| }, | |
| { | |
| "type": "file", |
|
|
||
| // These files are common to all VHDs, and will be uploaded to the Packer VM regardless of distro | ||
| dynamic "provisioner" { | ||
| for_each = "${local.common_file_upload}" |
There was a problem hiding this comment.
The dynamic provisioner blocks use "${local.common_file_upload}" with unnecessary string interpolation. In HCL2, for_each expects a map or set directly, so this should be written as 'for_each = local.common_file_upload' without the quotes and interpolation markers.
| } | ||
|
|
||
| dynamic "provisioner" { | ||
| for_each = "${local.midway_file_downloads}" |
There was a problem hiding this comment.
The dynamic provisioner blocks use "${local.midway_file_downloads}" with unnecessary string interpolation. In HCL2, for_each expects a map or set directly, so this should be written as 'for_each = local.midway_file_downloads' without the quotes and interpolation markers.
| for_each = "${local.midway_file_downloads}" | |
| for_each = local.midway_file_downloads |
| } | ||
|
|
||
| dynamic "provisioner" { | ||
| for_each = "${local.post_build_file_downloads}" |
There was a problem hiding this comment.
The dynamic provisioner blocks use "${local.post_build_file_downloads}" with unnecessary string interpolation. In HCL2, for_each expects a map or set directly, so this should be written as 'for_each = local.post_build_file_downloads' without the quotes and interpolation markers.
| for_each = "${local.post_build_file_downloads}" | |
| for_each = local.post_build_file_downloads |
| "source": "parts/linux/cloud-init/artifacts/pam-d-common-auth", | ||
| "destination": "/home/packer/pam-d-common-auth" | ||
| }, | ||
| { | ||
| "type": "file", |
There was a problem hiding this comment.
The file pam-d-common-auth is uploaded twice to the same destination /home/packer/pam-d-common-auth from the same source. This is a duplicate entry that should be removed to avoid confusion and unnecessary file transfer operations.
| "source": "parts/linux/cloud-init/artifacts/pam-d-common-auth", | |
| "destination": "/home/packer/pam-d-common-auth" | |
| }, | |
| { | |
| "type": "file", |
| }, | ||
| { | ||
| "type": "file", | ||
| "source": "parts/linux/cloud-init/artifacts/update_certs.service", | ||
| "destination": "/home/packer/update_certs.service" |
There was a problem hiding this comment.
Both file entries upload to the same destination /home/packer/update_certs.service, but from different sources. The second upload will overwrite the first one. This appears to be a duplicate or conflicting configuration. Verify which source file is the correct one to use for Azure Linux/Mariner builds.
| }, | |
| { | |
| "type": "file", | |
| "source": "parts/linux/cloud-init/artifacts/update_certs.service", | |
| "destination": "/home/packer/update_certs.service" |
|
|
||
| variable "captured_sig_version" { | ||
| type = string | ||
| default = "${env("$${CAPTURED_SIG_VERSION")}" |
There was a problem hiding this comment.
This environment variable reference has incorrect syntax. The dollar sign escaping is malformed - it should be either ${env("CAPTURED_SIG_VERSION")} without the extra $${} wrapper, or if you're trying to reference a literal ${CAPTURED_SIG_VERSION} string, the syntax is incorrect. This will cause the variable to fail to read the environment variable properly.
| default = "${env("$${CAPTURED_SIG_VERSION")}" | |
| default = "${env("CAPTURED_SIG_VERSION")}" |
| for_each = "${local.ubuntu_file_upload}" | ||
| content { | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "ubuntu" | ||
| } | ||
| } | ||
|
|
||
| // AzureLinux-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.azlinux_file_upload}" | ||
| content { | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "cblmariner" | ||
| } | ||
| } | ||
|
|
||
| // Flatcar-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.flatcar_file_upload}" | ||
| content { | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "flatcar" | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The conditional logic for the when attribute is incorrect. In HCL2, conditionals should use a boolean expression directly, but here lower(var.os_sku) == "ubuntu" returns a boolean, which should work. However, verify that the when attribute is supported in this context, as it's not a standard Packer provisioner attribute. The correct approach would be to wrap the entire dynamic block with a conditional or use only at the build level.
| for_each = "${local.ubuntu_file_upload}" | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| when = lower(var.os_sku) == "ubuntu" | |
| } | |
| } | |
| // AzureLinux-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = "${local.azlinux_file_upload}" | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| when = lower(var.os_sku) == "cblmariner" | |
| } | |
| } | |
| // Flatcar-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = "${local.flatcar_file_upload}" | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| when = lower(var.os_sku) == "flatcar" | |
| } | |
| } | |
| for_each = lower(var.os_sku) == "ubuntu" ? local.ubuntu_file_upload : [] | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| } | |
| } | |
| // AzureLinux-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = lower(var.os_sku) == "cblmariner" ? local.azlinux_file_upload : [] | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| } | |
| } | |
| // Flatcar-specific file uploads | |
| dynamic "provisioner" { | |
| for_each = lower(var.os_sku) == "flatcar" ? local.flatcar_file_upload : [] | |
| content { | |
| type = "file" | |
| source = provisioner.value.source | |
| destination = provisioner.value.destination | |
| } | |
| } |
| "source": "parts/linux/cloud-init/artifacts/pam-d-common-password", | ||
| "destination": "/home/packer/pam-d-common-password" | ||
| }, | ||
| { | ||
| "type": "file", |
There was a problem hiding this comment.
The file pam-d-common-password is uploaded twice to the same destination /home/packer/pam-d-common-password from the same source. This is a duplicate entry that should be removed to avoid confusion and unnecessary file transfer operations.
| "source": "parts/linux/cloud-init/artifacts/pam-d-common-password", | |
| "destination": "/home/packer/pam-d-common-password" | |
| }, | |
| { | |
| "type": "file", |
| "source": "parts/linux/cloud-init/artifacts/pam-d-su", | ||
| "destination": "/home/packer/pam-d-su" | ||
| }, | ||
| { | ||
| "type": "file", |
There was a problem hiding this comment.
The file pam-d-su is uploaded twice to the same destination /home/packer/pam-d-su from the same source. This is a duplicate entry that should be removed to avoid confusion and unnecessary file transfer operations.
| "source": "parts/linux/cloud-init/artifacts/pam-d-su", | |
| "destination": "/home/packer/pam-d-su" | |
| }, | |
| { | |
| "type": "file", |
|
|
||
| variable "file_downloads" { | ||
| type = string | ||
| default = "vhdbuilder/packer/buildconfig/dynamic-provisioners/file-downloads.json" |
There was a problem hiding this comment.
The file path references file-downloads.json (with a hyphen), but the actual file in the directory is named file_downloads.json (with an underscore). This mismatch will cause a file not found error when Packer tries to load this JSON file. Either rename the actual file to use a hyphen, or update this path to use an underscore.
| default = "vhdbuilder/packer/buildconfig/dynamic-provisioners/file-downloads.json" | |
| default = "vhdbuilder/packer/buildconfig/dynamic-provisioners/file_downloads.json" |
| destination = "${var.aks_node_controller}" | ||
| source = "/home/packer/aks-node-controller" |
There was a problem hiding this comment.
Incorrect provisioner file configuration. The destination and source parameters are swapped. According to Packer documentation, for file provisioners, source should be the local path and destination should be the remote path. Here, var.aks_node_controller appears to be a local file path but is assigned to destination, while /home/packer/aks-node-controller should be the destination on the VM.
| destination = "${var.aks_node_controller}" | |
| source = "/home/packer/aks-node-controller" | |
| source = "/home/packer/aks-node-controller" | |
| destination = "${var.aks_node_controller}" |
| image_offer = "${var.img_offer}" | ||
| image_publisher = "${var.img_publisher}" | ||
| image_sku = "${var.img_sku}" | ||
| image_version = "${var.img_version}" | ||
|
|
||
| subscription_id = "${var.subscription_id}" | ||
| user_assigned_managed_identities = "${var.msi_resource_strings}" | ||
| use_azure_cli_auth = "true" | ||
| location = "${var.location}" | ||
| vm_size = "${var.vm_size}" | ||
|
|
||
| managed_image_resource_group_name = "${local.managed_image_resource_group_name}" | ||
| managed_image_name = "${local.managed_image_name}" | ||
| managed_image_storage_account_type = "Premium_LRS" | ||
|
|
||
| virtual_network_resource_group_name = "${var.vnet_resource_group_name}" | ||
| virtual_network_name = "${var.vnet_name}" | ||
| virtual_network_subnet_name = "${var.subnet_name}" | ||
|
|
||
| secure_boot_enabled = "${local.secure_boot_enabled}" | ||
| vtpm_enabled = "${local.vtpm_enabled}" | ||
| security_type = "${local.security_type}" | ||
| security_encryption_type = "${local.security_encryption_type}" | ||
|
|
||
| custom_data_file = "${local.custom_data_file}" |
There was a problem hiding this comment.
Unnecessary string interpolation. In HCL2, when referencing a variable directly, you don't need the wrapping quotes and dollar sign. Change "${var.img_offer}" to var.img_offer for cleaner syntax. This applies to all variable references in this source block.
| secure_boot_enabled = "${local.secure_boot_enabled}" | ||
| vtpm_enabled = "${local.vtpm_enabled}" | ||
| security_type = "${local.security_type}" | ||
| security_encryption_type = "${local.security_encryption_type}" | ||
|
|
||
| custom_data_file = "${local.custom_data_file}" | ||
|
|
||
| os_disk_size_gb = 30 | ||
| os_type = "Linux" | ||
| polling_duration_timeout = "1h" | ||
| ssh_read_write_timeout = "5m" | ||
|
|
||
| shared_image_gallery_destination { | ||
| specialized = "${local.specialized_image}" | ||
| confidential_vm_image_encryption_type = "${local.cvm_encryption_type}" |
There was a problem hiding this comment.
Unnecessary string interpolation. In HCL2, when referencing a local variable directly, you don't need the wrapping quotes and dollar sign. Change "${local.secure_boot_enabled}" to local.secure_boot_enabled for cleaner syntax. This applies to all local references in this source block.
| azure_tags = { | ||
| SkipLinuxAzSecPack = "true" | ||
| branch = "${var.branch}" | ||
| buildDefinitionName = "${var.build_definition_name}" | ||
| buildId = "${var.build_id}" | ||
| buildNumber = "${var.build_number}" | ||
| createdBy = "aks-vhd-pipeline" | ||
| image_sku = "${var.img_sku}" | ||
| now = "${var.create_time}" | ||
| os = "Linux" | ||
| } |
There was a problem hiding this comment.
Unnecessary string interpolation. In HCL2, when referencing a variable or local directly, you don't need the wrapping quotes and dollar sign. Change "${var.sig_gallery_name}" to var.sig_gallery_name for cleaner syntax. This applies to all references in this shared_image_gallery_destination block.
| "ENABLE_FIPS=${var.enable_fips}", | ||
| "IMG_SKU=${var.img_sku}", | ||
| "PRIVATE_PACKAGES_URL=${var.private_packages_url}", | ||
| "VHD_BUILD_TIMESTAMP=${local.vhd_build_timestamp}" |
There was a problem hiding this comment.
Incorrect variable reference. vhd_build_timestamp should be referenced as var.vhd_build_timestamp instead of local.vhd_build_timestamp. The value comes from the settings.json file as a variable, not a local value computed at runtime.
| "VHD_BUILD_TIMESTAMP=${local.vhd_build_timestamp}" | |
| "VHD_BUILD_TIMESTAMP=${var.vhd_build_timestamp}" |
| dynamic "provisioner" { | ||
| for_each = "${local.common_file_upload}" | ||
| content { | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| } | ||
| } | ||
|
|
||
| // Ubuntu-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.ubuntu_file_upload}" | ||
| content { | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "ubuntu" | ||
| } | ||
| } | ||
|
|
||
| // AzureLinux-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.azlinux_file_upload}" | ||
| content { | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "cblmariner" | ||
| } | ||
| } | ||
|
|
||
| // Flatcar-specific file uploads | ||
| dynamic "provisioner" { | ||
| for_each = "${local.flatcar_file_upload}" | ||
| content { | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] | ||
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "flatcar" | ||
| } | ||
| } | ||
|
|
||
| // Architecture-specific aks-node-controller upload | ||
| provisioner "file" { | ||
| destination = "${var.aks_node_controller}" | ||
| source = "/home/packer/aks-node-controller" | ||
| } | ||
|
|
||
|
|
||
| // Build Process begins | ||
| // pre-install-dependencies.sh, install-dependencies.sh, post-install-dependencies.sh, and list-images.sh are run in order, typically with reboots and file downloads in between | ||
| provisioner "shell" { | ||
| inline = ["/bin/bash -ux /home/packer/pre-install-dependencies.sh"] | ||
| environment_vars = [ | ||
| "FEATURE_FLAGS=${var.feature_flags}", | ||
| "BUILD_NUMBER=${var.build_number}", | ||
| "BUILD_ID=${var.build_id}", | ||
| "COMMIT=${var.commit}", | ||
| "HYPERV_GENERATION=${var.hyperv_generation}", | ||
| "CONTAINER_RUNTIME=${var.container_runtime}", | ||
| "TELEPORTD_PLUGIN_DOWNLOAD_URL=${var.teleportd_plugin_download_url}", | ||
| "ENABLE_FIPS=${var.enable_fips}", | ||
| "IMG_SKU=${var.img_sku}", | ||
| "UA_TOKEN=${var.ua_token}", | ||
| "VHD_BUILD_TIMESTAMP=${local.vhd_build_timestamp}" | ||
| ] | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| expect_disconnect = true | ||
| inline = "${local.reboot_command}" | ||
| pause_after = "60s" | ||
| skip_clean = true | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| inline = ["/bin/bash -ux /home/packer/install-dependencies.sh"] | ||
| environment_vars = [ | ||
| "FEATURE_FLAGS=${var.feature_flags}", | ||
| "BUILD_NUMBER=${var.build_number}", | ||
| "BUILD_ID=${var.build_id}", | ||
| "COMMIT=${var.commit}", | ||
| "HYPERV_GENERATION=${var.hyperv_generation}", | ||
| "CONTAINER_RUNTIME=${var.container_runtime}", | ||
| "TELEPORTD_PLUGIN_DOWNLOAD_URL=${var.teleportd_plugin_download_url}", | ||
| "ENABLE_FIPS=${var.enable_fips}", | ||
| "IMG_SKU=${var.img_sku}", | ||
| "PRIVATE_PACKAGES_URL=${var.private_packages_url}", | ||
| "VHD_BUILD_TIMESTAMP=${local.vhd_build_timestamp}" | ||
| ] | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| inline = ["sudo /bin/bash /home/packer/generate-disk-usage.sh"] | ||
| } | ||
|
|
||
| dynamic "provisioner" { | ||
| for_each = "${local.midway_file_downloads}" | ||
| content { | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] | ||
| type = "file" | ||
| direction = "download" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| } | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| expect_disconnect = true | ||
| inline = "${local.reboot_command}" | ||
| pause_after = "60s" | ||
| skip_clean = true | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| inline = ["/bin/bash -ux /home/packer/post-install-dependencies.sh"] | ||
| environment_vars = [ | ||
| "FEATURE_FLAGS=${var.feature_flags}", | ||
| "BUILD_NUMBER=${var.build_number}", | ||
| "BUILD_ID=${var.build_id}", | ||
| "COMMIT=${var.commit}", | ||
| "HYPERV_GENERATION=${var.hyperv_generation}", | ||
| "CONTAINER_RUNTIME=${var.container_runtime}", | ||
| "TELEPORTD_PLUGIN_DOWNLOAD_URL=${var.teleportd_plugin_download_url}", | ||
| "ENABLE_FIPS=${var.enable_fips}", | ||
| "IMG_SKU=${var.img_sku}" | ||
| ] | ||
| } | ||
|
|
||
| provisioner "shell" { | ||
| inline = ["/bin/bash -ux /home/packer/list-images.sh"] | ||
| environment_vars = [ | ||
| "SKU_NAME=${var.sku_name}", | ||
| "IMAGE_VERSION=${var.image_version}", | ||
| "CONTAINER_RUNTIME=${var.container_runtime}" | ||
| ] | ||
| } | ||
|
|
||
| dynamic "provisioner" { | ||
| for_each = "${local.post_build_file_downloads}" |
There was a problem hiding this comment.
Unnecessary string interpolation in for_each expressions. In HCL2, when referencing a local variable directly, you don't need the wrapping quotes and dollar sign. Change for_each = "${local.common_file_upload}" to for_each = local.common_file_upload for cleaner syntax. This applies to all for_each expressions in this build block.
|
|
||
| provisioner "shell" { | ||
| expect_disconnect = true | ||
| inline = "${local.reboot_command}" |
There was a problem hiding this comment.
Unnecessary string interpolation. Change inline = "${local.reboot_command}" to inline = [local.reboot_command] since inline expects a list of strings, not a single interpolated string.
| inline = "${local.reboot_command}" | |
| inline = [local.reboot_command] |
|
|
||
| provisioner "shell" { | ||
| expect_disconnect = true | ||
| inline = "${local.reboot_command}" |
There was a problem hiding this comment.
Unnecessary string interpolation. Change inline = "${local.reboot_command}" to inline = [local.reboot_command] since inline expects a list of strings, not a single interpolated string.
| type = string | ||
| default = "${env("OS_SKU")}" | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing required variable declarations. The variables resource_group_name, gallery_subscription_id, create_time, and vhd_build_timestamp are generated in the settings.json file (see produce-packer-settings.sh lines 224, 223, 228, and 232) but are not declared in this variables file. These variables need to be declared to receive values from the var-file. Add these variable declarations with appropriate types (all should be string type).
| variable "resource_group_name" { | |
| type = string | |
| } | |
| variable "gallery_subscription_id" { | |
| type = string | |
| } | |
| variable "create_time" { | |
| type = string | |
| } | |
| variable "vhd_build_timestamp" { | |
| type = string | |
| } |
|
|
||
| variable "captured_sig_version" { | ||
| type = string | ||
| default = "${env("$${CAPTURED_SIG_VERSION")}" |
There was a problem hiding this comment.
There's an invalid variable reference with incorrect escape sequence. The syntax "${env("$${CAPTURED_SIG_VERSION")}" includes an extra dollar sign and brace that should not be present. It should be "${env("CAPTURED_SIG_VERSION")}" to properly reference the environment variable.
| default = "${env("$${CAPTURED_SIG_VERSION")}" | |
| default = "${env("CAPTURED_SIG_VERSION")}" |
| image_version = "${var.captured_sig_version}" | ||
| replication_regions = ["${var.location}"] | ||
| resource_group = "${var.resource_group_name}" | ||
| subscription = "${local.gallery_subscription_id}" |
There was a problem hiding this comment.
The local variable "gallery_subscription_id" is referenced here but is never defined in the locals block or anywhere in variables.pkr.hcl. This will cause a build failure. Based on the produce-packer-settings.sh script (line 223), this should likely reference a variable named "gallery_subscription_id" that needs to be added to the variables file.
| subscription = "${local.gallery_subscription_id}" | |
| subscription = "${var.gallery_subscription_id}" |
| image_offer = "${var.img_offer}" | ||
| image_publisher = "${var.img_publisher}" | ||
| image_sku = "${var.img_sku}" | ||
| image_version = "${var.img_version}" | ||
|
|
||
| subscription_id = "${var.subscription_id}" | ||
| user_assigned_managed_identities = "${var.msi_resource_strings}" | ||
| use_azure_cli_auth = "true" | ||
| location = "${var.location}" | ||
| vm_size = "${var.vm_size}" |
There was a problem hiding this comment.
Unnecessary string interpolation. The syntax "${var.img_offer}" can be simplified to just var.img_offer since it's already a string variable. This applies to all similar cases in this file (lines 3-12, 18-27, etc.). While not strictly incorrect, removing the unnecessary interpolation makes the code cleaner and more idiomatic HCL2.
| dynamic "provisioner" { | ||
| for_each = "${local.ubuntu_file_upload}" | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] |
There was a problem hiding this comment.
The labels for dynamic provisioners should only contain the provisioner type, not include the source name. The labels should be ["file"] instead of ["azure-arm.nodelifecycle-image-builder"]. This pattern appears throughout the build block but is inconsistent with standard Packer HCL2 syntax for dynamic blocks.
| dynamic "provisioner" { | ||
| for_each = "${local.flatcar_file_upload}" | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] |
There was a problem hiding this comment.
The labels for dynamic provisioners should only contain the provisioner type, not include the source name. The labels should be ["file"] instead of ["azure-arm.nodelifecycle-image-builder"]. This pattern appears throughout the build block but is inconsistent with standard Packer HCL2 syntax for dynamic blocks.
|
|
||
| provisioner "shell" { | ||
| expect_disconnect = true | ||
| inline = "${local.reboot_command}" |
There was a problem hiding this comment.
The inline parameter expects an array but the value is incorrectly wrapped in string interpolation syntax. It should be inline = [local.reboot_command] instead of inline = "${local.reboot_command}" to properly pass an array of commands to the shell provisioner.
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "ubuntu" |
There was a problem hiding this comment.
The when condition is checking "os_sku" but should be checking "os_version" to match the logic in the locals block (line 15 of variables.pkr.hcl) which uses os_version for Ubuntu detection. This mismatch will cause Ubuntu-specific files to not be uploaded when they should be.
| when = lower(var.os_sku) == "ubuntu" | |
| when = lower(var.os_version) == "ubuntu" |
| type = "file" | ||
| source = provisioner.value.source | ||
| destination = provisioner.value.destination | ||
| when = lower(var.os_sku) == "flatcar" |
There was a problem hiding this comment.
The when condition is checking "os_sku" but should be checking "os_version" to match the logic in the locals block (line 15 of variables.pkr.hcl) which uses os_version for Flatcar detection. This mismatch will cause Flatcar-specific files to not be uploaded when they should be.
| when = lower(var.os_sku) == "flatcar" | |
| when = lower(var.os_version) == "flatcar" |
| dynamic "provisioner" { | ||
| for_each = "${local.post_build_file_downloads}" | ||
| labels = ["azure-arm.nodelifecycle-image-builder"] |
There was a problem hiding this comment.
The labels for dynamic provisioners should only contain the provisioner type, not include the source name. The labels should be ["file"] instead of ["azure-arm.nodelifecycle-image-builder"]. This pattern appears throughout the build block but is inconsistent with standard Packer HCL2 syntax for dynamic blocks.
|
|
||
| variable "resource_group_name" { | ||
| type = string | ||
| default = "${env("AZURE_RESOURCE_GROUP_NAM")}" |
There was a problem hiding this comment.
The environment variable name appears to be truncated. It should be "AZURE_RESOURCE_GROUP_NAME" but is currently "AZURE_RESOURCE_GROUP_NAM" (missing the final "E"). This will cause the variable to not be populated correctly from the environment.
| default = "${env("AZURE_RESOURCE_GROUP_NAM")}" | |
| default = "${env("AZURE_RESOURCE_GROUP_NAME")}" |
What this PR does / why we need it:
Migrates Packer template to HCL due to JSON deprecation and to make sure of additional Packer features
Which issue(s) this PR fixes:
JSON deprecation, reduces clutter and maintenance issues in the repo