diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index 8c874cc679..c091939c13 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -27,6 +27,7 @@ import com.google.common.base.Verify; import io.grpc.Attributes; import io.grpc.EquivalentAddressGroup; import io.grpc.NameResolver; +import io.grpc.NameResolver.Helper.ConfigOrError; import io.grpc.ProxiedSocketAddress; import io.grpc.ProxyDetector; import io.grpc.Status; @@ -285,24 +286,15 @@ final class DnsNameResolver extends NameResolver { Attributes.Builder attrs = Attributes.newBuilder(); if (!resolutionResults.txtRecords.isEmpty()) { - Map serviceConfig = null; - try { - for (Map possibleConfig : parseTxtResults(resolutionResults.txtRecords)) { - try { - serviceConfig = - maybeChooseServiceConfig(possibleConfig, random, getLocalHostname()); - } catch (RuntimeException e) { - logger.log(Level.WARNING, "Bad service config choice " + possibleConfig, e); - } - if (serviceConfig != null) { - break; - } - } - } catch (RuntimeException e) { - logger.log(Level.WARNING, "Can't parse service Configs", e); - } + ConfigOrError> serviceConfig = + parseServiceConfig(resolutionResults.txtRecords, random, getLocalHostname()); if (serviceConfig != null) { - attrs.set(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, serviceConfig); + if (serviceConfig.getError() != null) { + savedListener.onError(serviceConfig.getError()); + return; + } else { + attrs.set(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, serviceConfig.getConfig()); + } } } else { logger.log(Level.FINE, "No TXT records found for {0}", new Object[]{host}); @@ -311,6 +303,35 @@ final class DnsNameResolver extends NameResolver { } } + @Nullable + static ConfigOrError> parseServiceConfig( + List rawTxtRecords, Random random, String localHostname) { + List> possibleServiceConfigChoices; + try { + possibleServiceConfigChoices = parseTxtResults(rawTxtRecords); + } catch (IOException | RuntimeException e) { + return ConfigOrError.fromError( + Status.UNKNOWN.withDescription("failed to parse TXT records").withCause(e)); + } + Map possibleServiceConfig = null; + for (Map possibleServiceConfigChoice : possibleServiceConfigChoices) { + try { + possibleServiceConfig = + maybeChooseServiceConfig(possibleServiceConfigChoice, random, localHostname); + } catch (RuntimeException e) { + return ConfigOrError.fromError( + Status.UNKNOWN.withDescription("failed to pick service config choice").withCause(e)); + } + if (possibleServiceConfig != null) { + break; + } + } + if (possibleServiceConfig == null) { + return null; + } + return ConfigOrError.>fromConfig(possibleServiceConfig); + } + private void resolve() { if (resolving || shutdown || !cacheRefreshRequired()) { return; @@ -403,35 +424,27 @@ final class DnsNameResolver extends NameResolver { return new ResolutionResults(addresses, txtRecords, balancerAddresses); } + /** + * + * @throws IOException if one of the txt records contains improperly formatted JSON. + */ @SuppressWarnings("unchecked") @VisibleForTesting - static List> parseTxtResults(List txtRecords) { - List> serviceConfigs = new ArrayList<>(); + static List> parseTxtResults(List txtRecords) throws IOException { + List> possibleServiceConfigChoices = new ArrayList<>(); for (String txtRecord : txtRecords) { - if (txtRecord.startsWith(SERVICE_CONFIG_PREFIX)) { - List> choices; - try { - Object rawChoices = JsonParser.parse(txtRecord.substring(SERVICE_CONFIG_PREFIX.length())); - if (!(rawChoices instanceof List)) { - throw new IOException("wrong type " + rawChoices); - } - List listChoices = (List) rawChoices; - for (Object obj : listChoices) { - if (!(obj instanceof Map)) { - throw new IOException("wrong element type " + rawChoices); - } - } - choices = (List>) listChoices; - } catch (IOException e) { - logger.log(Level.WARNING, "Bad service config: " + txtRecord, e); - continue; - } - serviceConfigs.addAll(choices); - } else { + if (!txtRecord.startsWith(SERVICE_CONFIG_PREFIX)) { logger.log(Level.FINE, "Ignoring non service config {0}", new Object[]{txtRecord}); + continue; } + Object rawChoices = JsonParser.parse(txtRecord.substring(SERVICE_CONFIG_PREFIX.length())); + if (!(rawChoices instanceof List)) { + throw new ClassCastException("wrong type " + rawChoices); + } + List listChoices = (List) rawChoices; + possibleServiceConfigChoices.addAll(ServiceConfigUtil.checkObjectList(listChoices)); } - return serviceConfigs; + return possibleServiceConfigChoices; } @Nullable diff --git a/core/src/main/java/io/grpc/internal/JsonParser.java b/core/src/main/java/io/grpc/internal/JsonParser.java index 56d7863e4a..10feb3f32e 100644 --- a/core/src/main/java/io/grpc/internal/JsonParser.java +++ b/core/src/main/java/io/grpc/internal/JsonParser.java @@ -40,7 +40,7 @@ public final class JsonParser { private JsonParser() {} /** - * Parses a json string, returning either a {@code Map}, {@code List}, + * Parses a json string, returning either a {@code Map}, {@code List}, * {@code String}, {@code Double}, {@code Boolean}, or {@code null}. */ @SuppressWarnings("unchecked") diff --git a/core/src/main/java/io/grpc/internal/ServiceConfigUtil.java b/core/src/main/java/io/grpc/internal/ServiceConfigUtil.java index 25d815b1a9..5cab5834a7 100644 --- a/core/src/main/java/io/grpc/internal/ServiceConfigUtil.java +++ b/core/src/main/java/io/grpc/internal/ServiceConfigUtil.java @@ -530,7 +530,7 @@ public final class ServiceConfigUtil { } @SuppressWarnings("unchecked") - private static List> checkObjectList(List rawList) { + static List> checkObjectList(List rawList) { for (int i = 0; i < rawList.size(); i++) { if (!(rawList.get(i) instanceof Map)) { throw new ClassCastException( diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index 1ee7784058..68a1eea53b 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -32,6 +32,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import com.google.common.base.Stopwatch; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.net.InetAddresses; import com.google.common.testing.FakeTicker; @@ -39,6 +40,7 @@ import io.grpc.Attributes; import io.grpc.EquivalentAddressGroup; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.NameResolver; +import io.grpc.NameResolver.Helper.ConfigOrError; import io.grpc.ProxyDetector; import io.grpc.Status; import io.grpc.Status.Code; @@ -58,6 +60,7 @@ import java.net.SocketAddress; import java.net.URI; import java.net.UnknownHostException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; @@ -878,7 +881,7 @@ public class DnsNameResolverTest { } @Test - public void parseTxtResults_misspelledName() { + public void parseTxtResults_misspelledName() throws Exception { List txtRecords = new ArrayList<>(); txtRecords.add("some_record"); txtRecords.add("_grpc_config=[]"); @@ -889,50 +892,36 @@ public class DnsNameResolverTest { } @Test - public void parseTxtResults_badTypeIgnored() { - Logger logger = Logger.getLogger(DnsNameResolver.class.getName()); - Level level = logger.getLevel(); - logger.setLevel(Level.SEVERE); - try { - List txtRecords = new ArrayList<>(); - txtRecords.add("some_record"); - txtRecords.add("grpc_config={}"); + public void parseTxtResults_badTypeFails() throws Exception { + thrown.expect(ClassCastException.class); + thrown.expectMessage("wrong type"); + List txtRecords = new ArrayList<>(); + txtRecords.add("some_record"); + txtRecords.add("grpc_config={}"); - List> results = DnsNameResolver.parseTxtResults(txtRecords); - - assertThat(results).isEmpty(); - } finally { - logger.setLevel(level); - } + DnsNameResolver.parseTxtResults(txtRecords); } @Test - public void parseTxtResults_badInnerTypeIgnored() { - Logger logger = Logger.getLogger(DnsNameResolver.class.getName()); - Level level = logger.getLevel(); - logger.setLevel(Level.SEVERE); - try { - List txtRecords = new ArrayList<>(); - txtRecords.add("some_record"); - txtRecords.add("grpc_config=[\"bogus\"]"); + public void parseTxtResults_badInnerTypeFails() throws Exception { + thrown.expect(ClassCastException.class); + thrown.expectMessage("not object"); - List> results = DnsNameResolver.parseTxtResults(txtRecords); + List txtRecords = new ArrayList<>(); + txtRecords.add("some_record"); + txtRecords.add("grpc_config=[\"bogus\"]"); - assertThat(results).isEmpty(); - } finally { - logger.setLevel(level); - } + DnsNameResolver.parseTxtResults(txtRecords); } @Test - public void parseTxtResults_combineAll() { + public void parseTxtResults_combineAll() throws Exception { Logger logger = Logger.getLogger(DnsNameResolver.class.getName()); Level level = logger.getLevel(); logger.setLevel(Level.SEVERE); try { List txtRecords = new ArrayList<>(); txtRecords.add("some_record"); - txtRecords.add("grpc_config=[\"bogus\", {}]"); txtRecords.add("grpc_config=[{}, {}]"); // 2 records txtRecords.add("grpc_config=[{\"\":{}}]"); // 1 record @@ -1017,6 +1006,44 @@ public class DnsNameResolverTest { enableJndi, enableJndiLocalhost, "2001-db8-1234--as3.ipv6-literal.net")); } + @Test + public void parseServiceConfig_capturesParseError() { + ConfigOrError> result = DnsNameResolver.parseServiceConfig( + Arrays.asList("grpc_config=bogus"), new Random(), "localhost"); + + assertThat(result).isNotNull(); + assertThat(result.getError().getCode()).isEqualTo(Status.Code.UNKNOWN); + assertThat(result.getError().getDescription()).contains("failed to parse TXT records"); + } + + @Test + public void parseServiceConfig_capturesChoiceError() { + ConfigOrError> result = DnsNameResolver.parseServiceConfig( + Arrays.asList("grpc_config=[{\"hi\":{}}]"), new Random(), "localhost"); + + assertThat(result).isNotNull(); + assertThat(result.getError().getCode()).isEqualTo(Status.Code.UNKNOWN); + assertThat(result.getError().getDescription()).contains("failed to pick"); + } + + @Test + public void parseServiceConfig_noChoiceIsNull() { + ConfigOrError> result = DnsNameResolver.parseServiceConfig( + Arrays.asList("grpc_config=[]"), new Random(), "localhost"); + + assertThat(result).isNull(); + } + + @Test + public void parseServiceConfig_matches() { + ConfigOrError> result = DnsNameResolver.parseServiceConfig( + Arrays.asList("grpc_config=[{\"serviceConfig\":{}}]"), new Random(), "localhost"); + + assertThat(result).isNotNull(); + assertThat(result.getError()).isNull(); + assertThat(result.getConfig()).isEqualTo(ImmutableMap.of()); + } + private void testInvalidUri(URI uri) { try { provider.newNameResolver(uri, helper);