Skip to content

Export TXTEntry so callers can set TXT records on advertised services#277

Open
mcuelenaere wants to merge 1 commit into
pion:mainfrom
mcuelenaere:claude/nostalgic-fermat-8e8a71
Open

Export TXTEntry so callers can set TXT records on advertised services#277
mcuelenaere wants to merge 1 commit into
pion:mainfrom
mcuelenaere:claude/nostalgic-fermat-8e8a71

Conversation

@mcuelenaere
Copy link
Copy Markdown

Summary

The DNS-SD server support (#253) lets callers advertise a service with WithService, but ServiceInstance.Text used the unexported txtKeyValue element type — so external callers had no exported type, constructor, or setter to populate TXT records. The server already serialized Text and Browse() already decoded into it; the only missing piece was the public API to construct entries.

This renames txtKeyValue → the exported TXTEntry and adds two ergonomic constructors that hide the nil-vs-empty Value footgun.

Before

// Impossible from outside the package — txtKeyValue is unexported.
mdns.WithService(mdns.ServiceInstance{
    Instance: "My Web Server",
    Service:  "_http._tcp",
    Port:     8080,
    // Text: ??? no exported element type
})

After

mdns.WithService(mdns.ServiceInstance{
    Instance: "My Web Server",
    Service:  "_http._tcp",
    Port:     8080,
    Text: []mdns.TXTEntry{
        mdns.NewTXTEntry("version", "1.2.3"), // "version=1.2.3"
        mdns.NewTXTEntry("setup", ""),         // "setup="
        mdns.NewTXTFlag("secure"),             // "secure" (boolean attr)
    },
})

Backward compatibility

Fully backward-compatible: txtKeyValue was never exported, so no external caller could reference it. Existing validation (non-empty key, 255-byte per-string limit, case-insensitive dedup keeping first) is unchanged.

Tests

  • TestNewTXTEntry / TestNewTXTFlag — constructor semantics, incl. explicit empty value (key=) vs boolean flag (nil → bare key)
  • TestNewTXTConstructors_Encode — entries → wire strings → decode round-trip
  • TestCreateServiceAnswerTXTWithEntries — server emits the configured TXT entries in its TXT answer
  • e2e testReverse now advertises a TXT entry + flag against avahi-browse

Refs #253

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.62%. Comparing base (2575a81) to head (a54bccf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
+ Coverage   80.57%   80.62%   +0.04%     
==========================================
  Files           7        7              
  Lines        1596     1600       +4     
==========================================
+ Hits         1286     1290       +4     
  Misses        201      201              
  Partials      109      109              
Flag Coverage Δ
go 80.62% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mcuelenaere mcuelenaere force-pushed the claude/nostalgic-fermat-8e8a71 branch 2 times, most recently from 691e4cb to a54bccf Compare May 29, 2026 13:28
mcuelenaere added a commit to mcuelenaere/kvm that referenced this pull request May 29, 2026
JetKVM devices now advertise themselves as the DNS-SD service type
`_jetkvm._tcp` on the local network, so macOS/iOS clients can discover
every device on the LAN via
NWBrowser(for: .bonjour(type: "_jetkvm._tcp", domain: nil)), and
`dns-sd -B _jetkvm._tcp` / `avahi-browse -r _jetkvm._tcp` work too.

The advertised record carries:
  - instance: the device hostname (e.g. jetkvm-abc123)
  - host:     <hostname>.local (existing A/AAAA resolution preserved)
  - port:     80, or 443 when TLS is enabled
  - TXT:      version=<fw>, id=<deviceId>, setup=<true|false>

Implementation uses pion/mdns/v2 (already in the tree transitively via
pion/webrtc). pion takes the IPv4 and IPv6 multicast packet conns
separately, so the existing MDNSMode=ipv4_only / ipv6_only config is
honored by simply not binding the disabled family — no custom
responder, no second mDNS library. We switch from the legacy Server()
(A/AAAA only) to NewServer() so PTR/SRV/TXT are answered too.

Lifecycle:
  - starts on the first networkStateChanged once the network is up
  - refreshes after device setup completes so the `setup` TXT flips
  - refreshes after a TLS mode change so the advertised port follows
  - Stop() closes the sockets on shutdown

NOTE: DNS-SD TXT publication needs an exported TXTEntry API that is not
yet in a tagged pion/mdns release; go.mod pins pion/mdns/v2 to a fork
branch via `replace` until pion/mdns#277 merges and ships. This PR
stays in draft until then.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The DNS-SD server support (pion#253) lets callers advertise a service via
WithService, but ServiceInstance.Text used the unexported txtKeyValue
element type, so external callers had no way to populate TXT records.

Rename txtKeyValue to the exported TXTEntry and add NewTXTEntry and
NewTXTFlag constructors to hide the nil-vs-empty Value distinction.
The renamed type was never exported, so this is backward-compatible.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@mcuelenaere mcuelenaere force-pushed the claude/nostalgic-fermat-8e8a71 branch from a54bccf to 7ee4387 Compare May 29, 2026 16:02
Copy link
Copy Markdown
Member

@backkem backkem left a comment

Choose a reason for hiding this comment

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

nits:

  1. NewTXTEntry/NewTXTFlag don't validate keys (empty, contains =, non-ASCII). Validation only fires at encode time and even then only checks empty key + length. Pre-existing gap, but constructors are the natural place to add it.
  2. NewTXTEntry only accepts string values. RFC 6763 §6.5 allows opaque binary. Callers can still use TXTEntry{Key: "k", Value: bytes} directly, but worth a godoc note.
  3. testReverse adds TXT entries but never asserts on them in the avahi-browse response. The e2e change is cosmetic as-is.

I think only the 1st one is worth considering changing before we merge because it would require changing the signature of NewTXTEntry/NewTXTFlag to add in a follow-up. What do others think?

@jwetzell
Copy link
Copy Markdown

jwetzell commented Jun 3, 2026

Something else worth noting about text entries (that is kind of a pre-existing problem) is that keys are not case sensitive. Not sure how this should play out in practice since right now this is just a slice so we don't have any way to force this behavior until encode time.

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

Successfully merging this pull request may close these issues.

3 participants