-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
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
152e1b3
to
2e31246
Compare
Hi @jackofallops , could you please help review this PR? Thanks! |
Do we need to introduce some unit testing to prevent this (or similar errors) from happening in the future? Thanks! |
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.
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
Hi @katbyte , I added acctest for |
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.
Thanks for the changes, LGTM. Can we add one more test case maybe?
@@ -165,6 +165,36 @@ func TestAccAzureRMLoadBalancer_privateIP(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestAccAzureRMLoadBalancer_ZoneRedundant(t *testing.T) { |
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.
Can we add one more test that takes a "1", "2" or "3"?
Hi @WodansSon , I added a test for single zone |
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.
@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.
Co-authored-by: WS <[email protected]>
@WodansSon Thanks for the suggestions, I've applied them. |
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.
@ms-henglu thanks for pushing those changes so quickly. This LGTM now! 🚀 HBY @jackofallops and @katbyte
Hi @ms-henglu 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? |
@jackofallops Thanks!! I have fixed them all, please check. |
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.
@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?
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.
@ms-henglu, looks like we finally got this all straightened out. Thanks for pushing the comment. LGTM! 🚀
azurerm_lb
- fix zone behaviour bug introduced in recent API upgrade
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! |
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. |
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 aszone-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.The tests are listed as the following.
(fixes #12215)