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.

Sign up for GitHub

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

Jump to bottom

Implement per-route options in Cloud Controller #4080

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Implement per-route options in Cloud Controller #4080

wants to merge 10 commits into from

Conversation

hoffmaen
Copy link

@hoffmaen hoffmaen commented Nov 8, 2024 •
edited
Loading

A short explanation of the proposed change:

This Pull-Request adds per-route options to the Cloud Controller. Support is added for the following APIs:

  • POST /v3/routes
  • GET /v3/routes
  • PATCH /v3/routes

Per-route options support is also added to the manifest.

An explanation of the use cases your change solves

Users / app developers want to define load-balancing algorithms per route, instead of being forced to use the globally defined algorithm.

Links to any other associated PRs

a18e and others added 2 commits November 13, 2024 14:02
@a18e @hoffmaen
Introduce per-route options
da667dd 
This commit adds a configurable load-balancing algorithm as a first example of per-route options.
It adds the 'options' field to the route object in the V3 API, and the app manifest.
The options field is an object storing key-value pairs, with 'lb_algo' being the only supported key for now.
The supported load-balancing algorithms are 'round-robin' and 'least-connections'.
The options field is introduced as a column in the database route table, and forwarded to the Diego backend.

Co-authored-by: Alexander Nicke <[email protected]>
See: cloudfoundry/capi-release#482
See: https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0027-generic-per-route-features.md
@hoffmaen
Add route-options documentation
d5e369c 
Add documentation for the route options object, and its supported fields.
@a18e
Adjust route options behaviour for manifest push
35d8b72 
Overwrite behaviour for route options is now fixed and tested:
Existing options are not modified if options is nil, {} or not provided
A single option (e.g. loadbalancing-algorithm) can be removed by setting its value to nil
@hoffmaen hoffmaen marked this pull request as ready for review November 21, 2024 09:37
@jochenehret jochenehret self-requested a review November 22, 2024 09:00
jochenehret
jochenehret reviewed Nov 22, 2024
View reviewed changes
Copy link
Contributor

@jochenehret jochenehret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll test the proposed changes on a dev landscape.

@jochenehret
Copy link
Contributor

I've tested the new feature on a dev landscape and everything works as expected. There are however a few corner cases that we should discuss:

1. "options" with null value

This request is accepted:

cf curl -X PATCH /v3/routes/<guid> -d '{"options": null}'

It doesn't change anything. Is that intended or should the request be rejected?

2. Valid option key with null value

cf curl -X PATCH /v3/routes/<guid> -d '{"options": {"lb_algo": null}}'

Accepted and sets the "options" field to {} (empty map). Shouldn't this be rejected?

3. Create new route without "options"

Use the CF CLI to create a new route:

cf -v create-route <domain> --hostname <host>

This will send a POST request:

cf curl -X POST /v3/routes -d ...

Now the "option" field in the database is set to "null" (text "null"). For existing routes the option field is initially null (matches "where is null"). If found no problems with the "null" text, but I guess it would be better to set the "options" value to null for new routes.

4. "options" in app manifest

Pushing an app with manifest works:

version: 1
applications:
- name: test
  routes:
  - route: example.com
    options:
      loadbalancing-algorithm: round-robin

For configuration with null values, we can observe the same behaviour as above:
Setting "options" to null doesn't change anything.
Replacing "round-robin" with null and pushing again sets the options to {} (empty map).

jochenehret
jochenehret reviewed Nov 22, 2024
View reviewed changes
app/messages/manifest_routes_update_message.rb

record.errors.add(:routes, message: 'must be a list of route objects')
if contains_invalid_route_options?(record.routes)
record.errors.add(:routes, message: 'contains invalid route options')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good if all validation error messages contained the list of valid options.

@jochenehret
Copy link
Contributor

Did you plan to create a new acceptance test for this feature? You could add it to this capi-bara-test:
https://github.com/cloudfoundry/capi-bara-tests/blob/fff7cf1f121c13e58c06e1c9d5cfec0e887f724c/baras/manifest.go#L141

a18e and others added 2 commits November 25, 2024 14:57
@hoffmaen
Copy link
Author

Thanks for your review @jochenehret

1. "options" with null value

This request is accepted:

cf curl -X PATCH /v3/routes/<guid> -d '{"options": null}'

It doesn't change anything. Is that intended or should the request be rejected?

You found a recently introduced bug, we fix it. According to the tracking issue, this request should unset all options (which it does now again).

2. Valid option key with null value

cf curl -X PATCH /v3/routes/<guid> -d '{"options": {"lb_algo": null}}'

Accepted and sets the "options" field to {} (empty map). Shouldn't this be rejected?

No, this is the expected behavior, as documented in the tracking issue.

3. Create new route without "options"

Use the CF CLI to create a new route:

cf -v create-route <domain> --hostname <host>

This will send a POST request:

cf curl -X POST /v3/routes -d ...

Now the "option" field in the database is set to "null" (text "null"). For existing routes the option field is initially null (matches "where is null"). If found no problems with the "null" text, but I guess it would be better to set the "options" value to null for new routes.

There is another post in the tracking issue. @Gerg argues for always providing the options field, even if no options are set. If we adopt his idea, we could replace null (or 'null' accidentally) with an empty hash.

4. "options" in app manifest

Pushing an app with manifest works:

version: 1
applications:
- name: test
  routes:
  - route: example.com
    options:
      loadbalancing-algorithm: round-robin

For configuration with null values, we can observe the same behaviour as above: Setting "options" to null doesn't change anything. Replacing "round-robin" with null and pushing again sets the options to {} (empty map).

We fixed this - unsetting options in general or loadbalancing-algorithm in particular is not possible any more.

hoffmaen and others added 2 commits November 26, 2024 14:21
@hoffmaen
WIP: Add options case for route_update_spec
f62163e
@a18e
Adjust behaviour to new decisions:
9c630be 
options default: {}
API:
 options is not nullable
 specific option is additive
 specific option is nullable
 empty hash does not change anything (additive)
 get empty options -> {}
manifest:
 options and specific option is not nullable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jochenehret jochenehret jochenehret left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoffmaen @jochenehret @a18e