xds: httpfilter API changes to support filter state retention (A83) #8745
Conversation
easwars
added
Type: Feature
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8745 +/- ##
==========================================
- Coverage 83.38% 83.29% -0.09%
==========================================
Files 418 419 +1
Lines 32367 32595 +228
==========================================
+ Hits 26988 27149 +161
- Misses 4014 4051 +37
- Partials 1365 1395 +30
🚀 New features to boost your workflow:
|
| func (provider) IsClient() bool { return true } | ||
| func (provider) IsServer() bool { return false } |
There was a problem hiding this comment.
As discussed it would be nice if we could find a way to use the type system to avoid potential for disagreement between these functions and the filter that is produced's behavior, but this is OK for now.
There was a problem hiding this comment.
As I got rid of the 2-layer thing, I was able to bring back the optional interfaces like we had before.
| type Filter interface { | ||
| // TypeURLs are the proto message types supported by this filter. A filter | ||
| // will be registered by each of its supported message types. | ||
| // A FilterProvider is responsible for parsing a HTTP filter's configuration |
| // capable of working on a server. | ||
| IsServer() bool | ||
| // Build creates a new Filter instance with the given name. | ||
| Build(name string) Filter |
There was a problem hiding this comment.
What if we didn't do this whole 2-layer thing and just added "name" to the Build*Interceptor methods instead?
We already said the FilterProvider would need to track instances directly inside of itself.
The builder functions could return a close func().
It seems like we could still do everything we need to do. Existing filters would just ignore the new name parameter and return func(){}. The new ones would do all their internal accounting in one centralized place.
| // this case, the RPC will not be intercepted by this filter. | ||
| BuildServerInterceptor(config, override FilterConfig) (iresolver.ServerInterceptor, error) | ||
|
|
||
| // Close closes the filter, allowing it to perform any required cleanup. |
There was a problem hiding this comment.
As discussed, please TODO since close is never called.
There was a problem hiding this comment.
Based on the other changes, we no longer have this method.
| } | ||
| i, err := ib.BuildClientInterceptor(filter.Config, override) | ||
| filterInstance := filter.FilterProvider.Build(filter.Name) | ||
| i, err := filterInstance.BuildClientInterceptor(filter.Config, override) |
There was a problem hiding this comment.
As discussed, add TODO.
Note that with a close function being returned, it would have been more obvious that it wasn't getting called.
| } | ||
| si, err := sb.BuildServerInterceptor(filter.Config, override) | ||
| filterInstance := filter.FilterProvider.Build(filter.Name) | ||
| si, err := filterInstance.BuildServerInterceptor(filter.Config, override) |
| } | ||
|
|
||
| func init() { | ||
| httpfilter.Register(builder{}) |
There was a problem hiding this comment.
Sorry, but would you mind also reverting this renaming to make the diffs easier to consume?
| type Filter interface { | ||
| // TypeURLs are the proto message types supported by this filter. A filter | ||
| // will be registered by each of its supported message types. | ||
| type FilterProvider interface { |
There was a problem hiding this comment.
Should we revert the old name, too? A "Filter" was already effectively a provider (of interceptors), and it retains that property with these changes.
Basically this whole change becomes:
- Add a
nameandcancel functo the instance builders, to allowFilters to track instances.
? Does that actually satisfy all our goals? I really think it does, but I'm not 100% sure.
There was a problem hiding this comment.
Done.
Does that actually satisfy all our goals? I really think it does, but I'm not 100% sure
I think it does. For example, in the GCP Auth Filter (A83), the Filter would contain a cache of call creds, and a map from name to the interceptor. Whenever Build is called, it would consult the map and return an entry from the map if one exists, otherwise it would create a new one and pass it a ref to the cache.
|
But now that we've gotten rid of the two-level thing, I think there is a problem when there is more than one grpc channel in the process and filter names are the same across channels (which seems very possible), but the configs are different. Let's say we have channel-A with a "GCP Auth filter" with a config that has a cache size of 10, and channel-B with a "GCP Auth filter" with a config that has a cache size of 20. With the two-level approach:
With our flat approach though, this is not going to be possible:
I think we should go back to the two-level approach. Let me know if I'm missing something. |
I see. I thought the But then what is the lifecycle of a
When does the cached state get reused / not across interceptors? I thought we throw them away when the config changes? Or is it only certain parts of the config that determine that? |
We could have the xDS resolver and the xDS server do that instead. With the two-level approach, they would maintain a map from filter name to
And as long as the filter name does not change, the resolver and the server will continue using the same
Yeah, it depends on the config and the filter. For example, in the GCP Auth filter, the only configuration is the size of the cache of call credentials. If the size changes, we don't want to throw away the old cache, but retain as many entries from the old cache as possible with the new size. And in the ext_authz filter, let's say some allowed_headers or header_mutation_rules change, but the address of the ext_authz server does not, then we would want to continue to use the existing grpc channel to the ext_authz server instead of creating a new one. With the two-level approach, the |
OK so within a channel or server the lifecycle is (filter name + type)? So even if the config changes we keep using the same instance? And there will be like a map in the xds resolver and server with (name+type) as the key so it can do re-use? So yes it seems we need the two-level approach, then, to handle that extra dimension. What if we:
Then we keep the property of the type system telling us what a filter supports, instead of having redundant WDYT? |
Yes, that's correct. |
|
With your suggestions for renaming, the API would look something like this: // Instances of these are stored in the registry, keyed by the type_urls
type Filter interface {
TypeURLs() []string
ParseFilterConfig(proto.Message) (FilterConfig, error)
ParseFilterConfigOverride(proto.Message) (FilterConfig, error)
IsTerminal() bool
}
// A Filter can optionally implement this interface
type ClientFilterBuilder interface {
BuildClientFilter() ClientFilter
}
// A Filter can optionally implement this interface
type ServerFilterBuilder interface {
BuildServerFilter() ServerFilter
}
type ClientFilter interface {
BuildClientInterceptor(configs) (iresolver.ClientInterceptor, func(), error) // The returned cancel func to be invoked when the interceptor is no longer needed
}
type ServerFilter interface {
BuildServerInterceptor(configs) (iresolver.ServerInterceptor, func(), error) // The returned cancel func to be invoked when the interceptor is no longer needed
}I gave the methods in the EDIT: Also, I changed the |
|
With the above approach, the We also discussed adding some kind of generic reference counted |
|
@dfawley : Assigning back to you to make sure my understanding matches yours, before I start making the changes. Thanks. |
3 participants
How the API looked before this change?
Filter, that was responsible for functionality like config parsing and optionally for building client and server interceptors (or filter instances).Filters keyed by the supported type_urls.How the API looks after this change?
FilterProvider, that contains functionality for config parsing and other things like "what are the supported type_urls?", "Are filters produced by this provider supposed to be terminal?", "Are filters produced by this provider supposed to work on the client/server?" etc.FilterProviderwill also contain functionality to create filter instances represented by a new interface,Filter.Filterinterface contains functionality to build client and server interceptors. If this filter is not supported on either the client or the server, that functionality can be a no-op.Filterwill also contain a newClosemethod allowing it to free up any resources allocated.FilterProviderss keyed by the supported type_urls.Why is this change required?
How will this be used?
Filterinstances only when the filter names in the xDS resources change. Otherwise, they will only create new interceptors using the updated filter configuration from the existingFilter. This will allowFilterinstances to share state across interceptors and across state updates.The changes here and inspired by similar changes made for Java and described here: https://github.com/grpc/proposal/blob/master/A83-xds-gcp-authn-filter.md#java
RELEASE NOTES: None