Parse addl docker container image info #1127

mattpag · 2018-05-22T13:09:28Z

Parse and store three new container_info fields: repository, tag and digest.
More information on the changes in the first commit message.

alfred-landrum · 2018-05-29T17:04:51Z

userspace/libsinsp/container.cpp

@@ -763,6 +815,82 @@ bool sinsp_container_manager::parse_docker(sinsp_container_info* container)
 		container->m_imageid = imgstr.substr(cpos + 1);
 	}

+	auto split_container_image = [](const string &image, string &repo, string &tag, string &digest)


@mattpag : how about breaking this out to a separate function & adding some unit tests to verify the parsing? The test strings will also be a useful way to document what we expect.

sure, will do

mstemm

Ok, all done reviewing. Take a look at the comments and let me know what you think.

mstemm · 2018-06-07T16:01:32Z

userspace/libsinsp/container.cpp

-	bool qdres = wh_query_docker(m_inspector->get_wmi_handle(), 
-		(char*)message.c_str(), 
+	bool qdres = wh_query_docker(m_inspector->get_wmi_handle(),
+		(char*)message.c_str(),


the variable message doesn't exist any longer, so I don't think this will compile, will it? Maybe you want to keep old line 696 around.

Actually, this seems to conflict with other uses of get_docker() where you're trying to pass full urls. So I guess url needs to be converted to message.

The parameter url of parse_docker is already the right thing to pass to wh_query_docker, I just missed to change message to url on line 740. Will do.

mstemm · 2018-06-07T17:32:40Z

userspace/libsinsp/container.cpp

 	return sinsp_docker_response::RESP_OK;
 }

 bool sinsp_container_manager::parse_docker(sinsp_container_info* container)
 {
 	string json;
-        sinsp_docker_response resp = get_docker("/v1.24", container->m_id, json);
+#ifndef CYGWING_AGENT
+	sinsp_docker_response resp = get_docker("http://dummy" + m_api_version + "/containers/" + container->m_id + "/json", json);


You might want to use localhost instead of dummy, just in case some future version of curl tries to do validation of the hostname part.

I agree, will do

mstemm · 2018-06-07T17:34:18Z

userspace/libsinsp/container.cpp

 	Json::Value root;
 	Json::Reader reader;
-	bool parsingSuccessful = reader.parse(json.substr(pos), root);
+	bool parsingSuccessful = reader.parse(json, root);


I guess this searching for the '{' is no longer needed, as you're expecting libcurl to properly return errors? Is it still not required on windows?

I'm not sure I understand what you mean.
For windows, I'm doing it directly inside get_docker.
When using libcurl is not required because json will already contain only the response json, not the full HTTP response as before, or will fail with errors.

Ok, I see. Sounds good.

mstemm · 2018-06-07T17:37:43Z

userspace/libsinsp/container.cpp

+		if(get_docker("GET /v1.30/images/" + container->m_imageid + "/json?digests=1 HTTP/1.1\r\nHost: docker \r\n\r\n", img_json) == sinsp_docker_response::RESP_OK)
+#endif
+		{
+			size_t img_pos = img_json.find("{");


Given that we removed a similar search for '{' above, can we also remove it here?

Yes I missed to remove it after a refactoring, it's not needed

mstemm · 2018-06-07T17:38:36Z

userspace/libsinsp/container.cpp

+					   container->m_imagetag,
+					   container->m_imagedigest);
+
+	if(container->m_imagedigest.empty())


Just double checking--it's not possible to combine this call to get_docker with the one at the beginning of parse_docker ? I'm guessing the formats differ?

I kept them separate because if container->m_image already contain a full qualified image name there's no need to query the images endpoint at all.
Also, note that when using libcurl, we're using HTTP1.1 pipelining so it will be almost the same as doing it in a single get_docker call.

Ok, sounds good. Just making sure.

mstemm · 2018-06-07T17:41:07Z

userspace/libsinsp/container.cpp

-	m_last_flush_time_ns(0)
+	m_last_flush_time_ns(0),
+	m_unix_socket_path(string(scap_get_host_root()) + "/var/run/docker.sock"),
+	m_api_version("/v1.24")


I like the change to embed the api version in the object, but if I recall I think the api version depends on the endpoint you're fetching, right? If that's the case, how about having multiple api versions here for the endpoints the object might need to fetch?

I don't think we need an additional api version, 1.24 is fine for both the endpoints we might query (see https://docs.docker.com/engine/api/v1.24/#3-endpoints).

mstemm · 2018-06-07T17:42:34Z

userspace/libsinsp/container.cpp

 {
+#ifndef CYGWING_AGENT
+	curl_global_init(CURL_GLOBAL_DEFAULT);


Now that both mesos and the container object might need to initialize libcurl, do you want to have a single global that does this outside of any particular object? (Not really necessary, but it would be a nice to have).

I agree, will do

mstemm · 2018-06-07T17:46:28Z

userspace/libsinsp/container.h

@@ -127,6 +133,9 @@ class sinsp_container_info
 	string m_name;
 	string m_image;
 	string m_imageid;
+	string m_imagerepo;


Do you want to make these "N/A" or similar, instead of blank, in the case where it can't be parsed? Thinking particularly about parsing old capture files where the container json event is missing these fields.

Sure, I'll create a default constructor that assign <N/A> to all the string fields.

Actually, sysdig already print out <N/A> if string fields are empty, isn't it enough?

Oh ok, that works then.

mattpag · 2018-06-08T10:38:56Z

@mstemm PTAL

If they are not already specified in the container endpoint, query the docker local daemon to retrieve also the repository, tag and digest of the container image. Also in this commit, the communication medium to the daemon has been switched from a manual implementation, hard to mantain, to libcurl. This also allow us to easily exploit HTTP/1.1 features like pipelining.

When it starts, libcurl will have already been initialized by the container manager.

mattpag requested a review from mstemm May 22, 2018 13:09

alfred-landrum reviewed May 29, 2018

View reviewed changes

mattpag force-pushed the more-container-info branch from 777b874 to cb06669 Compare May 31, 2018 19:03

mattpag mentioned this pull request May 31, 2018

Add libcurl include directory in CMakeLists falcosecurity/falco#374

Merged

mattpag force-pushed the more-container-info branch from cb06669 to 7091f84 Compare June 1, 2018 08:46

mstemm requested changes Jun 7, 2018

View reviewed changes

mstemm approved these changes Jun 11, 2018

View reviewed changes

mattpag force-pushed the more-container-info branch from 6a3eb3f to ae6f49f Compare June 12, 2018 12:34

mattpag added 10 commits June 14, 2018 10:19

parsers: load the new container image info if available

15db0c9

Add filters to retrieve new container image info

0df3d7b

Remove libcurl global initialization from mesos

0d602ce

When it starts, libcurl will have already been initialized by the container manager.

Move split_container_image function to sinsp_utils

23e63ba

Move libcurl global init in inspector

a40c72f

Make sinsp_container_docker_engine::m_curl, m_curlm static

031bc00

Fix OSX builds

a727217

Fix libcurl cleanup order

e387d06

Add ability to split repository in sinsp_utils::split_container_image

e3cf1a4

mattpag force-pushed the more-container-info branch from bb45283 to e3cf1a4 Compare June 14, 2018 08:19

mattpag merged commit 678fb9c into dev Jun 14, 2018

mattpag deleted the more-container-info branch June 14, 2018 16:14

anoop-sysd mentioned this pull request Jun 26, 2018

http_code type should be long not int [SMAGENT-951] #1159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse addl docker container image info #1127

Parse addl docker container image info #1127

mattpag commented May 22, 2018

alfred-landrum May 29, 2018

mattpag May 30, 2018

mstemm left a comment

mstemm Jun 7, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018

mstemm Jun 11, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018 •

edited

Loading

mstemm Jun 11, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018

mstemm Jun 7, 2018

mattpag Jun 8, 2018 •

edited

Loading

mattpag Jun 8, 2018

mstemm Jun 11, 2018

mattpag commented Jun 8, 2018

Parse addl docker container image info #1127

Parse addl docker container image info #1127

Conversation

mattpag commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstemm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattpag Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattpag Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattpag commented Jun 8, 2018

mattpag Jun 8, 2018 •

edited

Loading

mattpag Jun 8, 2018 •

edited

Loading