provider/azurerm: Load Balancer resources by stack72 · Pull Request #9199 · hashicorp/terraform

stack72 · 2016-10-03T21:36:32Z

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

carinadigital · 2016-10-04T18:02:46Z

+			return fmt.Errorf("Bad: Get on loadBalancerClient: %s", err)
+		}
+
+		if resp.StatusCode == http.StatusNotFound {


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.

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.

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

carinadigital · 2016-10-04T18:06:38Z

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


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.

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

jen20

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.

jen20 · 2016-10-05T17:55:35Z

+package azurerm
+
+import (
+	"fmt"


This needs goimports running on it.

jen20 · 2016-10-05T17:55:59Z

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


This needs it's error message finishing :-)

jen20 · 2016-10-05T17:58:54Z

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


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.

Shameless plug #8103 :)

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 · 2016-10-05T19:19:39Z

+			},
+
+			"frontend_ip_configuration": {
+				Type:     schema.TypeSet,


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

jen20 · 2016-10-05T19:21:31Z

+	resGroup := id.ResourceGroup
+	name := id.Path["loadBalancers"]
+
+	_, err = loadBalancerClient.Delete(resGroup, name, make(chan struct{}))


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

jen20 · 2016-10-05T19:28:54Z

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


Refreshing => Removing

jen20 · 2016-10-05T19:29:12Z

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


Needs an error message

jen20 · 2016-10-05T19:29:35Z

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


Needs an error message

jen20 · 2016-10-05T19:29:49Z

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


Needs an error message

jen20 · 2016-10-05T19:30:02Z

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


Needs an error message

stack72 · 2016-10-05T21:29:08Z

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

stack72 · 2016-10-05T21:48:47Z

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

carinadigital · 2016-10-06T10:03:54Z

+		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)


Error string capitalisation inconsistent. Loadbalancer->LoadBalancer

carinadigital · 2016-10-06T10:32:59Z

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


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

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

carinadigital · 2016-10-06T11:00:23Z

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


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.

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

carinadigital · 2016-10-06T12:57:52Z

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


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=[]

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 · 2016-10-06T13:28:46Z

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

Apply a plan of the example azurerm_lb_rule at https://github.com/hashicorp/terraform/pull/9199/files#diff-c9de7ad93d35494c7721357083bae149R15
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 · 2016-10-06T13:31:48Z

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.

carinadigital · 2016-10-06T14:55:45Z

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


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 · 2016-10-06T15:56:44Z

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 · 2016-10-06T16:17:39Z

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

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

jen20 · 2016-10-07T18:00:59Z

+package azurerm
+
+import (
+	"fmt"


jen20 · 2016-10-07T18:01:59Z

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


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.

ryanmoran · 2016-10-08T18:20:10Z

Fantastic work on this! This is so appreciated.

ghost · 2020-04-21T02:26:42Z

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.

jen20 added enhancement provider/azurerm new-resource labels Oct 3, 2016

This was referenced Oct 3, 2016

A simplifed load balancer resource for AzureRM #6429

Closed

provider/azure: added load balancer support. #4343

Closed

[WIP] : Azure Load Balancer with separated resource #6467

Closed

This was referenced Oct 3, 2016

[WIP] Azure Resource Manager: Load Balancers #8130

Closed

provider/azurerm: currently lacking load balancer resource #8007

Closed

stack72 force-pushed the arm-loadbalancer branch 3 times, most recently from 966b7b3 to 25d9730 Compare October 4, 2016 10:49

stack72 mentioned this pull request Oct 4, 2016

provider/azurerm: write load_balanacer attributes to network_interface_card hash #9207

Merged

stack72 force-pushed the arm-loadbalancer branch 5 times, most recently from e29038d to dff6941 Compare October 4, 2016 17:40

carinadigital reviewed Oct 4, 2016

View reviewed changes

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

jen20 suggested changes Oct 5, 2016

View reviewed changes

stack72 force-pushed the arm-loadbalancer branch from eacaeba to 463bd66 Compare October 5, 2016 21:42

stack72 force-pushed the arm-loadbalancer branch from aab5a95 to 2647571 Compare October 6, 2016 09:57

stack72 changed the title ~~[WIP] provider/azurerm: Loadbalancer resource~~ provider/azurerm: Loadbalancer resource Oct 6, 2016

carinadigital reviewed Oct 6, 2016

View reviewed changes

stack72 force-pushed the arm-loadbalancer branch from 2647571 to 79d6cc8 Compare October 6, 2016 10:36

carinadigital reviewed Oct 6, 2016

View reviewed changes

stack72 added 3 commits October 6, 2016 14:18

provider/azurerm: Documentation for the ARM LB resources

fd4aa3b

provider/azurerm: Changes to Arm LB Resources post review

5444c27

stack72 force-pushed the arm-loadbalancer branch from ef82f62 to 5444c27 Compare October 6, 2016 13:21

carinadigital reviewed Oct 6, 2016

View reviewed changes

jen20 changed the title ~~provider/azurerm: Loadbalancer resource~~ provider/azurerm: Load Balancer resources Oct 7, 2016

provider/azurerm: Loadbalancer => LoadBalancer

7d3d707

jen20 force-pushed the arm-loadbalancer branch from 45e0f62 to 7d3d707 Compare October 7, 2016 18:12

jen20 approved these changes Oct 7, 2016

View reviewed changes

jen20 merged commit a085c8d into master Oct 7, 2016

jen20 deleted the arm-loadbalancer branch October 7, 2016 18:14

pmcatominey mentioned this pull request Oct 10, 2016

provider/azurerm: fix computed tags causing non-empty plan #9307

Merged

pmcq mentioned this pull request Oct 10, 2016

azurerm can't create LoadBalancer rule with probe #9311

Closed

tombuildsstuff mentioned this pull request Mar 31, 2017

Azure provider should support load balancers #3055

Closed

ghost locked and limited conversation to collaborators Apr 21, 2020

Conversation

stack72 commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jen20 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stack72 commented Oct 5, 2016

Uh oh!

stack72 commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carinadigital commented Oct 6, 2016

Uh oh!

stack72 commented Oct 6, 2016

Uh oh!

carinadigital Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carinadigital commented Oct 6, 2016

Uh oh!

carinadigital commented Oct 6, 2016

Uh oh!

jen20 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stack72 commented Oct 3, 2016 •

edited

Loading

carinadigital Oct 6, 2016 •

edited

Loading