[arm_sql_database] - Make collation case insensitive

[hbuckle]

No description provided.

[tombuildsstuff]

hi @hbuckle

Thanks for this PR :)

Whilst historically we've made some fields in the AzureRM Provider case-insensitive - we've found that this can lead to issues where the casing is important within the Azure API, such that allowing these to be case-insensitive can lead to hard to reproduce issues (for example, the DataBricks API behaves differently for standard vs Standard SKU's) - instead we're raising bugs on the API in question to get these issues fixed.

Out of interest what's the issue/diff that you're seeing here, and are you seeing this on a older resource or a newly created one?

Thanks!

[hbuckle]

This was to address an issue I saw after I created an empty database with Terraform, then restored an existing database from another server over the top of it. The casing of collation in the restored database was different than in the Terraform configuration so Terraform wanted to destroy and recreate the database

resource "azurerm_sql_database" "database" {
  name                = "database"
  resource_group_name = "${azurerm_resource_group.sql.name}"
  location            = "${azurerm_resource_group.sql.location}"
  server_name         = "${azurerm_sql_server.sql_server.name}"
  collation           = "SQL_LATIN1_GENERAL_CP1_CI_AS"
  max_size_bytes      = "268435456000"

  create_mode                      = "Default"
  requested_service_objective_name = "ElasticPool"
  elastic_pool_name                = "${azurerm_mssql_elasticpool.sql.name}"
}

This was the plan output after restoring the other database

module.sql.azurerm_sql_database.database (new resource required)
id:                                 "/subscriptions/***/resourceGroups/***/providers/Microsoft.Sql/servers/***/databases/database" => <computed> (forces new resource)
collation:                          "SQL_Latin1_General_CP1_CI_AS" => "SQL_LATIN1_GENERAL_CP1_CI_AS" (forces new resource)
create_mode:                        "Default" => "Default"
creation_date:                      "2019-03-27T22:39:31.363Z" => <computed>
default_secondary_location:         "UK West" => <computed>
elastic_pool_name:                  "***" => "***"
encryption:                         "" => <computed>
location:                           "uksouth" => "uksouth"
max_size_bytes:                     "268435456000" => "268435456000"
name:                               "database" => "database"
requested_service_objective_id:     "d1737d22-a8ea-4de7-9bd0-33395d2a7419" => <computed>
requested_service_objective_name:   "ElasticPool" => "ElasticPool"
resource_group_name:                "***" => "***"
restore_point_in_time:              "" => <computed>
server_name:                        "***" => "***"
source_database_deletion_date:      "" => <computed>
source_database_id:                 "" => <computed>
threat_detection_policy.#:          "1" => <computed>

[hbuckle]

I wouldn't have thought the API would be case sensitive on collation as it's a database setting, but I agree it's worth checking.
The other thing I thought of is whether SQL server itself cares about the case of the collation setting. I don't think it does, but I'm not a SQL server expert

[ArieHein]

SQL doesn't care about the case of the collation.
It just a matter of how the server was installed.
If the person installing it did it manually from the UI it will choose from a list of available collation, not type it manually.
If the db was created by some automation tool then its whatever was in the documentation of that tool or what every code the programmer copied from the search engine results.

This is actually a great find and great PR that will save me and probably others some headaches

Update: I actually think the
DiffSuppressFunc: suppress.CaseDifference,
should probably be done one most string based variables when dealing with windows resources
if it isn't yet.

Looked at the sqldatabase name and it should have the suppress as well. If the database name went to alternative case after restore.

[katbyte]

@hbuckle & @ArieHein,

We are purposely not moving towards case insensitivity on string fields for a variety of reasons, from API's being inconsistent in their casing to if you did want to change the casing of the coalition, after this PR you can no longer.

Terraform will warn of such changes and in this case you can simply change your terraform config to use SQL_Latin1_General_CP1_CI_AS instead.

[ArieHein]

@katbyte In the case that was presented, it is a valid If I restore a DB, it should not destroy it on next terraform execution just because the collation is in a different case.
Most windows resources dont care about the case, for example the name of the sqldatabase name
that is checked for case. IT just forces the person restoring the DB, who sometimes has no idea about terraform, making sure he keeps the right casing of the sqlserver name that was "imposed" via earlier terraform execution.

My suggestion was to use it on "most" resource parameters, API might not be one of them.

[hbuckle]

To confirm, I just performed the following test

resource "azurerm_resource_group" "resource_group" {
  name     = "collation"
  location = "uksouth"
}

resource "azurerm_sql_server" "test" {
  name                         = "hbtestcollation"
  resource_group_name          = "${azurerm_resource_group.resource_group.name}"
  location                     = "uksouth"
  version                      = "12.0"
  administrator_login          = "xxx"
  administrator_login_password = "xxx"
}

resource "azurerm_sql_database" "test" {
  name                = "mysqldatabase"
  resource_group_name = "${azurerm_resource_group.resource_group.name}"
  location            = "uksouth"
  server_name         = "${azurerm_sql_server.test.name}"
  collation           = "SQL_LATIN1_GENERAL_CP1_CI_AS"
}

$uri = "https://management.azure.com/subscriptions/xxx/resourceGroups/collation/providers/Microsoft.Sql/servers/hbtestcollation/databases/mysqldatabase?api-version=2017-10-01-preview"
$update = @{
  properties = @{
    collation = "sql_latin1_general_cp1_ci_as"
  }
} | ConvertTo-Json -Depth 99
Invoke-RestMethod $uri -Method Patch -Authentication Bearer -Token $token -Body $update -ContentType "application/json"

Invoke-RestMethod $uri -Authentication Bearer -Token $token -Method Get

SQL_LATIN1_GENERAL_CP1_CI_AS

So it seems the API ignores the case in this instance

[hbuckle]

@tombuildsstuff In light of the above test would it be possible to merge this? If not I'll look into an alternative workaround. Thanks!

[katbyte]

@hbuckle,

Case sensitivity in the provider has been an ongoing debate internally as we would like to generally be one or the other overall. As azure is a mixed bag (with some APIs case matters and other it does not) it rules out the possibility of the provider being primarily case insensitive. Case sensitivity is compatible with a case insensitive API (provided it doesn't change the case!) while the other way around is not.

We strive for consistency across resources and properties so users can make certain assumptions and expect a uniform behaviour even if the APIs might not behave as such. Additionally as APIs evolve and change over time their sensitivity to case may change (as has happened. Switching to more case sensitivity means this is now no longer will affect terraform configurations when the switch. Using collation as an example, it is aware of the casing when created and is not normalized, updates are discarded. But if this were to change we would have to release a new provider version that breaks everyone's existing terraform configurations.

For all of these reasons we have decided to try and take the provider towards more case sensitivity and consistency except in cases where it is absolutely necessary such as when the APIs are broken and require us to ignore it until they are fixed.

I hope this gives some insight into why we are so hesitant to mark any property as case insensitive to even if it may make some workflows easier.

[tombuildsstuff]

hey @hbuckle

Sorry for the delayed movement here - we've been waiting to chat about this as a team.

From our side we're trying to determine our criteria for making a field case sensitive, which we believe boils down to:

• a Resource ID needs to be treated as case sensitive because whilst the Azure API's treat these as case insensitive in some circumstances, in some others (such as dependent resources which take a Resource ID) they're not
• as such fields which are used in the Resource ID (such as the name, resource_group_name etc) need to be case sensitive
• fields which represent Enum/Constant values need to be case sensitive due to previous experiences with the API, where provisioning standard vs Standard both succeed, but provision different results
• other fields which are unrelated to this may be case insensitive where it makes sense, but only where it's confirmed this is necessary (such as this) - this is because in general Terraform is case sensitive, and as such the user experience should match this

As such, in this specific case we believe it makes sense for this field to be case insensitive - since the API ignores the difference on casing during an update. Given this we believe we should make this field case sensitive (and merge this PR) to ensure the behaviour matches the Azure API here - which we'll do as a part of #2688.

Apologies for the delay in reviewing this, as mentioned we've been trying to clarify the requirements for case sensitivity.

Thanks!

[ghost]

This has been released in version 1.28.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 = "~> 1.28.0"
}
# ... other configuration ...

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!