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.

Sign up for GitHub

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

Jump to bottom

refactor(mdns): Parse messages using trust-dns-proto instead of dns-parse #3102

Merged
merged 7 commits into from
Nov 14, 2022
Merged

refactor(mdns): Parse messages using trust-dns-proto instead of dns-parse #3102

merged 7 commits into from
Nov 14, 2022

Conversation

jxs
Copy link
Member

@jxs jxs commented Nov 9, 2022 •
edited by thomaseizinger
Loading

Description

While researching #3100 and #2676, I found that dns-parser the dependency we are using to parse the DNS messages only supports ASCII names, IDN's seem to support UTF8.

dns-parser looks unmaintained since 2018 and trust-dns supports UTF8, so while I am not sure this PR will solve the issues above mentioned, it will maybe help address the flakiness mentioned on #2676 (I myself have had cases where the mdns tests fail sometimes) since trust-dns is more popular and probably used. We will also stop having DEBUG logs for UTF8 DNS messages.

Links to any relevant issues

#3100 and #2676

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@jxs jxs force-pushed the replace-dns-parser-trust-dns-proto branch from b0b4ba9 to d5d0c9e Compare November 9, 2022 16:47
thomaseizinger
thomaseizinger approved these changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Ma233
Copy link

Ma233 commented Nov 14, 2022

Hi, is this pull request ready to be merged and released?

@thomaseizinger thomaseizinger changed the title mdns: replace dns-parse with trust-dns-proto refactor(mdns): Replace dns-parse with trust-dns-proto Nov 14, 2022
@thomaseizinger
Copy link
Contributor

Hi, is this pull request ready to be merged and released?

I think this is safe to merge, yes.

@thomaseizinger thomaseizinger changed the title refactor(mdns): Replace dns-parse with trust-dns-proto refactor(mdns): Parse messages using trust-dns-proto instead of dns-parse Nov 14, 2022
@mergify mergify bot merged commit 5f196dd into libp2p:master Nov 14, 2022
@jxs jxs mentioned this pull request Nov 14, 2022
Closed
@thomaseizinger
Copy link
Contributor

@jxs: This PR actually had a broken job: https://github.com/libp2p/rust-libp2p/actions/runs/3465658904/jobs/5788668836

@thomaseizinger thomaseizinger mentioned this pull request Nov 15, 2022
Merged
4 tasks
@thomaseizinger
Copy link
Contributor

@jxs: This PR actually had a broken job: libp2p/rust-libp2p/actions/runs/3465658904/jobs/5788668836

I opened an issue here: https://github.com/bluejekyll/trust-dns/issues/1830

@thomaseizinger
Copy link
Contributor

I'd like to avoid dragging in the entirety of tokio in so I am tempted to revert this to get a clean master branch back.

We can then patch trust-dns-proto, wait for their next release and bring this change back.

Thoughts?

@mxinden
Copy link
Member

mxinden commented Nov 15, 2022

Reverting works for me. Thanks for fixing this upstream @jxs.

@jxs
Copy link
Member Author

jxs commented Nov 15, 2022

@jxs: This PR actually had a broken job: libp2p/rust-libp2p/actions/runs/3465658904/jobs/5788668836

I opened an issue here: bluejekyll/trust-dns#1830

Thanks Thomas, yeah makes sense to revert. Meanwhile I submitted https://github.com/bluejekyll/trust-dns/pull/1831.

@thomaseizinger
Copy link
Contributor

@jxs: This PR actually had a broken job: libp2p/rust-libp2p/actions/runs/3465658904/jobs/5788668836

I opened an issue here: bluejekyll/trust-dns#1830

Thanks Thomas, yeah makes sense to revert. Meanwhile I submitted https://github.com/bluejekyll/trust-dns/pull/1831.

Thanks for submitting the PR!

thomaseizinger added a commit that referenced this pull request Nov 17, 2022
@thomaseizinger
Revert "refactor(mdns): Parse messages using trust-dns-proto instea…
1023edc 
…d of `dns-parse` (#3102)"

This reverts commit 5f196dd.
@jxs jxs mentioned this pull request Aug 31, 2023
Merged
mergify bot pushed a commit that referenced this pull request Sep 4, 2023
@jxs
deps: bump trust-dns-proto from 0.22.0 to 0.23.0
50b8b63 
On #3102 we left the `tokio-runtime` feature because of https://github.com/bluejekyll/trust-dns/issues/1830, meanwhile https://github.com/bluejekyll/trust-dns/pull/1831 was merged and `0.23.0` was released with it, so we no longer require the `tokio-runtime` feature.

Pull-Request: #4418.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jxs @Ma233 @thomaseizinger @mxinden