Service Discovery goodbye message

cerna commented 5 years ago

Hello, looking at ServiceDiscovery.Advertise() method to announce new service I don't see any way how to deannounce announced service. Typical way of doing this is to send a type of "goodbye message" where the Time-To-Live property is set to zero so listeners can update theirs records and take these services out of the roster.

On the same line when disposing of ServiceDiscovery object, there is no clean up. So services will "appear" alive to the end ot TTL, which could be a while.

Theoretically I can do something dirty like this:

ServiceProfile service = new ServiceProfile("x1", "_xservice._tcp", 5011);
sd.Advertise(service);
mdns.Start();
service.Resources.ForEach(s => { s.TTL = TimeSpan.Zero; });

Hovewer then it will still stay in DNS server collection.

richardschneider commented 5 years ago

Yes, goodbye packets are specified in https://tools.ietf.org/html/rfc6762#section-10.1

ServiceDiscovery is already IDisposable, so we can have it set the TTLs to zero and then send an unsolicited response with all the resource records.

Are you interested in submitting a PR?

richardschneider commented 5 years ago

On second thought, enhancing Dispose is not a good idea. Dispose is synchronous and sending a response is async.

Perhaps a new method ShutdownAsync which sends the goodbye packet.

cerna commented 5 years ago

Well, maybe I got this wrong, but I thought that all is needed is to purge ServiceDiscovery.profiles and ServiceDiscovery.NameServer.Catalog collections of the service one wants to stop advertising or better yet first set them to TTL TimeSpan.Zero and then do the purge(?)

richardschneider commented 5 years ago

Both of us are correct. I tend to use one ServiceDiscovery per each ServiceProfile. This way after disposing of the SD a query for the service will not be answered.

When ServiceDiscovery.Dispose is called, it will no longer answer any queries.

However, if you are using one ServiceDiscovery for multiple service profiles, then you are correct. An Unadvertise(ServiceProfile) is needed to remove the resource records from the NameServer.Catalog.

Neither case will send goodbye packets so that other MDNS hosts know that the service is now dead. As you mentioned, when TTL is reached they will then know that the service is dead.

So, this leads to two methods

ServiceDiscovery.UnadvertiseAsync(ServiceProfile), which sends good bye packets and then removes the resource records from the catalog.
ServiceDiscovery.Unadvertise(), performs UnadvertiseAsync for all profiles.

cerna commented 5 years ago

Yeah, how you described possible solution at the end was how I was thinking about implementing it. ServiceDiscovery.Unadvertise() would be then called in Dispose() or maybe Stop(). (I have little problem with current flow of logic of Stop() method, because at least in me it evokes Start-Pause-Stop scenario which evidently is not how it works.)

I will try to cook something up but I will have to study/understand the inner workings of underlying Makaretu.DNS some more first.

EDIT: BTW, I have been trying to make heads or tails of how this works and I see no forced "announcement traffic" at the start of Advertise(ServiceProfile) either (i.e. no method SendReplyMessageWithoutQuerry(parameters)). Is that correct? And is that normalised behaviour? (Sorry I am using your knowledge of specification as is.)

NickAcPT commented 5 years ago

Has there been any progress on this issue?

gilzad commented 5 years ago

I got busy with this in an attempt to exchange Apple's Bonjour by net-mdns. Not succeeding so far, I approached with this experiment:

public void Unadvertise()
{
    Message message = new Message();
    foreach (KeyValuePair<string, Node> catEntry in NameServer.Catalog)
    {
        foreach(ResourceRecord resourceRecord in catEntry.Value.Resources)
        {
            resourceRecord.TTL = TimeSpan.Zero;
            message.Answers.Add(resourceRecord);
        }
    }
    this.Mdns.SendAnswer(message);
    NameServer.Catalog.Clear();
}

In Wireshark I can see that the previously announced services are 'answered' with a TTL of 0.

