Simplify VirtualService API, possibly other APIs

[kdorosh]

Virtual Service API can be cleaned up:

• [ ] Rename feature-rc1 branch to master and master to v0.x.y; duplicate of https://github.com/solo-io/gloo/issues/1843
• [x] Remove the conversion job, rename all v2 stuff to v1
• [x] Delete EVERY deprecated field/message from our protos
  • [x] Undeprecate extensions and clearly document their new purpose (https://github.com/solo-io/gloo/issues/1205)
• [x] Address all TODO(kdorosh) and remove deprecated codepath support
  • [x] In Gloo
  • [x] In GlooE
• [x] ~Add an “enterprise” block to the plugin definition and allow configuring all enterprise plugins there (strongly typed).~ decided we'd rather all plugins live at the same level
  • [x] Deprecate/remove the their opaque configs under extensions
  • [x] Update glooctl writes to use new convention
    • [x] Rate limiting
    • [x] Ext auth
  • [x] Update docs to use new convention
    • [x] Ext auth
    • [x] Rate limiting
    • [x] JWT
    • [x] RBAC
    • [x] WAF
  • [x] ~Write cronjob to move old configs to new config~ we are supporting both for now
• [x] Flatten/simplify other virtual service configs where sensible

Ticket is to determine the difficulty of implementation and whether the changes will have an acceptable payoff in the long run.

Other ideas include moving the entire RoutePlugins config up one level: https://github.com/solo-io/gloo/blob/master/projects/gateway/api/v1/virtual_service.proto#L215

Open to concrete ideas/suggestions for other simplifications here.

Anything come to mind @rickducott @yuval-k @ilackarms @EItanya @mitchdraft ?

[ilackarms]

there are a number of breaking changes that i'm tracking:

• [x] de-duplication of prefixRewrite in route plugins
• [x] de-duplication of destinations in tcpGateway (for Gateway)
• [ ] eliminate single vs multiple destination type (from the route action); simply make destination an array of destinations; duplicate of https://github.com/solo-io/gloo/issues/1928
• [ ] ability to provide ResourceRef without providing a namespace - should default to the namespace of the referencing object
  • this behavior can be made forced/default for certain fields such as secret refs (increased security, @yuval-k ); duplicate of https://github.com/solo-io/gloo/issues/1929
• [x] support an array of matchers on a route. currently each route only allows one matcher, which has led to a lot of duplication in user code
• [ ] refactor the transformation plugin API, currently it exposes the filter config directly to the user; the filter config is fairly low-level; we should be able to refactor this to something more user friendly imo; duplicate of https://github.com/solo-io/gloo/issues/1930

there may be more i detect as i go through, but this is the list i have currently

[mitchdraft]

+1 to array of destinations. All sounds good to me. Two comments:

• ~I'm not sure if we want to give up the flexibility of the enterprise opaque types. Perhaps we could transfer all current use cases to strong-typing and keep the field as is in case there is a need for it in future GlooE features.~
  • UPDATE: after discussing with @kdorosh I don't think the field would add much future value and am in favor of removing
• Could we make these changes on a feature branch so we don't need to release multiple breaking changes? @EItanya recently enabled branch protection on feature-xyz branches

[rickducott]

• [x] I want to move away from the word "plugins" entirely in our API. Some reasons:
• It is an unnecessary layer of nesting
• It exposes an internal implementation detail without providing any clarity to the user
• It has confused us internally -- confusing a plugin that is part of the product from a plugin that is written and maintained by a customer
• It has confused people externally -- expecting the API can be extended with custom plugins without having to rebuild the server

[kdorosh]

Adding to the list here:

• [x] proposed simplifications to the rate-limiting API: https://github.com/solo-io/gloo/pull/1213
• [x] Simplify WAF API by using the Settings message for routes and vhosts instead of having their own messages (remove settings nesting and proto duplication)
  • [x] Also consider removing the CoreRuleSet message if proto supports optional oneofs
• [x] Same trick for RBAC, remove the Config message and flatten this up a level
  • [x] Combine with Vhost/Route extensions and remove their specific messages as well

[kdorosh]

after discussion with @ilackarms and @yuval-k , leaning towards a rename of plugins rather than a complete removal/flattening of the plugins API.

Flattening would be challenging, and possibly more trouble than it's worth because we'd have to flatten plugins to two different messages for VirtualHosts and Routes:

• Gloo VirtualHost: https://github.com/solo-io/gloo/blob/master/projects/gloo/api/v1/proxy.proto#L129-L154
• Gateway VirtualHost: https://github.com/solo-io/gloo/blob/master/projects/gateway/api/v1/virtual_service.proto#L149-L174
• Gloo Route: https://github.com/solo-io/gloo/blob/master/projects/gloo/api/v1/proxy.proto#L156-L185
• Gateway Route: https://github.com/solo-io/gloo/blob/master/projects/gateway/api/v1/virtual_service.proto#L306-L332

This means that every plugin we add in the future would have to be added and documented twice, and would lead to large amounts of API bloat. (proto3 any and writing our own marshal/unmarshal logic for gogoproto are even uglier alternatives)

The reason for the near-duplicate messages is to keep the Gloo/Gateway abstraction -- Gloo Routes (and therefore Gloo VirtualHosts with Gloo Routes) don't need to and shouldn't know about route delegation (the differing config on the Route messages).

A proper rename addresses all of @rickducott 's concerns (bar the unnecessary nesting, which I'm arguing here is necessary)

