Skip to content
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

provider/azurerm: Load Balancer resources #9199

Merged
merged 4 commits into from
Oct 7, 2016
Merged

provider/azurerm: Load Balancer resources #9199

merged 4 commits into from
Oct 7, 2016

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Oct 3, 2016

  • azurerm_lb
  • azurerm_lb_backend_address_pool
  • azurerm_lb_rule
  • azurerm_lb_nat_rule
  • azurerm_lb_probe
  • azurerm_lb_nat_pool

Current test status:

% make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMLoadbalancer'                           ✹
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/05 12:52:28 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMLoadbalancer -timeout 120m
=== RUN   TestAccAzureRMLoadbalancerBackEndAddressPool_basic
--- PASS: TestAccAzureRMLoadbalancerBackEndAddressPool_basic (207.26s)
=== RUN   TestAccAzureRMLoadbalancerBackEndAddressPool_removal
--- PASS: TestAccAzureRMLoadbalancerBackEndAddressPool_removal (165.89s)
=== RUN   TestAccAzureRMLoadbalancerNatRule_basic
--- PASS: TestAccAzureRMLoadbalancerNatRule_basic (179.30s)
=== RUN   TestAccAzureRMLoadbalancerNatRule_removal
--- PASS: TestAccAzureRMLoadbalancerNatRule_removal (180.73s)
=== RUN   TestAccAzureRMLoadbalancerRule_basic
--- PASS: TestAccAzureRMLoadbalancerRule_basic (170.40s)
=== RUN   TestAccAzureRMLoadbalancerRule_removal
--- PASS: TestAccAzureRMLoadbalancerRule_removal (204.23s)
=== RUN   TestAccAzureRMLoadbalancer_basic
--- PASS: TestAccAzureRMLoadbalancer_basic (136.03s)
=== RUN   TestAccAzureRMLoadbalancer_frontEndConfig
--- PASS: TestAccAzureRMLoadbalancer_frontEndConfig (214.47s)
=== RUN   TestAccAzureRMLoadbalancer_tags
--- PASS: TestAccAzureRMLoadbalancer_tags (215.52s)
=== RUN   TestAccAzureRMLoadbalancerProbe_basic
--- PASS: TestAccAzureRMLoadbalancerProbe_basic (183.36s)
=== RUN   TestAccAzureRMLoadbalancerProbe_removal
--- PASS: TestAccAzureRMLoadbalancerProbe_removal (185.86s)
=== RUN   TestAccAzureRMLoadbalancerNatPool_basic
--- PASS: TestAccAzureRMLoadbalancerNatPool_basic (161.47s)
=== RUN   TestAccAzureRMLoadbalancerNatPool_removal
--- PASS: TestAccAzureRMLoadbalancerNatPool_removal (167.38s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm    1673.852s

return fmt.Errorf("Bad: Get on loadBalancerClient: %s", err)
}

if resp.StatusCode == http.StatusNotFound {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience the ArmClients raise an err for 404 responses while also filling the resp object.
A 404 response will actually trigger at line 117 and these lines will never be executed. It's a common error I've seen in other AzureRM acceptance tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @carinadigital

We are in noway near ready to start reviewing this - give us another day yet :) I definitely take your point! I Just found the same issue elsewhere haha

P.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these have been addressed now. Thanks @carinadigital!

