http conn manager: Support Route Mutability

[mengmichael1]

Title: StreamFilter: Support Route Mutability

Description:
At Lyft we have a Http::StreamDecoderFilter that goes through rewriting the Host: header, clearing the route cache, and having the HTTP connection manager reevaluate the route selection -- just to update the routeEntry clusterName of a route (a Router::RouteConstSharedPtr). There is no support for route mutability.

Proposing two changes to enable a filter to directly mutate properties of a route, rather than having to go through the roundabout process of rewriting headers & clearing the route cache.

Proposal:

1. Add a new setRoute(const Router::RouteConstSharedPtr route) method to the StreamFilter API (in StreamFilterCallbacks) that sets the current request’s route as the passed-in RouteConstSharedPtr argument.
2. Add a new class called DelegatingRoute, which implements its base/parent class of Router:Route by delegating all method calls to the baseRoute that it wraps around. DelegatingRoute will take in a baseRoute (a RouteConstSharedPtr) in its constructor, and calls to Route class methods such as directResponseEntry and routeEntry will simply get delegated to the baseRoute.

Rationale for 2:
As part of enabling route mutability, the goal is to build a mechanism that enables developers to easily take an existing route, and create a new route that mutates specific properties (In our case, routeEntry, but another example can be tracingConfig ) while preserving all the rest of the properties/behavior of the existing route. We introduce the concept of a DelegatingRoute as this mechanism. An alternative approach, deep copying an existing route, is prohibitively expensive.

Usage:
Usage would be a filter calling setRoute(r) where r is a derived/child class of DelegatingRoute. In addition, DelegatingRoute will be useful in unit testing (will be used to test Lyft’s new filter modifying clusterName) for any changes related to modifying a route.

[mattklein123]

Note I already discussed this with the Lyft team internally. +1 from me. This has been asked for previously, is easy to implement, and will allow filters to fully customize route selection easily. cc @alyssawilk @snowp for thoughts.

[snowp]

In general very much in favor of this, just a few immediate thoughts:

I imagine this would not survive a clearRouteCache? Documenting this behavior would probably be sufficient, though I can imagine this causing some confusion whenever someone makes use of a filter that modifies the route, followed by another filter that modifies headers + clears the cache. This is similar to the limitations of Router::RouteConstSharedPtr route(const Router::RouteCallback& cb), so I think this is fine.

Would you also be adding a delegating RouteEntry? I would imagine you want to only override parts of the route entry.

I think another option would be to expose mutating functions on the Route/RouteEntry, allowing for it to be modified directly instead of having to decorate it with a wrapper object to override values. That said, the wrapper allows for more flexibility without code changes (we avoid changes for each thing on the route we want to change), so it seems like a good direction.

Overall I think this is good, this would have been very helpful for me in the past, where I've jumped through similar hoops to update the final route.

[alyssawilk]

Definitely a fan of 1)
For 2) is the plan that every stream default to the delegate, and by default it gets the route's tracing config and route entry, but the same way you can setRoute you can setTracingConfig to override bits? I think this is fine, though I wonder about the pros and cons of doing this programatically, vs a route re-architecture where we refactor the monolithic entry into something with named components so having a clone isn't prohibitively expensive. Either way that's a much larger change and I think 2) is a fine workaround and possibly has some long term benefits that the rearchitecting wouldn't have.

[mengmichael1]

Thanks for the feedback everyone!

@mattklein123 Saw you added the "help wanted" tag, to clarify I'll be taking on this implementation work :)

@snowp Correct, this wouldn't survive a clearRouteCache in subsequent filters. But like you said, should be fine since Router::RouteConstSharedPtr route(const Router::RouteCallback& cb) has the same limitations.

Yes, the plan is to also add a DelegatingRouteEntry that wraps around an existing Router::RouteEntry*. And I overall like the idea of using a wrapper object to preserve the existing immutable characteristic of Route/RouteEntry (unsure how much rearchitecting would be needed for mutating functions).

@alyssawilk Yes that is exactly the plan for (2)! For now, my plan was just to enable overriding the route entry (as Snow pointed out, I'll be adding a DelegatingRouteEntry). As part of this change, do you think I should also include set methods for the rest of the Route class attributes? (e.g. setTracingConfig, setDecorator, setDirectResponseEntry, etc). I'm thinking I could leave it to others to add in the future if they find this delegating mechanism useful?

[mengmichael1]

@mattklein123 @snowp With #15266 merged, can we close this issue now?

[mattklein123]

Thank you!