hashicorp / terraform-provider-azurerm

Terraform provider for Azure Resource Manager
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs
Mozilla Public License 2.0
4.6k stars 4.65k forks source link

azurerm_mssql_virtual_machine auto_backup encryption_password failing #12811

Open dkisby opened 3 years ago

dkisby commented 3 years ago

Terraform (and Provider) Versions

Terraform Version: v1.0.3 azurerm v2.70.0 random v3.1.0

Affected resources

azurerm_mssql_virtual_machine

Terraform configuration files

provider "azurerm" {
  features {}
}

resource "azurerm_resource_group" "rg" {
  name     = "test-rg"
  location = "West Europe"
}

resource "azurerm_network_interface" "nic" {
  name                = "nic-vmtest"
  location            = azurerm_resource_group.rg.location
  resource_group_name = azurerm_resource_group.rg.name

  ip_configuration {
    name                          = "internal"
    subnet_id                     = "<subnetID>"
    private_ip_address_allocation = "Dynamic"
  }
}

resource "azurerm_windows_virtual_machine" "vm" {
  name                = "testvm"
  resource_group_name = azurerm_resource_group.rg.name
  location            = azurerm_resource_group.rg.location
  size                = "Standard_D2as_v4"
  admin_username      = "adminuser"
  admin_password      = "P@$$w0rd1234!"
  network_interface_ids = [
    azurerm_network_interface.nic.id,
  ]

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

  source_image_reference {
    publisher = "MicrosoftSQLServer"
    offer     = "sql2016sp2-ws2019"
    sku       = "standard"
    version   = "latest"
  }
}

resource "azurerm_managed_disk" "datadisk" {
    count                = 2
    name                 = "DataDisk_${count.index +1}"
    location             = azurerm_resource_group.rg.location
    resource_group_name  = azurerm_resource_group.rg.name
    storage_account_type = "Standard_LRS"
    create_option        = "Empty"
    disk_size_gb         = 128
}

resource "azurerm_virtual_machine_data_disk_attachment" "datadisk01" {
    count              = 2
    managed_disk_id    = element(azurerm_managed_disk.datadisk.*.id, count.index)
    virtual_machine_id = element(azurerm_windows_virtual_machine.vm.*.id, count.index)
    lun                = "1${count.index}"
    caching            = "ReadWrite"
}

resource "random_password" "encryptionpassword" {
    length = 12
    special = true
}

resource "azurerm_mssql_virtual_machine" "db_vm" {
  virtual_machine_id               = azurerm_windows_virtual_machine.vm.id
  sql_license_type                 = "PAYG"
  auto_patching {
    day_of_week                            = "Saturday"
    maintenance_window_duration_in_minutes = 60
    maintenance_window_starting_hour       = 6
  }
  auto_backup {
    encryption_enabled              = true
    encryption_password             = random_password.encryptionpassword.result
    retention_period_in_days        = 14
    system_databases_backup_enabled = true
    storage_blob_endpoint           = "https://test.blob.core.windows.net/automaticbackup"
    storage_account_access_key      = "<accessKey>"
    manual_schedule {
      full_backup_frequency           = "Daily"
      full_backup_start_hour          = 4
      full_backup_window_in_hours     = 4
      log_backup_frequency_in_minutes = 60
    }
  }
}

Expected behaviour

Encryption password is created and used as the encryption_password in auto_backup

Actual behaviour

