Skip to content

Commit

Permalink
xds: fix parsing RouteLookupClusterSpecifier mistake (#8641)
Browse files Browse the repository at this point in the history 
- 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`
  • Loading branch information
@dapengzhang0
dapengzhang0 authored Nov 10, 2021
1 parent b3579db commit ad0971e
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 147 deletions.
26 changes: 11 additions & 15 deletions rls/src/main/java/io/grpc/rls/RlsProtoData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.common.base.Objects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import io.grpc.Internal;
import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name;
import java.util.HashSet;
import java.util.List;
Expand All @@ -36,8 +35,7 @@
import javax.annotation.concurrent.Immutable;

/** RlsProtoData is a collection of internal representation of RouteLookupService proto messages. */
@Internal
public final class RlsProtoData {
final class RlsProtoData {

private RlsProtoData() {}

Expand Down Expand Up @@ -142,7 +140,7 @@ public String toString() {

/** A config object for gRPC RouteLookupService. */
@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_CACHE_SIZE = 5 * 1024 * 1024;
Expand All @@ -164,8 +162,7 @@ public static final class RouteLookupConfig {
@Nullable
private final String defaultTarget;

/** Constructor. */
public RouteLookupConfig(
RouteLookupConfig(
List<GrpcKeyBuilder> grpcKeyBuilders,
String lookupService,
long lookupServiceTimeoutInMillis,
Expand Down Expand Up @@ -336,16 +333,15 @@ private static void checkUniqueName(List<GrpcKeyBuilder> grpcKeyBuilders) {
* is true, one of the specified names must be present for the keybuilder to match.
*/
@Immutable
public static final class NameMatcher {
static final class NameMatcher {

private final String key;

private final ImmutableList<String> names;

private final boolean optional;

/** Constructor. */
public NameMatcher(String key, List<String> names, @Nullable Boolean optional) {
NameMatcher(String key, List<String> names, @Nullable Boolean optional) {
this.key = checkNotNull(key, "key");
this.names = ImmutableList.copyOf(checkNotNull(names, "names"));
this.optional = optional != null ? optional : true;
Expand Down Expand Up @@ -398,7 +394,7 @@ public String toString() {
}

/** 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;

Expand All @@ -407,7 +403,7 @@ public static final class GrpcKeyBuilder {
private final ImmutableMap<String, String> constantKeys;

/** Constructor. All args should be nonnull. Headers should head unique keys. */
public GrpcKeyBuilder(
GrpcKeyBuilder(
List<Name> names, List<NameMatcher> headers, ExtraKeys extraKeys,
Map<String, String> constantKeys) {
checkState(names != null && !names.isEmpty(), "names cannot be empty");
Expand Down Expand Up @@ -486,7 +482,7 @@ public String toString() {
* required and includes the proto package name. The method name may be omitted, in which case
* any method on the given service is matched.
*/
public static final class Name {
static final class Name {

private final String service;

Expand All @@ -497,7 +493,7 @@ public Name(String service) {
}

/** The primary constructor. */
public Name(String service, String method) {
Name(String service, String method) {
checkState(
!checkNotNull(service, "service").isEmpty(),
"service must not be empty or null");
Expand Down Expand Up @@ -542,7 +538,7 @@ public String toString() {
}

@AutoValue
public abstract static class ExtraKeys {
abstract static class ExtraKeys {
static final ExtraKeys DEFAULT = create(null, null, null);

@Nullable abstract String host();
Expand All @@ -551,7 +547,7 @@ public abstract static class ExtraKeys {

@Nullable abstract String method();

public static ExtraKeys create(
static ExtraKeys create(
@Nullable String host, @Nullable String service, @Nullable String method) {
return new AutoValue_RlsProtoData_ExtraKeys(host, service, method);
}
Expand Down
2 changes: 1 addition & 1 deletion xds/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ dependencies {
libraries.autovalue_annotation,
libraries.opencensus_proto,
libraries.protobuf_util
implementation project(path: ':grpc-rls')
def nettyDependency = implementation project(':grpc-netty')

testImplementation project(':grpc-rls')
testImplementation project(':grpc-core').sourceSets.test.output

annotationProcessor libraries.autovalue
Expand Down
21 changes: 17 additions & 4 deletions xds/src/main/java/io/grpc/xds/MessagePrinter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@

package io.grpc.xds;

import com.google.protobuf.Descriptors.Descriptor;
import com.google.protobuf.InvalidProtocolBufferException;
import com.google.protobuf.Message;
import com.google.protobuf.MessageOrBuilder;
import com.google.protobuf.TypeRegistry;
import com.google.protobuf.util.JsonFormat;
Expand Down Expand Up @@ -46,7 +48,7 @@ private static class LazyHolder {
static final JsonFormat.Printer printer = newPrinter();

private static JsonFormat.Printer newPrinter() {
TypeRegistry registry =
TypeRegistry.Builder registry =
TypeRegistry.newBuilder()
.add(Listener.getDescriptor())
.add(io.envoyproxy.envoy.api.v2.Listener.getDescriptor())
Expand All @@ -71,9 +73,20 @@ private static JsonFormat.Printer newPrinter() {
.add(io.envoyproxy.envoy.config.cluster.aggregate.v2alpha.ClusterConfig
.getDescriptor())
.add(ClusterLoadAssignment.getDescriptor())
.add(io.envoyproxy.envoy.api.v2.ClusterLoadAssignment.getDescriptor())
.build();
return JsonFormat.printer().usingTypeRegistry(registry);
.add(io.envoyproxy.envoy.api.v2.ClusterLoadAssignment.getDescriptor());
try {
@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());
}
}

Expand Down
107 changes: 34 additions & 73 deletions xds/src/main/java/io/grpc/xds/RouteLookupServiceClusterSpecifierPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,14 @@
package io.grpc.xds;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.protobuf.Any;
import com.google.protobuf.InvalidProtocolBufferException;
import com.google.protobuf.Message;
import com.google.protobuf.util.Durations;
import io.grpc.lookup.v1.GrpcKeyBuilder;
import io.grpc.lookup.v1.GrpcKeyBuilder.ExtraKeys;
import io.grpc.lookup.v1.GrpcKeyBuilder.Name;
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;
import io.grpc.internal.JsonParser;
import io.grpc.internal.JsonUtil;
import java.io.IOException;
import java.util.Map;

/** The ClusterSpecifierPlugin for RouteLookup policy. */
final class RouteLookupServiceClusterSpecifierPlugin implements ClusterSpecifierPlugin {
Expand All @@ -49,84 +45,49 @@ public String[] typeUrls() {
}

@Override
@SuppressWarnings("unchecked")
public ConfigOrError<RlsPluginConfig> parsePlugin(Message rawProtoMessage) {
if (!(rawProtoMessage instanceof Any)) {
return ConfigOrError.fromError("Invalid config type: " + rawProtoMessage.getClass());
}

Any anyMessage = (Any) rawProtoMessage;
RouteLookupConfig configProto;
try {
configProto = anyMessage.unpack(RouteLookupConfig.class);
} catch (InvalidProtocolBufferException e) {
return ConfigOrError.fromError("Invalid proto: " + e);
}
try {
List<GrpcKeyBuilder> keyBuildersProto = configProto.getGrpcKeybuildersList();
List<RlsProtoData.GrpcKeyBuilder> keyBuilders =
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);
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) {
return ConfigOrError.fromError("Invalid proto: " + e);
}
String jsonString = MessagePrinter.print(configProto);
try {
Map<String, ?> jsonMap = (Map<String, ?>) JsonParser.parse(jsonString);
Map<String, ?> config = JsonUtil.getObject(jsonMap, "routeLookupConfig");
return ConfigOrError.fromConfig(RlsPluginConfig.create(config));
} catch (IOException e) {
return ConfigOrError.fromError(
"Unable to parse RouteLookupClusterSpecifier: " + jsonString);
}
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));
} catch (RuntimeException e) {
return ConfigOrError.fromError(
"Error parsing RouteLookupConfig: \n" + configProto + "\n reason: " + e);
return ConfigOrError.fromError("Error parsing RouteLookupConfig: " + e);
}
}

@AutoValue
abstract static class RlsPluginConfig implements PluginConfig {

abstract RlsProtoData.RouteLookupConfig config();
abstract ImmutableMap<String, ?> config();

static RlsPluginConfig create(RlsProtoData.RouteLookupConfig config) {
return new AutoValue_RouteLookupServiceClusterSpecifierPlugin_RlsPluginConfig(config);
static RlsPluginConfig create(Map<String, ?> config) {
return new AutoValue_RouteLookupServiceClusterSpecifierPlugin_RlsPluginConfig(
ImmutableMap.copyOf(config));
}

@Override
Expand Down
Loading

0 comments on commit ad0971e

Please sign in to comment.