From 9f3db2337ffda523fd5703d1beaf72d8a8262eb7 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Tue, 2 Nov 2021 09:22:45 -0700 Subject: [PATCH 01/15] rls: overhall RouteLookupConfig validation --- .../main/java/io/grpc/internal/JsonUtil.java | 17 + .../java/io/grpc/rls/CachingRlsLbClient.java | 6 +- .../io/grpc/rls/RlsLoadBalancerProvider.java | 16 - .../java/io/grpc/rls/RlsProtoConverters.java | 100 +++-- .../main/java/io/grpc/rls/RlsProtoData.java | 131 ++---- .../java/io/grpc/rls/RlsRequestFactory.java | 6 - .../io/grpc/rls/CachingRlsLbClientTest.java | 15 +- .../java/io/grpc/rls/RlsLoadBalancerTest.java | 8 +- .../io/grpc/rls/RlsProtoConvertersTest.java | 419 +++++++++++++++++- .../java/io/grpc/rls/RlsProtoDataTest.java | 56 --- .../io/grpc/rls/RlsRequestFactoryTest.java | 31 +- 11 files changed, 539 insertions(+), 266 deletions(-) delete mode 100644 rls/src/test/java/io/grpc/rls/RlsProtoDataTest.java diff --git a/core/src/main/java/io/grpc/internal/JsonUtil.java b/core/src/main/java/io/grpc/internal/JsonUtil.java index d80b4ed44c4..e0eefcbbe72 100644 --- a/core/src/main/java/io/grpc/internal/JsonUtil.java +++ b/core/src/main/java/io/grpc/internal/JsonUtil.java @@ -182,6 +182,23 @@ public static Long getStringAsDuration(Map obj, String key) { } } + /** + * Gets a string from an object for the given key, parsed as a long integer. If + * the key is not present, this returns null. If the value is not a String or not properly + * formatted, throws an exception. + */ + public static Long getStringAsLong(Map obj, String key) { + String value = getString(obj, key); + if (value == null) { + return null; + } + try { + return Long.valueOf(value); + } catch (NumberFormatException e) { + throw new RuntimeException(e); + } + } + /** * Gets a boolean from an object for the given key. If the key is not present, this returns null. * If the value is not a Boolean, throws an exception. diff --git a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java index 87d01360f49..7bd9f4b68e3 100644 --- a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java +++ b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java @@ -128,9 +128,9 @@ private CachingRlsLbClient(Builder builder) { synchronizationContext = helper.getSynchronizationContext(); lbPolicyConfig = checkNotNull(builder.lbPolicyConfig, "lbPolicyConfig"); RouteLookupConfig rlsConfig = lbPolicyConfig.getRouteLookupConfig(); - maxAgeNanos = TimeUnit.MILLISECONDS.toNanos(rlsConfig.getMaxAgeInMillis()); - staleAgeNanos = TimeUnit.MILLISECONDS.toNanos(rlsConfig.getStaleAgeInMillis()); - callTimeoutNanos = TimeUnit.MILLISECONDS.toNanos(rlsConfig.getLookupServiceTimeoutInMillis()); + maxAgeNanos = rlsConfig.getMaxAgeInNanos(); + staleAgeNanos = rlsConfig.getStaleAgeInNanos(); + callTimeoutNanos = rlsConfig.getLookupServiceTimeoutInNanos(); timeProvider = checkNotNull(builder.timeProvider, "timeProvider"); throttler = checkNotNull(builder.throttler, "throttler"); linkedHashLruCache = diff --git a/rls/src/main/java/io/grpc/rls/RlsLoadBalancerProvider.java b/rls/src/main/java/io/grpc/rls/RlsLoadBalancerProvider.java index ca1ed714e82..3755fe3f77c 100644 --- a/rls/src/main/java/io/grpc/rls/RlsLoadBalancerProvider.java +++ b/rls/src/main/java/io/grpc/rls/RlsLoadBalancerProvider.java @@ -67,22 +67,6 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map rawLoadBalanc JsonUtil.getString(rawLoadBalancingConfigPolicy, "childPolicyConfigTargetFieldName"), JsonUtil.checkObjectList( checkNotNull(JsonUtil.getList(rawLoadBalancingConfigPolicy, "childPolicy")))); - // Checking all valid targets to make sure the config is always valid. This strict check - // prevents child policy to handle invalid child policy. - for (String validTarget : routeLookupConfig.getValidTargets()) { - ConfigOrError childPolicyConfigOrError = - lbPolicy - .getEffectiveLbProvider() - .parseLoadBalancingPolicyConfig(lbPolicy.getEffectiveChildPolicy(validTarget)); - if (childPolicyConfigOrError.getError() != null) { - return - ConfigOrError.fromError( - childPolicyConfigOrError - .getError() - .augmentDescription( - "failed to parse childPolicy for validTarget: " + validTarget)); - } - } return ConfigOrError.fromConfig(new LbPolicyConfiguration(routeLookupConfig, lbPolicy)); } catch (Exception e) { return ConfigOrError.fromError( diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index ce89def4467..6357ce505df 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -18,8 +18,10 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.base.Converter; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import io.grpc.internal.JsonUtil; import io.grpc.lookup.v1.RouteLookupRequest; @@ -30,8 +32,10 @@ import io.grpc.rls.RlsProtoData.NameMatcher; import io.grpc.rls.RlsProtoData.RouteLookupConfig; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; @@ -41,6 +45,9 @@ */ final class RlsProtoConverters { + private static final long MAX_AGE_NANOS = TimeUnit.MINUTES.toNanos(5); + private static final long MAX_CACHE_SIZE = 5 * 1024 * 1024; + /** * RouteLookupRequestConverter converts between {@link RouteLookupRequest} and {@link * RlsProtoData.RouteLookupRequest}. @@ -96,31 +103,47 @@ static final class RouteLookupConfigConverter @Override protected RouteLookupConfig doForward(Map json) { List grpcKeyBuilders = - GrpcKeyBuilderConverter - .covertAll(JsonUtil.checkObjectList(JsonUtil.getList(json, "grpcKeyBuilders"))); + GrpcKeyBuilderConverter.covertAll( + checkNotNull(JsonUtil.getListOfObjects(json, "grpcKeyBuilders"), "grpcKeyBuilders")); + checkArgument(!grpcKeyBuilders.isEmpty(), "must have at least one GrpcKeyBuilder"); + Set names = new HashSet<>(); + for (GrpcKeyBuilder keyBuilder : grpcKeyBuilders) { + for (Name name : keyBuilder.getNames()) { + checkArgument(names.add(name), "duplicate names in grpc_keybuilders: " + name); + } + } String lookupService = JsonUtil.getString(json, "lookupService"); - long timeout = - TimeUnit.SECONDS.toMillis( - orDefault( - JsonUtil.getNumberAsLong(json, "lookupServiceTimeout"), - 0L)); - Long maxAge = - convertTimeIfNotNull( - TimeUnit.SECONDS, TimeUnit.MILLISECONDS, JsonUtil.getNumberAsLong(json, "maxAge")); - Long staleAge = - convertTimeIfNotNull( - TimeUnit.SECONDS, TimeUnit.MILLISECONDS, JsonUtil.getNumberAsLong(json, "staleAge")); - long cacheSize = orDefault(JsonUtil.getNumberAsLong(json, "cacheSizeBytes"), Long.MAX_VALUE); - List validTargets = JsonUtil.checkStringList(JsonUtil.getList(json, "validTargets")); + // TODO(creamsoup) also check if it is URI + checkArgument(!Strings.isNullOrEmpty(lookupService), "lookupService must not be empty"); + long timeout = orDefault( + JsonUtil.getStringAsDuration(json, "lookupServiceTimeout"), + SECONDS.toNanos(10)); + checkArgument(timeout > 0, "lookupServiceTimeout should be positive"); + Long maxAge = JsonUtil.getStringAsDuration(json, "maxAge"); + Long staleAge = JsonUtil.getStringAsDuration(json, "staleAge"); + if (maxAge == null) { + checkArgument(staleAge == null, "to specify staleAge, must have maxAge"); + maxAge = MAX_AGE_NANOS; + } + if (staleAge == null) { + staleAge = MAX_AGE_NANOS; + } + maxAge = Math.min(maxAge, MAX_AGE_NANOS); + staleAge = Math.min(staleAge, maxAge); + long cacheSize = orDefault(JsonUtil.getStringAsLong(json, "cacheSizeBytes"), MAX_CACHE_SIZE); + checkArgument(cacheSize > 0, "cacheSize must be positive"); + cacheSize = Math.min(cacheSize, MAX_CACHE_SIZE); String defaultTarget = JsonUtil.getString(json, "defaultTarget"); + if (Strings.isNullOrEmpty(defaultTarget)) { + defaultTarget = null; + } return new RouteLookupConfig( grpcKeyBuilders, lookupService, - /* lookupServiceTimeoutInMillis= */ timeout, - /* maxAgeInMillis= */ maxAge, - /* staleAgeInMillis= */ staleAge, + /* lookupServiceTimeoutInNanos= */ timeout, + /* maxAgeInNanos= */ maxAge, + /* staleAgeInNanos= */ staleAge, /* cacheSizeBytes= */ cacheSize, - validTargets, defaultTarget); } @@ -131,13 +154,6 @@ private static T orDefault(@Nullable T value, T defaultValue) { return value; } - private static Long convertTimeIfNotNull(TimeUnit from, TimeUnit to, Long value) { - if (value == null) { - return null; - } - return to.convert(value, from); - } - @Override protected Map doBackward(RouteLookupConfig routeLookupConfig) { throw new UnsupportedOperationException(); @@ -155,10 +171,16 @@ public static List covertAll(List> keyBuilders) { @SuppressWarnings("unchecked") static GrpcKeyBuilder convert(Map keyBuilder) { + List rawRawNames = JsonUtil.getList(keyBuilder, "names"); + checkArgument( + rawRawNames != null && !rawRawNames.isEmpty(), + "each keyBuilder must have at least one name"); List> rawNames = JsonUtil.checkObjectList(JsonUtil.getList(keyBuilder, "names")); List names = new ArrayList<>(); for (Map rawName : rawNames) { + String serviceName = JsonUtil.getString(rawName, "service"); + checkArgument(!Strings.isNullOrEmpty(serviceName), "service must not be empty or null"); names.add( new Name( JsonUtil.getString(rawName, "service"), JsonUtil.getString(rawName, "method"))); @@ -167,13 +189,12 @@ static GrpcKeyBuilder convert(Map keyBuilder) { JsonUtil.checkObjectList(JsonUtil.getList(keyBuilder, "headers")); List nameMatchers = new ArrayList<>(); for (Map rawHeader : rawHeaders) { - NameMatcher matcher = - new NameMatcher( - JsonUtil.getString(rawHeader, "key"), - (List) rawHeader.get("names"), - (Boolean) rawHeader.get("optional")); + Boolean requiredMatch = JsonUtil.getBoolean(rawHeader, "requiredMatch"); checkArgument( - matcher.isOptional(), "NameMatcher for GrpcKeyBuilders shouldn't be required"); + requiredMatch == null || !requiredMatch, + "requiredMatch shouldn't be specified for gRPC"); + NameMatcher matcher = new NameMatcher( + JsonUtil.getString(rawHeader, "key"), (List) rawHeader.get("names")); nameMatchers.add(matcher); } ExtraKeys extraKeys = ExtraKeys.DEFAULT; @@ -188,9 +209,24 @@ static GrpcKeyBuilder convert(Map keyBuilder) { if (constantKeys == null) { constantKeys = ImmutableMap.of(); } + checkUniqueKey(nameMatchers, constantKeys.keySet()); return new GrpcKeyBuilder(names, nameMatchers, extraKeys, constantKeys); } } + private static void checkUniqueKey(List nameMatchers, Set constantKeys) { + Set keys = new HashSet<>(); + keys.addAll(constantKeys); + keys.add("host"); + keys.add("service"); + keys.add("method"); + for (NameMatcher nameMatcher : nameMatchers) { + keys.add(nameMatcher.getKey()); + } + if (keys.size() != nameMatchers.size() + constantKeys.size() + 3) { + throw new IllegalArgumentException("keys in KeyBuilder must be unique"); + } + } + private RlsProtoConverters() {} } diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoData.java b/rls/src/main/java/io/grpc/rls/RlsProtoData.java index 5192e9cf075..9e62f228a1f 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoData.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoData.java @@ -16,7 +16,6 @@ package io.grpc.rls; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; @@ -26,12 +25,8 @@ 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; import java.util.Map; -import java.util.Set; -import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; @@ -144,23 +139,18 @@ public String toString() { @Immutable public 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; - private final ImmutableList grpcKeyBuilders; private final String lookupService; - private final long lookupServiceTimeoutInMillis; + private final long lookupServiceTimeoutInNanos; - private final long maxAgeInMillis; + private final long maxAgeInNanos; - private final long staleAgeInMillis; + private final long staleAgeInNanos; private final long cacheSizeBytes; - private final ImmutableList validTargets; - @Nullable private final String defaultTarget; @@ -168,40 +158,18 @@ public static final class RouteLookupConfig { public RouteLookupConfig( List grpcKeyBuilders, String lookupService, - long lookupServiceTimeoutInMillis, - @Nullable Long maxAgeInMillis, - @Nullable Long staleAgeInMillis, + long lookupServiceTimeoutInNanos, + @Nullable Long maxAgeInNanos, + @Nullable Long staleAgeInNanos, long cacheSizeBytes, - List validTargets, @Nullable String defaultTarget) { - checkState( - !checkNotNull(grpcKeyBuilders, "grpcKeyBuilders").isEmpty(), - "must have at least one GrpcKeyBuilder"); - checkUniqueName(grpcKeyBuilders); this.grpcKeyBuilders = ImmutableList.copyOf(grpcKeyBuilders); - // TODO(creamsoup) also check if it is URI - checkState( - lookupService != null && !lookupService.isEmpty(), "lookupService must not be empty"); this.lookupService = lookupService; - checkState( - lookupServiceTimeoutInMillis > 0, "lookupServiceTimeoutInMillis should be positive"); - this.lookupServiceTimeoutInMillis = lookupServiceTimeoutInMillis; - if (maxAgeInMillis == null) { - checkState( - staleAgeInMillis == null, "To specify staleAgeInMillis, must have maxAgeInMillis"); - } - if (maxAgeInMillis == null || maxAgeInMillis == 0) { - maxAgeInMillis = MAX_AGE_MILLIS; - } - if (staleAgeInMillis == null || staleAgeInMillis == 0) { - staleAgeInMillis = MAX_AGE_MILLIS; - } - this.maxAgeInMillis = Math.min(maxAgeInMillis, MAX_AGE_MILLIS); - this.staleAgeInMillis = Math.min(staleAgeInMillis, this.maxAgeInMillis); - checkArgument(cacheSizeBytes > 0, "cacheSize must be positive"); - this.cacheSizeBytes = Math.min(cacheSizeBytes, MAX_CACHE_SIZE); - this.validTargets = ImmutableList.copyOf(checkNotNull(validTargets, "validTargets")); + this.lookupServiceTimeoutInNanos = lookupServiceTimeoutInNanos; + this.maxAgeInNanos = maxAgeInNanos; + this.staleAgeInNanos = staleAgeInNanos; + this.cacheSizeBytes = cacheSizeBytes; if (defaultTarget != null && defaultTarget.isEmpty()) { defaultTarget = null; } @@ -227,22 +195,22 @@ String getLookupService() { } /** Returns the timeout value for lookup service requests. */ - long getLookupServiceTimeoutInMillis() { - return lookupServiceTimeoutInMillis; + long getLookupServiceTimeoutInNanos() { + return lookupServiceTimeoutInNanos; } /** Returns the maximum age the result will be cached. */ - long getMaxAgeInMillis() { - return maxAgeInMillis; + long getMaxAgeInNanos() { + return maxAgeInNanos; } /** * Returns the time when an entry will be in a staled status. When cache is accessed whgen the * entry is in staled status, it will */ - long getStaleAgeInMillis() { - return staleAgeInMillis; + long getStaleAgeInNanos() { + return staleAgeInNanos; } /** @@ -255,15 +223,6 @@ long getCacheSizeBytes() { return cacheSizeBytes; } - /** - * Returns the list of all the possible targets that can be returned by the lookup service. If - * a target not on this list is returned, it will be treated the same as an RPC error from the - * RLS. - */ - ImmutableList getValidTargets() { - return validTargets; - } - /** * Returns the default target to use if needed. If nonempty (implies request processing * strategy SYNC_LOOKUP_DEFAULT_TARGET_ON_ERROR is set), it will be used if RLS returns an @@ -284,9 +243,9 @@ public boolean equals(Object o) { return false; } RouteLookupConfig that = (RouteLookupConfig) o; - return lookupServiceTimeoutInMillis == that.lookupServiceTimeoutInMillis - && maxAgeInMillis == that.maxAgeInMillis - && staleAgeInMillis == that.staleAgeInMillis + return lookupServiceTimeoutInNanos == that.lookupServiceTimeoutInNanos + && maxAgeInNanos == that.maxAgeInNanos + && staleAgeInNanos == that.staleAgeInNanos && cacheSizeBytes == that.cacheSizeBytes && Objects.equal(grpcKeyBuilders, that.grpcKeyBuilders) && Objects.equal(lookupService, that.lookupService) @@ -298,9 +257,9 @@ public int hashCode() { return Objects.hashCode( grpcKeyBuilders, lookupService, - lookupServiceTimeoutInMillis, - maxAgeInMillis, - staleAgeInMillis, + lookupServiceTimeoutInNanos, + maxAgeInNanos, + staleAgeInNanos, cacheSizeBytes, defaultTarget); } @@ -310,26 +269,15 @@ public String toString() { return MoreObjects.toStringHelper(this) .add("grpcKeyBuilders", grpcKeyBuilders) .add("lookupService", lookupService) - .add("lookupServiceTimeoutInMillis", lookupServiceTimeoutInMillis) - .add("maxAgeInMillis", maxAgeInMillis) - .add("staleAgeInMillis", staleAgeInMillis) + .add("lookupServiceTimeoutInNanos", lookupServiceTimeoutInNanos) + .add("maxAgeInNanos", maxAgeInNanos) + .add("staleAgeInNanos", staleAgeInNanos) .add("cacheSize", cacheSizeBytes) .add("defaultTarget", defaultTarget) .toString(); } } - private static void checkUniqueName(List grpcKeyBuilders) { - Set names = new HashSet<>(); - for (GrpcKeyBuilder grpcKeyBuilder : grpcKeyBuilders) { - int prevSize = names.size(); - names.addAll(grpcKeyBuilder.getNames()); - if (names.size() != prevSize + grpcKeyBuilder.getNames().size()) { - throw new IllegalStateException("Names in the GrpcKeyBuilders should be unique"); - } - } - } - /** * NameMatcher extract a key based on a given name (e.g. header name or query parameter name). * The name must match one of the names listed in the "name" field. If the "required_match" field @@ -342,13 +290,10 @@ public static final class NameMatcher { private final ImmutableList names; - private final boolean optional; - /** Constructor. */ - public NameMatcher(String key, List names, @Nullable Boolean optional) { + public NameMatcher(String key, List names) { this.key = checkNotNull(key, "key"); this.names = ImmutableList.copyOf(checkNotNull(names, "names")); - this.optional = optional != null ? optional : true; } /** The name that will be used in the RLS key_map to refer to this value. */ @@ -361,13 +306,6 @@ ImmutableList names() { return names; } - /** - * Indicates if this extraction optional. A key builder will still match if no value is found. - */ - boolean isOptional() { - return optional; - } - @Override public boolean equals(Object o) { if (this == o) { @@ -377,14 +315,13 @@ public boolean equals(Object o) { return false; } NameMatcher matcher = (NameMatcher) o; - return optional == matcher.optional - && java.util.Objects.equals(key, matcher.key) + return java.util.Objects.equals(key, matcher.key) && java.util.Objects.equals(names, matcher.names); } @Override public int hashCode() { - return java.util.Objects.hash(key, names, optional); + return java.util.Objects.hash(key, names); } @Override @@ -392,7 +329,6 @@ public String toString() { return MoreObjects.toStringHelper(this) .add("key", key) .add("names", names) - .add("optional", optional) .toString(); } } @@ -412,19 +348,11 @@ public GrpcKeyBuilder( 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) { - Set names = new HashSet<>(); - for (NameMatcher header : headers) { - checkState(names.add(header.key), "key in headers must be unique"); - } - } - /** * Returns names. To match, one of the given Name fields must match; the service and method * fields are specified as fixed strings. The service name is required and includes the proto @@ -498,9 +426,6 @@ public Name(String service) { /** The primary constructor. */ public Name(String service, String method) { - checkState( - !checkNotNull(service, "service").isEmpty(), - "service must not be empty or null"); this.service = service; this.method = method; } diff --git a/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java b/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java index e181d64833d..d3aecabd034 100644 --- a/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java +++ b/rls/src/main/java/io/grpc/rls/RlsRequestFactory.java @@ -21,8 +21,6 @@ import com.google.common.base.MoreObjects; 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; @@ -107,10 +105,6 @@ private Map createRequestHeaders( } if (value != null) { rlsRequestHeaders.put(nameMatcher.getKey(), value); - } else if (!nameMatcher.isOptional()) { - throw new StatusRuntimeException( - Status.INVALID_ARGUMENT.withDescription( - 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 aa64ec890b6..c8222a02b8a 100644 --- a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java +++ b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java @@ -210,7 +210,7 @@ public void get_noError_lifeCycle() throws Exception { assertThat(resp.hasData()).isTrue(); // cache hit for staled entry - fakeTimeProvider.forwardTime(ROUTE_LOOKUP_CONFIG.getStaleAgeInMillis(), TimeUnit.MILLISECONDS); + fakeTimeProvider.forwardTime(ROUTE_LOOKUP_CONFIG.getStaleAgeInNanos(), TimeUnit.NANOSECONDS); resp = getInSyncContext(routeLookupRequest); assertThat(resp.hasData()).isTrue(); @@ -226,7 +226,7 @@ public void get_noError_lifeCycle() throws Exception { assertThat(resp.hasData()).isTrue(); // existing cache expired - fakeTimeProvider.forwardTime(ROUTE_LOOKUP_CONFIG.getMaxAgeInMillis(), TimeUnit.MILLISECONDS); + fakeTimeProvider.forwardTime(ROUTE_LOOKUP_CONFIG.getMaxAgeInNanos(), TimeUnit.NANOSECONDS); resp = getInSyncContext(routeLookupRequest); @@ -423,16 +423,15 @@ private static RouteLookupConfig getRouteLookupConfig() { new GrpcKeyBuilder( 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("user", ImmutableList.of("User", "Parent")), + new NameMatcher("id", ImmutableList.of("X-Google-Id"))), ExtraKeys.create("server", "service-key", "method-key"), ImmutableMap.of())), /* lookupService= */ "service1", - /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2), - /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), - /* staleAgeInMillis= */ TimeUnit.SECONDS.toMillis(240), + /* lookupServiceTimeoutInNanos= */ TimeUnit.SECONDS.toNanos(2), + /* maxAgeInNanos= */ TimeUnit.SECONDS.toNanos(300), + /* staleAgeInNanos= */ TimeUnit.SECONDS.toNanos(240), /* cacheSizeBytes= */ 1000, - /* validTargets= */ ImmutableList.of("a valid target"), DEFAULT_TARGET); } diff --git a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java index fba295f98f0..bf0bc47fcc4 100644 --- a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java @@ -420,11 +420,11 @@ private String getRlsConfigJsonStr() { + " }\n" + " ],\n" + " \"lookupService\": \"localhost:8972\",\n" - + " \"lookupServiceTimeout\": 2,\n" - + " \"maxAge\": 300,\n" - + " \"staleAge\": 240,\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"maxAge\": \"300s\",\n" + + " \"staleAge\": \"240s\",\n" + " \"validTargets\": [\"localhost:9001\", \"localhost:9002\"]," - + " \"cacheSizeBytes\": 1000,\n" + + " \"cacheSizeBytes\": \"1000\",\n" + " \"defaultTarget\": \"" + defaultTarget + "\",\n" + " \"requestProcessingStrategy\": \"SYNC_LOOKUP_DEFAULT_TARGET_ON_ERROR\"\n" + "}"; diff --git a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java index bfb331e6cc8..5efc036175e 100644 --- a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java @@ -17,6 +17,7 @@ package io.grpc.rls; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; import com.google.common.base.Converter; import com.google.common.collect.ImmutableList; @@ -166,11 +167,11 @@ public void convert_jsonRlsConfig() throws IOException { + " }\n" + " ],\n" + " \"lookupService\": \"service1\",\n" - + " \"lookupServiceTimeout\": 2,\n" - + " \"maxAge\": 300,\n" - + " \"staleAge\": 240,\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"maxAge\": \"300s\",\n" + + " \"staleAge\": \"240s\",\n" + " \"validTargets\": [\"a valid target\"]," - + " \"cacheSizeBytes\": 1000,\n" + + " \"cacheSizeBytes\": \"1000\",\n" + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + "}"; @@ -180,29 +181,28 @@ public void convert_jsonRlsConfig() throws IOException { new GrpcKeyBuilder( 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("user", ImmutableList.of("User", "Parent")), + new NameMatcher("id", ImmutableList.of("X-Google-Id"))), 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("user", ImmutableList.of("User", "Parent")), + new NameMatcher("password", ImmutableList.of("Password"))), 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"))), ExtraKeys.create("host-key", "service-key", "method-key"), ImmutableMap.of("constKey1", "value1"))), /* lookupService= */ "service1", - /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2), - /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), - /* staleAgeInMillis= */ TimeUnit.SECONDS.toMillis(240), + /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toNanos(2), + /* maxAgeInNanos= */ TimeUnit.SECONDS.toNanos(300), + /* staleAgeInNanos= */ TimeUnit.SECONDS.toNanos(240), /* cacheSizeBytes= */ 1000, - /* validTargets= */ ImmutableList.of("a valid target"), /* defaultTarget= */ "us_east_1.cloudbigtable.googleapis.com"); RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); @@ -211,4 +211,397 @@ public void convert_jsonRlsConfig() throws IOException { RouteLookupConfig converted = converter.convert(parsedJson); assertThat(converted).isEqualTo(expectedConfig); } + + @Test + public void convert_jsonRlsConfig_emptyKeyBuilders() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [],\n" + + " \"lookupService\": \"service1\",\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"maxAge\": \"300s\",\n" + + " \"staleAge\": \"240s\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"cacheSizeBytes\": \"1000\",\n" + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + try { + converter.convert(parsedJson); + fail("Exception expected"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains("must have at least one GrpcKeyBuilder"); + } + } + + @Test + public void convert_jsonRlsConfig_namesNotUnique() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"service\": \"service1\",\n" + + " \"method\": \"create\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"id\"," + + " \"names\": [\"X-Google-Id\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " },\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"service\": \"service1\",\n" + + " \"method\": \"create\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"password\"," + + " \"names\": [\"Password\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " },\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"service\": \"service3\",\n" + + " \"method\": \"*\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\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" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"maxAge\": \"300s\",\n" + + " \"staleAge\": \"240s\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"cacheSizeBytes\": \"1000\",\n" + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + try { + converter.convert(parsedJson); + fail("Exception expected"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat() + .contains("duplicate names in grpc_keybuilders: Name{service=service1, method=create}"); + } + } + + @Test + public void convert_jsonRlsConfig_defaultValues() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"service\": \"service1\",\n" + + " \"method\": \"create\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"id\"," + + " \"names\": [\"X-Google-Id\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " }\n" + + " ],\n" + + " \"lookupService\": \"service1\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfig expectedConfig = + new RouteLookupConfig( + ImmutableList.of( + new GrpcKeyBuilder( + ImmutableList.of(new Name("service1", "create")), + ImmutableList.of( + new NameMatcher("user", ImmutableList.of("User", "Parent")), + new NameMatcher("id", ImmutableList.of("X-Google-Id"))), + ExtraKeys.DEFAULT, + ImmutableMap.of())), + /* lookupService= */ "service1", + /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toNanos(10), + /* maxAgeInNanos= */ TimeUnit.MINUTES.toNanos(5), + /* staleAgeInNanos= */ TimeUnit.MINUTES.toNanos(5), + /* cacheSizeBytes= */ 5 * 1024 * 1024, + /* defaultTarget= */ "us_east_1.cloudbigtable.googleapis.com"); + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + RouteLookupConfig converted = converter.convert(parsedJson); + assertThat(converted).isEqualTo(expectedConfig); + } + + @Test + public void convert_jsonRlsConfig_staleAgeCappedByMaxAge() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"service\": \"service1\",\n" + + " \"method\": \"create\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"id\"," + + " \"names\": [\"X-Google-Id\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " }\n" + + " ],\n" + + " \"lookupService\": \"service1\",\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"maxAge\": \"300s\",\n" + + " \"staleAge\": \"400s\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"cacheSizeBytes\": \"1000\",\n" + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfig expectedConfig = + new RouteLookupConfig( + ImmutableList.of( + new GrpcKeyBuilder( + ImmutableList.of(new Name("service1", "create")), + ImmutableList.of( + new NameMatcher("user", ImmutableList.of("User", "Parent")), + new NameMatcher("id", ImmutableList.of("X-Google-Id"))), + ExtraKeys.DEFAULT, + ImmutableMap.of())), + /* lookupService= */ "service1", + /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toNanos(2), + /* maxAgeInNanos= */ TimeUnit.SECONDS.toNanos(300), + /* staleAgeInNanos= */ TimeUnit.SECONDS.toNanos(300), + /* cacheSizeBytes= */ 1000, + /* defaultTarget= */ "us_east_1.cloudbigtable.googleapis.com"); + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + RouteLookupConfig converted = converter.convert(parsedJson); + assertThat(converted).isEqualTo(expectedConfig); + } + + @Test + public void convert_jsonRlsConfig_staleAgeGivenWithoutMaxAge() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"service\": \"service1\",\n" + + " \"method\": \"create\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"id\"," + + " \"names\": [\"X-Google-Id\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " }\n" + + " ],\n" + + " \"lookupService\": \"service1\",\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"staleAge\": \"240s\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"cacheSizeBytes\": \"1000\",\n" + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + try { + converter.convert(parsedJson); + fail("Exception expected"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains("to specify staleAge, must have maxAge"); + } + } + + @Test + public void convert_jsonRlsConfig_keyBuilderWithoutName() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [\n" + + " {\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"id\"," + + " \"names\": [\"X-Google-Id\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " }\n" + + " ],\n" + + " \"lookupService\": \"service1\",\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"staleAge\": \"240s\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"cacheSizeBytes\": \"1000\",\n" + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + try { + converter.convert(parsedJson); + fail("Exception expected"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains("each keyBuilder must have at least one name"); + } + } + + @Test + public void convert_jsonRlsConfig_nameWithoutService() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"method\": \"create\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"user\"," + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"id\"," + + " \"names\": [\"X-Google-Id\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " }\n" + + " ],\n" + + " \"lookupService\": \"service1\",\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"staleAge\": \"240s\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"cacheSizeBytes\": \"1000\",\n" + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + try { + converter.convert(parsedJson); + fail("Exception expected"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains("service must not be empty or null"); + } + } + + @Test + public void convert_jsonRlsConfig_keysNotUnique() throws IOException { + String jsonStr = "{\n" + + " \"grpcKeyBuilders\": [\n" + + " {\n" + + " \"names\": [\n" + + " {\n" + + " \"service\": \"service1\"\n" + + " }\n" + + " ],\n" + + " \"headers\": [\n" + + " {\n" + + " \"key\": \"service\"," // duplicate to extra_keys + + " \"names\": [\"User\", \"Parent\"],\n" + + " \"optional\": true\n" + + " },\n" + + " {\n" + + " \"key\": \"id\"," + + " \"names\": [\"X-Google-Id\"],\n" + + " \"optional\": true\n" + + " }\n" + + " ]\n" + + " }\n" + + " ],\n" + + " \"lookupService\": \"service1\",\n" + + " \"lookupServiceTimeout\": \"2s\",\n" + + " \"staleAge\": \"240s\",\n" + + " \"validTargets\": [\"a valid target\"]," + + " \"cacheSizeBytes\": \"1000\",\n" + + " \"defaultTarget\": \"us_east_1.cloudbigtable.googleapis.com\"\n" + + "}"; + + RouteLookupConfigConverter converter = new RouteLookupConfigConverter(); + @SuppressWarnings("unchecked") + Map parsedJson = (Map) JsonParser.parse(jsonStr); + try { + converter.convert(parsedJson); + fail("Exception expected"); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains("keys in KeyBuilder must be unique"); + } + } } diff --git a/rls/src/test/java/io/grpc/rls/RlsProtoDataTest.java b/rls/src/test/java/io/grpc/rls/RlsProtoDataTest.java deleted file mode 100644 index 9cfb6c753fb..00000000000 --- a/rls/src/test/java/io/grpc/rls/RlsProtoDataTest.java +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright 2021 The gRPC Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.grpc.rls; - -import static com.google.common.truth.Truth.assertThat; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -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 java.util.concurrent.TimeUnit; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link RlsProtoData}. */ -@RunWith(JUnit4.class) -public class RlsProtoDataTest { - @Test - public void maxCacheSize() { - RouteLookupConfig config = new RouteLookupConfig( - ImmutableList.of( - new GrpcKeyBuilder( - 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)), - ExtraKeys.create("server", "service-key", "method-key"), - ImmutableMap.of())), - "service1", - /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2), - /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), - /* staleAgeInMillis= */ TimeUnit.SECONDS.toMillis(240), - /* cacheSizeBytes= */ 20 * 1000 * 1000, - ImmutableList.of("a-valid-target"), - "default-target"); - assertThat(config.getCacheSizeBytes()).isEqualTo(5 * 1024 * 1024); - } -} diff --git a/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java b/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java index b0d197ff525..641b0c65e75 100644 --- a/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java @@ -17,13 +17,10 @@ package io.grpc.rls; 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; @@ -44,28 +41,27 @@ public class RlsRequestFactoryTest { new GrpcKeyBuilder( 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("user", ImmutableList.of("User", "Parent")), + new NameMatcher("id", ImmutableList.of("X-Google-Id"))), 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("user", ImmutableList.of("User", "Parent")), + new NameMatcher("password", ImmutableList.of("Password"))), 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"))), 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"))), ExtraKeys.create(null, null, null), ImmutableMap.of("const-key-4", "const-value-4"))), /* lookupService= */ "bigtable-rls.googleapis.com", @@ -73,7 +69,6 @@ public class RlsRequestFactoryTest { /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), /* staleAgeInMillis= */ TimeUnit.SECONDS.toMillis(240), /* cacheSizeBytes= */ 1000, - /* validTargets= */ ImmutableList.of("a valid target"), /* defaultTarget= */ "us_east_1.cloudbigtable.googleapis.com"); private final RlsRequestFactory factory = new RlsRequestFactory( @@ -94,20 +89,6 @@ public void create_pathMatches() { "const-key-1", "const-value-1"); } - @Test - public void create_missingRequiredHeader() { - Metadata metadata = new Metadata(); - - try { - RouteLookupRequest unused = factory.create("com.google.service2", "Create", metadata); - fail(); - } catch (StatusRuntimeException e) { - assertThat(e.getStatus().getCode()).isEqualTo(Code.INVALID_ARGUMENT); - assertThat(e.getStatus().getDescription()) - .isEqualTo("Missing mandatory metadata(user) not found"); - } - } - @Test public void create_pathFallbackMatches() { Metadata metadata = new Metadata(); From dc2e72e037d4efa77cc06f9050b8e83e8743680e Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Thu, 18 Nov 2021 10:29:27 -0800 Subject: [PATCH 02/15] remove check in constructor --- rls/src/main/java/io/grpc/rls/RlsProtoData.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoData.java b/rls/src/main/java/io/grpc/rls/RlsProtoData.java index 2ddf00db207..747e0eba39f 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoData.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoData.java @@ -167,9 +167,6 @@ static final class RouteLookupConfig { this.maxAgeInNanos = maxAgeInNanos; this.staleAgeInNanos = staleAgeInNanos; this.cacheSizeBytes = cacheSizeBytes; - if (defaultTarget != null && defaultTarget.isEmpty()) { - defaultTarget = null; - } this.defaultTarget = defaultTarget; } From 3b1f2a3f888fa0df24c0b96c5d21b27da082d92e Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Thu, 18 Nov 2021 10:55:38 -0800 Subject: [PATCH 03/15] validate URI --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index 6357ce505df..55ac78e0338 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -31,6 +31,7 @@ import io.grpc.rls.RlsProtoData.GrpcKeyBuilder.Name; import io.grpc.rls.RlsProtoData.NameMatcher; import io.grpc.rls.RlsProtoData.RouteLookupConfig; +import java.net.URI; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -113,8 +114,8 @@ protected RouteLookupConfig doForward(Map json) { } } String lookupService = JsonUtil.getString(json, "lookupService"); - // TODO(creamsoup) also check if it is URI checkArgument(!Strings.isNullOrEmpty(lookupService), "lookupService must not be empty"); + URI.create(lookupService); // Will throw IllegalArgumentException if it's not a valid URI. long timeout = orDefault( JsonUtil.getStringAsDuration(json, "lookupServiceTimeout"), SECONDS.toNanos(10)); From d1b0839b6fcde48f296deda6504687f795c9547e Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Fri, 19 Nov 2021 10:24:58 -0800 Subject: [PATCH 04/15] reset JsonUtil.java --- .../main/java/io/grpc/internal/JsonUtil.java | 17 ----------------- .../java/io/grpc/rls/RlsProtoConverters.java | 2 +- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/JsonUtil.java b/core/src/main/java/io/grpc/internal/JsonUtil.java index d505e774664..117135fe634 100644 --- a/core/src/main/java/io/grpc/internal/JsonUtil.java +++ b/core/src/main/java/io/grpc/internal/JsonUtil.java @@ -217,23 +217,6 @@ public static Long getStringAsDuration(Map obj, String key) { } } - /** - * Gets a string from an object for the given key, parsed as a long integer. If - * the key is not present, this returns null. If the value is not a String or not properly - * formatted, throws an exception. - */ - public static Long getStringAsLong(Map obj, String key) { - String value = getString(obj, key); - if (value == null) { - return null; - } - try { - return Long.valueOf(value); - } catch (NumberFormatException e) { - throw new RuntimeException(e); - } - } - /** * Gets a boolean from an object for the given key. If the key is not present, this returns null. * If the value is not a Boolean, throws an exception. diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index 55ac78e0338..da3d52fe0fb 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -131,7 +131,7 @@ protected RouteLookupConfig doForward(Map json) { } maxAge = Math.min(maxAge, MAX_AGE_NANOS); staleAge = Math.min(staleAge, maxAge); - long cacheSize = orDefault(JsonUtil.getStringAsLong(json, "cacheSizeBytes"), MAX_CACHE_SIZE); + long cacheSize = orDefault(JsonUtil.getNumberAsLong(json, "cacheSizeBytes"), MAX_CACHE_SIZE); checkArgument(cacheSize > 0, "cacheSize must be positive"); cacheSize = Math.min(cacheSize, MAX_CACHE_SIZE); String defaultTarget = JsonUtil.getString(json, "defaultTarget"); From 2fb3ad65cf1de56574f475abf1ede09c84e6b08e Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 09:40:50 -0800 Subject: [PATCH 05/15] add comment 5MiB --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index da3d52fe0fb..01fa739c4ee 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -47,7 +47,7 @@ final class RlsProtoConverters { private static final long MAX_AGE_NANOS = TimeUnit.MINUTES.toNanos(5); - private static final long MAX_CACHE_SIZE = 5 * 1024 * 1024; + private static final long MAX_CACHE_SIZE = 5 * 1024 * 1024; // 5MiB /** * RouteLookupRequestConverter converts between {@link RouteLookupRequest} and {@link From b83406986dab3972ca162b40b32ac27af074af72 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 09:58:35 -0800 Subject: [PATCH 06/15] use URI constructor --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index 01fa739c4ee..d93d6032fa1 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -32,6 +32,7 @@ import io.grpc.rls.RlsProtoData.NameMatcher; import io.grpc.rls.RlsProtoData.RouteLookupConfig; import java.net.URI; +import java.net.URISyntaxException; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -115,7 +116,12 @@ protected RouteLookupConfig doForward(Map json) { } String lookupService = JsonUtil.getString(json, "lookupService"); checkArgument(!Strings.isNullOrEmpty(lookupService), "lookupService must not be empty"); - URI.create(lookupService); // Will throw IllegalArgumentException if it's not a valid URI. + try { + new URI(lookupService); + } catch (URISyntaxException x) { + throw new IllegalArgumentException( + "The lookupService field is not valid URI: " + lookupService, x); + } long timeout = orDefault( JsonUtil.getStringAsDuration(json, "lookupServiceTimeout"), SECONDS.toNanos(10)); From 28d1270b4d1ba72170d666c4bc4da190d93ed31b Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 10:04:35 -0800 Subject: [PATCH 07/15] DEFAULT_LOOKUP_SERVICE_TIMEOUT --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index d93d6032fa1..cc119aca594 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static java.util.concurrent.TimeUnit.MINUTES; import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.base.Converter; @@ -38,7 +39,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; /** @@ -47,8 +47,9 @@ */ final class RlsProtoConverters { - private static final long MAX_AGE_NANOS = TimeUnit.MINUTES.toNanos(5); + private static final long MAX_AGE_NANOS = MINUTES.toNanos(5); private static final long MAX_CACHE_SIZE = 5 * 1024 * 1024; // 5MiB + private static final long DEFAULT_LOOKUP_SERVICE_TIMEOUT = SECONDS.toNanos(10); /** * RouteLookupRequestConverter converts between {@link RouteLookupRequest} and {@link @@ -124,7 +125,7 @@ protected RouteLookupConfig doForward(Map json) { } long timeout = orDefault( JsonUtil.getStringAsDuration(json, "lookupServiceTimeout"), - SECONDS.toNanos(10)); + DEFAULT_LOOKUP_SERVICE_TIMEOUT); checkArgument(timeout > 0, "lookupServiceTimeout should be positive"); Long maxAge = JsonUtil.getStringAsDuration(json, "maxAge"); Long staleAge = JsonUtil.getStringAsDuration(json, "staleAge"); From 1e92dc4be82a2a10c31db65329a7fdbe25757ee0 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 10:07:07 -0800 Subject: [PATCH 08/15] Strings.emptyToNull() --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index cc119aca594..adf31776db7 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -141,10 +141,7 @@ protected RouteLookupConfig doForward(Map json) { long cacheSize = orDefault(JsonUtil.getNumberAsLong(json, "cacheSizeBytes"), MAX_CACHE_SIZE); checkArgument(cacheSize > 0, "cacheSize must be positive"); cacheSize = Math.min(cacheSize, MAX_CACHE_SIZE); - String defaultTarget = JsonUtil.getString(json, "defaultTarget"); - if (Strings.isNullOrEmpty(defaultTarget)) { - defaultTarget = null; - } + String defaultTarget = Strings.emptyToNull(JsonUtil.getString(json, "defaultTarget")); return new RouteLookupConfig( grpcKeyBuilders, lookupService, From 357f4313809c5da125272a1b36c32400b33dbb0c Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 13:16:22 -0800 Subject: [PATCH 09/15] reuse variables --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index adf31776db7..7d5d363187a 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -180,15 +180,12 @@ static GrpcKeyBuilder convert(Map keyBuilder) { checkArgument( rawRawNames != null && !rawRawNames.isEmpty(), "each keyBuilder must have at least one name"); - List> rawNames = - JsonUtil.checkObjectList(JsonUtil.getList(keyBuilder, "names")); + List> rawNames = JsonUtil.checkObjectList(rawRawNames); List names = new ArrayList<>(); for (Map rawName : rawNames) { String serviceName = JsonUtil.getString(rawName, "service"); checkArgument(!Strings.isNullOrEmpty(serviceName), "service must not be empty or null"); - names.add( - new Name( - JsonUtil.getString(rawName, "service"), JsonUtil.getString(rawName, "method"))); + names.add(new Name(serviceName, JsonUtil.getString(rawName, "method"))); } List> rawHeaders = JsonUtil.checkObjectList(JsonUtil.getList(keyBuilder, "headers")); From 1fe675ea7d7c91a70bd3946574cef16d6786a91d Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 13:42:55 -0800 Subject: [PATCH 10/15] headers field not provided --- .../java/io/grpc/rls/RlsProtoConverters.java | 5 ++++- .../java/io/grpc/rls/RlsProtoConvertersTest.java | 16 +--------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index 7d5d363187a..c0fc3d29876 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -187,8 +187,11 @@ static GrpcKeyBuilder convert(Map keyBuilder) { checkArgument(!Strings.isNullOrEmpty(serviceName), "service must not be empty or null"); names.add(new Name(serviceName, JsonUtil.getString(rawName, "method"))); } + List rawRawHeaders = JsonUtil.getList(keyBuilder, "headers"); List> rawHeaders = - JsonUtil.checkObjectList(JsonUtil.getList(keyBuilder, "headers")); + rawRawHeaders == null + ? new ArrayList>() + : JsonUtil.checkObjectList(JsonUtil.getList(keyBuilder, "headers")); List nameMatchers = new ArrayList<>(); for (Map rawHeader : rawHeaders) { Boolean requiredMatch = JsonUtil.getBoolean(rawHeader, "requiredMatch"); diff --git a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java index 5efc036175e..ecbb7039fac 100644 --- a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java @@ -335,18 +335,6 @@ public void convert_jsonRlsConfig_defaultValues() throws IOException { + " \"service\": \"service1\",\n" + " \"method\": \"create\"\n" + " }\n" - + " ],\n" - + " \"headers\": [\n" - + " {\n" - + " \"key\": \"user\"," - + " \"names\": [\"User\", \"Parent\"],\n" - + " \"optional\": true\n" - + " },\n" - + " {\n" - + " \"key\": \"id\"," - + " \"names\": [\"X-Google-Id\"],\n" - + " \"optional\": true\n" - + " }\n" + " ]\n" + " }\n" + " ],\n" @@ -360,9 +348,7 @@ public void convert_jsonRlsConfig_defaultValues() throws IOException { ImmutableList.of( new GrpcKeyBuilder( ImmutableList.of(new Name("service1", "create")), - ImmutableList.of( - new NameMatcher("user", ImmutableList.of("User", "Parent")), - new NameMatcher("id", ImmutableList.of("X-Google-Id"))), + ImmutableList.of(), ExtraKeys.DEFAULT, ImmutableMap.of())), /* lookupService= */ "service1", From 86d7c9407751259005b6a0b89a81aeccb60563ba Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 13:44:46 -0800 Subject: [PATCH 11/15] intellij nit --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index c0fc3d29876..f786f919bcd 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -220,8 +220,7 @@ static GrpcKeyBuilder convert(Map keyBuilder) { } private static void checkUniqueKey(List nameMatchers, Set constantKeys) { - Set keys = new HashSet<>(); - keys.addAll(constantKeys); + Set keys = new HashSet<>(constantKeys); keys.add("host"); keys.add("service"); keys.add("method"); From 789b320ac9b7082997110d41e6601975b18f47e0 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 13:51:29 -0800 Subject: [PATCH 12/15] nit: extraKeys.size replaced to 3 --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index f786f919bcd..aefe5e3679e 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -23,6 +23,7 @@ import com.google.common.base.Converter; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.grpc.internal.JsonUtil; import io.grpc.lookup.v1.RouteLookupRequest; @@ -221,13 +222,12 @@ static GrpcKeyBuilder convert(Map keyBuilder) { private static void checkUniqueKey(List nameMatchers, Set constantKeys) { Set keys = new HashSet<>(constantKeys); - keys.add("host"); - keys.add("service"); - keys.add("method"); + List extraKeys = ImmutableList.of("host", "service", "method"); + keys.addAll(extraKeys); for (NameMatcher nameMatcher : nameMatchers) { keys.add(nameMatcher.getKey()); } - if (keys.size() != nameMatchers.size() + constantKeys.size() + 3) { + if (keys.size() != nameMatchers.size() + constantKeys.size() + extraKeys.size()) { throw new IllegalArgumentException("keys in KeyBuilder must be unique"); } } From 3252604efa44f256aaafe6cdf7286480e45781be Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 16:12:06 -0800 Subject: [PATCH 13/15] non-null maxAge and staleAge --- rls/src/main/java/io/grpc/rls/RlsProtoData.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoData.java b/rls/src/main/java/io/grpc/rls/RlsProtoData.java index 747e0eba39f..25ac5efa2c0 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoData.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoData.java @@ -156,8 +156,8 @@ static final class RouteLookupConfig { List grpcKeyBuilders, String lookupService, long lookupServiceTimeoutInNanos, - @Nullable Long maxAgeInNanos, - @Nullable Long staleAgeInNanos, + long maxAgeInNanos, + long staleAgeInNanos, long cacheSizeBytes, @Nullable String defaultTarget) { From 287d6bb91d3b9e399560da529f0ff342fa565327 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 17:33:47 -0800 Subject: [PATCH 14/15] address comments --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 7 +++---- rls/src/main/java/io/grpc/rls/RlsProtoData.java | 8 +++----- .../test/java/io/grpc/rls/RlsProtoConvertersTest.java | 9 ++++----- rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java | 6 +++--- 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index aefe5e3679e..f8cace0d12a 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -120,9 +120,9 @@ protected RouteLookupConfig doForward(Map json) { checkArgument(!Strings.isNullOrEmpty(lookupService), "lookupService must not be empty"); try { new URI(lookupService); - } catch (URISyntaxException x) { + } catch (URISyntaxException e) { throw new IllegalArgumentException( - "The lookupService field is not valid URI: " + lookupService, x); + "The lookupService field is not valid URI: " + lookupService, e); } long timeout = orDefault( JsonUtil.getStringAsDuration(json, "lookupServiceTimeout"), @@ -191,8 +191,7 @@ static GrpcKeyBuilder convert(Map keyBuilder) { List rawRawHeaders = JsonUtil.getList(keyBuilder, "headers"); List> rawHeaders = rawRawHeaders == null - ? new ArrayList>() - : JsonUtil.checkObjectList(JsonUtil.getList(keyBuilder, "headers")); + ? new ArrayList>() : JsonUtil.checkObjectList(rawRawHeaders); List nameMatchers = new ArrayList<>(); for (Map rawHeader : rawHeaders) { Boolean requiredMatch = JsonUtil.getBoolean(rawHeader, "requiredMatch"); diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoData.java b/rls/src/main/java/io/grpc/rls/RlsProtoData.java index 25ac5efa2c0..4709894833c 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoData.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoData.java @@ -411,14 +411,11 @@ static final class Name { private final String service; + @Nullable private final String method; - public Name(String service) { - this(service, "*"); - } - /** The primary constructor. */ - Name(String service, String method) { + Name(String service, @Nullable String method) { this.service = service; this.method = method; } @@ -427,6 +424,7 @@ String getService() { return service; } + @Nullable String getMethod() { return method; } diff --git a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java index ecbb7039fac..5d1dcc1cadb 100644 --- a/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsProtoConvertersTest.java @@ -186,14 +186,14 @@ public void convert_jsonRlsConfig() throws IOException { ExtraKeys.DEFAULT, ImmutableMap.of()), new GrpcKeyBuilder( - ImmutableList.of(new Name("service1")), + ImmutableList.of(new Name("service1", "*")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent")), new NameMatcher("password", ImmutableList.of("Password"))), ExtraKeys.DEFAULT, ImmutableMap.of()), new GrpcKeyBuilder( - ImmutableList.of(new Name("service3")), + ImmutableList.of(new Name("service3", "*")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"))), ExtraKeys.create("host-key", "service-key", "method-key"), @@ -332,8 +332,7 @@ public void convert_jsonRlsConfig_defaultValues() throws IOException { + " {\n" + " \"names\": [\n" + " {\n" - + " \"service\": \"service1\",\n" - + " \"method\": \"create\"\n" + + " \"service\": \"service1\"\n" + " }\n" + " ]\n" + " }\n" @@ -347,7 +346,7 @@ public void convert_jsonRlsConfig_defaultValues() throws IOException { new RouteLookupConfig( ImmutableList.of( new GrpcKeyBuilder( - ImmutableList.of(new Name("service1", "create")), + ImmutableList.of(new Name("service1", null)), ImmutableList.of(), ExtraKeys.DEFAULT, ImmutableMap.of())), diff --git a/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java b/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java index 641b0c65e75..0d834601279 100644 --- a/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsRequestFactoryTest.java @@ -46,20 +46,20 @@ public class RlsRequestFactoryTest { 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 Name("com.google.service1", "*")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent")), new NameMatcher("password", ImmutableList.of("Password"))), 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 Name("com.google.service2", "*")), ImmutableList.of( new NameMatcher("password", ImmutableList.of("Password"))), ExtraKeys.create(null, "service-3", "method-3"), ImmutableMap.of()), new GrpcKeyBuilder( - ImmutableList.of(new Name("com.google.service3")), + ImmutableList.of(new Name("com.google.service3", "*")), ImmutableList.of( new NameMatcher("user", ImmutableList.of("User", "Parent"))), ExtraKeys.create(null, null, null), From 344b0dd7dd4cdf3a10d0343588ccb9f4c26f8b15 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Mon, 29 Nov 2021 17:39:33 -0800 Subject: [PATCH 15/15] make extra_key_names static --- rls/src/main/java/io/grpc/rls/RlsProtoConverters.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java index f8cace0d12a..b4d3fcc3b29 100644 --- a/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java +++ b/rls/src/main/java/io/grpc/rls/RlsProtoConverters.java @@ -51,6 +51,8 @@ final class RlsProtoConverters { private static final long MAX_AGE_NANOS = MINUTES.toNanos(5); private static final long MAX_CACHE_SIZE = 5 * 1024 * 1024; // 5MiB private static final long DEFAULT_LOOKUP_SERVICE_TIMEOUT = SECONDS.toNanos(10); + private static final ImmutableList EXTRA_KEY_NAMES = + ImmutableList.of("host", "service", "method"); /** * RouteLookupRequestConverter converts between {@link RouteLookupRequest} and {@link @@ -221,12 +223,11 @@ static GrpcKeyBuilder convert(Map keyBuilder) { private static void checkUniqueKey(List nameMatchers, Set constantKeys) { Set keys = new HashSet<>(constantKeys); - List extraKeys = ImmutableList.of("host", "service", "method"); - keys.addAll(extraKeys); + keys.addAll(EXTRA_KEY_NAMES); for (NameMatcher nameMatcher : nameMatchers) { keys.add(nameMatcher.getKey()); } - if (keys.size() != nameMatchers.size() + constantKeys.size() + extraKeys.size()) { + if (keys.size() != nameMatchers.size() + constantKeys.size() + EXTRA_KEY_NAMES.size()) { throw new IllegalArgumentException("keys in KeyBuilder must be unique"); } }