Revisiting filterchain dependency

[auni53]

Related issues: #151, #3559, #3286.
Original design from Randy

A couple years ago, @curiouserrandy led a bunch of discussion around dependency in the filterchain. Key issues included the ability to express a filter-to-filter dependency, express a filter-to-filterState() dependency, and for filters to lazily evaluate information based on their dependents. Randy wrote a design doc that was generally agreed upon. Of the four actionables, two were completed -- adding filterState(), and allowing the router to parse arbitrary variables out of filterState(). Randy designed a DependencyManager API, but this work was deferred (until now).

I'm seeking feedback from the community & @envoyproxy/senior-maintainers to get started on this. Reviewing old discussions and the original design doc, it seems like the world hasn't changed that much, so I'm hoping to pick up the thread and get going on this. As far as our project's purposes go, the original document over-emphasizes the importance of lazy evaluation optimization (we haven't actually needed this), and under-emphasizes filter-to-filter dependency (managing multiple filter chain environments is starting to get very messy). As well, I would suggest that we make the API around filterState() keys more strict -- I'm concerned that setting/getting depends on nothing more than a simple string identifier. The DNS format is designed to prevent collisions, but I think there should be more protection around string constants as filterstate keys, perhaps through static registration of each key. As well, this part of the API was also difficult for me to implement and validate without reading the underlying router code.

Lastly, I have some suggestions to improve upon the current design:

• Instead of having filters call a dependencyManager() to express their requirements, I would prefer that this is handled at the listener config level. Rather than expressing dependency through args to providesData(direction, name), we should introduce a Dependency proto with the same fields. This would be easier to extend upon going forward, but it would also make dependencies for a given a easier to understand at-a-glance, assuming that protos are easier to read/write than cpp code.

• There does not appear to be a need for runtime flexibility around whether filters offer a piece of data or not, i.e. a filter may or may not offer datum X. I think a lot of this complexity was around the uncertainty of what custom header variables the router might access or not. We could implement something like FilterState::KeyChain, and ask users to statically register what keys are possible. This way, filterchain initialization knows what keys it may need and therefore what filters are necessary.

• I would like to reduce the extent to which the router filter is special; rather than special logic around custom headers, the router should simply express a dependency on either filterState() key X or the filter that sets data into X. A user should configure the router to depend on the filter in the chain that offers corresponding filterState() data. Alternatively, this dependency could be inferred from the suggested FilterState::KeyChain construct.

What are your thoughts? And I'm still learning this part of the codebase so excuse me for any imprecision, happy to clarify any of the ideas here.

[snowp]

The DNS format is designed to prevent collisions, but I think there should be more protection around string constants as filterstate keys, perhaps through static registration of each key.

How would a registration mechanism do better than just a string key here? Would it enforce that only one registered handle could ever exist for a given name, and each consuming filter would have to reference that handle? Trying to see how this would boil down to more than just a C++ wrapper around a string lookup

Instead of having filters call a dependencyManager() to express their requirements, I would prefer that this is handled at the listener config level.

Without knowing the work done here that well, this seems a bit odd to me: presumably users configuring their listener chains shouldn't be required to understand the low level cross-filter interactions?

I would like to reduce the extent to which the router filter is special

Could you elaborate on this? It's not clear to me what part of the router you'd replace with FilterState interactions.

[auni53]

Hi Snow, thanks for the feedback!

How would a registration mechanism do better than just a string key here? Would it enforce that only one registered handle could ever exist for a given name, and each consuming filter would have to reference that handle? Trying to see how this would boil down to more than just a C++ wrapper around a string lookup

Given filterState key "com.auni", we have code that has to set `%PER_REQUEST_STATE(com.auni)% in config. I'm worried about maintenance of this going forward, and we kept them in sync with a constant in the filter that our config references. I think this is a little messy, I would rather the constants be registered and managed within Envoy. This is basically a cpp wrapper around string lookup, yes.

Without knowing the work done here that well, this seems a bit odd to me: presumably users configuring their listener chains shouldn't be required to understand the low level cross-filter interactions?

Hm, I see your point here, and I think the dependency mechanism should be totally optional. Right now there's no enforcement on low level cross-filter interactions, we just fail if, e.g., the router filter isn't at the end. Perhaps there should be a default dependency configuration for each filter which users can modify, though this wouldn't really cover "router filter must be at the end". In our case, we introduce several filters that depend on one another, and also expose custom header variable data that the router will use. Currently, we rely on inconsistent CHECKs and conditionals in our filters with inconsistent dependency-missing handling. Instead, users should be able to specify this dependency beforehand and see a failure on FilterChain instantiation or config validation.

Could you elaborate on this? It's not clear to me what part of the router you'd replace with FilterState interactions.

What I mean is, we should reduce the extent to which dependency management has special logic to handle the fact that the router depends on filters providing specific filterState strings. This might actually be a tradeoff to consider though: in one possibility, the user must configure the router to specify what filters must precede it or what filterState() data must be provided; on the other hand, this information could be inferred through the KeyChain idea (the router requires the presence of everything in the KeyChain). I would like this FilterState()-to-router-filter logic for custom headers to be handled within the router filter or its config if possible, rather than at the FilterFactory level.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.