Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNS-SD API #7

Closed
Gozala opened this issue May 16, 2018 · 19 comments
Closed

DNS-SD API #7

Gozala opened this issue May 16, 2018 · 19 comments
Assignees
Labels
Discovery Service Discovery API related issues

Comments

@Gozala
Copy link
Contributor

Gozala commented May 16, 2018

mdns used for local peer discovery and is critical for enabling dweb protocols on the local network. There is an mdns artifacts from Project Flyweb that likely could be utilized to expose WebExtensions API.

@Gozala Gozala changed the title mdns Multicast DNS API May 16, 2018
@Gozala Gozala added this to the Discovery APIs milestone May 16, 2018
@Gozala
Copy link
Contributor Author

Gozala commented May 16, 2018

Feedback from @mafintosh

assuming the udp client above can support multicast and REUSEADDR we don't really need a dedicated multicast api

@Gozala
Copy link
Contributor Author

Gozala commented May 16, 2018

Chrome OS apps seem to have mdns API

@autonome
Copy link
Contributor

@Gozala
Copy link
Contributor Author

Gozala commented May 23, 2018

@autonome It looks like this is what IPFS is currently using for mdns:
https://github.com/libp2p/js-libp2p-mdns

And my guess is this is what Dat uses
https://github.com/mafintosh/multicast-dns

Would be worth taking a look what the API is to try and match that

@mafintosh
Copy link

mafintosh commented May 23, 2018 via email

@justindarc
Copy link

Hello. I helped to develop the mDNS implementation for FlyWeb. I'd like to know more about this project. Is there a channel on IRC/Slack?

@autonome
Copy link
Contributor

autonome commented May 24, 2018 via email

@Gozala
Copy link
Contributor Author

Gozala commented Jun 7, 2018

@autonome has landed simple discovery API and I made an example add-on https://github.com/mozilla/libdweb/tree/master/demo/mdns. There is also gif in readme now showing it in action.

@rektide
Copy link

rektide commented Jun 14, 2018

It's discovery only, and once upon a day had implementations (Opera!), but perhaps there's some value to mentioning that there was as Discovery API specification that web pages could use to find mdns services.

@RangerMauve
Copy link

MDNS is great for discovery, but won't this require the ability to listen on ports from a WebExtension to be useful?

@Gozala
Copy link
Contributor Author

Gozala commented Jun 19, 2018

MDNS is great for discovery, but won't this require the ability to listen on ports from a WebExtension to be useful?

That's why we also expose UDP / TCP socket APIs.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 12, 2018

Code that crashes the Nightly:

const {
  classes: Cc,
  interfaces: Ci,
  utils: Cu,
  results: Cr,
  manager: Cm
} = Components

Cu.import("resource://gre/modules/XPCOMUtils.jsm");

const DNS_SD_CID = "@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1"
const sd = Cc[DNS_SD_CID].getService(Ci.nsIDNSServiceDiscovery)

let serviceType = '_http._tcp';

let registrationListener = {
  onServiceRegistered: function(serviceInfo) {
    console.log('svcRegistered', serviceInfo);
  },
  onServiceUnregistered: function(serviceInfo) {
    console.log('svcUnregistered', serviceInfo);
  },
  onRegistrationFailed: function(serviceInfo, errorCode) {
    console.log('regFailed', serviceInfo, errorCode);
  },
  onUnregistrationFailed: function(serviceInfo, errorCode) {
    console.log('unregFailed', serviceInfo, errorCode);
  }
};

let serviceInfo = {
  port: 80,
  serviceType: '_http._tcp'
};

sd.registerService(serviceInfo, registrationListener);

@Gozala
Copy link
Contributor Author

Gozala commented Jul 12, 2018

@autonome Following does not seems to crash:

Cu.import("resource://gre/modules/XPCOMUtils.jsm");

class DNSServiceInfo {
  constructor(host="", port=0, serviceName="", serviceType="", domainName="", attributes=versionAttr) {
    this.host = host  
    this.port = port
    this.address = host
    this.serviceName = serviceName
    this.serviceType = serviceType
    this.domainName = domainName
    this.attributes = attributes
  }
}
DNSServiceInfo.prototype.QueryInterface = ChromeUtils.generateQI([Ci.nsIDNSServiceInfo])

