Add timezone field to Virtual Machine (Windows Configuration)
#1265
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,9 @@ func resourceArmVirtualMachine() *schema.Resource { | |
| State: schema.ImportStatePassthrough, | ||
| }, | ||
|
|
||
| MigrateState: resourceAzureRMVirtualMachineMigrateState, | ||
| SchemaVersion: 1, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "name": { | ||
| Type: schema.TypeString, | ||
|
|
@@ -374,6 +377,14 @@ func resourceArmVirtualMachine() *schema.Resource { | |
| Optional: true, | ||
| Default: false, | ||
| }, | ||
| "timezone": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ForceNew: true, | ||
| Default: "UTC", | ||
| DiffSuppressFunc: ignoreCaseDiffSuppressFunc, | ||
| ValidateFunc: validateAzureVMTimeZone(), | ||
| }, | ||
| "winrm": { | ||
| Type: schema.TypeList, | ||
| Optional: true, | ||
|
|
@@ -1019,6 +1030,10 @@ func flattenAzureRmVirtualMachineOsProfileWindowsConfiguration(config *compute.W | |
| result["enable_automatic_upgrades"] = *config.EnableAutomaticUpdates | ||
| } | ||
|
|
||
| if config.TimeZone != nil { | ||
| result["timezone"] = *config.TimeZone | ||
| } | ||
|
|
||
| if config.WinRM != nil { | ||
| listeners := make([]map[string]interface{}, 0, len(*config.WinRM.Listeners)) | ||
| for _, i := range *config.WinRM.Listeners { | ||
|
|
@@ -1279,7 +1294,9 @@ func expandAzureRmVirtualMachineOsProfileWindowsConfig(d *schema.ResourceData) ( | |
| osProfilesWindowsConfig := d.Get("os_profile_windows_config").(*schema.Set).List() | ||
|
|
||
| osProfileConfig := osProfilesWindowsConfig[0].(map[string]interface{}) | ||
| config := &compute.WindowsConfiguration{ | ||
| TimeZone: utils.String(osProfileConfig["timezone"].(string)), | ||
| } | ||
|
|
||
| if v := osProfileConfig["provision_vm_agent"]; v != nil { | ||
| provision := v.(bool) | ||
|
|
@@ -1595,6 +1612,7 @@ func resourceArmVirtualMachineStorageOsProfileWindowsConfigHash(v interface{}) i | |
| if v, ok := m["enable_automatic_upgrades"]; ok { | ||
| buf.WriteString(fmt.Sprintf("%t-", v.(bool))) | ||
| } | ||
| buf.WriteString(fmt.Sprintf("%s-", m["timezone"].(string))) | ||
|
There was a problem hiding this comment. there's a crash where where this field doesn't exist - as such we need to nil-check this. There was a problem hiding this comment. But it contains a default value. If the user didn't provide anything, will the default value be populated to the field? What about the root field using d.Get()? How will Terraform handle the default value? There was a problem hiding this comment. It is caused by So what is the recommended way of handling default values in There was a problem hiding this comment.
That's not true for existing values in the state (or, older VM's which didn't expose this field) which won't have this value (and thus will crash) - but we can work around that by nil-checking it There was a problem hiding this comment. given this field is case insensitive in the schema you'll also want to make this value consistent by either upper/lower-casing it; otherwise you'll get spurious diff's here |
||
| } | ||
|
|
||
| return hashcode.String(buf.String()) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package azurerm | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "log" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/hashicorp/terraform/terraform" | ||
| ) | ||
|
|
||
| func resourceAzureRMVirtualMachineMigrateState( | ||
| v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { | ||
| switch v { | ||
| case 0: | ||
| log.Println("[INFO] Found AzureRM Virtual Machine State v0; migrating to v1") | ||
| return migrateAzureRMVirtualMachineStateV0toV1(is) | ||
| default: | ||
| return is, fmt.Errorf("Unexpected schema version: %d", v) | ||
| } | ||
| } | ||
|
|
||
| func migrateAzureRMVirtualMachineStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) { | ||
| if is.Empty() { | ||
| log.Println("[DEBUG] Empty InstanceState; nothing to migrate.") | ||
| return is, nil | ||
| } | ||
|
|
||
| log.Printf("[DEBUG] ARM Virtual Machine Attributes before Migration: %#v", is.Attributes) | ||
|
|
||
| if is.Attributes["os_profile_windows_config.#"] == "1" { | ||
| hash, err := computeAzureRMVirtualMachineStateV1WinConfigHash(is, "UTC") | ||
| if err != nil { | ||
| return is, fmt.Errorf("Failed to calculate the new hash code for os_profile_windows_config field: %#v", err) | ||
| } | ||
| replaceAzureRMVirtualMachineStateV0WinConfigWithNewHash(is, hash) | ||
| tzKey := fmt.Sprintf("%s.%d.%s", "os_profile_windows_config", hash, "timezone") | ||
| is.Attributes[tzKey] = "UTC" | ||
| } | ||
|
|
||
| log.Printf("[DEBUG] ARM Virtual Machine Attributes after State Migration: %#v", is.Attributes) | ||
|
|
||
| return is, nil | ||
| } | ||
|
|
||
| func computeAzureRMVirtualMachineStateV1WinConfigHash(is *terraform.InstanceState, timezone string) (int, error) { | ||
| data := make(map[string]interface{}) | ||
| for k, v := range is.Attributes { | ||
| if !strings.HasPrefix(k, "os_profile_windows_config.") { | ||
| continue | ||
| } | ||
| paths := strings.Split(k, ".") | ||
| if len(paths) == 3 && (paths[2] == "enable_automatic_upgrades" || paths[2] == "provision_vm_agent") { | ||
| b, err := strconv.ParseBool(v) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| data[paths[2]] = b | ||
| } | ||
| } | ||
| data["timezone"] = timezone | ||
| return resourceArmVirtualMachineStorageOsProfileWindowsConfigHash(data), nil | ||
|
There was a problem hiding this comment. The underlying data structures will diverge significantly in a future version of Terraform - as such direct manipulation of the state will cause hard to diagnose crashes later. Instead we need to use the schema parser (example) rather than assuming the format for keys, (as here) This means we can simply assign the new value for the field, and allow Terraform Core to recompute the hash value; meaning the tests should stay the same, but all of this can disappear There was a problem hiding this comment. Nice suggestion. I think this should be mentioned in the guideline as well. There was a problem hiding this comment. (heads up that this needs to be resolved before we can merge this, which is why this is still "requested changes" 😄) Just to add some additional context to testing this; you'll want to compare the state of a VM using the current provider release (currently v1.6.0) to the current development build you're working on; for instance: provider "azurerm" {
version = "=1.6.0"
}
resource "azurerm_resource_group" "test" {
# ..
}
# ...$ rm -rf .terraform/
$ terraform init
$ terraform applythen un-pinning the provider version and re-initing/applying should show the state migration: provider "azurerm" {
}
resource "azurerm_resource_group" "test" {
# ..
}
# ...$ rm -rf .terraform/
$ terraform init # to use the development version you've built via `make`
$ terraform plan|applyIn order to test this effectively unfortunately you'll want to destroy & recreate the VM for each iteration, which the Hope that helps :) There was a problem hiding this comment. Thanks for the explanations, and the test steps is really helpful here 👍 . So AFAIK, we can only do the manual test here for this kind of issues? Are there any unit-test infrastructure support it? There was a problem hiding this comment.
The associated unit test is checking the logic from a given version - this is more exploratory testing / validation (so no, unfortunately not afaik) |
||
| } | ||
|
|
||
| func replaceAzureRMVirtualMachineStateV0WinConfigWithNewHash(is *terraform.InstanceState, hash int) { | ||
| toDel := make([]string, 0) | ||
| toAdd := make(map[string]string) | ||
| for k, v := range is.Attributes { | ||
| if !strings.HasPrefix(k, "os_profile_windows_config.") { | ||
| continue | ||
| } | ||
| paths := strings.Split(k, ".") | ||
| if len(paths) > 2 { | ||
| toDel = append(toDel, k) | ||
| paths[1] = fmt.Sprintf("%d", hash) | ||
| toAdd[strings.Join(paths, ".")] = v | ||
| } | ||
| } | ||
| for _, k := range toDel { | ||
| delete(is.Attributes, k) | ||
| } | ||
| for k, v := range toAdd { | ||
| is.Attributes[k] = v | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. (which means this function can be removed) |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package azurerm | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/hashicorp/terraform/terraform" | ||
| ) | ||
|
|
||
| func TestAzureRMVirtualMachineMigrateState(t *testing.T) { | ||
| cases := map[string]struct { | ||
| StateVersion int | ||
| ID string | ||
| InputAttributes map[string]string | ||
| ExpectedAttributes map[string]string | ||
| Meta interface{} | ||
| }{ | ||
| "v0_1_simple": { | ||
| StateVersion: 0, | ||
| ID: "azurevm-dummy-id", | ||
| InputAttributes: map[string]string{ | ||
| "os_profile_windows_config.#": "1", | ||
| "os_profile_windows_config.429474957.additional_unattend_config.#": "0", | ||
| "os_profile_windows_config.429474957.enable_automatic_upgrades": "false", | ||
| "os_profile_windows_config.429474957.provision_vm_agent": "false", | ||
| "os_profile_windows_config.429474957.winrm.#": "0", | ||
| }, | ||
| ExpectedAttributes: map[string]string{ | ||
| "os_profile_windows_config.#": "1", | ||
| "os_profile_windows_config.2229351482.additional_unattend_config.#": "0", | ||
| "os_profile_windows_config.2229351482.enable_automatic_upgrades": "false", | ||
| "os_profile_windows_config.2229351482.provision_vm_agent": "false", | ||
| "os_profile_windows_config.2229351482.timezone": "UTC", | ||
| "os_profile_windows_config.2229351482.winrm.#": "0", | ||
| }, | ||
| }, | ||
| "v0_1_full": { | ||
| StateVersion: 0, | ||
| ID: "azurevm-dummy-id", | ||
| InputAttributes: map[string]string{ | ||
| "os_profile_windows_config.#": "1", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.#": "2", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.0.component": "shellsetup", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.0.content": "autologon content", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.0.pass": "mypass", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.1.component": "shellsetup2", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.0.setting_name": "AutoLogon", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.1.content": "first logon content", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.1.pass": "mypass2", | ||
| "os_profile_windows_config.2256145325.additional_unattend_config.1.setting_name": "FirstLogonCommands", | ||
| "os_profile_windows_config.2256145325.enable_automatic_upgrades": "true", | ||
| "os_profile_windows_config.2256145325.provision_vm_agent": "true", | ||
| "os_profile_windows_config.2256145325.winrm.#": "1", | ||
| "os_profile_windows_config.2256145325.winrm.0.certificate_url": "", | ||
| "os_profile_windows_config.2256145325.winrm.0.protocol": "http", | ||
| }, | ||
| ExpectedAttributes: map[string]string{ | ||
| "os_profile_windows_config.#": "1", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.#": "2", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.0.component": "shellsetup", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.0.content": "autologon content", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.0.pass": "mypass", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.0.setting_name": "AutoLogon", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.1.component": "shellsetup2", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.1.content": "first logon content", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.1.pass": "mypass2", | ||
| "os_profile_windows_config.1257668355.additional_unattend_config.1.setting_name": "FirstLogonCommands", | ||
| "os_profile_windows_config.1257668355.enable_automatic_upgrades": "true", | ||
| "os_profile_windows_config.1257668355.provision_vm_agent": "true", | ||
| "os_profile_windows_config.1257668355.timezone": "UTC", | ||
| "os_profile_windows_config.1257668355.winrm.#": "1", | ||
| "os_profile_windows_config.1257668355.winrm.0.certificate_url": "", | ||
| "os_profile_windows_config.1257668355.winrm.0.protocol": "http", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for tn, tc := range cases { | ||
| is := &terraform.InstanceState{ | ||
| ID: tc.ID, | ||
| Attributes: tc.InputAttributes, | ||
| } | ||
| is, err := resourceAzureRMVirtualMachineMigrateState(tc.StateVersion, is, tc.Meta) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("bad: %s, err: %#v", tn, err) | ||
| } | ||
|
|
||
| for k, v := range tc.ExpectedAttributes { | ||
| actual := is.Attributes[k] | ||
| if actual != v { | ||
| t.Fatalf("Bad Virtual Machine Migrate for %q: %q\n\n expected: %q", k, actual, v) | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also needs a state migration to update the
resourceArmVirtualMachineStorageOsProfileWindowsConfigHashfunction and a default value (UTC)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Is there any samples? I found one here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that'll do as an example, there's a few in the codebase - this value also needs to be added to the Hash function, otherwise this won't be detected as ForceNew for existing Virtual Machines (which is why the state migrations needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.