Skip to content

Conversation

@dapengzhang0
Copy link
Contributor

@dapengzhang0 dapengzhang0 commented Oct 19, 2021

Add RlsClusterSpecifierPlugin as per the design doc

The structure of ClusterSpecifierPlugin is very similar to io.grpc.xds.Filter.

The following changes to the existing code are made:

  • move ConfigOrError class out of Filter class to be shared with ClusterSpecifierPlugin
  • make io.grpc.rls.RlsProtoData public to be accessible by io.grpc.xds
  • treat empty defaultTarget in io.grpc.rls.RlsProtoData.RouteLookupConfig as null to support both json and proto config without defaultTarget field specified.

@dapengzhang0 dapengzhang0 requested a review from sergiitk October 19, 2021 22:54
Copy link
Member

@sergiitk sergiitk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some notes.

Copy link
Member

@sergiitk sergiitk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing my nits :)

@dapengzhang0 dapengzhang0 merged commit f30d07d into grpc:master Oct 27, 2021
@dapengzhang0 dapengzhang0 deleted the xds-rls branch October 27, 2021 16:07
@ejona86
Copy link
Member

ejona86 commented Oct 28, 2021

Why is this exposing RlsProtoData to xds? That is not what the design called for. The design seems to imply that the proto would be converted to json, but certainly it is json being passed to the existing LB policy parsing logic. Is that not feasible?

@dapengzhang0
Copy link
Contributor Author

ejona86: Why is this exposing RlsProtoData to xds? That is not what the design called for. The design seems to imply that the proto would be converted to json, but certainly it is json being passed to the existing LB policy parsing logic. Is that not feasible?

It is feasible to use json if the plugin TypedExtension is encoded with TypedStruct (typeUrl = "type.googleapis.com/envoy.config.route.v3.FilterConfig") instead of RlsProtoData, and the plugin implementation should avoid using RlsProtoData proto. But this should be called out in the design. cc @dfawley

@ejona86
Copy link
Member

ejona86 commented Oct 28, 2021

TypedStruct (typeUrl = "type.googleapis.com/envoy.config.route.v3.FilterConfig")

That would not be the correct type. It would be grpc.lookup.v1.RlsClusterSpecifier.

It is never encoded with RlsProtoData. That's our internal representation. Even if it is encoded as a proto, we can convert RLSClusterSpecifier to JSON using the protobuf tooling and use the existing JSON-based parsing API. No need to re-create the parsing logic.

@dapengzhang0
Copy link
Contributor Author

That would not be the correct type. It would be grpc.lookup.v1.RlsClusterSpecifier.

Sorry, I copy & paste in mistake. It should be (typUrl="type.googleapis.com/udpa.type.v1.TypedStruct") in the top level of TypedExtension. Then inside the TypedStruct, the inner level typeUrl is "grpc.lookup.v1.RlsClusterSpecifier"

@dfawley
Copy link
Member

dfawley commented Oct 28, 2021

this should be called out in the design

The design does note that the RLS cluster specifier plugin should emit, as part of the config, "routeLookupConfig": <JSON form of RLSClusterSpecifier.config>.

dapengzhang0 added a commit that referenced this pull request Nov 10, 2021
- Partially revert the change of RlsProtoData.java  in #8612  by removing `public` accessor
- Have grpc-xds no longer strongly depend on grpc-rls. The application will need grpc-rls as runtime dependencies if they need route lookup feature in xds.
- Parse RouteLookupServiceClusterSpecifierPlugin config to the Json/Map representation of `io.grpc.lookup.v1.RouteLookupClusterSpecifier` instead of `io.grpc.rls.RlsProtoData.RouteLookupConfig`
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants