[Bug:] `azurerm_synapse_sql_pool` - correct the behavior of `geo_backup_policy_enabled` #23217

WodansSon · 2023-09-07T23:53:44Z

Resource behavioral changes in this PR:

Exposes the new storage_account_type field which in v3.0 will inherit the default value of GRS, which matches the current resource and the API's behavior. In v4.0 of the provider the storage_account_type field will become a Required field. In both v3.0 and v4.0 this field is exposed as ForceNew which will trigger a destroy and re-create if the value changes (e.g., GRS -> LRS).
The geo_backup_enabled field remains unchanged in both v3.0 and v4.0 of the provider and continues to be an Optional field with the default value of GRS.
Enables the ability to create a Synapse Sql Pool resource with the storage_account_type of LRS.
Prevents the resource, in v3.0 and v4.0, from being created in an invalid state (e.g., storage_account_type = "LRS" and geo_backup_enabled = true).
Allows previously existing resources to remain, as is without error, while also enforcing the correct behavior on all net new Synapse Sql Pool resources.
Allows the toggling of geo_backup_enabled field, in both v3.0 and v4.0, if the storage_account_type is GRS.

WodansSon · 2023-09-11T19:03:34Z

jackofallops

Thanks @WodansSon for the changes and the continued effort on this one. I've left a few comments inline below, but this is looking pretty much there now, and should be good to go after those.

jackofallops · 2023-09-20T11:04:09Z

internal/services/synapse/synapse_sql_pool_resource.go

+		resource.Schema["storage_account_type"] = &pluginsdk.Schema{
+			Type:     pluginsdk.TypeString,
+			Default:  string(synapse.StorageAccountTypeGRS),
+			Optional: true,
+			ValidateFunc: validation.StringInSlice([]string{
+				string(synapse.StorageAccountTypeLRS),
+				string(synapse.StorageAccountTypeGRS),
+			}, false),
+		}


I think this will be safe to make ForceNew too, since this can only ever have been GRS in the past?

Since we have an Optional field here with a Default of the only value it could have been in the past, we'd be fine to simply set ForceNew on the schema and not worry about the state (since the value read from the API will match the Default value, the state will just be updated as there's no diff)

jackofallops · 2023-09-20T12:30:58Z

website/docs/r/synapse_sql_pool.html.markdown

@@ -10,6 +10,8 @@ description: |-

 Manages a Synapse SQL Pool.

+!> **BREAKING CHANGE:** In v4.0 of the AzureRM Provider, the `storage_account_type` field will be `Required`. Changing the `storage_account_type`, in the v4.0 AzureRM Provider, will force a new Synapse SQL Pool to be created.


In retrospect, we can omit this as it will be included in the upgrade guide for 4.0.

Suggested change

!> **BREAKING CHANGE:** In v4.0 of the AzureRM Provider, the `storage_account_type` field will be `Required`. Changing the `storage_account_type`, in the v4.0 AzureRM Provider, will force a new Synapse SQL Pool to be created.

jackofallops · 2023-09-20T14:17:15Z

internal/services/synapse/synapse_sql_pool_resource.go

+	// We only need to do this check for v3.0 providers since in v4.0
+	// this field will be Required ForceNew...
+	if !features.FourPointOhBeta() {
+		if d.HasChange("storage_account_type") {
+			oldStorageType, newStorageType := d.GetChange("storage_account_type")
+			return fmt.Errorf("the `storage_account_type` cannot be changed once it has been set, was `%s` got `%s`", oldStorageType.(string), newStorageType.(string))
+		}
+	}
+


If we make the property ForceNew as described above, we can remove this.

jackofallops

Thanks @WodansSon for the updates. Just a couple more things to look at below. The only scenario not catered for by the changes suggested below is for cases where a user has imported an LRS backed resource outside of Terraform, and I think we can have a note in the changelog to call this out for users in this edge case scenario? WDTY?

jackofallops · 2023-09-21T12:06:29Z

internal/services/synapse/synapse_sql_pool_resource.go

+		resource.Schema["storage_account_type"] = &pluginsdk.Schema{
+			Type:     pluginsdk.TypeString,
+			Default:  string(synapse.StorageAccountTypeGRS),
+			Optional: true,
+			ValidateFunc: validation.StringInSlice([]string{
+				string(synapse.StorageAccountTypeLRS),
+				string(synapse.StorageAccountTypeGRS),
+			}, false),
+		}