class DNSRegistrationListener {
  onServiceRegistered(serviceInfo) {
    console.log('onServiceRegistered', serviceInfo);
  }
  onServiceUnregistered(serviceInfo) {
    console.log('onServiceUnregistered', serviceInfo);
  }
  onRegistrationFailed(serviceInfo, errorCode) {
    console.log('onRegistrationFailed', serviceInfo, errorCode);
  }
  onUnregistrationFailed(serviceInfo, errorCode) {
    console.log('onUnregistrationFailed', serviceInfo, errorCode);
  }
}

const SERVICE_TYPE = "_presentation-ctrl._tcp"
const LATEST_VERSION = 1

const versionAttr = Cc["@mozilla.org/hash-property-bag;1"]
                      .createInstance(Ci.nsIWritablePropertyBag2)
versionAttr.setPropertyAsUint32("version", LATEST_VERSION)



const mDNS = Cc["@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1"]
.getService(Ci.nsIDNSServiceDiscovery)

const service = new DNSServiceInfo("device.local", 12345, "service.name", SERVICE_TYPE)
const listener = new DNSRegistrationListener()
mDNS.registerService(service, listener)

I suspect that C++ implementation attempts to cast serviceInfo as nsIDNSServiceInfo or attempts to use attributes field that was no present serviceInfo

@Gozala Gozala assigned Gozala and unassigned autonome Jul 13, 2018
@Gozala
Copy link
Contributor Author

Gozala commented Jul 13, 2018

Posting some discussion from IRC where @justindarc provides some insight on why the gecko implementation is they way it is that i think might be relevant to wider audience:

<•dietrich> we probably shouldn't crash Firefox when passing invalid params, but seeing as we're just lucky this code didn't get removed, let's not draw attention to it just yet ;)
9:23 AM J haha
9:23 AM yeah...
9:23 AM it really is a major PITA...
9:24 AM i don't envy our devs that have to work in that mess every day
9:24 AM D<•dietrich> there are a few mdns implementations in node, maybe we just write browserify transforms that use the tcp/udp socket apis
9:24 AM yeah for real
9:25 AM J so... the tricky thing is how to handle situations where there's an existing system-provided responder
9:25 AM because only one service can listen on 5353
9:26 AM on macOS, you pretty much are required to use apple's impl
9:26 AM D<•dietrich> gah, i forgot already
9:26 AM J on linux, it is hit or miss depending on whether or not avahi is installed...
9:27 AM D<•dietrich> whose idea was it to not use a port range
9:27 AM J for real
9:27 AM D<•dietrich> oh wait, that is perfectly apple.
9:28 AM S justindarc: so basically you need to try to listen, fail, and then switch to whatever is the native thing to do?
9:29 AM J pretty much, yeah :-/
9:29 AM S argh!
9:29 AM J even if you look at node_mdns, it does just that: https://github.com/agnat/node_mdns
9:30 AM S was trying to play with that yesterday but I recall it not building on Windows (or it was mdns and not node_mdns), I tried many of them yesterday.
9:30 AM J its got bindings to all the native implementations on various platforms... this is alot like what we had to do in gecko
9:31 AM one thing on windows...
9:31 AM windows 10 was arriving right around the time we were working on this...
9:31 AM prior to windows 10, there is NO system mDNS responder (unless, of course, you have iTunes installed)
9:31 AM installing iTunes on windows gives you Apple's "Bonjour" service
9:32 AM but anyhow... windows 10 introduced microsoft's own system-provided mDNS responder... with a catch...
9:32 AM their implementation didn't seem to expose any public-facing API for 3rd party apps to hook into... it seemed incomplete when we last looked at it
9:33 AM so, the JS implementation we have in gecko worked around this somehow, but i'm fuzzy on remembering the details..
9:33 AM i think it had something to do with how we attached to port 5353
9:34 AM a-ha... see here...
9:34 AM https://searchfox.org/mozilla-central/source/netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm#634-679
9:34 AM "Gets a non-exclusive socket on 0.0.0.0:5353"...
9:35 AM i believe this was how we worked around those hit-or-miss system implementations
9:36 AM there was a lot of use of wireshark during this whole thing trying to figure out how to make it work
9:36 AM S omg, that sounds very painful
9:36 AM yup
9:46 AM so, re-reading this... i think this is how it worked... we'd send queries out on a socket with a random port (sending the query to multicast 224.0.0.251:5353)
9:46 AM that would work even on systems with a system mDNS responder
9:47 AM we would also try and get a non-exclusive listener socket on 5353 to receive queries from other hosts
9:47 AM when we send a query, clients would respond directly back to us on the port we sent the query from
9:51 AM and i think the reason we separated out sending and receiving to two different sockets like that was how we got this to work alongside system mDNS responders
9:52 AM so... i have a question... now that you're providing all the props to serviceInfo, is it working?
9:52 AM or are you still seeing crashes?
10:56 AM ↔ Caspy7 (was Caspy77) nipped out
11:07 AM D<•dietrich> justindarc: nope, see gozala's second code sample in that issue - he says no more crashing when passing values for all parameters! i haven't tried end-to-end test yet though
11:08 AM → pfrazee joined (sid217471@moz-6kq8rr.hathersage.irccloud.com)
11:08 AM D<•dietrich> eg, to see if registering a service shows up in other mdns listeners
11:08 AM P hola!
11:08 AM → bret joined (sid12421@moz-r6c304.brockwell.irccloud.com)
11:09 AM B 👋
11:09 AM D<•dietrich> buenos días pfrazee!
11:09 AM B ohai dietrich long time :V
11:09 AM G Irakli Gozalishvili justindarc: it’s not crashing, and issue was with propertyBags

