Append CORS settings defined at Route level with Virtual Host level

[kevin-shelaga]

Version

None

Is your feature request related to a problem? Please describe.

Customer would like to define 1 set of exposeHeaders at the route level, and another set at the virtual host level and have them combined.

Describe the solution you'd like

append certain CORS at VH and others at route, have them concatenated.

Describe alternatives you've considered

route options overwrites, so just define the full list at the route level.

Additional Context

No response

┆Issue is synchronized with this Asana task by Unito

[DuncanDoyle]

Do we have more info about the actual use-case? When we say "append certain CORS at VH and others at route, have them concatenated.", what "certain CORS" are we talking about? allowOrigin? allowMethods? exposeHeaders? I'd like to understand what CORS configurations are set on the VirtualHostOptions level and which are set on the RouteOptions level.

There are a number of things that (might) make supporting this feature a bit trickier than expected:

1. We can't break the current API and its current behaviour, as other users might rely on the fact that the RouteOptions override the VirtualHostOptions. Changing the "override" strategy into a "merge" strategy could potentially break (customer) environments, or make them vulnerable to security threads (Cross Site Request Forgery, etc.).

2. The above means that we either should specify a strategy in the:

• the CRs of these resources
• Gloo Edge Settings
• ... somewhere else

3. Merge/override behaviour can be different for different fields:

• If a field does not exist in a lower level config (e.g. RouteOption), we should probably merge the value from a higher level config (e.g. VirtualHostOption) if the strategy is set to "merge"
• If a field is an array, we should probably merge the values of these 2 arrays when strategy is "merge".
• If a field is a singular value, and the it's defined in both the higher level and lower level config .... and the strategy is "merge" ... what should the semantics be? .... Merging is impossible here ..... so it should probably be an override ..... ???

4. When using delegation principles (i.e. platform team defines VirtualServices with VirtualHostOptions and application teams define the RouteTable with RouteOptions), how do we deal with the fact that the platform team might want to restrict the "merge/override" strategies for these configurations (in particular because there can be security implications)?

5. How and where do we expose the final configuration of merged policies, so users can clearly see the actual policy configuration that has been applied? We should not force users to look at the low-level Envoy configuration, so probably a "status" of some sort would be useful ...

[kevin-shelaga]

thank you @DuncanDoyle As i mentioned in my submission the customer wants to have a generic set of exposeHeaders at the VS level which platform engineers will enforce across all RTs. Then dev teams can choose to expose additional headers at the RT level which they control.

[nrjpoddar]

@sam-heilbron to look at this issue for scope/estimation.

cc @nfuden

[bewebi]

I've got an initial implementation in this branch, adding a field to Settings that toggles whether or not to merge CORS settings, but I think questions 3, 4, and 5 from @DuncanDoyle's post still need addressing

To the third point, the most future-proof mechanism would probably be to have it set granularly per-CORS setting This is likely overly verbose, but a helm value can always abstract that away to some extent If we don't go this route we will have to make decisions about what the one and only merge behavior will be For each the options could be:

• VH
• Route
  • This is what Envoy implements without our intervention
• Intersection (ie most restrictive)
• Union (ie most permissive)
  • This is the "append" behavior that's been requested for the list fields

To the fourth, would this be a concern if Settings was used? RBAC can be applied to that resource, but that could be too coarse, if different teams need access to this setting than would typically have access to the Settings CR

To the fifth, I agree that it would be ideal for the resulting config to be accessible to users without having to parse Envoy config, and that status (in this case VS status) might be a reasonable place for that Could we clarify whether this is strictly required as part of this issue? I expect it to be at least as complex to implement as the actual functionality, so it may be preferable to spin it off as a follow-up

[bewebi]

solo-io/solo-projects#6757 has been merged to address the customer requests and will be released in EE 1.18.0-beta1

The updated CORS guide includes instructions on using the new CorsPolicyMergeSettings API

We have provided the customer with a test image and testing instructions but haven't heard back, so this issue can be reopened if there is feedback that needs to be acted on