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
Make dnsdist dynamic truncate do right thing on TCP/IP #5647
Make dnsdist dynamic truncate do right thing on TCP/IP #5647
Conversation
Winfried noted that our new dynamic truncation rule worked fine on UDP, but on TCP/IP a truncate would be converted into a drop, which was not the intended effect. This commit makes dynamic truncate a NOOP on TCP.
Note: there is already a test that should have caught this bug. However, that test did not function it appears. Still investigating. |
test_DynBlocks.py:TestDynBlockQPSActionTruncated.testDynBlocksQRate |
… dynamic truncation
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.
2 nits, 2 questions. Test and code look as advertised in the PR
dq.dh->qr = true; | ||
return true; | ||
} | ||
else { |
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.
nit: Personally, I am not a fan superfluous else
statements. As there is already a return in the body of the if
.
} | ||
else { | ||
vinfolog("Query from %s for %s over TCP *not* truncated because of dynamic block", dq.remote->toStringWithPort(), dq.qname->toString()); | ||
} | ||
} | ||
else { |
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.
Shouldn't this block also do
g_stats.dynBlocked++;
got->blocks++;
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.
See above.
else { | ||
vinfolog("Query from %s for %s over TCP *not* truncated because of dynamic block", dq.remote->toStringWithPort(), dq.qname->toString()); | ||
} | ||
|
||
} | ||
else { |
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.
Shouldn't this block also do
g_stats.dynBlocked++;
got->blocks++;
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.
No, it precisely should not do that - on TCP "truncate" does nothing.
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.
This is in the 'dropped' block
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.
good catch, redoing.
dq.dh->qr = true; | ||
return true; | ||
} | ||
else { |
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.
nit: Personally, I am not a fan superfluous else
statements. As there is already a return in the body of the if
.
…the updating to a function
Winfried noted that our new dynamic truncation rule worked fine on UDP, but on TCP/IP a truncate would be converted into a drop, which was not the intended effect.
This commit makes dynamic truncate a NOOP on TCP.
Short description
Checklist
I have: