[mdns] add PublishKey() & UnpublishKey() methods

[abtink]

This commit adds new methods in Mdns::Publisher to publish or unpublish a key record for a given (host or service instance) name. New methods are implemented for both MDNSResponder and Avahi sub-classes.

In the MDNSResponder implementation, if a key registration is for a service instance name matching a service registration,
DNSServiceAddRecord() is used to associate the new record with the service. Otherwise, DNSServiceRegisterRecord() is used to register the KEY record on its own. The implementation handles cases when related service and key registrations are updated or unregistered.

This commit also simplifies and updates the test/mdns/main.cpp tests, adding a common Test() function that takes a function pointer to run the test and handles all common boilerplate code, as well as adding a common callback to check the registration result. New test cases are also added to check key registration, registering keys on their own, and registering keys and services in different orders.

---

• This PR contains the commit [mdns-mdnssd] simplify service and host registrations #2008
• Please check and review the last commit on this PR. Thanks.

[wgtdkp]

@abtink Why not carry the KEY record in the service or host struct?

[abtink]

@abtink Why not carry the KEY record in the service or host struct?

There are use-cases where we want to publish a KEY record without the host or service.

• When an SRP host/service lease is expired but its key-lease is not (this is to keep the claim on the host/service name during key-lease interval).
• When host has no valid address to publish but we need to claim the host name.

[superwhd]

LGTM 👍

[wgtdkp]

There are use-cases where we want to publish a KEY record without the host or service.

• When an SRP host/service lease is expired but its key-lease is not (this is to keep the claim on the host/service name during key-lease interval).
• When host has no valid address to publish but we need to claim the host name.

They can be eliminated by making A/AAAA and SRV records optional for a host and service registration.

Th cons I see with new PublishKey() API is that:

1. The client (i.e. Advertising Proxy) needs to take care of the relationship between a service/host and KEY registration.
2. more code in the mdnssd and avahi to suppor the new APIs.

Putting KEY record together with the service and host struct will eliminates the issues. This also follows the SRP definition that the KEY record is part of the Service Description Instruction.

We have separate PublishService and PublishHost APIs because there are two different DNS resource groups for service and host (similar to SRP have different Service and Host Description Instructions). This is not the case for the KEY record. For a service, it's just a group of DNS resource records which are keyed with the same DNS name. Like the TXT, SRV. I don't know a good reason why the KER RR is not in this service RR group.

[abtink]

Thanks @wgtdkp

I'm not sure your suggestion would make it simpler to implement and/or easier to use.

We need to support cases where we don't want to publish SRV/TXT records but want to publish KEY record (similarly for hosts with AAAA and KEY records).

They can be eliminated by making A/AAAA and SRV records optional for a host and service registration.

If I understand it correctly, your suggestion is to update the existing PublishService() and PublishHost() APIs so that we can instruct them to publish/unpublish certain sets of records. For example, we need a way to ask PublishService() to register SRV, TXT, and KEY records, or to publish just the KEY record and not SRV/TXT.

Similarly, we need to figure out how to Unpublish(). For example, after publishing SRV/TXT/KEY records for a service, we may want to unpublish all records, or just unpublish the SRV and TXT records and keep the KEY registration unchanged.

I feel that this model would be more complex to use. It is practically adding the PublishKey()/UnpublishKey() APIs indirectly inside the existing APIs, and I think it makes the overall logic harder to follow.

---

Th cons I see with new PublishKey() API is that:

1. The client (i.e. Advertising Proxy) needs to take care of the relationship between a service/host and KEY registration.
2. more code in the mdnssd and avahi to suppor the new APIs.

Putting KEY record together with the service and host struct will eliminates the issues.

I don't think the new model of updating existing APIs will eliminate these and may even make it more complex:

1. The client (e.g., advertising-proxy) still needs to manage this. Having separate APIs to publish/unpublish key make it easier for it to update records separately.

2. We still need code in mdnssd and avahi sub-classes to support the key registration whether it is added as a new API or we update the existing APIs.

   • mdns: The mdns implementation still needs to track and use different mDNSResponder APIs based on whether we have a service registered or not. This also requires tracking different types of RecordRef in ServiceRegistration objects, which makes it more complex.
   • avahi: I think the avahi implementation would become much more complex.
     • Specifically, we would need to track and use different AvahiGroup objects (one for the key and one for the other records) within the same Host/ServiceRegistration instances and manage the groups separately. Currently, we simply release/free the AvahiGroup object from the destructor of Host/ServiceRegistration which unregisters its records.
     • Handling of callbacks: We need to wait for separate callbacks (from avahi ) for service SRV/TXT and KEY registrations before we can report status and invoke the PublishService() callback. This makes the handling of callbacks more complex.

3. New API model make it more complex to handle duplicate calls (and different ways to interpret them): For example, how to behave if user calls PublishService() asking all SRV, TXT, KEY records to be registered and later calls PublishService() again (for the same service name) this time asking only KEY record to be registered (user is trying to keep key and unregister SRV/TXT)

   • Should we reject this request and instead require user to use UnpublishService() API, or handle it by checking the difference between new request and previous one and unregister SRV/TX and keep KEY unchanged (this will require new code to implement this behaviors).
   • Having separate PublishKey() API avoids such cases and makes the API user's intention is clear.

[wgtdkp]

avahi: I think the avahi implementation would become much more complex.

• Specifically, we would need to track and use different AvahiGroup objects (one for the key and one for the other records) within the same Host/ServiceRegistration instances and manage the groups separately. Currently, we simply release/free the AvahiGroup object from the destructor of Host/ServiceRegistration which unregisters its records.
• Handling of callbacks: We need to wait for separate callbacks (from avahi ) for service SRV/TXT and KEY registrations before we can report status and invoke the PublishService() callback. This makes the handling of callbacks more complex.

I think avahi is easier because we can use avahi_entry_group_add_record() to publish the KEY RR together with the host address or service in a single avahi_entry_group. I would suggest we always put all DNS RRs for a single name in the same avahi_entry_group to maintain the atomicity and simplicity. If we use separate "PublishKey" API, we won't be able to leverage this. From this perspective, avahi provides more reasonable/flexible API than mDNSResponder.

Should we reject this request and instead require user to use UnpublishService() API, or handle it by checking the difference between new request and previous one and unregister SRV/TX and keep KEY unchanged (this will require new code to implement this behaviors).

UnpublishService() should unpublish everything that have been published with the given name. So if you want to publish only the KEY RR, PublishService() should be called with null/empty TXT and SRV (or use boolean flags to indicate what to publish).

---

Even, I would be glad if we can change the mDNS APIs to have only:

class DnsResourceGroup {
public:
    explicit DnsResourceGroup(string dnsName);

    void addService(string hostname, int port, int weight, byte[] txt);

    void addAddress(byte[] address);

    void addKey(byte[] key);

    // Any other RRs associated with the same name...
}

PublishDnsResources(DnsResourceGroup resourceGroup);
UnpublishDnsResources(String dnsName);

This makes the API more similar to avahi which I think is more extensible, and it can further simplify the implementation.

[abtink]

@wgtdkp, I am not convinced that your suggestion is simpler or better.

We do want to be able to publish and unpublish keys for a name independently of host and service related records. This is used in adv-proxy where a service/host entry lease expires but its key lease does not.

Having separate APIs for Publish/UnpublishKey() makes this clear and simpler to use.

UnpublishService() should unpublish everything that have been published with the given name. So if you want to publish only the KEY RR, PublishService() should be called with null/empty TXT and SRV (or use boolean flags to indicate what to publish).

If user publishes a service and key, and later wants to unpublish the service without affecting the key registration (this is a common use case in adv-proxy when the lease expires), they must first unpublish everything and then remember to re-publish the key. This is inefficient and complex.

Your new suggested API model puts the responsibility (and burden) on user of the API to manage groups under same name and update records in a group. In my opinion, this should be managed by Publisher itself and it should itself group the related registrations (if it wants to).

Out goal should be to make the APIs easier to use, rather than maybe easier to implement.

---

We can separately discuss other ideas and possible redesign of Publisher and prototype them in other PRs (if they make sense). There are some aspects of the current design of Publisher that I think can be improved:

• Improve the distinction and responsibility of what the base class does and what it delegates to the sub-classes, and clarify how the base class expects the sub-classes to call its protected methods.
• For example, Publisher provides HandleDuplicate{Service/Host}Registration(), which sub-classes are expected to call from their own implementation of PublishService/Host().
• Another example: The base class defines the telemetry information and populates some related variables, but then expects sub-classes to update some other fields: mHost/ServiceRegistrationBeginTime are fully managed by the base class but m{Service/Host}InstanceResolutionBeginTime need to be updated by sub-classes.

[wgtdkp]

LGTM thanks @abtink !

[wgtdkp]

Why the name is accounted here?

[abtink]

I had to re-read the code to remember.

This is following the same pattern as existing IsOutDated() methods for HostRegisteration and ServiceRegisteration. So I did the same pattern to be consistent.

I agree none of them need to get a aName input and compare actually. It may make sense to change them all in separate PR to be consistent.

[wgtdkp]

Looks like this CL needs to be rebased. @abtink