From cd346832babd623df3b2c7c2e705f3da2285bc19 Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Tue, 7 Sep 2021 21:32:33 -0700 Subject: [PATCH] rls: migrate deprecated server/path to extraKeys (#8469) The [`server` and `path` fields](https://github.com/grpc/grpc-java/blob/v1.40.1/rls/src/main/proto/grpc/lookup/v1/rls.proto#L25-L32) in `RouteLookupRequest` are deprecated. Instead, we will send the server/path information in side of [`key_map`](https://github.com/grpc/grpc-java/blob/v1.40.1/rls/src/main/proto/grpc/lookup/v1/rls.proto#L45). The keys for the server, service and method in the `key_map` will be the _values_ of `host`, `service`, `method` fields respectively in [`extraKeys`](https://github.com/grpc/grpc-java/blob/v1.40.1/rls/src/main/proto/grpc/lookup/v1/rls_config.proto#L69) in RlsConfig. We will also include all entries in the [`constantKey`](https://github.com/grpc/grpc-java/blob/v1.40.1/rls/src/main/proto/grpc/lookup/v1/rls_config.proto#L80) in RlsConfig into `RouteLookupRequest`. Other changes: - Add AutoValue library for ExtraKeys class, just like data classes used in grpc-xds. Will migrate other data classes to AutoValue as well. - Not to keep `targetType` field in the route lookup request data class, because we always use "grpc" as targetType. --- repositories.bzl | 2 + rls/BUILD.bazel | 21 +++++ rls/build.gradle | 13 +++ .../java/io/grpc/rls/RlsProtoConverters.java | 29 +++--- .../main/java/io/grpc/rls/RlsProtoData.java | 88 +++++++++---------- .../java/io/grpc/rls/RlsRequestFactory.java | 64 ++++++++------ .../io/grpc/rls/CachingRlsLbClientTest.java | 40 ++++----- .../java/io/grpc/rls/RlsLoadBalancerTest.java | 21 +++-- .../io/grpc/rls/RlsProtoConvertersTest.java | 38 ++++---- .../io/grpc/rls/RlsRequestFactoryTest.java | 54 +++++++----- 10 files changed, 217 insertions(+), 153 deletions(-) diff --git a/repositories.bzl b/repositories.bzl index 6cf75aa7bb..0d6e9ab2f7 100644 --- a/repositories.bzl +++ b/repositories.bzl @@ -16,6 +16,8 @@ IO_GRPC_GRPC_JAVA_ARTIFACTS = [ "com.google.auth:google-auth-library-oauth2-http:0.22.0", "com.google.code.findbugs:jsr305:3.0.2", "com.google.code.gson:gson:jar:2.8.6", + "com.google.auto.value:auto-value:1.7.4", + "com.google.auto.value:auto-value-annotations:1.7.4", "com.google.errorprone:error_prone_annotations:2.9.0", "com.google.guava:failureaccess:1.0.1", "com.google.guava:guava:30.1-android", diff --git a/rls/BUILD.bazel b/rls/BUILD.bazel index 0da03fc924..4daa702956 100644 --- a/rls/BUILD.bazel +++ b/rls/BUILD.bazel @@ -7,12 +7,14 @@ java_library( ]), visibility = ["//visibility:public"], deps = [ + ":autovalue", ":rls_java_grpc", "//api", "//core", "//core:internal", "//core:util", "//stub", + "@com_google_auto_value_auto_value_annotations//jar", "@com_google_code_findbugs_jsr305//jar", "@com_google_guava_guava//jar", "@io_grpc_grpc_proto//:rls_java_proto", @@ -20,6 +22,25 @@ java_library( ], ) +java_plugin( + name = "autovalue_plugin", + processor_class = "com.google.auto.value.processor.AutoValueProcessor", + deps = [ + "@com_google_auto_value_auto_value//jar", + ], +) + +java_library( + name = "autovalue", + exported_plugins = [ + ":autovalue_plugin", + ], + neverlink = 1, + exports = [ + "@com_google_auto_value_auto_value//jar", + ], +) + java_grpc_library( name = "rls_java_grpc", srcs = ["@io_grpc_grpc_proto//:rls_proto"], diff --git a/rls/build.gradle b/rls/build.gradle index a2ebf2a62e..45f17fb71c 100644 --- a/rls/build.gradle +++ b/rls/build.gradle @@ -14,7 +14,9 @@ dependencies { implementation project(':grpc-core'), project(':grpc-protobuf'), project(':grpc-stub'), + libraries.autovalue_annotation, libraries.guava + annotationProcessor libraries.autovalue compileOnly libraries.javax_annotation testImplementation libraries.truth, project(':grpc-grpclb'), @@ -24,6 +26,17 @@ dependencies { signature "org.codehaus.mojo.signature:java17:1.0@signature" } +[compileJava].each() { + it.options.compilerArgs += [ + // only has AutoValue annotation processor + "-Xlint:-processing", + ] + appendToProperty( + it.options.errorprone.excludedPaths, + ".*/build/generated/sources/annotationProcessor/java/.*", + "|") +} + javadoc { // Do not publish javadoc since currently there is no public API. failOnError false // no public or protected classes found to document diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index 32df13c426..ce89def446 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -20,9 +20,11 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.base.Converter; +import com.google.common.collect.ImmutableMap; import io.grpc.internal.JsonUtil; import io.grpc.lookup.v1.RouteLookupRequest; import io.grpc.lookup.v1.RouteLookupResponse; +import io.grpc.rls.RlsProtoData.ExtraKeys; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name; import io.grpc.rls.RlsProtoData.NameMatcher; @@ -46,25 +48,16 @@ final class RlsProtoConverters { static final class RouteLookupRequestConverter extends Converter { - @SuppressWarnings("deprecation") @Override protected RlsProtoData.RouteLookupRequest doForward(RouteLookupRequest routeLookupRequest) { - return - new RlsProtoData.RouteLookupRequest( - /* server= */ routeLookupRequest.getServer(), - /* path= */ routeLookupRequest.getPath(), - /* targetType= */ routeLookupRequest.getTargetType(), - routeLookupRequest.getKeyMapMap()); + return new RlsProtoData.RouteLookupRequest(routeLookupRequest.getKeyMapMap()); } - @SuppressWarnings("deprecation") @Override protected RouteLookupRequest doBackward(RlsProtoData.RouteLookupRequest routeLookupRequest) { return RouteLookupRequest.newBuilder() - .setServer(routeLookupRequest.getServer()) - .setPath(routeLookupRequest.getPath()) - .setTargetType(routeLookupRequest.getTargetType()) + .setTargetType("grpc") .putAllKeyMap(routeLookupRequest.getKeyMap()) .build(); } @@ -183,7 +176,19 @@ final class RlsProtoConverters { matcher.isOptional(), "NameMatcher for GrpcKeyBuilders shouldn't be required"); nameMatchers.add(matcher); } - return new GrpcKeyBuilder(names, nameMatchers); + ExtraKeys extraKeys = ExtraKeys.DEFAULT; + Map rawExtraKeys = + (Map) JsonUtil.getObject(keyBuilder, "extraKeys"); + if (rawExtraKeys != null) { + extraKeys = ExtraKeys.create( + rawExtraKeys.get("host"), rawExtraKeys.get("service"), rawExtraKeys.get("method")); + } + Map constantKeys = + (Map) JsonUtil.getObject(keyBuilder, "constantKeys"); + if (constantKeys == null) { + constantKeys = ImmutableMap.of(); + } + return new GrpcKeyBuilder(names, nameMatchers, extraKeys, constantKeys); } } diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoData.java b/rls/src/main/java/io/grpc/rls/RlsProtoData.java index fbcb6feb21..3556ca609b 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoData.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoData.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; import com.google.common.base.Objects; import com.google.common.collect.ImmutableList; @@ -40,46 +41,12 @@ final class RlsProtoData { @Immutable static final class RouteLookupRequest { - private final String server; - - private final String path; - - private final String targetType; - private final ImmutableMap keyMap; - RouteLookupRequest( - String server, String path, String targetType, Map keyMap) { - this.server = checkNotNull(server, "server"); - this.path = checkNotNull(path, "path"); - this.targetType = checkNotNull(targetType, "targetName"); + RouteLookupRequest(Map keyMap) { this.keyMap = ImmutableMap.copyOf(checkNotNull(keyMap, "keyMap")); } - /** - * Returns a full host name of the target server, {@literal e.g.} firestore.googleapis.com. Only - * set for gRPC requests; HTTP requests must use key_map explicitly. - */ - String getServer() { - return server; - } - - /** - * Returns a full path of the request, {@literal i.e.} "/service/method". Only set for gRPC - * requests; HTTP requests must use key_map explicitly. - */ - String getPath() { - return path; - } - - /** - * Returns the target type allows the client to specify what kind of target format it would like - * from RLS to allow it to find the regional server, {@literal e.g.} "grpc". - */ - String getTargetType() { - return targetType; - } - /** Returns a map of key values extracted via key builders for the gRPC or HTTP request. */ ImmutableMap getKeyMap() { return keyMap; @@ -94,23 +61,17 @@ final class RlsProtoData { return false; } RouteLookupRequest that = (RouteLookupRequest) o; - return Objects.equal(server, that.server) - && Objects.equal(path, that.path) - && Objects.equal(targetType, that.targetType) - && Objects.equal(keyMap, that.keyMap); + return Objects.equal(keyMap, that.keyMap); } @Override public int hashCode() { - return Objects.hashCode(server, path, targetType, keyMap); + return Objects.hashCode(keyMap); } @Override public String toString() { return MoreObjects.toStringHelper(this) - .add("server", server) - .add("path", path) - .add("targetName", targetType) .add("keyMap", keyMap) .toString(); } @@ -300,6 +261,7 @@ final class RlsProtoData { * error. Note that requests can be routed only to a subdomain of the original target, * {@literal e.g.} "us_east_1.cloudbigtable.googleapis.com". */ + @Nullable String getDefaultTarget() { return defaultTarget; } @@ -431,12 +393,18 @@ final class RlsProtoData { private final ImmutableList names; private final ImmutableList headers; + private final ExtraKeys extraKeys; + private final ImmutableMap constantKeys; - public GrpcKeyBuilder(List names, List headers) { + public GrpcKeyBuilder( + List names, List headers, ExtraKeys extraKeys, + Map constantKeys) { checkState(names != null && !names.isEmpty(), "names cannot be empty"); this.names = ImmutableList.copyOf(names); checkUniqueKey(checkNotNull(headers, "headers")); this.headers = ImmutableList.copyOf(headers); + this.extraKeys = checkNotNull(extraKeys, "extraKeys"); + this.constantKeys = ImmutableMap.copyOf(checkNotNull(constantKeys, "constantKeys")); } private static void checkUniqueKey(List headers) { @@ -464,6 +432,14 @@ final class RlsProtoData { return headers; } + ExtraKeys getExtraKeys() { + return extraKeys; + } + + ImmutableMap getConstantKeys() { + return constantKeys; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -473,12 +449,14 @@ final class RlsProtoData { return false; } GrpcKeyBuilder that = (GrpcKeyBuilder) o; - return Objects.equal(names, that.names) && Objects.equal(headers, that.headers); + return Objects.equal(names, that.names) && Objects.equal(headers, that.headers) + && Objects.equal(extraKeys, that.extraKeys) + && Objects.equal(constantKeys, that.constantKeys); } @Override public int hashCode() { - return Objects.hashCode(names, headers); + return Objects.hashCode(names, headers, extraKeys, constantKeys); } @Override @@ -486,6 +464,8 @@ final class RlsProtoData { return MoreObjects.toStringHelper(this) .add("names", names) .add("headers", headers) + .add("extraKeys", extraKeys) + .add("constantKeys", constantKeys) .toString(); } @@ -548,4 +528,20 @@ final class RlsProtoData { } } } + + @AutoValue + abstract static class ExtraKeys { + static final ExtraKeys DEFAULT = create(null, null, null); + + @Nullable abstract String host(); + + @Nullable abstract String service(); + + @Nullable abstract String method(); + + static ExtraKeys create( + @Nullable String host, @Nullable String service, @Nullable String method) { + return new AutoValue_RlsProtoData_ExtraKeys(host, service, method); + } + } } diff --git a/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java b/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java index b9bcb037cf..e181d64833 100644 --- a/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java +++ b/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java @@ -19,17 +19,18 @@ package io.grpc.rls; import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.base.MoreObjects; -import com.google.common.collect.HashBasedTable; -import com.google.common.collect.Table; +import com.google.common.collect.ImmutableMap; import io.grpc.Metadata; import io.grpc.Status; import io.grpc.StatusRuntimeException; +import io.grpc.rls.RlsProtoData.ExtraKeys; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name; import io.grpc.rls.RlsProtoData.NameMatcher; import io.grpc.rls.RlsProtoData.RouteLookupConfig; import io.grpc.rls.RlsProtoData.RouteLookupRequest; import java.util.HashMap; +import java.util.List; import java.util.Map; import javax.annotation.CheckReturnValue; @@ -40,10 +41,7 @@ import javax.annotation.CheckReturnValue; final class RlsRequestFactory { private final String target; - /** - * schema: Path(/serviceName/methodName or /serviceName/*), rls request headerName, header fields. - */ - private final Table keyBuilderTable; + private final Map keyBuilderTable; RlsRequestFactory(RouteLookupConfig rlsConfig, String target) { checkNotNull(rlsConfig, "rlsConfig"); @@ -51,18 +49,15 @@ final class RlsRequestFactory { this.keyBuilderTable = createKeyBuilderTable(rlsConfig); } - private static Table createKeyBuilderTable( + private static Map createKeyBuilderTable( RouteLookupConfig config) { - Table table = HashBasedTable.create(); + Map table = new HashMap<>(); for (GrpcKeyBuilder grpcKeyBuilder : config.getGrpcKeyBuilders()) { - for (NameMatcher nameMatcher : grpcKeyBuilder.getHeaders()) { - for (Name name : grpcKeyBuilder.getNames()) { - String method = - name.getMethod() == null || name.getMethod().isEmpty() - ? "*" : name.getMethod(); - String path = "/" + name.getService() + "/" + method; - table.put(path, nameMatcher.getKey(), nameMatcher); - } + for (Name name : grpcKeyBuilder.getNames()) { + boolean hasMethod = name.getMethod() == null || name.getMethod().isEmpty(); + String method = hasMethod ? "*" : name.getMethod(); + String path = "/" + name.getService() + "/" + method; + table.put(path, grpcKeyBuilder); } } return table; @@ -74,20 +69,35 @@ final class RlsRequestFactory { checkNotNull(service, "service"); checkNotNull(method, "method"); String path = "/" + service + "/" + method; - Map keyBuilder = keyBuilderTable.row(path); - // if no matching keyBuilder found, fall back to wildcard match (ServiceName/*) - if (keyBuilder.isEmpty()) { - keyBuilder = keyBuilderTable.row("/" + service + "/*"); + GrpcKeyBuilder grpcKeyBuilder = keyBuilderTable.get(path); + if (grpcKeyBuilder == null) { + // if no matching keyBuilder found, fall back to wildcard match (ServiceName/*) + grpcKeyBuilder = keyBuilderTable.get("/" + service + "/*"); } - Map rlsRequestHeaders = createRequestHeaders(metadata, keyBuilder); - return new RouteLookupRequest(target, path, "grpc", rlsRequestHeaders); + if (grpcKeyBuilder == null) { + return new RouteLookupRequest(ImmutableMap.of()); + } + Map rlsRequestHeaders = + createRequestHeaders(metadata, grpcKeyBuilder.getHeaders()); + ExtraKeys extraKeys = grpcKeyBuilder.getExtraKeys(); + Map constantKeys = grpcKeyBuilder.getConstantKeys(); + if (extraKeys.host() != null) { + rlsRequestHeaders.put(extraKeys.host(), target); + } + if (extraKeys.service() != null) { + rlsRequestHeaders.put(extraKeys.service(), service); + } + if (extraKeys.method() != null) { + rlsRequestHeaders.put(extraKeys.method(), method); + } + rlsRequestHeaders.putAll(constantKeys); + return new RouteLookupRequest(rlsRequestHeaders); } private Map createRequestHeaders( - Metadata metadata, Map keyBuilder) { + Metadata metadata, List keyBuilder) { Map rlsRequestHeaders = new HashMap<>(); - for (Map.Entry entry : keyBuilder.entrySet()) { - NameMatcher nameMatcher = entry.getValue(); + for (NameMatcher nameMatcher : keyBuilder) { String value = null; for (String requestHeaderName : nameMatcher.names()) { value = metadata.get(Metadata.Key.of(requestHeaderName, Metadata.ASCII_STRING_MARSHALLER)); @@ -96,11 +106,11 @@ final class RlsRequestFactory { } } if (value != null) { - rlsRequestHeaders.put(entry.getKey(), value); + rlsRequestHeaders.put(nameMatcher.getKey(), value); } else if (!nameMatcher.isOptional()) { throw new StatusRuntimeException( Status.INVALID_ARGUMENT.withDescription( - String.format("Missing mandatory metadata(%s) not found", entry.getKey()))); + String.format("Missing mandatory metadata(%s) not found", nameMatcher.getKey()))); } } return rlsRequestHeaders; diff --git a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java index c2c221800a..aa64ec890b 100644 --- a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java +++ b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java @@ -65,6 +65,7 @@ import io.grpc.rls.LbPolicyConfiguration.ChildPolicyWrapper; import io.grpc.rls.LruCache.EvictionListener; import io.grpc.rls.LruCache.EvictionType; import io.grpc.rls.RlsProtoConverters.RouteLookupResponseConverter; +import io.grpc.rls.RlsProtoData.ExtraKeys; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name; import io.grpc.rls.RlsProtoData.NameMatcher; @@ -191,9 +192,8 @@ public class CachingRlsLbClientTest { public void get_noError_lifeCycle() throws Exception { setUpRlsLbClient(); InOrder inOrder = inOrder(evictionListener); - RouteLookupRequest routeLookupRequest = - new RouteLookupRequest( - "bigtable.googleapis.com", "/foo/bar", "grpc", ImmutableMap.of()); + RouteLookupRequest routeLookupRequest = new RouteLookupRequest(ImmutableMap.of( + "server", "bigtable.googleapis.com", "service-key", "foo", "method-key", "bar")); rlsServerImpl.setLookupTable( ImmutableMap.of( routeLookupRequest, @@ -242,9 +242,8 @@ public class CachingRlsLbClientTest { public void rls_overDirectPath() throws Exception { CachingRlsLbClient.enableOobChannelDirectPath = true; setUpRlsLbClient(); - RouteLookupRequest routeLookupRequest = - new RouteLookupRequest( - "bigtable.googleapis.com", "/foo/bar", "grpc", ImmutableMap.of()); + RouteLookupRequest routeLookupRequest = new RouteLookupRequest(ImmutableMap.of( + "server", "bigtable.googleapis.com", "service-key", "foo", "method-key", "bar")); rlsServerImpl.setLookupTable( ImmutableMap.of( routeLookupRequest, @@ -276,8 +275,8 @@ public class CachingRlsLbClientTest { @Test public void get_throttledAndRecover() throws Exception { setUpRlsLbClient(); - RouteLookupRequest routeLookupRequest = - new RouteLookupRequest("server", "/foo/bar", "grpc", ImmutableMap.of()); + RouteLookupRequest routeLookupRequest = new RouteLookupRequest(ImmutableMap.of( + "server", "bigtable.googleapis.com", "service-key", "foo", "method-key", "bar")); rlsServerImpl.setLookupTable( ImmutableMap.of( routeLookupRequest, @@ -319,9 +318,8 @@ public class CachingRlsLbClientTest { public void get_updatesLbState() throws Exception { setUpRlsLbClient(); InOrder inOrder = inOrder(helper); - RouteLookupRequest routeLookupRequest = - new RouteLookupRequest( - "bigtable.googleapis.com", "/foo/bar", "grpc", ImmutableMap.of()); + RouteLookupRequest routeLookupRequest = new RouteLookupRequest(ImmutableMap.of( + "server", "bigtable.googleapis.com", "service-key", "service1", "method-key", "create")); rlsServerImpl.setLookupTable( ImmutableMap.of( routeLookupRequest, @@ -349,7 +347,8 @@ public class CachingRlsLbClientTest { Metadata headers = new Metadata(); PickResult pickResult = pickerCaptor.getValue().pickSubchannel( new PickSubchannelArgsImpl( - TestMethodDescriptors.voidMethod().toBuilder().setFullMethodName("foo/bar").build(), + TestMethodDescriptors.voidMethod().toBuilder().setFullMethodName("service1/create") + .build(), headers, CallOptions.DEFAULT)); assertThat(pickResult.getStatus().isOk()).isTrue(); @@ -360,8 +359,7 @@ public class CachingRlsLbClientTest { fakeBackoffProvider.nextPolicy = createBackoffPolicy(100, TimeUnit.MILLISECONDS); // try to get invalid RouteLookupRequest invalidRouteLookupRequest = - new RouteLookupRequest( - "bigtable.googleapis.com", "/doesn/exists", "grpc", ImmutableMap.of()); + new RouteLookupRequest(ImmutableMap.of()); CachedRouteLookupResponse errorResp = getInSyncContext(invalidRouteLookupRequest); assertThat(errorResp.isPending()).isTrue(); fakeTimeProvider.forwardTime(SERVER_LATENCY_MILLIS, TimeUnit.MILLISECONDS); @@ -369,7 +367,7 @@ public class CachingRlsLbClientTest { errorResp = getInSyncContext(invalidRouteLookupRequest); assertThat(errorResp.hasError()).isTrue(); - // Channel is still READY because the subchannel for method /foo/bar is still READY. + // Channel is still READY because the subchannel for method /service1/create is still READY. // Method /doesn/exists will use fallback child balancer and fail immediately. inOrder.verify(helper) .updateBalancingState(eq(ConnectivityState.READY), pickerCaptor.capture()); @@ -387,10 +385,10 @@ public class CachingRlsLbClientTest { @Test public void get_childPolicyWrapper_reusedForSameTarget() throws Exception { setUpRlsLbClient(); - RouteLookupRequest routeLookupRequest = - new RouteLookupRequest("server", "/foo/bar", "grpc", ImmutableMap.of()); - RouteLookupRequest routeLookupRequest2 = - new RouteLookupRequest("server", "/foo/baz", "grpc", ImmutableMap.of()); + RouteLookupRequest routeLookupRequest = new RouteLookupRequest(ImmutableMap.of( + "server", "bigtable.googleapis.com", "service-key", "foo", "method-key", "bar")); + RouteLookupRequest routeLookupRequest2 = new RouteLookupRequest(ImmutableMap.of( + "server", "bigtable.googleapis.com", "service-key", "foo", "method-key", "baz")); rlsServerImpl.setLookupTable( ImmutableMap.of( routeLookupRequest, new RouteLookupResponse(ImmutableList.of("target"), "header"), @@ -426,7 +424,9 @@ public class CachingRlsLbClientTest { ImmutableList.of(new Name("service1", "create")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"), true), - new NameMatcher("id", ImmutableList.of("X-Google-Id"), true)))), + new NameMatcher("id", ImmutableList.of("X-Google-Id"), true)), + ExtraKeys.create("server", "service-key", "method-key"), + ImmutableMap.of())), /* lookupService= */ "service1", /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2), /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), diff --git a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java index 3b7e84bd54..fba295f98f 100644 --- a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java @@ -144,13 +144,15 @@ public class RlsLoadBalancerTest { .build(); fakeRlsServerImpl.setLookupTable( ImmutableMap.of( - new RouteLookupRequest( - "fake-bigtable.googleapis.com", "/com.google/Search", "grpc", - ImmutableMap.of()), + new RouteLookupRequest(ImmutableMap.of( + "server", "fake-bigtable.googleapis.com", + "service-key", "com.google", + "method-key", "Search")), new RouteLookupResponse(ImmutableList.of("wilderness"), "where are you?"), - new RouteLookupRequest( - "fake-bigtable.googleapis.com", "/com.google/Rescue", "grpc", - ImmutableMap.of()), + new RouteLookupRequest(ImmutableMap.of( + "server", "fake-bigtable.googleapis.com", + "service-key", "com.google", + "method-key", "Rescue")), new RouteLookupResponse(ImmutableList.of("civilization"), "you are safe"))); rlsLb = (RlsLoadBalancer) provider.newLoadBalancer(helper); @@ -409,7 +411,12 @@ public class RlsLoadBalancerTest { + " \"names\": [\"PermitId\"],\n" + " \"optional\": true\n" + " }\n" - + " ]\n" + + " ],\n" + + " \"extraKeys\": {\n" + + " \"host\": \"server\",\n" + + " \"service\": \"service-key\",\n" + + " \"method\": \"method-key\"\n" + + " }\n" + " }\n" + " ],\n" + " \"lookupService\": \"localhost:8972\",\n" diff --git a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java index a50cdeb9f6..bfb331e6cc 100644 --- a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java @@ -27,6 +27,7 @@ import io.grpc.lookup.v1.RouteLookupResponse; import io.grpc.rls.RlsProtoConverters.RouteLookupConfigConverter; import io.grpc.rls.RlsProtoConverters.RouteLookupRequestConverter; import io.grpc.rls.RlsProtoConverters.RouteLookupResponseConverter; +import io.grpc.rls.RlsProtoData.ExtraKeys; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name; import io.grpc.rls.RlsProtoData.NameMatcher; @@ -41,40 +42,29 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class RlsProtoConvertersTest { - @SuppressWarnings("deprecation") @Test public void convert_toRequestProto() { Converter converter = new RouteLookupRequestConverter(); RouteLookupRequest proto = RouteLookupRequest.newBuilder() - .setServer("server") - .setPath("path") - .setTargetType("target") .putKeyMap("key1", "val1") .build(); RlsProtoData.RouteLookupRequest object = converter.convert(proto); - assertThat(object.getServer()).isEqualTo("server"); - assertThat(object.getPath()).isEqualTo("path"); - assertThat(object.getTargetType()).isEqualTo("target"); assertThat(object.getKeyMap()).containsExactly("key1", "val1"); } - @SuppressWarnings("deprecation") @Test public void convert_toRequestObject() { Converter converter = new RouteLookupRequestConverter().reverse(); RlsProtoData.RouteLookupRequest requestObject = - new RlsProtoData.RouteLookupRequest( - "server", "path", "target", ImmutableMap.of("key1", "val1")); + new RlsProtoData.RouteLookupRequest(ImmutableMap.of("key1", "val1")); RouteLookupRequest proto = converter.convert(requestObject); - assertThat(proto.getServer()).isEqualTo("server"); - assertThat(proto.getPath()).isEqualTo("path"); - assertThat(proto.getTargetType()).isEqualTo("target"); + assertThat(proto.getTargetType()).isEqualTo("grpc"); assertThat(proto.getKeyMapMap()).containsExactly("key1", "val1"); } @@ -164,7 +154,15 @@ public class RlsProtoConvertersTest { + " \"names\": [\"User\", \"Parent\"],\n" + " \"optional\": true\n" + " }\n" - + " ]\n" + + " ],\n" + + " \"extraKeys\": {\n" + + " \"host\": \"host-key\",\n" + + " \"service\": \"service-key\",\n" + + " \"method\": \"method-key\"\n" + + " }, \n" + + " \"constantKeys\": {\n" + + " \"constKey1\": \"value1\"\n" + + " }\n" + " }\n" + " ],\n" + " \"lookupService\": \"service1\",\n" @@ -183,16 +181,22 @@ public class RlsProtoConvertersTest { ImmutableList.of(new Name("service1", "create")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"), true), - new NameMatcher("id", ImmutableList.of("X-Google-Id"), true))), + new NameMatcher("id", ImmutableList.of("X-Google-Id"), true)), + ExtraKeys.DEFAULT, + ImmutableMap.of()), new GrpcKeyBuilder( ImmutableList.of(new Name("service1")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"), true), - new NameMatcher("password", ImmutableList.of("Password"), true))), + new NameMatcher("password", ImmutableList.of("Password"), true)), + ExtraKeys.DEFAULT, + ImmutableMap.of()), new GrpcKeyBuilder( ImmutableList.of(new Name("service3")), ImmutableList.of( - new NameMatcher("user", ImmutableList.of("User", "Parent"), true)))), + new NameMatcher("user", ImmutableList.of("User", "Parent"), true)), + ExtraKeys.create("host-key", "service-key", "method-key"), + ImmutableMap.of("constKey1", "value1"))), /* lookupService= */ "service1", /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2), /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), diff --git a/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java b/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java index 8661e02334..b0d197ff52 100644 --- a/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java @@ -20,9 +20,11 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import io.grpc.Metadata; import io.grpc.Status.Code; import io.grpc.StatusRuntimeException; +import io.grpc.rls.RlsProtoData.ExtraKeys; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder; import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name; import io.grpc.rls.RlsProtoData.NameMatcher; @@ -43,21 +45,29 @@ public class RlsRequestFactoryTest { ImmutableList.of(new Name("com.google.service1", "Create")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"), true), - new NameMatcher("id", ImmutableList.of("X-Google-Id"), true))), + new NameMatcher("id", ImmutableList.of("X-Google-Id"), true)), + ExtraKeys.create("server-1", null, null), + ImmutableMap.of("const-key-1", "const-value-1")), new GrpcKeyBuilder( ImmutableList.of(new Name("com.google.service1")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"), true), - new NameMatcher("password", ImmutableList.of("Password"), true))), + new NameMatcher("password", ImmutableList.of("Password"), true)), + ExtraKeys.create(null, "service-2", null), + ImmutableMap.of("const-key-2", "const-value-2")), new GrpcKeyBuilder( ImmutableList.of(new Name("com.google.service2")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"), false), - new NameMatcher("password", ImmutableList.of("Password"), true))), + new NameMatcher("password", ImmutableList.of("Password"), true)), + ExtraKeys.create(null, "service-3", "method-3"), + ImmutableMap.of()), new GrpcKeyBuilder( ImmutableList.of(new Name("com.google.service3")), ImmutableList.of( - new NameMatcher("user", ImmutableList.of("User", "Parent"), true)))), + new NameMatcher("user", ImmutableList.of("User", "Parent"), true)), + ExtraKeys.create(null, null, null), + ImmutableMap.of("const-key-4", "const-value-4"))), /* lookupService= */ "bigtable-rls.googleapis.com", /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2), /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), @@ -77,10 +87,11 @@ public class RlsRequestFactoryTest { metadata.put(Metadata.Key.of("foo", Metadata.ASCII_STRING_MARSHALLER), "bar"); RouteLookupRequest request = factory.create("com.google.service1", "Create", metadata); - assertThat(request.getTargetType()).isEqualTo("grpc"); - assertThat(request.getPath()).isEqualTo("/com.google.service1/Create"); - assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com"); - assertThat(request.getKeyMap()).containsExactly("user", "test", "id", "123"); + assertThat(request.getKeyMap()).containsExactly( + "user", "test", + "id", "123", + "server-1", "bigtable.googleapis.com", + "const-key-1", "const-value-1"); } @Test @@ -106,10 +117,11 @@ public class RlsRequestFactoryTest { RouteLookupRequest request = factory.create("com.google.service1" , "Update", metadata); - assertThat(request.getTargetType()).isEqualTo("grpc"); - assertThat(request.getPath()).isEqualTo("/com.google.service1/Update"); - assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com"); - assertThat(request.getKeyMap()).containsExactly("user", "test", "password", "hunter2"); + assertThat(request.getKeyMap()).containsExactly( + "user", "test", + "password", "hunter2", + "service-2", "com.google.service1", + "const-key-2", "const-value-2"); } @Test @@ -121,10 +133,10 @@ public class RlsRequestFactoryTest { RouteLookupRequest request = factory.create("com.google.service1", "Update", metadata); - assertThat(request.getTargetType()).isEqualTo("grpc"); - assertThat(request.getPath()).isEqualTo("/com.google.service1/Update"); - assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com"); - assertThat(request.getKeyMap()).containsExactly("user", "test"); + assertThat(request.getKeyMap()).containsExactly( + "user", "test", + "service-2", "com.google.service1", + "const-key-2", "const-value-2"); } @Test @@ -135,10 +147,6 @@ public class RlsRequestFactoryTest { metadata.put(Metadata.Key.of("foo", Metadata.ASCII_STRING_MARSHALLER), "bar"); RouteLookupRequest request = factory.create("abc.def.service999", "Update", metadata); - - assertThat(request.getTargetType()).isEqualTo("grpc"); - assertThat(request.getPath()).isEqualTo("/abc.def.service999/Update"); - assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com"); assertThat(request.getKeyMap()).isEmpty(); } @@ -151,9 +159,7 @@ public class RlsRequestFactoryTest { RouteLookupRequest request = factory.create("com.google.service3", "Update", metadata); - assertThat(request.getTargetType()).isEqualTo("grpc"); - assertThat(request.getPath()).isEqualTo("/com.google.service3/Update"); - assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com"); - assertThat(request.getKeyMap()).containsExactly("user", "test"); + assertThat(request.getKeyMap()).containsExactly( + "user", "test", "const-key-4", "const-value-4"); } }