Since we have an Optional field here with a Default of the only value it could have been in the past, we'd be fine to simply set ForceNew on the schema and not worry about the state (since the value read from the API will match the Default value, the state will just be updated as there's no diff)

jackofallops · 2023-09-21T13:06:38Z

internal/services/synapse/synapse_sql_pool_resource.go

+	if !features.FourPointOhBeta() {
+		var oldValue interface{}
+
+		// Since the pre-existing resources will not have a value for this field
+		// in the state file, we need to create a fake state entry for it here
+		// which contains the default GRS value.
+		if oldValue, _ = d.GetChange("storage_account_type"); len(oldValue.(string)) == 0 {
+			oldValue = string(synapse.StorageAccountTypeGRS)
+		}
+
+		// If the value changes (e.g., GRS -> LRS) destroy and re-create the resource.
+		// This aligns the v3.0 behavior with the v4.0 behavior while not impacting the
+		// existing resources unless they attempt to change the underlying storage
+		// account type for the Sql Pool...
+		if oldValue.(string) != value.(string) {
+			return d.ForceNew("storage_account_type")
+		}
 	}


As noted above, we can set this on the schema and don't need to do anything with it here.

WodansSon · 2023-09-21T21:39:46Z

katbyte

LGTM 💾

<Actions> <action id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>"hashicorp/azurerm" updated from "3.73.0" to "3.74.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.74.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.74.0
NOTES:

* `azurerm_synapse_sql_pool` - users that have imported `azurerm_synapse_sql_pool` resources that were created outside of Terraform using an `LRS` storage account type will need to use `ignore_changes` to avoid the resource from being destroyed and recreated.

FEATURES:

