Implement per-route options in Cloud Controller

[maxmoehl]

Issue

RFC0027 Generic Per-Route Features got accepted but is not yet implemented in the cloud controller.

Context

See the RFC for details.

Expected result

• Cloud Controller accepts the new per-route options at the relevant routes API endpoints.
• Cloud Controller accepts the new per-route options in the manifest.
• Cloud Controller forwards the per-route options to BBS.

API Changes

In the RFC there was a discussion about the semantics of the API for this feature, please read it before commenting on that topic. Based on that discussion and some offline consulting with @stephanme I propose we go for the following behaviour:

• POST /v3/routes will accept the optional options field and add them to the route. For each option only the valid values are acceptable, null is not.
• GET /v3/routes(/:guid)? and GET /v3/apps/:guid/routes return the options field if at least one option is set. Unset options are not returned, if no option is set the options field is not returned at all.
• PATCH /v3/routes/:guid in addition to the valid values also accepts null for each option or the options field as a whole to unset either individual values or the entire set of options. Providing some options will merge them with the existing stored options, with the provided ones overwriting any stored ones. If the options field is not provided in the request the options will not be modified by this operation.
• POST /v3/spaces/:guid/actions/apply_manifest is only capable of setting options to a value, existing options which are not specified in the manifest remain untouched. This implies that there is no way to unset a per-route option via the manifest.

ping @tcdowney @beyhan @stephanme who took part in the discussion and @Gerg who was also mentioned.

Please raise your comments in this issue, we plan to start working on this soon! If you prefer, we can also join the CAPI office hours to discuss this topic.

[stephanme]

My major requirement is that the API changes are done in a compatible way. I.e. the new options parameter for POST /v3/routes shall be optional.

For the route object, there are three variants what to return if no options are set:

• always return an empty options hash (I prefer this variant but it may confuse clients that don't ignore extra fields that the client doesn't yet understand)
• return options set to null
• return a route object without options field (sort of implicit null value, is probably the most compatible solution)

For PATCH /v3/routes/:guid, the options parameter must be optional but if specified, the update semantics need to be specified:

• merge specified options like done for labels/annotations, see Updating labels and annotations
• overwrite semantics, i.e. the caller must specify the complete options object and it is set as a whole

I don't have a strong opinion here but tend to the merge semantics: it is used before in the API and it may make it easier for clients to set/unset individual route options without changing others.

[maxmoehl]

My major requirement is that the API changes are done in a compatible way. I.e. the new options parameter for POST /v3/routes shall be optional.

Yes, of course! I've updated the description.

For the route object, there are three variants what to return if no options are set:

• always return an empty options hash (I prefer this variant but it may confuse clients that don't ignore extra fields that the client doesn't yet understand)
• return options set to null
• return a route object without options field (sort of implicit null value, is probably the most compatible solution)

Since you mention the last option is probably the most compatible one I've included that one for now, feel free to object if you think another option should be preferred.

For PATCH /v3/routes/:guid, the options parameter must be optional but if specified, the update semantics need to be specified:

• merge specified options like done for labels/annotations, see Updating labels and annotations
• overwrite semantics, i.e. the caller must specify the complete options object and it is set as a whole

I don't have a strong opinion here but tend to the merge semantics: it is used before in the API and it may make it easier for clients to set/unset individual route options without changing others.

I've updated the issue to specify the merge semantics.

[hoffmaen]

For PATCH /v3/routes/:guid, the options parameter must be optional but if specified, the update semantics need to be specified:

• merge specified options like done for labels/annotations, see Updating labels and annotations
• overwrite semantics, i.e. the caller must specify the complete options object and it is set as a whole

I don't have a strong opinion here but tend to the merge semantics: it is used before in the API and it may make it easier for clients to set/unset individual route options without changing others.

@stephanme @maxmoehl
Regarding the update semantics: With the merge strategy, how would we allow to remove a previously set load-balancing algorithm?
With 'options': {}, it would not do anything, so do we have another option than allowing empty values for lb_algo to unset a previously set load-balancing algorithm?

[maxmoehl]

• PATCH /v3/routes/:guid in addition to the valid values also accepts null for each option or the options field as a whole to unset either individual values or the entire set of options.

This should cover the use-case if I understand you correctly.

[Gerg]

Overall, any fields that use the JSON serializer could be good prior art for this: https://github.com/cloudfoundry/cloud_controller_ng/blob/3e1d2f423c5b5d5b959a49e09db6a0e2680ebfdd/lib/cloud_controller/serializer.rb#L5

From a quick survey, that looks like:

• task env vars
• service binding creds
• service binding volume_mounts
• revision env vars
• droplet process types
• droplet side cars
• app env vars

However, I suspect most of those are not user-updatable.

[Gerg]

GET /v3/routes(/:guid)? and GET /v3/apps/:guid/routes return the options field if at least one option is set. Unset options are not returned, if no option is set the options field is not returned at all

I agree with Stephan regarding "return the options field if at least one option is set": we should always return the options field, and it can be an empty object ({}), if no options are specified.

POST /v3/spaces/:guid/actions/apply_manifest is only capable of setting options to a value, existing options which are not specified in the manifest remain untouched. This implies that there is no way to unset a per-route option via the manifest.

