`azurerm_log_analytics_workspace` - prevent ForceNew when `sku` is `LACluster` #19608

ziyeqf · 2022-12-08T06:58:55Z

Since we have updated to go-azure-sdk, this workaround needs an update.
workaround was invovled in #17069

Test

LogAnalyticsWorkspace

TF_ACC=1 go test -v ./internal/services/loganalytics -run=TestAccLogAnalyticsWorkspace -timeout=600m
=== RUN   TestAccLogAnalyticsWorkspace_basic
=== PAUSE TestAccLogAnalyticsWorkspace_basic
=== RUN   TestAccLogAnalyticsWorkspace_requiresImport
=== PAUSE TestAccLogAnalyticsWorkspace_requiresImport
=== RUN   TestAccLogAnalyticsWorkspace_complete
=== PAUSE TestAccLogAnalyticsWorkspace_complete
=== RUN   TestAccLogAnalyticsWorkspace_freeTier
    log_analytics_workspace_resource_test.go:92: `TestAccLogAnalyticsWorkspace_freeTier` due to subscription pricing tier configuration (e.g. Pricing tier doesn't match the subscription's billing model.)
--- SKIP: TestAccLogAnalyticsWorkspace_freeTier (0.00s)
=== RUN   TestAccLogAnalyticsWorkspace_withDefaultSku
=== PAUSE TestAccLogAnalyticsWorkspace_withDefaultSku
=== RUN   TestAccLogAnalyticsWorkspace_withVolumeCap
=== PAUSE TestAccLogAnalyticsWorkspace_withVolumeCap
=== RUN   TestAccLogAnalyticsWorkspace_removeVolumeCap
=== PAUSE TestAccLogAnalyticsWorkspace_removeVolumeCap
=== RUN   TestAccLogAnalyticsWorkspace_withInternetIngestionEnabled
=== PAUSE TestAccLogAnalyticsWorkspace_withInternetIngestionEnabled
=== RUN   TestAccLogAnalyticsWorkspace_withInternetQueryEnabled
=== PAUSE TestAccLogAnalyticsWorkspace_withInternetQueryEnabled
=== RUN   TestAccLogAnalyticsWorkspace_withCapacityReservation
=== PAUSE TestAccLogAnalyticsWorkspace_withCapacityReservation
=== RUN   TestAccLogAnalyticsWorkspace_negativeOne
=== PAUSE TestAccLogAnalyticsWorkspace_negativeOne
=== RUN   TestAccLogAnalyticsWorkspace_cmkForQueryForced
=== PAUSE TestAccLogAnalyticsWorkspace_cmkForQueryForced
=== CONT  TestAccLogAnalyticsWorkspace_basic
=== CONT  TestAccLogAnalyticsWorkspace_withInternetIngestionEnabled
=== CONT  TestAccLogAnalyticsWorkspace_negativeOne
=== CONT  TestAccLogAnalyticsWorkspace_withDefaultSku
=== CONT  TestAccLogAnalyticsWorkspace_requiresImport
=== CONT  TestAccLogAnalyticsWorkspace_cmkForQueryForced
=== CONT  TestAccLogAnalyticsWorkspace_removeVolumeCap
=== CONT  TestAccLogAnalyticsWorkspace_withVolumeCap
--- PASS: TestAccLogAnalyticsWorkspace_basic (190.72s)
=== CONT  TestAccLogAnalyticsWorkspace_withCapacityReservation
--- PASS: TestAccLogAnalyticsWorkspace_negativeOne (191.73s)
=== CONT  TestAccLogAnalyticsWorkspace_withInternetQueryEnabled
--- PASS: TestAccLogAnalyticsWorkspace_withVolumeCap (251.00s)
=== CONT  TestAccLogAnalyticsWorkspace_complete
--- PASS: TestAccLogAnalyticsWorkspace_withDefaultSku (256.03s)
--- PASS: TestAccLogAnalyticsWorkspace_requiresImport (294.43s)
--- PASS: TestAccLogAnalyticsWorkspace_withInternetIngestionEnabled (365.87s)
--- PASS: TestAccLogAnalyticsWorkspace_removeVolumeCap (367.59s)
--- PASS: TestAccLogAnalyticsWorkspace_cmkForQueryForced (371.00s)
--- PASS: TestAccLogAnalyticsWorkspace_withCapacityReservation (329.00s)
--- PASS: TestAccLogAnalyticsWorkspace_withInternetQueryEnabled (341.60s)
--- PASS: TestAccLogAnalyticsWorkspace_complete (293.67s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/loganalytics	545.855s

test with a cluster which will set sku to LACluster

TF_ACC=1 go test -v ./internal/services/loganalytics -run=TestAccLogAnalyticsLinkedService_withWriteAccessResourceId -timeout=600m
=== RUN   TestAccLogAnalyticsLinkedService_withWriteAccessResourceId
=== PAUSE TestAccLogAnalyticsLinkedService_withWriteAccessResourceId
=== CONT  TestAccLogAnalyticsLinkedService_withWriteAccessResourceId
--- PASS: TestAccLogAnalyticsLinkedService_withWriteAccessResourceId (1460.34s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/loganalytics  1461.482s

…luster`

tombuildsstuff

hey @ziyeqf

Thanks for this PR - I've left a comment inline but if we can fix that up then we should be able to take another look here 👍

Thanks!

internal/services/loganalytics/log_analytics_workspace_resource.go

katbyte · 2022-12-15T21:28:06Z

internal/services/loganalytics/log_analytics_linked_service_resource_test.go

+  lifecycle {
+    ignore_changes = [
+      sku
+    ]
+  }
 }


if this worked before this means you are going to affect peoples configurations?

How come this is needed now?

Per the description in #17069, it used to make it not "force new", but still changed. then, due to SDK upgraded, it becomes "force new" again. fix this workaround only to make it not "force new", "ignore_changes" is still needed.

I have no idea what's this acctest status before sdk upgradtion, because it has been skipped on TC...But on current main branch we can see it fails and the property shows "force replcement"

TF_ACC=1 go test -v ./internal/services/loganalytics -run=TestAccLogAnalyticsLinkedService_withWriteAccessResourceId -timeout=60m [ main][$][🐹 v1.19.4][⏱ 0ms][16:27:09] === RUN TestAccLogAnalyticsLinkedService_withWriteAccessResourceId === PAUSE TestAccLogAnalyticsLinkedService_withWriteAccessResourceId === CONT TestAccLogAnalyticsLinkedService_withWriteAccessResourceId testcase.go:110: Step 1/2 error: After applying this test step and performing a `terraform refresh`, the plan was not empty. stdout Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place -/+ destroy and then create replacement Terraform will perform the following actions: # azurerm_log_analytics_linked_service.test will be updated in-place ~ resource "azurerm_log_analytics_linked_service" "test" { id = "/subscriptions/{SubscriptionId}/resourceGroups/{rgName}providers/Microsoft.OperationalInsights/workspaces/acctestLAW-221216162722060884/linkedServices/Cluster" name = "Cluster" ~ workspace_id = "/subscriptions/{subscriptionId}/resourceGroups/{rgName}/providers/Microsoft.OperationalInsights/workspaces/acctestLAW-221216162722060884" -> (known after apply) # (2 unchanged attributes hidden) } # azurerm_log_analytics_workspace.test must be replaced -/+ resource "azurerm_log_analytics_workspace" "test" { - cmk_for_query_forced = false -> null ~ id = "/subscriptions/{subscriptionId}/resourceGroups/{rgname}/providers/Microsoft.OperationalInsights/workspaces/acctestLAW-221216162722060884" -> (known after apply) name = "acctestLAW-221216162722060884" ~ primary_shared_key = (sensitive value) + reservation_capacity_in_gb_per_day = (known after apply) ~ secondary_shared_key = (sensitive value) ~ sku = "LACluster" -> "PerGB2018" # forces replacement - tags = {} -> null ~ workspace_id = "XXXXXX" -> (known after apply) # (7 unchanged attributes hidden) } Plan: 1 to add, 1 to change, 1 to destroy.

again, we shouldn't need to ignore changes, its a sign something is wrong with the resource.

As this seems to be a separate issue from the one being fixed in this PR, I'm going to remove this until we dedicate some more time to looking into it

ziyeqf · 2022-12-16T02:14:54Z

Hi @WodansSon, could you please help take a look at this PR. I may misunderstand some codes of the workaround in #17069

WodansSon · 2023-01-10T00:24:42Z

internal/services/loganalytics/log_analytics_workspace_resource.go

@@ -221,7 +220,7 @@ func resourceLogAnalyticsWorkspaceCreateUpdate(d *pluginsdk.ResourceData, meta i
 	t := d.Get("tags").(map[string]interface{})

 	if isLACluster {
-		sku.Name = "lacluster"
+		sku.Name = workspaces.WorkspaceSkuNameEnumLACluster


I believe this maybe a breaking change depending on how the new enum cases the value. In the previous version we used lacluster. So, if there is an existing resource with the sku lacluster and the new value in the enum is LACluster that my cause an issue here?

We'll need to do a state migration and fix it on read perhapes?

@ziyeqf - Are you able to check if this is an issue (i.e. build one with a released version, then manage it with your updated code) and add the state migration if needed?

My test steps:

deploy with latest released version.

run terraform plan and then a ForceNew diff shows.

~ sku = "LACluster" -> "PerGB2018" # forces replacement

plan with updated code, a diff without ForceNew shows.

~ sku = "LACluster" -> "PerGB2018"

apply with updated code, it worked as expected.

My explain is:
After changed to azure-go-sdk, it has begun to set LACluster to state file, which was "" before, then the workaround goes invalid.

For the casing change here, it's only between provider and service, we used to use lacluster in payload, and LACluster now. Per the test result I think it should be ok?

Code ref:

terraform-provider-azurerm/internal/services/loganalytics/log_analytics_workspace_resource.go

Lines 382 to 394 in 4ce0783

skuName := ""

if props.Sku != nil {

sku := *props.Sku

for _, v := range workspaces.PossibleValuesForWorkspaceSkuNameEnum() {

if strings.EqualFold(v, string(sku.Name)) {

skuName = v

}

}

if capacityReservationLevel := sku.CapacityReservationLevel; capacityReservationLevel != nil {

d.Set("reservation_capacity_in_gb_per_day", capacityReservationLevel)

}

}

d.Set("sku", skuName)

Constant ref:
https://github.com/hashicorp/go-azure-sdk/blob/10bd4eb4bbb6822a564b2d0fcdf348e8ed3a8e8c/resource-manager/operationalinsights/2020-08-01/workspaces/constants.go#L177-L186

Hi @ziyeqf - I think you should test with a lacluster value and check the effect of the change to LACluster, since this Enum change will likely result in a diff, which is what I think @WodansSon was pointing out? This may need a state migration to update the value to prevent this.
Thanks!

@jackofallops I don't think there will be a config with sku set to lacluster, it's now allowed in validateFunc and will throw error in plan stage. And for state file, it will be set to LACluster in refresh stage.

@jackofallops I don't think there will be a config with sku set to lacluster, it's not allowed in validateFunc and will throw error in plan stage. And for state file, it will be set to LACluster in refresh stage.

sorry for typo, it's "not" allowed.

previous versions allowed lacluster - we'll need to account for that as we don't know what version someone will be upgrading from so we can't make that asausmption

So.. we need to allow lacluster in validateFunc again?

Apologies for the back and forth @ziyeqf. Your implementation here is correct. We are only setting lacluster during create and update and the api doesn't care about casing. During the read, we are setting sku to LACluster so the end user shouldn't encounter any state changes from this PR

mbfrahry

LGTM

mbfrahry · 2023-02-21T21:08:26Z

internal/services/loganalytics/log_analytics_linked_service_resource_test.go

+  lifecycle {
+    ignore_changes = [
+      sku
+    ]
+  }
 }


As this seems to be a separate issue from the one being fixed in this PR, I'm going to remove this until we dedicate some more time to looking into it

mbfrahry · 2023-02-21T21:14:31Z

internal/services/loganalytics/log_analytics_workspace_resource.go

@@ -221,7 +220,7 @@ func resourceLogAnalyticsWorkspaceCreateUpdate(d *pluginsdk.ResourceData, meta i
 	t := d.Get("tags").(map[string]interface{})

 	if isLACluster {
-		sku.Name = "lacluster"
+		sku.Name = workspaces.WorkspaceSkuNameEnumLACluster


Apologies for the back and forth @ziyeqf. Your implementation here is correct. We are only setting lacluster during create and update and the api doesn't care about casing. During the read, we are setting sku to LACluster so the end user shouldn't encounter any state changes from this PR

github-actions · 2023-02-24T03:40:03Z

This functionality has been released in v3.45.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!

JoshWeepie · 2023-02-28T21:02:27Z

Hi, isn't this 2 year old bug related and the exact same issue? How come this was only done for the LACluster SKU?

#12177

ziyeqf · 2023-03-14T09:01:14Z

@JoshWeepie Thanks for pointing that, the mentioned issue is a little different as there is some limits on changing SKU.
But I may look into #12177. Thanks!

github-actions · 2023-04-14T02:07: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.

azurerm_log_analytics_workspace - fix workaround when sku is `LAC…

2f558ed

…luster`

github-actions bot added service/log-analytics size/XS labels Dec 8, 2022

ziyeqf force-pushed the tengzh/vanguard/sentinel/log_workspace_lacluster branch from 0ae37d3 to 2f558ed Compare December 8, 2022 06:59

tombuildsstuff suggested changes Dec 12, 2022

View reviewed changes

internal/services/loganalytics/log_analytics_workspace_resource.go Outdated Show resolved Hide resolved

github-actions bot added the waiting-response label Dec 12, 2022

add condition

693448e

github-actions bot removed the waiting-response label Dec 13, 2022

empty commit to re trigger CI

80d91b1

ziyeqf mentioned this pull request Dec 15, 2022

New Resource: azurerm_sentinel_log_analytics_workspace_onboarding #19692

Merged

katbyte requested changes Dec 15, 2022

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Dec 15, 2022

katbyte added the waiting-response label Dec 19, 2022

WodansSon reviewed Jan 10, 2023

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Feb 2, 2023

jackofallops self-assigned this Feb 6, 2023

github-actions bot added waiting-response and removed waiting-response labels Feb 17, 2023

mbfrahry added 2 commits February 21, 2023 13:08

Update log_analytics_linked_service_resource_test.go

38db6a6

Update log_analytics_linked_service_resource_test.go

197ef8f

mbfrahry approved these changes Feb 21, 2023

View reviewed changes

mbfrahry changed the title ~~azurerm_log_analytics_workspace - fix workaround when sku is LACluster~~ azurerm_log_analytics_workspace - prevent ForceNew when sku is LACluster Feb 21, 2023

mbfrahry merged commit c2d8266 into hashicorp:main Feb 21, 2023

mbfrahry added a commit that referenced this pull request Feb 21, 2023

Update changelog for #19608

46c208f

github-actions bot added this to the v3.45.0 milestone Feb 21, 2023

ziyeqf deleted the tengzh/vanguard/sentinel/log_workspace_lacluster branch March 14, 2023 09:01

github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_log_analytics_workspace` - prevent ForceNew when `sku` is `LACluster` #19608

`azurerm_log_analytics_workspace` - prevent ForceNew when `sku` is `LACluster` #19608

ziyeqf commented Dec 8, 2022 •

edited

Loading

tombuildsstuff left a comment

katbyte Dec 15, 2022

ziyeqf Dec 16, 2022 •

edited

Loading

katbyte Feb 17, 2023

mbfrahry Feb 21, 2023

ziyeqf commented Dec 16, 2022

WodansSon Jan 10, 2023

katbyte Jan 10, 2023

jackofallops Feb 1, 2023

ziyeqf Feb 2, 2023 •

edited

Loading

jackofallops Feb 6, 2023

ziyeqf Feb 6, 2023 •

edited

Loading

ziyeqf Feb 9, 2023

katbyte Feb 17, 2023

ziyeqf Feb 17, 2023

mbfrahry Feb 21, 2023

mbfrahry left a comment

mbfrahry Feb 21, 2023

mbfrahry Feb 21, 2023

github-actions bot commented Feb 24, 2023

JoshWeepie commented Feb 28, 2023 •

edited

Loading

ziyeqf commented Mar 14, 2023

github-actions bot commented Apr 14, 2023

	skuName := ""
	if props.Sku != nil {
	sku := *props.Sku
	for _, v := range workspaces.PossibleValuesForWorkspaceSkuNameEnum() {
	if strings.EqualFold(v, string(sku.Name)) {
	skuName = v
	}
	}
	if capacityReservationLevel := sku.CapacityReservationLevel; capacityReservationLevel != nil {
	d.Set("reservation_capacity_in_gb_per_day", capacityReservationLevel)
	}
	}
	d.Set("sku", skuName)

azurerm_log_analytics_workspace - prevent ForceNew when sku is LACluster #19608

azurerm_log_analytics_workspace - prevent ForceNew when sku is LACluster #19608

Conversation

ziyeqf commented Dec 8, 2022 • edited Loading

Test

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyeqf Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyeqf commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyeqf Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyeqf Feb 6, 2023 • edited Loading

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

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2023

JoshWeepie commented Feb 28, 2023 • edited Loading

ziyeqf commented Mar 14, 2023

github-actions bot commented Apr 14, 2023

`azurerm_log_analytics_workspace` - prevent ForceNew when `sku` is `LACluster` #19608

`azurerm_log_analytics_workspace` - prevent ForceNew when `sku` is `LACluster` #19608

ziyeqf commented Dec 8, 2022 •

edited

Loading

ziyeqf Dec 16, 2022 •

edited

Loading

ziyeqf Feb 2, 2023 •

edited

Loading

ziyeqf Feb 6, 2023 •

edited

Loading

JoshWeepie commented Feb 28, 2023 •

edited

Loading