Allow Managed boot diagnostic storage account

[SimenRost]

Is there an existing issue for this?

• [X] I have searched the existing issues

Greenfield/Brownfield provisioning

greenfield

Terraform Version

1.4.2

Module Version

0.1.0

AzureRM Provider Version

3.43.0

Affected Resource(s)/Data Source(s)

azurerm_linux_virtual_machine, azurerm_windows_virtual_machine

Terraform Configuration Files

terraform {
  required_version = ">= 1.2"

  required_providers {
    azurerm = {
      source                = "hashicorp/azurerm"
      version               = ">= 3.11, < 4.0"
    }
  }
}

module "virtual-machine" {
  source  = "Azure/virtual-machine/azurerm"
  version = "0.1.0"

  #Does nothing
  network_security_group_id = "Dummyvar"

  location                   = "norwayeast"
  image_os                   = "windows"
  resource_group_name        = "test-rg"

  boot_diagnostics                     = true
  boot_diagnostics_storage_account_uri = null

  new_network_interface = {
    name = "test-nic"
  }
  admin_username                  = "azureuser"
  disable_password_authentication = false
  admin_password                  = "eafrdg6rsyh6erg4"
  name                            = "ghi-test"

  os_simple = "WindowsServer"
  subnet_id = var.subnet_id
}

tfvars variables values

N/A

Debug Output/Panic Output

N/A

Expected Behaviour

I should be able to enable Boot Diagnostics by telling the provider to not use a storage url, as per provider docs

Actual Behaviour

The precondition check in both the Windows and Linux resource fails when null is the value, this is however accepted by the provider, and should allow the user to enable boot diagnostics with a Microsoft managed storage account.

Steps to Reproduce

No response

Important Factoids

No response

References

No response

[lonegunmanb]

Thanks @SimenRost for opening this issue, apology for the late reply. As you've pointed out, the precondition's expression is:

condition     = !var.boot_diagnostics ? true : var.new_boot_diagnostics_storage_account != null || var.boot_diagnostics_storage_account_uri != null

If you'd like to keep var.boot_diagnostics_storage_account_uri == null, then you must assign a valid value to new_boot_diagnostics_storage_account so this module could create a storage account for you.

In short answer, if you'd like to use an existing storage account, please use boot_diagnostics_storage_account_uri; otherwise if you want to let us create one for you, you could use var.new_boot_diagnostics_storage_account.

[SimenRost]

Thanks for the reply. I would like to use the Azure managed storage account that would be created the same way as if I used the raw azurerm resource.

Passing a null value will utilize a Managed Storage Account to store Boot Diagnostics.

Which should be the same as this option in the portal.

[lonegunmanb]

Hi @SimenRost, thanks for the clarification. The only information that a boot_diagnostics block requires is boot_diagnostics_storage_account_uri, so if you've set var.new_boot_diagnostics_storage_account object, that means you'd like us to create a storage account for you, whilst you can customize all arguments for this new storage account; or you can set boot_diagnostics_storage_account_uri to use an existing storage account. It is equal to what you've seen on the portal.

1. If you don't want to use boot diagnostics, please set var.boot_diagnostics to false
2. If you want use your own storage account, please provide us your boot_diagnostics_storage_account_uri
3. If you want this module create a storage account for you, you can use var.new_boot_diagnostics_storage_account object.

[SimenRost]

Neither 2 or 3 allows me to use a "managed storage account".

Consider this working deployment using the azurerm provider.

resource "azurerm_linux_virtual_machine" "example" {
  name                  = "example-machine"
  resource_group_name   = data.azurerm_resource_group.example.name
  location              = data.azurerm_resource_group.example.location
  size                  = "Standard_F2"
  admin_username        = "adminuser"
  network_interface_ids = [azurerm_network_interface.example.id]

  admin_ssh_key {
    username   = "adminuser"
    public_key = file("~/.ssh/id_rsa.pub")
  }

  boot_diagnostics {
    storage_account_uri = null
  }

  os_disk {
    caching              = "ReadWrite"
    storage_account_type = "Standard_LRS"
  }

  source_image_reference {
    publisher = "Canonical"
    offer     = "UbuntuServer"
    sku       = "16.04-LTS"
    version   = "latest"
  }
}

Which results in a managed storage account for boot diagnostics, as shown here.

With this module I would expect to be able to use this type too, and some variation of this code to enable it.

module "virtual-machine" {
  source   = "Azure/virtual-machine/azurerm"
  version  = "0.1.0"
  image_os = "linux"

