extproc: implement ClientFilter and ClientFilterBuilder interface by eshitachandwani · Pull Request #9086 · grpc/grpc-go

eshitachandwani · 2026-04-22T13:15:42Z

This PR is part of implementing A93: xds-ext-proc

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 channel The function will be implemented later when implementing gRFC A102.

The interceptor struct has a resolver.ClientInterceptor embedded for now, but that will be removed in a later PR when we implement the resolver.ClientInterceptor interface. Since the filter will not be registered and no one is calling the resolver.ClientInterceptor function fro extproc filter , it will not panic.

#ext-proc-a93

RELEASE NOTES: None

codecov · 2026-04-22T13:19:32Z

Codecov Report

❌ Patch coverage is 90.47619% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.19%. Comparing base (037c6ef) to head (1db23e1).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/xds/httpfilter/extproc/ext_proc.go	86.20%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9086      +/-   ##
==========================================
- Coverage   83.20%   83.19%   -0.02%     
==========================================
  Files         417      417              
  Lines       33648    33698      +50     
==========================================
+ Hits        27998    28035      +37     
- Misses       4235     4244       +9     
- Partials     1415     1419       +4

Files with missing lines	Coverage Δ
internal/xds/httpfilter/extproc/config.go	`78.94% <100.00%> (+10.94%)`	⬆️
internal/xds/httpfilter/extproc/ext_proc.go	`76.92% <86.20%> (+4.70%)`	⬆️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

eshitachandwani · 2026-04-26T13:48:18Z

/gemini review

gemini-code-assist

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 · 2026-05-12T10:16:45Z

As per offline discussion , changed override config to use the new optional type and changed the functions and tests accordingly.

eshitachandwani · 2026-05-12T10:16:56Z

/gemini review

gemini-code-assist

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.

easwars · 2026-05-12T22:25:28Z

@eshitachandwani
There is a whole lot of overlap with 9073, in which I had a bunch of comments. So, moving this back to you until the former is merged.

…rceptor

eshitachandwani · 2026-05-21T04:59:41Z

/gemini review

gemini-code-assist

Code Review

This pull request implements the BuildClientFilter and BuildClientInterceptor methods for the extproc HTTP filter, along with logic to merge base and override configurations. The review identified several critical issues: embedding the resolver.ClientInterceptor interface without implementing its methods will cause a nil pointer dereference panic when invoked; creating a new grpc.ClientConn for every interceptor is inefficient and risks resource exhaustion; and the configuration merging logic performs a shallow copy of slices, which could lead to unintended side effects if the shared underlying arrays are modified.

easwars · 2026-05-22T05:23:08Z

+	createExtProcChannel = func(xdsresource.GRPCServiceConfig) (*grpc.ClientConn, error) {
+		return nil, fmt.Errorf("dialing external processing server not implemented")
+	}


Nit: Please consider having this method return a (grpc.ClientConnInterface, func(), error) instead of (*grpc.ClientConn, error). We only care about the methods in the grpc.ClientConnInterface and Close. The latter will be satisfied by the second return value func(). The reason for making the return type be the interface is that it allows the implementer to get fancy and return a wrapped ClientConn if required.

Done. Changed it to return (grpc.ClientConnInterface, func() error, error) . I changed it to func() error since clientconn.Close is a function that returns error.

easwars · 2026-05-22T05:25:30Z

+	if err != nil {
+		return nil, fmt.Errorf("extproc: failed to create client: %v", err)
+	}
+	extClient := v3procservicepb.NewExternalProcessorClient(cc)


Why do we have to create the stub here and pass it as a separate field when we are anyways passing the cc?

I am not sure what you mean. We need the stub to create a new RPC every time NewStream is called. We actually so not need clientconn if we have the cancel function. So changed it to have the cancel function and the v3procservicepb.ExternalProcessorClient stub.

easwars · 2026-05-22T05:26:00Z

+	resolver.ClientInterceptor
+	config    baseConfig
+	extClient v3procservicepb.ExternalProcessorClient
+	cc        *grpc.ClientConn


Same here. Make this to be of the interface type and store the cancel func as well.

If we have the cancel function and the v3procservicepb.ExternalProcessorClient stub , we do not need the interface. Changed it to such.

easwars · 2026-05-22T05:27:44Z

