xds: add terminal http filter verification, remove lame route filter, add hcm as terminal network filter verification #8342
Conversation
| Set<String> names = new HashSet<>(); | ||
| for (io.envoyproxy.envoy.extensions.filters.network.http_connection_manager.v3.HttpFilter | ||
| httpFilter : proto.getHttpFiltersList()) { | ||
| NamedFilterConfig lastFilter = new NamedFilterConfig(null, null); |
There was a problem hiding this comment.
This initialization is not the right thing to do. It will produce literal "null" in the error message. The variable lastFilter seems not needed: for client, if the list is empty, throw an exception here; and validate the last filter inside the loop.
There was a problem hiding this comment.
The lastFilter check outside of the loop was intended to cover the empty list case. Because previous empty filter chains would end up a lame_filter later on, but we removed that part here.
But I think the way you described here is better - functionally the same but error message more friendly. Updated.
| } | ||
| StructOrError<FilterConfig> filterConfig = | ||
| parseHttpFilter(httpFilter, filterRegistry, isForClient); | ||
| if (i == proto.getHttpFiltersCount() - 1 && filterConfig != null) { |
There was a problem hiding this comment.
(For client) fail validation if(i == proto.getHttpFiltersCount() - 1 && filterConfig == null)
There was a problem hiding this comment.
Why is this validation client-only? For server-side today parseHttpFilter == false, but if http filters are enabled on server-side, then it seems this is required on server-side as well.
There was a problem hiding this comment.
There is nothing to validate on the terminal filter for server-side. Does the last filter in filter chain have to have a typed config for server-side?
There was a problem hiding this comment.
oh, probably yes.
Last time we talked about hcm being the last filter in filter-chain for the server.
Then then it sounded like the terminal filter was for the client. But actually routing config is both a client and server filter.
How do we know that TD is going to follow this?
Side note, I wonder if we will be able to do end to end test for the server side, i might missed something but it does not seem to work: currently parseHttpFilter == false , so filterConfig is null, but the server requires it to be non-null.
There was a problem hiding this comment.
It only allows on and only one filter in filter chain at server-side, it must be hcm typed config.
But http-filters inside hcm for server-side, it seems grpc/proposal#250 terminal filter change isn't limited to client.
There was a problem hiding this comment.
The Listener has an explicit FilterChain message along with FilterChainMatches for deciding which chain to use. That's why I assumed that's what you were talking about. I don't know how common it is to say "filter chain" applying to http filters. It doesn't seem wrong, but may need to be careful. I chose to interpret what you said in light of Ivy's interpretation, where she mentioned hcm which is a network filter. Both of us have been messing with network filters more than some other people, so we may be particularly prone to interpreting it that way.
There is nothing to validate on the terminal filter for server-side. Does the last filter in filter chain have to have a typed config for server-side?
To re-answer this in light of it being about http filters: both client-side and server-side behave the same for validation here (when RBAC flag is enabled, which enables http filters on server-side). Router is "expected" in both cases and RPCs will probably fail if Router isn't present. Any filters after Router are ignored. All of that is covered in gRFC A39.
There was a problem hiding this comment.
I don't know how common it is to say "filter chain" applying to http filters. It doesn't seem wrong, but may need to be careful.
Actually gRFC A39 is also using the term 'filter chain'.
both client-side and server-side behave the same for validation here (when RBAC flag is enabled, which enables http filters on server-side). Router is "expected" in both cases and RPCs will probably fail if Router isn't present.
If so, the gRFC A36 need be updated to reflect this. @markdroth
Any filters after Router are ignored.
The latest change in gRFC A39 enforces that the Router must be the last filter.
There was a problem hiding this comment.
If so, the gRFC A36 need be updated to reflect this.
Nah, that is the old behavior. I'm just not doing a good job keeping track of what the precise currently-expected behavior is. You linked to the gRFC change that changed from the behavior I mentioned (it introduced the concept of "terminal filter"), and that is correct.
The latest change in gRFC A39 enforces that the Router must be the last filter.
That's fine. That gRFC is authoritative.
There was a problem hiding this comment.
I misunderstood the purpose of this PR. I was skimming and saw your comment about doing validation specific for client-side only, and that sounded wrong. I wasn't really trying to get in the middle of "what is the client's validation behavior" and instead really only wanted to make sure the client vs server validation was not messed up.
There was a problem hiding this comment.
it looks there is no doubt that server side httpFilter should also follow terminal filter verification. updated.
… add hcm as terminal network filter verification (grpc#8342) * xds: add terminal filter verification, remove lame route filter * move last filter check inline * add server validate terminal filter
… add hcm as terminal network filter verification (grpc#8342) * xds: add terminal filter verification, remove lame route filter * move last filter check inline * add server validate terminal filter
3 participants
grpc/proposal#251
grpc/proposal#250
cc @ejona86