-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Docker support in OSQuery. #3241
Conversation
O.o, wow! |
#include <boost/algorithm/string/split.hpp> | ||
#include <boost/asio.hpp> | ||
#include <boost/foreach.hpp> | ||
#include <cstdlib> |
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, Is it possible to move the C++ includes above the boost
? And include a \n\n
between the two categories.
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.
@theopolis Thanks for the quick review. Will update with changes.
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.
Done
#include "osquery/core/json.h" | ||
|
||
namespace pt = boost::property_tree; | ||
using boost::asio::local::stream_protocol; |
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.
Do you mind aliasing this instead of bringing it into local scope?
using stream_protocol = boost::asio::local::stream_protocol;
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.
Changed to use "local" alias
* docker domain is configured to use a different path specify that path. | ||
*/ | ||
FLAG(string, | ||
docker_sock_path, |
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.
Solid! How about docker_socket
?
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.
Changed
Row r; | ||
r["id"] = tree.get<std::string>("ID", ""); | ||
r["containers"] = INTEGER(tree.get<int>("Containers", 0)); | ||
r["container_running"] = INTEGER(tree.get<int>("ContainersRunning", 0)); |
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.
Should these (running, paused, stopped) be containers_running
, etc?
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. Fixed all three
} | ||
|
||
for (const auto& entry : tree) { | ||
try { |
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.
Do we need to wrap this in a try
?
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.
Nope. Removed
|
||
for (const auto& entry : tree) { | ||
try { | ||
const pt::ptree node = entry.second; |
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.
Can you grab const&
?
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.
Done
const pt::ptree node = entry.second; | ||
Row r; | ||
r["id"] = getValue(node, ids, "Id"); | ||
for (const auto& name : node.get_child("Names")) { |
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.
If you .count("Names")
you can check for existence.
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.
Am assuming you want me to check for count("Names") > 0 before iterating over the names?
*/ | ||
QueryData genContainerLabels(QueryContext& context) { | ||
return getLabels(context, | ||
std::string("container"), |
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.
You can provide "const strings" here, drop the std::string()
for readability.
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.
Done
if (!key_str.empty()) { | ||
key_str.append("%2C"); // comma | ||
} | ||
key_str.append("%22").append(item).append("%22%3Atrue"); // "item":true |
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 seems scary, can I inject arbitrary content into the API call with a where clause?
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 added utility method to validate sha256 hash/prefix.
@spasam updated the pull request - view changes |
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 also added sample output from the tables to the pull request summary. If you have time, take a peek. Thanks
https://github.com/facebook/osquery/files/971156/examples.txt
#include <boost/algorithm/string/split.hpp> | ||
#include <boost/asio.hpp> | ||
#include <boost/foreach.hpp> | ||
#include <cstdlib> |
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.
Done
#include "osquery/core/json.h" | ||
|
||
namespace pt = boost::property_tree; | ||
using boost::asio::local::stream_protocol; |
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.
Changed to use "local" alias
* docker domain is configured to use a different path specify that path. | ||
*/ | ||
FLAG(string, | ||
docker_sock_path, |
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.
Changed
Row r; | ||
r["id"] = tree.get<std::string>("ID", ""); | ||
r["containers"] = INTEGER(tree.get<int>("Containers", 0)); | ||
r["container_running"] = INTEGER(tree.get<int>("ContainersRunning", 0)); |
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. Fixed all three
if (!key_str.empty()) { | ||
key_str.append("%2C"); // comma | ||
} | ||
key_str.append("%22").append(item).append("%22%3Atrue"); // "item":true |
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 added utility method to validate sha256 hash/prefix.
} | ||
|
||
for (const auto& entry : tree) { | ||
try { |
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.
Nope. Removed
|
||
for (const auto& entry : tree) { | ||
try { | ||
const pt::ptree node = entry.second; |
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.
Done
const pt::ptree node = entry.second; | ||
Row r; | ||
r["id"] = getValue(node, ids, "Id"); | ||
for (const auto& name : node.get_child("Names")) { |
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.
Am assuming you want me to check for count("Names") > 0 before iterating over the names?
*/ | ||
QueryData genContainerLabels(QueryContext& context) { | ||
return getLabels(context, | ||
std::string("container"), |
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.
Done
@spasam updated the pull request - view changes |
I took the liberty of fixing few "make docs" errors in unrelated files. |
Looking good! Will deep dive later today and pull/test locally, one observation before I can provide a complete review, is it possible to mimic the processes table in docker_processes? |
@theopolis Thanks. Docker processes output is based on the query string arguments. Docker daemon runs "ps" with provided arguments and returns the results. I tried to be as close to "processes" as possible. I might be missing few things. Let me look again.
|
@spasam updated the pull request - view changes |
ok to test |
@theopolis Thanks for triggering tests. Anything I have to do to resolve these failures. Looks like workspace is not clean!
|
Yeah, looks like our previous break-fix was actually a timebomb-bug in disguise. This should fix it: #3244 will need to rebase when that lands. |
That was fast. Just saw the other pull request. Thanks! |
@spasam updated the pull request - view changes |
Looks like this is now pulling in some unrelated commits. Can you squash all of your commits and remove the previously-landed commits from this branch? |
Yeah, sorry. I did a merge from upstream and pushed. Not sure what the work flow was to trigger the builds again with your build fix. Will squash!. |
UNIX domain socket provided by docker daemon is used to make API calls. Docker Engine API (v1.27) is used as reference. No support is added for docker swarm related APIs. This should work on all platforms where docker UNIX domain socket is exposed. Supports following top level tables: - docker_info - docker_version - docker_containers - docker_container_labels - docker_container_mounts - docker_container_networks - docker_container_ports - docker_images - docker_image_labels - docker_networks - docker_network_labels - docker_volumes - docker_volume_labels Following tables require WHERE clause with container ID: - docker_container_processes - docker_container_stats docker_container_processes almost resembles process table with the following exceptions: docker_container_processes does not have following columns - path - cwd - root - on_disk - user_time - system_time docker_container_processes has these additional columns: - id (docker container id) - user - time - cpu - mem Unrelated to docker changes, I fixed few documentation errors for "make docs" to pass.
@spasam updated the pull request - view changes |
Reverted unrelated commits and squashed docker commits. Updated commit message. |
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 💃
@theopolis Need anything else for me? |
UNIX domain socket provided by docker daemon is used to make API calls.
Docker Engine API (v1.27) is used as reference. No support is added for
docker swarm related APIs. This should work on all platforms where
docker UNIX domain socket is exposed.
Supports following top level tables:
Examples