_services._dns-sd._udp.local: type PTR, class IN, _soap._tcp.local
    Name: _services._dns-sd._udp.local
    Type: PTR (domain name PoinTeR) (12)
    .000 0000 0000 0001 = Class: IN (0x0001)
    0... .... .... .... = Cache flush: False
    Time to live: 0
    Data length: 13
    Domain Name: _soap._tcp.local
z1._soap._tcp.local: type TXT, class IN
    Name: z1._soap._tcp.local
    Type: TXT (Text strings) (16)
    .000 0000 0000 0001 = Class: IN (0x0001)
    0... .... .... .... = Cache flush: False
    Time to live: 0
    Data length: 8
    TXT Length: 7
    TXT: foo=bar
z1._soap._tcp.local: type SRV, class IN, priority 0, weight 0, port 5012, target z1.soap.local
    Service: z1
    Protocol: _soap
    Name: _tcp.local
    Type: SRV (Server Selection) (33)
    .000 0000 0000 0001 = Class: IN (0x0001)
    0... .... .... .... = Cache flush: False
    Time to live: 0
    Data length: 16
    Priority: 0
    Weight: 0
    Port: 5012
    Target: z1.soap.local
_soap._tcp.local: type PTR, class IN, z1._soap._tcp.local
    Name: _soap._tcp.local
    Type: PTR (domain name PoinTeR) (12)
    .000 0000 0000 0001 = Class: IN (0x0001)
    0... .... .... .... = Cache flush: False
    Time to live: 0
    Data length: 2
    Domain Name: z1._soap._tcp.local
z1.soap.local: type A, class IN, addr 192.168.200.1
    Name: z1.soap.local
    Type: A (Host Address) (1)
    .000 0000 0000 0001 = Class: IN (0x0001)
    0... .... .... .... = Cache flush: False
    Time to live: 0
    Data length: 4
    Address: 192.168.200.1

However, I assume I'm doing several things wrong here. For one thing the service 'z1' doesn't dissapear in my client (cache flush?), for another thing I'm not sure if I'm creating my unsolicited answers the right way. Any help is welcome.

richardschneider commented 5 years ago

Thanks for spending your time on this!!!

Your Unadvertise seems correct to me.

My reading of the spec indicates that the cache flush bit should NOT be set on a good-bye record.

So the fault must be in "my client". Can you point me to the software.

Also, has this software been tested to deal with TTL 0. Note that section 10.1 delays pruning by 1 second.

richardschneider commented 5 years ago

I think that the good-bye record should NOT contain the _services._dns-sd._udp.local PTR

gilzad commented 5 years ago

Thanks for your support!

The client application I'm using is Apple's own DNSServiceBrowser.NET (needs a running instance of Bonjour on your machine). It's been uploaded to github, too.

DNSServiceBrowser.NET does successfully notice the removal of a service on goodbye (TTL 0).

You're right, I wasn't too sure about purging all services like _services._dns-sd._udp.local, I changed that among other things after comparing our goodbye-message to a working one from Bonjour.

public void Unadvertise()
{
    Message message = new Message();

    foreach (ServiceProfile profile in this.profiles)
    {
        foreach(ResourceRecord resourceRecord in profile.Resources)
        {
            resourceRecord.TTL = TimeSpan.Zero;
            message.Answers.Add(resourceRecord);
        }
    }

    this.Mdns.SendAnswer(message.CreateResponse());
    NameServer.Catalog.Clear();
}

I'm using message.CreateResponse() now. Earlier I was incorrectly sending queries instead of unsolicited answers.

But now the actual answer-records are missing in the answer-packet (should unadvertise z1._soap._tcp.local):

