extproc: implement ClientFilter and ClientFilterBuilder interface#9086
extproc: implement ClientFilter and ClientFilterBuilder interface#9086eshitachandwani wants to merge 11 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9086 +/- ##
==========================================
+ Coverage 80.52% 83.01% +2.49%
==========================================
Files 413 417 +4
Lines 33543 33563 +20
==========================================
+ Hits 27012 27864 +852
+ Misses 4316 4273 -43
+ Partials 2215 1426 -789
🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Envoy external processing filter, providing configuration parsing logic and the infrastructure to build a client interceptor. Feedback addresses a critical uninitialized function variable that would cause a panic during filter construction and suggests a performance optimization for slice allocation when converting string matchers.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
eshitachandwani
assigned
easwars,
mbissa and
eshitachandwani and unassigned
easwars and
mbissa
|
As per offline discussion , changed override config to use the new optional type and changed the functions and tests accordingly. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a generic Option[T] type for optional values and establishes the foundational configuration and builder logic for the xDS external processing (extproc) HTTP filter. Feedback highlights critical concerns regarding resource management, specifically the creation of unmanaged grpc.ClientConn instances that could lead to leaks and performance issues. Additionally, the interceptor struct currently embeds an uninitialized interface, which would result in a nil pointer dereference panic if invoked.
| cc, err := createExtProcChannel(config.server) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("extproc: failed to create client: %v", err) | ||
| } | ||
| extClient := v3procservicepb.NewExternalProcessorClient(cc) |
There was a problem hiding this comment.
The current implementation creates a new grpc.ClientConn to the external processing server on every call to BuildClientInterceptor. This is problematic for several reasons:
- Resource Leak: The
iresolver.ClientInterceptorinterface does not have aClosemethod, and the gRPC-Go xDS framework does not currently provide a mechanism to close interceptors when they are replaced (e.g., during a configuration update). This means every connection created here will leak until the application exits. - Performance: Creating a
ClientConnis an expensive operation that involves starting name resolution and connection management goroutines. - Redundancy: Multiple routes or clusters using the same external processing server will each create their own connection instead of sharing one.
The clientFilter should instead manage a cache of ClientConn instances (e.g., a map[string]*grpc.ClientConn keyed by the server configuration) and reuse them. These connections should then be closed in the clientFilter.Close() method.
There was a problem hiding this comment.
Channel retention is in discussion and will be done later.
| } | ||
|
|
||
| type interceptor struct { | ||
| resolver.ClientInterceptor |
There was a problem hiding this comment.
The interceptor struct embeds the resolver.ClientInterceptor interface but does not initialize it. Since this is an interface, any attempt by the gRPC framework to call its methods (such as NewStream) on the returned interceptor will result in a nil pointer dereference panic.
While the PR description notes that this will be addressed in a later PR, it is highly recommended to provide a placeholder implementation of the interface methods that returns an error (e.g., using status.Error(codes.Unimplemented, ...)), or at least add a prominent TODO or comment in the code to warn that this struct is currently in a broken state if used.
There was a problem hiding this comment.
The next PR will do this.
|
@eshitachandwani |
3 participants
This PR is part of implementing A93: xds-ext-proc
NOTE : This PR has some coinciding changes with #9073 , they can be reviewed in that PR. The list of coinciding files:
This PR adds the builder that implements the ClientFilter and ClientFilterBuilder interface. That includes creating the interceptor config from the base and the override config. It also includes making the ext_proc channel.
THis PR has a placeholder function for converting grpcService to ServerConfig proto. The function will be implemented later when implementing gRFC A102.
The
interceptorstruct has aresolver.ClientInterceptorembedded for now, but that will be removed in a later PR when we implement theresolver.ClientInterceptorinterface. Since the filter will not be registered and no one is calling theresolver.ClientInterceptorfunction fro extproc filter , it will not panic.#ext-proc-a93
RELEASE NOTES: None