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

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

Closed
garrettgalow opened this issue Jun 12, 2018 · 12 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@garrettgalow
Copy link
Contributor

Actual Behavior

When adding a monitor to a pool that is currently being used. Terraform attempts to destroy and re-create the pool. This fails as the pool is being used by the Load Balancer.

Expected Behavior

The pool would be recreated, or if necessary the LB would be recomputed as neccesary to create all resources as modified.

`
Error: Error applying plan:

1 error(s) occurred:

  • cloudflare_load_balancer_pool.gke_pool (destroy): 1 error(s) occurred:

  • cloudflare_load_balancer_pool.gke_pool: error deleting CloudFlare Load Balancer Pool: error from makeRequest: HTTP status 412: content "{\n "result": null,\n "success": false,\n "errors": [\n {\n "code": 1005,\n "message": "This object is referenced by other objects, delete them first."\n }\n ],\n "messages": []\n}\n"
    `

Terraform Version

Terraform v0.11.7

  • provider.azurerm v1.6.0
  • provider.cloudflare v1.0.0
  • provider.google v1.13.0
  • provider.kubernetes v1.1.0

Affected Resource(s)

Please list the resources as a list, for example:

  • cloudflare_load_balancer_pool
  • cloudflare_load_balancer
@patryk patryk added the kind/bug Categorizes issue or PR as related to a bug. label Jul 12, 2018
@oliviabarrick
Copy link

This is a really serious issue, by the way. It makes the load balancer resource essentially unusable.

@SteveGoldthorpe-Work
Copy link
Contributor

I hit this today.
origins.#: "2" => "1" (forces new resource)

You can't force a new resource for pools as there will be LBs that depend on them.
cloudflare_load_balancer_pool.za-preprod-wongatest-haproxy: error deleting Cloudflare Load Balancer Pool: error from makeRequest: HTTP status 412: content "{\n \"result\": null,\n \"success\": false,\n \"errors\": [\n {\n \"code\": 1005,\n \"message\": \"This object is referenced by other objects, delete them first.\"\n }\n ],\n \"messages\": []\n}\n"

In the provider resource_cloudflare_load_balancer_pool.go needs to implement an update method using ModifyLoadBalancerPool and remove all the ForceNew flags where appropriate.

@puckey
Copy link
Contributor

puckey commented Oct 25, 2018

I am running into the same issue. Is it possible to give us an idea if this will be looked at, at some point? I was hoping to use Cloudflare's geo based load balancing for my website, but I am worried about running into these issues when I want to make changes after going to production. I am happy to work on a reproduction of the bug, if this helps speeding up the process.

@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 25, 2018 via email

@jacobbednarz
Copy link
Member

After looking into this, I can see that the cloudflare_load_balancer_pool resource is the culprit. Whenever the origins change, it wants to force creation of a new resource https://github.com/terraform-providers/terraform-provider-cloudflare/blob/61c97e18d48a1a3471b1d76e460bd085876d7b9d/cloudflare/resource_cloudflare_load_balancer_pool.go#L36-L41

However, the organisation and user load balancer pools both support in-place updates. The resource was added in #40 however the git blame doesn't really cover why ForceNew was included in the element schema. Perhaps it was related to not having the update method implemented yet 🤷‍♂️

Anyways, I'll put up a change for this shortly.

@jacobbednarz
Copy link
Member

The plot thickens.

  • cloudflare_load_balancer_pool points at the user load balancer pool resource however I'm continually getting an error message about origin length needing to be between 0 and 1. I've raised a ticket with support (1592498) to confirm whether it's possible to use this endpoint as an organisation member. I have a personal account but no load balancing enabled so unable to test there.
  • cloudflare_load_balancer does not support organisation level management (at the moment). This will need to be added.

@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 25, 2018

I've raised a ticket with support (1592498) to confirm whether it's possible to use this endpoint as an organisation member.

The answer here is that organisation members cannot use /user endpoints for load balancing. If a user belongs to an organisation, they must use the /organisations/:id endpoints instead.

@jacobbednarz
Copy link
Member

@puckey @SteveGoldthorpe-Work I've opened a fix for this in #140. Can you please check it out and report back if it addresses your issues? I don't have a personal account with it enabled to confirm.

@puckey
Copy link
Contributor

puckey commented Oct 26, 2018

Amazing, thanks!

I built and installed your branch into ~/.terraform.d/plugins, but I don't think terraform is picking it up.. the log messages and errors I am seeing are the same as before. Will try again on Monday.

@SteveGoldthorpe-Work
Copy link
Contributor

Don't have a personal account either.
PR works fine with my enterprise account (I set use_org_from_zone in my cloudflare provider definition).

@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 26, 2018 via email

@puckey
Copy link
Contributor

puckey commented Oct 26, 2018

Thanks – I can verify that it is working for me. I can now add & change a monitor without it recreating the load balancer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

6 participants