From 87e248e83cf266166ec4e84aa0a2d1190bc9ab86 Mon Sep 17 00:00:00 2001 From: Jihun Cho Date: Thu, 17 Oct 2019 16:36:59 -0700 Subject: [PATCH 1/4] core,grpclb: DnsNameResolver will use srv record by default if grpclb --- .../io/grpc/NameResolverRegistryTest.java | 8 ++- .../internal/BaseDnsNameResolverProvider.java | 72 +++++++++++++++++++ .../io/grpc/internal/DnsNameResolver.java | 16 +++-- .../internal/DnsNameResolverProvider.java | 39 +--------- .../internal/DnsNameResolverProviderTest.java | 46 +++++++++++- .../io/grpc/internal/DnsNameResolverTest.java | 3 +- .../grpclb/GrpclbNameResolverProvider.java | 53 ++++++++++++++ .../services/io.grpc.NameResolverProvider | 1 + .../GrpclbNameResolverProviderTest.java | 69 ++++++++++++++++++ 9 files changed, 260 insertions(+), 47 deletions(-) create mode 100644 core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java create mode 100644 grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolverProvider.java create mode 100644 grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider create mode 100644 grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java diff --git a/api/src/test/java/io/grpc/NameResolverRegistryTest.java b/api/src/test/java/io/grpc/NameResolverRegistryTest.java index 1870f312868..abbe10af2ac 100644 --- a/api/src/test/java/io/grpc/NameResolverRegistryTest.java +++ b/api/src/test/java/io/grpc/NameResolverRegistryTest.java @@ -21,6 +21,7 @@ import static org.mockito.Mockito.mock; import io.grpc.NameResolver.ServiceConfigParser; +import io.grpc.internal.BaseDnsNameResolverProvider; import io.grpc.internal.DnsNameResolverProvider; import java.lang.Thread.UncaughtExceptionHandler; import java.net.URI; @@ -152,8 +153,11 @@ public void newNameResolver_noProvider() { @Test public void baseProviders() { List providers = NameResolverRegistry.getDefaultRegistry().providers(); - assertThat(providers).hasSize(1); - assertThat(providers.get(0)).isInstanceOf(DnsNameResolverProvider.class); + assertThat(providers).hasSize(2); + // 2 name resolvers from grpclb and core + for (NameResolverProvider provider : providers) { + assertThat(provider).isInstanceOf(BaseDnsNameResolverProvider.class); + } assertThat(NameResolverRegistry.getDefaultRegistry().asFactory().getDefaultScheme()) .isEqualTo("dns"); } diff --git a/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java new file mode 100644 index 00000000000..6a69af0b29c --- /dev/null +++ b/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java @@ -0,0 +1,72 @@ +/* + * Copyright 2019 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.internal; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; +import com.google.common.base.Stopwatch; +import io.grpc.Internal; +import io.grpc.InternalServiceProviders; +import io.grpc.NameResolver; +import io.grpc.NameResolverProvider; +import java.net.URI; + +/** + * Base provider of name resolvers for name agnostic consumption. + */ +@Internal +public abstract class BaseDnsNameResolverProvider extends NameResolverProvider { + + private static final String SCHEME = "dns"; + + @VisibleForTesting + public static final String ENABLE_GRPCLB_PROPERTY_NAME = + "io.grpc.internal.DnsNameResolverProvider.enable_grpclb"; + + /** Returns boolean value of {@link #ENABLE_GRPCLB_PROPERTY_NAME}. */ + protected abstract boolean isSrvEnabled(); + + @Override + public DnsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) { + if (SCHEME.equals(targetUri.getScheme())) { + String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath"); + Preconditions.checkArgument(targetPath.startsWith("/"), + "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri); + String name = targetPath.substring(1); + return new DnsNameResolver( + targetUri.getAuthority(), + name, + args, + GrpcUtil.SHARED_CHANNEL_EXECUTOR, + Stopwatch.createUnstarted(), + InternalServiceProviders.isAndroid(getClass().getClassLoader()), + isSrvEnabled()); + } else { + return null; + } + } + + @Override + public String getDefaultScheme() { + return SCHEME; + } + + @Override + protected boolean isAvailable() { + return true; + } +} diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index 8e07dbdb8ec..b85dd79ccb9 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -92,8 +92,6 @@ final class DnsNameResolver extends NameResolver { System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_jndi", "true"); private static final String JNDI_LOCALHOST_PROPERTY = System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_jndi_localhost", "false"); - private static final String JNDI_SRV_PROPERTY = - System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_grpclb", "false"); private static final String JNDI_TXT_PROPERTY = System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_service_config", "false"); @@ -117,8 +115,6 @@ final class DnsNameResolver extends NameResolver { @VisibleForTesting static boolean enableJndiLocalhost = Boolean.parseBoolean(JNDI_LOCALHOST_PROPERTY); @VisibleForTesting - static boolean enableSrv = Boolean.parseBoolean(JNDI_SRV_PROPERTY); - @VisibleForTesting static boolean enableTxt = Boolean.parseBoolean(JNDI_TXT_PROPERTY); private static final ResourceResolverFactory resourceResolverFactory = @@ -152,6 +148,7 @@ final class DnsNameResolver extends NameResolver { /** True if using an executor resource that should be released after use. */ private final boolean usingExecutorResource; + private final boolean enableSrv; private boolean resolving; @@ -159,8 +156,14 @@ final class DnsNameResolver extends NameResolver { // from any thread. private NameResolver.Listener2 listener; - DnsNameResolver(@Nullable String nsAuthority, String name, Args args, - Resource executorResource, Stopwatch stopwatch, boolean isAndroid) { + DnsNameResolver( + @Nullable String nsAuthority, + String name, + Args args, + Resource executorResource, + Stopwatch stopwatch, + boolean isAndroid, + boolean enableSrv) { Preconditions.checkNotNull(args, "args"); // TODO: if a DNS server is provided as nsAuthority, use it. // https://www.captechconsulting.com/blogs/accessing-the-dusty-corners-of-dns-with-java @@ -184,6 +187,7 @@ final class DnsNameResolver extends NameResolver { Preconditions.checkNotNull(args.getSynchronizationContext(), "syncContext"); this.executor = args.getBlockingExecutor(); this.usingExecutorResource = executor == null; + this.enableSrv = enableSrv; } @Override diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index 67e60351631..e0702cccd9b 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java @@ -16,13 +16,6 @@ package io.grpc.internal; -import com.google.common.base.Preconditions; -import com.google.common.base.Stopwatch; -import io.grpc.InternalServiceProviders; -import io.grpc.NameResolver; -import io.grpc.NameResolverProvider; -import java.net.URI; - /** * A provider for {@link DnsNameResolver}. * @@ -38,37 +31,11 @@ *
  • {@code "dns:///foo.googleapis.com"} (without port)
    • * */ -public final class DnsNameResolverProvider extends NameResolverProvider { - - private static final String SCHEME = "dns"; - - @Override - public DnsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) { - if (SCHEME.equals(targetUri.getScheme())) { - String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath"); - Preconditions.checkArgument(targetPath.startsWith("/"), - "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri); - String name = targetPath.substring(1); - return new DnsNameResolver( - targetUri.getAuthority(), - name, - args, - GrpcUtil.SHARED_CHANNEL_EXECUTOR, - Stopwatch.createUnstarted(), - InternalServiceProviders.isAndroid(getClass().getClassLoader())); - } else { - return null; - } - } - - @Override - public String getDefaultScheme() { - return SCHEME; - } +public final class DnsNameResolverProvider extends BaseDnsNameResolverProvider { @Override - protected boolean isAvailable() { - return true; + protected boolean isSrvEnabled() { + return Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false")); } @Override diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index 9dc402a93d6..d1b63e8d334 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -16,6 +16,8 @@ package io.grpc.internal; +import static com.google.common.truth.Truth.assertThat; +import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; @@ -25,6 +27,9 @@ import io.grpc.NameResolver.ServiceConfigParser; import io.grpc.SynchronizationContext; import java.net.URI; +import javax.annotation.Nullable; +import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -46,15 +51,52 @@ public void uncaughtException(Thread t, Throwable e) { .setServiceConfigParser(mock(ServiceConfigParser.class)) .build(); - private DnsNameResolverProvider provider = new DnsNameResolverProvider(); + @Nullable + private String jndiSrvProperty; + + @Before + public void memoizeSrvProperty() { + jndiSrvProperty = System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME); + } + + @After + public void restoreSrvProperty() { + if (jndiSrvProperty == null) { + System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); + } else { + System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, jndiSrvProperty); + } + } + + @Test + public void enableSrv_noDefault() { + System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); + + assertThat(new DnsNameResolverProvider().isSrvEnabled()).isFalse(); + } + + @Test + public void enableSrv_defaultEnabled() { + System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true"); + + assertThat(new DnsNameResolverProvider().isSrvEnabled()).isTrue(); + } + + @Test + public void enableSrv_defaultDisabled() { + System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false"); + + assertThat(new DnsNameResolverProvider().isSrvEnabled()).isFalse(); + } @Test public void isAvailable() { - assertTrue(provider.isAvailable()); + assertTrue(new DnsNameResolverProvider().isAvailable()); } @Test public void newNameResolver() { + DnsNameResolverProvider provider = new DnsNameResolverProvider(); assertSame(DnsNameResolver.class, provider.newNameResolver(URI.create("dns:///localhost:443"), args).getClass()); assertNull( diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index 7214db6edac..6baa87ad0ad 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -182,7 +182,8 @@ private DnsNameResolver newResolver( private DnsNameResolver newResolver( String name, Stopwatch stopwatch, boolean isAndroid, NameResolver.Args args) { DnsNameResolver dnsResolver = - new DnsNameResolver(null, name, args, fakeExecutorResource, stopwatch, isAndroid); + new DnsNameResolver( + null, name, args, fakeExecutorResource, stopwatch, isAndroid, /* enableSrv= */ false); // By default, using the mocked ResourceResolver to avoid I/O dnsResolver.setResourceResolver(new JndiResourceResolver(recordFetcher)); return dnsResolver; diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolverProvider.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolverProvider.java new file mode 100644 index 00000000000..cd4f24c37fd --- /dev/null +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolverProvider.java @@ -0,0 +1,53 @@ +/* + * Copyright 2019 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.grpclb; + +import io.grpc.Internal; +import io.grpc.internal.BaseDnsNameResolverProvider; + +/** + * A provider for {@code io.grpc.internal.DnsNameResolver} for gRPC lb. + * + *

    It resolves a target URI whose scheme is {@code "dns"}. The (optional) authority of the target + * URI is reserved for the address of alternative DNS server (not implemented yet). The path of the + * target URI, excluding the leading slash {@code '/'}, is treated as the host name and the optional + * port to be resolved by DNS. Example target URIs: + * + *

      + *
    • {@code "dns:///foo.googleapis.com:8080"} (using default DNS)
      • + *
    • {@code "dns://8.8.8.8/foo.googleapis.com:8080"} (using alternative DNS (not implemented + * yet))
      • + *
    • {@code "dns:///foo.googleapis.com"} (without port)
      • + *
    + * + *

    Note: the main difference between {@code io.grpc.DnsNameResolver} is service record is enabled + * by default. + */ +@Internal +public class GrpclbNameResolverProvider extends BaseDnsNameResolverProvider { + + @Override + protected boolean isSrvEnabled() { + return Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true")); + } + + @Override + protected int priority() { + // Must be higher than DnsNameResolverProvider#priority. + return 6; + } +} diff --git a/grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider b/grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider new file mode 100644 index 00000000000..b538b086947 --- /dev/null +++ b/grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider @@ -0,0 +1 @@ +io.grpc.grpclb.GrpclbNameResolverProvider diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java new file mode 100644 index 00000000000..89d373dca7f --- /dev/null +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java @@ -0,0 +1,69 @@ +/* + * Copyright 2019 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.grpclb; + +import static com.google.common.truth.Truth.assertThat; +import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME; + +import javax.annotation.Nullable; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class GrpclbNameResolverProviderTest { + + @Nullable + private String jndiSrvProperty; + + @Before + public void memoizeSrvProperty() { + jndiSrvProperty = System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME); + } + + @After + public void restoreSrvProperty() { + if (jndiSrvProperty == null) { + System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); + } else { + System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, jndiSrvProperty); + } + } + + @Test + public void enableSrv_noDefault() { + System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); + + assertThat(new GrpclbNameResolverProvider().isSrvEnabled()).isTrue(); + } + + @Test + public void enableSrv_defaultEnabled() { + System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true"); + + assertThat(new GrpclbNameResolverProvider().isSrvEnabled()).isTrue(); + } + + @Test + public void enableSrv_defaultDisabled() { + System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false"); + + assertThat(new GrpclbNameResolverProvider().isSrvEnabled()).isFalse(); + } +} \ No newline at end of file From c1f780003050043ba5ddd9d2a697769ff4a91122 Mon Sep 17 00:00:00 2001 From: Jihun Cho Date: Thu, 17 Oct 2019 16:41:15 -0700 Subject: [PATCH 2/4] change test names, javadoc --- .../main/java/io/grpc/internal/BaseDnsNameResolverProvider.java | 2 +- .../test/java/io/grpc/internal/DnsNameResolverProviderTest.java | 2 +- .../java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java index 6a69af0b29c..eef818fe923 100644 --- a/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java @@ -37,7 +37,7 @@ public abstract class BaseDnsNameResolverProvider extends NameResolverProvider { public static final String ENABLE_GRPCLB_PROPERTY_NAME = "io.grpc.internal.DnsNameResolverProvider.enable_grpclb"; - /** Returns boolean value of {@link #ENABLE_GRPCLB_PROPERTY_NAME}. */ + /** Returns boolean value of system property {@link #ENABLE_GRPCLB_PROPERTY_NAME}. */ protected abstract boolean isSrvEnabled(); @Override diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index d1b63e8d334..4c672337e20 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -69,7 +69,7 @@ public void restoreSrvProperty() { } @Test - public void enableSrv_noDefault() { + public void enableSrv_useDefault() { System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); assertThat(new DnsNameResolverProvider().isSrvEnabled()).isFalse(); diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java index 89d373dca7f..940976107e1 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java @@ -47,7 +47,7 @@ public void restoreSrvProperty() { } @Test - public void enableSrv_noDefault() { + public void enableSrv_useDefault() { System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); assertThat(new GrpclbNameResolverProvider().isSrvEnabled()).isTrue(); From fa6419cdc7c1a1644f24d685d69ba733e41feedf Mon Sep 17 00:00:00 2001 From: Jihun Cho Date: Fri, 18 Oct 2019 11:16:54 -0700 Subject: [PATCH 3/4] apply review comments --- .../internal/BaseDnsNameResolverProvider.java | 2 - .../internal/DnsNameResolverProvider.java | 5 +- .../internal/DnsNameResolverProviderTest.java | 46 +------------ ... => SecretGrpclbNameResolverProvider.java} | 30 +++++--- .../services/io.grpc.NameResolverProvider | 2 +- .../GrpclbNameResolverProviderTest.java | 69 ------------------- 6 files changed, 26 insertions(+), 128 deletions(-) rename grpclb/src/main/java/io/grpc/grpclb/{GrpclbNameResolverProvider.java => SecretGrpclbNameResolverProvider.java} (68%) delete mode 100644 grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java diff --git a/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java index eef818fe923..b623ced6b78 100644 --- a/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/BaseDnsNameResolverProvider.java @@ -19,7 +19,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Stopwatch; -import io.grpc.Internal; import io.grpc.InternalServiceProviders; import io.grpc.NameResolver; import io.grpc.NameResolverProvider; @@ -28,7 +27,6 @@ /** * Base provider of name resolvers for name agnostic consumption. */ -@Internal public abstract class BaseDnsNameResolverProvider extends NameResolverProvider { private static final String SCHEME = "dns"; diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index e0702cccd9b..d622cd62a8c 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java @@ -33,9 +33,12 @@ */ public final class DnsNameResolverProvider extends BaseDnsNameResolverProvider { + private static final boolean SRV_ENABLED = + Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false")); + @Override protected boolean isSrvEnabled() { - return Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false")); + return SRV_ENABLED; } @Override diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index 4c672337e20..9dc402a93d6 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -16,8 +16,6 @@ package io.grpc.internal; -import static com.google.common.truth.Truth.assertThat; -import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; @@ -27,9 +25,6 @@ import io.grpc.NameResolver.ServiceConfigParser; import io.grpc.SynchronizationContext; import java.net.URI; -import javax.annotation.Nullable; -import org.junit.After; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -51,52 +46,15 @@ public void uncaughtException(Thread t, Throwable e) { .setServiceConfigParser(mock(ServiceConfigParser.class)) .build(); - @Nullable - private String jndiSrvProperty; - - @Before - public void memoizeSrvProperty() { - jndiSrvProperty = System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME); - } - - @After - public void restoreSrvProperty() { - if (jndiSrvProperty == null) { - System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); - } else { - System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, jndiSrvProperty); - } - } - - @Test - public void enableSrv_useDefault() { - System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); - - assertThat(new DnsNameResolverProvider().isSrvEnabled()).isFalse(); - } - - @Test - public void enableSrv_defaultEnabled() { - System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true"); - - assertThat(new DnsNameResolverProvider().isSrvEnabled()).isTrue(); - } - - @Test - public void enableSrv_defaultDisabled() { - System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false"); - - assertThat(new DnsNameResolverProvider().isSrvEnabled()).isFalse(); - } + private DnsNameResolverProvider provider = new DnsNameResolverProvider(); @Test public void isAvailable() { - assertTrue(new DnsNameResolverProvider().isAvailable()); + assertTrue(provider.isAvailable()); } @Test public void newNameResolver() { - DnsNameResolverProvider provider = new DnsNameResolverProvider(); assertSame(DnsNameResolver.class, provider.newNameResolver(URI.create("dns:///localhost:443"), args).getClass()); assertNull( diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolverProvider.java b/grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java similarity index 68% rename from grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolverProvider.java rename to grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java index cd4f24c37fd..e6d28e5a200 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbNameResolverProvider.java +++ b/grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java @@ -16,7 +16,6 @@ package io.grpc.grpclb; -import io.grpc.Internal; import io.grpc.internal.BaseDnsNameResolverProvider; /** @@ -37,17 +36,26 @@ *

    Note: the main difference between {@code io.grpc.DnsNameResolver} is service record is enabled * by default. */ -@Internal -public class GrpclbNameResolverProvider extends BaseDnsNameResolverProvider { +// Make it package-private so that it cannot be directly referenced by users. Java service loader +// requires the provider to be public, but we can hide it under a package-private class. +final class SecretGrpclbNameResolverProvider { - @Override - protected boolean isSrvEnabled() { - return Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true")); - } + private SecretGrpclbNameResolverProvider() {} + + public static final class Provider extends BaseDnsNameResolverProvider { + + private static final boolean SRV_ENABLED = + Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true")); + + @Override + protected boolean isSrvEnabled() { + return SRV_ENABLED; + } - @Override - protected int priority() { - // Must be higher than DnsNameResolverProvider#priority. - return 6; + @Override + protected int priority() { + // Must be higher than DnsNameResolverProvider#priority. + return 6; + } } } diff --git a/grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider b/grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider index b538b086947..bf02b5e8470 100644 --- a/grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider +++ b/grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider @@ -1 +1 @@ -io.grpc.grpclb.GrpclbNameResolverProvider +io.grpc.grpclb.SecretGrpclbNameResolverProvider$Provider diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java deleted file mode 100644 index 940976107e1..00000000000 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbNameResolverProviderTest.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Copyright 2019 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.grpclb; - -import static com.google.common.truth.Truth.assertThat; -import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME; - -import javax.annotation.Nullable; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class GrpclbNameResolverProviderTest { - - @Nullable - private String jndiSrvProperty; - - @Before - public void memoizeSrvProperty() { - jndiSrvProperty = System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME); - } - - @After - public void restoreSrvProperty() { - if (jndiSrvProperty == null) { - System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); - } else { - System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, jndiSrvProperty); - } - } - - @Test - public void enableSrv_useDefault() { - System.clearProperty(ENABLE_GRPCLB_PROPERTY_NAME); - - assertThat(new GrpclbNameResolverProvider().isSrvEnabled()).isTrue(); - } - - @Test - public void enableSrv_defaultEnabled() { - System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true"); - - assertThat(new GrpclbNameResolverProvider().isSrvEnabled()).isTrue(); - } - - @Test - public void enableSrv_defaultDisabled() { - System.setProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false"); - - assertThat(new GrpclbNameResolverProvider().isSrvEnabled()).isFalse(); - } -} \ No newline at end of file From 7e77e2b70d2393c13e055a0add5d99ea82d379bb Mon Sep 17 00:00:00 2001 From: Jihun Cho Date: Fri, 18 Oct 2019 11:34:07 -0700 Subject: [PATCH 4/4] add tests for default value and priority --- .../internal/DnsNameResolverProvider.java | 2 +- .../internal/DnsNameResolverProviderTest.java | 10 ++++ .../SecretGrpclbNameResolverProvider.java | 2 +- .../SecretGrpclbNameResolverProviderTest.java | 50 +++++++++++++++++++ 4 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 grpclb/src/test/java/io/grpc/grpclb/SecretGrpclbNameResolverProviderTest.java diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index d622cd62a8c..06ff1c85953 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java @@ -42,7 +42,7 @@ protected boolean isSrvEnabled() { } @Override - protected int priority() { + public int priority() { return 5; } } diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index 9dc402a93d6..3f5df9bdbbb 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -16,9 +16,12 @@ package io.grpc.internal; +import static com.google.common.truth.Truth.assertThat; +import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.mock; import io.grpc.NameResolver; @@ -60,4 +63,11 @@ public void newNameResolver() { assertNull( provider.newNameResolver(URI.create("notdns:///localhost:443"), args)); } + + @Test + public void isSrvEnabled_falseByDefault() { + assumeTrue(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME) == null); + + assertThat(provider.isSrvEnabled()).isFalse(); + } } diff --git a/grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java b/grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java index e6d28e5a200..1856c78c14e 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java +++ b/grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java @@ -53,7 +53,7 @@ protected boolean isSrvEnabled() { } @Override - protected int priority() { + public int priority() { // Must be higher than DnsNameResolverProvider#priority. return 6; } diff --git a/grpclb/src/test/java/io/grpc/grpclb/SecretGrpclbNameResolverProviderTest.java b/grpclb/src/test/java/io/grpc/grpclb/SecretGrpclbNameResolverProviderTest.java new file mode 100644 index 00000000000..e5d4b3501f2 --- /dev/null +++ b/grpclb/src/test/java/io/grpc/grpclb/SecretGrpclbNameResolverProviderTest.java @@ -0,0 +1,50 @@ +/* + * Copyright 2019 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.grpclb; + +import static com.google.common.truth.Truth.assertThat; +import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME; +import static org.junit.Assume.assumeTrue; + +import io.grpc.internal.DnsNameResolverProvider; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class SecretGrpclbNameResolverProviderTest { + + @Test + public void priority_shouldBeHigherThanDefaultDnsNameResolver() { + DnsNameResolverProvider defaultDnsNameResolver = new DnsNameResolverProvider(); + SecretGrpclbNameResolverProvider.Provider grpclbDnsNameResolver = + new SecretGrpclbNameResolverProvider.Provider(); + + assertThat(defaultDnsNameResolver.priority()) + .isLessThan(grpclbDnsNameResolver.priority()); + } + + @Test + public void isSrvEnabled_trueByDefault() { + assumeTrue(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME) == null); + + SecretGrpclbNameResolverProvider.Provider grpclbDnsNameResolver = + new SecretGrpclbNameResolverProvider.Provider(); + + assertThat(grpclbDnsNameResolver.isSrvEnabled()).isTrue(); + } +} \ No newline at end of file