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

DoH3 support #1987

Merged
merged 13 commits into from
Oct 6, 2023
Merged

DoH3 support #1987

merged 13 commits into from
Oct 6, 2023

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jul 7, 2023 •
edited
Loading

This adds DNS-over-HTTP/3 support.

I'm not familiar with all the other TrustDNS crates, so I'm not too sure whats going on there. I made sure that DoH3 was added to every crate that was also using DoQ.

Requires #2036.
Fixes #1939.

@daxpedda daxpedda force-pushed the http3 branch 2 times, most recently from 83e4489 to 203e3a8 Compare July 7, 2023 20:54
@daxpedda
Copy link
Contributor Author

daxpedda commented Jul 14, 2023 •
edited
Loading

Apparently the h3's implementation of grease isn't compatible with Cloudflare's, will write up a bug report.

This was referenced Jul 14, 2023
Merged
Open
@daxpedda daxpedda force-pushed the http3 branch 2 times, most recently from 94156ec to 4c3aa4d Compare July 15, 2023 14:27
@daxpedda
Copy link
Contributor Author

daxpedda commented Jul 21, 2023 •
edited
Loading

Currently blocked until we figure out https://github.com/bluejekyll/trust-dns/issues/1990.

For anybody interested, I already got it working locally with Google and Cloudflare. Proto already contains unit tests that succeed on CI right now.

@daxpedda daxpedda mentioned this pull request Sep 22, 2023
Closed
@daxpedda daxpedda force-pushed the http3 branch 4 times, most recently from a6e5b62 to f8591f4 Compare September 23, 2023 15:31
@daxpedda daxpedda closed this Sep 23, 2023
@daxpedda daxpedda reopened this Sep 23, 2023
@daxpedda daxpedda marked this pull request as ready for review September 23, 2023 17:17
@daxpedda
Copy link
Contributor Author

This is ready for review.

@daxpedda daxpedda force-pushed the http3 branch 2 times, most recently from 18ea7a7 to adb1310 Compare September 29, 2023 12:40
@daxpedda
Copy link
Contributor Author

Rebased on #2036.

@bluejekyll
Copy link
Member

I'm planning to review this today, it looks like you feel it's ready at this point?

@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 30, 2023 •
edited
Loading

I'm planning to review this today, it looks like you feel it's ready at this point?

Yes, this is ready to be reviewed!
I'm gonna keep rebasing it on main so it's also remains in a mergeable state as well.

bluejekyll
bluejekyll reviewed Oct 3, 2023
View reviewed changes
Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Overall, this looks really good, and was clearly a lot of work. Thank you! really awesome to see h3 support landing. I left a few comments that I hope you don't mind discussing?

I wanted to review this earlier, but didn't have time until now. I hope that's not too much of a bother.

bin/Cargo.toml Outdated Show resolved Hide resolved
crates/proto/src/h3/h3_client_stream.rs
H3ClientConnect(Box::pin(self.connect(name_server, dns_name)) as _)
}

/// Creates a new H3Stream with existing connection
Copy link
Member

Choose a reason for hiding this comment

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

at some point we should add some examples for this use case.

crates/proto/src/h3/h3_client_stream.rs Show resolved Hide resolved
crates/proto/src/h3/h3_server.rs Show resolved Hide resolved
crates/proto/src/https/https_client_stream.rs Outdated Show resolved Hide resolved
crates/resolver/src/h3.rs Show resolved Hide resolved
crates/server/src/server/h3_handler.rs Show resolved Hide resolved
crates/server/src/server/h3_handler.rs Show resolved Hide resolved
@daxpedda daxpedda mentioned this pull request Oct 4, 2023
Open
@daxpedda daxpedda requested a review from bluejekyll October 4, 2023 11:30
@bluejekyll bluejekyll mentioned this pull request Oct 4, 2023
Open
@daxpedda
Copy link
Contributor Author

daxpedda commented Oct 5, 2023

Rebased and ready for review again.

bluejekyll
bluejekyll approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for all the changes and all the work! This was significant!

@bluejekyll bluejekyll merged commit a846914 into hickory-dns:main Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluejekyll bluejekyll bluejekyll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DNS over HTTP/3 (DoH3)
2 participants
@daxpedda @bluejekyll