xds: add support for HTTP filters (gRFC A39)

[dfawley]

This is a fairly large PR, so apologies for that up-front.

I would recommend reviewing in this order:

1. httpfilter.go and config_selector.go - main interface definitions
2. xds/internal/client - parsing/validation of xds data
3. xds/internal/resolver - usage of the parsed HTTP filter config
4. leftovers: router plugin (a fancy nop), stream.go/transport code to signal interceptors when RPC is completed (done with connection; needed for fault injection functionality - coming soon)

[menghanl]

Reviewed 7 of 18 files at r1.
Reviewable status: 7 of 18 files reviewed, 10 unresolved discussions (waiting on @dfawley and @menghanl)

---

stream.go, line 192 at r1 (raw file):

		if rpcConfig.Interceptor != nil {
			rpcInfo.Context = nil
			newCtx, cs, err := rpcConfig.Interceptor.NewStream(ctx, rpcInfo, iresolver.NOPClientStream{})

Why is this using a NOP stream?

I think we should try to use the real stream, even though it's not necessary now.
Otherwise, the API seems incomplete.

If time is the concern here, just pass nil then?

---

internal/resolver/config_selector.go, line 64 at r1 (raw file):

	// ClientStream after done is called, since the interceptor is invoked by
	// application-layer operations.
	Done()

Done(DoneInfo)?

---

internal/resolver/config_selector.go, line 76 at r1 (raw file):

// ClientInterceptor is an interceptor for gRPC client streams.
type ClientInterceptor interface {

What's the reason to not just use the old interceptors?
Dependency problem?
Or because of the Done function?

---

internal/resolver/config_selector.go, line 80 at r1 (raw file):

	// should not be used during NewStream.  RPCInfo.Context should not be used
	// (will be nil).
	NewStream(context.Context, RPCInfo, ClientStream) (context.Context, ClientStream, error)

If ClientInterceptor is picked from ConfigSelector, it should already have RPCInfo, right? So we don't need it again?

---

internal/resolver/config_selector.go, line 84 at r1 (raw file):

// ServerInterceptor is unimplementable; do not use.
type ServerInterceptor interface {

Are there plans to support this in the near future?

---

xds/internal/httpfilter/httpfilter.go, line 39 at r1 (raw file):

// client side or server side or both, respectively.
type Filter interface {
	// TypeURLs are the registered proto message types supported by this filter.

Add something like This filter will be registered for each of the returned URLs.

---

xds/internal/httpfilter/httpfilter.go, line 42 at r1 (raw file):

	TypeURLs() []string
	// ParseFilterConfig parses the provided configuration proto.Message.  This
	// may be an anypb.Any or a udpa.type.v1.TypedStruct for filters that do

What type to accept should be a detail of the implementation, not the interface, right?

---

xds/internal/httpfilter/httpfilter.go, line 46 at r1 (raw file):

	// passed to Build.
	ParseFilterConfig(proto.Message) (FilterConfig, error)
	// ParseFilterConfigOverrid parses the provided override configuration

Document how this is different from ParseFilterConfig()?
Is this basically the same method, but not as strict?

---

xds/internal/httpfilter/httpfilter.go, line 89 at r1 (raw file):

// an init() function), and is not thread-safe. If multiple filters are
// registered with the same type URL, the one registered last will take effect.
func Register(b Filter) {

What if the Filter doesn't implement Client or Server?

---

xds/internal/httpfilter/router/router.go, line 42 at r1 (raw file):

// IsRouterFilter returns true iff a HTTP filter is a Router filter.
func IsRouterFilter(b httpfilter.Filter) bool {

Is "router" a category of "filter"?
What if b embeds a *builder?

And will we still need this if ParseFilterConfig actually does parsing, and return error if config is invalid? Can we use that instead?

[dfawley]

Why is this using a NOP stream?

I think we should try to use the real stream, even though it's not necessary now.
Otherwise, the API seems incomplete.

If time is the concern here, just pass nil then?

Yes, the API is incomplete. nil doesn't work, since we need an underlying Done defined, to be called by the interceptor's Done method. We could do nil checks in all interceptors instead, but this way is nicer.

Done(DoneInfo)?

This is an internal-only API, so we don't need to future-proof it.

What's the reason to not just use the old interceptors?
Dependency problem?
Or because of the Done function?

Three reasons:

1. It's the wrong domain. These functions are not invoked by the API layer, so they don't want things like CallOptions.
2. Circular dependency problem.
3. Done.

If ClientInterceptor is picked from ConfigSelector, it should already have RPCInfo, right? So we don't need it again?

Including it here makes interceptors reusable. I.e. we can build/cache one interceptor for each route, and not need to reconstruct. I'm not doing that in this PR, but that's why I passed it here.

...ServerInterceptor...
Are there plans to support this in the near future?

It's not too far off. @easwars will need to implement these eventually before we release server-side support.

Add something like This filter will be registered for each of the returned URLs.

Done

What type to accept should be a detail of the implementation, not the interface, right?

You would think so. But this is actually an important part of the API. We never call Parse with anything besides a *anypb.Any or a *udpa.type.v1.TypedStruct.

Document how this is different from ParseFilterConfig()?
Is this basically the same method, but not as strict?

The only difference is the source of the configuration passed to it. "Strictness" depends upon the implementation. Updated.

What if the Filter doesn't implement Client or Server?

Then Doom On You! In reality, it means it won't be recognized by either side. It can parse configs, but can't function. So we will fail validation of this filter and NACK if it's in LDS (since we check client/server capability depending on the listener), but will pass validation in RDS (since we don't know whether it's intended for the client or server).

Is "router" a category of "filter"?

router is a filter.

What if b embeds a *builder?

Who would do that? Remember, this is all internal.

The goal here is to recognize the router filter when we find it in the list of filters. The router filter is special because it is (currently?) implemented by a ton of hard-coded logic in the xds resolver. If it's absent, we will fail RPCs. If it's present, we ignore any filters after it.

And will we still need this if ParseFilterConfig actually does parsing, and return error if config is invalid? Can we use that instead?

ParseFilterConfig doesn't do parsing today because we don't support any of the configuration for the router filter. When we need to support that configuration, there's a couple options:

1. Implement the router filter like any other filter (but maybe with some kind of special operations it can perform since it needs to actually do the routing step?).
2. Export the configuration type so the xds resolver can read that configuration and act upon it.

[menghanl]

Reviewed 6 of 18 files at r1, 1 of 1 files at r2.
Reviewable status: 13 of 18 files reviewed, 12 unresolved discussions (waiting on @dfawley and @menghanl)

---

xds/internal/client/lds_test.go, line 419 at r2 (raw file):

			if ((err != nil) != test.wantErr) ||
				!cmp.Equal(update, test.wantUpdate, cmpopts.EquateEmpty(),
					cmp.Transformer("any", func(a *anypb.Any) string {

This handles all proto messages: https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp#Transform

---

xds/internal/client/xds.go, line 191 at r2 (raw file):

		name := filter.GetName()
		if name == "" {
			return nil, errors.New("filter missing name field")

Also print the filter?

[menghanl]

Reviewed 5 of 18 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @dfawley)

---

xds/internal/resolver/serviceconfig.go, line 193 at r2 (raw file):

			// Ignore any filters after the router filter.  The router itself
			// is currently a nop.
			break

Error here, instead of having a magic fail filter at the end of the list

---

xds/internal/resolver/serviceconfig.go, line 276 at r2 (raw file):

			}
			cs.clusters[cluster] = ci
			cs.routes[i].clusterHFCO[cluster] = wc.HTTPFilterConfigOverride

Add this override to clusters, instead of a separate map?

---

xds/internal/resolver/serviceconfig.go, line 329 at r2 (raw file):

}

func (il *interceptorList) Done() {

What's this method for?
Interceptor doesn't need a Done.

[dfawley]

Error here, instead of having a magic fail filter at the end of the list

Done

Add this override to clusters, instead of a separate map?

Good idea. Done.

What's this method for? Interceptor doesn't need a Done.

Good question. Done.

[menghanl]

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dfawley)

---

xds/internal/client/lds_test.go, line 419 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

This handles all proto messages: https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp#Transform

How about this? You don't need to add a transformer for each new proto type.

[dfawley]

How about this?

Done