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

mDNS: always set the "cache flush" bit #154

Merged
merged 6 commits into from
Jul 26, 2024

Conversation

lucasvr
Copy link
Contributor

@lucasvr lucasvr commented Apr 17, 2024

Always broadcast mDNS messages with the "cache flush" bit set for TXT, SRV, A, and AAAA records. This is useful when the Matter device reboots and gets assigned a different IP address, for example. In such a case, hosts with an outdated cache may need to wait until the original TTL expires (up to 60 seconds, per the TTL definitions on mdns/builtin.rs).

lucasvr added 2 commits April 17, 2024 15:18
Always broadcast mDNS messages with the "cache flush" bit set for
TXT, SRV, A, and AAAA records. This is useful when the Matter device
reboots and gets assigned a different IP address, for example. In
such a case, hosts with an outdated cache may need to wait until the
original TTL expires (up to 60 seconds, per the TTL definitions on
mdns/builtin.rs).
@ivmarkov
Copy link
Contributor

ivmarkov commented Jun 12, 2024

UPDATE: GH links to lines in PR do not work as I expected. I'll update with inline-comments

@lucasvr Sorry for replying so late, but after implementing and submitting this, I think I'm finally somewhat qualified to assess what you are doing here.

Yes, my interpretation matches yours now. Setting the cache-flush bits unconditionally might help (or at least not hurt) for all but those PTR records where we are not the authority (as in https://datatracker.ietf.org/doc/html/rfc6762#section-10.2).

One question though: I think we should also cache-flush those PTR answers where we redirect ("point") a service-type or a service subtype to its host record?

However, in your PR, I see two things which worry me a bit:

  • Contrary to what the PR title says, you are cache-flushing also PTR records. Is this by accident or by design?
  • You are also cache-flushing PTR records where we are not the authority (i.e. "what DNS-SD service-types and sub-types you have?") (I'll comment on these lines in the PR itself shortly.)
    My interpretation of the spec is that we should definitely not cache-flush non-authority answers as I interpret you are doing in this PR. Specifically where the spec says:
    "
    The cache-flush bit MUST NOT ever be set in any shared resource
    record. To do so would cause all the other shared versions of this
    resource record with different rdata from different responders to be
    immediately deleted from all the caches on the network.
    "

Please let me know if I misunderstood your changes, or the spec. In any case, once #186 is in, I plan to apply a variation of the changes you have suggested here. Thanks again for noticing this and sorry for the late reply! I should've at least left a note why I'm not replying immediately (as in - not being qualified enough to comment).

Copy link
Contributor

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

As per my other comment: #154 (comment)

@lucasvr
Copy link
Contributor Author

lucasvr commented Jun 19, 2024

  • Contrary to what the PR title says, you are cache-flushing also PTR records. Is this by accident or by design?
  • You are also cache-flushing PTR records where we are not the authority (i.e. "what DNS-SD service-types and sub-types you have?") (I'll comment on these lines in the PR itself shortly.)

That was not by design. While it wouldn't hurt to set the cache-flush bit on those cases, it's indeed more sane to leave them out from PTR records. Thanks for the careful code review.

I've updated the PR to address your suggestions. Feel free to use this as a reference only (in case you want to apply a variation of this PR at a later point in time).

@andy31415 andy31415 merged commit d03c5f3 into project-chip:main Jul 26, 2024
12 checks passed
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.

4 participants