@Gozala
Copy link
Contributor Author

Gozala commented Jul 13, 2018

There was some more discussion on the IRC specifically I was trying to figure out why differences between nsIDNSServiceDiscovery and dns-discovery / libp2p-mdns both IPFS and Dat seem to differ from implementation in gecko in a same way. Specifically gecko implements DNS-SD according to https://tools.ietf.org/html/rfc6763 which requires serviceType like _dat._udp / _ipfs._tcp or more specifically names in libp2p are like QmNPubsDWATVngE3d5WDSNe7eVrFLuk38qb9t6vdLnu2aK.ipfs.local which according to the mentioned spec should rather be QmNPubsDWATVngE3d5WDSNe7eVrFLuk38qb9t6vdLnu2aK._ipfs._tcp.local (Used ipfs example as it was easier to copy from their readme, but I believe it's similar for dat).

Issue

  1. As things stand I'm unable to use nsIDNSServiceDiscovery to discover either ipfs or dat as our implementation requires serviceType
  2. I'm unable to announce service such that dat or ipfs would discover as again our implementation requires serviceType.
  3. Not using gecko built-in implementation has some issues as system service may be listening on 5353 which causes issues.

Questions

  • Is there specific reason to not use DNS-SD in dat / ipfs ? Glancing through the implementations it seems to me that packet exchange is equivalent across all except omitted serviceType.
  • If difference with DNS-SD is not deliberate, or not super important would you consider using DNS-SD instead from what I can tell it should be fairly simple change.
  • If switch to DNS-SD is a no go how about incorporating DNS-SD along with what is already used ? That would at least allow ipfs / dat to talk to libdweb based implementation that could use DNS-SD

@mafintosh @lidel @olizilla I could use your input here.

@lidel
Copy link
Contributor

lidel commented Jul 15, 2018

@Gozala did some digging and on the IPFS side it seems both go and js implementations of mDNS discovery will be changed to use the new suffix: _ipfs._upd.local, which should be compatible with DNS-SD. Relevant discussion about future of mDNS discovery in IPFS: libp2p/libp2p#28

@Gozala Gozala changed the title Multicast DNS API DNS SD API Jul 17, 2018
@Gozala Gozala changed the title DNS SD API DNS-SD API Jul 17, 2018
@Gozala
Copy link
Contributor Author

Gozala commented Jul 17, 2018

mDNS -> ServiceDiscovery

