azurerm_mssql_virtual_machine - fix nilpointer + sql_license_type optional

[aristosvo]

Fixes #21137

As an additional measure sql_license_type is changed to optional, as this makes usage of this resource in environment=china possible

[lonegunmanb]

Hi @aristosvo I think this issue is more than a nil pointer. The sql_license_type is not required but optional, I've changed the schema and tested the following Terraform code in eastus region, it worked. :

provider "azurerm" {
  features {}
}

variable "location" {
  default = "eastus"
}

variable "prefix" {
  default = "fasfjfdasf"
}

resource "azurerm_resource_group" "example" {
  name     = "${var.prefix}-resources"
  location = var.location
}

resource "azurerm_virtual_network" "example" {
  name                = "${var.prefix}-VN"
  address_space       = ["10.0.0.0/16"]
  location            = azurerm_resource_group.example.location
  resource_group_name = azurerm_resource_group.example.name
}

resource "azurerm_subnet" "example" {
  name                 = "${var.prefix}-SN"
  resource_group_name  = azurerm_resource_group.example.name
  virtual_network_name = azurerm_virtual_network.example.name
  address_prefixes     = ["10.0.0.0/24"]
}

resource "azurerm_subnet_network_security_group_association" "example" {
  subnet_id                 = azurerm_subnet.example.id
  network_security_group_id = azurerm_network_security_group.example.id
}

resource "azurerm_public_ip" "vm" {
  name                = "${var.prefix}-PIP"
  location            = azurerm_resource_group.example.location
  resource_group_name = azurerm_resource_group.example.name
  allocation_method   = "Dynamic"
}

resource "azurerm_network_security_group" "example" {
  name                = "${var.prefix}-NSG"
  location            = azurerm_resource_group.example.location
  resource_group_name = azurerm_resource_group.example.name
}

resource "azurerm_network_security_rule" "RDPRule" {
  name                        = "RDPRule"
  resource_group_name         = azurerm_resource_group.example.name
  priority                    = 1000
  direction                   = "Inbound"
  access                      = "Allow"
  protocol                    = "Tcp"
  source_port_range           = "*"
  destination_port_range      = 3389
  source_address_prefix       = "167.220.255.0/25"
  destination_address_prefix  = "*"
  network_security_group_name = azurerm_network_security_group.example.name
}

resource "azurerm_network_security_rule" "MSSQLRule" {
  name                        = "MSSQLRule"
  resource_group_name         = azurerm_resource_group.example.name
  priority                    = 1001
  direction                   = "Inbound"
  access                      = "Allow"
  protocol                    = "Tcp"
  source_port_range           = "*"
  destination_port_range      = 1433
  source_address_prefix       = "167.220.255.0/25"
  destination_address_prefix  = "*"
  network_security_group_name = azurerm_network_security_group.example.name
}

resource "azurerm_network_interface" "example" {
  name                = "${var.prefix}-NIC"
  location            = azurerm_resource_group.example.location
  resource_group_name = azurerm_resource_group.example.name

  ip_configuration {
    name                          = "exampleconfiguration1"
    subnet_id                     = azurerm_subnet.example.id
    private_ip_address_allocation = "Dynamic"
    public_ip_address_id          = azurerm_public_ip.vm.id
  }
}

resource "azurerm_network_interface_security_group_association" "example" {
  network_interface_id      = azurerm_network_interface.example.id
  network_security_group_id = azurerm_network_security_group.example.id
}

resource "azurerm_virtual_machine" "example" {
  name                  = "${var.prefix}-VM"
  location              = azurerm_resource_group.example.location
  resource_group_name   = azurerm_resource_group.example.name
  network_interface_ids = [azurerm_network_interface.example.id]
  vm_size               = "Standard_DS14_v2"

  storage_image_reference {
    publisher = "MicrosoftSQLServer"
    offer     = "SQL2017-WS2016"
    sku       = "SQLDEV"
    version   = "latest"
  }

  storage_os_disk {
    name              = "${var.prefix}-OSDisk"
    caching           = "ReadOnly"
    create_option     = "FromImage"
    managed_disk_type = "Premium_LRS"
  }

  os_profile {
    computer_name  = "winhost01"
    admin_username = "exampleadmin"
    admin_password = "Password1234!"
  }

  os_profile_windows_config {
    timezone                  = "Pacific Standard Time"
    provision_vm_agent        = true
    enable_automatic_upgrades = true
  }
}

resource "azurerm_mssql_virtual_machine" "example" {
  virtual_machine_id = azurerm_virtual_machine.example.id
}

Should we change sql_license_type to optional?

[aristosvo]

@lonegunmanb Is the only change in your test setup the schema change to optional?

[lonegunmanb]

Yes, exactly.

[Drilli]

Guys, I think the task #21137 is not being solved here. The issue there is when setting the environment to "china":

[lonegunmanb]

Hi @Drilli, whether the environment is china or not, this issue was caused by a nil sql_license_type returned by the service side, so I believe this nil check will fix the panic you've seen. The question is, I'm convinced to believe that sql_license_type is not required but optional because I did create a such resource with a null value. This patch still could cause issues, since it has the following restriction:

"sql_license_type": {
				Type:     pluginsdk.TypeString,
				Required: true,
				ForceNew: true,
				ValidateFunc: validation.StringInSlice([]string{
					string(sqlvirtualmachines.SqlServerLicenseTypePAYG),
					string(sqlvirtualmachines.SqlServerLicenseTypeAHUB),
					string(sqlvirtualmachines.SqlServerLicenseTypeDR),
				}, false),
			},

If we do set this field to an empty string as this patch did, then the value just violates the validation. Should we change the schema?

[Drilli]

@lonegunmanb yes, that field should be optional. Furthermore, in China where I am currently working on something, that field MUST BE optional; in China the License Management is not supported, therefore the license type should not be set. I have cloned the code locally, changed it to optional and tried using my own local azurerm provider and it works with that field being optional.

This is the task where I have reported this some weeks ago, but not addressed yet:
#20967

[Drilli]

Hi @Drilli, whether the environment is china or not, this issue was caused by a nil sql_license_type returned by the service side, so I believe this nil check will fix the panic you've seen. The question is, I'm convinced to believe that sql_license_type is not required but optional because I did create a such resource with a null value. This patch still could cause issues, since it has the following restriction:

"sql_license_type": {
				Type:     pluginsdk.TypeString,
				Required: true,
				ForceNew: true,
				ValidateFunc: validation.StringInSlice([]string{
					string(sqlvirtualmachines.SqlServerLicenseTypePAYG),
					string(sqlvirtualmachines.SqlServerLicenseTypeAHUB),
					string(sqlvirtualmachines.SqlServerLicenseTypeDR),
				}, false),
			},

If we do set this field to an empty string as this patch did, then the value just violates the validation. Should we change the schema?

And for the fix, let`s see but I think it is not going to fix #21137. I tried removing "azurerm_mssql_virtual_machine" resource from my terraform script and I still get the plugin crash.

[aristosvo]

@lonegunmanb @Drilli I made sql_license_type optional, I'll start the test suite to see if it has any drawbacks.

[lonegunmanb]

Hi @Drilli this resource has been recorded in your state, removing it from your code would cause a resource deletion, are you sure that's what you want?
If so, then maybe try refresh=false to avoid reading status from the api? Again, it would cause your resource loss so please be careful.

[lonegunmanb]

I have cloned the code locally, changed it to optional and tried using my own local azurerm provider and it works with that field being optional.

I think maybe you can make your homebrew provider base on this pr's branch to solve your panic issue first? @Drilli

[aristosvo]

Test suite run was successful, one failure for a test which is also failing for the main branch:

[katbyte]

THanks @aristosvo - LGTM ☔

[Zurina]

Related issue here: #21181

[Drilli]

I can confirm this is working now. I cloned main branch and used that as custom local azurerm provider. It is now working. Both issues have been fixed:

1. Plugin crash
2. sql_license_type optional,

Thanks guys.

[github-actions]

This functionality has been released in v3.50.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.