terraform plan
Error: auto_backup: `encryption_password` is required when `encryption_enabled` is true
 with azurerm_mssql_virtual_machine.db_vm,
  on main.tf line 69, in resource "azurerm_mssql_virtual_machine" "db_vm":
  69: resource "azurerm_mssql_virtual_machine" "db_vm" {

Steps to Reproduce

  1. terraform plan
wasfree commented 3 years ago

Hi @dkisby,

seems this issue is related to a pre-checking if encryption_enabled is true and encryption_password has a empty string. Unfortunately the random_password resource will provide the result only after apply, while the pre-checking expecting a non-empty string on a earlier stage.

There are few ways to resolve this issue:

  1. Remove this pre-checking and rely on documentation. So it's up to the user to provider a proper configuration.
  2. We could deprecate encryption_enabled and only look for encryption_password. If encryption_password was set we also enable encryption, if not we disable it. This would also make the pre-checking obsolete.

To be honest encryption_enabled isn't really needed at all, if someone want to encrypt his backup and provided a encryption_password this should be enough. As a third way Azure could remove this value from API but even this was agreed it won't happen in the near future. I'm tending to the second option, but before raising a PR it would be great to have some feedback if this is a acceptable way.

dkisby commented 3 years ago

Hi @wasfree

I had a look at No.1, and this doesn't seem to work. Because although I can omit the auto-backup config from the deployment and rely on the user to configure the auto-backup as per documentation, then if they ever run the deployment again then they are faced with the following issue:

# azurerm_mssql_virtual_machine.db_vm must be replaced
-/+ resource "azurerm_mssql_virtual_machine" "db_vm" {
      ~ id                    = "vmId" -> (known after apply)
      - r_services_enabled    = false -> null

      - auto_backup { # forces replacement
          - encryption_enabled              = true -> null
          - retention_period_in_days        = 30 -> null
          - system_databases_backup_enabled = true -> null
        }

      + auto_patching {
          + day_of_week                            = "Saturday"
          + maintenance_window_duration_in_minutes = 60
          + maintenance_window_starting_hour       = 6
        }
    }

Because this forces replacement, it tries to destroy the azurerm_mssql_virtual_machine and redeploy, but it can't complete this process and reaches timeout during the destroy process. Leaving the SQL VM in a broken state.

No.2 sounds like the answer to me.

wasfree commented 3 years ago

Hi @dkisby,

there has to be a misunderstanding in No. 1, the pre-checking has to be disabled in terraform provider code not in terraform configuration. It won't work to configure this outside of terraform because encryption_enabled has a default value of false which applies if auto_backup block is defined.

dkisby commented 3 years ago

Hi @wasfree

Ah, my apologies, I misread No. 1. Either way, I still think No. 2 sounds like the way forward, if pre-checking provides no value then deprecate it.

jdelforno commented 2 years ago

Atm, we're forced to split out the deployment pipeline jobs to first, generate a secret and then use said secret when deploying backup encryption.

It's not a huge hassle but it's definitely inconvenient.

PtrckGdwn commented 1 year ago

Hi @wasfree,

I was wondering if the solution you outlined is something that can be implemented? It would make life quite a bit easier / cleaner if it is doable.

Hi @dkisby,

seems this issue is related to a pre-checking if encryption_enabled is true and encryption_password has a empty string. Unfortunately the random_password resource will provide the result only after apply, while the pre-checking expecting a non-empty string on a earlier stage.

There are few ways to resolve this issue:

  1. Remove this pre-checking and rely on documentation. So it's up to the user to provider a proper configuration.
  2. We could deprecate encryption_enabled and only look for encryption_password. If encryption_password was set we also enable encryption, if not we disable it. This would also make the pre-checking obsolete.

To be honest encryption_enabled isn't really needed at all, if someone want to encrypt his backup and provided a encryption_password this should be enough. As a third way Azure could remove this value from API but even this was agreed it won't happen in the near future. I'm tending to the second option, but before raising a PR it would be great to have some feedback if this is a acceptable way.

danpetitt commented 1 year ago

At the moment I can see no solution past this in a CI environment.

We can't run apply with a specific password, then re-edit the stack to add the random password and then re-run an apply.

Surely after two years, there must be a solution/workaround thats usable?

bel-from-nz commented 2 days ago

Wasn't sure if I should start a new bug as this is related to the above.

If you add a lifecycle to ignore changes and deploy, then go into the Azure portal and set the auto backup to storage account on with encryption enabled, this pre-check is triggered on the next plan done.

resource "azurerm_mssql_virtual_machine" "sql_vm" {
  virtual_machine_id = module.vm_windows.resource_id
  sql_license_type   = "PAYG"

  lifecycle {
    ignore_changes = all
  }
}

Expected outcome: changes are ignored

Actual outcome: Error on plan

╷
│ Error: auto_backup: `encryption_password` is required when `encryption_enabled` is true
│
│   with azurerm_mssql_virtual_machine.sql_vm,
│   on main.tf line 90, in resource "azurerm_mssql_virtual_machine" "sql_vm":
│   90: resource "azurerm_mssql_virtual_machine" "sql_vm" {
│
╵

Tested with azurerm 3.116.0 and 4.9.0

kewalaka commented 2 days ago

@wasfree this issue persists on azurerm 4.9 as well - is it worth updating the v2.x legacy label so it can be noted as an on-going problem. Your option 2 looks reasonable to me.