-
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
Add support for vmss application gateway backend pools #884
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 @agolomoodysaada
Thanks for this PR - I've taken a look through and left a couple of comments inline but this mostly LGTM. If we can fix those up - this should be good to merge :)
Thanks!
for _, pool := range *properties.ApplicationGatewayBackendAddressPools { | ||
addressPools = append(addressPools, *pool.ID) | ||
} | ||
config["application_gateway_backend_address_pool_ids"] = schema.NewSet(schema.HashString, addressPools) |
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.
so we probably want to set a Set regardless here (e.g. to an empty list if it's not specified) - as such could we make this:
addressPools := make([]interface{}, 0)
if properties.ApplicationGatewayBackendAddressPools != nil {
for _, pool := range *properties.ApplicationGatewayBackendAddressPools {
addressPools = append(addressPools, *pool.ID)
}
}
config["application_gateway_backend_address_pool_ids"] = schema.NewSet(schema.HashString, addressPools)
load_balancer_inbound_nat_rules_ids = ["${element(azurerm_lb_nat_pool.lbnatpool.*.id, count.index)}"] | ||
name = "TestIPConfiguration" | ||
subnet_id = "${azurerm_subnet.test.id}" | ||
application_gateway_backend_address_pool_ids = ["${azurerm_application_gateway.mygateway.backend_address_pool.id}"] |
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.
given this isn't present in the example (and I believe it conflicts with the LB fields?), it might be prudent to comment this out as an example?
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.
I see your point. While it's not common, they're actually not mutually exclusive. You could techinically wire your VMSS with both a load balancer (for NAT rules or internal LBs) and an application gateway (for public HTTP traffic). Should I still comment this out?
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.
I commented it out anyway.
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 confirming that - I guess we can leave it commented out for the moment either way :)
4627c62
to
af5f85d
Compare
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 @agolomoodysaada
Thanks for pushing those updates - I've taken another look through and this just needs the acceptance test updating and this should be good to merge :)
Thanks!
@@ -374,6 +374,7 @@ func TestAccAzureRMVirtualMachineScaleSet_loadBalancer(t *testing.T) { | |||
Check: resource.ComposeTestCheckFunc( | |||
testCheckAzureRMVirtualMachineScaleSetExists("azurerm_virtual_machine_scale_set.test"), | |||
testCheckAzureRMVirtualMachineScaleSetHasLoadbalancer("azurerm_virtual_machine_scale_set.test"), | |||
testCheckAzureRMVirtualMachineScaleSetHasApplicationGateway("azurerm_virtual_machine_scale_set.test"), |
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.
So at the moment this test will fail since there's no Application Gateway's defined in the config used in this test. Could we create a separate config (as linked above) + test for creating a VMSS with an Application Gateway configured, and revert this test as-is? This'll mean we have test cases for both VMSS with an LB and an App Gateway - both of which are valid
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.
Makes sense. Thank you for the feedback.
For some reason the tests were passing locally for me. Do I have to configure something locally to make the test fail?
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.
Oh I see. I have to run make testacc
. Is there a way to specify an individual acceptance test to run?
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.
there is:
TF_ACC=1 go test ./azurerm -v -timeout 120m -run=TestPrefixYouWantToRun
where TestPrefixYouWantToRun
is the prefix of the test you want to run (which can be a full test name) - hope that helps :)
2c19717
to
63fb584
Compare
63fb584
to
67e0190
Compare
@tombuildsstuff I made the changes you requested and tried running the acceptance test. Unfortunately, after +20mins (it takes a long time for appgateway to provision), I get the following error
What am I doing wrong? |
@agolomoodysaada I have opened #1027 to replace this one and re-used your commit (most of it). There were couple of test failures, one was due to the issue you have mentioned above; I have worked around it (which seems to be the only way) by referencing the element directly. The second failure was because the backend pools were not loaded correctly into state and hence test check was failing. Both issues were resolved and included in #1027 |
hey @agolomoodysaada Apologies for the delayed response here - I'd missed that I'd not replied to your comment. I hope you don't mind, but I'm going to close this PR in favour of #1027 which includes your original commits/attributions that's been recently opened by @abn. Thanks! |
Thank you @abn and @tombuildsstuff ... Glad to see this merged |
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! |
Closes #857