+
+func (s) TestBuildClientInterceptor(t *testing.T) {
+	origCreateExtProcChannel := createExtProcChannel
+	defer func() { createExtProcChannel = origCreateExtProcChannel }()


Nit/Optional: Personally I find it more readable when it is:

origFoo := foo foo := overriddenFoo defer func() { foo = origFoo} ()

easwars · 2026-05-22T05:29:19Z

+		cfg        httpfilter.FilterConfig
+		override   httpfilter.FilterConfig
+		wantConfig baseConfig
+		wantErr    string


I don't see much value in wantErr being a string here. Why not just a bool?

I just always thought comparing the error strings is a better test but since its the same error message every time , I guess it does not add value here. But I have added a new failure case for when channel creation fails , so retaining the wantErr string here. Let me know if we should change it?

easwars · 2026-05-22T05:31:06Z

+				if !ok {
+					t.Fatalf("BuildClientInterceptor() returned %T, want *interceptor", intptr)
+				}


Is this ever possible? Why not just do ic := intptr.(*interceptor)? And if there is a bug in the code that ends up causing a different interceptor type to be returned, the test will panic, which is fine.

easwars · 2026-05-22T05:31:56Z

+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			intptr, err := f.BuildClientInterceptor(tc.cfg, tc.override)
+			if tc.wantErr == "" {


I think this test logic is still reasonably complicated for a table driven test. Please consider splitting into success and failure cases.

easwars · 2026-05-22T05:32:28Z

+	}, nil
+}
+
+type interceptor struct {


Please call this clientInterceptor. We will have a serverInterceptor soonish.

eshitachandwani added 2 commits April 22, 2026 16:49

BuildClientInterceptor

0542270

minor fixes

d31b4c9

eshitachandwani added this to the 1.82 Release milestone Apr 22, 2026

eshitachandwani added the Type: Feature New features or improvements in behavior label Apr 22, 2026

eshitachandwani added 4 commits April 23, 2026 18:45

minor

d0d83d3

remove mode override and fix test

02e1edb

vet

f36a295

test fix

328deca

gemini-code-assist Bot reviewed Apr 26, 2026

View reviewed changes

Comment thread internal/xds/httpfilter/extproc/ext_proc.go Outdated

Comment thread internal/xds/httpfilter/extproc/config.go Outdated

eshitachandwani and others added 3 commits April 26, 2026 22:33

Update internal/xds/httpfilter/extproc/ext_proc.go

3319dae

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

gemini review

7ada0ba

minor fixes

f3e7083

eshitachandwani requested review from easwars and mbissa April 27, 2026 07:20

eshitachandwani assigned easwars, mbissa and eshitachandwani and unassigned easwars and mbissa Apr 27, 2026

change the parameters to be complete interceptorCfg not proto

87fb4c7

eshitachandwani assigned easwars and mbissa and unassigned eshitachandwani Apr 30, 2026

change to use override

76f6f0a

gemini-code-assist Bot reviewed May 12, 2026

View reviewed changes

Comment thread internal/xds/httpfilter/extproc/ext_proc.go Outdated

Comment thread internal/xds/httpfilter/extproc/ext_proc.go

easwars assigned eshitachandwani and unassigned easwars and mbissa May 12, 2026

eshitachandwani and others added 3 commits May 21, 2026 09:34

Merge remote-tracking branch 'upstream/master' into build_client_inte…

a1ebc59

…rceptor

Merge branch 'master' into build_client_interceptor

1dbeeed

merge upstream and resolve conflict

5b914d3

gemini-code-assist Bot reviewed May 21, 2026

View reviewed changes

Comment thread internal/xds/httpfilter/extproc/ext_proc.go

Comment thread internal/xds/httpfilter/extproc/ext_proc.go Outdated

Comment thread internal/xds/httpfilter/extproc/config.go

eshitachandwani assigned easwars and mbissa and unassigned eshitachandwani May 21, 2026

easwars reviewed May 22, 2026

View reviewed changes

easwars assigned eshitachandwani and unassigned easwars May 22, 2026

eshitachandwani added 2 commits May 22, 2026 12:40

address review comments

00fb76b

add defer

1db23e1

eshitachandwani requested a review from easwars May 22, 2026 08:57

eshitachandwani assigned easwars and unassigned eshitachandwani May 22, 2026

Conversation

eshitachandwani commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eshitachandwani commented Apr 26, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

eshitachandwani commented May 12, 2026

Uh oh!

eshitachandwani commented May 12, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

easwars commented May 12, 2026

Uh oh!

eshitachandwani commented May 21, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eshitachandwani May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eshitachandwani commented Apr 22, 2026 •

edited

Loading

codecov Bot commented Apr 22, 2026 •

edited

Loading

eshitachandwani May 22, 2026 •

edited

Loading