This can be addressed in @tcdowney and my forthcoming manifest RFC.

[Gerg]

We don't seem to have too many cases of object fields that allow updates. I poked around some, and found some examples:

App Lifecycle

One potentially interesting case of partial update of object fields is app lifecycle:

❯ cf curl /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a | jq .lifecycle
{
  "type": "buildpack",
  "data": {
    "buildpacks": [
      "java_buildpack"
    ],
    "stack": "cflinuxfs4"
  }
}

❯ cf curl /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a -X PATCH -d '{"lifecycle": {"data": {"buildpacks": ["ruby_buildpack"]}}}'

❯ cf curl /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a | jq .lifecycle
{
  "type": "buildpack",
  "data": {
    "buildpacks": [
      "ruby_buildpack"
    ],
    "stack": "cflinuxfs4"
  }
}

Quotas

I didn't try this myself, but the quota API docs say:

This endpoint will only update the parameters specified in the request body. Any unspecified parameters will retain their existing values.

Process Health Checks

This also appears to deep merge:

❯ cf curl /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a/processes/web | jq .health_check
{
  "type": "http",
  "data": {
    "timeout": null,
    "invocation_timeout": 100,
    "interval": null,
    "endpoint": "/"
  }
}

❯ cf curl -X PATCH /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a/processes/web -d '{"health_check": {"data": {"endpoint": "/foo"}}}'

❯ cf curl /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a/processes/web | jq .health_check
{
  "type": "http",
  "data": {
    "timeout": null,
    "invocation_timeout": 100,
    "interval": null,
    "endpoint": "/foo"
  }
}

And is nullable with an explicit null:

❯ cf curl -X PATCH /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a/processes/web -d '{"health_check": {"data": {"invocation_timeout": null}}}'

❯ cf curl /v3/apps/878ac323-40b8-44c8-9e60-529a058e231a/processes/web | jq .health_check
{
  "type": "http",
  "data": {
    "timeout": null,
    "invocation_timeout": null,
    "interval": null,
    "endpoint": "/foo"
  }
}

---

There are probably some additional cases, but the above should be a good corpus to start from.

[Gerg]

Another option to avoid this whole thing is to move these options to a Pseudo-Resource, nested under a route. This would be similar to app env vars, which are not on the app resource itself.

For example, something like:

PATCH /v3/routes/:guid/options
{
  "my-option": "my-value"
}

I think the main issue with this would be that the options would not be present during initial route creation.

[hoffmaen]

I agree with Stephan regarding "return the options field if at least one option is set": we should always return the options field, and it can be an empty object ({}), if no options are specified.

Hi @Gerg , thanks for your comments. This one is contradicting though: Stefan and Max voted for option 3 (no options -> return a route object without options field). This is what we have implemented. You propose to always return options. I agree with @maxmoehl and @stephanme here - we should keep it as compatible as possible, so anyone who's not using the options field will not see any changes in the API fields.

Other than that, the options hash and any of its fields are nullable when explicitely setting fields to null. Any other changes are additive.

[Gerg]

I must have misread @stephanme. I was referencing:

always return an empty options hash (I prefer this variant but it may confuse clients that don't ignore extra fields that the client doesn't yet understand)

A few points:

1. We make additive changes to the API all the time, so clients need to be able to handle unexpected fields.
2. Even if the options field is omitted when empty, it will still be present in cases where options are set, which would then break the additional-field-adverse client.
3. To the contrary, having fields sometimes present and sometimes omitted can break clients, if the clients are actively reading those keys. I think Golang especially struggles with optional keys when parsing JSON, but it could happen in other languages as well. For example: A client author designing their client around a route that has options set, which could then break when encountering a route with no options set. Typically, client authors can use API versioning to determine what fields to parse, but that's not possible if the field is dynamic resource-to-resource.
4. I don't think we have any examples on the v3 API where we omit a field if it is empty. The existing pattern is to return an empty value.

cc @cloudfoundry/wg-app-runtime-interfaces-capi-approvers for additional perspectives.

[peanball]

Hi, better late than never I would like to consider unifying the naming of options used in the API, Gorouter and the CF CLI.

Specifically, for lb_algo we use loadbalancing-algorithm in the manifest.

Showing a long string like this in the UI (CLI) is quite inconvenient as it uses up a lot of space.

The "algorithm" part of the property could also be considered superfluous, as "load balancing: round-robin" is equally telling for how requests will be handled.

My proposal (from the cf-cli perspective) is thus to shorten it to lb or loadbalancing. Either would work, and lb might be a bit too terse.

From the existing changes, only Gorouter and this PR would need adjustment, as the other components are just passing through the properties without further inspection.

See also cloudfoundry/cli#3314 for more details on how the cli might look like and show these options.

[hoffmaen]

Hi @peanball , thanks for your valuable input!

Your suggestion to unify the identifier for the load-balancing algorithm absolutely makes sense. I am neither a fan of lb_algo, nor loadbalancing-algorithm. It's confusing and unnecessary complicated to have these, instead of the short but meaningful proposed term loadbalancing. I'll adopt this idea in cloudfoundry/cli#3314.