Frame 5: 54 bytes on wire (432 bits), 54 bytes captured (432 bits) on interface 0
Ethernet II, Src: HewlettP_7b:fb:a4 (00:24:81:7b:fb:a4), Dst: IPv4mcast_fb (01:00:5e:00:00:fb)
Internet Protocol Version 4, Src: 192.168.200.1, Dst: 224.0.0.251
User Datagram Protocol, Src Port: 5353, Dst Port: 5353
Multicast Domain Name System (response)
    Transaction ID: 0x0000
    Flags: 0x8400 Standard query response, No error
        1... .... .... .... = Response: Message is a response
        .000 0... .... .... = Opcode: Standard query (0)
        .... .1.. .... .... = Authoritative: Server is an authority for domain
        .... ..0. .... .... = Truncated: Message is not truncated
        .... ...0 .... .... = Recursion desired: Don't do query recursively
        .... .... 0... .... = Recursion available: Server can't do recursive queries
        .... .... .0.. .... = Z: reserved (0)
        .... .... ..0. .... = Answer authenticated: Answer/authority portion was not authenticated by the server
        .... .... ...0 .... = Non-authenticated data: Unacceptable
        .... .... .... 0000 = Reply code: No error (0)
    Questions: 0
    Answer RRs: 0
    Authority RRs: 0
    Additional RRs: 0
    [Unsolicited: True]

Here's a valid/working goodbye-message, unadvertising the service Service_1._soap._tcp.local:

Frame 13: 94 bytes on wire (752 bits), 94 bytes captured (752 bits) on interface 0
Ethernet II, Src: HewlettP_7b:fb:a4 (00:24:81:7b:fb:a4), Dst: IPv4mcast_fb (01:00:5e:00:00:fb)
Internet Protocol Version 4, Src: 192.168.200.1, Dst: 224.0.0.251
User Datagram Protocol, Src Port: 5353, Dst Port: 5353
Multicast Domain Name System (response)
    Transaction ID: 0x0000
    Flags: 0x8400 Standard query response, No error
        1... .... .... .... = Response: Message is a response
        .000 0... .... .... = Opcode: Standard query (0)
        .... .1.. .... .... = Authoritative: Server is an authority for domain
        .... ..0. .... .... = Truncated: Message is not truncated
        .... ...0 .... .... = Recursion desired: Don't do query recursively
        .... .... 0... .... = Recursion available: Server can't do recursive queries
        .... .... .0.. .... = Z: reserved (0)
        .... .... ..0. .... = Answer authenticated: Answer/authority portion was not authenticated by the server
        .... .... ...0 .... = Non-authenticated data: Unacceptable
        .... .... .... 0000 = Reply code: No error (0)
    Questions: 0
    Answer RRs: 1
    Authority RRs: 0
    Additional RRs: 0
    Answers
        _soap._tcp.local: type PTR, class IN, Service_1._soap._tcp.local
            Name: _soap._tcp.local
            Type: PTR (domain name PoinTeR) (12)
            .000 0000 0000 0001 = Class: IN (0x0001)
            0... .... .... .... = Cache flush: False
            Time to live: 0
            Data length: 12
            Domain Name: Service_1._soap._tcp.local
    [Unsolicited: True]

Is there a way I can create a response-message without loosing the answer-records?

gilzad commented 5 years ago

Ok, made some process.

public void Unadvertise()
{
    Message message = new Message().CreateResponse();//populate a response-msg with records.

    foreach (ServiceProfile profile in this.profiles)
    {
        foreach (ResourceRecord resourceRecord in profile.Resources)
        {
            resourceRecord.TTL = TimeSpan.Zero;
            message.Answers.Add(resourceRecord);
        }
    }

    this.Mdns.SendAnswer(message);
    NameServer.Catalog.Clear();
}

Of course it made no sense to create a resonse-message after populating a different instance. So having records inside a response-message does work now. Trying to do some more investigation before writing to not spam the thread here (sorry).

richardschneider commented 5 years ago

Great progress! the messsage documentation is in Makaretu.Dns https://richardschneider.github.io/net-dns/api/Makaretu.Dns.Message.html

You want var message = new Message { QR = true }

I suggest that you send one message per profile. Otherwise, the message length might be too big.

richardschneider commented 5 years ago

No worries about spam. It's good to see how others are understanding the package.

I just raised #57

gilzad commented 5 years ago

Thanks a bunch for the encouragement! I got it solved in a very simplified temporary solution now.

public void Unadvertise()
{
  Message message = new Message { QR = true };//create a response message

  foreach (ServiceProfile profile in this.profiles)
  {
    foreach (ResourceRecord resourceRecord in profile.Resources)
    {
      if (resourceRecord.Type == DnsType.SRV)//only consider announced Service-records
      {
        //Here's a dangerous hack: we strip 'instance' from 'instance._type._protocol._domain'
        //This won't work as soon as a hostname is added, e.g. 'MyPC._instance._type._protocol._domain'
        string name = resourceRecord.Name.Substring(resourceRecord.Name.IndexOf("._") + 1);
        //At least for Bonjour the goodbye-message needs to be sent as a pointer.
        PTRRecord pTRRecord = new PTRRecord { Name = name, DomainName = resourceRecord.Name };
        pTRRecord.TTL = TimeSpan.Zero;
        message.Answers.Add(pTRRecord);
      }
    }
  }
  this.Mdns.SendAnswer(message);
  NameServer.Catalog.Clear();
}

So, before I split this into ServiceDiscovery.UnadvertiseAsync(ServiceProfile) and ServiceDiscovery.Unadvertise(), I have to find a clean solution to extract _type._protocol._domain from instance._type._protocol._domain (maybe concat backwards).

And maybe I can find out how AVAHI behaves against this solution. It's also worth noting that pisker's solution does send goodbyes for Text, Pointer and Service.

But all in all, that's how I got this working with a Bonjour client. Here's an accepted goodbye-record (coming from net-mdns this time ;) ):

Frame 5: 87 bytes on wire (696 bits), 87 bytes captured (696 bits) on interface 0
Ethernet II, Src: HewlettP_7b:fb:a4 (00:24:81:7b:fb:a4), Dst: IPv4mcast_fb (01:00:5e:00:00:fb)
Internet Protocol Version 4, Src: 192.168.200.1, Dst: 224.0.0.251
User Datagram Protocol, Src Port: 5353, Dst Port: 5353
Multicast Domain Name System (response)
    Transaction ID: 0x0000
    Flags: 0x8400 Standard query response, No error
        1... .... .... .... = Response: Message is a response
        .000 0... .... .... = Opcode: Standard query (0)
        .... .1.. .... .... = Authoritative: Server is an authority for domain
        .... ..0. .... .... = Truncated: Message is not truncated
        .... ...0 .... .... = Recursion desired: Don't do query recursively
        .... .... 0... .... = Recursion available: Server can't do recursive queries
        .... .... .0.. .... = Z: reserved (0)
        .... .... ..0. .... = Answer authenticated: Answer/authority portion was not authenticated by the server
        .... .... ...0 .... = Non-authenticated data: Unacceptable
        .... .... .... 0000 = Reply code: No error (0)
    Questions: 0
    Answer RRs: 1
    Authority RRs: 0
    Additional RRs: 0
    Answers
        _soap._tcp.local: type PTR, class IN, z1._soap._tcp.local
            Name: _soap._tcp.local
            Type: PTR (domain name PoinTeR) (12)
            .000 0000 0000 0001 = Class: IN (0x0001)
            0... .... .... .... = Cache flush: False
            Time to live: 0
            Data length: 5
            Domain Name: z1._soap._tcp.local
    [Unsolicited: True]

richardschneider commented 5 years ago

Here are my thoughts on Unadvertise

it should send one good-bye message response per profile, so that the message length is not exceeded
each message should have all the resource records of the profile with TTL = 0, so that other's will remove it from it's cache
each message should have a PTR to the profile's service instance with TTL = 0, so that other's will remove it from it's cache
The above resources should be removed from NameServer Catalog, so that we will not respond to a query for the profile

The PTR to a service instance is

new PTRRecord 
{ 
    Name = profile.QualifiedServiceName, 
    DomainName = profile.service.FullyQualifiedName
};

It would be nice if the PTR record is in the message Answers and the other resources are in the message AdditionalRecords, so that if the message length is too big the PTR record is guaranteed to be sent. Please make sure that the Bonjour client accepts this; the spec is unclear on this point,

The last point on removing from Catalog, can be modifed. Since we are setting TTL to zero for all profile resources the NameServer will ignore them and eventually prune them. So just remove the service PTR record from the Catalog.

Catalog.TryRemove(profile.QualifiedServiceName, out Node _);

gilzad commented 5 years ago

I think fully understand. I'll take care of this and come up with a patch or a new 'ServiceDiscovery.cs' asap.

Regarding the removal from the NameServer: Are you saying I just have to remove the PTR by its service name instead of clearing the whole catalog?

gilzad commented 5 years ago

Ok, I've done the implementation as you suggested. Only sending a PTR per profile, adding the remaining records to the additional answers and only removing the PTR from the Nameserver by its QualifiedServiceName.

However, I have a little issue with your suggestion on ServiceDiscovery.UnadvertiseAsync(ServiceProfile). The way you're suggesting its head, it would automatically create and send a message with each invocation.

So I'd like to ask if you'd be okay with an architecture like this:

/// <summary>
/// Sends a goodbye message for the provided
/// profile and removes its pointer from the name sever.
/// </summary>
/// <param name="profile">The profile to send a goodbye message for.</param>
/// <param name="message">An instance of <see cref="Makaretu.Dns.Message"/> to populate with goodbye-messages and to send them with.</param>
/// <param name="suppressAnswer">True if the goodbye messages should be prepared only, false to send them implicitly.</param>
public void UnadvertiseAsync(ServiceProfile profile, Message message, bool suppressAnswer = false)
{
    PTRRecord pTRRecord = new PTRRecord { Name = profile.QualifiedServiceName, DomainName = profile.FullyQualifiedName };
    pTRRecord.TTL = TimeSpan.Zero;

    message.Answers.Add(pTRRecord);
    profile.Resources.ForEach(resource => message.AdditionalRecords.Add(resource));

    if (!suppressAnswer)
    {
        this.Mdns.SendAnswer(message);
    }

    NameServer.Catalog.TryRemove(profile.QualifiedServiceName, out Node _);
}

/// <summary>
/// Sends a goodbye message for each anounced service.
/// </summary>
public void Unadvertise()
{
    Message message = new Message { QR = true };
    foreach (ServiceProfile profile in this.profiles)
    {
        UnadvertiseAsync(profile, message);
    }
    this.Mdns.SendAnswer(message);
}

richardschneider commented 5 years ago

Yes, I want one message per profile.

We are now done with design issues and moving to implementation. So I suggest you now create a PR.

+1 for XML comments!!!

A few comments to speed things up

use var instead type name
do not use this., it is not needed
fix all warnings in the code
the TTL of the resources is NOT set to zero
UnadvertiseAsync is not async, remove Async suffix.

gilzad commented 5 years ago

Raised #59 .

Interesting thoughts on var and this.. I use

the type name instead of var to immediately grasp the type
this. to easily distinguish global variables from local ones.

But a consistent linting in an established project should be respected, of course. So I did the pull request with your preferred style.

I'm not sure what you're suggesting with

fix all warnings in the code

If it's the lack of respective tests, I need some further advice on this.

You were right about the TTL for the other resources and UnadvertiseAsync, I wasn't careful there. Fixed in the PR.

*edit: typo on 'var' vs 'type name'.

richardschneider commented 5 years ago

I'll create a new release, real soon,

richardschneider / net-mdns

Service Discovery goodbye message #47