search

betr-io / terraform-provider-mssql

Terraform provider for Microsoft SQL Server
https://registry.terraform.io/providers/betr-io/mssql/latest
MIT License
35 stars 28 forks source link

Check if String_Split already exists #40

Closed alxy closed 1 year ago

alxy commented 2 years ago

Potential fix for #31

When multiple mssql_user resources are created using this provider, the compatibility level is less than 130, this seems to be an issue as the provider tried to define the custom function multiple times. I am testing on Azure currently (which seems to have compatibility level 150) so I cannot reliably test if this really solves it.

As the user reported the issue didn't exist on 0.2.3 this seems like a safe addition and shouldn't break anything.

alxy commented 2 years ago

Ok, its actually very easy to change the compatibility level for a database. I can confirm that this fix works.

Previous output when creating multiple users and compatibility level 120:

╷
│ Error: unable to create user [sqldb-test].[my-test-user-0]: mssql: There is already an object named 'String_Split' in the database.
│
│   with mssql_user.example[0],
│   on main.tf line 2, in resource "mssql_user" "example":
│    2: resource "mssql_user" "example" {
│
╵
╷
│ Error: unable to create user [sqldb-test].[my-test-user-5]: mssql: There is already an object named 'String_Split' in the database.
│
│   with mssql_user.example[5],
│   on main.tf line 2, in resource "mssql_user" "example":
│    2: resource "mssql_user" "example" {
│
╵
...

With this change applied:

mssql_user.example[0]: Creating...
mssql_user.example[1]: Creating...
mssql_user.example[3]: Creating...
mssql_user.example[4]: Creating...
mssql_user.example[2]: Creating...
mssql_user.example[0]: Creation complete after 2s [id=sqlserver://mysqlserver.database.windows.net:1433/sqldb-test/my-test-user-0]
mssql_user.example[3]: Creation complete after 2s [id=sqlserver://mysqlserver.database.windows.net:1433/sqldb-test/my-test-user-3]
mssql_user.example[1]: Creation complete after 2s [id=sqlserver://mysqlserver.database.windows.net:1433/sqldb-test/my-test-user-1]
mssql_user.example[2]: Creation complete after 2s [id=sqlserver://mysqlserver.database.windows.net:1433/sqldb-test/my-test-user-2]
mssql_user.example[4]: Creation complete after 2s [id=sqlserver://mysqlserver.database.windows.net:1433/sqldb-test/my-test-user-4]
mkusmiy commented 2 years ago

please explain how does this change eliminate the race condition I would absolutely like the issue with the race condition to be fixed, have spent some time investigating it and commented in corresponding issue without any feedback from anyone. https://github.com/betr-io/terraform-provider-mssql/pull/24 The logic you introduce was already there till v0.2.3 and was changed in v0.2.4. Bringing additional check is good but does not eliminate race condition when two threads at the same time try to create String_Split function. I believe your test case might be inaccurate and that's why your testing passes - during the first terraform run against a DB trying to create multiple users - this function is created on the DB, but TF fails with the race condition issue. However the function is not eliminated on the DB, so during next terraform run using the check you added it passes because the first failing run has created the function.

alxy commented 2 years ago

The problem was for us it also worked on 0.2.3 and stopped working in 0.2.4, and we were always on compatibility level 150. I don't know why this change introduced in #24 caused this, but what I can tell that the current logic is definitely flawed in a sense that it only checks for the compat level, and then ALWAYS adds the function. I'd say the condition needs to be at least two fold with a checl if the function already exists.

I think you might be right with your assumption that my test was inaccurate, as the the function was probably already defined by some previous run. Anyway, this code is still an improvement over the exisitng code I would argue, even though it doesnt fix the race conditon problem.

Thinking about it the best way imo to say that the provider needs at least compatibility level 130, and if it is lower than the user is responsible for defining the missing function. This might be done by the provider you linked. This would also leave the code-base a bit cleaner, and not make the already complex SQL more complex...

magne commented 1 year ago

Abandoned in favor of #52, which has the same implementation and also fixes the concurrency issue.