[rickducott]

What is the proposed name?

Just to be clear, this is what we have now:

plugins:
  hostRewrite: 
    hostRewrite: foo
  prefixRewrite:
    prefixRewrite: bar
  ...

And after the change, we will have:

newName: 
  hostRewrite: foo
  prefixRewrite: bar
  ...

Is that right?

[kdorosh]

Initial rename idea is trafficPolicy.

Yes, we would still be flattening the host/prefix rewrites.

[kdorosh]

Another recent @ilackarms idea is to delete the VirtualHost abstraction altogether from the Gateway API, looking into it more

edit: I'm a big fan of this idea -- as it flattens nesting while making it clearer that our VirtualService API translates (eventually) to Envoy's VirtualHost API

[ilackarms]

@kdorosh we can remove the virtualHost field from the Virtual Service entirely + move field in the VirtualHost up one level.

the current structure exists because the original virtualHost was imported directly from gloo

[ilackarms]

@rickducott @kdorosh personally i like options as a replacement for plugins. it communicates that these are optional fields which extend the basic functionality (route to a destination)

[ilackarms]

another api break worth considering:

• [x] gateways that have empty virtualService refs should only select vs in their own namespace. currently they select all vs in the cluster, which is problematic for multi-tenant and self-service config models

[marcogschmidt]

• [ ] Another resource that I strongly feel we should consider refactoring is the Destination message; duplicate of https://github.com/solo-io/gloo/issues/1931

message Destination {

    oneof destination_type {
        core.solo.io.ResourceRef upstream = 10;
        KubernetesServiceDestination kube = 11;
        ConsulServiceDestination consul = 12;
    }

    DestinationSpec destination_spec = 2;
    Subset subset = 3;
}

My concerns:

• From just looking at the API, it is not possible to understand which destination_types support DestinationSpec and Subsets
• Some DestinationSpecs intuitively feel like they should be destination_types (e.g. aws, azure)
• Subsets is a pretty low-level concept that I think we can expose in more intuitive ways (e.g. ConsulServiceDestinations allow you to target services based on tags and data centers; behind the scenes we use subsets, but the user does not have to be aware of that).

I think we can make this central part of our APIs much more user-friendly. Such a refactor will however require vast changes to most of Gloo's components, e.g. FDS, UDS, a good number of plugins, and all the Gloo translation logic.

[ilackarms]

• [x] empty matcher should default to match all instead of return err

[ilackarms]

• [x] when we do the breaking change we should default FDS to whitelist mode (less aggressive)

[kdorosh]

• [x] get rid of upstreamSpec, it looks weird nested under spec:

  spec:
  upstreamSpec:

[ilackarms]

linking

• [x] #1489 and
• [ ] #1484

as relevant (potentialy breaking) changes

[kdorosh]

• [ ] Move:

  // prefix for addressing envoy stats for the tcp proxy
  string stat_prefix = 3;

  In HttpListener and TcpListener into appropriate HttpConnectionManagerSettings message, or up higher because they apply to all listener types

duplicate of https://github.com/solo-io/gloo/issues/1932

[kdorosh]

I opened individual issues for each unfinished bullet point to track any remaining open work.

No need to track progress on this epic anymore now that Gloo 1.0 is out.