From 7b145a9632a285758e2af9dd9f3fbb1eef4f8e2c Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Tue, 22 Aug 2023 09:50:55 -0700 Subject: [PATCH 01/14] Handle DNS SERVFAIL differently that NXDOMAIN --- .../dns/discovery/netty/DefaultDnsClient.java | 9 +++- .../discovery/netty/DefaultDnsClientTest.java | 47 ++++++++++++++++++- .../dns/discovery/netty/TestDnsServer.java | 2 + .../dns/discovery/netty/TestRecordStore.java | 21 ++++++++- .../src/test/resources/log4j2.xml | 0 5 files changed, 76 insertions(+), 3 deletions(-) create mode 100644 servicetalk-dns-discovery-netty/src/test/resources/log4j2.xml diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java index d508e5d0b8..08b7b17147 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java @@ -42,10 +42,12 @@ import io.netty.handler.codec.dns.DefaultDnsQuestion; import io.netty.handler.codec.dns.DnsRawRecord; import io.netty.handler.codec.dns.DnsRecord; +import io.netty.handler.codec.dns.DnsResponseCode; import io.netty.resolver.ResolvedAddressTypes; import io.netty.resolver.dns.DefaultAuthoritativeDnsServerCache; import io.netty.resolver.dns.DefaultDnsCache; import io.netty.resolver.dns.DefaultDnsCnameCache; +import io.netty.resolver.dns.DnsErrorCauseException; import io.netty.resolver.dns.DnsNameResolver; import io.netty.resolver.dns.DnsNameResolverBuilder; import io.netty.resolver.dns.NameServerComparator; @@ -955,7 +957,7 @@ private static Publisher>> AbstractDnsPublisher pub, boolean generateAggregateEvent) { return pub.onErrorResume(cause -> { AbstractDnsPublisher.AbstractDnsSubscription subscription = pub.subscription; - if (subscription != null) { + if (subscription != null && !isServFailCaused(cause)) { List> events = subscription.generateInactiveEvent(); if (!events.isEmpty()) { return (generateAggregateEvent ? Publisher.>>from( @@ -968,6 +970,11 @@ private static Publisher>> }); } + private static boolean isServFailCaused(final Throwable t) { + return t.getCause() != null && t.getCause() instanceof DnsErrorCauseException && + DnsResponseCode.SERVFAIL.equals(((DnsErrorCauseException) t.getCause()).getCode()); + } + private static Publisher newDuplicateSrv(String serviceName, String resolvedAddress) { return failed(new IllegalStateException("Duplicate SRV entry for SRV name " + serviceName + " for address " + resolvedAddress)); diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index 9da0d16e78..a45fc3a069 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -1,5 +1,5 @@ /* - * Copyright © 2018-2020 Apple Inc. and the ServiceTalk project authors + * Copyright © 2018-2020,2023 Apple Inc. and the ServiceTalk project authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -31,6 +31,7 @@ import io.servicetalk.transport.netty.internal.EventLoopAwareNettyIoExecutor; import io.netty.channel.EventLoopGroup; +import org.apache.directory.server.dns.messages.RecordType; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -446,6 +447,37 @@ void srvRecordRemovalPropagatesError() throws Exception { assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } + @Test + void srvFailResumesCorrectly() throws Exception { + setup(); + final String domain = "sd.servicetalk.io"; + final String targetDomain1 = "target1.mysvc.servicetalk.io"; + final int targetPort = 9876; + final String ip1 = nextIp(); + recordStore.addSrv(domain, targetDomain1, targetPort, DEFAULT_TTL); + recordStore.addIPv4Address(targetDomain1, DEFAULT_TTL, ip1); + + TestPublisherSubscriber> subscriber = dnsSrvQueryWithInfRetry(domain); + Subscription subscription = subscriber.awaitSubscription(); + subscription.request(10); + + List> signals = subscriber.takeOnNext(1); + assertHasEvent(signals, ip1, targetPort, AVAILABLE); + + // Fail subsequent A queries with SERVFAIL + recordStore.servFail.set(new TestRecordStore.ServFail("target1", RecordType.A)); + advanceTime(); + // Expect no changes (SERVFAIL should be retried indefinitely) + assertThat(subscriber.pollOnNext(50, MILLISECONDS), is(nullValue())); + + // Restore functionality for A queries + recordStore.servFail.set(null); + advanceTime(); + + List> resumeSignals = subscriber.takeOnNext(1); + assertHasEvent(resumeSignals, ip1, targetPort, AVAILABLE); + } + @ParameterizedTest(name = "{displayName} [{index}] srvFilterDuplicateEvents={0}") @ValueSource(booleans = {false, true}) void srvDuplicateAddresses(boolean srvFilterDuplicateEvents) throws Exception { @@ -1155,6 +1187,19 @@ private TestPublisherSubscriber> dnsSr return subscriber; } + private TestPublisherSubscriber> dnsSrvQueryWithInfRetry(String domain) { + Publisher> publisher = client.dnsSrvQuery(domain) + .retry((__, err) -> { + err.printStackTrace(); + return true; + }) + .flatMapConcatIterable(identity()); + TestPublisherSubscriber> subscriber = + new TestPublisherSubscriber<>(); + toSource(publisher).subscribe(subscriber); + return subscriber; + } + private TestPublisherSubscriber> dnsQuery(String domain) { return dnsQuery(domain, (i, t) -> Completable.failed(t)); } diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java index ef29379d35..572a7c2299 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java @@ -47,6 +47,8 @@ import java.net.UnknownHostException; import java.util.Set; +import javax.annotation.Nullable; + import static java.util.Objects.requireNonNull; class TestDnsServer extends DnsServer { diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java index f3eabfb114..8a89476af5 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java @@ -15,6 +15,7 @@ */ package io.servicetalk.dns.discovery.netty; +import org.apache.directory.server.dns.DnsException; import org.apache.directory.server.dns.messages.QuestionRecord; import org.apache.directory.server.dns.messages.RecordClass; import org.apache.directory.server.dns.messages.RecordType; @@ -32,6 +33,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; import static java.util.Collections.emptySet; @@ -39,6 +41,7 @@ import static org.apache.directory.server.dns.messages.RecordType.AAAA; import static org.apache.directory.server.dns.messages.RecordType.CNAME; import static org.apache.directory.server.dns.messages.RecordType.SRV; +import static org.apache.directory.server.dns.messages.ResponseCode.SERVER_FAILURE; final class TestRecordStore implements RecordStore { private static final Logger LOGGER = LoggerFactory.getLogger(TestRecordStore.class); @@ -47,6 +50,18 @@ final class TestRecordStore implements RecordStore { private final Map>> recordsToReturnByDomain = new ConcurrentHashMap<>(); + static class ServFail { + private final String name; + private final RecordType type; + + ServFail(final String name, final RecordType type) { + this.name = name; + this.type = type; + } + } + + public final AtomicReference servFail = new AtomicReference<>(null); + public synchronized void addSrv(final String domain, String targetDomain, final int port, final int ttl) { addSrv(domain, targetDomain, port, ttl, SRV_DEFAULT_WEIGHT, SRV_DEFAULT_PRIORITY); } @@ -168,8 +183,12 @@ private boolean removeRecords(ResourceRecord rr, List recordList @Nullable @Override - public synchronized Set getRecords(final QuestionRecord questionRecord) { + public synchronized Set getRecords(final QuestionRecord questionRecord) throws DnsException { final String domain = questionRecord.getDomainName(); + final ServFail fail = servFail.get(); + if (fail != null && domain.contains(fail.name) && fail.type.convert() >= questionRecord.getRecordType().convert()) { + throw new DnsException(SERVER_FAILURE); + } final Map> recordsToReturn = recordsToReturnByDomain.get(domain); LOGGER.debug("Getting {} records for {}", questionRecord.getRecordType(), domain); if (recordsToReturn != null) { diff --git a/servicetalk-dns-discovery-netty/src/test/resources/log4j2.xml b/servicetalk-dns-discovery-netty/src/test/resources/log4j2.xml new file mode 100644 index 0000000000..e69de29bb2 From 23c4c3ae018142327d85d0d4a17af0e880139fac Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Mon, 11 Dec 2023 17:55:39 -0800 Subject: [PATCH 02/14] nxdomain behavior change --- .../dns/discovery/netty/DefaultDnsClient.java | 39 ++++++--- .../DefaultDnsServiceDiscovererBuilder.java | 8 +- .../discovery/netty/DefaultDnsClientTest.java | 87 ++++++++++--------- 3 files changed, 73 insertions(+), 61 deletions(-) diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java index 08b7b17147..bdaf570098 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java @@ -42,7 +42,6 @@ import io.netty.handler.codec.dns.DefaultDnsQuestion; import io.netty.handler.codec.dns.DnsRawRecord; import io.netty.handler.codec.dns.DnsRecord; -import io.netty.handler.codec.dns.DnsResponseCode; import io.netty.resolver.ResolvedAddressTypes; import io.netty.resolver.dns.DefaultAuthoritativeDnsServerCache; import io.netty.resolver.dns.DefaultDnsCache; @@ -82,6 +81,7 @@ import static io.netty.handler.codec.dns.DefaultDnsRecordDecoder.decodeName; import static io.netty.handler.codec.dns.DnsRecordType.SRV; +import static io.netty.handler.codec.dns.DnsResponseCode.NXDOMAIN; import static io.servicetalk.client.api.ServiceDiscovererEvent.Status.AVAILABLE; import static io.servicetalk.concurrent.api.AsyncCloseables.toAsyncCloseable; import static io.servicetalk.concurrent.api.Completable.completed; @@ -107,6 +107,7 @@ import static io.servicetalk.transport.netty.internal.EventLoopAwareNettyIoExecutors.toEventLoopAwareNettyIoExecutor; import static io.servicetalk.utils.internal.ThrowableUtils.addSuppressed; import static java.lang.Integer.toHexString; +import static java.lang.System.getProperty; import static java.lang.System.identityHashCode; import static java.nio.ByteBuffer.wrap; import static java.util.Collections.emptyList; @@ -120,6 +121,8 @@ final class DefaultDnsClient implements DnsClient { private static final Logger LOGGER = LoggerFactory.getLogger(DefaultDnsClient.class); + private static final String NX_DOMAIN_INVALIDATES_PROPERTY = "io.servicetalk.dns.discovery.nxdomain.invalidation"; + private static final boolean NX_DOMAIN_INVALIDATES = parseProperty(NX_DOMAIN_INVALIDATES_PROPERTY, true); private static final Comparator INET_ADDRESS_COMPARATOR = comparing(o -> wrap(o.getAddress())); private static final Comparator HOST_AND_PORT_COMPARATOR = comparing(HostAndPort::hostName) .thenComparingInt(HostAndPort::port); @@ -137,7 +140,6 @@ final class DefaultDnsClient implements DnsClient { private final IntFunction srvHostNameRepeater; private final int srvConcurrency; private final boolean srvFilterDuplicateEvents; - private final boolean inactiveEventsOnError; private final DnsResolverAddressTypes addressTypes; private final String id; private boolean closed; @@ -145,7 +147,7 @@ final class DefaultDnsClient implements DnsClient { DefaultDnsClient(final String id, final IoExecutor ioExecutor, final int consolidateCacheSize, final int minTTL, final int maxTTL, final int minCacheTTL, final int maxCacheTTL, final int negativeTTLCacheSeconds, final long ttlJitterNanos, - final int srvConcurrency, final boolean inactiveEventsOnError, + final int srvConcurrency, final boolean completeOncePreferredResolved, final boolean srvFilterDuplicateEvents, Duration srvHostNameRepeatInitialDelay, Duration srvHostNameRepeatJitter, @Nullable Integer maxUdpPayloadSize, @Nullable final Integer ndots, @@ -157,7 +159,6 @@ final class DefaultDnsClient implements DnsClient { final ServiceDiscovererEvent.Status missingRecordStatus) { this.srvConcurrency = srvConcurrency; this.srvFilterDuplicateEvents = srvFilterDuplicateEvents; - this.inactiveEventsOnError = inactiveEventsOnError; // Implementation of this class expects to use only single EventLoop from IoExecutor this.nettyIoExecutor = toEventLoopAwareNettyIoExecutor(ioExecutor).next(); // We must use nettyIoExecutor for the repeater for thread safety! @@ -254,9 +255,8 @@ public Publisher>> dnsQuery(final return defer(() -> { final DnsDiscoveryObserver discoveryObserver = newDiscoveryObserver(address); ARecordPublisher pub = new ARecordPublisher(address, discoveryObserver); - Publisher>> events = inactiveEventsOnError ? - recoverWithInactiveEvents(pub, false) : - pub; + Publisher>> events = + recoverWithInactiveEvents(pub, false); return discoveryObserver == null ? events : events.beforeFinally(new TerminalSignalConsumer() { @Override public void onComplete() { @@ -340,7 +340,7 @@ public Publisher>> dnsSrvQu return empty(); } }, srvConcurrency) - .liftSync(inactiveEventsOnError ? SrvInactiveCombinerOperator.EMIT : SrvInactiveCombinerOperator.NO_EMIT); + .liftSync(SrvInactiveCombinerOperator.EMIT); return discoveryObserver == null ? events : events.beforeFinally(new TerminalSignalConsumer() { @Override @@ -782,7 +782,7 @@ private void cancel0() { private void cancelAndTerminate0(Throwable cause) { assertInEventloop(); - LOGGER.debug("{} subscription for {} will be cancelled and terminated with an error.", + LOGGER.warn("{} subscription for {} will be cancelled and terminated with an error.", DefaultDnsClient.this, AbstractDnsPublisher.this, cause); try { cancel0(); @@ -957,7 +957,7 @@ private static Publisher>> AbstractDnsPublisher pub, boolean generateAggregateEvent) { return pub.onErrorResume(cause -> { AbstractDnsPublisher.AbstractDnsSubscription subscription = pub.subscription; - if (subscription != null && !isServFailCaused(cause)) { + if (subscription != null && shouldRevokeState(cause)) { List> events = subscription.generateInactiveEvent(); if (!events.isEmpty()) { return (generateAggregateEvent ? Publisher.>>from( @@ -970,9 +970,12 @@ private static Publisher>> }); } - private static boolean isServFailCaused(final Throwable t) { - return t.getCause() != null && t.getCause() instanceof DnsErrorCauseException && - DnsResponseCode.SERVFAIL.equals(((DnsErrorCauseException) t.getCause()).getCode()); + private static boolean shouldRevokeState(final Throwable t) { + // ISE => Subscriber exceptions (downstream of retry) + return t instanceof SrvAddressRemovedException || t instanceof IllegalStateException || + t instanceof ClosedDnsServiceDiscovererException || (NX_DOMAIN_INVALIDATES && t.getCause() != null && + t.getCause() instanceof DnsErrorCauseException && + NXDOMAIN.equals(((DnsErrorCauseException) t.getCause()).getCode())); } private static Publisher newDuplicateSrv(String serviceName, String resolvedAddress) { @@ -989,7 +992,6 @@ private static final class SrvInactiveCombinerOperator implements PublisherOperator>, Collection>> { static final SrvInactiveCombinerOperator EMIT = new SrvInactiveCombinerOperator(true); - static final SrvInactiveCombinerOperator NO_EMIT = new SrvInactiveCombinerOperator(false); private final boolean emitAggregatedEvents; private SrvInactiveCombinerOperator(boolean emitAggregatedEvents) { @@ -1108,4 +1110,13 @@ static SrvAddressRemovedException newInstance(Class clazz, String method) { return unknownStackTrace(new SrvAddressRemovedException(), clazz, method); } } + + @Nullable + private static boolean parseProperty(final String name, final boolean defaultValue) { + final String value = getProperty(name); + if (value == null) { + return defaultValue; + } + return Boolean.parseBoolean(value); + } } diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java index 0819a9e40b..be12428b79 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java @@ -128,7 +128,6 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco private int negativeTTLCacheSeconds = DEFAULT_NEGATIVE_TTL_CACHE_SECONDS; private Duration ttlJitter = ofSeconds(DEFAULT_TTL_POLL_JITTER_SECONDS); private int srvConcurrency = 2048; - private boolean inactiveEventsOnError; private boolean completeOncePreferredResolved = true; private boolean srvFilterDuplicateEvents; private Duration srvHostNameRepeatInitialDelay = ofSeconds(10); @@ -300,11 +299,6 @@ public DefaultDnsServiceDiscovererBuilder missingRecordStatus(ServiceDiscovererE return asHostAndPortDiscoverer(build()); } - DefaultDnsServiceDiscovererBuilder inactiveEventsOnError(boolean inactiveEventsOnError) { - this.inactiveEventsOnError = inactiveEventsOnError; - return this; - } - DefaultDnsServiceDiscovererBuilder srvConcurrency(int srvConcurrency) { this.srvConcurrency = ensurePositive(srvConcurrency, "srvConcurrency"); return this; @@ -369,7 +363,7 @@ DnsClient build() { ioExecutor == null ? globalExecutionContext().ioExecutor() : ioExecutor, consolidateCacheSize, minTTLSeconds, maxTTLSeconds, minTTLCacheSeconds, maxTTLCacheSeconds, negativeTTLCacheSeconds, ttlJitter.toNanos(), - srvConcurrency, inactiveEventsOnError, completeOncePreferredResolved, srvFilterDuplicateEvents, + srvConcurrency, completeOncePreferredResolved, srvFilterDuplicateEvents, srvHostNameRepeatInitialDelay, srvHostNameRepeatJitter, maxUdpPayloadSize, ndots, optResourceEnabled, queryTimeout, dnsResolverAddressTypes, localAddress, dnsServerAddressStreamProvider, observer, missingRecordStatus); diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index a45fc3a069..2f79d0315a 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -316,13 +316,11 @@ void srvWithCNAMEEntryLowerTTLDoesNotFail() throws Exception { assertNull(subscriber.pollTerminal(50, MILLISECONDS)); } - @ParameterizedTest(name = "{displayName} [{index}] inactiveEventsOnError={0}") - @ValueSource(booleans = {false, true}) - void srvCNAMEDuplicateAddresses(boolean inactiveEventsOnError) throws Exception { + @Test + void srvCNAMEDuplicateAddresses() throws Exception { setup(builder -> builder .dnsServerAddressStreamProvider(new SequentialDnsServerAddressStreamProvider( - dnsServer2.localAddress(), dnsServer.localAddress())) - .inactiveEventsOnError(inactiveEventsOnError)); + dnsServer2.localAddress(), dnsServer.localAddress()))); final String domain = "sd.servicetalk.io"; final String srvCNAME = "sdcname.servicetalk.io"; final String targetDomain1 = "target1.mysvc.servicetalk.io"; @@ -354,20 +352,16 @@ void srvCNAMEDuplicateAddresses(boolean inactiveEventsOnError) throws Exception createSrvRecord(domain, targetDomain2, targetPort, ttl)); advanceTime(); - if (inactiveEventsOnError) { - signals = subscriber.takeOnNext(2); - assertHasEvent(signals, ip1, targetPort, EXPIRED); - assertHasEvent(signals, ip2, targetPort, EXPIRED); - } else { - assertThat(subscriber.pollOnNext(50, MILLISECONDS), is(nullValue())); - } + signals = subscriber.takeOnNext(2); + assertHasEvent(signals, ip1, targetPort, EXPIRED); + assertHasEvent(signals, ip2, targetPort, EXPIRED); assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @ParameterizedTest(name = "{displayName} [{index}] missingRecordStatus={0}") @MethodSource("missingRecordStatus") void srvInactiveEventsAggregated(ServiceDiscovererEvent.Status missingRecordStatus) throws Exception { - setup(builder -> builder.inactiveEventsOnError(true).missingRecordStatus(missingRecordStatus)); + setup(builder -> builder.missingRecordStatus(missingRecordStatus)); final String domain = "sd.servicetalk.io"; final String targetDomain1 = "target1.mysvc.servicetalk.io"; final String targetDomain2 = "target2.mysvc.servicetalk.io"; @@ -443,7 +437,9 @@ void srvRecordRemovalPropagatesError() throws Exception { createSrvRecord(domain, targetDomain1, targetPort, DEFAULT_TTL), createSrvRecord(domain, targetDomain2, targetPort, DEFAULT_TTL)); advanceTime(); - assertThat(subscriber.pollOnNext(50, MILLISECONDS), is(nullValue())); + signals = subscriber.takeOnNext(2); + assertHasEvent(signals, ip1, targetPort, EXPIRED); + assertHasEvent(signals, ip2, targetPort, EXPIRED); assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -479,7 +475,7 @@ void srvFailResumesCorrectly() throws Exception { } @ParameterizedTest(name = "{displayName} [{index}] srvFilterDuplicateEvents={0}") - @ValueSource(booleans = {false, true}) + @ValueSource(booleans = {false}) void srvDuplicateAddresses(boolean srvFilterDuplicateEvents) throws Exception { setup(builder -> builder.srvFilterDuplicateEvents(srvFilterDuplicateEvents)); final String domain = "sd.servicetalk.io"; @@ -517,11 +513,9 @@ void srvDuplicateAddresses(boolean srvFilterDuplicateEvents) throws Exception { assertEvent(subscriber.takeOnNext(), ip1, targetPort, EXPIRED); } - @ParameterizedTest(name = "{displayName} [{index}] inactiveEventsOnError={0}") - @ValueSource(booleans = {false, true}) - void srvAAAAFailsGeneratesInactive(boolean inactiveEventsOnError) throws Exception { + @Test + void srvAAAAFailsGeneratesInactive() throws Exception { setup(builder -> builder - .inactiveEventsOnError(inactiveEventsOnError) .srvHostNameRepeatDelay(ofMillis(200), ofMillis(10)) .dnsResolverAddressTypes(IPV4_PREFERRED)); final String domain = "sd.servicetalk.io"; @@ -553,10 +547,9 @@ void srvAAAAFailsGeneratesInactive(boolean inactiveEventsOnError) throws Excepti assertEvent(subscriber.takeOnNext(), ip1, targetPort, AVAILABLE); } - @ParameterizedTest(name = "{displayName} [{index}] inactiveEventsOnError={0}") - @ValueSource(booleans = {false, true}) - void srvRecordFailsGeneratesInactive(boolean inactiveEventsOnError) throws Exception { - setup(builder -> builder.inactiveEventsOnError(inactiveEventsOnError)); + @Test + void srvRecordFailsGeneratesInactive() throws Exception { + setup(); final String domain = "sd.servicetalk.io"; final String targetDomain1 = "target1.mysvc.servicetalk.io"; final String targetDomain2 = "target2.mysvc.servicetalk.io"; @@ -583,9 +576,7 @@ void srvRecordFailsGeneratesInactive(boolean inactiveEventsOnError) throws Excep recordStore.removeSrv(domain, targetDomain2, targetPort, DEFAULT_TTL); advanceTime(); - if (inactiveEventsOnError) { - assertEvent(subscriber.takeOnNext(), ip2, targetPort, EXPIRED); - } + assertEvent(subscriber.takeOnNext(), ip2, targetPort, EXPIRED); assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -600,23 +591,25 @@ void unknownHostDiscover() throws Exception { } @Test - void singleADiscover() throws Exception { + void unknownHostAfterKnownDiscover() throws Exception { setup(); - final String ip = nextIp(); - final String domain = "servicetalk.io"; - recordStore.addIPv4Address(domain, DEFAULT_TTL, ip); + final String targetDomain1 = "sd.domain.com"; + final String ip1 = nextIp(); - TestPublisherSubscriber> subscriber = dnsQuery(domain); + recordStore.addIPv4Address(targetDomain1, DEFAULT_TTL, ip1); + + TestPublisherSubscriber> subscriber = dnsQuery(targetDomain1); Subscription subscription = subscriber.awaitSubscription(); - subscription.request(1); + subscription.request(Long.MAX_VALUE); - assertEvent(subscriber.takeOnNext(), ip, AVAILABLE); + List> signals = subscriber.takeOnNext(1); + assertHasEvent(signals, ip1, AVAILABLE); - // Remove the ip - recordStore.removeIPv4Address(domain, DEFAULT_TTL, ip); - subscription.request(1); + recordStore.removeIPv4Addresses(targetDomain1); advanceTime(); - assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); + + signals = subscriber.takeOnNext(1); + assertHasEvent(signals, ip1, EXPIRED); } @Test @@ -636,8 +629,12 @@ void singleDiscoverMultipleRecords() throws Exception { // Remove all the ips recordStore.removeIPv4Address(domain, DEFAULT_TTL, ips); - subscription.request(1); + subscription.request(ips.length); advanceTime(); + signals = subscriber.takeOnNext(ips.length); + for (String ip : ips) { + assertHasEvent(signals, ip, EXPIRED); + } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -694,8 +691,15 @@ void repeatDiscoverMultipleRecords() throws Exception { // Remove all the IPs recordStore.removeIPv4Address(domain, DEFAULT_TTL, ips); recordStore.removeIPv4Address(domain, DEFAULT_TTL, ips2); - subscription.request(1); + subscription.request(ips.length + ips2.length); advanceTime(); + signals = subscriber.takeOnNext(ips.length + ips2.length); + for (String ip : ips) { + assertHasEvent(signals, ip, EXPIRED); + } + for (String ip : ips2) { + assertHasEvent(signals, ip, EXPIRED); + } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -726,7 +730,9 @@ void repeatDiscoverMultipleHosts() throws Exception { subscription1.request(1); subscription2.request(1); advanceTime(); + assertEvent(subscriber1.takeOnNext(), ip1, EXPIRED); assertThat(subscriber1.awaitOnError(), instanceOf(UnknownHostException.class)); + assertEvent(subscriber2.takeOnNext(), ip2, EXPIRED); assertThat(subscriber2.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -734,7 +740,6 @@ void repeatDiscoverMultipleHosts() throws Exception { @MethodSource("missingRecordStatus") void repeatDiscoverNxDomainAndRecover(ServiceDiscovererEvent.Status missingRecordStatus) throws Exception { setup(builder -> builder - .inactiveEventsOnError(true) .missingRecordStatus(missingRecordStatus)); final String ip = nextIp(); final String domain = "servicetalk.io"; @@ -960,6 +965,7 @@ void preferIpv4ButOnlyAAAARecordIsPresent(DnsResolverAddressTypes addressTypes) // Remove all ips recordStore.removeIPv6Address(domain, DEFAULT_TTL, ipv6); advanceTime(); + assertEvent(subscriber.takeOnNext(), ipv6, EXPIRED); assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -980,6 +986,7 @@ void preferIpv6ButOnlyARecordIsPresent(DnsResolverAddressTypes addressTypes) thr // Remove all ips recordStore.removeIPv4Address(domain, DEFAULT_TTL, ipv4); advanceTime(); + assertEvent(subscriber.takeOnNext(), ipv4, EXPIRED); assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } From 2d7fa0bb24ba4896920cf566cce9ac2a4cf6723b Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Mon, 11 Dec 2023 18:05:36 -0800 Subject: [PATCH 03/14] comments --- .../discovery/netty/DefaultDnsClientTest.java | 5 +++-- .../dns/discovery/netty/TestDnsServer.java | 2 -- .../dns/discovery/netty/TestRecordStore.java | 17 ++++++++++++++--- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index 2f79d0315a..597008b903 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -461,13 +461,14 @@ void srvFailResumesCorrectly() throws Exception { assertHasEvent(signals, ip1, targetPort, AVAILABLE); // Fail subsequent A queries with SERVFAIL - recordStore.servFail.set(new TestRecordStore.ServFail("target1", RecordType.A)); + final TestRecordStore.ServFail fail = new TestRecordStore.ServFail("target1", RecordType.A); + recordStore.addFail(fail); advanceTime(); // Expect no changes (SERVFAIL should be retried indefinitely) assertThat(subscriber.pollOnNext(50, MILLISECONDS), is(nullValue())); // Restore functionality for A queries - recordStore.servFail.set(null); + recordStore.removeFail(fail); advanceTime(); List> resumeSignals = subscriber.takeOnNext(1); diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java index 572a7c2299..ef29379d35 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestDnsServer.java @@ -47,8 +47,6 @@ import java.net.UnknownHostException; import java.util.Set; -import javax.annotation.Nullable; - import static java.util.Objects.requireNonNull; class TestDnsServer extends DnsServer { diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java index 8a89476af5..b953fa7572 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java @@ -58,9 +58,21 @@ static class ServFail { this.name = name; this.type = type; } + + static ServFail of(final QuestionRecord question) { + return new ServFail(question.getDomainName(), question.getRecordType()); + } + } + + private final Set failSet = new HashSet<>(null); + + public synchronized void addFail(final ServFail fail) { + failSet.add(fail); } - public final AtomicReference servFail = new AtomicReference<>(null); + public synchronized void removeFail(final ServFail fail) { + failSet.remove(fail); + } public synchronized void addSrv(final String domain, String targetDomain, final int port, final int ttl) { addSrv(domain, targetDomain, port, ttl, SRV_DEFAULT_WEIGHT, SRV_DEFAULT_PRIORITY); @@ -185,8 +197,7 @@ private boolean removeRecords(ResourceRecord rr, List recordList @Override public synchronized Set getRecords(final QuestionRecord questionRecord) throws DnsException { final String domain = questionRecord.getDomainName(); - final ServFail fail = servFail.get(); - if (fail != null && domain.contains(fail.name) && fail.type.convert() >= questionRecord.getRecordType().convert()) { + if (failSet.contains(ServFail.of(questionRecord))) { throw new DnsException(SERVER_FAILURE); } final Map> recordsToReturn = recordsToReturnByDomain.get(domain); From d0250791b1622cb7125c24bf3df5c2202caa92e8 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Tue, 12 Dec 2023 09:10:48 -0800 Subject: [PATCH 04/14] Update servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java Co-authored-by: Bryce Anderson --- .../io/servicetalk/dns/discovery/netty/DefaultDnsClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java index bdaf570098..9191c83bb9 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java @@ -973,7 +973,7 @@ private static Publisher>> private static boolean shouldRevokeState(final Throwable t) { // ISE => Subscriber exceptions (downstream of retry) return t instanceof SrvAddressRemovedException || t instanceof IllegalStateException || - t instanceof ClosedDnsServiceDiscovererException || (NX_DOMAIN_INVALIDATES && t.getCause() != null && + t instanceof ClosedDnsServiceDiscovererException || (NX_DOMAIN_INVALIDATES && t.getCause() instanceof DnsErrorCauseException && NXDOMAIN.equals(((DnsErrorCauseException) t.getCause()).getCode())); } From b4b37763cc62cc52fea83239b6e82376a20ea388 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Tue, 12 Dec 2023 09:11:34 -0800 Subject: [PATCH 05/14] comments --- .../io/servicetalk/dns/discovery/netty/TestRecordStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java index b953fa7572..d2db33929a 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java @@ -64,7 +64,7 @@ static ServFail of(final QuestionRecord question) { } } - private final Set failSet = new HashSet<>(null); + private final Set failSet = new HashSet<>(); public synchronized void addFail(final ServFail fail) { failSet.add(fail); From ced7e5ff492b9ffcc415611a3395be9c4416c06f Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Thu, 18 Jan 2024 13:31:13 -0800 Subject: [PATCH 06/14] checkstyle --- .../servicetalk/dns/discovery/netty/DefaultDnsClientTest.java | 2 +- .../io/servicetalk/dns/discovery/netty/TestRecordStore.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index 597008b903..fa1cc01edc 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -1,5 +1,5 @@ /* - * Copyright © 2018-2020,2023 Apple Inc. and the ServiceTalk project authors + * Copyright © 2018-2020, 2023 Apple Inc. and the ServiceTalk project authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java index d2db33929a..976f44f510 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java @@ -33,7 +33,6 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; import static java.util.Collections.emptySet; From 52da3753c47a5c719ebe345cc00ee0e4d1b365d4 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Fri, 16 Feb 2024 20:01:18 -0800 Subject: [PATCH 07/14] improvements --- .../DefaultDnsServiceDiscovererBuilder.java | 5 ++++ .../discovery/netty/DefaultDnsClientTest.java | 11 +++++--- .../dns/discovery/netty/TestRecordStore.java | 26 +++++++++++++++++++ 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java index be12428b79..dad9919a0c 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java @@ -313,6 +313,11 @@ DefaultDnsServiceDiscovererBuilder srvHostNameRepeatDelay( Duration initialDelay, Duration jitter) { this.srvHostNameRepeatInitialDelay = requireNonNull(initialDelay); this.srvHostNameRepeatJitter = requireNonNull(jitter); + ensurePositive(srvHostNameRepeatInitialDelay, "srvHostNameRepeatInitialDelay"); + ensurePositive(srvHostNameRepeatJitter, "srvHostNameRepeatJitter"); + if (srvHostNameRepeatJitter.toNanos() >= srvHostNameRepeatInitialDelay.toNanos()) { + throw new IllegalArgumentException("The jitter value should be less than the initial delay."); + } return this; } diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index fa1cc01edc..94ddce118a 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -79,6 +79,7 @@ import static io.servicetalk.transport.netty.internal.NettyIoExecutors.createIoExecutor; import static java.net.InetAddress.getByName; import static java.time.Duration.ofMillis; +import static java.time.Duration.ofNanos; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.function.Function.identity; @@ -461,14 +462,15 @@ void srvFailResumesCorrectly() throws Exception { assertHasEvent(signals, ip1, targetPort, AVAILABLE); // Fail subsequent A queries with SERVFAIL - final TestRecordStore.ServFail fail = new TestRecordStore.ServFail("target1", RecordType.A); - recordStore.addFail(fail); + final TestRecordStore.ServFail aFail = new TestRecordStore.ServFail(targetDomain1, RecordType.A); + recordStore.addFail(aFail); advanceTime(); + // Expect no changes (SERVFAIL should be retried indefinitely) assertThat(subscriber.pollOnNext(50, MILLISECONDS), is(nullValue())); // Restore functionality for A queries - recordStore.removeFail(fail); + recordStore.removeFail(aFail); advanceTime(); List> resumeSignals = subscriber.takeOnNext(1); @@ -1233,7 +1235,8 @@ private DefaultDnsServiceDiscovererBuilder dnsClientBuilder() { .dnsServerAddressStreamProvider(new SingletonDnsServerAddressStreamProvider(dnsServer.localAddress())) .ndots(1) .ttl(1, 5, 0, 0) - .ttlJitter(Duration.ofNanos(1)); + .ttlJitter(Duration.ofNanos(1)) + .srvHostNameRepeatDelay(ofNanos(10), ofNanos(1)); } private static void assertEvent(@Nullable ServiceDiscovererEvent event, diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java index 976f44f510..0cdff5c0c6 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/TestRecordStore.java @@ -31,6 +31,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import javax.annotation.Nullable; @@ -61,6 +62,31 @@ static class ServFail { static ServFail of(final QuestionRecord question) { return new ServFail(question.getDomainName(), question.getRecordType()); } + + @Override + public String toString() { + return "ServFail{" + + "name='" + name + '\'' + + ", type=" + type + + '}'; + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + final ServFail fail = (ServFail) o; + return Objects.equals(name, fail.name) && type == fail.type; + } + + @Override + public int hashCode() { + return Objects.hash(name, type); + } } private final Set failSet = new HashSet<>(); From c64758d067235c62116b4f3d82a1e8def336887c Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Fri, 22 Mar 2024 11:15:13 -0700 Subject: [PATCH 08/14] bump netty --- .../servicetalk/dns/discovery/netty/DefaultDnsClientTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index 94ddce118a..dcac662ec4 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -1200,7 +1200,7 @@ private TestPublisherSubscriber> dnsSr private TestPublisherSubscriber> dnsSrvQueryWithInfRetry(String domain) { Publisher> publisher = client.dnsSrvQuery(domain) .retry((__, err) -> { - err.printStackTrace(); + LOGGER.error("Retrying error ", err); return true; }) .flatMapConcatIterable(identity()); From 4ef7d68214ac4ce348f979e91eefcbf48fa87038 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Tue, 26 Mar 2024 21:56:00 -0700 Subject: [PATCH 09/14] comments --- .../dns/discovery/netty/DefaultDnsClient.java | 20 +++------ .../DefaultDnsServiceDiscovererBuilder.java | 17 ++++++-- .../discovery/netty/DefaultDnsClientTest.java | 43 ++++++++++++------- .../src/test/resources/log4j2.xml | 0 4 files changed, 45 insertions(+), 35 deletions(-) delete mode 100644 servicetalk-dns-discovery-netty/src/test/resources/log4j2.xml diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java index 9191c83bb9..ec769cd28f 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java @@ -46,7 +46,6 @@ import io.netty.resolver.dns.DefaultAuthoritativeDnsServerCache; import io.netty.resolver.dns.DefaultDnsCache; import io.netty.resolver.dns.DefaultDnsCnameCache; -import io.netty.resolver.dns.DnsErrorCauseException; import io.netty.resolver.dns.DnsNameResolver; import io.netty.resolver.dns.DnsNameResolverBuilder; import io.netty.resolver.dns.NameServerComparator; @@ -97,6 +96,7 @@ import static io.servicetalk.concurrent.internal.SubscriberUtils.newExceptionForInvalidRequestN; import static io.servicetalk.concurrent.internal.SubscriberUtils.safeOnError; import static io.servicetalk.concurrent.internal.ThrowableUtils.unknownStackTrace; +import static io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.NX_DOMAIN_INVALIDATES; import static io.servicetalk.dns.discovery.netty.DnsClients.mapEventList; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV6_PREFERRED; @@ -121,8 +121,6 @@ final class DefaultDnsClient implements DnsClient { private static final Logger LOGGER = LoggerFactory.getLogger(DefaultDnsClient.class); - private static final String NX_DOMAIN_INVALIDATES_PROPERTY = "io.servicetalk.dns.discovery.nxdomain.invalidation"; - private static final boolean NX_DOMAIN_INVALIDATES = parseProperty(NX_DOMAIN_INVALIDATES_PROPERTY, true); private static final Comparator INET_ADDRESS_COMPARATOR = comparing(o -> wrap(o.getAddress())); private static final Comparator HOST_AND_PORT_COMPARATOR = comparing(HostAndPort::hostName) .thenComparingInt(HostAndPort::port); @@ -782,7 +780,7 @@ private void cancel0() { private void cancelAndTerminate0(Throwable cause) { assertInEventloop(); - LOGGER.warn("{} subscription for {} will be cancelled and terminated with an error.", + LOGGER.debug("{} subscription for {} will be cancelled and terminated with an error.", DefaultDnsClient.this, AbstractDnsPublisher.this, cause); try { cancel0(); @@ -974,8 +972,9 @@ private static boolean shouldRevokeState(final Throwable t) { // ISE => Subscriber exceptions (downstream of retry) return t instanceof SrvAddressRemovedException || t instanceof IllegalStateException || t instanceof ClosedDnsServiceDiscovererException || (NX_DOMAIN_INVALIDATES && - t.getCause() instanceof DnsErrorCauseException && - NXDOMAIN.equals(((DnsErrorCauseException) t.getCause()).getCode())); + // string matching is done on purpose to avoid the hard Netty dependency + (t.getCause() != null && t.getCause().getClass().getSimpleName().contains("DnsErrorCauseException")) && + NXDOMAIN.equals(((io.netty.resolver.dns.DnsErrorCauseException) t.getCause()).getCode())); } private static Publisher newDuplicateSrv(String serviceName, String resolvedAddress) { @@ -1110,13 +1109,4 @@ static SrvAddressRemovedException newInstance(Class clazz, String method) { return unknownStackTrace(new SrvAddressRemovedException(), clazz, method); } } - - @Nullable - private static boolean parseProperty(final String name, final boolean defaultValue) { - final String value = getProperty(name); - if (value == null) { - return defaultValue; - } - return Boolean.parseBoolean(value); - } } diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java index dad9919a0c..f9ab6aa25b 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java @@ -62,6 +62,8 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco */ @Deprecated // FIXME: 0.43 - consider removing this system property private static final String SKIP_BINDING_PROPERTY = "io.servicetalk.dns.discovery.netty.skipBinding"; + private static final String NX_DOMAIN_INVALIDATES_PROPERTY = "io.servicetalk.dns.discovery.nxdomain.invalidation"; + static boolean NX_DOMAIN_INVALIDATES = parseProperty(NX_DOMAIN_INVALIDATES_PROPERTY, true); @Nullable private static final SocketAddress DEFAULT_LOCAL_ADDRESS = getBoolean(SKIP_BINDING_PROPERTY) ? null : new InetSocketAddress(0); @@ -101,6 +103,7 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco LOGGER.debug("-D{}={}", NEGATIVE_TTL_CACHE_SECONDS_PROPERTY, negativeCacheTtlValue); LOGGER.debug("Default negative TTL cache in seconds: {}", DEFAULT_NEGATIVE_TTL_CACHE_SECONDS); LOGGER.debug("Default missing records status: {}", DEFAULT_MISSING_RECOREDS_STATUS); + LOGGER.debug("-D{}: {}", NX_DOMAIN_INVALIDATES_PROPERTY, NX_DOMAIN_INVALIDATES); } } @@ -311,10 +314,8 @@ DefaultDnsServiceDiscovererBuilder completeOncePreferredResolved(boolean complet DefaultDnsServiceDiscovererBuilder srvHostNameRepeatDelay( Duration initialDelay, Duration jitter) { - this.srvHostNameRepeatInitialDelay = requireNonNull(initialDelay); - this.srvHostNameRepeatJitter = requireNonNull(jitter); - ensurePositive(srvHostNameRepeatInitialDelay, "srvHostNameRepeatInitialDelay"); - ensurePositive(srvHostNameRepeatJitter, "srvHostNameRepeatJitter"); + this.srvHostNameRepeatInitialDelay = ensurePositive(initialDelay, "srvHostNameRepeatInitialDelay"); + this.srvHostNameRepeatJitter = ensurePositive(jitter, "srvHostNameRepeatJitter"); if (srvHostNameRepeatJitter.toNanos() >= srvHostNameRepeatInitialDelay.toNanos()) { throw new IllegalArgumentException("The jitter value should be less than the initial delay."); } @@ -388,4 +389,12 @@ private static int parseProperty(final String name, final int defaultValue) { return defaultValue; } } + + private static boolean parseProperty(final String name, final boolean defaultValue) { + final String value = getProperty(name); + if (value == null) { + return defaultValue; + } + return Boolean.parseBoolean(value); + } } diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index dcac662ec4..030d430dbc 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -66,6 +66,7 @@ import static io.servicetalk.client.api.ServiceDiscovererEvent.Status.EXPIRED; import static io.servicetalk.concurrent.api.SourceAdapters.toSource; import static io.servicetalk.concurrent.internal.DeliberateException.DELIBERATE_EXCEPTION; +import static io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.NX_DOMAIN_INVALIDATES; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_ONLY; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED_RETURN_ALL; @@ -478,7 +479,7 @@ void srvFailResumesCorrectly() throws Exception { } @ParameterizedTest(name = "{displayName} [{index}] srvFilterDuplicateEvents={0}") - @ValueSource(booleans = {false}) + @ValueSource(booleans = {false, true}) void srvDuplicateAddresses(boolean srvFilterDuplicateEvents) throws Exception { setup(builder -> builder.srvFilterDuplicateEvents(srvFilterDuplicateEvents)); final String domain = "sd.servicetalk.io"; @@ -593,26 +594,36 @@ void unknownHostDiscover() throws Exception { assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } - @Test - void unknownHostAfterKnownDiscover() throws Exception { - setup(); - final String targetDomain1 = "sd.domain.com"; - final String ip1 = nextIp(); + @ParameterizedTest(name = "{displayName} [{index}] cache={0}") + @ValueSource(booleans = {false, true}) + void unknownHostAfterKnownDiscover(boolean nxInvalidation) throws Exception { + boolean restore = NX_DOMAIN_INVALIDATES; + try { + NX_DOMAIN_INVALIDATES = nxInvalidation; + setup(); + final String targetDomain1 = "sd.domain.com"; + final String ip1 = nextIp(); - recordStore.addIPv4Address(targetDomain1, DEFAULT_TTL, ip1); + recordStore.addIPv4Address(targetDomain1, DEFAULT_TTL, ip1); - TestPublisherSubscriber> subscriber = dnsQuery(targetDomain1); - Subscription subscription = subscriber.awaitSubscription(); - subscription.request(Long.MAX_VALUE); + TestPublisherSubscriber> subscriber = dnsQuery(targetDomain1); + Subscription subscription = subscriber.awaitSubscription(); + subscription.request(Long.MAX_VALUE); - List> signals = subscriber.takeOnNext(1); - assertHasEvent(signals, ip1, AVAILABLE); + List> signals = subscriber.takeOnNext(1); + assertHasEvent(signals, ip1, AVAILABLE); - recordStore.removeIPv4Addresses(targetDomain1); - advanceTime(); + recordStore.removeIPv4Addresses(targetDomain1); + advanceTime(); - signals = subscriber.takeOnNext(1); - assertHasEvent(signals, ip1, EXPIRED); + if (NX_DOMAIN_INVALIDATES) { + signals = subscriber.takeOnNext(1); + assertHasEvent(signals, ip1, EXPIRED); + } + assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); + } finally { + NX_DOMAIN_INVALIDATES = restore; + } } @Test diff --git a/servicetalk-dns-discovery-netty/src/test/resources/log4j2.xml b/servicetalk-dns-discovery-netty/src/test/resources/log4j2.xml deleted file mode 100644 index e69de29bb2..0000000000 From b4072760a1501c33e20a3f2175edc9c0f4792fb3 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Tue, 26 Mar 2024 22:24:37 -0700 Subject: [PATCH 10/14] checkstyle/pmd --- .../gradle/checkstyle/suppressions.xml | 24 +++++++++++++++++++ .../gradle/spotbugs/test-exclusions.xml | 4 ++++ .../dns/discovery/netty/DefaultDnsClient.java | 1 - .../DefaultDnsServiceDiscovererBuilder.java | 1 + 4 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml diff --git a/servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml b/servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml new file mode 100644 index 0000000000..0a3237db60 --- /dev/null +++ b/servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml @@ -0,0 +1,24 @@ + + + + + + + diff --git a/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml b/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml index bdabf555ea..23aabdd625 100644 --- a/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml +++ b/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml @@ -43,4 +43,8 @@ + + + + diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java index ec769cd28f..6e92f6b01d 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java @@ -107,7 +107,6 @@ import static io.servicetalk.transport.netty.internal.EventLoopAwareNettyIoExecutors.toEventLoopAwareNettyIoExecutor; import static io.servicetalk.utils.internal.ThrowableUtils.addSuppressed; import static java.lang.Integer.toHexString; -import static java.lang.System.getProperty; import static java.lang.System.identityHashCode; import static java.nio.ByteBuffer.wrap; import static java.util.Collections.emptyList; diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java index f9ab6aa25b..872ca2522c 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java @@ -63,6 +63,7 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco @Deprecated // FIXME: 0.43 - consider removing this system property private static final String SKIP_BINDING_PROPERTY = "io.servicetalk.dns.discovery.netty.skipBinding"; private static final String NX_DOMAIN_INVALIDATES_PROPERTY = "io.servicetalk.dns.discovery.nxdomain.invalidation"; + @SuppressWarnings("PMD.MutableStaticState") static boolean NX_DOMAIN_INVALIDATES = parseProperty(NX_DOMAIN_INVALIDATES_PROPERTY, true); @Nullable private static final SocketAddress DEFAULT_LOCAL_ADDRESS = From 0b105b9c0707ca02dc6ff03d79e5320112c79472 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Wed, 10 Apr 2024 18:10:17 -0700 Subject: [PATCH 11/14] opt-in nxvalidation further discussion pending --- .../gradle/checkstyle/suppressions.xml | 24 ---- .../dns/discovery/netty/DefaultDnsClient.java | 25 ++-- .../DefaultDnsServiceDiscovererBuilder.java | 20 +++- .../discovery/netty/DefaultDnsClientTest.java | 110 +++++++++++------- 4 files changed, 97 insertions(+), 82 deletions(-) delete mode 100644 servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml diff --git a/servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml b/servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml deleted file mode 100644 index 0a3237db60..0000000000 --- a/servicetalk-dns-discovery-netty/gradle/checkstyle/suppressions.xml +++ /dev/null @@ -1,24 +0,0 @@ - - - - - - - diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java index 6e92f6b01d..3b9ef40895 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsClient.java @@ -96,7 +96,6 @@ import static io.servicetalk.concurrent.internal.SubscriberUtils.newExceptionForInvalidRequestN; import static io.servicetalk.concurrent.internal.SubscriberUtils.safeOnError; import static io.servicetalk.concurrent.internal.ThrowableUtils.unknownStackTrace; -import static io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.NX_DOMAIN_INVALIDATES; import static io.servicetalk.dns.discovery.netty.DnsClients.mapEventList; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV6_PREFERRED; @@ -134,6 +133,7 @@ final class DefaultDnsClient implements DnsClient { @Nullable private final DnsServiceDiscovererObserver observer; private final ServiceDiscovererEvent.Status missingRecordStatus; + private final boolean nxInvalidation; private final IntFunction srvHostNameRepeater; private final int srvConcurrency; private final boolean srvFilterDuplicateEvents; @@ -153,7 +153,8 @@ final class DefaultDnsClient implements DnsClient { @Nullable final SocketAddress localAddress, @Nullable final DnsServerAddressStreamProvider dnsServerAddressStreamProvider, @Nullable final DnsServiceDiscovererObserver observer, - final ServiceDiscovererEvent.Status missingRecordStatus) { + final ServiceDiscovererEvent.Status missingRecordStatus, + final boolean nxInvalidation) { this.srvConcurrency = srvConcurrency; this.srvFilterDuplicateEvents = srvFilterDuplicateEvents; // Implementation of this class expects to use only single EventLoop from IoExecutor @@ -170,6 +171,7 @@ final class DefaultDnsClient implements DnsClient { this.addressTypes = dnsResolverAddressTypes; this.observer = observer; this.missingRecordStatus = missingRecordStatus; + this.nxInvalidation = nxInvalidation; this.id = id + '@' + toHexString(identityHashCode(this)); asyncCloseable = toAsyncCloseable(graceful -> { if (nettyIoExecutor.isCurrentThreadEventLoop()) { @@ -253,7 +255,7 @@ public Publisher>> dnsQuery(final final DnsDiscoveryObserver discoveryObserver = newDiscoveryObserver(address); ARecordPublisher pub = new ARecordPublisher(address, discoveryObserver); Publisher>> events = - recoverWithInactiveEvents(pub, false); + recoverWithInactiveEvents(pub, false, nxInvalidation); return discoveryObserver == null ? events : events.beforeFinally(new TerminalSignalConsumer() { @Override public void onComplete() { @@ -297,7 +299,7 @@ public Publisher>> dnsSrvQu // any pending scheduled tasks. SrvInactiveCombinerOperator is used to filter the aggregated collection of // inactive events if necessary. Publisher>> events = - recoverWithInactiveEvents(new SrvRecordPublisher(serviceName, discoveryObserver), true) + recoverWithInactiveEvents(new SrvRecordPublisher(serviceName, discoveryObserver), true, nxInvalidation) .flatMapConcatIterable(identity()) .flatMapMerge(srvEvent -> { assertInEventloop(); @@ -311,8 +313,10 @@ public Publisher>> dnsSrvQu return newDuplicateSrv(serviceName, srvEvent.address().hostName()); } + // NXDOMAIN = invalidation for A queries part of SRV lookups for backwards compatibility. + // This is a behavior difference between plain A lookups and SRV rooted A lookups. Publisher>> returnPub = - recoverWithInactiveEvents(aPublisher, false); + recoverWithInactiveEvents(aPublisher, false, true); return srvFilterDuplicateEvents ? srvFilterDups(returnPub, availableAddresses, srvEvent.address().port()) : returnPub.map(ev -> mapEventList(ev, inetAddress -> @@ -951,10 +955,10 @@ private static Publisher Publisher>> recoverWithInactiveEvents( - AbstractDnsPublisher pub, boolean generateAggregateEvent) { + AbstractDnsPublisher pub, boolean generateAggregateEvent, boolean nxInvalidation) { return pub.onErrorResume(cause -> { AbstractDnsPublisher.AbstractDnsSubscription subscription = pub.subscription; - if (subscription != null && shouldRevokeState(cause)) { + if (subscription != null && shouldRevokeState(cause, nxInvalidation)) { List> events = subscription.generateInactiveEvent(); if (!events.isEmpty()) { return (generateAggregateEvent ? Publisher.>>from( @@ -967,12 +971,13 @@ private static Publisher>> }); } - private static boolean shouldRevokeState(final Throwable t) { + private static boolean shouldRevokeState(final Throwable t, final boolean nxInvalidation) { // ISE => Subscriber exceptions (downstream of retry) return t instanceof SrvAddressRemovedException || t instanceof IllegalStateException || - t instanceof ClosedDnsServiceDiscovererException || (NX_DOMAIN_INVALIDATES && + t instanceof ClosedDnsServiceDiscovererException || (nxInvalidation && // string matching is done on purpose to avoid the hard Netty dependency - (t.getCause() != null && t.getCause().getClass().getSimpleName().contains("DnsErrorCauseException")) && + (t.getCause() != null && t.getCause().getClass().getName() + .equals("io.netty.resolver.dns.DnsErrorCauseException")) && NXDOMAIN.equals(((io.netty.resolver.dns.DnsErrorCauseException) t.getCause()).getCode())); } diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java index 872ca2522c..a23cbec5e8 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java @@ -63,8 +63,7 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco @Deprecated // FIXME: 0.43 - consider removing this system property private static final String SKIP_BINDING_PROPERTY = "io.servicetalk.dns.discovery.netty.skipBinding"; private static final String NX_DOMAIN_INVALIDATES_PROPERTY = "io.servicetalk.dns.discovery.nxdomain.invalidation"; - @SuppressWarnings("PMD.MutableStaticState") - static boolean NX_DOMAIN_INVALIDATES = parseProperty(NX_DOMAIN_INVALIDATES_PROPERTY, true); + static final boolean DEFAULT_NX_DOMAIN_INVALIDATES = parseProperty(NX_DOMAIN_INVALIDATES_PROPERTY, false); @Nullable private static final SocketAddress DEFAULT_LOCAL_ADDRESS = getBoolean(SKIP_BINDING_PROPERTY) ? null : new InetSocketAddress(0); @@ -104,7 +103,7 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco LOGGER.debug("-D{}={}", NEGATIVE_TTL_CACHE_SECONDS_PROPERTY, negativeCacheTtlValue); LOGGER.debug("Default negative TTL cache in seconds: {}", DEFAULT_NEGATIVE_TTL_CACHE_SECONDS); LOGGER.debug("Default missing records status: {}", DEFAULT_MISSING_RECOREDS_STATUS); - LOGGER.debug("-D{}: {}", NX_DOMAIN_INVALIDATES_PROPERTY, NX_DOMAIN_INVALIDATES); + LOGGER.debug("-D{}: {}", NX_DOMAIN_INVALIDATES_PROPERTY, DEFAULT_NX_DOMAIN_INVALIDATES); } } @@ -141,6 +140,7 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco @Nullable private DnsServiceDiscovererObserver observer; private ServiceDiscovererEvent.Status missingRecordStatus = DEFAULT_MISSING_RECOREDS_STATUS; + private boolean nxInvalidation = DEFAULT_NX_DOMAIN_INVALIDATES; /** * Creates a new {@link DefaultDnsServiceDiscovererBuilder}. @@ -291,6 +291,18 @@ public DefaultDnsServiceDiscovererBuilder missingRecordStatus(ServiceDiscovererE return this; } + /** + * Modify the behavior of the system flag about invalidating DNS state when NXDOMAIN is seen. + * Default behavior is controlled through {@link #NX_DOMAIN_INVALIDATES_PROPERTY}. + * + * @param nxInvalidation Flag to enable/disable behavior. + * @return {@code this} builder. + */ + DefaultDnsServiceDiscovererBuilder nxInvalidates(final boolean nxInvalidation) { + this.nxInvalidation = nxInvalidation; + return this; + } + @Override public ServiceDiscoverer> buildSrvDiscoverer() { @@ -373,7 +385,7 @@ DnsClient build() { srvConcurrency, completeOncePreferredResolved, srvFilterDuplicateEvents, srvHostNameRepeatInitialDelay, srvHostNameRepeatJitter, maxUdpPayloadSize, ndots, optResourceEnabled, queryTimeout, dnsResolverAddressTypes, localAddress, dnsServerAddressStreamProvider, observer, - missingRecordStatus); + missingRecordStatus, nxInvalidation); return filterFactory == null ? rawClient : filterFactory.create(rawClient); } diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index 030d430dbc..dfc0622631 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -66,7 +66,7 @@ import static io.servicetalk.client.api.ServiceDiscovererEvent.Status.EXPIRED; import static io.servicetalk.concurrent.api.SourceAdapters.toSource; import static io.servicetalk.concurrent.internal.DeliberateException.DELIBERATE_EXCEPTION; -import static io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.NX_DOMAIN_INVALIDATES; +import static io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.DEFAULT_NX_DOMAIN_INVALIDATES; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_ONLY; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED_RETURN_ALL; @@ -354,16 +354,21 @@ void srvCNAMEDuplicateAddresses() throws Exception { createSrvRecord(domain, targetDomain2, targetPort, ttl)); advanceTime(); - signals = subscriber.takeOnNext(2); - assertHasEvent(signals, ip1, targetPort, EXPIRED); - assertHasEvent(signals, ip2, targetPort, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + signals = subscriber.takeOnNext(2); + assertHasEvent(signals, ip1, targetPort, EXPIRED); + assertHasEvent(signals, ip2, targetPort, EXPIRED); + } else { + assertThat(subscriber.pollOnNext(50, MILLISECONDS), is(nullValue())); + } + assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @ParameterizedTest(name = "{displayName} [{index}] missingRecordStatus={0}") @MethodSource("missingRecordStatus") void srvInactiveEventsAggregated(ServiceDiscovererEvent.Status missingRecordStatus) throws Exception { - setup(builder -> builder.missingRecordStatus(missingRecordStatus)); + setup(builder -> builder.nxInvalidates(true).missingRecordStatus(missingRecordStatus)); final String domain = "sd.servicetalk.io"; final String targetDomain1 = "target1.mysvc.servicetalk.io"; final String targetDomain2 = "target2.mysvc.servicetalk.io"; @@ -439,9 +444,13 @@ void srvRecordRemovalPropagatesError() throws Exception { createSrvRecord(domain, targetDomain1, targetPort, DEFAULT_TTL), createSrvRecord(domain, targetDomain2, targetPort, DEFAULT_TTL)); advanceTime(); - signals = subscriber.takeOnNext(2); - assertHasEvent(signals, ip1, targetPort, EXPIRED); - assertHasEvent(signals, ip2, targetPort, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + signals = subscriber.takeOnNext(2); + assertHasEvent(signals, ip1, targetPort, EXPIRED); + assertHasEvent(signals, ip2, targetPort, EXPIRED); + } else { + assertThat(subscriber.pollOnNext(50, MILLISECONDS), is(nullValue())); + } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -479,7 +488,7 @@ void srvFailResumesCorrectly() throws Exception { } @ParameterizedTest(name = "{displayName} [{index}] srvFilterDuplicateEvents={0}") - @ValueSource(booleans = {false, true}) + @ValueSource(booleans = {true}) void srvDuplicateAddresses(boolean srvFilterDuplicateEvents) throws Exception { setup(builder -> builder.srvFilterDuplicateEvents(srvFilterDuplicateEvents)); final String domain = "sd.servicetalk.io"; @@ -520,6 +529,7 @@ void srvDuplicateAddresses(boolean srvFilterDuplicateEvents) throws Exception { @Test void srvAAAAFailsGeneratesInactive() throws Exception { setup(builder -> builder + .nxInvalidates(true) .srvHostNameRepeatDelay(ofMillis(200), ofMillis(10)) .dnsResolverAddressTypes(IPV4_PREFERRED)); final String domain = "sd.servicetalk.io"; @@ -580,7 +590,9 @@ void srvRecordFailsGeneratesInactive() throws Exception { recordStore.removeSrv(domain, targetDomain2, targetPort, DEFAULT_TTL); advanceTime(); - assertEvent(subscriber.takeOnNext(), ip2, targetPort, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + assertEvent(subscriber.takeOnNext(), ip2, targetPort, EXPIRED); + } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -597,33 +609,30 @@ void unknownHostDiscover() throws Exception { @ParameterizedTest(name = "{displayName} [{index}] cache={0}") @ValueSource(booleans = {false, true}) void unknownHostAfterKnownDiscover(boolean nxInvalidation) throws Exception { - boolean restore = NX_DOMAIN_INVALIDATES; - try { - NX_DOMAIN_INVALIDATES = nxInvalidation; - setup(); - final String targetDomain1 = "sd.domain.com"; - final String ip1 = nextIp(); + setup(defaultDnsServiceDiscovererBuilder -> { + defaultDnsServiceDiscovererBuilder.nxInvalidates(nxInvalidation); + return defaultDnsServiceDiscovererBuilder; + }); + final String targetDomain1 = "sd.domain.com"; + final String ip1 = nextIp(); - recordStore.addIPv4Address(targetDomain1, DEFAULT_TTL, ip1); + recordStore.addIPv4Address(targetDomain1, DEFAULT_TTL, ip1); - TestPublisherSubscriber> subscriber = dnsQuery(targetDomain1); - Subscription subscription = subscriber.awaitSubscription(); - subscription.request(Long.MAX_VALUE); + TestPublisherSubscriber> subscriber = dnsQuery(targetDomain1); + Subscription subscription = subscriber.awaitSubscription(); + subscription.request(Long.MAX_VALUE); - List> signals = subscriber.takeOnNext(1); - assertHasEvent(signals, ip1, AVAILABLE); + List> signals = subscriber.takeOnNext(1); + assertHasEvent(signals, ip1, AVAILABLE); - recordStore.removeIPv4Addresses(targetDomain1); - advanceTime(); + recordStore.removeIPv4Addresses(targetDomain1); + advanceTime(); - if (NX_DOMAIN_INVALIDATES) { - signals = subscriber.takeOnNext(1); - assertHasEvent(signals, ip1, EXPIRED); - } - assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); - } finally { - NX_DOMAIN_INVALIDATES = restore; + if (nxInvalidation) { + signals = subscriber.takeOnNext(1); + assertHasEvent(signals, ip1, EXPIRED); } + assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @Test @@ -645,9 +654,11 @@ void singleDiscoverMultipleRecords() throws Exception { recordStore.removeIPv4Address(domain, DEFAULT_TTL, ips); subscription.request(ips.length); advanceTime(); - signals = subscriber.takeOnNext(ips.length); - for (String ip : ips) { - assertHasEvent(signals, ip, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + signals = subscriber.takeOnNext(ips.length); + for (String ip : ips) { + assertHasEvent(signals, ip, EXPIRED); + } } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -707,12 +718,14 @@ void repeatDiscoverMultipleRecords() throws Exception { recordStore.removeIPv4Address(domain, DEFAULT_TTL, ips2); subscription.request(ips.length + ips2.length); advanceTime(); - signals = subscriber.takeOnNext(ips.length + ips2.length); - for (String ip : ips) { - assertHasEvent(signals, ip, EXPIRED); - } - for (String ip : ips2) { - assertHasEvent(signals, ip, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + signals = subscriber.takeOnNext(ips.length + ips2.length); + for (String ip : ips) { + assertHasEvent(signals, ip, EXPIRED); + } + for (String ip : ips2) { + assertHasEvent(signals, ip, EXPIRED); + } } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -744,9 +757,13 @@ void repeatDiscoverMultipleHosts() throws Exception { subscription1.request(1); subscription2.request(1); advanceTime(); - assertEvent(subscriber1.takeOnNext(), ip1, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + assertEvent(subscriber1.takeOnNext(), ip1, EXPIRED); + } assertThat(subscriber1.awaitOnError(), instanceOf(UnknownHostException.class)); - assertEvent(subscriber2.takeOnNext(), ip2, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + assertEvent(subscriber2.takeOnNext(), ip2, EXPIRED); + } assertThat(subscriber2.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -754,6 +771,7 @@ void repeatDiscoverMultipleHosts() throws Exception { @MethodSource("missingRecordStatus") void repeatDiscoverNxDomainAndRecover(ServiceDiscovererEvent.Status missingRecordStatus) throws Exception { setup(builder -> builder + .nxInvalidates(true) .missingRecordStatus(missingRecordStatus)); final String ip = nextIp(); final String domain = "servicetalk.io"; @@ -979,7 +997,9 @@ void preferIpv4ButOnlyAAAARecordIsPresent(DnsResolverAddressTypes addressTypes) // Remove all ips recordStore.removeIPv6Address(domain, DEFAULT_TTL, ipv6); advanceTime(); - assertEvent(subscriber.takeOnNext(), ipv6, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + assertEvent(subscriber.takeOnNext(), ipv6, EXPIRED); + } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } @@ -1000,7 +1020,9 @@ void preferIpv6ButOnlyARecordIsPresent(DnsResolverAddressTypes addressTypes) thr // Remove all ips recordStore.removeIPv4Address(domain, DEFAULT_TTL, ipv4); advanceTime(); - assertEvent(subscriber.takeOnNext(), ipv4, EXPIRED); + if (DEFAULT_NX_DOMAIN_INVALIDATES) { + assertEvent(subscriber.takeOnNext(), ipv4, EXPIRED); + } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } From c17796cfc7704a17db3bfb38981ec1b63c47d0a4 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Wed, 10 Apr 2024 18:10:57 -0700 Subject: [PATCH 12/14] pmd --- .../gradle/spotbugs/test-exclusions.xml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml b/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml index 23aabdd625..bdabf555ea 100644 --- a/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml +++ b/servicetalk-dns-discovery-netty/gradle/spotbugs/test-exclusions.xml @@ -43,8 +43,4 @@ - - - - From 26eacd392090a444ae81b3483433c95b9b7ba4a0 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Thu, 11 Apr 2024 11:17:55 -0700 Subject: [PATCH 13/14] test both conditions --- .../DefaultDnsServiceDiscovererBuilder.java | 13 +-- .../discovery/netty/DefaultDnsClientTest.java | 80 +++++++++++-------- 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java index a23cbec5e8..365dff57cc 100644 --- a/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java +++ b/servicetalk-dns-discovery-netty/src/main/java/io/servicetalk/dns/discovery/netty/DefaultDnsServiceDiscovererBuilder.java @@ -19,6 +19,7 @@ import io.servicetalk.client.api.ServiceDiscovererEvent; import io.servicetalk.transport.api.HostAndPort; import io.servicetalk.transport.api.IoExecutor; +import io.servicetalk.utils.internal.DurationUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -63,7 +64,7 @@ public final class DefaultDnsServiceDiscovererBuilder implements DnsServiceDisco @Deprecated // FIXME: 0.43 - consider removing this system property private static final String SKIP_BINDING_PROPERTY = "io.servicetalk.dns.discovery.netty.skipBinding"; private static final String NX_DOMAIN_INVALIDATES_PROPERTY = "io.servicetalk.dns.discovery.nxdomain.invalidation"; - static final boolean DEFAULT_NX_DOMAIN_INVALIDATES = parseProperty(NX_DOMAIN_INVALIDATES_PROPERTY, false); + private static final boolean DEFAULT_NX_DOMAIN_INVALIDATES = getBoolean(NX_DOMAIN_INVALIDATES_PROPERTY); @Nullable private static final SocketAddress DEFAULT_LOCAL_ADDRESS = getBoolean(SKIP_BINDING_PROPERTY) ? null : new InetSocketAddress(0); @@ -328,7 +329,7 @@ DefaultDnsServiceDiscovererBuilder completeOncePreferredResolved(boolean complet DefaultDnsServiceDiscovererBuilder srvHostNameRepeatDelay( Duration initialDelay, Duration jitter) { this.srvHostNameRepeatInitialDelay = ensurePositive(initialDelay, "srvHostNameRepeatInitialDelay"); - this.srvHostNameRepeatJitter = ensurePositive(jitter, "srvHostNameRepeatJitter"); + this.srvHostNameRepeatJitter = DurationUtils.ensureNonNegative(jitter, "srvHostNameRepeatJitter"); if (srvHostNameRepeatJitter.toNanos() >= srvHostNameRepeatInitialDelay.toNanos()) { throw new IllegalArgumentException("The jitter value should be less than the initial delay."); } @@ -402,12 +403,4 @@ private static int parseProperty(final String name, final int defaultValue) { return defaultValue; } } - - private static boolean parseProperty(final String name, final boolean defaultValue) { - final String value = getProperty(name); - if (value == null) { - return defaultValue; - } - return Boolean.parseBoolean(value); - } } diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index dfc0622631..78f1c15065 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -36,6 +36,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; @@ -66,7 +67,6 @@ import static io.servicetalk.client.api.ServiceDiscovererEvent.Status.EXPIRED; import static io.servicetalk.concurrent.api.SourceAdapters.toSource; import static io.servicetalk.concurrent.internal.DeliberateException.DELIBERATE_EXCEPTION; -import static io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.DEFAULT_NX_DOMAIN_INVALIDATES; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_ONLY; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED; import static io.servicetalk.dns.discovery.netty.DnsResolverAddressTypes.IPV4_PREFERRED_RETURN_ALL; @@ -318,9 +318,11 @@ void srvWithCNAMEEntryLowerTTLDoesNotFail() throws Exception { assertNull(subscriber.pollTerminal(50, MILLISECONDS)); } - @Test - void srvCNAMEDuplicateAddresses() throws Exception { + @ParameterizedTest(name = "{displayName} [{index}] nxInvalidation={0}") + @ValueSource(booleans = {true, false}) + void srvCNAMEDuplicateAddresses(boolean nxInvalidation) throws Exception { setup(builder -> builder + .nxInvalidates(nxInvalidation) .dnsServerAddressStreamProvider(new SequentialDnsServerAddressStreamProvider( dnsServer2.localAddress(), dnsServer.localAddress()))); final String domain = "sd.servicetalk.io"; @@ -354,7 +356,7 @@ void srvCNAMEDuplicateAddresses() throws Exception { createSrvRecord(domain, targetDomain2, targetPort, ttl)); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { signals = subscriber.takeOnNext(2); assertHasEvent(signals, ip1, targetPort, EXPIRED); assertHasEvent(signals, ip2, targetPort, EXPIRED); @@ -417,9 +419,10 @@ void srvInactiveEventsAggregated(ServiceDiscovererEvent.Status missingRecordStat assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } - @Test - void srvRecordRemovalPropagatesError() throws Exception { - setup(); + @ParameterizedTest(name = "{displayName} [{index}] nxInvalidation={0}") + @ValueSource(booleans = {true, false}) + void srvRecordRemovalPropagatesError(boolean nxInvalidation) throws Exception { + setup(builder -> builder.nxInvalidates(nxInvalidation)); final String domain = "sd.servicetalk.io"; final String targetDomain1 = "target1.mysvc.servicetalk.io"; final String targetDomain2 = "target2.mysvc.servicetalk.io"; @@ -444,7 +447,7 @@ void srvRecordRemovalPropagatesError() throws Exception { createSrvRecord(domain, targetDomain1, targetPort, DEFAULT_TTL), createSrvRecord(domain, targetDomain2, targetPort, DEFAULT_TTL)); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { signals = subscriber.takeOnNext(2); assertHasEvent(signals, ip1, targetPort, EXPIRED); assertHasEvent(signals, ip2, targetPort, EXPIRED); @@ -488,7 +491,7 @@ void srvFailResumesCorrectly() throws Exception { } @ParameterizedTest(name = "{displayName} [{index}] srvFilterDuplicateEvents={0}") - @ValueSource(booleans = {true}) + @ValueSource(booleans = {true, false}) void srvDuplicateAddresses(boolean srvFilterDuplicateEvents) throws Exception { setup(builder -> builder.srvFilterDuplicateEvents(srvFilterDuplicateEvents)); final String domain = "sd.servicetalk.io"; @@ -561,9 +564,10 @@ void srvAAAAFailsGeneratesInactive() throws Exception { assertEvent(subscriber.takeOnNext(), ip1, targetPort, AVAILABLE); } - @Test - void srvRecordFailsGeneratesInactive() throws Exception { - setup(); + @ParameterizedTest(name = "{displayName} [{index}] nxInvalidation={0}") + @ValueSource(booleans = {true, false}) + void srvRecordFailsGeneratesInactive(boolean nxInvalidation) throws Exception { + setup(builder -> builder.nxInvalidates(nxInvalidation)); final String domain = "sd.servicetalk.io"; final String targetDomain1 = "target1.mysvc.servicetalk.io"; final String targetDomain2 = "target2.mysvc.servicetalk.io"; @@ -590,7 +594,7 @@ void srvRecordFailsGeneratesInactive() throws Exception { recordStore.removeSrv(domain, targetDomain2, targetPort, DEFAULT_TTL); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { assertEvent(subscriber.takeOnNext(), ip2, targetPort, EXPIRED); } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); @@ -635,9 +639,10 @@ void unknownHostAfterKnownDiscover(boolean nxInvalidation) throws Exception { assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } - @Test - void singleDiscoverMultipleRecords() throws Exception { - setup(); + @ParameterizedTest(name = "{displayName} [{index}] nxInvalidation={0}") + @ValueSource(booleans = {true, false}) + void singleDiscoverMultipleRecords(boolean nxInvalidation) throws Exception { + setup(builder -> builder.nxInvalidates(nxInvalidation)); final String domain = "servicetalk.io"; final String[] ips = {nextIp(), nextIp(), nextIp(), nextIp(), nextIp()}; recordStore.addIPv4Address(domain, DEFAULT_TTL, ips); @@ -654,7 +659,7 @@ void singleDiscoverMultipleRecords() throws Exception { recordStore.removeIPv4Address(domain, DEFAULT_TTL, ips); subscription.request(ips.length); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { signals = subscriber.takeOnNext(ips.length); for (String ip : ips) { assertHasEvent(signals, ip, EXPIRED); @@ -689,9 +694,10 @@ void singleDiscoverDuplicateRecords() throws Exception { } } - @Test - void repeatDiscoverMultipleRecords() throws Exception { - setup(); + @ParameterizedTest(name = "{displayName} [{index}] nxInvalidation={0}") + @ValueSource(booleans = {true, false}) + void repeatDiscoverMultipleRecords(boolean nxInvalidation) throws Exception { + setup(builder -> builder.nxInvalidates(nxInvalidation)); final String domain = "servicetalk.io"; final String[] ips = {nextIp(), nextIp(), nextIp(), nextIp(), nextIp()}; recordStore.addIPv4Address(domain, DEFAULT_TTL, ips); @@ -718,7 +724,7 @@ void repeatDiscoverMultipleRecords() throws Exception { recordStore.removeIPv4Address(domain, DEFAULT_TTL, ips2); subscription.request(ips.length + ips2.length); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { signals = subscriber.takeOnNext(ips.length + ips2.length); for (String ip : ips) { assertHasEvent(signals, ip, EXPIRED); @@ -730,9 +736,10 @@ void repeatDiscoverMultipleRecords() throws Exception { assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } - @Test - void repeatDiscoverMultipleHosts() throws Exception { - setup(); + @ParameterizedTest(name = "{displayName} [{index}] nxInvalidation={0}") + @ValueSource(booleans = {true, false}) + void repeatDiscoverMultipleHosts(boolean nxInvalidation) throws Exception { + setup(builder -> builder.nxInvalidates(nxInvalidation)); final String ip1 = nextIp(); final String domain1 = "servicetalk.io"; final String ip2 = nextIp(); @@ -757,11 +764,11 @@ void repeatDiscoverMultipleHosts() throws Exception { subscription1.request(1); subscription2.request(1); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { assertEvent(subscriber1.takeOnNext(), ip1, EXPIRED); } assertThat(subscriber1.awaitOnError(), instanceOf(UnknownHostException.class)); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { assertEvent(subscriber2.takeOnNext(), ip2, EXPIRED); } assertThat(subscriber2.awaitOnError(), instanceOf(UnknownHostException.class)); @@ -981,9 +988,10 @@ void returnAll(DnsResolverAddressTypes addressTypes) throws Exception { } @ParameterizedTest(name = "{displayName} [{index}] dnsResolverAddressTypes={0}") - @EnumSource(value = DnsResolverAddressTypes.class, names = {"IPV4_PREFERRED", "IPV4_PREFERRED_RETURN_ALL"}) - void preferIpv4ButOnlyAAAARecordIsPresent(DnsResolverAddressTypes addressTypes) throws Exception { - setup(builder -> builder.dnsResolverAddressTypes(addressTypes)); + @CsvSource({"IPV4_PREFERRED, true", "IPV4_PREFERRED, false", + "IPV4_PREFERRED_RETURN_ALL, true", "IPV4_PREFERRED_RETURN_ALL, false"}) + void preferIpv4ButOnlyAAAARecordIsPresent(DnsResolverAddressTypes addressTypes, boolean nxInvalidation) throws Exception { + setup(builder -> builder.dnsResolverAddressTypes(addressTypes).nxInvalidates(nxInvalidation)); final String ipv6 = nextIp6(); final String domain = "servicetalk.io"; recordStore.addIPv6Address(domain, DEFAULT_TTL, ipv6); @@ -997,16 +1005,18 @@ void preferIpv4ButOnlyAAAARecordIsPresent(DnsResolverAddressTypes addressTypes) // Remove all ips recordStore.removeIPv6Address(domain, DEFAULT_TTL, ipv6); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { assertEvent(subscriber.takeOnNext(), ipv6, EXPIRED); } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); } - @ParameterizedTest(name = "{displayName} [{index}] dnsResolverAddressTypes={0}") - @EnumSource(value = DnsResolverAddressTypes.class, names = {"IPV6_PREFERRED", "IPV6_PREFERRED_RETURN_ALL"}) - void preferIpv6ButOnlyARecordIsPresent(DnsResolverAddressTypes addressTypes) throws Exception { - setup(builder -> builder.dnsResolverAddressTypes(addressTypes)); + @ParameterizedTest(name = "{displayName} [{index}] dnsResolverAddressTypes={0} invalidation={1}") + @CsvSource({"IPV6_PREFERRED, true", "IPV6_PREFERRED, false", + "IPV6_PREFERRED_RETURN_ALL, true", "IPV6_PREFERRED_RETURN_ALL, false"}) + void preferIpv6ButOnlyARecordIsPresent(DnsResolverAddressTypes addressTypes, boolean nxInvalidation) + throws Exception { + setup(builder -> builder.dnsResolverAddressTypes(addressTypes).nxInvalidates(nxInvalidation)); final String ipv4 = nextIp(); final String domain = "servicetalk.io"; recordStore.addIPv4Address(domain, DEFAULT_TTL, ipv4); @@ -1020,7 +1030,7 @@ void preferIpv6ButOnlyARecordIsPresent(DnsResolverAddressTypes addressTypes) thr // Remove all ips recordStore.removeIPv4Address(domain, DEFAULT_TTL, ipv4); advanceTime(); - if (DEFAULT_NX_DOMAIN_INVALIDATES) { + if (nxInvalidation) { assertEvent(subscriber.takeOnNext(), ipv4, EXPIRED); } assertThat(subscriber.awaitOnError(), instanceOf(UnknownHostException.class)); From 90c4fefe40677a94640f01074ff51237486aa701 Mon Sep 17 00:00:00 2001 From: Thomas Kountis Date: Thu, 11 Apr 2024 11:20:25 -0700 Subject: [PATCH 14/14] checkstyle --- .../servicetalk/dns/discovery/netty/DefaultDnsClientTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java index 78f1c15065..8b50d95d28 100644 --- a/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java +++ b/servicetalk-dns-discovery-netty/src/test/java/io/servicetalk/dns/discovery/netty/DefaultDnsClientTest.java @@ -990,7 +990,8 @@ void returnAll(DnsResolverAddressTypes addressTypes) throws Exception { @ParameterizedTest(name = "{displayName} [{index}] dnsResolverAddressTypes={0}") @CsvSource({"IPV4_PREFERRED, true", "IPV4_PREFERRED, false", "IPV4_PREFERRED_RETURN_ALL, true", "IPV4_PREFERRED_RETURN_ALL, false"}) - void preferIpv4ButOnlyAAAARecordIsPresent(DnsResolverAddressTypes addressTypes, boolean nxInvalidation) throws Exception { + void preferIpv4ButOnlyAAAARecordIsPresent(DnsResolverAddressTypes addressTypes, boolean nxInvalidation) + throws Exception { setup(builder -> builder.dnsResolverAddressTypes(addressTypes).nxInvalidates(nxInvalidation)); final String ipv6 = nextIp6(); final String domain = "servicetalk.io";