Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upDocker support in OSQuery. #3241
Conversation
|
O.o, wow! |
| #include <boost/algorithm/string/split.hpp> | ||
| #include <boost/asio.hpp> | ||
| #include <boost/foreach.hpp> | ||
| #include <cstdlib> |
theopolis
May 2, 2017
Member
nit, Is it possible to move the C++ includes above the boost? And include a \n\n between the two categories.
nit, Is it possible to move the C++ includes above the boost? And include a \n\n between the two categories.
spasam
May 2, 2017
Author
Contributor
Done
Done
| #include "osquery/core/json.h" | ||
|
|
||
| namespace pt = boost::property_tree; | ||
| using boost::asio::local::stream_protocol; |
theopolis
May 2, 2017
Member
Do you mind aliasing this instead of bringing it into local scope?
using stream_protocol = boost::asio::local::stream_protocol;
Do you mind aliasing this instead of bringing it into local scope?
using stream_protocol = boost::asio::local::stream_protocol;
spasam
May 2, 2017
Author
Contributor
Changed to use "local" alias
Changed to use "local" alias
| * docker domain is configured to use a different path specify that path. | ||
| */ | ||
| FLAG(string, | ||
| docker_sock_path, |
theopolis
May 2, 2017
Member
Solid! How about docker_socket?
Solid! How about docker_socket?
spasam
May 2, 2017
Author
Contributor
Changed
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)); |
theopolis
May 2, 2017
Member
Should these (running, paused, stopped) be containers_running, etc?
Should these (running, paused, stopped) be containers_running, etc?
spasam
May 2, 2017
Author
Contributor
Good catch. Fixed all three
Good catch. Fixed all three
| } | ||
|
|
||
| for (const auto& entry : tree) { | ||
| try { |
theopolis
May 2, 2017
Member
Do we need to wrap this in a try?
Do we need to wrap this in a try?
spasam
May 2, 2017
Author
Contributor
Nope. Removed
Nope. Removed
|
|
||
| for (const auto& entry : tree) { | ||
| try { | ||
| const pt::ptree node = entry.second; |
theopolis
May 2, 2017
Member
Can you grab const&?
Can you grab const&?
spasam
May 2, 2017
Author
Contributor
Done
Done
| const pt::ptree node = entry.second; | ||
| Row r; | ||
| r["id"] = getValue(node, ids, "Id"); | ||
| for (const auto& name : node.get_child("Names")) { |
theopolis
May 2, 2017
Member
If you .count("Names") you can check for existence.
If you .count("Names") you can check for existence.
spasam
May 2, 2017
Author
Contributor
Am assuming you want me to check for count("Names") > 0 before iterating over the names?
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"), |
theopolis
May 2, 2017
Member
You can provide "const strings" here, drop the std::string() for readability.
You can provide "const strings" here, drop the std::string() for readability.
spasam
May 2, 2017
Author
Contributor
Done
Done
| if (!key_str.empty()) { | ||
| key_str.append("%2C"); // comma | ||
| } | ||
| key_str.append("%22").append(item).append("%22%3Atrue"); // "item":true |
theopolis
May 2, 2017
Member
This seems scary, can I inject arbitrary content into the API call with a where clause?
This seems scary, can I inject arbitrary content into the API call with a where clause?
spasam
May 2, 2017
Author
Contributor
I added utility method to validate sha256 hash/prefix.
I added utility method to validate sha256 hash/prefix.
|
@spasam updated the pull request - view changes |
|
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> |
spasam
May 2, 2017
Author
Contributor
Done
Done
| #include "osquery/core/json.h" | ||
|
|
||
| namespace pt = boost::property_tree; | ||
| using boost::asio::local::stream_protocol; |
spasam
May 2, 2017
Author
Contributor
Changed to use "local" alias
Changed to use "local" alias
| * docker domain is configured to use a different path specify that path. | ||
| */ | ||
| FLAG(string, | ||
| docker_sock_path, |
spasam
May 2, 2017
Author
Contributor
Changed
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)); |
spasam
May 2, 2017
Author
Contributor
Good catch. Fixed all three
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 |
spasam
May 2, 2017
Author
Contributor
I added utility method to validate sha256 hash/prefix.
I added utility method to validate sha256 hash/prefix.
| } | ||
|
|
||
| for (const auto& entry : tree) { | ||
| try { |
spasam
May 2, 2017
Author
Contributor
Nope. Removed
Nope. Removed
|
|
||
| for (const auto& entry : tree) { | ||
| try { | ||
| const pt::ptree node = entry.second; |
spasam
May 2, 2017
Author
Contributor
Done
Done
| const pt::ptree node = entry.second; | ||
| Row r; | ||
| r["id"] = getValue(node, ids, "Id"); | ||
| for (const auto& name : node.get_child("Names")) { |
spasam
May 2, 2017
Author
Contributor
Am assuming you want me to check for count("Names") > 0 before iterating over the names?
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"), |
spasam
May 2, 2017
Author
Contributor
Done
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. |
|
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