Skip to content

extproc: implement ClientFilter and ClientFilterBuilder interface #9086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eshitachandwani wants to merge 16 commits into
base: master
Choose a base branch
from
Open

extproc: implement ClientFilter and ClientFilterBuilder interface #9086

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0542270
BuildClientInterceptor
eshitachandwani Apr 22, 2026
d31b4c9
minor fixes
eshitachandwani Apr 22, 2026
d0d83d3
minor
eshitachandwani Apr 23, 2026
02e1edb
remove mode override and fix test
eshitachandwani Apr 26, 2026
f36a295
vet
eshitachandwani Apr 26, 2026
328deca
test fix
eshitachandwani Apr 26, 2026
3319dae
Update internal/xds/httpfilter/extproc/ext_proc.go
eshitachandwani Apr 26, 2026
7ada0ba
gemini review
eshitachandwani Apr 26, 2026
f3e7083
minor fixes
eshitachandwani Apr 27, 2026
87fb4c7
change the parameters to be complete interceptorCfg not proto
eshitachandwani Apr 30, 2026
76f6f0a
change to use override
eshitachandwani May 12, 2026
a1ebc59
Merge remote-tracking branch 'upstream/master' into build_client_inte…
eshitachandwani May 21, 2026
1dbeeed
Merge branch 'master' into build_client_interceptor
eshitachandwani May 21, 2026
5b914d3
merge upstream and resolve conflict
eshitachandwani May 21, 2026
00fb76b
address review comments
eshitachandwani May 22, 2026
1db23e1
add defer
eshitachandwani May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions internal/xds/httpfilter/extproc/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,28 @@ func processingModesFromProto(pm *v3procfilterpb.ProcessingMode) processingModes
responseTrailerMode: resolveHeaderMode(pm.GetResponseTrailerMode(), modeSkip),
}
}

// newInterceptorConfig creates the interceptor config from the base and
// override filter configs. If a field is set in both the base and override
// configs, the value from the override config will be used.
func newInterceptorConfig(base baseConfig, override overrideConfig) baseConfig {
ic := base
Comment thread
eshitachandwani marked this conversation as resolved.

// Apply overrides if present.
if val, ok := override.server.Value(); ok {
ic.server = val
}
if val, ok := override.failureModeAllow.Value(); ok {
ic.failureModeAllow = val
}
if override.requestAttributes != nil {
ic.requestAttributes = override.requestAttributes
}
if override.responseAttributes != nil {
ic.responseAttributes = override.responseAttributes
}
if val, ok := override.processingModes.Value(); ok {
ic.processingModes = val
}
return ic
}
274 changes: 274 additions & 0 deletions internal/xds/httpfilter/extproc/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,17 @@ import (
"regexp"
"strings"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal/grpctest"
"google.golang.org/grpc/internal/optional"
"google.golang.org/grpc/internal/xds/httpfilter"
"google.golang.org/grpc/internal/xds/matcher"
"google.golang.org/grpc/internal/xds/xdsclient/xdsresource"
"google.golang.org/grpc/metadata"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/testing/protocmp"
"google.golang.org/protobuf/types/known/anypb"
Expand All @@ -48,6 +53,15 @@ func Test(t *testing.T) {
grpctest.RunSubTests(t, s{})
}

const testBaseURI = "base-uri"

// incorrectFilterConfig embeds httpfilter.FilterConfig but is not of type
// baseConfig/overrideConfig, and is used to test incorrect config types being
// passed to BuildClientInterceptor.
type incorrectFilterConfig struct {
httpfilter.FilterConfig
}

// testParseGRPCServiceConfig is a helper function that parses a GrpcService
// proto message into a GRPCServiceConfig. This is a temporary test
// implementation that will be removed once gRFC A102 is implemented.
Expand Down Expand Up @@ -86,6 +100,9 @@ var cmpOpts = []cmp.Option{
}
return r.String()
}),
cmp.Comparer(func(x, y matcher.StringMatcher) bool {
return x.Equal(y)
}),
}

func (s) TestParseFilterConfig_Success(t *testing.T) {
Expand Down Expand Up @@ -515,3 +532,260 @@ func (s) TestParseFilterConfigOverride_Errors(t *testing.T) {
})
}
}

