-
Notifications
You must be signed in to change notification settings - Fork 908
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
rec: Refactor Protobuf options, add query/response selection #6698
Conversation
pdns/pdns_recursor.cc
Outdated
@@ -1713,7 +1716,7 @@ static void handleRunningTCPQuestion(int fd, FDMultiplexer::funcparam_t& var) | |||
try { | |||
const struct dnsheader* dh = (const struct dnsheader*) conn->data; | |||
|
|||
if (!luaconfsLocal->protobufTaggedOnly) { | |||
if (logQuery && (!luaconfsLocal->protobufExportConfig.taggedOnly || !dc->d_policyTags.empty())) { |
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 feel writing it like this makes it easier to read:
if (logQuery && !(luaconfsLocal->protobufExportConfig.taggedOnly && dc->d_policyTags.empty())) {
...
}
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.
LGTM
* ``asyncConnect``: bool - When set to false (default) the first connection to the server during startup will block up to ``timeout`` seconds, otherwise the connection is done in a separate thread, after the first message has been queued | ||
* ``logQueries=true``: bool - Whether to export queries | ||
* ``logResponses=true``: bool - Whether to export responses | ||
|
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.
Maybe adding a reference to setProtobufMasks()
here would be great.
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.
Just a curiosity, is there an option (would it be useful) to anonymize IPs using ipcipher
?
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.
Maybe adding a reference to
setProtobufMasks()
here would be great.
Done!
Just a curiosity, is there an option (would it be useful) to anonymize IPs using
ipcipher
?
There isn't. While I totally agree it would be nice to have it, it's probably out of scope for this PR.
pdns/rec-lua-conf.cc
Outdated
try { | ||
ComboAddress server(server_); | ||
lci.protobufExportConfig.server = ComboAddress(server_); |
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 will change protobuf server's IP address no matter it has already been enabled or not. I think it should be moved after line 365 (if (!lci.protobufExportConfig.enabled) {
).
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.
Nice catch, thanks!
- Turn the protobuf options into a table because there are already too many of them - Split the masks applied to the initiator to a separate `setProtobufMasks` directive - Add the possibility to log only queries, or only responses - Add the possibility to select queries (FFI only) and responses for export from the Lua hooks - Add regression tests for the protobuf features
a33b0a5
to
8fcd94b
Compare
Short description
setProtobufMasks
directiveCloses #6361.
Checklist
I have: