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

Fixing bug with minimumOrigins=0 #1338

Merged
merged 4 commits into from Aug 1, 2023
Merged

Fixing bug with minimumOrigins=0 #1338

merged 4 commits into from Aug 1, 2023

Conversation

indeed-kelvin
Copy link
Contributor

Provide a general summary of your changes in the title above. You should
remove this overview, any sections and any section descriptions you
don't need below before submitting. There isn't a strict requirement to
use this template if you can structure your description and still cover
these points.

Description

Closes #1337

Has your change been tested?

Confirmed pool gets created properly with the code snippet from issue #1337

Screenshots (if appropriate):

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Folks will have to use a int pointer for minimumOrigins instead of int for LoadBalancerPool.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented in cloudflare/api-schemas
    and relies on stable APIs.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2023

changelog detected ✅

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2023

Codecov Report

Merging #1338 (11cb0be) into master (b9ac804) will increase coverage by 0.08%.
The diff coverage is 58.14%.

@@            Coverage Diff             @@
##           master    #1338      +/-   ##
==========================================
+ Coverage   48.33%   48.42%   +0.08%     
==========================================
  Files         133      134       +1     
  Lines       13023    13104      +81     
==========================================
+ Hits         6295     6345      +50     
- Misses       5201     5220      +19     
- Partials     1527     1539      +12     
Impacted Files Coverage Δ
load_balancing.go 59.40% <ø> (ø)
regional_tiered_cache.go 43.75% <43.75%> (ø)
logpush.go 51.81% <46.15%> (-0.30%) ⬇️
images.go 44.70% <47.82%> (-0.12%) ⬇️
rulesets.go 30.03% <52.17%> (-4.61%) ⬇️
pages_deployment.go 38.88% <61.76%> (+4.64%) ⬆️
pages_project.go 58.22% <90.00%> (+7.40%) ⬆️
access_policy.go 70.27% <100.00%> (ø)
pagination.go 100.00% <100.00%> (+28.57%) ⬆️

... and 2 files with indirect coverage changes

@jacobbednarz
Copy link
Member

@tc80 are you able to confirm if this is something we intentionally don't support? i'm not sure why someone would want an empty pool but i may be missing something.

@tc80
Copy link
Contributor

tc80 commented Jul 20, 2023

@jacobbednarz thanks for tagging me!

From the API docs, minimum_origins is:

The minimum number of origins that must be healthy for this pool to serve traffic. If the number of healthy origins falls below this number, the pool will be marked unhealthy and will failover to the next available pool.

So a bit different from an "empty pool". I suppose if minimum_origins=0, then a pool will never be marked unhealthy and will always be able to serve traffic. I'll ask the team, but if this is allowed via the API then it should also be allowed via terraform.

@indeed-kelvin
Copy link
Contributor Author

indeed-kelvin commented Jul 21, 2023

i'm not sure why someone would want an empty pool but i may be missing something.

To add some context, we are building a kubernetes operator to manage cloudflare objects and we have separate CRDs for pools and origins. We want our users to be able to create pool objects in a cluster and then origin objects from any cluster. Thus we have a need to be able to create pools with no origins so users can attach origins to them later. We tested this behavior and found we can do so only if minimum_origins is set to 0. If it is set to anything higher, we get the error: "the minimum_origins must be in range [1, 0]: validation failed" if we don't define any origins. Thus not being able to set minimum_origins to 0 effectively prevents us from being able to create empty pools.

kelvin@IT-USA-VY1005 % curl --request POST -H "X-Auth-Key: $CF_API_KEY" \
  -H "X-Auth-Email: $CF_API_EMAIL" \
  -H "Content-Type: application/json" \
  "https://api.cloudflare.com/client/v4/accounts/<redacted>/load_balancers/pools" \
  --data '{
    "name": "pool-sample",
    "description": "pool-sample",
    "enabled": true,
    "latitude": 15,
    "longitude": 10,
    "minimum_origins": 1
  }'
{
  "result": null,
  "success": false,
  "errors": [
    {
      "code": 1002,
      "message": "the minimum_origins must be in range [1, 0]: validation failed"
    }
  ],
  "messages": []
}

kelvin@IT-USA-VY1005 % curl --request POST -H "X-Auth-Key: $CF_API_KEY" \
  -H "X-Auth-Email: $CF_API_EMAIL" \
  -H "Content-Type: application/json" \
  "https://api.cloudflare.com/client/v4/accounts/<redacted>/load_balancers/pools" \
  --data '{
    "name": "pool-sample",
    "description": "pool-sample",
    "enabled": true,
    "latitude": 15,
    "longitude": 10,
    "minimum_origins": 0
  }'
{
  "result": {
    "description": "pool-sample",
    "created_on": "2023-07-21T19:53:14.944732Z",
    "modified_on": "2023-07-21T19:53:14.944732Z",
    "id": "2cfcd8f564eaf40b6bd09195c122e21e",
    "enabled": true,
    "minimum_origins": 0,
    "name": "pool-sample",
    "notification_email": "",
    "check_regions": null,
    "latitude": 15,
    "longitude": 10,
    "origins": []
  },
  "success": true,
  "errors": [],
  "messages": []
}

@tc80
Copy link
Contributor

tc80 commented Aug 1, 2023

talked with the team and this should be okay to merge 👍
cc @gofish

@jacobbednarz jacobbednarz merged commit 77cf46d into cloudflare:master Aug 1, 2023
11 checks passed
@github-actions github-actions bot added this to the v0.74.0 milestone Aug 1, 2023
github-actions bot pushed a commit that referenced this pull request Aug 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

This functionality has been released in v0.74.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attempts to create a pool with 0 origins will always fail
4 participants