After more chatter (following #7 (comment)) on IRC it became clear that they way IPFS and Dat use mDNS for discovery is remarkably similar to DNS-Based Service Discovery (as per rfc6763) except some incompatibilities described in the previous comment. As per @lidel's comment above it seems that IPFS is going to resolve incompatibility and as I understand @mafintosh also agreed to do that on Dat side, which is needless to say I'm super excited about!

In the light of this I renamed mDNS API to ServiceDiscovery and landed implementation that allows discovery of services and announcement. Readme contains some examples.

I would like make few notes and request feedback on the API itself as I'm having some reservations:

  1. I took some inspiration from @watson/bonjour library, specifically I liked idea of breaking serviceType parameter _http._tcp into two parameters {type:"http", protocol:"tcp"} as it reduces error surface.
  2. For service discovery you'd do browser.ServiceDiscovery.discover({ type:"http", protocol:"tcp" }) which returns AsyncIterator of DiscoveredService's that are objects like {name: 'Service Name', type: 'http', domain: 'local', protocol: 'tcp', lost:false}. Notice lost flag it will be true if service is expired / stopped.
    • Idea is to use for await loop, if you break the loop discovery will be cancelled, otherwise you'll get updates on found / lost services. When you start discovery in nsIDNSServiceDiscovery it eventually finishes not sure on what condition (@justindarc probably knows) when that happens iteration will finish. In a way AsyncIterator is just a replacement for event emitters that we do not have.
  3. Additionally DiscoveredService has method addresses() which returns promise that resolves to array of objects like {address:'0.0.0.0', port:3000, host: 'hostname.local', attributes:{version:'1.01}}.
    • I really dislike that you need to iterate services first and then explicitly fetch addresses. It's two-step process because nsIDNSServiceDiscovery API has startDiscovery to discover services and has resolveService to get address and (as it turns out) attributes.
    • I assumed that attributes would be included in discovered services (meaning in startDiscovery) but turns out they aren't, instead they are returned by resolveService so service.addresses() makes even less sense. Depending on feedback I'd either rename or do entirely different API. BTW @justindarc I have being wondering if attributes could differ across resolved instances for the same service.
    • I think very much prefer @watson/bonjour API as services there preclude all the attributes + addresses. Unless there is a good reason to not do it I'm leaning to do the same. (Alternatively I could expose browser.ServiceDiscovery.browse to discover {name, type, protocol} objects, browser.ServiceDiscovery.discover to discover {name, type, protocol, addresses, host, port, attributes} and browser.ServiceDiscovery.resolve to map from first to second. Thoughts ?
  4. To announce service there is:
    const service = await browser.ServiceDiscovery.announce({
      name: "My dweb service",
      type: "dweb",
      protocol: "tcp", // must be "tcp" or "udp"
      port: 3000, // ommting port will just assign you available one.
      attributes: {
        // optional txt records
        version: "1.0."
      }
    })
  5. service above will have all the same properties as passed in except name might be different if conflicting name exists.
  6. announced service also has .expire() method that can be used to let discoverers know service is no longer alive.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 23, 2018

@justindarc BTW I have also wondered if there was a reason to resolve hostname like this: https://github.com/mozilla/gecko-dev/blob/86897859913403b68829dbf9a154f5a87c4b0638/netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm#L732-L744 instead of say:

Cc['@mozilla.org/network/dns-service;1']
  .getService(Ci.nsIDNSService)
  .myHostName

@Gozala Gozala added the Discovery Service Discovery API related issues label Jul 23, 2018
@justindarc
Copy link

@Gozala sorry for the late response. Here's my responses to the questions you tagged me in:

  1. It is never really clear when discovery "finishes". Since discovery relies on UDP, packet loss is always a possibility and therefore most applications using DNS-SD will re-send the query packet periodically and maintain their own list of discovered services. One thing to consider is when to "expire" services that were at once found, but no longer respond to queries after going offline. Because packets can sometimes trickle in late in response to an mDNS/DNS-SD query, I would suggest setting a reasonable timeout for the query until you consider it "complete".
  2. I can't remember the exactly reason for this. I seem to recall this having to do with certain implementations not broadcasting all the necessary DNS packets to get the addresses during the initial multicast discovery process. But take that with a massive grain of salt.
  3. That should be doable. If you look here, you can specify host as part of the service. It falls back to the OS hostname in getHostname() as you already noted. But, as you can see, it is designed to be overridable.

@Gozala Gozala closed this as completed Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discovery Service Discovery API related issues
Projects
None yet
Development

No branches or pull requests

7 participants