-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix and document discovery queries behavior on distributed queries and add tests #7655
Fix and document discovery queries behavior on distributed queries and add tests #7655
Conversation
|
osquery/distributed/distributed.cpp
Outdated
@@ -229,7 +233,7 @@ Status Distributed::acceptWork(const std::string& work) { | |||
return Status(1, "Distributed query is not a string"); | |||
} | |||
|
|||
if (queries_to_run.empty() || queries_to_run.count(name)) { | |||
if (!hasDiscoveryQueries || queries_to_run.count(name)) { |
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'm not sure I understand how this fixes the bug described in #5260. How is this behavior different that prior? (I'm not enough of a c person to discern if there's something clever happening)
Are there cases hasDiscoveryQueries is false, but not !queries_to_run.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.
Hi!, it's a pretty confusing feature/logic.
Currently in master
:
queries_to_run
starts empty.- First
for
loop iterates the receiveddiscovery
queries. For each discovery query, if they return > 0 results, then the queryname
is added toqueries_to_run
. - Second
for
loop iterates the receivedqueries
and, "stores them in the database to be run in runQueries" ifqueries_to_run
is empty or if the correspondingdiscovery
query returned > 0 results.
Are there cases hasDiscoveryQueries is false, but not !queries_to_run.empty()?
The bug in master
is that if there are discovery queries and all of them return no results, then queries_to_run
will be empty, and therefore all queries are incorrectly enqueued for execution (because it's using queries_to_run.empty()
in the if
).
if (!hasDiscoveryQueries || queries_to_run.count(name)) {
I believe this change fixes the above bug:
- All queries are queued if there are no
discovery
queries (hasDiscoveryQueries=false
). - If
hasDiscoveryQueries=true
, then the query is only enqueued if it returned resultsqueries_to_run.count(name) == true
.
Let me know if it makes sense.
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.
Also, this PR just fixes a corner-case in the current logic, it does not try to change such logic (discussed in #5260 (comment)).
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've ended up fixing the "odd behavior" and adding documentation instead. Let me know if (by reading docs and tests) it now makes more sense.
89a06da
to
7387f4b
Compare
ad6452a
to
dad93c0
Compare
dad93c0
to
6d74278
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.
This looks to be well-documented and tested. Thank you @lucasmrod! @directionless any further questions/concerns from you? Otherwise I'll merge later today.
This PR fixes and attempts to document the discovery queries behavior in distributed queries.
The main change:
If a distributed query does not have its corresponding discovery query, then it will always be executed.
(Currently on
master
this is not the case, if there's at least one discovery query then all those distributed queries that do not have one defined are not executed at all.)This PR also fixes #5260 and #7793 and adds some tests to
Distributed.acceptWork
.