if err != nil {
return nil, false, fmt.Errorf("Error making Read request on Azure Loadbalancer %s: %s", name, err)
}
if resp.StatusCode == http.StatusNotFound {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comment about ArmClients and 404 responses raising errors. I suspect this will fail a _disappears() acceptance test if there was one present.
Moving the conditional inside the if err != nil check should work as intended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these have been addressed now. Thanks @carinadigital!

@stack72 stack72 force-pushed the arm-loadbalancer branch 5 times, most recently from 25cc001 to f7c8bd0 Compare October 5, 2016 15:47
Copy link
Contributor

@jen20 jen20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stack72! Looks like a good start, there are some nits and some areas for improvement prior to merging. Other than that the pattern looks like it's as good as we can do for an upstream API like this.

package azurerm

import (
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs goimports running on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


resGroup, name, err := resourceGroupAndLBNameFromId(loadBalancerId)
if err != nil {
return nil, false, errwrap.Wrapf("TODO: error message {{err}}", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs it's error message finishing :-)

}

func validateLoadbalancerPrivateIpAddressAllocation(v interface{}, k string) (ws []string, errors []error) {
value := strings.ToLower(v.(string))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessary complex for something that could be:

value := strings.ToLower(v.(string))
if value == "static" || value == "dynamic" {
    // ok
} else {
    // not ok
}

Even if we keep the map lookup pattern, it should be map[string]struct{} rather than map[string]bool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shameless plug #8103 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like #8103 has been merged, we'll merge this as is (pending review) and then go back and fix it up to use the new bits.

},

"frontend_ip_configuration": {
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we strictly need this to be a Set? Is there any reason not to have it as a list?

resGroup := id.ResourceGroup
name := id.Path["loadBalancers"]

_, err = loadBalancerClient.Delete(resGroup, name, make(chan struct{}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check for a 404 here and remove from state if so?

}
if !exists {
d.SetId("")
log.Printf("[INFO] Loadbalancer %q not found. Refreshing from state", d.Get("name").(string))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refreshing => Removing


resGroup, loadBalancerName, err := resourceGroupAndLBNameFromId(d.Get("loadbalancer_id").(string))
if err != nil {
return errwrap.Wrapf("TODO: {{err}}", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs an error message

loadBalancer.Properties.InboundNatPools = &natPools
resGroup, loadBalancerName, err := resourceGroupAndLBNameFromId(d.Get("loadbalancer_id").(string))
if err != nil {
return errwrap.Wrapf("TODO: {{err}}", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs an error message


resGroup, loadBalancerName, err := resourceGroupAndLBNameFromId(d.Get("loadbalancer_id").(string))
if err != nil {
return errwrap.Wrapf("TODO: {{err}}", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs an error message


resGroup, loadBalancerName, err := resourceGroupAndLBNameFromId(d.Get("loadbalancer_id").(string))
if err != nil {
return errwrap.Wrapf("TODO: {{err}}", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs an error message

@stack72
Copy link
Contributor Author

stack72 commented Oct 5, 2016

@jen20 all changes made as requested. Also made the use of NAT and LoadBalancer consistent as well as wrapping errors from the API

@stack72
Copy link
Contributor Author

stack72 commented Oct 5, 2016

New test run post code review changes:

% make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMLoadbalancer'                         2 ↵
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/05 22:12:29 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMLoadbalancer -timeout 120m
=== RUN   TestAccAzureRMLoadbalancerBackEndAddressPool_basic
--- PASS: TestAccAzureRMLoadbalancerBackEndAddressPool_basic (161.58s)
=== RUN   TestAccAzureRMLoadbalancerBackEndAddressPool_removal
--- PASS: TestAccAzureRMLoadbalancerBackEndAddressPool_removal (149.93s)
=== RUN   TestAccAzureRMLoadbalancerNatPool_basic
--- PASS: TestAccAzureRMLoadbalancerNatPool_basic (157.05s)
=== RUN   TestAccAzureRMLoadbalancerNatPool_removal
--- PASS: TestAccAzureRMLoadbalancerNatPool_removal (173.25s)
=== RUN   TestAccAzureRMLoadbalancerNatRule_basic
--- PASS: TestAccAzureRMLoadbalancerNatRule_basic (157.80s)
=== RUN   TestAccAzureRMLoadbalancerNatRule_removal
--- PASS: TestAccAzureRMLoadbalancerNatRule_removal (173.46s)
=== RUN   TestAccAzureRMLoadbalancerProbe_basic
--- PASS: TestAccAzureRMLoadbalancerProbe_basic (157.15s)
=== RUN   TestAccAzureRMLoadbalancerProbe_removal
--- PASS: TestAccAzureRMLoadbalancerProbe_removal (176.79s)
=== RUN   TestAccAzureRMLoadbalancerRule_basic
--- PASS: TestAccAzureRMLoadbalancerRule_basic (153.18s)
=== RUN   TestAccAzureRMLoadbalancerRule_removal
--- PASS: TestAccAzureRMLoadbalancerRule_removal (176.95s)
=== RUN   TestAccAzureRMLoadbalancer_basic
--- PASS: TestAccAzureRMLoadbalancer_basic (106.80s)
=== RUN   TestAccAzureRMLoadbalancer_frontEndConfig
--- PASS: TestAccAzureRMLoadbalancer_frontEndConfig (194.34s)
=== RUN   TestAccAzureRMLoadbalancer_tags
--- PASS: TestAccAzureRMLoadbalancer_tags (127.80s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm    2066.118s

@stack72 stack72 changed the title [WIP] provider/azurerm: Loadbalancer resource provider/azurerm: Loadbalancer resource Oct 6, 2016
if resp.StatusCode == http.StatusNotFound {
return nil, false, nil
}
return nil, false, fmt.Errorf("Error making Read request on Azure Loadbalancer %s: %s", name, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error string capitalisation inconsistent. Loadbalancer->LoadBalancer

if err != nil {
return errwrap.Wrapf("Error Getting LoadBalancer {{err}}", err)
}
if read.ID == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the lbClient.Get() was successful, can read.ID ever be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a successful api call can still potentially return an empty result. This is a guard against a panic when dereferencing

func testAccAzureRMLoadbalancerBackEndAddressPool_basic(rInt int, addressPoolName string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
name = "acctestrg-%d"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some resources and regions, resource_group_name is returned from the API in mixed capitalisation. Adding capitalisation to name would extend the test to catch this error.
e.g. "acctestrg-%d" -> "acctestRG-%d"
This might be better fixed in a different PR, but I thought it's worth mentioning.
"West US" loadbalancer API does pass the capitalisation test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @carinadigital yeah this is outside of the scope of this PR - there is another issue for this AFAICR

location = "West US"
resource_group_name = "${azurerm_resource_group.test.name}"
loadbalancer_id = "${azurerm_lb.test.id}"
name = "LB Rule"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example name for azurerm_lb_rule given here is invalid (whitespace is an invalid character).
Consider a validate function for azurerm_lb_rule.name in the schema. The validation rules are in the error output below.

It would also be nice if name was the first attribute listed of the resource, rather than in the middle.

* azurerm_lb_rule.hmrc: Error Creating/Updating LoadBalancer network.LoadBalancersClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidResourceName" Message="Resource name LB Rule is invalid. The name can be up to 80 characters long. It must begin with a word character, and it must end with a word character or with '_'. The name may contain word characters or '.', '-', '_'." Details=[]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this one is resolved with a validation function.

Adds support for the elusive Azure LoadBalancer

* [x] `azurerm_lb`
* [x] `azurerm_lb_backend_address_pool`
* [x] `azurerm_lb_rule`
* [x] `azurerm_lb_nat_rule`
* [x] `azurerm_lb_probe`
* [x] `azurerm_lb_nat_pool`

Test Results:

```
make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMLoadbalancer'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
TF_ACC=1 go test ./builtin/providers/azurerm -v
-run=TestAccAzureRMLoadbalancer -timeout 120m
=== RUN   TestAccAzureRMLoadbalancerBackEndAddressPool_basic
--- PASS: TestAccAzureRMLoadbalancerBackEndAddressPool_basic (207.26s)
=== RUN   TestAccAzureRMLoadbalancerBackEndAddressPool_removal
--- PASS: TestAccAzureRMLoadbalancerBackEndAddressPool_removal (165.89s)
=== RUN   TestAccAzureRMLoadbalancerNatRule_basic
--- PASS: TestAccAzureRMLoadbalancerNatRule_basic (179.30s)
=== RUN   TestAccAzureRMLoadbalancerNatRule_removal
--- PASS: TestAccAzureRMLoadbalancerNatRule_removal (180.73s)
=== RUN   TestAccAzureRMLoadbalancerRule_basic
--- PASS: TestAccAzureRMLoadbalancerRule_basic (170.40s)
=== RUN   TestAccAzureRMLoadbalancerRule_removal
--- PASS: TestAccAzureRMLoadbalancerRule_removal (204.23s)
=== RUN   TestAccAzureRMLoadbalancer_basic
--- PASS: TestAccAzureRMLoadbalancer_basic (136.03s)
=== RUN   TestAccAzureRMLoadbalancer_frontEndConfig
--- PASS: TestAccAzureRMLoadbalancer_frontEndConfig (214.47s)
=== RUN   TestAccAzureRMLoadbalancer_tags
--- PASS: TestAccAzureRMLoadbalancer_tags (215.52s)
=== RUN   TestAccAzureRMLoadbalancerProbe_basic
--- PASS: TestAccAzureRMLoadbalancerProbe_basic (183.36s)
=== RUN   TestAccAzureRMLoadbalancerProbe_removal
--- PASS: TestAccAzureRMLoadbalancerProbe_removal (185.86s)
=== RUN   TestAccAzureRMLoadbalancerNatPool_basic
--- PASS: TestAccAzureRMLoadbalancerNatPool_basic (161.47s)
=== RUN   TestAccAzureRMLoadbalancerNatPool_removal
--- PASS: TestAccAzureRMLoadbalancerNatPool_removal (167.38s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm
1673.852s
```
@carinadigital
Copy link
Contributor

Great work on this 👍
It looks like the update of azurerm_lb_rule port attribute does not work.

  1. Apply a plan of the example azurerm_lb_rule at https://github.com/hashicorp/terraform/pull/9199/files#diff-c9de7ad93d35494c7721357083bae149R15
  2. Change ports and re-apply.
azurerm_lb_rule.test: Modifying...
  frontend_port: "3389" => "80"
Error applying plan:

1 error(s) occurred:

* azurerm_lb_rule.test: A LoadBalancer Rule with name "LBRule" already exists.

Maybe a TestAccAzureRMLoadbalancerRule_update() test.

@stack72
Copy link
Contributor Author

stack72 commented Oct 6, 2016

Thanks @carinadigital

Good point about the update now - i can't see anything in the docs that says it can't be updated. So I think adding the update test will be a good addition!

Adding this now

P.

return errwrap.Wrapf("Error Getting LoadBalancer By ID {{err}}", err)
}
if !exists {
d.SetId("")
Copy link
Contributor

@carinadigital carinadigital Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another case where the LoadBalancer exists, but the LoadBalancer_rule does not?
I can't see another d.SetId("") in the resourceArmLoadbalancerRuleRead().

@carinadigital
Copy link
Contributor

Apologies if I'm reviewing something before it's finished.
I encountered an error when adding a rule with a probe_id. The probe's stored ID in my state file is the same as the LoadBalancer's ID.
The error message below when applying suggests the this ID is wrong.

* azurerm_lb_rule.myrule: Error Creating/Updating LoadBalancer network.LoadBalancersClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidRequestFormat" Message="Cannot parse the request." Details=[{"code":"InvalidJsonReferenceWrongType","message":"Reference Id /subscriptions/[myID]/resourceGroups/dev/providers/Microsoft.Network/loadBalancers/jump is referencing resource of a wrong type. The Id is expected to reference resources of type loadBalancers/probes. Path properties.loadBalancingRules[0].properties.probe."}]

Looking up the probe object in the azure cli suggest this:
/subscriptions/[myID]/resourceGroups/dev/providers/Microsoft.Network/loadBalancers/jump
to
/subscriptions/[myID]/resourceGroups/dev/providers/Microsoft.Network/loadBalancers/jump/pro bes/jump-probe

@carinadigital
Copy link
Contributor

I think I confirmed this is the problem by suffixing the string "/probes/jump-probe" and seeing it work correctly.

resource "azurerm_lb_rule" "test" {
  location = "West US"
  resource_group_name = "${azurerm_resource_group.test.name}"
  loadbalancer_id = "${azurerm_lb.test.id}"
  name = "LBRule"
  protocol = "Tcp"
  frontend_port = 22
  backend_port = 2
  frontend_ip_configuration_name = "PublicIPAddress"
  probe_id = "${azurerm_lb_probe.test.id}/probes/jump-probe"
}

@jen20 jen20 changed the title provider/azurerm: Loadbalancer resource provider/azurerm: Load Balancer resources Oct 7, 2016
@jen20 jen20 force-pushed the arm-loadbalancer branch from 45e0f62 to 7d3d707 Compare October 7, 2016 18:12
Copy link
Contributor

@jen20 jen20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is good to merge. We should probably rebase it to squash the commits first though.

package azurerm

import (
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

func validateLoadbalancerPrivateIpAddressAllocation(v interface{}, k string) (ws []string, errors []error) {
value := strings.ToLower(v.(string))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like #8103 has been merged, we'll merge this as is (pending review) and then go back and fix it up to use the new bits.

@jen20 jen20 merged commit a085c8d into master Oct 7, 2016
@jen20 jen20 deleted the arm-loadbalancer branch October 7, 2016 18:14
@ryanmoran
Copy link
Contributor

Fantastic work on this! This is so appreciated.

pmcatominey pushed a commit to BedeGaming/terraform that referenced this pull request Oct 10, 2016
The tag schema was changed in hashicorp#9199, setting the Computed flag, this was causing
the plan to not be empty for resources which support tags but none were set, as
no value would be set by flattenAndSetTags.

Setting an empty map instead fixes the issue, ran original failing test and an
update tags test to ensure nothing else was broken.

```
TF_ACC=1 go test ./builtin/providers/azurerm -v -run TestAccAzureRMCdnProfile_basic -timeout 120m
=== RUN   TestAccAzureRMCdnProfile_basic
--- PASS: TestAccAzureRMCdnProfile_basic (208.11s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	208.188s
```
pmcatominey pushed a commit to BedeGaming/terraform that referenced this pull request Oct 10, 2016
The tag schema was changed in hashicorp#9199, setting the Computed flag, this was causing
the plan to not be empty for resources which support tags but none were set, as
no value would be set by flattenAndSetTags.

Setting an empty map instead fixes the issue, ran original failing test and an
update tags test to ensure nothing else was broken.

Depends on hashicorp#9305.

```
TF_ACC=1 go test ./builtin/providers/azurerm -v -run TestAccAzureRMCdnProfile -timeout 120m
=== RUN   TestAccAzureRMCdnProfile_importWithTags
--- PASS: TestAccAzureRMCdnProfile_importWithTags (171.64s)
=== RUN   TestAccAzureRMCdnProfile_basic
--- PASS: TestAccAzureRMCdnProfile_basic (162.70s)
=== RUN   TestAccAzureRMCdnProfile_withTags
--- PASS: TestAccAzureRMCdnProfile_withTags (203.12s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	537.538s
```
@ghost
Copy link

ghost commented Apr 21, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants