azurerm_mssql_database create mode read during import

[aristosvo]

Fixes #11024

[tombuildsstuff]

hey @aristosvo

Thanks for this PR - taking a look through this looks pretty good to me, but if we can move this to the Import function rather than the Read (since this is an import-specific fix) then this should otherwise be good to go 👍

Thanks!

[tombuildsstuff]

@aristosvo I've tested this locally and realised there's some further changes needed here, so I hope you don't mind but I'm going to push a couple of commits to fix this, namely it appears we also need to change the Computed value out for a Default (since the API doesn't return one) and add a state migration here to ensure that's set for older resources - will confirm the upgrade path in a while.

[aristosvo]

No problem! Thanks for taking over!

[aristosvo]

@tombuildsstuff Just curious, the state migration is just adding the previous schema and a migration function to a migration package? Are there specific tricks to take into account?

Looks a bit less complicated than it sounded 👍🏽

[tombuildsstuff]

@aristosvo pretty much, the complexity there is more in terms of testing than the code itself - also de-duping references to the schema, since it's a point-in-time reference to the schema at the time the migration should occur

From a testing perspective it's then confirming the upgrade path - but this is a one-way upgrade so it's a little bit of a pain to test, all in all it's not that involved it's just painful copying/making the point in time reference of the schema :)

[tombuildsstuff]

State migration runs clean:

No changes. Infrastructure is up-to-date.

This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, no
actions need to be performed.

[aristosvo]

@tombuildsstuff Although the code works fine, the initial problem is not solved.

BTW, you probably know everything I write down here, but I'm writing it down for myself to completely understand what's going on 🤣

This is the setup:

variable "instance_name" {
  type    = string
  default = "test-mssql-db-2191"
}
variable "resource_group" {
  type    = string
  default = "test-mssql-db-rg"
}
variable "db_name" {
  type    = string
  default = "test-db"
}
variable "location" {
  type    = string
  default = "westeurope"
}
variable "labels" {
  type = map(any)
  default = {}
}
variable "sku_name" {
  type    = string
  default = "GP_Gen5_2"
}
variable "max_storage_gb" {
  type    = number
  default = 10
}

provider "azurerm" {
  features {}
}

resource "azurerm_resource_group" "azure-sql-fog" {
  name     = var.resource_group
  location = var.location
  tags     = var.labels
}

resource "random_string" "username" {
  length  = 16
  special = false
  number  = false
}

resource "random_password" "password" {
  length           = 64
  override_special = "~_-."
  min_upper        = 2
  min_lower        = 2
  min_special      = 2
}

resource "azurerm_sql_server" "primary_azure_sql_db_server" {
  depends_on                   = [azurerm_resource_group.azure-sql-fog]
  name                         = format("%s-primary", var.instance_name)
  resource_group_name          = var.resource_group
  location                     = var.location
  version                      = "12.0"
  administrator_login          = random_string.username.result
  administrator_login_password = random_password.password.result
  tags                         = var.labels
}

locals {
  default_pair = {
    // https://docs.microsoft.com/en-us/azure/best-practices-availability-paired-regions
    "eastasia"           = "southeastasia"
    "southeastasia"      = "eastasia"
    "centralus"          = "eastus2"
    "eastus"             = "westus"
    "eastus2"            = "centralus"
    "westus"             = "eastus"
    "northcentralus"     = "southcentralus"
    "southcentralus"     = "northcentralus"
    "northeurope"        = "westeurope"
    "westeurope"         = "northeurope"
    "japanwest"          = "japaneast"
    "japaneast"          = "japanwest"
    "brazilsouth"        = "southcentralus"
    "australiaeast"      = "australiasoutheast"
    "australiasoutheast" = "australiaeast"
    "australiacentral"   = "australiacentral2"
    "australiacentral2"  = "australiacentral"
    "southindia"         = "centralindia"
    "centralindia"       = "southindia"
    "westindia"          = "southindia"
    "canadacentral"      = "canadaeast"
    "canadaeast"         = "canadacentral"
    "uksouth"            = "ukwest"
    "ukwest"             = "uksouth"
    "westcentralus"      = "westus2"
    "westus2"            = "westcentralus"
    "koreacentral"       = "koreasouth"
    "koreasouth"         = "koreacentral"
    "francecentral"      = "francesouth"
    "francesouth"        = "francecentral"
    "uaenorth"           = "uaecentral"
    "uaecentral"         = "uaenorth"
    "southafricanorth"   = "southafricawest"
    "southafricawest"    = "southafricanorth"
    "germanycentral"     = "germanynortheast"
    "germanynortheast"   = "germanycentral"
  }
}

resource "azurerm_sql_server" "secondary_sql_db_server" {
  depends_on                   = [azurerm_resource_group.azure-sql-fog]
  name                         = format("%s-secondary", var.instance_name)
  resource_group_name          = var.resource_group
  location                     = local.default_pair[var.location]
  version                      = "12.0"
  administrator_login          = random_string.username.result
  administrator_login_password = random_password.password.result
  tags                         = var.labels
}

resource "azurerm_mssql_database" "azure_sql_db" {
  name        = var.db_name
  server_id   = azurerm_sql_server.primary_azure_sql_db_server.id
  sku_name    = var.sku_name
  max_size_gb = var.max_storage_gb
  tags        = var.labels
}

resource "azurerm_sql_failover_group" "failover_group" {
  depends_on          = [azurerm_resource_group.azure-sql-fog]
  name                = var.instance_name
  resource_group_name = var.resource_group
  server_name         = azurerm_sql_server.primary_azure_sql_db_server.name
  databases           = [azurerm_mssql_database.azure_sql_db.id]
  partner_servers {
    id = azurerm_sql_server.secondary_sql_db_server.id
  }

  read_write_endpoint_failover_policy {
    mode          = "Automatic"
    grace_minutes = 5
  }
}

# resource "azurerm_mssql_database" "secondary_db" {
#   name                = var.db_name
#   server_id           = azurerm_sql_server.secondary_sql_db_server.id
#   sku_name            = var.sku_name
#   max_size_gb         = var.max_storage_gb
#   tags                = var.labels
#   create_mode         = "Secondary"
# }

After the terraform apply, an import (of the uncommented secondary_db) creates a secondary_db in state with create_mode = "Default". During import, when calling value, ok := d.GetOk to get the configuration, ok = true and value = "Default", as just set.

From the documentation of StateFunc as found here:

// StateFunc is the function called to import a resource into the
// Terraform state. It is given a ResourceData with only ID set. This
// ID is going to be an arbitrary value given by the user and may not map
// directly to the ID format that the resource expects, so that should
// be validated.

This means we never can use configuration from our main.tf to initiate our resources. I don't understand why this pattern is used in more SQL resources like azurerm_mysql_server, maybe it used to work differently?

Possible solutions

• check and query for a FailoverGroup, make create_mode secondary if it is a secondary. For different create modes we have to find other patterns to determine the create_mode..

Importer: azSchema.ValidateResourceIDPriorToImportThen(func(id string) error {
	_, err := parse.DatabaseID(id)
	return err
}, func(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
	client := meta.(*clients.Client).MSSQL.DatabasesClient
	failovergroupClient := meta.(*clients.Client).MSSQL.FailoverGroupsClient
	ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d)
	defer cancel()

	id, err := parse.DatabaseID(d.Id())
	if err != nil {
		return nil, err
	}
	resp, err := client.Get(ctx, id.ResourceGroup, id.ServerName, id.Name)
	if err != nil {
		return nil, fmt.Errorf("reading MsSql Database %s (MsSql Server Name %q / Resource Group %q): %s", id.Name, id.ServerName, id.ResourceGroup, err)
	}

	if resp.FailoverGroupID != nil && *resp.FailoverGroupID != "" {
		failoverGroupId, err := parse.FailoverGroupID(*resp.FailoverGroupID)
		if err != nil {
			return nil, err
		}
		respFailoverGroup, err := failovergroupClient.Get(ctx, failoverGroupId.ResourceGroup, id.ServerName, id.ServerName, failoverGroupId.Name)
		if err != nil {
			return nil, err
		}
		if respFailoverGroup.FailoverGroupProperties != nil && *&respFailoverGroup.FailoverGroupProperties.ReplicationRole == "Secondary" {
			d.Set("create_mode", "Secondary")
			return []*schema.ResourceData{d}, nil
		}
	}
	d.Set("create_mode", "Default")

	return []*schema.ResourceData{d}, nil
}),
  

• make create_mode changeable like it is for azurerm_mysql_server resource
• ...?

[aristosvo]

@tombuildsstuff Ping! Is it possible to validate the Possible solutions here, so I can implement one of them?

[katbyte]

@aristosvo - this is similar to what you did with postgres? sounds like a good solution to me 🙂

[aristosvo]

@katbyte Thanks! I've submitted an improved version of my proposal, haven't tested it yet but the foundations is the same/better. Will rebase in a few minutes.

[aristosvo]

Against master:

❯ make acctests SERVICE='mssql' TESTARGS='-run=TestAccMsSqlDatabase_createSecondaryMode\|TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/mssql -run=TestAccMsSqlDatabase_createSecondaryMode\|TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/04/28 15:41:41 [DEBUG] not using binary driver name, it's no longer needed
2021/04/28 15:41:42 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccMsSqlDatabase_createSecondaryMode
=== PAUSE TestAccMsSqlDatabase_createSecondaryMode
=== RUN   TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup
=== PAUSE TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup
=== CONT  TestAccMsSqlDatabase_createSecondaryMode
=== CONT  TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup
    testing_new_import_state.go:249: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=11) "create_mode": (string) (len=9) "Secondary"
        }
=== CONT  TestAccMsSqlDatabase_createSecondaryMode
    testing_new_import_state.go:249: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=11) "create_mode": (string) (len=9) "Secondary"
        }
--- FAIL: TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup (437.81s)
--- FAIL: TestAccMsSqlDatabase_createSecondaryMode (459.95s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/mssql       463.287s
FAIL
make: *** [acctests] Error 1

Against PR branch:

❯ make acctests SERVICE='mssql' TESTARGS='-run=TestAccMsSqlDatabase_createSecondaryMode\|TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/mssql -run=TestAccMsSqlDatabase_createSecondaryMode\|TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/04/28 16:24:14 [DEBUG] not using binary driver name, it's no longer needed
2021/04/28 16:24:15 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccMsSqlDatabase_createSecondaryMode
=== PAUSE TestAccMsSqlDatabase_createSecondaryMode
=== RUN   TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup
=== PAUSE TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup
=== CONT  TestAccMsSqlDatabase_createSecondaryMode
=== CONT  TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup
--- PASS: TestAccMsSqlDatabase_createSecondaryMode (429.97s)
=== CONT  TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup
--- PASS: TestAccMsSqlDatabase_scaleReplicaSetWithFailovergroup (971.87s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/mssql        975.440s

Checked:

• TestAccMsSqlDatabase_createPITRMode -> not affected by change, passing
• TestAccMsSqlDatabase_createCopyMode -> import step ignores create_mode, but this is a minor thingy as create_mode doesn't affect other behaviour in any way for a create_mode = Copy compared with a create_mode = Default

[katbyte]

Thanks @aristosvo - LGTM 👍

[ghost]

This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.57.0"
}
# ... other configuration ...

[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.