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

[srp] implement AdvertisingProxy and define Dnssd platform APIs #9268

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

abtink
Copy link
Member

@abtink abtink commented Jul 10, 2023

This commit adds a generic SRP Advertising Proxy implementation to OpenThread core, which uses a set of newly defined otPlatDnssd platform APIs for DNS-SD (mDNS) support on infrastructure network on a Border Router.

Srp::Server provides ServiceUpdateHandler callback mechanism that allows platforms to implement their own advertising proxy function. While this is still supported, the new generic advertising proxy implementation makes it easier to port and support the proxy function on new platforms. The platform needs to provide the DNS-SD platform APIs, which are designed to be simple and easy to implement.

The AdvertisingProxy directly interacts with Srp::Server and its registered Host and Service entries, tracking whether an entry has been successfully advertised, is currently being advertised, or has been replaced by a new registration.

The AdvertisingProxy ensures that consecutive SRP updates for the same host or service are committed on the server in the order they are received, even if their advertisements are finished in a different order. This is important for SRP Replication support, as the server may receive a large number of SRP updates back-to-back for the same host.

The AdvertisingProxy will also register key records for SRP host and service instance names. This will keep the claim on the name of a removed entry while its key lease is not expired. It is also used when an SRP host registration has no off-mesh routable address.

This commit adds a detailed unit test test_srp_adv_proxy that validates the AdvertisingProxy under many scenarios. The test covers a range of cases, including delayed registration callbacks and timeouts, new registrations replacing outstanding advertisements, platform DNS-SD state changes and failures, host address changes adding/removing OMR addresses.


This PR currently contains the commit from #9208.

@size-report
Copy link

size-report bot commented Jul 10, 2023

Size Report of OpenThread

Merging #9268 into main(905a22e).

name branch text data bss total
ot-cli-ftd main 464768 760 66284 531812
#9268 464768 760 66284 531812
+/- 0 0 0 0
ot-ncp-ftd main 434860 760 61496 497116
#9268 434860 760 61496 497116
+/- 0 0 0 0
libopenthread-ftd.a main 234090 0 40230 274320
#9268 234090 0 40230 274320
+/- 0 0 0 0
libopenthread-cli-ftd.a main 56861 0 8075 64936
#9268 56861 0 8075 64936
+/- 0 0 0 0
libopenthread-ncp-ftd.a main 31855 0 5916 37771
#9268 31855 0 5916 37771
+/- 0 0 0 0
ot-cli-mtd main 363912 760 51180 415852
#9268 363912 760 51180 415852
+/- 0 0 0 0
ot-ncp-mtd main 346572 760 46408 393740
#9268 346572 760 46408 393740
+/- 0 0 0 0
libopenthread-mtd.a main 157460 0 25142 182602
#9268 157460 0 25142 182602
+/- 0 0 0 0
libopenthread-cli-mtd.a main 39720 0 8059 47779
#9268 39720 0 8059 47779
+/- 0 0 0 0
libopenthread-ncp-mtd.a main 24735 0 5916 30651
#9268 24735 0 5916 30651
+/- 0 0 0 0
ot-cli-ftd-br main 533152 768 130964 664884
#9268 533280 768 130964 665012
+/- +128 0 0 +128
libopenthread-ftd-br.a main 297075 5 104886 401966
#9268 297203 5 104886 402094
+/- +128 0 0 +128
libopenthread-cli-ftd-br.a main 70494 0 8099 78593
#9268 70494 0 8099 78593
+/- 0 0 0 0
ot-rcp main 62232 564 20604 83400
#9268 62232 564 20604 83400
+/- 0 0 0 0
libopenthread-rcp.a main 9534 0 5052 14586
#9268 9534 0 5052 14586
+/- 0 0 0 0
libopenthread-radio.a main 18815 0 214 19029
#9268 18815 0 214 19029
+/- 0 0 0 0

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Attention: 632 lines in your changes are missing coverage. Please review.

Comparison is base (905a22e) 79.75% compared to head (7bec257) 77.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9268      +/-   ##
==========================================
- Coverage   79.75%   77.19%   -2.57%     
==========================================
  Files         529      562      +33     
  Lines       73423    80392    +6969     
