xds: add HTTP filter is_optional support (gRFC A39) #4221
Conversation
|
This is rebased & ready for review |
xds/internal/client/xds.go
Outdated
| @@ -144,14 +144,31 @@ func unwrapHTTPFilterConfig(config *anypb.Any) (proto.Message, string, error) { | |||
| return s, s.GetTypeUrl(), nil | |||
| } | |||
|
|
|||
| func validateHTTPFilterConfig(cfg *anypb.Any, lds bool) (httpfilter.Filter, httpfilter.FilterConfig, error) { | |||
| func validateHTTPFilterConfig(cfg *anypb.Any, lds, optional, validateServer, validateClient bool) (httpfilter.Filter, httpfilter.FilterConfig, error) { | |||
There was a problem hiding this comment.
These boolean vars look weird to me.
It seems the checks can be done after this function returns. Do the type assertions there?
There was a problem hiding this comment.
Optional needs to be supported within the function, or we need to return a magic error so it can be treated specially by the caller, which I dislike (parse/unmarshal errors should not be ignored). I moved the server/client type assertion to the caller though.
xds/internal/client/xds.go
Outdated
| @@ -170,9 +187,23 @@ func processHTTPFilterOverrides(cfgs map[string]*anypb.Any) (map[string]httpfilt | |||
| } | |||
| m := make(map[string]httpfilter.FilterConfig) | |||
| for name, cfg := range cfgs { | |||
| _, config, err := validateHTTPFilterConfig(cfg, false) | |||
| optional := false | |||
| if typeURL := cfg.GetTypeUrl(); typeURL == "type.googleapis.com/envoy.config.route.v3.FilterConfig" { | |||
There was a problem hiding this comment.
Add a const for the string.
There was a problem hiding this comment.
Or, maybe use https://pkg.go.dev/github.com/golang/protobuf/ptypes#DynamicAny instead of manually check the type? We can do a type assertion on the message.
A similar change can be made for unwrapHTTPFilterConfig(), so you won't need the type string there.
There was a problem hiding this comment.
Or, maybe use pkg.go.dev/github.com/golang/protobuf/ptypes#DynamicAny instead of manually check the type? We can do a type assertion on the message.
I just discovered ptypes.Is; used that instead.
| Value: []byte{1, 2, 3}, | ||
| } | ||
|
|
||
| func wrappedOptionalFilter(name string) *anypb.Any { |
There was a problem hiding this comment.
Hmm, this function is defined in lds_test, but only used in rds_test..
There was a problem hiding this comment.
That's true but it seems to fit more in here with the other filter config stuff. I can move it if you feel strongly, LMK.
This PR contains all of #4206, with the is_optional support as a separate, following commit.