-
Notifications
You must be signed in to change notification settings - Fork 427
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
Preserve EDNS OPT record in a response if truncation occurs #1364
base: main
Are you sure you want to change the base?
Conversation
@@ -780,10 +797,10 @@ where | |||
let mut additional_count = count_was_truncated(additionals.emit(encoder))?; | |||
|
|||
if let Some(edns) = edns { | |||
encoder.set_max_size(real_encoder_max_size); | |||
// need to commit the error code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this comment mean btw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is referring to the fact that the response error code may need to be committed to the EDNS record... i.e. in EDNS the response (error code) is has high-order bits stored in the EDNS record that augment those in the original DNS Header. So I think that's what this may be talking about...
Codecov Report
@@ Coverage Diff @@
## main #1364 +/- ##
=======================================
Coverage 86.42% 86.42%
=======================================
Files 131 131
Lines 13766 13826 +60
=======================================
+ Hits 11897 11949 +52
- Misses 1869 1877 +8 |
97ad6da
to
15ecb3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like this change. See my one comment, maybe we don't need that in this PR, but it might be nice for us to add in a later one.
@@ -113,6 +113,18 @@ impl Edns { | |||
pub fn set_option(&mut self, option: EdnsOption) { | |||
self.options.insert(option); | |||
} | |||
|
|||
pub(crate) fn len(&self) -> u16 { | |||
1 // Name::root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should consider adding some consts for these things, or perhaps instead use std::mem::size_of
It would make the code a bit more self-documenting. Thoughts?
I know we have a similar practice elsewhere in the code, maybe I'll file a cleanup issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, could we use EncodedSize
for this?
Let's do this in another PR anyway
@vlad20012 please confirm that this is ready to land. If it is, either update the branch to current main, or confirm and I can update this branch. Thanks for this PR! |
15ecb3f
to
d65008d
Compare
d65008d
to
0e6682a
Compare
hm, it looks like this got approved and reviewed, and then we missed the fact that it was ready to go back then. Is this still worth doing? |
@vlad20012, do you think this is something worth updating and getting in? I think I missed when you updated this before... |
According to RFC 6891
Hence, we must preserve EDNS OPT record in an answer when truncation occurs