==========================================
+ Hits        58562    62062    +3500     
- Misses      14861    18330    +3469     
Files Coverage Δ
src/core/border_router/infra_if.cpp 90.90% <100.00%> (+47.83%) ⬆️
src/core/common/heap_data.hpp 100.00% <ø> (ø)
src/core/instance/instance.cpp 97.90% <ø> (-0.10%) ⬇️
src/core/net/srp_server.hpp 83.33% <100.00%> (-8.34%) ⬇️
tests/unit/test_platform.cpp 41.08% <ø> (+4.04%) ⬆️
tests/unit/test_srp_adv_proxy.cpp 100.00% <100.00%> (ø)
src/core/instance/instance.hpp 39.86% <66.66%> (-4.43%) ⬇️
src/core/net/dns_types.hpp 98.83% <0.00%> (-0.60%) ⬇️
src/core/net/dnssd.hpp 83.33% <83.33%> (ø)
src/core/net/srp_advertising_proxy.hpp 22.22% <22.22%> (ø)
... and 4 more

... and 356 files with indirect coverage changes

@wgtdkp
Copy link
Member

wgtdkp commented Jul 11, 2023

This commit adds a detailed unit test test_srp_adv_proxy that validates the AdvertisingProxy under many scenarios. The test covers a range of cases, including delayed registration callbacks and timeouts, new registrations replacing outstanding advertisements, platform DNS-SD state changes and failures.

Thanks @abtink ! It's really important but missing in OTBR!

include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Show resolved Hide resolved
@abtink abtink force-pushed the adv-proxy branch 3 times, most recently from dbf60ea to 2cc7254 Compare July 11, 2023 20:30
@superwhd superwhd self-requested a review July 13, 2023 02:19
@abtink abtink force-pushed the adv-proxy branch 3 times, most recently from 90cf618 to c29501e Compare July 21, 2023 00:59
@abtink
Copy link
Member Author

abtink commented Jul 26, 2023

The new commit makes large changes to the implementation:

  • It adds new otPlatDnssd APIs to register and unregister key records.
  • These APIs are used by AdvertisingProxy to keep the claim on the name of a removed host or service while its key lease is not expired.
  • They are also used when an SRP host registration has no off-mesh routable address (when the host and its services are un-advertised).
  • The unit test is updated to validate the key registration behavior.
  • A new detailed test case is added that covers host changing its addresses (adding/removing off-mesh routable address).
  • Another new test case is added that covers corner cases where an outstanding advertisement is removed before it is committed.
  • Common code patterns used in AdvertisingProxy are now defined as template methods, which are then used for Host or Service types (simplifying the code and removing repeated patterns).

@abtink
Copy link
Member Author

abtink commented Aug 24, 2023

In the new push, the implementation was updated to change how registrations are handled when the SRP client only includes mesh-local/link-local addresses. In the previous model, such services were not advertised on the AIL. In the new model, services are still advertised on the AIL, but without any addresses. This means that the service (SRV/TXT record) can be seen and queried on the AIL, but there will be no AAAA records for the hostname. This change is related to discussions in SPEC-1211. For now, I kept the new changes as a separate commit in this PR (will later squash them into one). (the new diff changes can seen in commit aa5cf05).

doc/ot_api_doc.h Outdated Show resolved Hide resolved
src/core/net/srp_advertising_proxy.hpp Outdated Show resolved Hide resolved
src/core/net/dns_types.hpp Outdated Show resolved Hide resolved
src/core/net/dns_types.hpp Outdated Show resolved Hide resolved
src/core/net/srp_advertising_proxy.cpp Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Show resolved Hide resolved
src/core/net/srp_advertising_proxy.cpp Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
src/core/net/dnssd.hpp Show resolved Hide resolved
src/core/net/dnssd.hpp Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
src/core/net/dnssd.hpp Show resolved Hide resolved
src/core/net/srp_advertising_proxy.cpp Show resolved Hide resolved
Copy link
Contributor

@superwhd superwhd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@wgtdkp
Copy link
Member

wgtdkp commented Jan 23, 2024

Hi @abtink , what's your plan on this PR as well as openthread/ot-br-posix#2042? I suppose you still want to check in the two changes, but both are conflicted now.

@abtink
Copy link
Member Author

abtink commented Jan 23, 2024

Hi @abtink , what's your plan on this PR as well as openthread/ot-br-posix#2042? I suppose you still want to check in the two changes, but both are conflicted now.

I'd like to wait till we have the new PR on mDNS in OT core up and then update this to work with new APIs defined by it.
We keep the option of platform providing mDNS but want to make sure the API definitions are aligned.

@abtink
Copy link
Member Author

abtink commented Jan 31, 2024

Rebased on main. We can move with final review and merge of this.

Copy link
Member

@wgtdkp wgtdkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks abtink 👍
This is really a great work!
Especially the comprehensive tests in tests/unit/test_srp_adv_proxy.cpp are what missing in ot-br-posix. I think we will deprecate the impl in ot-br-posix once this is merged.

