rls: fix wrong lookup request server field

The server filed in lookup request as specified in go/dynamic-request-routing/#heading=h.eqjtcpo6u8ep should be the original target, not the RLS server where the lookup request is sent to.
This commit is contained in:
ZHANG Dapeng 2021-02-08 15:53:36 -08:00 committed by GitHub
parent 2cd45e7a24
commit cb3317b1fd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 23 additions and 17 deletions

View File

@ -137,7 +137,8 @@ final class CachingRlsLbClient {
builder.evictionListener, builder.evictionListener,
scheduledExecutorService, scheduledExecutorService,
timeProvider); timeProvider);
RlsRequestFactory requestFactory = new RlsRequestFactory(lbPolicyConfig.getRouteLookupConfig()); RlsRequestFactory requestFactory = new RlsRequestFactory(
lbPolicyConfig.getRouteLookupConfig(), helper.getAuthority());
rlsPicker = new RlsPicker(requestFactory); rlsPicker = new RlsPicker(requestFactory);
// It is safe to use helper.getUnsafeChannelCredentials() because the client authenticates the // It is safe to use helper.getUnsafeChannelCredentials() because the client authenticates the
// RLS server using the same authority as the backends, even though the RLS servers addresses // RLS server using the same authority as the backends, even though the RLS servers addresses

View File

@ -45,9 +45,9 @@ final class RlsRequestFactory {
*/ */
private final Table<String, String, NameMatcher> keyBuilderTable; private final Table<String, String, NameMatcher> keyBuilderTable;
RlsRequestFactory(RouteLookupConfig rlsConfig) { RlsRequestFactory(RouteLookupConfig rlsConfig, String target) {
checkNotNull(rlsConfig, "rlsConfig"); checkNotNull(rlsConfig, "rlsConfig");
this.target = rlsConfig.getLookupService(); this.target = checkNotNull(target, "target");
this.keyBuilderTable = createKeyBuilderTable(rlsConfig); this.keyBuilderTable = createKeyBuilderTable(rlsConfig);
} }
@ -109,7 +109,7 @@ final class RlsRequestFactory {
@Override @Override
public String toString() { public String toString() {
return MoreObjects.toStringHelper(this) return MoreObjects.toStringHelper(this)
.add("lookupService", target) .add("target", target)
.add("keyBuilderTable", keyBuilderTable) .add("keyBuilderTable", keyBuilderTable)
.toString(); .toString();
} }

View File

@ -187,7 +187,8 @@ public class CachingRlsLbClientTest {
public void get_noError_lifeCycle() throws Exception { public void get_noError_lifeCycle() throws Exception {
InOrder inOrder = inOrder(evictionListener); InOrder inOrder = inOrder(evictionListener);
RouteLookupRequest routeLookupRequest = RouteLookupRequest routeLookupRequest =
new RouteLookupRequest("server", "/foo/bar", "grpc", ImmutableMap.<String, String>of()); new RouteLookupRequest(
"bigtable.googleapis.com", "/foo/bar", "grpc", ImmutableMap.<String, String>of());
rlsServerImpl.setLookupTable( rlsServerImpl.setLookupTable(
ImmutableMap.of( ImmutableMap.of(
routeLookupRequest, routeLookupRequest,
@ -277,7 +278,8 @@ public class CachingRlsLbClientTest {
public void get_updatesLbState() throws Exception { public void get_updatesLbState() throws Exception {
InOrder inOrder = inOrder(helper); InOrder inOrder = inOrder(helper);
RouteLookupRequest routeLookupRequest = RouteLookupRequest routeLookupRequest =
new RouteLookupRequest("service1", "/foo/bar", "grpc", ImmutableMap.<String, String>of()); new RouteLookupRequest(
"bigtable.googleapis.com", "/foo/bar", "grpc", ImmutableMap.<String, String>of());
rlsServerImpl.setLookupTable( rlsServerImpl.setLookupTable(
ImmutableMap.of( ImmutableMap.of(
routeLookupRequest, routeLookupRequest,
@ -317,7 +319,7 @@ public class CachingRlsLbClientTest {
// try to get invalid // try to get invalid
RouteLookupRequest invalidRouteLookupRequest = RouteLookupRequest invalidRouteLookupRequest =
new RouteLookupRequest( new RouteLookupRequest(
"service1", "/doesn/exists", "grpc", ImmutableMap.<String, String>of()); "bigtable.googleapis.com", "/doesn/exists", "grpc", ImmutableMap.<String, String>of());
CachedRouteLookupResponse errorResp = getInSyncContext(invalidRouteLookupRequest); CachedRouteLookupResponse errorResp = getInSyncContext(invalidRouteLookupRequest);
assertThat(errorResp.isPending()).isTrue(); assertThat(errorResp.isPending()).isTrue();
fakeTimeProvider.forwardTime(SERVER_LATENCY_MILLIS, TimeUnit.MILLISECONDS); fakeTimeProvider.forwardTime(SERVER_LATENCY_MILLIS, TimeUnit.MILLISECONDS);
@ -581,7 +583,7 @@ public class CachingRlsLbClientTest {
@Override @Override
public String getAuthority() { public String getAuthority() {
return DEFAULT_TARGET; return "bigtable.googleapis.com";
} }
@Override @Override

View File

@ -145,10 +145,12 @@ public class RlsLoadBalancerTest {
fakeRlsServerImpl.setLookupTable( fakeRlsServerImpl.setLookupTable(
ImmutableMap.of( ImmutableMap.of(
new RouteLookupRequest( new RouteLookupRequest(
"localhost:8972", "/com.google/Search", "grpc", ImmutableMap.<String, String>of()), "fake-bigtable.googleapis.com", "/com.google/Search", "grpc",
ImmutableMap.<String, String>of()),
new RouteLookupResponse(ImmutableList.of("wilderness"), "where are you?"), new RouteLookupResponse(ImmutableList.of("wilderness"), "where are you?"),
new RouteLookupRequest( new RouteLookupRequest(
"localhost:8972", "/com.google/Rescue", "grpc", ImmutableMap.<String, String>of()), "fake-bigtable.googleapis.com", "/com.google/Rescue", "grpc",
ImmutableMap.<String, String>of()),
new RouteLookupResponse(ImmutableList.of("civilization"), "you are safe"))); new RouteLookupResponse(ImmutableList.of("civilization"), "you are safe")));
rlsLb = (RlsLoadBalancer) provider.newLoadBalancer(helper); rlsLb = (RlsLoadBalancer) provider.newLoadBalancer(helper);

View File

@ -58,7 +58,7 @@ public class RlsRequestFactoryTest {
ImmutableList.of(new Name("com.google.service3")), ImmutableList.of(new Name("com.google.service3")),
ImmutableList.of( ImmutableList.of(
new NameMatcher("user", ImmutableList.of("User", "Parent"), true)))), new NameMatcher("user", ImmutableList.of("User", "Parent"), true)))),
/* lookupService= */ "foo.google.com", /* lookupService= */ "bigtable-rls.googleapis.com",
/* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2), /* lookupServiceTimeoutInMillis= */ TimeUnit.SECONDS.toMillis(2),
/* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300), /* maxAgeInMillis= */ TimeUnit.SECONDS.toMillis(300),
/* staleAgeInMillis= */ TimeUnit.SECONDS.toMillis(240), /* staleAgeInMillis= */ TimeUnit.SECONDS.toMillis(240),
@ -66,7 +66,8 @@ public class RlsRequestFactoryTest {
/* validTargets= */ ImmutableList.of("a valid target"), /* validTargets= */ ImmutableList.of("a valid target"),
/* defaultTarget= */ "us_east_1.cloudbigtable.googleapis.com"); /* defaultTarget= */ "us_east_1.cloudbigtable.googleapis.com");
private final RlsRequestFactory factory = new RlsRequestFactory(RLS_CONFIG); private final RlsRequestFactory factory = new RlsRequestFactory(
RLS_CONFIG, "bigtable.googleapis.com");
@Test @Test
public void create_pathMatches() { public void create_pathMatches() {
@ -78,7 +79,7 @@ public class RlsRequestFactoryTest {
RouteLookupRequest request = factory.create("com.google.service1", "Create", metadata); RouteLookupRequest request = factory.create("com.google.service1", "Create", metadata);
assertThat(request.getTargetType()).isEqualTo("grpc"); assertThat(request.getTargetType()).isEqualTo("grpc");
assertThat(request.getPath()).isEqualTo("/com.google.service1/Create"); assertThat(request.getPath()).isEqualTo("/com.google.service1/Create");
assertThat(request.getServer()).isEqualTo("foo.google.com"); assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com");
assertThat(request.getKeyMap()).containsExactly("user", "test", "id", "123"); assertThat(request.getKeyMap()).containsExactly("user", "test", "id", "123");
} }
@ -107,7 +108,7 @@ public class RlsRequestFactoryTest {
assertThat(request.getTargetType()).isEqualTo("grpc"); assertThat(request.getTargetType()).isEqualTo("grpc");
assertThat(request.getPath()).isEqualTo("/com.google.service1/Update"); assertThat(request.getPath()).isEqualTo("/com.google.service1/Update");
assertThat(request.getServer()).isEqualTo("foo.google.com"); assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com");
assertThat(request.getKeyMap()).containsExactly("user", "test", "password", "hunter2"); assertThat(request.getKeyMap()).containsExactly("user", "test", "password", "hunter2");
} }
@ -122,7 +123,7 @@ public class RlsRequestFactoryTest {
assertThat(request.getTargetType()).isEqualTo("grpc"); assertThat(request.getTargetType()).isEqualTo("grpc");
assertThat(request.getPath()).isEqualTo("/com.google.service1/Update"); assertThat(request.getPath()).isEqualTo("/com.google.service1/Update");
assertThat(request.getServer()).isEqualTo("foo.google.com"); assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com");
assertThat(request.getKeyMap()).containsExactly("user", "test"); assertThat(request.getKeyMap()).containsExactly("user", "test");
} }
@ -137,7 +138,7 @@ public class RlsRequestFactoryTest {
assertThat(request.getTargetType()).isEqualTo("grpc"); assertThat(request.getTargetType()).isEqualTo("grpc");
assertThat(request.getPath()).isEqualTo("/abc.def.service999/Update"); assertThat(request.getPath()).isEqualTo("/abc.def.service999/Update");
assertThat(request.getServer()).isEqualTo("foo.google.com"); assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com");
assertThat(request.getKeyMap()).isEmpty(); assertThat(request.getKeyMap()).isEmpty();
} }
@ -152,7 +153,7 @@ public class RlsRequestFactoryTest {
assertThat(request.getTargetType()).isEqualTo("grpc"); assertThat(request.getTargetType()).isEqualTo("grpc");
assertThat(request.getPath()).isEqualTo("/com.google.service3/Update"); assertThat(request.getPath()).isEqualTo("/com.google.service3/Update");
assertThat(request.getServer()).isEqualTo("foo.google.com"); assertThat(request.getServer()).isEqualTo("bigtable.googleapis.com");
assertThat(request.getKeyMap()).containsExactly("user", "test"); assertThat(request.getKeyMap()).containsExactly("user", "test");
} }
} }