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.

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

linux/windows_virtual_machine_sccale_set: define Hash function for extension block to ignore protected_setting #11832

Closed
wants to merge 4 commits into from

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented May 24, 2021

Fixes: #11820.

The hash function is similar to the old vmss below, with some minor diffs due to schema diffs: https://github.com/terraform-providers/terraform-provider-azurerm/blob/84d710e21fca7abcf36f513f1c278c3151721fb1/azurerm/internal/services/compute/virtual_machine_scale_set_resource.go#L774

Meanwhile, this PR removes some unnecessary ignores which are introduced in #11425.

Test Result

💤 TF_ACC=1 go test -v -timeout=3h ./azurerm/internal/services/compute -run="TestAccLinuxVirtualMachineScaleSet_extensions|TestAccWindowsVirtualMachineScaleSet_extensions"
2021/05/24 12:57:17 [DEBUG] not using binary driver name, it's no longer needed
2021/05/24 12:57:17 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccLinuxVirtualMachineScaleSet_extensionsMultiple
=== PAUSE TestAccLinuxVirtualMachineScaleSet_extensionsMultiple
=== RUN   TestAccLinuxVirtualMachineScaleSet_extensionsUpdate
=== PAUSE TestAccLinuxVirtualMachineScaleSet_extensionsUpdate                                                                                                                                                                                               === RUN   TestAccLinuxVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension                                                                                                                                                                    === PAUSE TestAccLinuxVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension
=== RUN   TestAccLinuxVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension
=== PAUSE TestAccLinuxVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension
=== RUN   TestAccWindowsVirtualMachineScaleSet_extensionsDoNotRunOnOverProvisionedMachinesUpdate
=== PAUSE TestAccWindowsVirtualMachineScaleSet_extensionsDoNotRunOnOverProvisionedMachinesUpdate
=== RUN   TestAccWindowsVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension
=== PAUSE TestAccWindowsVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension
=== RUN   TestAccWindowsVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension
=== PAUSE TestAccWindowsVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension
=== CONT  TestAccLinuxVirtualMachineScaleSet_extensionsMultiple
=== CONT  TestAccWindowsVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension
=== CONT  TestAccWindowsVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension
=== CONT  TestAccWindowsVirtualMachineScaleSet_extensionsDoNotRunOnOverProvisionedMachinesUpdate                                                                                                                                                            === CONT  TestAccLinuxVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension                                                                                                                                                                  === CONT  TestAccLinuxVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension
=== CONT  TestAccLinuxVirtualMachineScaleSet_extensionsUpdate
=== CONT  TestAccWindowsVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension
    testing.go:620: Step 1/2 error: Error running apply: exit status 1

        Error: Error creating Windows Virtual Machine Scale Set "acctvm21" (Resource Group "acctestRG-210524125717280510"): compute.VirtualMachineScaleSetsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="BadRequest" Message="windowsConfiguration.enableAutomaticUpdates cannot be set to true if upgradePolicy.automaticOSUpgradePolicy.enableAutomaticOSUpgrade is already true, starting from API version 2019-03-01"

 on terraform_plugin_test.tf line 31, in resource "azurerm_windows_virtual_machine_scale_set" "test":
          31: resource "azurerm_windows_virtual_machine_scale_set" "test" {


--- FAIL: TestAccWindowsVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension (145.64s)
--- PASS: TestAccLinuxVirtualMachineScaleSet_extensionsMultiple (319.79s)
--- PASS: TestAccLinuxVirtualMachineScaleSet_extensionsAutomaticUpgradeWithHealthExtension (332.66s)
--- PASS: TestAccLinuxVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension (345.20s)
--- PASS: TestAccWindowsVirtualMachineScaleSet_extensionsRollingUpgradeWithHealthExtension (485.79s)
--- PASS: TestAccWindowsVirtualMachineScaleSet_extensionsDoNotRunOnOverProvisionedMachinesUpdate (510.60s)
--- PASS: TestAccLinuxVirtualMachineScaleSet_extensionsUpdate (774.25s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/compute     774.427s
FAIL

The only failure above is unrelated.

@ghost ghost added the size/S label May 24, 2021
@magodo magodo added regression service/vmss Virtual Machine Scale Sets labels May 24, 2021
Comment on lines 1583 to 1594
// we need to ensure the whitespace is consistent
settings := m["settings"].(string)
if settings != "" {
expandedSettings, err := structure.ExpandJsonFromString(settings)
if err == nil {
serializedSettings, err := structure.FlattenJsonToString(expandedSettings)
if err == nil {
buf.WriteString(fmt.Sprintf("%s-", serializedSettings))
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since protected settings isn't returned this means we won't track any changes to that field, which is something we want to avoid - instead we'd need to conditionally add that to the hash here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tombuildsstuff

I've updated the code to cache the existing extension settings in state, keyed by the extension name. The extension name is guaranteed to be unique in Azure today, otherwise, you'll get:

Code="BadRequest" Message="On resource 'foobar', extension name 'CustomScript' cannot be used for more than one extension."

During the flattening of the protected_setting, it will try to set it as long as there is something being set in the state. In v2.60.0, after changing the extension from List to Set, the protected_settings are not set back to the ResourceData during a Read/Import, while it is set into the state during the create/update (persumably because the Read got invoked during these operations will will read from source of config, which ends up being persisted into the state). Therefore, this fix should the state resulted into running the provisioning using v2.60.0 (verified locally).

@ghost ghost added size/M and removed size/S labels May 24, 2021
@hbuckle
Copy link
Contributor

hbuckle commented Jun 2, 2021

@tombuildsstuff any chance of getting this in for 2.62.0?

@magodo
Copy link
Collaborator Author

magodo commented Jun 2, 2021

Ping @tombuildsstuff, is there anything I can do to make it merged?

@hbuckle
Copy link
Contributor

hbuckle commented Jun 8, 2021

2.63.0 maybe?

@hbuckle
Copy link
Contributor

hbuckle commented Jun 15, 2021

@tombuildsstuff - could you re-review?

@hbuckle
Copy link
Contributor

hbuckle commented Jun 23, 2021

@tombuildsstuff any chance of getting this merged? I think it is the last thing needed to be able to move off the deprecated scale set resource

@hbuckle
Copy link
Contributor

hbuckle commented Jul 6, 2021

@katbyte - are you able to help with this one?

@hbuckle
Copy link
Contributor

hbuckle commented Jul 26, 2021

Paging @tombuildsstuff

@tombuildsstuff tombuildsstuff added this to the v2.71.0 milestone Aug 4, 2021
@tombuildsstuff tombuildsstuff self-assigned this Aug 4, 2021
@katbyte katbyte modified the milestones: v2.71.0, v2.72.0 Aug 6, 2021
@magodo magodo force-pushed the vmss_extension_hash branch from 212e602 to d90c39c Compare August 11, 2021 01:14
@tombuildsstuff tombuildsstuff modified the milestones: v2.72.0, v2.73.0 Aug 12, 2021
@katbyte katbyte modified the milestones: v2.73.0, v2.74.0 Aug 20, 2021
@hbuckle
Copy link
Contributor

hbuckle commented Aug 23, 2021

@katbyte @tombuildsstuff - any chance of getting this merged? Being stuck on the old scale set resource is causing us real problems now

@katbyte katbyte modified the milestones: v2.74.0, v2.75.0 Aug 27, 2021
@katbyte katbyte added this to the v2.76.0 milestone Sep 2, 2021
@katbyte katbyte modified the milestones: v2.76.0, v2.77.0 Sep 10, 2021
@hbuckle
Copy link
Contributor

hbuckle commented Sep 15, 2021

@tombuildsstuff @katbyte - bumping this one again...

@katbyte
Copy link
Collaborator

katbyte commented Sep 22, 2021

@hbuckle - @mbfrahry as opened #13440 to get this in this week as such i'm going to close this, thanks for getting it started!!

@katbyte katbyte closed this Sep 22, 2021
@github-actions
Copy link

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

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression service/vmss Virtual Machine Scale Sets size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_windows_virtual_machine_scale_set perpetual diff with extension protected_settings
5 participants