Comment on lines +1107 to +1108
keyRecord.SetFlags(Dns::KeyRecord::kAuthConfidPermitted, Dns::KeyRecord::kOwnerNonZone,
Dns::KeyRecord::kSignatoryFlagGeneral);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read RFC3445 Section 3 and found it's violating the following statement.

In the flags field, all bits except bit 7 are reserved and MUST be
   zero.  If Bit 7 (Zone bit) is set to 1, then the KEY is a DNS Zone
   key.  If Bit 7 is set to 0, the KEY is not a zone key.  SIG(0)/TKEY
   are examples of DNSSEC keys that are not zone keys.

Here kSignatoryFlagGeneral is 1 at the 16th bit.

I wonder if there's a following RFC that allows setting 1s at bits other than the 7th bit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is following the same flags and format as in key RR used by SRP (e.g. in SRP client or checked in SRP server). Here we are just re-constructing the key RR that SRP sever got from client.

Regarding registering key on mDNS in adv-proxy, its purpose is to keep the claim to them name the record data is not really important. There were discussions of even allowing empty record data for it.

@jwhui jwhui requested a review from Vyrastas February 6, 2024 21:17
Copy link
Member

@Vyrastas Vyrastas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyedit nits for the API, everything else looks good

*
* @{
*
* The DNS-SD platform API are used only when `OPENTHREAD_CONFIG_PLATFORM_DNSSD_ENABLE` is enabled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are used only when --> is only available for use when

Copy link
Member Author

@abtink abtink Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I changed it to "APIs are used".

Since these are platform APIs, we expect platform to provide them and we (OT stack) will use them and we want to document under which build config these APIs are used by the OT stack.

include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Outdated Show resolved Hide resolved
include/openthread/platform/dnssd.h Show resolved Hide resolved
This commit adds a generic SRP Advertising Proxy implementation to
OpenThread core, which uses a set of newly defined `otPlatDnssd`
platform APIs for DNS-SD (mDNS) support on infrastructure network on
a Border Router.

`Srp::Server` provides `ServiceUpdateHandler` callback mechanism that
allows platforms to implement their own advertising proxy function.
While this is still supported, the new generic advertising proxy
implementation makes it easier to port and support the proxy function
on new platforms. The platform needs to provide the DNS-SD platform
APIs, which are designed to be simple and easy to implement.

The `AdvertisingProxy` directly interacts with `Srp::Server` and its
registered `Host` and `Service` entries, tracking whether an entry
has been successfully advertised, is currently being advertised, or
has been replaced by a new registration.

The `AdvertisingProxy` ensures that consecutive SRP updates for the
same host or service are committed on the server in the order they
are received, even if their advertisements are finished in a
different order. This is important for SRP Replication support, as
the server may receive a large number of SRP updates back-to-back for
the same host.

The `AdvertisingProxy` will also register key records for SRP host and
service instance names. This will keep the claim on the name of a
removed entry while its key lease is not expired. It is also used
when an SRP host registration has no off-mesh routable address.

This commit adds a detailed unit test `test_srp_adv_proxy` that
validates the `AdvertisingProxy` under many scenarios. The test
covers a range of cases, including delayed registration callbacks and
timeouts, new registrations replacing outstanding advertisements,
platform DNS-SD state changes and failures, host address changes
adding/removing OMR addresses.
@jwhui jwhui merged commit b212a0a into openthread:main Feb 9, 2024
100 checks passed
superwhd added a commit to superwhd/openthread that referenced this pull request Sep 2, 2024
Bug: 320820261

* github/main:
  [cli-tcat] define `VendorInfo` as a member variable (openthread#9869)
  [cli] style fixes & remove core defined error constants (openthread#9868)
  [ip6] add multicast forwarding restrictions to host (openthread#9863)
  [netdiag] update comments and fix style (openthread#9866)
  [cli] add references to CoAP concepts guide from CLI (openthread#9856)
  [netdiag] add support for Vendor App URL TLV (openthread#9816)
  [tools] add missing tcat ble client files (openthread#9857)
  [mle] generate challenge for "Child Update Request" early (openthread#9850)
  [mbedtls] make debug logging independent from builtin (openthread#9848)
  [cli] update coaps readme (openthread#9849)
  [slaac] implement address deprecation mechanism (openthread#9585)
  [cli] add Doxygen tags for coaps commands (openthread#9837)
  [instance] init `BackboneRouter::Local` before `Mle` (openthread#9843)
  [csl] reset data poll timer after successful csl transmission (openthread#9840)
  github-actions: bump actions/setup-python from 4.7.1 to 5.0.0 (openthread#9845)
  Bump grpcio from 1.53.0 to 1.53.2 in /tools/harness-simulation/harness (openthread#9842)
  [srp] implement `AdvertisingProxy` and define `Dnssd` platform APIs (openthread#9268)
  [tools] add tcat ble client (openthread#9739)
  [mbedtls] update to 2.28.7 (openthread#9835)
  [ip6] simplify `Ip6::SelectSourceAddress()` (openthread#9832)
  [cli] add Doxygen tags to CoAP commands (openthread#9821)
  [ip6] mesh-local addresses as preferred, platform control flag on host stack (openthread#9815)
  github-actions: bump step-security/harden-runner from 2.6.1 to 2.7.0 (openthread#9831)
  [ip6] `SelectSourceAddress()` rule 1 on preferring same address (openthread#9830)
  [posix] do not enable loop back to host (openthread#9826)
  [ip6] always deliver multicast to host (openthread#9824)
  [ncp] set message origin to host untrusted (openthread#9825)
  [trel] fix typo in `OPENTHREAD_CONFIG_TREL_PEER_TABLE_SIZE` (openthread#9823)
  [ci] fix artifact name conflict (openthread#9827)
  [routing-manager] fix `IsDeprecated()` to handle large preferred lifetime (openthread#9822)
  [common] add `ClearAllBytes()` template function (openthread#9818)
  [netdata-publisher] remove SRP unicast on anycast entry added by a BR (openthread#9807)
  [codecov] enable use of `CODECOV_TOKEN` (openthread#9819)
  [spinel] allow registering callback to restore vendor properties (openthread#9773)
  [key-manager] check if the key is present before exporting the key (openthread#9814)
  [cli] commissioner commands (openthread#9798)
  [spinel] fix set rx on when idle for RCP (openthread#9812)
  [net-diag] add Eui64Tlv support (openthread#9795)
  github-actions: bump github/codeql-action from 2.22.5 to 3.23.2 (openthread#9810)
  [multipan] Fix for multipan use case to handle tx timeout. (openthread#9781)
  [coap-secure] fix and test to validate max connection attempt limit (openthread#9803)
  [ip6] add method for creating/parsing IPv4-mapped IPv6 address (openthread#9799)
  [coap-secure] introduce maximum connection attempt limit for CoAP agent (openthread#9694)
  [border-agent] enable `otBorderAgentGetId` API when Border Agent is enabled (openthread#9794)
  [spinel] fix passing an unexpected transmit error to mac layer (openthread#9745)
  [docs] add TREL config group (openthread#9791)
  [cli] use `static constexpr` for constants instead of of `enum` (openthread#9789)
  [radio] add API for resetting CSL parameters (openthread#9772)
  github-actions: bump actions/upload-artifact from 3.1.3 to 4.2.0 (openthread#9788)
  [posix] use /dev/socket/ot-daemon for OT daemon socket (openthread#9776)
  [posix] fix netlink address deletion on posix (openthread#9779)
  [github-actions] remove use of geekyeggo/delete-artifact (openthread#9785)
  [net-diag] handle `MleCounterTlv` in `AppendDiagTlv()` (openthread#9777)
  [github-actions] add write permissions for geekyeggo/delete-artifact (openthread#9780)
  github-actions: bump geekyeggo/delete-artifact from 2.0.0 to 4.1.0 (openthread#9774)
  [github-actions] bump actions/upload-artifact and actions/download-artifact (openthread#9769)
  [tcat] define config defaults (openthread#9768)
  [posix] only add IPv4 route if NAT64 is enabled (openthread#9762)
  [srp] simplify crypto `ReadOrGenerateKey` in srp client (openthread#9764)
  [cli] add pd state (openthread#9757)
  [dtls] fix `OPENTHREAD_CONFIG_DTLS_ENABLE` check (openthread#9767)
  [test] move heap plat APIs in unit test under `extern "C"` block (openthread#9766)
  [srp] update config for auto-start mode to be enabled by default (openthread#9738)
  [ncp] differentiate DETACHED role and DISABLED role (openthread#9760)
  [docs] add Aqara to who supports list (openthread#9758)
  [cli] separate ping sender as a module (openthread#9756)
  [docs] fixed some common typos (openthread#9752)
  [telemetry] add API for TREL telemetry (openthread#9710)
  Bump pycryptodome from 3.17 to 3.19.1 in /tests/scripts/thread-cert (openthread#9755)
  [trel] add config for peer table size and packet pool size (openthread#9750)
  [cli] add Doxygen tags for history commands (openthread#9751)
  [telemetry] add upstream DNS metrics (openthread#9729)
  [dns] add `Name::ComapreMultipleLabels()` and update `Matches()` (openthread#9744)
  [dns] clarify trailing dot in `Name::ExtractLabels()` (openthread#9743)
  [heap-data] add `Matches()` to compare `Heap::Data` with a given buffer (openthread#9740)
  [posix] ignore required anycast address (openthread#9717)
  [cmake] always define `OPENTHREAD_FTD/MTD/RADIO` (openthread#9733)
  [dns] add `Name::Matches()` to compare DNS names (openthread#9734)
  [docs] create config variables list (openthread#9721)
  [factory-diags] fix usage of `OPENTHREAD_RADIO` (openthread#9736)
  [spinel] add spinel net role DISABLED (openthread#9731)
  [dns-types] add template variants of reading DNS names or labels (openthread#9720)
  [github-actions] avoid `dependabot` pull requests in forks (openthread#9728)
  [config] fix typos in config docs (openthread#9725)
  [spinel] fix `OPENTHREAD_ENABLE_SPINEL_VENDOR_HOOK` (openthread#9718)
  github-actions: bump docker/login-action from 2.2.0 to 3.0.0 (openthread#9723)
  [posix] prevent implicit conversion of `aUrl` to `RadioUrl` (openthread#9722)
  [posix] fix circular dependency on `OPENTHREAD_POSIX_CONFIG_DAEMON_ENABLE` (openthread#9719)
  [posix] fix usage of `__linux__` (openthread#9714)
  [docs] reference groups added and typos fixed (openthread#9709)
  [cmake] config posix config file runtime location (openthread#9701)
  [cmake] posix platform runtime configuration file (openthread#9701)
  [cmake] platform power calibration enable (openthread#9701)
  [posix] verify in `otPlatUdpBindToNetif` if Backbone Interface is set. (openthread#9696)
  [unit-test] make sure `otPlatLog()` is defined as `extern "C"` (openthread#9711)
  [routing-manager] advertise local OMR in RA after present in netdata (openthread#9703)
  [cli] add aliases for TCP and SRP concepts guides (openthread#9707)
  [routing-manager] deprecate OMR/on-mesh prefixes when removed from netdata (openthread#9599)
  github-actions: bump actions/setup-go from 4.1.0 to 5.0.0 (openthread#9706)
  [docs] SRP README update minor fix (openthread#9702)
  [cli] add `debug` command (openthread#9587)
  [github-actions] add arm-gcc-13 to build check (openthread#9695)
  [secure-transport] add `SetState()` and log state changes (openthread#9692)
  [mle] fix uninitialized `mLinkRequestAttempts` (openthread#9689)
  [tests] reduce flakiness of `test_publish_meshcop_service.py` (openthread#9690)
  [coap] remove duplicate method for setting `ConnectedCallback` (openthread#9691)
  [spinel] make `kTxWaitUs` configurable during compile time (openthread#9687)
  [secure-transport] rename `HandleUdpReceive` to `HandleReceive` (openthread#9686)
  [mesh-forwarder] finalize message direct tx on drop or eviction (openthread#9682)
  [meshcop-tlvs] simplify and enhance `ChannelMaskTlv` (openthread#9675)
  [tests] rename `get_netdata_nat64_prefix()` to `get_netdata_nat64_routes()` (openthread#9678)
  [posix] expose `platformInfraIfIsRunning()` as `otSysInfraIfIsRunning()` (openthread#9672)
  [telemetry] add api for DHCPv6 PD telemetry (openthread#9645)
  [crypto] remove PSA mbedtls hybrid setup of PBKDF2 function (openthread#9655)
  [crypto] add error handling to PBDF2 generate key function (openthread#9655)
  [tcat] initial commit of bluetooth-based commissioning (openthread#9210)
  [spinel] add vendor hook for the spinel host side (openthread#9560)
  github-actions: bump actions/setup-python from 4.7.0 to 4.7.1 (openthread#9677)
  [meshcop] `MeshCoP::ChannelTlv` to utilize `Mle::ChannelTlvValue` (openthread#9674)
  [srp-client] track registered addresses (openthread#9652)
  [cli] SRP Server Readme update - add srp server auto command (openthread#9669)

Change-Id: I6e9fbc4555569983cc8b8e3d1d03c2f2c82b6b9f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants