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

Add wireformat buffer to DnsResponse #1855

Merged
merged 12 commits into from Jan 5, 2023

Conversation

mattias-p
Copy link
Contributor

@mattias-p mattias-p commented Dec 7, 2022

This PR adds a wireformat buffer to the DnsResponse struct, allowing the caller to know the exact bytes that were received in a given response.

This is my second attempt at achieving the same goal. During the review of my first attempt (#1823) @djc suggested an alternative approach. We refined his idea a little and agreed that it seemed like a better way. This is an implementation of that idea.

Fixes #1814.

Copy link
Collaborator

@djc djc 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 pretty good to me!

crates/resolver/src/error.rs Outdated Show resolved Hide resolved
@djc djc requested a review from bluejekyll December 19, 2022 09:14
@mattias-p
Copy link
Contributor Author

I rebased this PR for two reasons:

  • CI gave me some new clippy errors unrelated to these changes, and a clippy fix landed on main.
  • I had a conflict in crates/resolver/src/error.rs.

When resolving the conflict I also integrated the two clean up commits.

I also realized that <trust_dns_client::rr::dnssec::tsig::TSigner as MessageFinalizer>::finalize_message() was creating a DnsResponse from a buffer without including the buffer in the DnsResponse, so I added a new commit for that.

@mattias-p mattias-p marked this pull request as ready for review December 22, 2022 21:51
@mattias-p
Copy link
Contributor Author

For a while I was worried that it would be confusing for people to know if the buffer is supposed to be Some or None. But it should be intuitive enough, right? I.e.:

  • When you're sending a response it's None.
  • When you're receiving a response it's Some.
  • When you call DnsResponse::new() yourself you know what you did.

I tried to come up with ways to make the API foolproof but I was unable to come up with something that carries its own weight.

@mattias-p
Copy link
Contributor Author

I had a closer look at the circumstances in which DnsResponses are constructed. In production code a buffer i always available, and in test code a buffer is seldom (never?) available. But in test code we could afford to serialize a buffer from the Message just so we can construct a DnsResponse, right? This way we can make the DnsResponse buffer into a plain Vec<u8> without the Option, and it always contains something that was received. From my point of view this is a much nicer API.

In the test code there are lots of places where a Message is constructed and a DnsResponse is built from that. I thought it was best to keep that constructor for that. But to avoid accidentally creating DnsResponses with generated buffers I thought it best to not use a special method instead of From.

The QuicStream::receive() method used to return a Message but I changed it to return DnsResponse instead to give the caller access to the buffer.

I have a changeset implementing the above but the commits are in a mess so I need to clean them up before I push them.

@mattias-p
Copy link
Contributor Author

I updated DnsResponse so that its buffer isn't wrapped in an Option but rather is always present.

To mitigate the risk of callers unintentionally synthesizing buffers for DnsResponse I replaced the impl From<Message> for DnsResponse with DnsResponse::from_message(). I'm not confident this is the best choice for placement and naming of this method but it is what I came up with. If you have a better suggestion I'll happily perform the search/replace.

@djc
Copy link
Collaborator

djc commented Dec 27, 2022

To mitigate the risk of callers unintentionally synthesizing buffers for DnsResponse I replaced the impl From<Message> for DnsResponse with DnsResponse::from_message(). I'm not confident this is the best choice for placement and naming of this method but it is what I came up with. If you have a better suggestion I'll happily perform the search/replace.

If this is only used in test code as you suggested earlier, we could guard the from_message() method with #[test]? I guess we might still want to expose it as a convenience to downstream crates?

@mattias-p
Copy link
Contributor Author

If this is only used in test code as you suggested earlier, we could guard the from_message() method with #[test]? I guess we might still want to expose it as a convenience to downstream crates?

It's already used by test code downstream in the client, resolver and server crates as well as in the integration-tests. I tried marking it with #[cfg(test)] but the other crates can't access it then, even when running their own tests.

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.

I like how this is narrow to just the DnsResponse type. Looks good.

@bluejekyll bluejekyll merged commit 5298d11 into hickory-dns:main Jan 5, 2023
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.

Getting the response wireformat
3 participants