-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_rule: strip read-only properties before sending request #7741
Conversation
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.
hey @magodo
Thanks for this PR :)
I've taken a look through and left some comments inline - but I think ultimately it might be better to downgrade the version of the Networking API being used until the API's fixed - what do you think?
Thanks!
azurerm/internal/services/network/tests/loadbalancer_rule_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/network/tests/loadbalancer_rule_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/network/tests/loadbalancer_rule_resource_test.go
Outdated
Show resolved
Hide resolved
for _, pool := range *loadBalancer.LoadBalancerPropertiesFormat.BackendAddressPools { | ||
pool.BackendIPConfigurations = nil | ||
} | ||
} |
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.
ultimately this is a hack to workaround version 2020-05-01 of the Networking API being broken - since we're not using that yet, it's probably best we downgrade until that API version is fixed so that these properties are ignored?
@tombuildsstuff I have updated the code accordingly. |
Is someone going to make a call on downgrading the API version? |
I've noticed this issue on LB creation as well, based on @tombuildsstuff's comment I've opened #8006 |
As discussed offline, closing this in favour of #8006 - since this ultimately needs to be fixed in the API (waiting for the Track2 SDK is one long-term fix - but that's not one we should rely on here) |
@tombuildsstuff Just FYI, the fix in Go SDK is implemented in Track1 (since v45.0). |
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 👉 [email protected]. Thanks! |
This is a workaround for #7691
The reason for that issue is that service can't tolerate the client sending
properties.backendAddressPools.n.properties.backendIPConfigurations
in request body for load balancer resource since 2020-05-01, as it is a READ-ONLY attribute (this only occurs whenazurerm_lb
's sku isStandard
).Currently, Azure Go SDK can trim the READ-ONLY attributes during marshaling, but only for the root level attributes, so, in this case, Go SDK will still marshal the attribute into the request body. This is tracked in issue: Azure/autorest.go#438.
Test Result
Before this change
After this change
(fix #7691)