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

Service Discovery goodbye message #47

Closed
cerna opened this issue Dec 15, 2018 · 20 comments
Closed

Service Discovery goodbye message #47

cerna opened this issue Dec 15, 2018 · 20 comments

Comments

@cerna
Copy link

cerna commented Dec 15, 2018

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
Copy link
Owner

richardschneider commented Dec 15, 2018

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
Copy link
Owner

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
Copy link
Author

cerna commented Dec 16, 2018

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
Copy link
Owner

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
Copy link
Author

cerna commented Dec 16, 2018

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
Copy link
Contributor

Has there been any progress on this issue?

@gilzad
Copy link
Contributor

gilzad commented Jun 4, 2019

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
Copy link
Owner

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
Copy link
Owner

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

@gilzad
Copy link
Contributor

gilzad commented Jun 5, 2019

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
Copy link
Contributor

gilzad commented Jun 5, 2019

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
Copy link
Owner

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
Copy link
Owner

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

I just raised #57

@gilzad
Copy link
Contributor

gilzad commented Jun 5, 2019

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
Copy link
Owner

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
Copy link
Contributor

gilzad commented Jun 6, 2019

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
Copy link
Contributor

gilzad commented Jun 6, 2019

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
Copy link
Owner

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
Copy link
Contributor

gilzad commented Jun 7, 2019

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
Copy link
Owner

I'll create a new release, real soon,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants