extproc: register filter and parse base and override config#9073
extproc: register filter and parse base and override config#9073eshitachandwani wants to merge 17 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9073 +/- ##
==========================================
+ Coverage 80.52% 83.11% +2.58%
==========================================
Files 413 417 +4
Lines 33543 33633 +90
==========================================
+ Hits 27012 27955 +943
+ Misses 4316 4253 -63
+ Partials 2215 1425 -790
🚀 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 implements the Envoy external processing (ext_proc) HTTP filter for xDS, including configuration parsing for base and override settings. The reviewer noted that the implementation incorrectly uses a non-existent GRPC body processing mode instead of STREAMED and lacks support for TypedStruct types in the configuration parser. As a result, the validation logic and unit tests require updates to correctly handle supported processing modes and additional configuration formats.
|
LGTM, barring the udpa.type.v1.TypedStruct and xds.type.v3.TypedStruct support comment from gemini. Can go ahead if we get an answer on those. |
It has been discussed that we only have to support |
|
As per offline discussion , added a new optional type implementation in the experimental package and using that for the override config. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a generic Option[T] type and implements the configuration parsing logic for the xDS external processing filter, including structures for base and override configurations. Critical feedback highlights potential runtime panics due to missing nil checks on protobuf message types and the ExtProcPerRoute override field. Other suggestions address misleading error prefixes in shared utilities, unnecessary whitespace, and various typos in comments and error messages.
| * | ||
| */ | ||
|
|
||
| // Package optional adds generic optional types. |
There was a problem hiding this comment.
Nit: Package optional implements a generic optional type.
|
|
||
| // Package optional adds generic optional types. | ||
| // | ||
| // All APIs in this package are experimental. |
There was a problem hiding this comment.
Actually, on second thoughts, this could be in internal. There is no need for us to provide an optional type that is exposed to users.
| // All APIs in this package are experimental. | ||
| package optional | ||
|
|
||
| // Option represents an optional value of type T. |
There was a problem hiding this comment.
Hmm .... I think this shuold also be called Optional, because the term Option is a highly overloaded term in Go. It usually represents some type that wraps options to be passed to another type.
There was a problem hiding this comment.
Changed. But then it will be used as optional.Optional that seems like a repetition to me and that is why I had used Option.
| isSet bool | ||
| } | ||
|
|
||
| // New creates a new Option that does not have a value set. This can also be |
There was a problem hiding this comment.
How about starting off with something like:
type Optional[T any] struct {
val T
set bool
}
func New[T any]() Optional[T] {
return Optional[T]{}
}
func NewValue[T any](v T) Optional[T] {
return Optional[T]{val: v, set: true}
}
func (o *Optional[T]) Get() (T, bool) {
return o.val, o.set
}
func (o *Optional[T]) Set(v T) {
o.val = v
o.set = true
}
Notice the pointer receiver in the above methods (although we dont need a pointer reciever for Get, I decided to go with it since there is a style guide recommendation to make all methods have the same receiver type where possible).
Get/Set seem to be much clearer than Value and WithValue. The latter is even more confusion since we also have a constructor NewValue.
Also, I don't see a point in Clear? We can add more methods as and when there is a requirement. Let's start off with the bare minimum to begin with.
There was a problem hiding this comment.
I also had something similar in mind when I started, but the main reason I changed it is, I thought we should have the function on same type as what we return from NewValue or New which is not a pointer. Otherwise users will have to get the pointer to the value and then call Get and Set. And I think we have precedence for having immutable functions. But compiler automatically handles that , so changed.
Also the reason fro not using Get is becuase the stye guide suggests so, but I agree that Get/Set is way cleaner. Changed.
Also removed the Clear function.
| // ChannelCredentials specifies the transport credentials to use to connect to | ||
| // the external server. Must not be nil. | ||
| ChannelCredentials json.RawMessage | ||
| // CallCredentials specifies the per-RPC credentials to use when making calls | ||
| // to the external server. | ||
| CallCredentials []json.RawMessage |
There was a problem hiding this comment.
These fields are still not comparable. We need it to be a string. Byte slices are not comparable. Please add a test to ensure that you can store this field in a map.
There was a problem hiding this comment.
Changed to string. But ServerConfig as a whole is still not comparable and cannot be used as a key because :
- metadata is a map which is not comparable
- We do not want to compare the timeout and metadata as they are set per RPC , not per channel.
I think we should move these fields out ofServerConfigand into theInterceptorConfig. WDYT ?
| func (s) TestParseFilterConfig(t *testing.T) { | ||
| origServerConfigFromGrpcService := serverConfigFromGrpcService | ||
| defer func() { serverConfigFromGrpcService = origServerConfigFromGrpcService }() | ||
| jsonBytes, _ := json.Marshal(insecure.NewCredentials()) |
There was a problem hiding this comment.
What does this even return? Please take a look at implementation of credentials in xds/bootstrap to understand how they work. They were specified in some gRFC (i can't recollect now).
There was a problem hiding this comment.
We no longer have the json type, so changed. I have overwritten this function to test that it correctly propogates errors and serverConfig incase of no error. We will remove this once A102 is implemented and test the actual implementation.
| wantErr: "extproc: invalid response body mode STREAMED", | ||
| }, | ||
| { | ||
| name: "ErrInvalidAllowExpression", |
There was a problem hiding this comment.
Please consider adding exhaustive tests for parsing the mutation rules in the httpfilter package instead of here.
In here, just add one case for a failing mutation rule and every other case can use a passing mutation rule.
| if tt.wantErr != "" && !strings.Contains(err.Error(), tt.wantErr) { | ||
| t.Fatalf("ParseFilterConfig() returned error = %v, wantErr %v", err, tt.wantErr) | ||
| } | ||
| if tt.wantErr == "" { | ||
| if err != nil { | ||
| t.Fatalf("ParseFilterConfig() returned unexpected error: %v", err) | ||
| } | ||
| if diff := cmp.Diff(got, tt.wantCfg, cmp.AllowUnexported(baseConfig{}, interceptorConfig{}, httpfilter.ServerConfig{}, processingModes{}, httpfilter.HeaderMutationRules{}), protocmp.Transform(), cmp.Transformer("RegexpToString", func(r *regexp.Regexp) string { | ||
| if r == nil { | ||
| return "" | ||
| } | ||
| return r.String() | ||
| })); diff != "" { | ||
| t.Fatalf("ParseFilterConfig() returned unexpected config (-got +want):\n%s", diff) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
I would still split these into two separate tests: one for success cases and one for failure cases. The test logic is reasonably complicated otherwise.
| got, err := b.ParseFilterConfigOverride(tt.override) | ||
| if tt.wantErr != "" && !strings.Contains(err.Error(), tt.wantErr) { | ||
| t.Fatalf("ParseFilterConfigOverride() returned error = %v, wantErr %v", err, tt.wantErr) | ||
| } | ||
| if tt.wantErr == "" { | ||
| if err != nil { | ||
| t.Fatalf("ParseFilterConfigOverride() returned unexpected error: %v", err) | ||
| } | ||
| if diff := cmp.Diff(got, tt.wantOverrideCfg, cmp.AllowUnexported(overrideConfig{}, interceptorConfig{}, httpfilter.ServerConfig{}, processingModes{}, httpfilter.HeaderMutationRules{}, interceptorOverrideConfig{}, optional.Option[httpfilter.ServerConfig]{}, optional.Option[processingModes]{}, optional.Option[bool]{}), protocmp.Transform(), cmp.Transformer("RegexpToString", func(r *regexp.Regexp) string { | ||
| if r == nil { | ||
| return "" | ||
| } | ||
| return r.String() | ||
| })); diff != "" { | ||
| t.Fatalf("ParseFilterConfigOverride() returned unexpected config (-got +want):\n%s", diff) | ||
| } | ||
| } | ||
| }) |
3 participants
This PR is part of implementing A93: xds-ext-proc
This PR adds the ext_proc filter and the builder that implements the builder interface. That includes parsing and validating the base and the override config. The registration of the filter is under the
GRPC_EXPERIMENTAL_XDS_EXT_PROC_ON_CLIENTflag.#ext-proc-a93
RELEASE NOTES: None