Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_log_analytics_workspace - prevent ForceNew when sku is LACluster #19608

Merged
merged 5 commits into from
Feb 21, 2023
Merged

azurerm_log_analytics_workspace - prevent ForceNew when sku is LACluster #19608

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f558ed
`azurerm_log_analytics_workspace` - fix workaround when `sku` is `LAC…
ziyeqf Dec 8, 2022
693448e
add condition
ziyeqf Dec 13, 2022
80d91b1
empty commit to re trigger CI
ziyeqf Dec 15, 2022
38db6a6
Update log_analytics_linked_service_resource_test.go
mbfrahry Feb 21, 2023
197ef8f
Update log_analytics_linked_service_resource_test.go
mbfrahry Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions internal/services/loganalytics/log_analytics_linked_service_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,14 @@ resource "azurerm_log_analytics_workspace" "test" {
resource_group_name = azurerm_resource_group.test.name
sku = "PerGB2018"
retention_in_days = 30

lifecycle {
ignore_changes = [
sku
]
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

How come this is needed now?

Copy link
Contributor Author

@ziyeqf ziyeqf Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger)
}

Expand Down
9 changes: 4 additions & 5 deletions internal/services/loganalytics/log_analytics_workspace_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,13 @@ func resourceLogAnalyticsWorkspaceCustomDiff(ctx context.Context, d *pluginsdk.R
// custom diff here because when you link the workspace to a cluster the
// cluster changes the sku to LACluster, so we need to ignore the change
// if it is LACluster else invoke the ForceNew as before...
//
// NOTE: Since LACluster is not in our enum the value is returned as ""

if d.HasChange("sku") {
old, new := d.GetChange("sku")
log.Printf("[INFO] Log Analytics Workspace SKU: OLD: %q, NEW: %q", old, new)
// If the old value is not LACluster(e.g. "") return ForceNew because they are
// really changing the sku...
if !strings.EqualFold(old.(string), "") {
if !strings.EqualFold(old.(string), string(workspaces.WorkspaceSkuNameEnumLACluster)) && !strings.EqualFold(old.(string), "") {
d.ForceNew("sku")
}
}
Expand Down Expand Up @@ -198,7 +197,7 @@ func resourceLogAnalyticsWorkspaceCreateUpdate(d *pluginsdk.ResourceData, meta i
if err == nil {
if resp.Model != nil && resp.Model.Properties != nil {
if azSku := resp.Model.Properties.Sku; azSku != nil {
if strings.EqualFold(string(azSku.Name), "lacluster") {
if strings.EqualFold(string(azSku.Name), string(workspaces.WorkspaceSkuNameEnumLACluster)) {
isLACluster = true
log.Printf("[INFO] Log Analytics Workspace %q (Resource Group %q): SKU is linked to Log Analytics cluster", name, resourceGroup)
}
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@ziyeqf ziyeqf Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My test steps:

  1. deploy with latest released version.
  2. run terraform plan and then a ForceNew diff shows.
      ~ sku                                = "LACluster" -> "PerGB2018" # forces replacement
  1. plan with updated code, a diff without ForceNew shows.
      ~ sku                        = "LACluster" -> "PerGB2018"
  1. 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@ziyeqf ziyeqf Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if skuName == "" {
// Default value if sku is not defined
sku.Name = workspaces.WorkspaceSkuNameEnumPerGBTwoZeroOneEight
Expand Down