Load balancer + load balancer pool resources (updated) #40

benjvi · 2018-03-18T08:50:02Z

Continued from #31 . Reopening this so others can contribute to address issues identified there in the next few days

nickithewatt · 2018-03-18T09:57:18Z

@catsby, @benjvi answered some of your original review comments in #31, however there are still a few outstanding review items to address in this PR - we will get onto those this week.

benjvi · 2018-03-23T08:41:20Z

@catsby merge conflicts now resolved here as well. Although at least the resources registered in the resources and list of resource links are likely to conflict again with the monitor PR. Otherwise, I think all the comments should have been addressed now

prdonahue · 2018-03-27T06:24:51Z

Hi @catsby is there anything else we need to do here or can this be merged?

catsby

The code looks good, just need the Set methods removed unless they are necessary, in which case we should document why.

catsby · 2018-04-03T19:31:39Z

cloudflare/resource_cloudflare_load_balancer.go

+						},
+					},
+				},
+				Set: HashByMapKey("pop"),


Custom Set methods are discouraged, unless there is a very specific reason for it.

catsby · 2018-04-03T19:33:55Z

cloudflare/resource_cloudflare_load_balancer_pool.go

+						},
+					},
+				},
+				Set: HashByMapKey("address"),


Same custom Set comment here, remove unless there's a very good reason for this

catsby · 2018-04-03T19:35:06Z

cloudflare/utils.go

@@ -28,3 +30,10 @@ func flattenIntList(list []int) []interface{} {
 func IntIdentity(i interface{}) int {
 	return i.(int)
 }
+
+func HashByMapKey(key string) func(v interface{}) int {


Assuming the custom Set methods are being removed, this should go too

benjvi · 2018-04-04T13:29:42Z

@catsby - for the pool elements, removing the set function breaks the diff in case where duplicates are specified. However I guess its better to explicitly throw an error rather than quietly override the duplicate value. So I added a guard to throw an error when undesirable duplicates are added. I think this way is the clearest. Otherwise I removed the Set methods and cleaned up as suggested

catsby · 2018-04-04T21:09:11Z

Hey @benjvi – I'm still getting an error when trying to create a load balancer pool. I can see on the console that I have load balancers enabled:

Here's a simple configuration I'm using:

variable "zone" {
  default = "hashicorptest.com"
}

resource "cloudflare_load_balancer_pool" "foo" {
  name = "example-lb-pool"

  origins {
    name    = "example-1"
    address = "192.0.2.1"
    enabled = false
  }

  origins {
    name    = "example-2"
    address = "192.0.2.2"
  }
}

Here's the error/log I'm getting:

[opencredo-load-balancer](4)$ terraform apply 
cloudflare_load_balancer_pool.foo: Creating...
  check_regions.#:            "" => "<computed>"
  created_on:                 "" => "<computed>"
  enabled:                    "" => "true"
  minimum_origins:            "" => "1"
  modified_on:                "" => "<computed>"
  name:                       "" => "example-lb-pool"
  origins.#:                  "" => "2"
  origins.2744668579.address: "" => "192.0.2.2"
  origins.2744668579.enabled: "" => "true"
  origins.2744668579.name:    "" => "example-2"
  origins.3163022560.address: "" => "192.0.2.1"
  origins.3163022560.enabled: "" => "false"
  origins.3163022560.name:    "" => "example-1"

Error: Error applying plan:

1 error(s) occurred:

* cloudflare_load_balancer_pool.foo: 1 error(s) occurred:

* cloudflare_load_balancer_pool.foo: error creating load balancer pool: error from makeRequest: HTTP status 400: content "{\n  \"result\": null,\n  \"success\": false,\n  \"errors\": [\n    {\n      \"code\": 1002,\n      \"message\": \"the origin list length must be in range [1, 0]: validation failed\"\n    }\n  ],\n  \"messages\": []\n}\n"

Any ideas what I'm doing wrong or missing?

catsby · 2018-04-04T21:10:36Z

Here's debug log info from the log:

2018-04-04T16:01:02.285-0500 [DEBUG] plugin.terraform-provider-cloudflare: 2018/04/04 16:01:02 [DEBUG] Creating CloudFlare Load Balancer Pool from struct: {ID: CreatedOn:<nil> ModifiedOn:<nil> Description: Name:example-lb-pool Enabled:true MinimumOrigins:1 Monitor: Origins:[{Name:example-1 Address:192.0.2.1 Enabled:false}] NotificationEmail: CheckRegions:[]}

benjvi · 2018-04-04T21:32:14Z

@catsby That is a strange one. Is that log line with the same config?? I just ran with the same config you posted and I get this log line instead:
2018-04-04T22:20:10.254+0100 [DEBUG] plugin.terraform-provider-cloudflare: 2018/04/04 22:20:10 [DEBUG] Creating CloudFlare Load Balancer Pool from struct: {ID: CreatedOn:<nil> ModifiedOn:<nil> Description: Name:example-lb-pool Enabled:true MinimumOrigins:1 Monitor: Origins:[{Name:example-2 Address:192.0.2.2 Enabled:true} {Name:example-1 Address:192.0.2.1 Enabled:false}] NotificationEmail: CheckRegions:[]}

I.e. both origin elements present instead of just one, and with a successful result. Looking at the code I can't even see any place where it might drop elements. Very confusing.

Regardless, given the opaque/misleading error message I would still suspect something is different on the account. Although I don't know what it would be - can you create a pool manually in the GUI?

catsby · 2018-04-05T15:38:20Z

Hrmm I'm not sure how I messed up the debug log, I didn't notice there was only one origin listed. Maybe I copied an older one, or one right before I added 2? Dunno ¯_(ツ)_/¯

I just tried again and get the same error, but with 2 origins included:

2018/04/05 10:24:20 [DEBUG] apply: cloudflare_load_balancer_pool.foo: executing Apply
cloudflare_load_balancer_pool.foo: Creating...
  check_regions.#:            "" => "<computed>"
2018-04-05T10:24:20.395-0500 [DEBUG] plugin.terraform-provider-cloudflare: 2018/04/05 10:24:20 [DEBUG] Creating CloudFlare Load Balancer Pool from struct: {ID: CreatedOn:<nil> ModifiedOn:<nil> Description: Name:example-lb-pool Enabled:true MinimumOrigins:1 Monitor: Origins:[{Name:example-2 Address:192.0.2.2 Enabled:true} {Name:example-1 Address:192.0.2.1 Enabled:false}] NotificationEmail: CheckRegions:[]}
  created_on:                 "" => "<computed>"
  enabled:                    "" => "true"
  minimum_origins:            "" => "1"
  modified_on:                "" => "<computed>"
  name:                       "" => "example-lb-pool"
  origins.#:                  "" => "2"
  origins.2744668579.address: "" => "192.0.2.2"
  origins.2744668579.enabled: "" => "true"
  origins.2744668579.name:    "" => "example-2"
  origins.3163022560.address: "" => "192.0.2.1"
  origins.3163022560.enabled: "" => "false"
  origins.3163022560.name:    "" => "example-1"
[...]

* cloudflare_load_balancer_pool.foo: error creating load balancer pool: error from makeRequest: HTTP status 400: content "{\n  \"result\": null,\n  \"success\": false,\n  \"errors\": [\n    {\n      \"code\": 1002,\n      \"message\": \"the origin list length must be in range [1, 0]: validation failed\"\n    }\n  ],\n  \"messages\": []\n}\n"

I have created a pool in the console, however, it doesn't show in the API 🤔

[opencredo-load-balancer](4)$ curl -X GET "https://api.cloudflare.com/client/v4/user/load_balancers/pools" -H "X-Auth-Email: $CLOUDFLARE_EMAIL" -H "X-Auth-Key: $CLOUDFLARE_TOKEN" -H "Content-Type: application/json"
{
  "result": [],
  "success": true,
  "errors": [],
  "messages": []
}

[opencredo-load-balancer](4)$ curl -s -X GET "https://api.cloudflare.com/client/v4/user" -H "X-Auth-Email: $CLOUDFLARE_EMAIL" -H "X-Auth-Key: $CLOUDFLARE_TOKEN" -H "Content-Type: application/json" | jq .
{
  "result": {
    "id": "<redacted>",
    "email": "terraform-acctest@hashicorp.com",
[.......]
}

I'll have to ask my contacts at Cloudflare ...

elithrar · 2018-04-05T15:54:35Z

The reason it "doesn't show in the API" is that the provider is using the /user endpoint, but the pools were (correctly) created under the /organization (our term for a "team"). Pools created under a user are only visible by you, not the rest of your team, which is never what you want.

The Provider should use the organization endpoints when the zone is owned by an organization, else the pools can never be attached to a Load Balancer.

catsby · 2018-04-05T16:02:43Z

Thanks @elithrar ! You're this mornings hero 😄 I've verified the test pool I created is there with the orgs endpoint

benjvi · 2018-04-06T15:05:42Z

@catsby All is working for me now in my newly organizations-enabled account. Can you verify? You will need to export CLOUDFLARE_ORG_ID to get the tests working (or CLOUDFLARE_ORG_ZONE which does a -somewhat convoluted- lookup using the Zone Name). This seems like the only way to get consistent behaviour when creating resources like load balancer pools which aren't in themselves associated with a zone

benjvi · 2018-04-06T15:07:01Z

Unexpectedly, I also found the performance of API calls looked very different when using the organizations API and rate limiting started causing some problems due to the tests being in parallel. I also made the other provider config fields settable through environment variables to avoid such issues

elithrar · 2018-04-06T15:32:29Z

Thanks @benjvi!

catsby · 2018-04-06T19:14:19Z

Looks good now @benjvi , thanks!

TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccCloudFlareLoadBalancer -timeout 120m
?       github.com/terraform-providers/terraform-provider-cloudflare    [no test files]
=== RUN   TestAccCloudFlareLoadBalancerMonitor_Import
=== PAUSE TestAccCloudFlareLoadBalancerMonitor_Import
=== RUN   TestAccCloudFlareLoadBalancerPool_Import
=== PAUSE TestAccCloudFlareLoadBalancerPool_Import
=== RUN   TestAccCloudFlareLoadBalancer_Import
=== PAUSE TestAccCloudFlareLoadBalancer_Import
=== RUN   TestAccCloudFlareLoadBalancerMonitor_Basic
=== PAUSE TestAccCloudFlareLoadBalancerMonitor_Basic
=== RUN   TestAccCloudFlareLoadBalancerMonitor_FullySpecified
=== PAUSE TestAccCloudFlareLoadBalancerMonitor_FullySpecified
=== RUN   TestAccCloudFlareLoadBalancerMonitor_Update
=== PAUSE TestAccCloudFlareLoadBalancerMonitor_Update
=== RUN   TestAccCloudFlareLoadBalancerMonitor_CreateAfterManualDestroy
=== PAUSE TestAccCloudFlareLoadBalancerMonitor_CreateAfterManualDestroy
=== RUN   TestAccCloudFlareLoadBalancerPool_Basic
=== PAUSE TestAccCloudFlareLoadBalancerPool_Basic
=== RUN   TestAccCloudFlareLoadBalancerPool_FullySpecified
=== PAUSE TestAccCloudFlareLoadBalancerPool_FullySpecified
=== RUN   TestAccCloudFlareLoadBalancerPool_ForceNew
=== PAUSE TestAccCloudFlareLoadBalancerPool_ForceNew
=== RUN   TestAccCloudFlareLoadBalancerPool_CreateAfterManualDestroy
=== PAUSE TestAccCloudFlareLoadBalancerPool_CreateAfterManualDestroy
=== RUN   TestAccCloudFlareLoadBalancer_Basic
=== PAUSE TestAccCloudFlareLoadBalancer_Basic
=== RUN   TestAccCloudFlareLoadBalancer_GeoBalanced
=== PAUSE TestAccCloudFlareLoadBalancer_GeoBalanced
=== RUN   TestAccCloudFlareLoadBalancer_DuplicatePool
=== PAUSE TestAccCloudFlareLoadBalancer_DuplicatePool
=== RUN   TestAccCloudFlareLoadBalancer_Update
=== PAUSE TestAccCloudFlareLoadBalancer_Update
=== RUN   TestAccCloudFlareLoadBalancer_CreateAfterManualDestroy
=== PAUSE TestAccCloudFlareLoadBalancer_CreateAfterManualDestroy
=== CONT  TestAccCloudFlareLoadBalancerMonitor_Import
=== CONT  TestAccCloudFlareLoadBalancerPool_ForceNew
=== CONT  TestAccCloudFlareLoadBalancerMonitor_Update
=== CONT  TestAccCloudFlareLoadBalancer_CreateAfterManualDestroy
=== CONT  TestAccCloudFlareLoadBalancer_Update
=== CONT  TestAccCloudFlareLoadBalancer_DuplicatePool
=== CONT  TestAccCloudFlareLoadBalancer_GeoBalanced
=== CONT  TestAccCloudFlareLoadBalancer_Basic
--- PASS: TestAccCloudFlareLoadBalancer_DuplicatePool (2.05s)
=== CONT  TestAccCloudFlareLoadBalancerPool_CreateAfterManualDestroy
--- PASS: TestAccCloudFlareLoadBalancerMonitor_Import (2.23s)
=== CONT  TestAccCloudFlareLoadBalancer_Import
--- PASS: TestAccCloudFlareLoadBalancerMonitor_Update (6.52s)
=== CONT  TestAccCloudFlareLoadBalancerMonitor_FullySpecified
--- PASS: TestAccCloudFlareLoadBalancerMonitor_FullySpecified (2.51s)
=== CONT  TestAccCloudFlareLoadBalancerPool_Basic
--- PASS: TestAccCloudFlareLoadBalancer_Basic (9.82s)
=== CONT  TestAccCloudFlareLoadBalancerPool_FullySpecified
--- PASS: TestAccCloudFlareLoadBalancerPool_ForceNew (9.89s)
=== CONT  TestAccCloudFlareLoadBalancerMonitor_CreateAfterManualDestroy
--- PASS: TestAccCloudFlareLoadBalancer_GeoBalanced (10.49s)
=== CONT  TestAccCloudFlareLoadBalancerPool_Import
--- PASS: TestAccCloudFlareLoadBalancerPool_CreateAfterManualDestroy (9.03s)
=== CONT  TestAccCloudFlareLoadBalancerMonitor_Basic
--- PASS: TestAccCloudFlareLoadBalancer_Update (11.71s)
--- PASS: TestAccCloudFlareLoadBalancer_Import (9.48s)
--- PASS: TestAccCloudFlareLoadBalancerPool_Basic (2.71s)
--- PASS: TestAccCloudFlareLoadBalancer_CreateAfterManualDestroy (12.00s)
--- PASS: TestAccCloudFlareLoadBalancerPool_FullySpecified (2.65s)
--- PASS: TestAccCloudFlareLoadBalancerMonitor_Basic (2.47s)
--- PASS: TestAccCloudFlareLoadBalancerPool_Import (4.64s)
--- PASS: TestAccCloudFlareLoadBalancerMonitor_CreateAfterManualDestroy (5.51s)
PASS
ok      github.com/terraform-providers/terraform-provider-cloudflare/cloudflare 15.420s

benjvi and others added 9 commits March 5, 2018 11:01

add load balancer resource

f3f68e3

add load balancer pool resource

7937fe0

add tests for load balancer pool resource

d54cd1e

add tests for load balancer resource

b971649

add docs for load balancers + pools

b5dcd77

clean up of load balancing resources

c08a673

simplify id in load balancer

b80b823

error checking for setting non-scalars in load balancer + pool

3271b44

Merge branch 'master' into load-balancer

a079c23

benjvi mentioned this pull request Mar 18, 2018

Load balancer + load balancer pool resources #31

Closed

benjvi added 3 commits March 21, 2018 11:18

address review comments for load balancer + pool

ad79ad6

document that load balancer feature needs explicitly enabling

feb72d2

Merge branch 'master' into updated-load-balancer

0f555c5

vancluever added kind/enhancement Categorizes issue or PR as related to improving an existing feature. kind/new-resource Categorizes issue or PR as related to creating a new resource. labels Mar 28, 2018

catsby reviewed Apr 3, 2018

View reviewed changes

validate + test lb with duplicate pool locations

2c76028

benjvi force-pushed the load-balancer branch from f8cb9a5 to 2c76028 Compare April 4, 2018 12:47

Merge branch 'master' into load-balancer

61e4aff

make org id configurable -> provider users org APIs

2626ddf

benjvi force-pushed the load-balancer branch from 24142ec to 2626ddf Compare April 6, 2018 15:11

catsby merged commit 755f8d4 into cloudflare:master Apr 6, 2018

This was referenced Oct 25, 2018

Modifications to LB Pools being used by LBs don't follow proper sequencing #66

Closed

Implement Update for cloudflare_load_balancer_pool #140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load balancer + load balancer pool resources (updated) #40

Load balancer + load balancer pool resources (updated) #40

benjvi commented Mar 18, 2018

nickithewatt commented Mar 18, 2018

benjvi commented Mar 23, 2018

prdonahue commented Mar 27, 2018

catsby left a comment

catsby Apr 3, 2018

catsby Apr 3, 2018

catsby Apr 3, 2018

benjvi commented Apr 4, 2018

catsby commented Apr 4, 2018

catsby commented Apr 4, 2018

benjvi commented Apr 4, 2018

catsby commented Apr 5, 2018

elithrar commented Apr 5, 2018

catsby commented Apr 5, 2018

benjvi commented Apr 6, 2018 •

edited

Loading

benjvi commented Apr 6, 2018

elithrar commented Apr 6, 2018

catsby commented Apr 6, 2018

Load balancer + load balancer pool resources (updated) #40

Load balancer + load balancer pool resources (updated) #40

Conversation

benjvi commented Mar 18, 2018

nickithewatt commented Mar 18, 2018

benjvi commented Mar 23, 2018

prdonahue commented Mar 27, 2018

catsby left a comment

Choose a reason for hiding this comment

catsby Apr 3, 2018

Choose a reason for hiding this comment

catsby Apr 3, 2018

Choose a reason for hiding this comment

catsby Apr 3, 2018

Choose a reason for hiding this comment

benjvi commented Apr 4, 2018

catsby commented Apr 4, 2018

catsby commented Apr 4, 2018

benjvi commented Apr 4, 2018

catsby commented Apr 5, 2018

elithrar commented Apr 5, 2018

catsby commented Apr 5, 2018

benjvi commented Apr 6, 2018 • edited Loading

benjvi commented Apr 6, 2018

elithrar commented Apr 6, 2018

catsby commented Apr 6, 2018

benjvi commented Apr 6, 2018 •

edited

Loading