func (s) TestBuildClientInterceptor(t *testing.T) {
origCreateExtProcChannel := createExtProcChannel
defer func() { createExtProcChannel = origCreateExtProcChannel }()
Copy link
Copy Markdown
Contributor

@easwars easwars May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Member Author

@eshitachandwani eshitachandwani May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

createExtProcChannel = func(cfg xdsresource.GRPCServiceConfig) (*grpc.ClientConn, error) {
return grpc.NewClient(cfg.TargetURI, grpc.WithTransportCredentials(insecure.NewCredentials()))
}

b := builder{}
f := b.BuildClientFilter()
defer f.Close()

tests := []struct {
name string
cfg httpfilter.FilterConfig
override httpfilter.FilterConfig
wantConfig baseConfig
wantErr string
Copy link
Copy Markdown
Contributor

@easwars easwars May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@eshitachandwani eshitachandwani May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}{
{
name: "NilConfig",
cfg: nil,
wantErr: "extproc: incorrect config type provided",
},
{
name: "IncorrectConfigType",
cfg: incorrectFilterConfig{},
wantErr: "extproc: incorrect config type provided",
},
{
name: "IncorrectOverrideType",
cfg: baseConfig{},
override: incorrectFilterConfig{},
wantErr: "extproc: incorrect override config type provided",
},
{
name: "ConfigUsingOnlyBase",
cfg: baseConfig{
failureModeAllow: true,
requestAttributes: []string{"attr1"},
responseAttributes: []string{"attr2"},
observabilityMode: true,
disableImmediateResponse: true,
deferredCloseTimeout: 10 * time.Second,
processingModes: processingModes{
requestHeaderMode: modeSend,
responseHeaderMode: modeSkip,
responseTrailerMode: modeSend,
requestBodyMode: modeSend,
responseBodyMode: modeSkip,
},
server: xdsresource.GRPCServiceConfig{
TargetURI: testBaseURI,
ChannelCredentials: "test-channel-creds",
CallCredentials: "test-call-creds",
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
Timeout: 5 * time.Second,
},
mutationRules: httpfilter.HeaderMutationRules{
AllowExpr: regexp.MustCompile("allow-.*"),
DisallowExpr: regexp.MustCompile("disallow-.*"),
DisallowAll: true,
DisallowIsError: true,
},
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
},
wantConfig: baseConfig{
failureModeAllow: true,
requestAttributes: []string{"attr1"},
responseAttributes: []string{"attr2"},
mutationRules: httpfilter.HeaderMutationRules{
AllowExpr: regexp.MustCompile("allow-.*"),
DisallowExpr: regexp.MustCompile("disallow-.*"),
DisallowAll: true,
DisallowIsError: true,
},
observabilityMode: true,
disableImmediateResponse: true,
deferredCloseTimeout: 10 * time.Second,
processingModes: processingModes{
requestHeaderMode: modeSend,
responseHeaderMode: modeSkip,
responseTrailerMode: modeSend,
requestBodyMode: modeSend,
responseBodyMode: modeSkip,
},
server: xdsresource.GRPCServiceConfig{
TargetURI: testBaseURI,
ChannelCredentials: "test-channel-creds",
CallCredentials: "test-call-creds",
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
Timeout: 5 * time.Second,
},
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
},
},
{
name: "ConfigUsingBaseAndOverride",
cfg: baseConfig{
failureModeAllow: false,
requestAttributes: []string{"base-attr1"},
responseAttributes: []string{"base-attr2"},
observabilityMode: true,
disableImmediateResponse: true,
processingModes: processingModes{
requestHeaderMode: modeSend,
responseHeaderMode: modeSkip,
responseTrailerMode: modeSend,
requestBodyMode: modeSend,
responseBodyMode: modeSkip,
},
server: xdsresource.GRPCServiceConfig{
TargetURI: testBaseURI,
Timeout: time.Second,
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
},
mutationRules: httpfilter.HeaderMutationRules{
AllowExpr: regexp.MustCompile("allow-.*"),
DisallowExpr: regexp.MustCompile("disallow-.*"),
DisallowAll: true,
DisallowIsError: true,
},
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
disallowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("disallow-header", false)},
deferredCloseTimeout: 10 * time.Second,
},
override: overrideConfig{
failureModeAllow: optional.New(true),
requestAttributes: []string{"override-attr1"},
responseAttributes: []string{"override-attr2"},
processingModes: optional.New(processingModes{
requestHeaderMode: modeSkip,
responseHeaderMode: modeSend,
responseTrailerMode: modeSkip,
requestBodyMode: modeSkip,
responseBodyMode: modeSend,
}),
server: optional.New(xdsresource.GRPCServiceConfig{
TargetURI: "override-uri",
}),
},
wantConfig: baseConfig{
failureModeAllow: true,
requestAttributes: []string{"override-attr1"},
responseAttributes: []string{"override-attr2"},
mutationRules: httpfilter.HeaderMutationRules{
AllowExpr: regexp.MustCompile("allow-.*"),
DisallowExpr: regexp.MustCompile("disallow-.*"),
DisallowAll: true,
DisallowIsError: true,
},
observabilityMode: true,
disableImmediateResponse: true,
deferredCloseTimeout: 10 * time.Second,
processingModes: processingModes{
requestHeaderMode: modeSkip,
responseHeaderMode: modeSend,
responseTrailerMode: modeSkip,
requestBodyMode: modeSkip,
responseBodyMode: modeSend,
},
server: xdsresource.GRPCServiceConfig{
TargetURI: "override-uri",
},
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
disallowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("disallow-header", false)},
},
},
{
name: "ConfigUsingBaseAndPartialOverride",
cfg: baseConfig{
failureModeAllow: false,
requestAttributes: []string{"base-attr1"},
responseAttributes: []string{"base-attr2"},
observabilityMode: true,
disableImmediateResponse: true,
deferredCloseTimeout: 10 * time.Second,
processingModes: processingModes{
requestHeaderMode: modeSend,
responseHeaderMode: modeSkip,
responseTrailerMode: modeSend,
requestBodyMode: modeSend,
responseBodyMode: modeSkip,
},
server: xdsresource.GRPCServiceConfig{
TargetURI: testBaseURI,
Timeout: time.Second,
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
},
mutationRules: httpfilter.HeaderMutationRules{
AllowExpr: regexp.MustCompile("allow-.*"),
DisallowExpr: regexp.MustCompile("disallow-.*"),
DisallowAll: true,
DisallowIsError: true,
},
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
disallowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("disallow-header", false)},
},
override: overrideConfig{
failureModeAllow: optional.New(true),
},
wantConfig: baseConfig{
failureModeAllow: true,
requestAttributes: []string{"base-attr1"},
responseAttributes: []string{"base-attr2"},
mutationRules: httpfilter.HeaderMutationRules{
AllowExpr: regexp.MustCompile("allow-.*"),
DisallowExpr: regexp.MustCompile("disallow-.*"),
DisallowAll: true,
DisallowIsError: true,
},
observabilityMode: true,
disableImmediateResponse: true,
deferredCloseTimeout: 10 * time.Second,
processingModes: processingModes{
requestHeaderMode: modeSend,
responseHeaderMode: modeSkip,
responseTrailerMode: modeSend,
requestBodyMode: modeSend,
responseBodyMode: modeSkip,
},
server: xdsresource.GRPCServiceConfig{
TargetURI: testBaseURI,
Timeout: time.Second,
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
},
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
disallowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("disallow-header", false)},
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
intptr, err := f.BuildClientInterceptor(tc.cfg, tc.override)
if tc.wantErr == "" {
Copy link
Copy Markdown
Contributor

@easwars easwars May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@eshitachandwani eshitachandwani May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if err != nil {
t.Fatalf("BuildClientInterceptor() returned unexpected error: %v", err)
}
ic, ok := intptr.(*interceptor)
if !ok {
t.Fatalf("BuildClientInterceptor() returned %T, want *interceptor", intptr)
}
Copy link
Copy Markdown
Contributor

@easwars easwars May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@eshitachandwani eshitachandwani May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

if diff := cmp.Diff(ic.config, tc.wantConfig, cmpOpts...); diff != "" {
t.Fatalf("Interceptor config returned unexpected diff (-got +want):\n%s", diff)
}
intptr.Close()
return
}
if err == nil {
t.Fatalf("BuildClientInterceptor() returned nil error, want error containing %q", tc.wantErr)
}
if !strings.Contains(err.Error(), tc.wantErr) {
t.Fatalf("BuildClientInterceptor() error = %v, want error containing %q", err, tc.wantErr)
}
})
}
}
Loading
Loading