Support load balancer origin weights and session affinity #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lawrencejones
changed the title
|
I've added session affinity here, as that comes in the same dependency update that the origin weights are included in. There is a question about whether we'd use the key I defer to the maintainers on this decision though. |
lawrencejones
force-pushed
the
lawrence-support-load-balancer-weights
branch
from
6e84fab to
6dc7d3e
Compare
|
Not sure exactly what's failing in the CI environment. If anyone more familiar with the setup could help, that would be appreciated. |
|
@prdonahue - can you re-start this build? Looks like a network error at the time. |
|
I'm going to re-touch the commit to get the build going again. |
lawrencejones
force-pushed
the
lawrence-support-load-balancer-weights
branch
from
6dc7d3e to
40aef1e
Compare
|
Yeah was a flaky build. @prdonahue should be all good to review :) |
|
There we go! Thanks @lawrencejones. I'm just running a full test ( |
|
Seeing a few test failures for tests that create/mutate a Load Balancer object -> ➜ ~/.go/src/github.com/terraform-providers/terraform-provider-cloudflare (pr-57/lb-weights) ✔ make testacc TESTARGS='-run=LoadBalancer'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=LoadBalancer -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_SessionAffinity
=== PAUSE TestAccCloudFlareLoadBalancer_SessionAffinity
=== 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_DuplicatePool
--- PASS: TestAccCloudFlareLoadBalancer_DuplicatePool (1.46s)
=== CONT TestAccCloudFlareLoadBalancer_CreateAfterManualDestroy
--- PASS: TestAccCloudFlareLoadBalancerMonitor_Import (1.89s)
=== CONT TestAccCloudFlareLoadBalancer_Update
--- FAIL: TestAccCloudFlareLoadBalancer_CreateAfterManualDestroy (2.27s)
testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:
* cloudflare_load_balancer.39xznlen2c: 1 error(s) occurred:
* cloudflare_load_balancer.39xznlen2c: error creating load balancer for zone: error from makeRequest: HTTP status 400: content "{\n \"result\": null,\n \"success\": false,\n \"errors\": [\n {\n \"code\": 6007,\n \"message\": \"Malformed request body.\"\n }\n ],\n \"messages\": []\n}\n"
=== CONT TestAccCloudFlareLoadBalancerPool_Basic
--- FAIL: TestAccCloudFlareLoadBalancer_Update (1.92s)
testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:
* cloudflare_load_balancer.nnjm0rx9s9: 1 error(s) occurred:
* cloudflare_load_balancer.nnjm0rx9s9: error creating load balancer for zone: error from makeRequest: HTTP status 400: content "{\n \"result\": null,\n \"success\": false,\n \"errors\": [\n {\n \"code\": 6007,\n \"message\": \"Malformed request body.\"\n }\n ],\n \"messages\": []\n}\n"
=== CONT TestAccCloudFlareLoadBalancerPool_FullySpecified
--- PASS: TestAccCloudFlareLoadBalancerMonitor_Update (3.91s)
=== CONT TestAccCloudFlareLoadBalancer_SessionAffinity
--- PASS: TestAccCloudFlareLoadBalancerPool_Basic (1.76s)
=== CONT TestAccCloudFlareLoadBalancer_GeoBalanced
--- PASS: TestAccCloudFlareLoadBalancerPool_ForceNew (6.23s)
=== CONT TestAccCloudFlareLoadBalancer_Basic
--- PASS: TestAccCloudFlareLoadBalancerPool_FullySpecified (2.62s)
=== CONT TestAccCloudFlareLoadBalancerMonitor_Basic
--- PASS: TestAccCloudFlareLoadBalancer_SessionAffinity (4.48s)
=== CONT TestAccCloudFlareLoadBalancerMonitor_FullySpecified
--- FAIL: TestAccCloudFlareLoadBalancer_GeoBalanced (3.02s)
testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:
* cloudflare_load_balancer.3217kq931c: 1 error(s) occurred:
* cloudflare_load_balancer.3217kq931c: error creating load balancer for zone: error from makeRequest: HTTP status 400: content "{\n \"result\": null,\n \"success\": false,\n \"errors\": [\n {\n \"code\": 6007,\n \"message\": \"Malformed request body.\"\n }\n ],\n \"messages\": []\n}\n"
=== CONT TestAccCloudFlareLoadBalancerMonitor_CreateAfterManualDestroy
--- PASS: TestAccCloudFlareLoadBalancerMonitor_Basic (2.21s)
=== CONT TestAccCloudFlareLoadBalancer_Import
--- FAIL: TestAccCloudFlareLoadBalancer_Basic (2.55s)
testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:
* cloudflare_load_balancer.xeeiqnioru: 1 error(s) occurred:
* cloudflare_load_balancer.xeeiqnioru: error creating load balancer for zone: error from makeRequest: HTTP status 400: content "{\n \"result\": null,\n \"success\": false,\n \"errors\": [\n {\n \"code\": 6007,\n \"message\": \"Malformed request body.\"\n }\n ],\n \"messages\": []\n}\n"
=== CONT TestAccCloudFlareLoadBalancerPool_CreateAfterManualDestroy
--- PASS: TestAccCloudFlareLoadBalancerMonitor_FullySpecified (1.87s)
=== CONT TestAccCloudFlareLoadBalancerPool_Import
--- FAIL: TestAccCloudFlareLoadBalancer_Import (2.12s)
testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:
* cloudflare_load_balancer.is8n37ej0f: 1 error(s) occurred:
* cloudflare_load_balancer.is8n37ej0f: error creating load balancer for zone: error from makeRequest: HTTP status 400: content "{\n \"result\": null,\n \"success\": false,\n \"errors\": [\n {\n \"code\": 6007,\n \"message\": \"Malformed request body.\"\n }\n ],\n \"messages\": []\n}\n"
--- PASS: TestAccCloudFlareLoadBalancerMonitor_CreateAfterManualDestroy (3.27s)
--- PASS: TestAccCloudFlareLoadBalancerPool_Import (1.83s)
--- PASS: TestAccCloudFlareLoadBalancerPool_CreateAfterManualDestroy (4.31s)
FAIL
FAIL github.com/terraform-providers/terraform-provider-cloudflare/cloudflare 13.110s
make: *** [testacc] Error 1Looking into the details. |
|
Ahh, these are all on tests I can't run (no multi-geo support from my cheap account). Any help with this would be appreciated! |
|
Email me your account details and I'll bump you up behind the scenes. It
won't "stick" if you make a subscription change (the sub change will reset
things), but you won't be charged any extra.
msilverlock @ cloudflare.com
…On Fri, May 4, 2018 at 8:52 AM Lawrence Jones ***@***.***> wrote:
Ahh, these are all on tests I can't run (no multi-geo support from my
cheap account). Any help with this would be appreciated!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<https://github.com/terraform-providers/terraform-provider-cloudflare/pull/57#issuecomment-386644504>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABIcP-vRG-StoKXxS4B5cP-9E3TSCpfks5tvHlCgaJpZM4TsXz->
.
|
|
OK, figured it out -> When adding session_affinity, the empty value is the string e.g. // Correct
' --data-binary '{"description":"","created_on":"2018-05-04T16:15:53.433059Z","modified_on":"2018-05-04T16:15:53.433059Z","id":"122e4868dcbfade9a3cd5bd594bea2e3","proxied":true,"enabled":true,"name":"repeater.questionable.services","session_affinity":"none","fallback_pool":"bfa5f38006d89f6abf0ffc311f71709b","default_pools":["bfa5f38006d89f6abf0ffc311f71709b"],"pop_pools":{},"region_pools":{}}' --compressed// Incorrect
' --data-binary '{"description":"","created_on":"2018-05-04T16:15:53.433059Z","modified_on":"2018-05-04T16:15:53.433059Z","id":"122e4868dcbfade9a3cd5bd594bea2e3","proxied":true,"enabled":true,"name":"repeater.questionable.services","session_affinity":null,"fallback_pool":"bfa5f38006d89f6abf0ffc311f71709b","default_pools":["bfa5f38006d89f6abf0ffc311f71709b"],"pop_pools":{},"region_pools":{}}' --compressedThus (and these aren't mutually exclusive) ->
|
|
That's great, thanks for that! Not going to get time today but will definitely fix this over the weekend- your comment should make it an easy fix. |
|
Addressing in cloudflare/cloudflare-go#179 -> waiting on build & a quick review from a colleague, then I can tag a new cloudflare-go release. |
|
Addressed in v0.8.5 of cloudflare-go - https://github.com/cloudflare/cloudflare-go/releases/tag/v0.8.5 If you update the pinned version of cloudflare-go to |
|
Apply this patch to the repo, or just run |
Updating the cloudflare-go library brings support for the weight parameter of load balancer origins. This allows configuration of traffic priority to each of the backend origins, which is critical in more complex load balancing setups.
The newer cloudflare-go library supports configuring session affinity values for Cloudflare load balancers. This commit updates the terraform resource to support this field.
lawrencejones
force-pushed
the
lawrence-support-load-balancer-weights
branch
from
40aef1e to
f0d0c43
Compare
|
Rebased with the latest version, ran the tests and other than some rate limits things seemed to work! Will wait for a full test run on your end to confirm though. |
|
Tests pass in full - @prdonahue to merge as I don't have write perms on this repo yet |
|
@catsby could we get this reviewed/merged please? |
|
Hey @prdonahue, just a quick ping on this. We're quite keen to use this in production, and it feels like we don't have anything blocking a merge? |
|
@lawrencejones sorry, waiting on a HashiCorp review. @catsby mind merging this? |
catsby
suggested changes
May 10, 2018
There was a problem hiding this comment.
Hello! There doesn't appear to be any documentation to match these new options, we'll need those added before we can merge.
Additionally, I see weight is being added to originsElem, which is used by at least the origins attribute. Because origins is a TypeSet and we're adding a new field to it, I beleive the hash ID for each preexisting origins block will change. As a result, anyone who has existing infrastructure and upgrades to this new version, will see a change as the IDs are now different. Because origins are ForceNew and Required, I believe this will result Terraform attempting to destroy and recreate any existing load balancer pools, which is probably not what we want to happen.
Can someone verify this is or is not the behavior you get when you upgrade? First create a load balancer pool with the current version of this Provider, then install a version with this new weight feature compiled in, and verify the results with terraform plan.
If this is the case, we'll need to include a State migration.
Example:
- https://github.com/terraform-providers/terraform-provider-aws/blob/b56b195b93d20bd73cd79774910febcb6cc56ee5/aws/resource_aws_instance.go#L28-L33
- https://github.com/terraform-providers/terraform-provider-aws/blob/b56b195b93d20bd73cd79774910febcb6cc56ee5/aws/resource_aws_instance_migrate.go
Let me know!
|
@prdonahue I reviewed, it needs documentation added, and I have a question/suspicion about the upgrade path |
|
Hey @catsby So I did the test and the behaviour isn't the one you're referring to but still bad, tho. When I create an LB configuration with the actual stable version (so without weights for the origins) everything goes fine. When I try to add the weights, I'm getting an error, which is good for me as we don't support this feature in the actual version: I did fetch locally the new branch and build it locally, then a The issue is that when I'm adding ~ cloudflare_load_balancer_pool.foo
origins.1712833627.address: "" => "192.0.2.1"
origins.1712833627.enabled: "" => "false"
origins.1712833627.name: "" => "example-1"
origins.1712833627.weight: "" => "0.2"
origins.1836039525.address: "" => "192.0.2.2"
origins.1836039525.enabled: "" => "true"
origins.1836039525.name: "" => "example-2"
origins.1836039525.weight: "" => "0.8"
origins.4022274604.address: "192.0.2.1" => ""
origins.4022274604.enabled: "false" => "false"
origins.4022274604.name: "example-1" => ""
origins.4022274604.weight: "1" => "0"
origins.4120949060.address: "192.0.2.2" => ""
origins.4120949060.enabled: "true" => "false"
origins.4120949060.name: "example-2" => ""
origins.4120949060.weight: "1" => "0"
Plan: 0 to add, 1 to change, 0 to destroy.So as you can see, the ID isn't rotating and a simple change is planned for the pool. The issue comes with the Here is my TF configuration file: Spotted a glitch in our cloudflare-go plugin as well, the plugin is creating the pool at the user level even if the user belongs to an Organisation ( Let me know if you want me to do some extended tests. |
|
Hey all! Really busy at work right now, so not going to have time to address this imminently, but if anyone wants to take over the PR in the meantime then let me know and I'll add you to the repo so you can push to this branch. Would hate to hold up anyone who was free to work on this! |
|
Would love to see this merged as we're relying on the |
|
Hey @grega, I’ve given you push access to this fork so you can add commits if you need this. We’re not doing any work on this just right now, so I don’t expect to get time to work on it soon. Hopefully this means we can both make changes whenever we might get the chance! |
ghost
added
the
|
Merged in #153. Thank you @lawrencejones for initial PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updating the cloudflare-go library brings support for the weight
parameter of load balancer origins. This allows configuration of traffic
priority to each of the backend origins, which is critical in more
complex load balancing setups.
I don't have a full-featured Cloudflare account, so I can't run all the tests. The fully specified load balancer test is within reach of my pauper setup, so I can provide test output for that!