  name                = "example-machine2"
  resource_group_name = data.azurerm_resource_group.example.name
  location            = data.azurerm_resource_group.example.location
  size                = "Standard_F2"
  admin_username      = "adminuser"

  new_network_interface = {
    name = "example-machine2-nic"
    ip_configurations = [{
      primary                       = true
      private_ip_address_allocation = "Dynamic"
    }]
  }

  subnet_id = data.azurerm_subnet.example.id

  admin_ssh_keys = [
    {
      public_key = file("~/.ssh/id_rsa.pub")
    }
  ]

  boot_diagnostics = true

  os_disk = {
    caching              = "ReadWrite"
    storage_account_type = "Standard_LRS"
  }

  source_image_reference = {
    publisher = "Canonical"
    offer     = "UbuntuServer"
    sku       = "16.04-LTS"
    version   = "latest"
  }
}

Removing the precondition allows it to run just fine, with the expected result.

diff --git a/main.tf b/main.tf
index 2cb18b1..ab98d2c 100644
--- a/main.tf
+++ b/main.tf
@@ -253,10 +253,6 @@ resource "azurerm_linux_virtual_machine" "vm_linux" {
       ]) == 1
       error_message = "Must provide one and only one of `vm_source_image_id`, `vm_source_image_reference` and `vm_os_simple`."
     }
-    precondition {
-      condition     = !var.boot_diagnostics ? true : var.new_boot_diagnostics_storage_account != null || var.boot_diagnostics_storage_account_uri != null
-      error_message = "Either `new_boot_diagnostics_storage_account` or `vm_boot_diagnostics_storage_account_uri` must be provided if `boot_diagnostics` is `true`."
-    }
     precondition {
       condition     = var.network_interface_ids != null || var.new_network_interface != null
       error_message = "Either `new_network_interface` or `network_interface_ids` must be provided."
@@ -447,10 +443,6 @@ resource "azurerm_windows_virtual_machine" "vm_windows" {
       ]) == 1
       error_message = "Must provide one and only one of `vm_source_image_id`, `vm_source_image_reference` and `vm_os_simple`."
     }
-    precondition {
-      condition     = !var.boot_diagnostics ? true : var.new_boot_diagnostics_storage_account != null || var.boot_diagnostics_storage_account_uri != null
-      error_message = "Either `new_boot_diagnostics_storage_account` or `vm_boot_diagnostics_storage_account_uri` must be provided if `boot_diagnostics` is `true`."
-    }
     precondition {
       condition     = var.network_interface_ids != null || var.new_network_interface != null
       error_message = "Either `new_network_interface` or `network_interface_ids` must be provided."

[lonegunmanb]

Hi @SimenRost sorry for the late reply, I have a question, for now the var.boot_diagnostics is used here and here, what if we removed the precondition and let var.boot_diagnostics to be true, and both of var.new_boot_diagnostics_storage_account var.boot_diagnostics_storage_account_uri to be null?

As we can see here:

resource "azurerm_storage_account" "boot_diagnostics" {
  count = var.boot_diagnostics && var.new_boot_diagnostics_storage_account != null ? 1 : 0

So a null for var.new_boot_diagnostics_storage_account would lead us an empty azurerm_storage_account.boot_diagnostics.

As the boot_diagnostics block showed:

dynamic "boot_diagnostics" {
    for_each = var.boot_diagnostics ? ["boot_diagnostics"] : []

    content {
      storage_account_uri = try(azurerm_storage_account.boot_diagnostics[0].primary_blob_endpoint, var.boot_diagnostics_storage_account_uri)
    }
  }

When azurerm_storage_account.boot_diagnostics[0].primary_blob_endpoint throw an error (due to the empty list), the storage_account_uri would be assigned to var.boot_diagnostics_storage_account_uri, which is also null, so is there any special reason to create such boot_diagnostics block with a null storage_account_uri?

[SimenRost]

For it to be managed by Azure (and not this module) storage_account_uri needs to be null, or not defined at all.

The only change that is needed to accomplish this, is to remove the precondition, as the existing logic already sets the values the provider expects.

[lonegunmanb]

Thanks @SimenRost for the explanation, I'll do some verification today.

[lonegunmanb]

Hi @SimenRost sorry for the late reply, I got covid so I was unable to work in past two weeks. I've done the verification, and I'll submit a pr to remove the precondition. Thanks for your help and patience.