Implement proper discovery with subtypes am: b366d24bd5
Original change: https://googleplex-android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/23209401
Change-Id: Ice72d478cb68c787c59de78942abfabf04978fe2
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/service-t/src/com/android/server/NsdService.java b/service-t/src/com/android/server/NsdService.java
index c136d4c..9a2cc5f 100644
--- a/service-t/src/com/android/server/NsdService.java
+++ b/service-t/src/com/android/server/NsdService.java
@@ -55,6 +55,7 @@
import android.os.UserHandle;
import android.text.TextUtils;
import android.util.Log;
+import android.util.Pair;
import android.util.SparseArray;
import com.android.internal.annotations.VisibleForTesting;
@@ -599,7 +600,10 @@
final NsdServiceInfo info = args.serviceInfo;
id = getUniqueId();
- final String serviceType = constructServiceType(info.getServiceType());
+ final Pair<String, String> typeAndSubtype =
+ parseTypeAndSubtype(info.getServiceType());
+ final String serviceType = typeAndSubtype == null
+ ? null : typeAndSubtype.first;
if (clientInfo.mUseJavaBackend
|| mDeps.isMdnsDiscoveryManagerEnabled(mContext)
|| useDiscoveryManagerForType(serviceType)) {
@@ -613,12 +617,17 @@
maybeStartMonitoringSockets();
final MdnsListener listener =
new DiscoveryListener(clientId, id, info, listenServiceType);
- final MdnsSearchOptions options = MdnsSearchOptions.newBuilder()
- .setNetwork(info.getNetwork())
- .setIsPassiveMode(true)
- .build();
+ final MdnsSearchOptions.Builder optionsBuilder =
+ MdnsSearchOptions.newBuilder()
+ .setNetwork(info.getNetwork())
+ .setIsPassiveMode(true);
+ if (typeAndSubtype.second != null) {
+ // The parsing ensures subtype starts with an underscore.
+ // MdnsSearchOptions expects the underscore to not be present.
+ optionsBuilder.addSubtype(typeAndSubtype.second.substring(1));
+ }
mMdnsDiscoveryManager.registerListener(
- listenServiceType, listener, options);
+ listenServiceType, listener, optionsBuilder.build());
storeDiscoveryManagerRequestMap(clientId, id, listener, clientInfo);
clientInfo.onDiscoverServicesStarted(clientId, info);
clientInfo.log("Register a DiscoveryListener " + id
@@ -697,7 +706,9 @@
id = getUniqueId();
final NsdServiceInfo serviceInfo = args.serviceInfo;
final String serviceType = serviceInfo.getServiceType();
- final String registerServiceType = constructServiceType(serviceType);
+ final Pair<String, String> typeSubtype = parseTypeAndSubtype(serviceType);
+ final String registerServiceType = typeSubtype == null
+ ? null : typeSubtype.first;
if (clientInfo.mUseJavaBackend
|| mDeps.isMdnsAdvertiserEnabled(mContext)
|| useAdvertiserForType(registerServiceType)) {
@@ -712,6 +723,10 @@
serviceInfo.getServiceName()));
maybeStartMonitoringSockets();
+ // TODO: pass in the subtype as well. Including the subtype in the
+ // service type would generate service instance names like
+ // Name._subtype._sub._type._tcp, which is incorrect
+ // (it should be Name._type._tcp).
mAdvertiser.addService(id, serviceInfo);
storeAdvertiserRequestMap(clientId, id, clientInfo);
} else {
@@ -778,7 +793,10 @@
final NsdServiceInfo info = args.serviceInfo;
id = getUniqueId();
- final String serviceType = constructServiceType(info.getServiceType());
+ final Pair<String, String> typeSubtype =
+ parseTypeAndSubtype(info.getServiceType());
+ final String serviceType = typeSubtype == null
+ ? null : typeSubtype.first;
if (clientInfo.mUseJavaBackend
|| mDeps.isMdnsDiscoveryManagerEnabled(mContext)
|| useDiscoveryManagerForType(serviceType)) {
@@ -871,7 +889,10 @@
final NsdServiceInfo info = args.serviceInfo;
id = getUniqueId();
- final String serviceType = constructServiceType(info.getServiceType());
+ final Pair<String, String> typeAndSubtype =
+ parseTypeAndSubtype(info.getServiceType());
+ final String serviceType = typeAndSubtype == null
+ ? null : typeAndSubtype.first;
if (serviceType == null) {
clientInfo.onServiceInfoCallbackRegistrationFailed(clientId,
NsdManager.FAILURE_BAD_PARAMETERS);
@@ -1315,28 +1336,39 @@
* Check the given service type is valid and construct it to a service type
* which can use for discovery / resolution service.
*
- * <p> The valid service type should be 2 labels, or 3 labels if the query is for a
+ * <p>The valid service type should be 2 labels, or 3 labels if the query is for a
* subtype (see RFC6763 7.1). Each label is up to 63 characters and must start with an
* underscore; they are alphanumerical characters or dashes or underscore, except the
* last one that is just alphanumerical. The last label must be _tcp or _udp.
*
+ * <p>The subtype may also be specified with a comma after the service type, for example
+ * _type._tcp,_subtype.
+ *
* @param serviceType the request service type for discovery / resolution service
* @return constructed service type or null if the given service type is invalid.
*/
@Nullable
- public static String constructServiceType(String serviceType) {
+ public static Pair<String, String> parseTypeAndSubtype(String serviceType) {
if (TextUtils.isEmpty(serviceType)) return null;
+ final String typeOrSubtypePattern = "_[a-zA-Z0-9-_]{1,61}[a-zA-Z0-9]";
final Pattern serviceTypePattern = Pattern.compile(
- "^(_[a-zA-Z0-9-_]{1,61}[a-zA-Z0-9]\\.)?"
- + "(_[a-zA-Z0-9-_]{1,61}[a-zA-Z0-9]\\._(?:tcp|udp))"
+ // Optional leading subtype (_subtype._type._tcp)
+ // (?: xxx) is a non-capturing parenthesis, don't capture the dot
+ "^(?:(" + typeOrSubtypePattern + ")\\.)?"
+ // Actual type (_type._tcp.local)
+ + "(" + typeOrSubtypePattern + "\\._(?:tcp|udp))"
// Drop '.' at the end of service type that is compatible with old backend.
- + "\\.?$");
+ // e.g. allow "_type._tcp.local."
+ + "\\.?"
+ // Optional subtype after comma, for "_type._tcp,_subtype" format
+ + "(?:,(" + typeOrSubtypePattern + "))?"
+ + "$");
final Matcher matcher = serviceTypePattern.matcher(serviceType);
if (!matcher.matches()) return null;
- return matcher.group(1) == null
- ? matcher.group(2)
- : matcher.group(1) + "_sub." + matcher.group(2);
+ // Use the subtype either at the beginning or after the comma
+ final String subtype = matcher.group(1) != null ? matcher.group(1) : matcher.group(3);
+ return new Pair<>(matcher.group(2), subtype);
}
@VisibleForTesting
diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java b/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java
index 298e632..4e6571f 100644
--- a/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java
+++ b/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java
@@ -28,13 +28,16 @@
import com.android.internal.annotations.GuardedBy;
import com.android.internal.annotations.VisibleForTesting;
+import com.android.net.module.util.CollectionUtils;
import com.android.net.module.util.SharedLog;
+import com.android.server.connectivity.mdns.util.MdnsUtils;
import java.net.Inet4Address;
import java.net.Inet6Address;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
@@ -209,9 +212,22 @@
private boolean responseMatchesOptions(@NonNull MdnsResponse response,
@NonNull MdnsSearchOptions options) {
- if (options.getResolveInstanceName() == null) return true;
- // DNS is case-insensitive, so ignore case in the comparison
- return options.getResolveInstanceName().equalsIgnoreCase(response.getServiceInstanceName());
+ final boolean matchesInstanceName = options.getResolveInstanceName() == null
+ // DNS is case-insensitive, so ignore case in the comparison
+ || MdnsUtils.equalsIgnoreDnsCase(options.getResolveInstanceName(),
+ response.getServiceInstanceName());
+
+ // If discovery is requiring some subtypes, the response must have one that matches a
+ // requested one.
+ final List<String> responseSubtypes = response.getSubtypes() == null
+ ? Collections.emptyList() : response.getSubtypes();
+ final boolean matchesSubtype = options.getSubtypes().size() == 0
+ || CollectionUtils.any(options.getSubtypes(), requiredSub ->
+ CollectionUtils.any(responseSubtypes, actualSub ->
+ MdnsUtils.equalsIgnoreDnsCase(
+ MdnsConstants.SUBTYPE_PREFIX + requiredSub, actualSub)));
+
+ return matchesInstanceName && matchesSubtype;
}
/**
diff --git a/tests/unit/java/com/android/server/NsdServiceTest.java b/tests/unit/java/com/android/server/NsdServiceTest.java
index 5ca3934..322b4d2 100644
--- a/tests/unit/java/com/android/server/NsdServiceTest.java
+++ b/tests/unit/java/com/android/server/NsdServiceTest.java
@@ -23,7 +23,7 @@
import static android.net.nsd.NsdManager.FAILURE_INTERNAL_ERROR;
import static android.net.nsd.NsdManager.FAILURE_OPERATION_NOT_RUNNING;
-import static com.android.server.NsdService.constructServiceType;
+import static com.android.server.NsdService.parseTypeAndSubtype;
import static com.android.testutils.ContextUtils.mockService;
import static libcore.junit.util.compat.CoreCompatChangeRule.DisableCompatChanges;
@@ -77,6 +77,7 @@
import android.os.Looper;
import android.os.Message;
import android.os.RemoteException;
+import android.util.Pair;
import androidx.annotation.NonNull;
import androidx.test.filters.SmallTest;
@@ -84,6 +85,7 @@
import com.android.server.NsdService.Dependencies;
import com.android.server.connectivity.mdns.MdnsAdvertiser;
import com.android.server.connectivity.mdns.MdnsDiscoveryManager;
+import com.android.server.connectivity.mdns.MdnsSearchOptions;
import com.android.server.connectivity.mdns.MdnsServiceBrowserListener;
import com.android.server.connectivity.mdns.MdnsServiceInfo;
import com.android.server.connectivity.mdns.MdnsSocketProvider;
@@ -104,6 +106,7 @@
import java.net.InetAddress;
import java.net.UnknownHostException;
+import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
@@ -969,6 +972,35 @@
}
@Test
+ @EnableCompatChanges(ENABLE_PLATFORM_MDNS_BACKEND)
+ public void testDiscoveryWithMdnsDiscoveryManager_UsesSubtypes() {
+ final String typeWithSubtype = SERVICE_TYPE + ",_subtype";
+ final NsdManager client = connectClient(mService);
+ final NsdServiceInfo regInfo = new NsdServiceInfo("Instance", typeWithSubtype);
+ final Network network = new Network(999);
+ regInfo.setHostAddresses(List.of(parseNumericAddress("192.0.2.123")));
+ regInfo.setPort(12345);
+ regInfo.setNetwork(network);
+
+ final RegistrationListener regListener = mock(RegistrationListener.class);
+ client.registerService(regInfo, NsdManager.PROTOCOL_DNS_SD, Runnable::run, regListener);
+ waitForIdle();
+ // TODO: also pass the subtype to MdnsAdvertiser
+ verify(mAdvertiser).addService(anyInt(), argThat(s ->
+ "Instance".equals(s.getServiceName())
+ && SERVICE_TYPE.equals(s.getServiceType())));
+
+ final DiscoveryListener discListener = mock(DiscoveryListener.class);
+ client.discoverServices(typeWithSubtype, PROTOCOL, network, Runnable::run, discListener);
+ waitForIdle();
+ final ArgumentCaptor<MdnsSearchOptions> optionsCaptor =
+ ArgumentCaptor.forClass(MdnsSearchOptions.class);
+ verify(mDiscoveryManager).registerListener(eq(SERVICE_TYPE + ".local"), any(),
+ optionsCaptor.capture());
+ assertEquals(Collections.singletonList("subtype"), optionsCaptor.getValue().getSubtypes());
+ }
+
+ @Test
public void testResolutionWithMdnsDiscoveryManager() throws UnknownHostException {
setMdnsDiscoveryManagerEnabled();
@@ -976,7 +1008,7 @@
final ResolveListener resolveListener = mock(ResolveListener.class);
final Network network = new Network(999);
final String serviceType = "_nsd._service._tcp";
- final String constructedServiceType = "_nsd._sub._service._tcp.local";
+ final String constructedServiceType = "_service._tcp.local";
final ArgumentCaptor<MdnsServiceBrowserListener> listenerCaptor =
ArgumentCaptor.forClass(MdnsServiceBrowserListener.class);
final NsdServiceInfo request = new NsdServiceInfo(SERVICE_NAME, serviceType);
@@ -984,12 +1016,14 @@
client.resolveService(request, resolveListener);
waitForIdle();
verify(mSocketProvider).startMonitoringSockets();
- // TODO(b/266167702): this is a bug, as registerListener should be done _service._tcp, and
- // _sub should be in the list of subtypes in the options.
+ final ArgumentCaptor<MdnsSearchOptions> optionsCaptor =
+ ArgumentCaptor.forClass(MdnsSearchOptions.class);
verify(mDiscoveryManager).registerListener(eq(constructedServiceType),
- listenerCaptor.capture(), argThat(options ->
- network.equals(options.getNetwork())
- && SERVICE_NAME.equals(options.getResolveInstanceName())));
+ listenerCaptor.capture(),
+ optionsCaptor.capture());
+ assertEquals(network, optionsCaptor.getValue().getNetwork());
+ // Subtypes are not used for resolution, only for discovery
+ assertEquals(Collections.emptyList(), optionsCaptor.getValue().getSubtypes());
final MdnsServiceBrowserListener listener = listenerCaptor.getValue();
final MdnsServiceInfo mdnsServiceInfo = new MdnsServiceInfo(
@@ -1013,8 +1047,7 @@
verify(resolveListener, timeout(TIMEOUT_MS)).onServiceResolved(infoCaptor.capture());
final NsdServiceInfo info = infoCaptor.getValue();
assertEquals(SERVICE_NAME, info.getServiceName());
- // TODO(b/266167702): this should be ._service._tcp (as per legacy behavior)
- assertEquals("._nsd._sub._service._tcp", info.getServiceType());
+ assertEquals("._service._tcp", info.getServiceType());
assertEquals(PORT, info.getPort());
assertTrue(info.getAttributes().containsKey("key"));
assertEquals(1, info.getAttributes().size());
@@ -1222,7 +1255,7 @@
final ResolveListener resolveListener = mock(ResolveListener.class);
final Network network = new Network(999);
final String serviceType = "_nsd._service._tcp";
- final String constructedServiceType = "_nsd._sub._service._tcp.local";
+ final String constructedServiceType = "_service._tcp.local";
final ArgumentCaptor<MdnsServiceBrowserListener> listenerCaptor =
ArgumentCaptor.forClass(MdnsServiceBrowserListener.class);
final NsdServiceInfo request = new NsdServiceInfo(SERVICE_NAME, serviceType);
@@ -1230,8 +1263,14 @@
client.resolveService(request, resolveListener);
waitForIdle();
verify(mSocketProvider).startMonitoringSockets();
+ final ArgumentCaptor<MdnsSearchOptions> optionsCaptor =
+ ArgumentCaptor.forClass(MdnsSearchOptions.class);
verify(mDiscoveryManager).registerListener(eq(constructedServiceType),
- listenerCaptor.capture(), argThat(options -> network.equals(options.getNetwork())));
+ listenerCaptor.capture(),
+ optionsCaptor.capture());
+ assertEquals(network, optionsCaptor.getValue().getNetwork());
+ // Subtypes are not used for resolution, only for discovery
+ assertEquals(Collections.emptyList(), optionsCaptor.getValue().getSubtypes());
client.stopServiceResolution(resolveListener);
waitForIdle();
@@ -1246,16 +1285,22 @@
}
@Test
- public void testConstructServiceType() {
+ public void testParseTypeAndSubtype() {
final String serviceType1 = "test._tcp";
final String serviceType2 = "_test._quic";
- final String serviceType3 = "_123._udp.";
- final String serviceType4 = "_TEST._999._tcp.";
+ final String serviceType3 = "_test._quic,_test1,_test2";
+ final String serviceType4 = "_123._udp.";
+ final String serviceType5 = "_TEST._999._tcp.";
+ final String serviceType6 = "_998._tcp.,_TEST";
+ final String serviceType7 = "_997._tcp,_TEST";
- assertEquals(null, constructServiceType(serviceType1));
- assertEquals(null, constructServiceType(serviceType2));
- assertEquals("_123._udp", constructServiceType(serviceType3));
- assertEquals("_TEST._sub._999._tcp", constructServiceType(serviceType4));
+ assertNull(parseTypeAndSubtype(serviceType1));
+ assertNull(parseTypeAndSubtype(serviceType2));
+ assertNull(parseTypeAndSubtype(serviceType3));
+ assertEquals(new Pair<>("_123._udp", null), parseTypeAndSubtype(serviceType4));
+ assertEquals(new Pair<>("_999._tcp", "_TEST"), parseTypeAndSubtype(serviceType5));
+ assertEquals(new Pair<>("_998._tcp", "_TEST"), parseTypeAndSubtype(serviceType6));
+ assertEquals(new Pair<>("_997._tcp", "_TEST"), parseTypeAndSubtype(serviceType7));
}
@Test
diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java
index 98cea62..bd59156 100644
--- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java
+++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java
@@ -42,6 +42,7 @@
import android.net.Network;
import android.text.TextUtils;
+import com.android.net.module.util.CollectionUtils;
import com.android.net.module.util.SharedLog;
import com.android.server.connectivity.mdns.MdnsServiceInfo.TextEntry;
import com.android.server.connectivity.mdns.MdnsServiceTypeClient.QueryTaskConfig;
@@ -53,6 +54,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
+import org.mockito.ArgumentMatcher;
import org.mockito.Captor;
import org.mockito.InOrder;
import org.mockito.Mock;
@@ -1086,6 +1088,87 @@
}
@Test
+ public void testProcessResponse_SubtypeDiscoveryLimitedToSubtype() {
+ client = new MdnsServiceTypeClient(
+ SERVICE_TYPE, mockSocketClient, currentThreadExecutor, mockNetwork, mockSharedLog);
+
+ final String matchingInstance = "instance1";
+ final String subtype = "_subtype";
+ final String otherInstance = "instance2";
+ final String ipV4Address = "192.0.2.0";
+ final String ipV6Address = "2001:db8::";
+
+ final MdnsSearchOptions options = MdnsSearchOptions.newBuilder()
+ // Search with different case. Note MdnsSearchOptions subtype doesn't start with "_"
+ .addSubtype("Subtype").build();
+
+ client.startSendAndReceive(mockListenerOne, options);
+ client.startSendAndReceive(mockListenerTwo, MdnsSearchOptions.getDefaultOptions());
+
+ // Complete response from instanceName
+ final MdnsPacket packetWithoutSubtype = createResponse(
+ matchingInstance, ipV4Address, 5353, SERVICE_TYPE_LABELS,
+ Collections.emptyMap() /* textAttributes */, TEST_TTL);
+ final MdnsPointerRecord originalPtr = (MdnsPointerRecord) CollectionUtils.findFirst(
+ packetWithoutSubtype.answers, r -> r instanceof MdnsPointerRecord);
+
+ // Add a subtype PTR record
+ final ArrayList<MdnsRecord> newAnswers = new ArrayList<>(packetWithoutSubtype.answers);
+ newAnswers.add(new MdnsPointerRecord(
+ // PTR should be _subtype._sub._type._tcp.local -> instance1._type._tcp.local
+ Stream.concat(Stream.of(subtype, "_sub"), Arrays.stream(SERVICE_TYPE_LABELS))
+ .toArray(String[]::new),
+ originalPtr.getReceiptTime(), originalPtr.getCacheFlush(), originalPtr.getTtl(),
+ originalPtr.getPointer()));
+ final MdnsPacket packetWithSubtype = new MdnsPacket(
+ packetWithoutSubtype.flags,
+ packetWithoutSubtype.questions,
+ newAnswers,
+ packetWithoutSubtype.authorityRecords,
+ packetWithoutSubtype.additionalRecords);
+ client.processResponse(packetWithSubtype, INTERFACE_INDEX, mockNetwork);
+
+ // Complete response from otherInstanceName, without subtype
+ client.processResponse(createResponse(
+ otherInstance, ipV4Address, 5353, SERVICE_TYPE_LABELS,
+ Collections.emptyMap() /* textAttributes */, TEST_TTL),
+ INTERFACE_INDEX, mockNetwork);
+
+ // Address update from otherInstanceName
+ client.processResponse(createResponse(
+ otherInstance, ipV6Address, 5353, SERVICE_TYPE_LABELS,
+ Collections.emptyMap(), TEST_TTL), INTERFACE_INDEX, mockNetwork);
+
+ // Goodbye from otherInstanceName
+ client.processResponse(createResponse(
+ otherInstance, ipV6Address, 5353, SERVICE_TYPE_LABELS,
+ Collections.emptyMap(), 0L /* ttl */), INTERFACE_INDEX, mockNetwork);
+
+ // mockListenerOne gets notified for the requested instance
+ final ArgumentMatcher<MdnsServiceInfo> subtypeInstanceMatcher = info ->
+ info.getServiceInstanceName().equals(matchingInstance)
+ && info.getSubtypes().equals(Collections.singletonList(subtype));
+ verify(mockListenerOne).onServiceNameDiscovered(argThat(subtypeInstanceMatcher));
+ verify(mockListenerOne).onServiceFound(argThat(subtypeInstanceMatcher));
+
+ // ...but does not get any callback for the other instance
+ verify(mockListenerOne, never()).onServiceFound(matchServiceName(otherInstance));
+ verify(mockListenerOne, never()).onServiceNameDiscovered(matchServiceName(otherInstance));
+ verify(mockListenerOne, never()).onServiceUpdated(matchServiceName(otherInstance));
+ verify(mockListenerOne, never()).onServiceRemoved(matchServiceName(otherInstance));
+
+ // mockListenerTwo gets notified for both though
+ final InOrder inOrder = inOrder(mockListenerTwo);
+ inOrder.verify(mockListenerTwo).onServiceNameDiscovered(argThat(subtypeInstanceMatcher));
+ inOrder.verify(mockListenerTwo).onServiceFound(argThat(subtypeInstanceMatcher));
+
+ inOrder.verify(mockListenerTwo).onServiceNameDiscovered(matchServiceName(otherInstance));
+ inOrder.verify(mockListenerTwo).onServiceFound(matchServiceName(otherInstance));
+ inOrder.verify(mockListenerTwo).onServiceUpdated(matchServiceName(otherInstance));
+ inOrder.verify(mockListenerTwo).onServiceRemoved(matchServiceName(otherInstance));
+ }
+
+ @Test
public void testNotifyAllServicesRemoved() {
client = new MdnsServiceTypeClient(
SERVICE_TYPE, mockSocketClient, currentThreadExecutor, mockNetwork, mockSharedLog);