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

RFC0027 CLI Support for Generic Per-Route Options [main] #3372

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

Conversation

Dariquest
Copy link
Contributor

CLI Support for Generic Per-Route Options

Introducing a new cli command 'update-route', which allows updates to route resources.
Extending the API of the Route to contain per route options and particularly the first one called "loadbalancing", containing a load balancing algorithm of a route.

Existing commands "create-route", "map-route", "routes", "route", "apps", "app" were also extended to accept and output the per-route options.

Detailed specification including that of for all the involved components is provided in the RFC

$ cf update-route -h
NAME:
   update-route - Update an existing route.

USAGE:
   cf update-route DOMAIN [--hostname HOSTNAME] [--path PATH] [--option OPTION=VALUE] [--remove-option OPTION]

EXAMPLES:
   cf update-route example.com -o loadbalancing=round-robin
   cf update-route example.com -o loadbalancing=least-connection
   cf update-route example.com -r loadbalancing

OPTIONS:
   --hostname, -n      Hostname for the HTTP route (required for shared domains)
   --path              Path for the HTTP route
   --option -o         Set the value of a per-route option, key-value pairs, repeat to set multiple options
   --remove-option -r  Unset a previously set option

SEE ALSO:
   create-route, map-route, routes, unmap-route

It is possible to provide options in the create-route and map-route commands.

EXAMPLES:
  cf create-route my-space example.com -o loadbalancing=round-robin
  cf map-route my-app example.com -o loadbalancing=least-connection

Why Is This PR Valuable?

Allow users to manage per-route options via the CLI instead of having to talk to the API directly.

Applicable Issues

GitHub Issue

How Urgent Is The Change?

No urgency

@Dariquest Dariquest marked this pull request as draft January 22, 2025 10:21
@Dariquest Dariquest marked this pull request as ready for review January 22, 2025 11:19
@Dariquest Dariquest marked this pull request as draft January 23, 2025 07:19
@Dariquest Dariquest marked this pull request as ready for review January 23, 2025 15:16
@Dariquest Dariquest changed the title RFC0027 Enable "update-route", "create-route", "map-route", "routes", "route", "apps" & "app" to manage generic per-route options RFC0027 CLI Support for Generic Per-Route Options [main] Jan 24, 2025
@hoffmaen
Copy link

hoffmaen commented Jan 27, 2025

I did some manual tests and found some unexpected behavior:

  1. Create a route without options:
$ cf create-route vcap.me --hostname testroute
Route testservice.vcap.me has been created.
OK.

$ cf routes
space   host             domain    port   path   protocol   app-protocol   apps   service instance   options
test       testroute     vcap.me                       http
  1. Use map-route with the new -o flag

a) Using a valid value for loadbalancing:

$ cf map-route testapp vcap.me --hostname testroute -o loadbalancer=round-robin                                                                                                                                                                                                                                                                                                                                              
Mapping route testroute.vcap.me to app testapp in org the-system_domain-org-name / space test as ccadmin...
OK

🔴 This does not set the option:

$ cf routes
Getting routes for org the-system_domain-org-name / space test as ccadmin...
space   host          domain    port   path   protocol   app-protocol   apps      service instance   options
test       testroute  vcap.me                      http          http1                 testapp

b) Using an invalid option:

$ cf map-route testapp vcap.me --hostname testroute -o apples=bananas                                                                                                                                                                                                                                                                                                                                                            
Mapping route testroute.vcap.me to app testapp in org the-system_domain-org-name / space test as ccadmin...
OK

🔴 This should be rejected with an error message.

  1. Using update-route -r does not remove option or return an error, when specifying key-value pairs
$ cf routes
Getting routes for org the-system_domain-org-name / space test as ccadmin...
space   host          domain    port   path   protocol   app-protocol   apps      service instance   options
test       testroute  vcap.me                      http          http1                 testapp                               {loadbalancing=round-robin}

$ cf update-route vcap.me --hostname testroute -r loadbalancing=round-robin                                                                                                                                                                                                                                                                                                                                                      
Updating route testroute.vcap.me for org the-system_domain-org-name / space test as ccadmin...
Route testroute.vcap.me has been updated
OK

$ cf routes
Getting routes for org the-system_domain-org-name / space test as ccadmin...
space   host          domain    port   path   protocol   app-protocol   apps      service instance   options
test       testroute  vcap.me                      http          http1                 testapp                               {loadbalancing=round-robin}
  1. using update-route -r does even update the option, when a valid option is provided:
$ cf update-route vcap.me --hostname testroute -r loadbalancing=least-connections
Updating route testroute.vcap.me for org the-system_domain-org-name / space test as ccadmin...
Route testroute.vcap.me has been updated
OK

$ cf routes
Getting routes for org the-system_domain-org-name / space test as ccadmin...
space   host          domain    port   path   protocol   app-protocol   apps      service instance   options
test       testroute  vcap.me                      http          http1                 testapp                               {loadbalancing=least-connections}

@Dariquest
Copy link
Contributor Author

Dariquest commented Jan 27, 2025

The case 2 a) & b) is an expected behaviour, since specified options only apply to routes, which do not exist and are being created by the map-route command. We spoke about it and we could extend this info in the help and add a warning output for existing routes. It might be helpful to specify options for the non existent routes directly without making the second command call necessary (update-route). On the other hand, one can completely remove this option for the map-route command.
Case 3) & Case 4) this is the wrong usage of the command (flag remove). I will add an error message or warning, a warning might be more appropriate, if we do not like to cancel the command if some options are specified correctly and options to remove are specified the wrong way.

@Dariquest
Copy link
Contributor Author

Dariquest commented Jan 27, 2025

The case 2 a) & b) is an expected behaviour, since specified options only apply to routes, which do not exist and are being created by the map-route command. We spoke about it and we could extend this info in the help and add a warning output for existing routes. It might be helpful to specify options for the non existent routes directly without making the second command call necessary (update-route). On the other hand, one can completely remove this option for the map-route command. Case 3) & Case 4) this is the wrong usage of the command (flag remove). I will add an error message or warning, a warning might be more appropriate, if we do not like to cancel the command if some options are specified correctly and options to remove are specified the wrong way.

After a discussion with @hoffmaen, we decided to keep the map-route options specification, but add an error message if the options are specified for an existing route.
In case of specifying a remove option in a key=value format, the value will be passed to the cloud controller as a key:
"key=value": null. The cloud controller takes care of the validation and delivers an error message in this case.

@hoffmaen
Copy link

Another issue found during the review with @Dariquest :

cf update-route vcap.me --hostname testroute -o loadbalancing

will unexpectedly remove the loadbalancing option from the route. It should instead fail, as the value is missing.

command/v7/create_route_command.go Outdated Show resolved Hide resolved
command/v7/map_route_command.go Outdated Show resolved Hide resolved
command/v7/update_route_command.go Outdated Show resolved Hide resolved
command/v7/map_route_command.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants