Conversation
|
@gnosek It's not a problem, that PR is not supposed to be merged |
adalton
left a comment
adalton
left a comment
There was a problem hiding this comment.
This is awesome -- thank you!
To summarize my comments around the docker engine, it'd be nice if we can hide some of the protected/private things that are platform dependent to the appropriate platform-dependent file. I think that'll simply the public header a great deal (both in terms of includes/forward declarations, as well as in macro guards around features and platforms). Since the file is for only that class, scoping it at that level is equivalent to making it a static class member.
// Linux implementation file
...
namespace
{
#if defined(HAS_CAPTURE)
CURLM* m_curlm;
CURL* m_curl;
#endif
bool s_query_image_info;
size_t curl_write_cb(...)
{
...
}
inline bool parse_containerd_mounts(...)
{
...
}
...
} // end namespace
// implementation of the class.
| @@ -0,0 +1,280 @@ | |||
| /* | |||
There was a problem hiding this comment.
I recommend that we name name the file the same as the class, here sinsp_container_engine_docker.cpp (with an optional _win suffix for Windows-only content).
It'll make it easier when we get to the point that we need a symbol and expect to have it in a file with the same name rather than a layer of indirection to find it. Hopefully it'll also dissuade folks from dumping other content in the same file.
There was a problem hiding this comment.
I have mixed feelings about this as the class name is pretty long and basically everything in the directory will be called sinsp_something. I like the idea of class<->file matching but I don't feel putting sinsp_ in front of everything will help much ;)
Maybe rename all the engines to sinsp::container_engine::docker etc. and move them into libsinsp/container_engine/docker.cpp and so on?
There was a problem hiding this comment.
Sorry for the delay in getting back to you. That sounds great to me.
There was a problem hiding this comment.
I just thought of something... we seem to include every directory in the include search path, so if you name this docker.{h,cpp} then you'll run into a name conflict with sysdig/userspace/libsinsp/docker.h.
(I really don't think we should include every directory in the include search path, but that's a different story.)
Also, there exists a class named sinsp, so I don't think we can get away with a namespace with the same name.
Maybe make this libsinsp::container_engine_docker in container_engine_docker.{h,cpp}?
| static void set_query_image_info(bool query_image_info); | ||
| static void parse_json_mounts(const Json::Value &mnt_obj, std::vector<sinsp_container_info::container_mount_info> &mounts); | ||
|
|
||
| protected: |
There was a problem hiding this comment.
I think (hope?) you can make this private
|
|
||
| static bool m_query_image_info; | ||
| #if !defined(CYGWING_AGENT) && defined(HAS_CAPTURE) | ||
| static CURLM *m_curlm; |
There was a problem hiding this comment.
Since you've separated things out, these don't really need to be static members of the class, they can be static (or better yet, members of an anonymous namespace) in the implementation file.
There was a problem hiding this comment.
Does it being protected hurt anything? I can see us e.g. subclassing the engine in some tests. We never subclass the engines in production code so this should be effectively a no-op change.
There was a problem hiding this comment.
A couple of points here:
- Ideally, tests should need access to the private state, so (again ideally) we shouldn't need a test to subclass this in order to get at this stuff.
- If we really can't manage to test this using its public API, then I'd suggest just making a test class (not subclass) and explicitly making this a friend of that test class.
That said, if there are existing tests for this class that use the approach that you describe, then leaving it protected is ok.
|
|
||
| protected: | ||
| #if !defined(CYGWING_AGENT) && defined(HAS_CAPTURE) | ||
| static size_t curl_write_callback(const char* ptr, size_t size, size_t nmemb, std::string* json); |
There was a problem hiding this comment.
Similar to the comment about the static members, all these static functions I suspect could just be stand-alone functions in an anonymous namespace in the implementation file. I think doing this will simplify the includes in this file too.
There was a problem hiding this comment.
It's not going to be pretty with configurable values (socket path, grpc timeout) but I'll move what I can to the cpp file. BTW it feels like !HAS_CAPTURE should have its own impl file as well.
userspace/libsinsp/container_lxc.h
Outdated
| bool resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info); | ||
| }; | ||
|
|
||
| class sinsp_container_engine_libvirt_lxc |
There was a problem hiding this comment.
I suggest we put this in its own file too
userspace/libsinsp/container_mesos.h
Outdated
| public: | ||
| bool resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info); | ||
| static bool set_mesos_task_id(sinsp_container_info* container, sinsp_threadinfo* tinfo); | ||
| protected: |
There was a problem hiding this comment.
Can this be private? Same comment in other classes.
There was a problem hiding this comment.
See other comment (tl;dr: I'm not sure it gives us anything)
There was a problem hiding this comment.
As a general rule, I always prefer exposing the minimum possible privilege. I agree that if there are no production subclasses, then private and protected are functionally equivalent today, but who knows what tomorrow might bring :)
gnosek
force-pushed
the
cri
branch
2 times, most recently
from
eba037c to
c544861
Compare
|
@adalton, please have another look. I deferred splitting the lxc engines until we decide about the name changes but I think I addressed everything else I could. |
adalton
left a comment
adalton
left a comment
There was a problem hiding this comment.
New changes look good to me.
| public: | ||
| bool resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info); | ||
| static bool set_mesos_task_id(sinsp_container_info* container, sinsp_threadinfo* tinfo); | ||
| protected: |
There was a problem hiding this comment.
As a general rule, I always prefer exposing the minimum possible privilege. I agree that if there are no production subclasses, then private and protected are functionally equivalent today, but who knows what tomorrow might bring :)
gnosek
force-pushed
the
cri
branch
2 times, most recently
from
121e221 to
4954b66
Compare
| @@ -0,0 +1,61 @@ | |||
| /* | |||
| Copyright (C) 2013-2018 Draios Inc dba Sysdig. | |||
There was a problem hiding this comment.
Nit: My understanding is we're now (within the last week or two) officially "Sysdig".
There was a problem hiding this comment.
Oh neat, I didn't know that! If we do change it to Sysdig, we should tackle that separately and change all the headers in one step.
|
|
||
| bool resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info); | ||
| static void cleanup(); | ||
| static void set_query_image_info(bool query_image_info); |
There was a problem hiding this comment.
I don't see an implementation for set_query_image_info() or parse_json_mounts()
There was a problem hiding this comment.
they're in userspace/libsinsp/container_docker_common.cpp:
set_query_image_info(): https://github.com/draios/sysdig/pull/1277/files#diff-7c1ac911c0444c40b5263133df8f03e1R43
parse_json_mounts(): https://github.com/draios/sysdig/pull/1277/files#diff-7c1ac911c0444c40b5263133df8f03e1R29
|
|
||
| std::string sinsp_container_engine_docker::build_request(const std::string &url) | ||
| { | ||
| return "http://localhost" + m_api_version + url; |
There was a problem hiding this comment.
You might want to consider passing a port to the constructor of this thing, defaulting the value to 80, and add it here. That'll make it easier to unit test if, for example, we spin up some "fake dockerd" to send pre-canned responses.
|
|
||
| bool parse_cri_mounts(const runtime::v1alpha2::ContainerStatus &status, sinsp_container_info *container) | ||
| { | ||
| for (const auto& mount : status.mounts()) |
There was a problem hiding this comment.
Nit: I think that if I were you, I'd do this (or some variation) locally:
using runtime::v1alpha2::MountPropagation;
and then remove the runtime::v1alpha bits from the case statements below.
|
|
||
| sinsp_container_engine_docker::docker_response sinsp_container_engine_docker::get_docker(sinsp_container_manager* manager, const string& url, string &json) | ||
| { | ||
| #ifdef HAS_CAPTURE |
There was a problem hiding this comment.
Nit: You might consider this to avoid the #else, but what you currently have is also fine.
{
docker_response response = docker_response::RESP_ERROR;
#if defined(HAS_CAPTURE) // or ifdef if you prefer
....
response = docker_response::RESP_OK; // This wouldn't be necessary -- it would be unreachable
#endif
return response;
}
| return total; | ||
| } | ||
|
|
||
| bool parse_cri_image(const runtime::v1alpha2::ContainerStatus &status, sinsp_container_info *container) |
There was a problem hiding this comment.
I wonder if it might make sense to put this CRI stuff in a separate component (maybe a namespace?), and make this a client of that component? The main motivation for this is that I think making it a separate component would make it easier to unit test -- you could test it directly via it's public API instead of having to nudge this client in the right ways to get it to hit this code.
| return "http://localhost" + m_api_version + url; | ||
| } | ||
|
|
||
| bool parse_cri(sinsp_container_manager* manager, sinsp_container_info *container, sinsp_threadinfo* tinfo) |
There was a problem hiding this comment.
Looks like this should go in your anonymous namespace
| #endif | ||
| } | ||
|
|
||
| std::string sinsp_container_engine_docker::build_request(const std::string &url) |
There was a problem hiding this comment.
Nit: I think this also could be a function in the anonymous namespace, you'd just have to pass a reference to the api version
There was a problem hiding this comment.
We define it in _win/_linux implementation files but call it from _common, so it would have to be declared in a header anyway. I don't think making a function from the method would buy us much.
gnosek
force-pushed
the
cri
branch
3 times, most recently
from
b24a65b to
b5fdf52
Compare
mstemm
left a comment
mstemm
left a comment
There was a problem hiding this comment.
I focused mainly on checking if the important container properties used by falco rules/security policies are set for crio containers, and it looks good to me.
I did recently add support for container health checks when parsing containers (https://github.com/draios/agent/pull/1139), which probably caused the merge conflicts. Let me know if you'd like me to help out adding support for health checks for crio containers.
anoop-sysd
changed the title
gnosek
force-pushed
the
cri
branch
2 times, most recently
from
59c0a32 to
ef7d7f3
Compare
adalton
left a comment
adalton
left a comment
There was a problem hiding this comment.
All remaining comments are nits -- nothing blocking the check-in
| @@ -0,0 +1,176 @@ | |||
| /* | |||
| Copyright (C) 2013-2018 Draios Inc dba Sysdig. | |||
There was a problem hiding this comment.
Nit: Since this is new, I'd make this just be 2019. Ditto for other new files with new content.
There was a problem hiding this comment.
Good catch. Should we have a "Happy new year!" commit for all the other files?
| return true; | ||
| } | ||
|
|
||
| bool parse_cri_env(const Json::Value &info, sinsp_container_info *container) |
There was a problem hiding this comment.
the container env is not being used anywhere except when looking for SYSDIG_AGENT_CONF. may be just limit this function to find this specific env only?
There was a problem hiding this comment.
I'd rather do this as a separate PR that limits Docker environments as well, is it OK?
There's nothing generic about this method, it's 100% Docker-specific
When there's no Docker socket, libcurl returns HTTP code zero, so let's not treat this as a fatal error in debug mode (preparing for CRI support).
CRI exposes a gRPC interface so we get native objects, not JSON::Values
The socket is currently hardcoded and the interface is blocking, but we do successfully get data from containerd over the CRI protocol.
Docker is a big chunk of the code itself so will be done in a separate commit or two
We don't need to rediscover we're using an ancient Docker version with every request.
After making the gRPC stub a file-level variable, they don't need to be class members at all.
We cannot distinguish them by process cgroups, so override the type once the CRI runtime responds with container metadata
Note: this won't work without further changes as now Docker will take over all matching containers, even if there's no metadata for them.
When `--cri` is passed on the command line and `--docker-then-cri` is _not_, don't try to get metadata from Docker at all. With `--docker-then-cri`, first try to get metadata from Docker, and when that fails, contact CRI.
Long-only options were parsed incorrectly whenever there was a short option passed after a long-only one (the long-only handler code was called twice, which was a big issue if it took an argument, since it would now be passed the short option's argument). Fix this by modelling the option parsing code after csysdig (long options are explicitly handled only when getopt_long returns 0). This required a fix (with some minor code duplication) for handling `--list-chisels` as the previous code handled long-only options in the main `getopt_long()` switch in a pretty convoluted way.
After connecting to CRI, we query it for the runtime name. If it's containerd, we switch the container type reported to CT_CONTAINERD.
| // host/image@sha256:digest | ||
| // sha256:digest | ||
| const auto &image_ref = status.image_ref(); | ||
| auto digest_start = image_ref.find("sha256:"); |
There was a problem hiding this comment.
I know it's a corner case, but wouldn't this also match images like myregistry/image_name_that_ends_with_sha256:mytag?
Or since tags never end up in image_ref() you're sure that there's only 1 occurrence of :?
| port, | ||
| container->m_imagerepo, | ||
| container->m_imagetag, | ||
| digest, |
There was a problem hiding this comment.
Should we think about cases in which the digest will be reported in status.image().image() but not in status.image_ref()? If so, in those cases digest should be then assigned to container->m_imagedigest.
5 participants
This PR adds support for talking the (gRPC-based) CRI protocol to get container metadata.
It's now out for review and testing but it's not ready to be merged yet. Apart from any issues to be found, the CRI socket is currently hardcoded and all containers are reported as Docker ones.
While I was here, I split container.cpp into per-engine files (it was hard to read even before I touched it).
@mattpag, this greatly conflicts with your async Docker work but we do have a plan to introduce async container metadata in a more general way.