xds: Delete check for router filter being last in config selector

[easwars]

gRFC A39 talks about gRPC's support for xDS HTTP filters. This gRFC was amended as part of PR 250. While we ended up implementing most of the changes mentioned in the amendment, we failed to implement the config time validation check to ensure that an LDS resource contains the router filter as the last filter.

HTTP filters received as part of an LDS resource are processed in this function:

grpc-go/xds/internal/xdsclient/xdsresource/unmarshal_lds.go

Line 195 in 417cf84

func processHTTPFilters(filters []*v3httppb.HttpFilter, server bool) ([]HTTPFilter, error) {

While we do validate that the filter chain is not empty and that the last filter in the chain is a terminal filter, we are not currently validating that the last filter is in-fact the router filter.

The router filter package does provide a utility function to check if a filter is the router filter:

grpc-go/xds/internal/httpfilter/router/router.go

Line 42 in 417cf84

func IsRouterFilter(b httpfilter.Filter) bool {

We can use that function here to ensure that the router filter is present in the list and that it is the last filter in the list.

[easwars]

@Aditya-Sood

[Aditya-Sood]

thanks @easwars, I'll get started on this

[Aditya-Sood]

hi @easwars
so I understand that we can additionally check for the terminal filter to also be a router filter as follows:

        if !ret[i].Filter.IsTerminal() {
                return nil, fmt.Errorf("http filter %q is not a terminal filter", ret[len(ret)-1].Name)
        }
+       if !router.IsRouterFilter(ret[len(ret)-1].Filter) {
+               return nil, fmt.Errorf("http filter %q is not a router filter", ret[len(ret)-1].Name)
+       }

however I'm still trying to figure out how to write the testcase for it

the IsRouterFilter() just checks whether the filter value passed can be asserted as a router.builder type (which in turn is an empty struct being used as a method receiver)

I found this testcase corresponding to the not a terminal filter test, trying to understand how to modify it for 'not a router filter` test

would you happen to have any pointers for me on how to do this?

[easwars]

If you scroll all the way to the bottom of unmarshal_lds_test.go, you will find a bunch of custom filter implementations used by the test such as clientOnlyHTTPFilter, serverOnlyHTTPFilter, errHTTPFilter etc. I would suggest adding another custom filter implementation here and call it something like terminalHTTPFilter. The implementation would be very similar to the type httpFilter (in the sense that it would embed both httpfilter.ClientInterceptorBuilder and httpfilter.ServerInterceptorBuilder), but the IsTerminal() method would return true.

And you would have to add a test case for both the client (TestUnmarshalListener_ClientSide) and server (TestUnmarshalListener_ServerSide) side to ensure that on both sides, if the terminal filter is not a router filter, we return the appropriate error.

Hope this helps.

[Aditya-Sood]

Thank you, this is really helpful
One quick query - do we need to create a terminalNonRouterHTTPFilter as well? (To cover the test case of a terminal filter failing the router.IsRouterFilter() method)

[easwars]

Thank you, this is really helpful One quick query - do we need to create a terminalNonRouterHTTPFilter as well? (To cover the test case of a terminal filter failing the router.IsRouterFilter() method)

I thought that was the only test case to add. What other cases were you planning to add?

[zasweq]

See this check: https://github.com/grpc/grpc-go/pull/4676/files#diff-9640c5b5baca36d84239eb2b3f001f169f596c89f84382eeba7abe8bbd4c5d9bR258, and unit tests here: https://github.com/grpc/grpc-go/pull/4676/files#diff-88532d359ba36ae99c09b45ae55f3788a9c528fea2472a6f1f969cbd166855f6R27

[easwars]

Is there an issue with checking that the last filter is in fact the router filter and not just any terminal filter?

[easwars]

Currently the config selector is checking if the last filter is the router filter, which doesn't seem like the correct place to have that check. If we at all should have that check, it should be in the xds client.

[zasweq]

I don't see an issue with an explicit check for the router filter. However, if what I mentioned earlier was correct (router is only terminal filter), router filter -> terminal filter, so the check does technically encapsulate "in face the router filter" and not "any terminal filter"

[Aditya-Sood]

hi @easwars
So initially I figured that there is a need for an additional terminalHTTPFilter because all the other filter types in the file (httpFilter, errHTTPFilter, serverOnlyHTTPFilter, clientOnlyHTTPFilter) were defined to return false for IsTerminal(); and so our new type would be the one to return true for it

However I was going through the actual tests which use those existing types and found that the filter chains in those tests in fact add a routerFilter explicitly as the last filter in the filter chain. Examples are as follows:

1. httpFilter - https://github.com/grpc/grpc-go/blob/master/xds/internal/xdsclient/xdsresource/unmarshal_lds_test.go#L359
2. errHTTPFilter - Surprisingly not used anywhere in the codebase except in the init() of unmarshal_lds_test.go - might have to create tests for it?
3. clientOnlyHTTPFilter - https://github.com/grpc/grpc-go/blob/master/xds/internal/xdsclient/xdsresource/unmarshal_lds_test.go#L470
4. serverOnlyHTTPFilter - https://github.com/grpc/grpc-go/blob/master/xds/internal/xdsclient/xdsresource/filter_chain_test.go#L1059

Now these Router type filters (i.e. router.builder type filters) already return true for IsTerminal() - link to router.builder type definition

Hence I think we could modify processHTTPFilters() to replace the IsTerminal() check with IsRouterFilter() check, because:
a. Router filters are Terminal filters by default (as they return true for IsTerminal() by definition)
b. All legal filter chains seem to use a Router filter as the last in the chain (based on my above observation of the existing test cases)

Then we just need to create one new NonRouterFilter HTTP filter type and test only for that

[easwars]

@Aditya-Sood : Sounds good.

[Aditya-Sood]

Raised #6281 for the Router filter check

Also regarding the errHTTPFilter:

errHTTPFilter - Surprisingly not used anywhere in the codebase except in the init() of unmarshal_lds_test.go - might have to create tests for it?

Shall I create test cases for this as well in a separate PR?

[zasweq]

As per discussion on #6281 and #6248, the xDS Client handling of this is correct and what the gRPC team designed. However, we have a trailing check in config selector that needs to be deleted from the deleted part in the gRFC. It is a currently a no-op, since emissions from the client will have router filter as last filter in chain, but it still is a cleanup that can be done.

[Aditya-Sood]

@zasweq could you confirm if this is the cleanup you are referring to be done as part of this PR?
#6248 (comment)

[Aditya-Sood]

hi @zasweq @easwars
I believe #6248 also fixes this issue, based on these changes?
forgot to mention this ticket in that PR

[zasweq]

Yeah, that is :). Closing this issue.