search

grpc / grpc-go

The Go language implementation of gRPC. HTTP/2 based RPC
https://grpc.io
Apache License 2.0
21.1k stars 4.39k forks source link

xDS doesn't allow multiple http filters with same name #5159

Closed costinm closed 2 years ago

costinm commented 2 years ago

See internal/xdsclient/xdsresource/unmarshal_lds.go processHTTPFilters This prevents having 2 RBAC http filters, with different action ( action is set in RBAC object, and the filter has only one rbac object).

The code setting the RBAC filtering should also process multiple filters.

What version of gRPC are you using?

master, 1.43

What version of Go are you using (go version)?

any

What operating system (Linux, Windows, …) and version?

What did you do?

If possible, provide a recipe for reproducing the error.

What did you expect to see?

What did you see instead?

menghanl commented 2 years ago

This is working as intended I believe. The name is used as a unique ID to identify a httpfilter. Please see the spec: https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md#top-level-filter-configs

If you have two filters, with different actions, what's the reason to not assign them different names?

github-actions[bot] commented 2 years ago

This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

costinm commented 2 years ago

The spec is mostly correct - however Envoy doesn't seem to require the name to be unique, and is using the name to decide how to decode the value. We use "Any" for the proto - not TypedStructure, because struct is more verbose and expensive. And with Any - there is no TypeUrl.

Changing Istio to generate typed structs for gRPC is possible in theory - but since authz filters can be pretty large and most of the code base is using Any, I'm not sure it's better.

I'm not aware of any reason for name to be a 'unique ID' - it seems it works just fine with same name used in multiple filters - and without a different way to pass the type, not sure how it could be made work with Any.

My suggestion is to not expect the name is unique in gRPC.

costinm commented 2 years ago

Not sure how to reopen the issue, sorry I lost track of this.

@sanjaypujare

sanjaypujare commented 2 years ago

CC @ejona86 as the RBAC design owner and for feedback on the Java implementation.

I think @menghanl is talking about "The logical instance name also allows configuring multiple instances of the same filter, which can be useful for cases like authorization policies." which requires the name to be unique? But if Envoy allows it then the gRFC has a different interpretation?

BTW, I am unable to open the issue.

ejona86 commented 2 years ago

Envoy allows it, but they think the behavior is wrong. There's a note in gRFC A39 about it:

Note that Envoy currently keys these maps by legacy filter names instead of by filter instance names, but this is expected to change, as per https://github.com/envoyproxy/envoy/issues/12274. gRPC will directly implement the desired semantics here.

ejona86 commented 2 years ago

We use "Any" for the proto - not TypedStructure, because struct is more verbose and expensive. And with Any - there is no TypeUrl.

Changing Istio to generate typed structs for gRPC is possible in theory...

RBAC doesn't use Struct, and Any totally has a type url. You need to change the filter name, not the config type. It seems that should be a fairly trivial change.

I'm not aware of any reason for name to be a 'unique ID'

typed_per_filter_config uses the name to identify the filter instance to apply the config to. Before using the name, it was not possible to provide override configuration if the filter type was used twice.

costinm commented 2 years ago

I don't know why the type URL in Any is not used - the code has been around for a pretty long time, and we support a range of Envoy versions.

And I don't disagree that the 'correct' implementation is to have unique names, it is very reasonable - but implementations should be able to tolerate non-unique names at least for backward compat.

But in practical terms - any change in Istio will be available in 1.14 ( ~3 months from now) and make it to users in ~6 months. I will file a bug on Istio as well, so in time it'll be unique.

But it would be good for gRPC implementation to be more compatible with envoy and XDS server behaviors - so people can take advantage of this feature sooner. If gRPC plans to support other XDS servers - you may find similar situations, since Envoy currently doesn't validate or enforces uniqueness of the name.

costinm commented 2 years ago

https://github.com/istio/istio/issues/37748

costinm commented 2 years ago

I think I found a safe solution, I can generate the unique names only for proxyless ( and let Istio security people deal with envoy - we should also make them unique there, but don't know how safe it is). Also I can keep the 'allow' filter name unchanged, and only modify the newer 'deny'. I'll handle it on istio side.