azurerm_virtual_machine_scale_set - prevent `public_ip_address_configuration` from being lost during update #3767

alexsapran · 2019-07-02T09:03:58Z

Why

This commit is fixing #3763 when we perform a simple update on an existing scale-set the definition of the scale-set was changed on the publicIPAddressConfiguration field.

This means that any new instances that would be started from the scale-set would not have public IPs when it would be expected to have public IP.

What

Create acceptance tests to mitigate the issue

Start with creating some test coverage around the issue.

Fix the parsing for the public ip configuration

Fix the actual issue that caused the public ip configuration of a scale-set to be removed.

Fixes: #3763

Create the required acceptance tests that would show the issue as described in the ticket hashicorp#3763 Related: hashicorp#3763

The public_ip_configuration is configured as a `schema.TypeList` but when we actually parsing the response from the API its a map so we need to do some massage of the data to match what the TF definition expects. Fixes: hashicorp#3763

kwilczynski · 2019-07-02T18:13:59Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -1331,8 +1331,9 @@ func flattenAzureRmVirtualMachineScaleSetNetworkProfile(profile *compute.Virtual
 						publicIpConfigs := make([]map[string]interface{}, 0, 1)
 						publicIpConfig := make(map[string]interface{})
 						publicIpConfig["name"] = *publicIpInfo.Name
-						publicIpConfig["domain_name_label"] = *publicIpInfo.VirtualMachineScaleSetPublicIPAddressConfigurationProperties.DNSSettings
+						publicIpConfig["domain_name_label"] = *publicIpInfo.VirtualMachineScaleSetPublicIPAddressConfigurationProperties.DNSSettings.DomainNameLabel


Nice catch!

Indeed! We are going to definitely need to add some.. many nil checks to this as the original code was lacking many. My suggestion is something like this:

if ipCfg := properties.PublicIPAddressConfiguration; ipCfg != nil { block := make(map[string]interface{}) if v := ipCfg.Name; v != nil { block["name"] = *v } if ipProperties := ipCfg.VirtualMachineScaleSetPublicIPAddressConfigurationProperties; ipProperties != nil { if dns := ipProperties.DNSSettings; dns != nil { if v := dns.DomainNameLabel; v != nil { block["domain_name_label"] = *v } } if v := ipProperties.IdleTimeoutInMinutes; v != nil { block["idle_timeout"] = *v } } config["public_ip_address_configuration"] = []map[string]interface{}{block} }

kwilczynski

Looks good!

kwilczynski · 2019-07-02T18:18:11Z

Hi @jeffreyCline, the fix is very simple and appears to solve the problem. Do you think you could have a look for us?

katbyte

Hi @alexsapran,

Thank you for spotting this and opening a PR to fix it! Aside from one comment i've left inline this LGTM 🙂 Once we add some nil checks there (and the tests pass) this will be good to merge

katbyte · 2019-07-02T19:41:12Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -1331,8 +1331,9 @@ func flattenAzureRmVirtualMachineScaleSetNetworkProfile(profile *compute.Virtual
 						publicIpConfigs := make([]map[string]interface{}, 0, 1)
 						publicIpConfig := make(map[string]interface{})
 						publicIpConfig["name"] = *publicIpInfo.Name
-						publicIpConfig["domain_name_label"] = *publicIpInfo.VirtualMachineScaleSetPublicIPAddressConfigurationProperties.DNSSettings
+						publicIpConfig["domain_name_label"] = *publicIpInfo.VirtualMachineScaleSetPublicIPAddressConfigurationProperties.DNSSettings.DomainNameLabel


Indeed! We are going to definitely need to add some.. many nil checks to this as the original code was lacking many. My suggestion is something like this:

if ipCfg := properties.PublicIPAddressConfiguration; ipCfg != nil { block := make(map[string]interface{}) if v := ipCfg.Name; v != nil { block["name"] = *v } if ipProperties := ipCfg.VirtualMachineScaleSetPublicIPAddressConfigurationProperties; ipProperties != nil { if dns := ipProperties.DNSSettings; dns != nil { if v := dns.DomainNameLabel; v != nil { block["domain_name_label"] = *v } } if v := ipProperties.IdleTimeoutInMinutes; v != nil { block["idle_timeout"] = *v } } config["public_ip_address_configuration"] = []map[string]interface{}{block} }

WodansSon

@alexsapran @kwilczynski Thanks for the PR! This is great and mostly LGTM, just fix up the nil checks and I think we are ready to merge this.

kwilczynski · 2019-07-02T23:33:32Z

Hi @katbyte and @jeffreyCline, thank you so much for the review!

I believe @alexsapran will be able to include the nil checks in the morning. Which brings me to a question: do you have more release-worthy features lined up to warrant a new version release, so that we could upgrade to an official upstream release ourselves? If no, then perhaps enough bug fixes lined up to make a point-release a reality?

katbyte · 2019-07-03T03:13:39Z

@kwilczynski,

given that we just released last friday and there are only a handful unreleased so most likely release date will be next week.

In order to follow the way we do nil checks in other places.

This commit adds some nil checks to the initial implementation.

alexsapran · 2019-07-03T06:40:46Z

Hi @jeffreyCline and @katbyte and thank you for taking the time to review my PR.

I have pushed 2 new commits ea555d0 and 1498394. The first is just to follow the pattern of the nil checks and the second is to follow your recommendation around the additional Nil checks that you requested.

Please let me know what do you think and if you are still +1

katbyte

Thanks for the revisions @alexsapran, LGTM now 🙂

ghost · 2019-07-30T03:47:05Z

This has been released in version 1.32.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.32.0"
}
# ... other configuration ...

kwilczynski · 2019-07-30T14:10:01Z

Hi @katbyte, thank you so much for your help and for cutting the release. Much appreciated!

ghost · 2019-08-03T14:50:30Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

alexsapran added 2 commits July 2, 2019 11:53

Create acceptance tests to mitigate the issue

84808e7

Create the required acceptance tests that would show the issue as described in the ticket hashicorp#3763 Related: hashicorp#3763

Fix the parsing for the public ip configuration

6d188de

The public_ip_configuration is configured as a `schema.TypeList` but when we actually parsing the response from the API its a map so we need to do some massage of the data to match what the TF definition expects. Fixes: hashicorp#3763

ghost added the size/L label Jul 2, 2019

Fix linting error

9680109

kwilczynski reviewed Jul 2, 2019

View reviewed changes

kwilczynski approved these changes Jul 2, 2019

View reviewed changes

WodansSon self-assigned this Jul 2, 2019

WodansSon added this to the v1.32.0 milestone Jul 2, 2019

WodansSon added bug service/vmss Virtual Machine Scale Sets labels Jul 2, 2019

katbyte requested changes Jul 2, 2019

View reviewed changes

WodansSon approved these changes Jul 2, 2019

View reviewed changes

katbyte added the waiting-response label Jul 3, 2019

alexsapran added 2 commits July 3, 2019 09:36

Refactor the publicIpInfo if block

ea555d0

In order to follow the way we do nil checks in other places.

Add Nil checks for the public ip configuration properties

1498394

This commit adds some nil checks to the initial implementation.

ghost removed the waiting-response label Jul 3, 2019

Lint linit lint

dcfca15

katbyte approved these changes Jul 3, 2019

View reviewed changes

katbyte changed the title ~~Address the case when updating the Virtual Machine Scale-Set it would loose the public ip configuration~~ azurerm_virtual_machine_scale_set - prevent public_ip_address_configuration from being lost during update Jul 3, 2019

katbyte merged commit e8087db into hashicorp:master Jul 3, 2019

katbyte added a commit that referenced this pull request Jul 3, 2019

Update CHANGELOG.md to include #3767

e4990b8

ghost locked and limited conversation to collaborators Aug 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_virtual_machine_scale_set - prevent `public_ip_address_configuration` from being lost during update #3767

azurerm_virtual_machine_scale_set - prevent `public_ip_address_configuration` from being lost during update #3767

alexsapran commented Jul 2, 2019 •

edited

Loading

kwilczynski Jul 2, 2019

katbyte Jul 2, 2019

kwilczynski left a comment

kwilczynski commented Jul 2, 2019

katbyte left a comment

katbyte Jul 2, 2019

WodansSon left a comment

kwilczynski commented Jul 2, 2019

katbyte commented Jul 3, 2019

alexsapran commented Jul 3, 2019

katbyte left a comment

ghost commented Jul 30, 2019

kwilczynski commented Jul 30, 2019

ghost commented Aug 3, 2019

azurerm_virtual_machine_scale_set - prevent public_ip_address_configuration from being lost during update #3767

azurerm_virtual_machine_scale_set - prevent public_ip_address_configuration from being lost during update #3767

Conversation

alexsapran commented Jul 2, 2019 • edited Loading

Why

What

kwilczynski Jul 2, 2019

Choose a reason for hiding this comment

katbyte Jul 2, 2019

Choose a reason for hiding this comment

kwilczynski left a comment

Choose a reason for hiding this comment

kwilczynski commented Jul 2, 2019

katbyte left a comment

Choose a reason for hiding this comment

katbyte Jul 2, 2019

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

kwilczynski commented Jul 2, 2019

katbyte commented Jul 3, 2019

alexsapran commented Jul 3, 2019

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Jul 30, 2019

kwilczynski commented Jul 30, 2019

ghost commented Aug 3, 2019

azurerm_virtual_machine_scale_set - prevent `public_ip_address_configuration` from being lost during update #3767

azurerm_virtual_machine_scale_set - prevent `public_ip_address_configuration` from being lost during update #3767

alexsapran commented Jul 2, 2019 •

edited

Loading