* **New Resource**: `azurerm_arc_resource_bridge_appliance` ([#23108](hashicorp/terraform-provider-azurerm#23108 **New Resource**: `azurerm_data_factory_dataset_azure_sql_table` ([#23264](hashicorp/terraform-provider-azurerm#23264 **New Resource**: `azurerm_function_app_connection` ([#23127](https://github.com/hashicorp/terraform-provider-azurerm/issues/23127))

ENHANCEMENTS:

* dependencies: updating to `v0.20230918.1115907` of `github.com/hashicorp/go-azure-sdk` ([#23337](hashicorp/terraform-provider-azurerm#23337 dependencies: downgrading to `v1.12.5` of `github.com/rickb777/date` ([#23296](hashicorp/terraform-provider-azurerm#23296 `mysql`: updating to use API Version `2022-01-01` ([#23320](hashicorp/terraform-provider-azurerm#23320 `azurerm_app_configuration` - support for the `replica` block ([#22452](hashicorp/terraform-provider-azurerm#22452 `azurerm_bot_channel_directline` - support for `user_upload_enabled`, `endpoint_parameters_enabled`, and `storage_enabled` ([#23149](hashicorp/terraform-provider-azurerm#23149 `azurerm_container_app` - support for scale rules ([#23294](hashicorp/terraform-provider-azurerm#23294 `azurerm_container_app_environment` - support for zone redundancy ([#23313](hashicorp/terraform-provider-azurerm#23313 `azurerm_container_group` - support for the `key_vault_user_identity_id` property for Customer Managed Keys ([#23332](hashicorp/terraform-provider-azurerm#23332 `azurerm_cosmosdb_account` - support for MongoDB connection strings ([#23331](hashicorp/terraform-provider-azurerm#23331 `azurerm_data_factory_dataset_delimited_text` - support for the `dynamic_file_system_enabled`, `dynamic_path_enabled`, and `dynamic_filename_enabled` properties ([#23261](hashicorp/terraform-provider-azurerm#23261 `azurerm_data_factory_dataset_parquet` - support for the `azure_blob_fs_location` block ([#23261](hashicorp/terraform-provider-azurerm#23261 `azurerm_monitor_diagnostic_setting` - validation to ensure either `category` or `category_group` are supplied in `enabled_log` and `log` blocks ([#23308](hashicorp/terraform-provider-azurerm#23308 `azurerm_network_interface` - support for the `auxiliary_mode` and `auxiliary_sku` properties ([#22979](hashicorp/terraform-provider-azurerm#22979 `azurerm_postgresql_flexible_server` - increased the maximum supported value for `storage_mb` ([#23277](hashicorp/terraform-provider-azurerm#23277 `azurerm_shared_image_version` - support for the `replicated_region_deletion_enabled` and `target_region.exclude_from_latest_enabled` properties ([#23147](hashicorp/terraform-provider-azurerm#23147 `azurerm_storage_account` - support for setting `domain_name` and `domain_guid` for `AADKERB` ([#22833](hashicorp/terraform-provider-azurerm#22833 `azurerm_storage_account_customer_managed_key` - support for cross-tenant customer-managed keys with the `federated_identity_client_id`, and `key_vault_uri` properties ([#20356](hashicorp/terraform-provider-azurerm#20356 `azurerm_web_application_firewall_policy` - support for the `rate_limit_duration`, `rate_limit_threshold`, `group_rate_limit_by`, and `request_body_inspect_limit_in_kb` properties ([#23239](https://github.com/hashicorp/terraform-provider-azurerm/issues/23239))

BUG FIXES:

* Data Source: `azurerm_container_app_environment`: fix `log_analytics_workspace_name` output to correct value ([#23298](hashicorp/terraform-provider-azurerm#23298 `azurerm_api_management_api` - set the `service_url` property when importing the resource ([#23011](hashicorp/terraform-provider-azurerm#23011 `azurerm_app_configuration` - prevent crash by nil checking the encryption configuration ([#23302](hashicorp/terraform-provider-azurerm#23302 `azurerm_app_configuration_feature` - update `percentage_filter_value` to accept correct type of float ([#23263](hashicorp/terraform-provider-azurerm#23263 `azurerm_container_app` - fix an issue with `commands` and `args` being overwritten when using multiple containers ([#23338](hashicorp/terraform-provider-azurerm#23338 `azurerm_key_vault_certificate` - fix issue where certificates couldn't be recovered anymore ([#23204](hashicorp/terraform-provider-azurerm#23204 `azurerm_key_vault_key` - the ForceNew when `expiration_date` is removed from the config file ([#23327](hashicorp/terraform-provider-azurerm#23327 `azurerm_linux_function_app` - fix a bug in setting the storage settings when using Elastic Premium plans ([#21212](hashicorp/terraform-provider-azurerm#21212 `azurerm_linux_web_app` - fix docker app stack update ([#23303](hashicorp/terraform-provider-azurerm#23303 `azurerm_linux_web_app` - fix crash in auto heal expansion ([#21328](hashicorp/terraform-provider-azurerm#21328 `azurerm_linux_web_app_slot` - fix docker app stack update ([#23303](hashicorp/terraform-provider-azurerm#23303 `azurerm_linux_web_app_slot` - fix crash in auto heal expansion ([#21328](hashicorp/terraform-provider-azurerm#21328 `azurerm_log_analytics_solution` - fix bug where the resource wasn't handling successful creation on subsequent applies ([#23312](hashicorp/terraform-provider-azurerm#23312 `azurerm_management_group_subscription_association` - fix bug to correctly mark resource as gone if not found during read ([#23335](hashicorp/terraform-provider-azurerm#23335 `azurerm_mssql_elasticpool` - remove check that prevents `license_type` from being set for certain skus ([#23262](hashicorp/terraform-provider-azurerm#23262 `azurerm_servicebus_queue` - fixing an issue where `auto_delete_on_idle` couldn't be set to `P10675199DT2H48M5.4775807S` ([#23296](hashicorp/terraform-provider-azurerm#23296 `azurerm_servicebus_topic` - fixing an issue where `auto_delete_on_idle` couldn't be set to `P10675199DT2H48M5.4775807S` ([#23296](hashicorp/terraform-provider-azurerm#23296 `azurerm_storage_account` - prevent sending unsupported blob properties in payload for `Storage` account kind ([#23288](hashicorp/terraform-provider-azurerm#23288 `azurerm_synapse_sql_pool` - expose `storage_account_type` ([#23217](hashicorp/terraform-provider-azurerm#23217 `azurerm_windows_function_app` - fix a bug in setting the storage settings when using Elastic Premium plans ([#21212](hashicorp/terraform-provider-azurerm#21212 `azurerm_windows_web_app` - fix docker app stack update ([#23303](hashicorp/terraform-provider-azurerm#23303 `azurerm_windows_web_app_slot` - fix docker app stack update ([#23303](https://github.com/hashicorp/terraform-provider-azurerm/issues/23303))

DEPRECATIONS:

* `azurerm_application_gateway` - deprecate `Standard` and `WAF` skus ([#23310](hashicorp/terraform-provider-azurerm#23310 `azurerm_bot_channel_web_chat` - deprecate `site_names` in favour of `site` block ([#23161](hashicorp/terraform-provider-azurerm#23161 `azurerm_monitor_diagnostic_setting` - deprecate `retention_policy` in favour of `azurerm_storage_management_policy` ([#23260](https://github.com/hashicorp/terraform-provider-azurerm/issues/23260))


</pre> </details> </details> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>

github-actions · 2024-05-13T02:11:36Z

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.

Initial Check-in...

780d734

WodansSon added documentation service/synapse labels Sep 7, 2023

github-actions bot added the size/M label Sep 7, 2023

WodansSon marked this pull request as draft September 7, 2023 23:54

Fix up test cases...

6cfff9e

github-actions bot added the service/stream-analytics label Sep 8, 2023

WodansSon marked this pull request as ready for review September 8, 2023 01:00

Wrap behavior change in feature flag...

74de63f

github-actions bot added size/L and removed size/M labels Sep 8, 2023

WodansSon added 3 commits September 8, 2023 16:25

Aligning Synapse test case RG names...

374fd56

Update doc and error msg...

1fd37df

Minor doc fix...

db4aaee

Refactor code for new behavior...

0995d9d

github-actions bot added size/XL waiting-response and removed size/L labels Sep 20, 2023

Fix lint issues...

a441078

github-actions bot added waiting-response and removed waiting-response labels Sep 20, 2023

Fix test lint errors...

acab02f

github-actions bot removed the waiting-response label Sep 20, 2023

jackofallops requested changes Sep 20, 2023

View reviewed changes

github-actions bot added the waiting-response label Sep 20, 2023

Address PR comments...

9d900f6

github-actions bot removed the waiting-response label Sep 21, 2023

jackofallops requested changes Sep 21, 2023

View reviewed changes

github-actions bot added the waiting-response label Sep 21, 2023

Address PR comments...

83371e6

github-actions bot removed the waiting-response label Sep 21, 2023

katbyte approved these changes Sep 22, 2023

View reviewed changes

WodansSon merged commit 165e9e4 into main Sep 22, 2023
23 checks passed

WodansSon deleted the b_synapse_sql_pool branch September 22, 2023 05:02

github-actions bot added this to the v3.74.0 milestone Sep 22, 2023

WodansSon added a commit that referenced this pull request Sep 22, 2023

updating to include #23217

9771ac3

WodansSon added a commit that referenced this pull request Sep 22, 2023

updating note for #23217

c00a18f

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug:] `azurerm_synapse_sql_pool` - correct the behavior of `geo_backup_policy_enabled` #23217

[Bug:] `azurerm_synapse_sql_pool` - correct the behavior of `geo_backup_policy_enabled` #23217

WodansSon commented Sep 7, 2023 •

edited

Loading

WodansSon commented Sep 11, 2023

jackofallops left a comment

jackofallops Sep 20, 2023

WodansSon Sep 21, 2023

jackofallops Sep 21, 2023

jackofallops Sep 20, 2023

WodansSon Sep 21, 2023

jackofallops Sep 20, 2023

WodansSon Sep 21, 2023

jackofallops left a comment

jackofallops Sep 21, 2023

jackofallops Sep 21, 2023

WodansSon Sep 21, 2023

WodansSon commented Sep 21, 2023

katbyte left a comment

github-actions bot commented May 13, 2024

		@@ -10,6 +10,8 @@ description: \|-

		Manages a Synapse SQL Pool.

		!> BREAKING CHANGE: In v4.0 of the AzureRM Provider, the `storage_account_type` field will be `Required`. Changing the `storage_account_type`, in the v4.0 AzureRM Provider, will force a new Synapse SQL Pool to be created.

[Bug:] azurerm_synapse_sql_pool - correct the behavior of geo_backup_policy_enabled #23217

[Bug:] azurerm_synapse_sql_pool - correct the behavior of geo_backup_policy_enabled #23217

Conversation

WodansSon commented Sep 7, 2023 • edited Loading

WodansSon commented Sep 11, 2023

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon commented Sep 21, 2023

katbyte left a comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2024

[Bug:] `azurerm_synapse_sql_pool` - correct the behavior of `geo_backup_policy_enabled` #23217

[Bug:] `azurerm_synapse_sql_pool` - correct the behavior of `geo_backup_policy_enabled` #23217

WodansSon commented Sep 7, 2023 •

edited

Loading