xds: server-side listener network filter validation #4312
Conversation
easwars
force-pushed
the
inbound_listener_validation
branch
from
1a21b20
to
097ef78
Compare
| * limitations under the License. | ||
| */ | ||
|
|
||
| package testutils |
There was a problem hiding this comment.
Can you move this to internal/testutils instead? Seems generically useful, not bound to xds.
| @@ -0,0 +1,36 @@ | |||
| /* | |||
| * | |||
| * Copyright 2020 gRPC authors. | |||
| // Server side filters are not currently supported, but this interface is | ||
| // defined for clarity. |
There was a problem hiding this comment.
Still mostly true, but removing this is fine. Just a note for future reference: I doubt we'll want to define server interceptors in iresolver since servers and resolvers have nothing to do with each other.
xds/internal/client/xds.go
Outdated
| func validateNetworkFilterChains(filterChains []*v3listenerpb.FilterChain) error { | ||
| for _, filterChain := range filterChains { | ||
| if filterChain == nil { | ||
| continue |
There was a problem hiding this comment.
Why would there be a nil message in a repeated FilterChain element? I'm not even sure this is possible using the proto APIs in any language.
There was a problem hiding this comment.
I didn't want to special case for the default_filter_chain at the call site. So, I decided to add this nil check here. If you see around line 272, I make the following single call:
if err := validateNetworkFilterChains(append(lis.GetFilterChains(), lis.GetDefaultFilterChain())); err != nil {
return nil, err
}Even if I take two arguments for this function: one a slice of filter_chains, and one a default_filter chain, i would have to do a nil check for the latter.
What do you think?
There was a problem hiding this comment.
In that case, how about appending only if it's non-nil?
chains := lis.GetFilterChains()
if def := lis.GetDefaultFilterChain(); def != nil {
chains = append(chains, def)
}
if err := validateNetworkFilterChains(chains); err != nil {
xds/internal/client/xds.go
Outdated
| for _, filter := range filterChain.GetFilters() { | ||
| name := filter.GetName() | ||
| if name == "" { | ||
| return errors.New("filter missing name field") |
There was a problem hiding this comment.
Can we improve this error message, and the ones below, at all? (e.g. there is a name field in the FilterChain this filter is within, although it seems optional.)
There was a problem hiding this comment.
Tried adding the filter_chain and the filter wherever appropriate. let me know if this looks better.
There was a problem hiding this comment.
For future PRs, please try to keep non-trivial cleanup changes (e.g. the
MarshalAnychange which results in quite a bit of diffs) separated from non-cleanup changes.
Sorry about that. I didn't want to get into it in this PR. But somehow got pulled into it as I was adding so many protos with anys.
xds/internal/client/xds.go
Outdated
| func validateNetworkFilterChains(filterChains []*v3listenerpb.FilterChain) error { | ||
| for _, filterChain := range filterChains { | ||
| if filterChain == nil { | ||
| continue |
There was a problem hiding this comment.
I didn't want to special case for the default_filter_chain at the call site. So, I decided to add this nil check here. If you see around line 272, I make the following single call:
if err := validateNetworkFilterChains(append(lis.GetFilterChains(), lis.GetDefaultFilterChain())); err != nil {
return nil, err
}Even if I take two arguments for this function: one a slice of filter_chains, and one a default_filter chain, i would have to do a nil check for the latter.
What do you think?
xds/internal/client/xds.go
Outdated
| for _, filter := range filterChain.GetFilters() { | ||
| name := filter.GetName() | ||
| if name == "" { | ||
| return errors.New("filter missing name field") |
There was a problem hiding this comment.
Tried adding the filter_chain and the filter wherever appropriate. let me know if this looks better.
| // Server side filters are not currently supported, but this interface is | ||
| // defined for clarity. |
| @@ -0,0 +1,36 @@ | |||
| /* | |||
| * | |||
| * Copyright 2020 gRPC authors. | |||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package testutils |
xds/internal/client/xds.go
Outdated
| func validateNetworkFilterChains(filterChains []*v3listenerpb.FilterChain) error { | ||
| for _, filterChain := range filterChains { | ||
| if filterChain == nil { | ||
| continue |
There was a problem hiding this comment.
In that case, how about appending only if it's non-nil?
chains := lis.GetFilterChains()
if def := lis.GetDefaultFilterChain(); def != nil {
chains = append(chains, def)
}
if err := validateNetworkFilterChains(chains); err != nil {
xds/internal/client/xds.go
Outdated
| return fmt.Errorf("filter chain {%+v} has unsupported network filter: %s", filterChain, tc.GetTypeUrl()) | ||
| } | ||
| hcm := &v3httppb.HttpConnectionManager{} | ||
| if err := ptypes.UnmarshalAny(tc, hcm); err != nil { | ||
| return fmt.Errorf("failed to unmarshal network filter: %v", err) | ||
| return fmt.Errorf("filter chain {%+v} failed unmarshaling of network filter: %v", filterChain, err) |
There was a problem hiding this comment.
Add the filter name to these two?
| "google.golang.org/protobuf/proto" | ||
| "google.golang.org/protobuf/types/known/anypb" | ||
|
|
||
| "google.golang.org/grpc/internal/testutils" |
There was a problem hiding this comment.
Can you reorganize these imports?
Usually we do:
Standard packages
<blank line>
Almost everything else
<blank line>
Protos and, sometimes, renamed imports
There was a problem hiding this comment.
Intellij sometimes messes with these imports way too much. Not sure if I have to change any of my settings.
Summary of changes:
validateNetworkFilterChains()function inxds/internal/client/xds.goprocessHTTPFilters()which validateshttp_filterson the client-sideRouterfilter#psm-security-server-side-filter-support