grpc
/
grpc-java
mirror of https://github.com/grpc/grpc-java.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

xds: fix parsing RouteLookupClusterSpecifier mistake (#8641) 

- 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`
This commit is contained in:
ZHANG Dapeng 2021-11-10 11:27:42 -08:00 committed by GitHub
parent b3579db574
commit ad0971ef5f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 109 additions and 147 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
rls/src/main/java/io/grpc/rls/RlsProtoData.java
View File

@ -25,7 +25,6 @@ import com.google.common.base.MoreObjects;
import com.google.common.base.Objects; import com.google.common.base.Objects;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMap;
import io.grpc.Internal;
import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name;
import java.util.HashSet; import java.util.HashSet;
import java.util.List; import java.util.List;
@ -36,8 +35,7 @@ import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable; import javax.annotation.concurrent.Immutable;
/** RlsProtoData is a collection of internal representation of RouteLookupService proto messages. */ /** RlsProtoData is a collection of internal representation of RouteLookupService proto messages. */
@Internal final class RlsProtoData {
public final class RlsProtoData {
private RlsProtoData() {} private RlsProtoData() {}
@ -142,7 +140,7 @@ public final class RlsProtoData {
/** A config object for gRPC RouteLookupService. */ /** A config object for gRPC RouteLookupService. */
@Immutable @Immutable
public static final class RouteLookupConfig { static final class RouteLookupConfig {
private static final long MAX_AGE_MILLIS = TimeUnit.MINUTES.toMillis(5); private static final long MAX_AGE_MILLIS = TimeUnit.MINUTES.toMillis(5);
private static final long MAX_CACHE_SIZE = 5 * 1024 * 1024; private static final long MAX_CACHE_SIZE = 5 * 1024 * 1024;
@ -164,8 +162,7 @@ public final class RlsProtoData {
@Nullable @Nullable
private final String defaultTarget; private final String defaultTarget;
/** Constructor. */ RouteLookupConfig(
public RouteLookupConfig(
List<GrpcKeyBuilder> grpcKeyBuilders, List<GrpcKeyBuilder> grpcKeyBuilders,
String lookupService, String lookupService,
long lookupServiceTimeoutInMillis, long lookupServiceTimeoutInMillis,
@ -336,7 +333,7 @@ public final class RlsProtoData {
* is true, one of the specified names must be present for the keybuilder to match. * is true, one of the specified names must be present for the keybuilder to match.
*/ */
@Immutable @Immutable
public static final class NameMatcher { static final class NameMatcher {
private final String key; private final String key;
@ -344,8 +341,7 @@ public final class RlsProtoData {
private final boolean optional; private final boolean optional;
/** Constructor. */ NameMatcher(String key, List<String> names, @Nullable Boolean optional) {
public NameMatcher(String key, List<String> names, @Nullable Boolean optional) {
this.key = checkNotNull(key, "key"); this.key = checkNotNull(key, "key");
this.names = ImmutableList.copyOf(checkNotNull(names, "names")); this.names = ImmutableList.copyOf(checkNotNull(names, "names"));
this.optional = optional != null ? optional : true; this.optional = optional != null ? optional : true;
@ -398,7 +394,7 @@ public final class RlsProtoData {
} }
/** GrpcKeyBuilder is a configuration to construct headers consumed by route lookup service. */ /** GrpcKeyBuilder is a configuration to construct headers consumed by route lookup service. */
public static final class GrpcKeyBuilder { static final class GrpcKeyBuilder {
private final ImmutableList<Name> names; private final ImmutableList<Name> names;
@ -407,7 +403,7 @@ public final class RlsProtoData {
private final ImmutableMap<String, String> constantKeys; private final ImmutableMap<String, String> constantKeys;
/** Constructor. All args should be nonnull. Headers should head unique keys. */ /** Constructor. All args should be nonnull. Headers should head unique keys. */
public GrpcKeyBuilder( GrpcKeyBuilder(
List<Name> names, List<NameMatcher> headers, ExtraKeys extraKeys, List<Name> names, List<NameMatcher> headers, ExtraKeys extraKeys,
Map<String, String> constantKeys) { Map<String, String> constantKeys) {
checkState(names != null && !names.isEmpty(), "names cannot be empty"); checkState(names != null && !names.isEmpty(), "names cannot be empty");
@ -486,7 +482,7 @@ public final class RlsProtoData {
* required and includes the proto package name. The method name may be omitted, in which case * required and includes the proto package name. The method name may be omitted, in which case
* any method on the given service is matched. * any method on the given service is matched.
*/ */
public static final class Name { static final class Name {
private final String service; private final String service;
@ -497,7 +493,7 @@ public final class RlsProtoData {
} }
/** The primary constructor. */ /** The primary constructor. */
public Name(String service, String method) { Name(String service, String method) {
checkState( checkState(
!checkNotNull(service, "service").isEmpty(), !checkNotNull(service, "service").isEmpty(),
"service must not be empty or null"); "service must not be empty or null");
@ -542,7 +538,7 @@ public final class RlsProtoData {
} }
@AutoValue @AutoValue
public abstract static class ExtraKeys { abstract static class ExtraKeys {
static final ExtraKeys DEFAULT = create(null, null, null); static final ExtraKeys DEFAULT = create(null, null, null);
@Nullable abstract String host(); @Nullable abstract String host();
@ -551,7 +547,7 @@ public final class RlsProtoData {
@Nullable abstract String method(); @Nullable abstract String method();
public static ExtraKeys create( static ExtraKeys create(
@Nullable String host, @Nullable String service, @Nullable String method) { @Nullable String host, @Nullable String service, @Nullable String method) {
return new AutoValue_RlsProtoData_ExtraKeys(host, service, method); return new AutoValue_RlsProtoData_ExtraKeys(host, service, method);
} }

2
xds/build.gradle
View File

@ -39,9 +39,9 @@ dependencies {
libraries.autovalue_annotation, libraries.autovalue_annotation,
libraries.opencensus_proto, libraries.opencensus_proto,
libraries.protobuf_util libraries.protobuf_util
implementation project(path: ':grpc-rls')
def nettyDependency = implementation project(':grpc-netty') def nettyDependency = implementation project(':grpc-netty')
testImplementation project(':grpc-rls')
testImplementation project(':grpc-core').sourceSets.test.output testImplementation project(':grpc-core').sourceSets.test.output
annotationProcessor libraries.autovalue annotationProcessor libraries.autovalue

21
xds/src/main/java/io/grpc/xds/MessagePrinter.java
View File

@ -16,7 +16,9 @@
package io.grpc.xds; package io.grpc.xds;
import com.google.protobuf.Descriptors.Descriptor;
import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.InvalidProtocolBufferException;
import com.google.protobuf.Message;
import com.google.protobuf.MessageOrBuilder; import com.google.protobuf.MessageOrBuilder;
import com.google.protobuf.TypeRegistry; import com.google.protobuf.TypeRegistry;
import com.google.protobuf.util.JsonFormat; import com.google.protobuf.util.JsonFormat;
@ -46,7 +48,7 @@ final class MessagePrinter {
static final JsonFormat.Printer printer = newPrinter(); static final JsonFormat.Printer printer = newPrinter();
private static JsonFormat.Printer newPrinter() { private static JsonFormat.Printer newPrinter() {
TypeRegistry registry = TypeRegistry.Builder registry =
TypeRegistry.newBuilder() TypeRegistry.newBuilder()
.add(Listener.getDescriptor()) .add(Listener.getDescriptor())
.add(io.envoyproxy.envoy.api.v2.Listener.getDescriptor()) .add(io.envoyproxy.envoy.api.v2.Listener.getDescriptor())
@ -71,9 +73,20 @@ final class MessagePrinter {
.add(io.envoyproxy.envoy.config.cluster.aggregate.v2alpha.ClusterConfig .add(io.envoyproxy.envoy.config.cluster.aggregate.v2alpha.ClusterConfig
.getDescriptor()) .getDescriptor())
.add(ClusterLoadAssignment.getDescriptor()) .add(ClusterLoadAssignment.getDescriptor())
.add(io.envoyproxy.envoy.api.v2.ClusterLoadAssignment.getDescriptor()) .add(io.envoyproxy.envoy.api.v2.ClusterLoadAssignment.getDescriptor());
.build(); try {
return JsonFormat.printer().usingTypeRegistry(registry); @SuppressWarnings("unchecked")
Class<? extends Message> routeLookupClusterSpecifierClass =
(Class<? extends Message>)
Class.forName("io.grpc.lookup.v1.RouteLookupClusterSpecifier");
Descriptor descriptor =
(Descriptor)
routeLookupClusterSpecifierClass.getDeclaredMethod("getDescriptor").invoke(null);
registry.add(descriptor);
} catch (Exception e) {
// Ignore. In most cases RouteLookup is not required.
}
return JsonFormat.printer().usingTypeRegistry(registry.build());
} }
} }

99
xds/src/main/java/io/grpc/xds/RouteLookupServiceClusterSpecifierPlugin.java
View File

@ -17,18 +17,14 @@
package io.grpc.xds; package io.grpc.xds;
import com.google.auto.value.AutoValue; import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.protobuf.Any; import com.google.protobuf.Any;
import com.google.protobuf.InvalidProtocolBufferException; import com.google.protobuf.InvalidProtocolBufferException;
import com.google.protobuf.Message; import com.google.protobuf.Message;
import com.google.protobuf.util.Durations; import io.grpc.internal.JsonParser;
import io.grpc.lookup.v1.GrpcKeyBuilder; import io.grpc.internal.JsonUtil;
import io.grpc.lookup.v1.GrpcKeyBuilder.ExtraKeys; import java.io.IOException;
import io.grpc.lookup.v1.GrpcKeyBuilder.Name; import java.util.Map;
import io.grpc.lookup.v1.NameMatcher;
import io.grpc.lookup.v1.RouteLookupConfig;
import io.grpc.rls.RlsProtoData;
import java.util.ArrayList;
import java.util.List;
/** The ClusterSpecifierPlugin for RouteLookup policy. */ /** The ClusterSpecifierPlugin for RouteLookup policy. */
final class RouteLookupServiceClusterSpecifierPlugin implements ClusterSpecifierPlugin { final class RouteLookupServiceClusterSpecifierPlugin implements ClusterSpecifierPlugin {
@ -49,84 +45,49 @@ final class RouteLookupServiceClusterSpecifierPlugin implements ClusterSpecifier
} }
@Override @Override
@SuppressWarnings("unchecked")
public ConfigOrError<RlsPluginConfig> parsePlugin(Message rawProtoMessage) { public ConfigOrError<RlsPluginConfig> parsePlugin(Message rawProtoMessage) {
if (!(rawProtoMessage instanceof Any)) { if (!(rawProtoMessage instanceof Any)) {
return ConfigOrError.fromError("Invalid config type: " + rawProtoMessage.getClass()); return ConfigOrError.fromError("Invalid config type: " + rawProtoMessage.getClass());
} }
Any anyMessage = (Any) rawProtoMessage;
RouteLookupConfig configProto;
try { try {
configProto = anyMessage.unpack(RouteLookupConfig.class); Any anyMessage = (Any) rawProtoMessage;
Class<? extends Message> protoClass;
try {
protoClass =
(Class<? extends Message>)
Class.forName("io.grpc.lookup.v1.RouteLookupClusterSpecifier");
} catch (ClassNotFoundException e) {
return ConfigOrError.fromError("Dependency for 'io.grpc:grpc-rls' is missing: " + e);
}
Message configProto;
try {
configProto = anyMessage.unpack(protoClass);
} catch (InvalidProtocolBufferException e) { } catch (InvalidProtocolBufferException e) {
return ConfigOrError.fromError("Invalid proto: " + e); return ConfigOrError.fromError("Invalid proto: " + e);
} }
String jsonString = MessagePrinter.print(configProto);
try { try {
List<GrpcKeyBuilder> keyBuildersProto = configProto.getGrpcKeybuildersList(); Map<String, ?> jsonMap = (Map<String, ?>) JsonParser.parse(jsonString);
List<RlsProtoData.GrpcKeyBuilder> keyBuilders = Map<String, ?> config = JsonUtil.getObject(jsonMap, "routeLookupConfig");
new ArrayList<>(keyBuildersProto.size());
for (GrpcKeyBuilder keyBuilderProto : keyBuildersProto) {
List<Name> namesProto = keyBuilderProto.getNamesList();
List<RlsProtoData.GrpcKeyBuilder.Name> names = new ArrayList<>(namesProto.size());
for (Name nameProto : namesProto) {
if (nameProto.getMethod().isEmpty()) {
names.add(new RlsProtoData.GrpcKeyBuilder.Name(nameProto.getService()));
} else {
names.add(
new RlsProtoData.GrpcKeyBuilder.Name(
nameProto.getService(), nameProto.getMethod()));
}
}
List<NameMatcher> headersProto = keyBuilderProto.getHeadersList();
List<RlsProtoData.NameMatcher> headers = new ArrayList<>(headersProto.size());
for (NameMatcher headerProto : headersProto) {
headers.add(
new RlsProtoData.NameMatcher(
headerProto.getKey(), headerProto.getNamesList(),
headerProto.getRequiredMatch()));
}
String host = null;
String service = null;
String method = null;
if (keyBuilderProto.hasExtraKeys()) {
ExtraKeys extraKeysProto = keyBuilderProto.getExtraKeys();
host = extraKeysProto.getHost();
service = extraKeysProto.getService();
method = extraKeysProto.getMethod();
}
RlsProtoData.ExtraKeys extraKeys =
RlsProtoData.ExtraKeys.create(host, service, method);
RlsProtoData.GrpcKeyBuilder keyBuilder =
new RlsProtoData.GrpcKeyBuilder(
names, headers, extraKeys, keyBuilderProto.getConstantKeysMap());
keyBuilders.add(keyBuilder);
}
RlsProtoData.RouteLookupConfig config = new RlsProtoData.RouteLookupConfig(
keyBuilders,
configProto.getLookupService(),
Durations.toMillis(configProto.getLookupServiceTimeout()),
configProto.hasMaxAge() ? Durations.toMillis(configProto.getMaxAge()) : null,
configProto.hasStaleAge() ? Durations.toMillis(configProto.getStaleAge()) : null,
configProto.getCacheSizeBytes(),
configProto.getValidTargetsList(),
configProto.getDefaultTarget());
return ConfigOrError.fromConfig(RlsPluginConfig.create(config)); return ConfigOrError.fromConfig(RlsPluginConfig.create(config));
} catch (RuntimeException e) { } catch (IOException e) {
return ConfigOrError.fromError( return ConfigOrError.fromError(
"Error parsing RouteLookupConfig: \n" + configProto + "\n reason: " + e); "Unable to parse RouteLookupClusterSpecifier: " + jsonString);
}
} catch (RuntimeException e) {
return ConfigOrError.fromError("Error parsing RouteLookupConfig: " + e);
} }
} }
@AutoValue @AutoValue
abstract static class RlsPluginConfig implements PluginConfig { abstract static class RlsPluginConfig implements PluginConfig {
abstract RlsProtoData.RouteLookupConfig config(); abstract ImmutableMap<String, ?> config();
static RlsPluginConfig create(RlsProtoData.RouteLookupConfig config) { static RlsPluginConfig create(Map<String, ?> config) {
return new AutoValue_RouteLookupServiceClusterSpecifierPlugin_RlsPluginConfig(config); return new AutoValue_RouteLookupServiceClusterSpecifierPlugin_RlsPluginConfig(
ImmutableMap.copyOf(config));
} }
@Override @Override

100
xds/src/test/java/io/grpc/xds/RouteLookupServiceClusterSpecifierPluginTest.java
View File

@ -26,8 +26,8 @@ import io.grpc.lookup.v1.GrpcKeyBuilder;
import io.grpc.lookup.v1.GrpcKeyBuilder.ExtraKeys; import io.grpc.lookup.v1.GrpcKeyBuilder.ExtraKeys;
import io.grpc.lookup.v1.GrpcKeyBuilder.Name; import io.grpc.lookup.v1.GrpcKeyBuilder.Name;
import io.grpc.lookup.v1.NameMatcher; import io.grpc.lookup.v1.NameMatcher;
import io.grpc.lookup.v1.RouteLookupClusterSpecifier;
import io.grpc.lookup.v1.RouteLookupConfig; import io.grpc.lookup.v1.RouteLookupConfig;
import io.grpc.rls.RlsProtoData;
import io.grpc.xds.RouteLookupServiceClusterSpecifierPlugin.RlsPluginConfig; import io.grpc.xds.RouteLookupServiceClusterSpecifierPlugin.RlsPluginConfig;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
@ -57,29 +57,38 @@ public class RouteLookupServiceClusterSpecifierPluginTest {
.addValidTargets("valid-target") .addValidTargets("valid-target")
.setDefaultTarget("default-target") .setDefaultTarget("default-target")
.build(); .build();
RouteLookupClusterSpecifier specifier =
RouteLookupClusterSpecifier.newBuilder().setRouteLookupConfig(routeLookupConfig).build();
RlsPluginConfig config = RlsPluginConfig config =
RouteLookupServiceClusterSpecifierPlugin.INSTANCE.parsePlugin(Any.pack(routeLookupConfig)) RouteLookupServiceClusterSpecifierPlugin.INSTANCE.parsePlugin(Any.pack(specifier))
.config; .config;
assertThat(config.typeUrl()).isEqualTo("type.googleapis.com/grpc.lookup.v1.RouteLookupConfig"); assertThat(config.typeUrl()).isEqualTo("type.googleapis.com/grpc.lookup.v1.RouteLookupConfig");
assertThat(config.config()).isEqualTo( assertThat(config.config()).isEqualTo(
new RlsProtoData.RouteLookupConfig( ImmutableMap.builder()
.put(
"grpcKeybuilders",
ImmutableList.of(ImmutableMap.of(
"names",
ImmutableList.of( ImmutableList.of(
new RlsProtoData.GrpcKeyBuilder( ImmutableMap.of("service", "service1", "method", "method1"),
ImmutableMap.of("service", "service2", "method", "method2")),
"headers",
ImmutableList.of( ImmutableList.of(
new RlsProtoData.GrpcKeyBuilder.Name("service1", "method1"), ImmutableMap.of(
new RlsProtoData.GrpcKeyBuilder.Name("service2", "method2")), "key", "key1", "names", ImmutableList.of("v1"),
ImmutableList.of( "requiredMatch", true)),
new RlsProtoData.NameMatcher("key1", ImmutableList.of("v1"), true)), "extraKeys",
RlsProtoData.ExtraKeys.create("host1", "service1", "method1"), ImmutableMap.of("host", "host1", "service", "service1", "method", "method1"),
ImmutableMap.of("key2", "value2") "constantKeys",
)), ImmutableMap.of("key2", "value2"))))
"rls-cbt.googleapis.com", .put("lookupService", "rls-cbt.googleapis.com")
1234, .put("lookupServiceTimeout", "1.234s")
56789L, .put("maxAge", "56.789s")
1000L, .put("staleAge", "1s")
5000, .put("cacheSizeBytes", "5000")
ImmutableList.of("valid-target"), .put("validTargets", ImmutableList.of("valid-target"))
"default-target")); .put("defaultTarget","default-target")
.build());
} }
@Test @Test
@ -96,47 +105,30 @@ public class RouteLookupServiceClusterSpecifierPluginTest {
.setCacheSizeBytes(5000) .setCacheSizeBytes(5000)
.addValidTargets("valid-target") .addValidTargets("valid-target")
.build(); .build();
RouteLookupClusterSpecifier specifier =
RouteLookupClusterSpecifier.newBuilder().setRouteLookupConfig(routeLookupConfig).build();
RlsPluginConfig config = RlsPluginConfig config =
RouteLookupServiceClusterSpecifierPlugin.INSTANCE.parsePlugin(Any.pack(routeLookupConfig)) RouteLookupServiceClusterSpecifierPlugin.INSTANCE.parsePlugin(Any.pack(specifier))
.config; .config;
assertThat(config.typeUrl()).isEqualTo("type.googleapis.com/grpc.lookup.v1.RouteLookupConfig"); assertThat(config.typeUrl()).isEqualTo("type.googleapis.com/grpc.lookup.v1.RouteLookupConfig");
assertThat(config.config()).isEqualTo( assertThat(config.config()).isEqualTo(
new RlsProtoData.RouteLookupConfig( ImmutableMap.builder()
.put(
"grpcKeybuilders",
ImmutableList.of(ImmutableMap.of(
"names",
ImmutableList.of( ImmutableList.of(
new RlsProtoData.GrpcKeyBuilder( ImmutableMap.of("service", "service1"),
ImmutableMap.of("service", "service2")),
"headers",
ImmutableList.of( ImmutableList.of(
new RlsProtoData.GrpcKeyBuilder.Name("service1"), ImmutableMap.of(
new RlsProtoData.GrpcKeyBuilder.Name("service2")), "key", "key1", "names", ImmutableList.of("v1"),
ImmutableList.of( "requiredMatch", true)))))
new RlsProtoData.NameMatcher("key1", ImmutableList.of("v1"), true)), .put("lookupService", "rls-cbt.googleapis.com")
RlsProtoData.ExtraKeys.create(null, null, null), .put("lookupServiceTimeout", "1.234s")
ImmutableMap.<String, String>of() .put("cacheSizeBytes", "5000")
)), .put("validTargets", ImmutableList.of("valid-target"))
"rls-cbt.googleapis.com", .build());
1234,
null,
null,
5000,
ImmutableList.of("valid-target"),
null));
}
@Test
public void parseInvalidConfig() {
RouteLookupConfig routeLookupConfig = RouteLookupConfig.newBuilder()
.addGrpcKeybuilders(
GrpcKeyBuilder.newBuilder()
.addNames(Name.newBuilder().setService("service1"))
.addNames(Name.newBuilder().setService("service2"))
.addHeaders(
NameMatcher.newBuilder().setKey("key1").addNames("v1").setRequiredMatch(true)))
.setLookupService("rls-cbt.googleapis.com")
.setLookupServiceTimeout(Durations.fromMillis(1234))
.setCacheSizeBytes(-5000) // negative
.addValidTargets("valid-target")
.build();
ConfigOrError<RlsPluginConfig> configOrError =
RouteLookupServiceClusterSpecifierPlugin.INSTANCE.parsePlugin(Any.pack(routeLookupConfig));
assertThat(configOrError.errorDetail).contains("cacheSize must be positive");
} }
} }