`azurerm_lb` - fix zone behaviour bug introduced in recent API upgrade #12208

ms-henglu · 2021-06-15T07:15:07Z

This PR fixed 2 issues.

For users who use azurerm 2.62.x, there'll be a breaking change when they update azurerm to latest version. This is fix is similar with the fix for public ip resource.
For users who updates to azurerm 2.63.0, resources will be deployed as no-zone by default and there's no way to set it as zone-redundant.

After this fix, 2.62.x users can update azurerm smoothly. But users who ever used 2.63.0, if they want to use a zone-redundant resource, they have modify config and add the following line.

availability_zone = "Zone-Redundant"

The tests are listed as the following.


=== RUN   TestAccAzureRMLoadBalancer_basic
=== PAUSE TestAccAzureRMLoadBalancer_basic
=== CONT  TestAccAzureRMLoadBalancer_basic
--- PASS: TestAccAzureRMLoadBalancer_basic (245.14s)
=== RUN   TestAccAzureRMLoadBalancer_requiresImport
=== PAUSE TestAccAzureRMLoadBalancer_requiresImport
=== CONT  TestAccAzureRMLoadBalancer_requiresImport
--- PASS: TestAccAzureRMLoadBalancer_requiresImport (239.84s)
=== RUN   TestAccAzureRMLoadBalancer_standard
=== PAUSE TestAccAzureRMLoadBalancer_standard
=== CONT  TestAccAzureRMLoadBalancer_standard
--- PASS: TestAccAzureRMLoadBalancer_standard (178.31s)
=== RUN   TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix
=== PAUSE TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix
=== CONT  TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix
--- PASS: TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix (223.97s)
=== RUN   TestAccAzureRMLoadBalancer_frontEndConfig
=== PAUSE TestAccAzureRMLoadBalancer_frontEndConfig
=== CONT  TestAccAzureRMLoadBalancer_frontEndConfig
--- PASS: TestAccAzureRMLoadBalancer_frontEndConfig (488.93s)
=== RUN   TestAccAzureRMLoadBalancer_tags
=== PAUSE TestAccAzureRMLoadBalancer_tags
=== CONT  TestAccAzureRMLoadBalancer_tags
--- PASS: TestAccAzureRMLoadBalancer_tags (312.32s)
=== RUN   TestAccAzureRMLoadBalancer_emptyPrivateIP
=== PAUSE TestAccAzureRMLoadBalancer_emptyPrivateIP
=== CONT  TestAccAzureRMLoadBalancer_emptyPrivateIP
--- PASS: TestAccAzureRMLoadBalancer_emptyPrivateIP (311.15s)
=== RUN   TestAccAzureRMLoadBalancer_privateIP
=== PAUSE TestAccAzureRMLoadBalancer_privateIP
=== CONT  TestAccAzureRMLoadBalancer_privateIP
--- PASS: TestAccAzureRMLoadBalancer_privateIP (259.72s)
=== RUN   TestAccAzureRMLoadBalancer_ZoneRedundant
=== PAUSE TestAccAzureRMLoadBalancer_ZoneRedundant
=== CONT  TestAccAzureRMLoadBalancer_ZoneRedundant
--- PASS: TestAccAzureRMLoadBalancer_ZoneRedundant (242.98s)
=== RUN   TestAccAzureRMLoadBalancer_NoZone
=== PAUSE TestAccAzureRMLoadBalancer_NoZone
=== CONT  TestAccAzureRMLoadBalancer_NoZone
--- PASS: TestAccAzureRMLoadBalancer_NoZone (245.47s)
=== RUN   TestAccAzureRMLoadBalancer_SingleZone
=== PAUSE TestAccAzureRMLoadBalancer_SingleZone
=== CONT  TestAccAzureRMLoadBalancer_SingleZone
--- PASS: TestAccAzureRMLoadBalancer_SingleZone (251.52s)
PASS

(fixes #12215)

ms-henglu · 2021-06-15T08:15:53Z

Hi @jackofallops , could you please help review this PR? Thanks!

brendandburns · 2021-06-15T17:26:33Z

Do we need to introduce some unit testing to prevent this (or similar errors) from happening in the future?

Thanks!

katbyte

Thanks @ms-henglu - could we add a couple tests here using the availability_zone property? this will make sure that going forward we will catch any changes that break this property going forward - thanks

ms-henglu · 2021-06-16T03:08:20Z

Hi @katbyte , I added acctest for availability_zone, please review again, thanks!

WodansSon

Thanks for the changes, LGTM. Can we add one more test case maybe?

WodansSon · 2021-06-16T03:42:01Z

azurerm/internal/services/loadbalancer/loadbalancer_resource_test.go

@@ -165,6 +165,36 @@ func TestAccAzureRMLoadBalancer_privateIP(t *testing.T) {
 	})
 }

+func TestAccAzureRMLoadBalancer_ZoneRedundant(t *testing.T) {


Can we add one more test that takes a "1", "2" or "3"?

ms-henglu · 2021-06-16T04:17:24Z

Hi @WodansSon , I added a test for single zone

WodansSon

@ms-henglu thanks for pushing those changes, this is really close. I have left a couple of comments around the documentation. Take a look and LMK when you have a chance.

website/docs/r/lb.html.markdown

Co-authored-by: WS <[email protected]>

ms-henglu · 2021-06-16T04:36:30Z

@WodansSon Thanks for the suggestions, I've applied them.

WodansSon

@ms-henglu thanks for pushing those changes so quickly. This LGTM now! 🚀 HBY @jackofallops and @katbyte

jackofallops · 2021-06-16T13:32:32Z

Hi @ms-henglu
I've run the acceptance tests on this and we're getting a bunch of failures around the Zone settings / behaviour, for example:

Error: creating/updating Load Balancer "acctestlb-210616060652758222" (Resource Group "acctestRG-210616060652758222"): network.LoadBalancersClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="ResourceCannotHaveMultipleZonesSpecified" Message="Resource /subscriptions/*******/resourceGroups/acctestRG-210616060652758222/providers/Microsoft.Network/loadBalancers/acctestlb-210616060652758222/frontendIPConfigurations/feip has 2 zones specified. Only one zone can be specified for this resource." Details=[]

and

Error: creating/updating Load Balancer "arm-test-loadbalancer-210616060912483394" (Resource Group "acctestRG-210616060912483394"): network.LoadBalancersClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="LoadBalancerFrontendIPConfigCannotHaveZoneWhenReferencingPublicIPAddress" Message="Load balancer frontendIPConfiguration /subscriptions/*******/resourceGroups/acctestRG-210616060912483394/providers/Microsoft.Network/loadBalancers/arm-test-loadbalancer-210616060912483394/frontendIPConfigurations/one-210616060912483394 has 1 zone specified and is referencing a publicIPAddress /subscriptions/*******/resourceGroups/acctestRG-210616060912483394/providers/Microsoft.Network/publicIPAddresses/test-ip-210616060912483394. Networking supports zones only for frontendIpconfigurations which reference a subnet." Details=[]

Could you take a look?

ms-henglu · 2021-06-16T16:26:36Z

@jackofallops Thanks!!

I have fixed them all, please check.

WodansSon

@ms-henglu, it looks like loadbalancers only support a single zone due to the error msg that is returned via the acceptance tests. Should we change the default behavior?

azurerm/internal/services/loadbalancer/loadbalancer_resource.go

WodansSon

@ms-henglu, looks like we finally got this all straightened out. Thanks for pushing the comment. LGTM! 🚀

WodansSon · 2021-06-17T03:36:53Z

github-actions · 2021-06-18T06:42:11Z

This functionality has been released in v2.64.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 · 2021-07-19T02:08:10Z

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 bot added documentation size/M labels Jun 15, 2021

fix zone behavior change for loadbalancer

2e31246

ms-henglu force-pushed the branch-210614-fix-zone-for-lb-ip branch from 152e1b3 to 2e31246 Compare June 15, 2021 07:22

katbyte requested changes Jun 15, 2021

View reviewed changes

katbyte added this to the v2.64.0 milestone Jun 15, 2021

WodansSon added the service/load-balancers label Jun 15, 2021

katbyte added the waiting-response label Jun 15, 2021

add test for availability_zone

2223346

WodansSon requested changes Jun 16, 2021

View reviewed changes

add test for single zone

64102da

github-actions bot added size/L and removed size/M labels Jun 16, 2021

WodansSon requested changes Jun 16, 2021

View reviewed changes

website/docs/r/lb.html.markdown Outdated Show resolved Hide resolved

website/docs/r/lb.html.markdown Outdated Show resolved Hide resolved

Apply document suggestions from code review

8488584

Co-authored-by: WS <[email protected]>

WodansSon approved these changes Jun 16, 2021

View reviewed changes

ms-henglu requested a review from katbyte June 16, 2021 07:50

fix acctest test

d3b9599

WodansSon requested changes Jun 16, 2021

View reviewed changes

azurerm/internal/services/loadbalancer/loadbalancer_resource.go Show resolved Hide resolved

azurerm/internal/services/loadbalancer/loadbalancer_resource.go Show resolved Hide resolved

WodansSon mentioned this pull request Jun 16, 2021

Fix: azurerm_lb zones should accept multiple #12228

Closed

WodansSon reviewed Jun 17, 2021

View reviewed changes

azurerm/internal/services/loadbalancer/loadbalancer_resource.go Show resolved Hide resolved

add todo for replacing hardcode with getting zone list by resource api

7f9c289

WodansSon approved these changes Jun 17, 2021

View reviewed changes

remove comments

071468d

jackofallops changed the title ~~fix zone behavior change for loadbalancer~~ azurerm_lb - fix zone behaviour bug introduced in recent API upgrade Jun 17, 2021

jackofallops merged commit ca5f029 into hashicorp:master Jun 17, 2021

jackofallops added a commit that referenced this pull request Jun 17, 2021

Update CHANGELOG.md for #12208

d04d290

This was referenced Jun 22, 2021

azurerm_lb always shows changed on resource with Public IP frontend. #12166

Closed

Issue with resource azurerm_lb using azurerm provider 2.1.0 and above #12211

Closed

WodansSon mentioned this pull request Jun 22, 2021

Optional Variable called "availability_zone" under "frontend_ip_configuration" for the resource "azurerm_lb" is actually a Required variable now #12286

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_lb` - fix zone behaviour bug introduced in recent API upgrade #12208

`azurerm_lb` - fix zone behaviour bug introduced in recent API upgrade #12208

ms-henglu commented Jun 15, 2021 •

edited by katbyte

Loading

ms-henglu commented Jun 15, 2021

brendandburns commented Jun 15, 2021

katbyte left a comment

ms-henglu commented Jun 16, 2021

WodansSon left a comment

WodansSon Jun 16, 2021

ms-henglu commented Jun 16, 2021

WodansSon left a comment

ms-henglu commented Jun 16, 2021

WodansSon left a comment

jackofallops commented Jun 16, 2021

ms-henglu commented Jun 16, 2021

WodansSon left a comment

WodansSon left a comment

WodansSon commented Jun 17, 2021

github-actions bot commented Jun 18, 2021

github-actions bot commented Jul 19, 2021

azurerm_lb - fix zone behaviour bug introduced in recent API upgrade #12208

azurerm_lb - fix zone behaviour bug introduced in recent API upgrade #12208

Conversation

ms-henglu commented Jun 15, 2021 • edited by katbyte Loading

ms-henglu commented Jun 15, 2021

brendandburns commented Jun 15, 2021

katbyte left a comment

Choose a reason for hiding this comment

ms-henglu commented Jun 16, 2021

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon Jun 16, 2021

Choose a reason for hiding this comment

ms-henglu commented Jun 16, 2021

WodansSon left a comment

Choose a reason for hiding this comment

ms-henglu commented Jun 16, 2021

WodansSon left a comment

Choose a reason for hiding this comment

jackofallops commented Jun 16, 2021

ms-henglu commented Jun 16, 2021

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon commented Jun 17, 2021

github-actions bot commented Jun 18, 2021

github-actions bot commented Jul 19, 2021

`azurerm_lb` - fix zone behaviour bug introduced in recent API upgrade #12208

`azurerm_lb` - fix zone behaviour bug introduced in recent API upgrade #12208

ms-henglu commented Jun 15, 2021 •

edited by katbyte

Loading