Skip to content
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

Domain name filterchecks #1213

Merged
merged 12 commits into from
Sep 11, 2018
Merged

Domain name filterchecks #1213

merged 12 commits into from
Sep 11, 2018

Conversation

mattpag
Copy link
Contributor

@mattpag mattpag commented Sep 6, 2018

Add support for a new set of filterchecks, in the form of fd.*ip.name, that allows to match the related IP address with a domain name, and to display a previously matched one.

Domain name resolutions are handled in a separate thread spawned the first time the new filterchecks are resolved. It's in charge of keep the resolutions in sync over time.

A new dependency for the Intel TBB library has been added. It's used for its high-performance concurrent maps.

Once the PR is approved, the TBB download URL has to be changed to point under download.draios.com.

@mattpag mattpag requested a review from mstemm September 6, 2018 09:11
@mattpag mattpag force-pushed the fqdn-filterchecks branch 4 times, most recently from 95aae31 to 9a3bb48 Compare September 6, 2018 10:41
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, first pass done. Lemme know what you think. (We can also chat via slack).

else if(info.m_timeout < max_refresh_timeout)
{
// double the timeout until 320 secs
info.m_timeout <<= 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really double the timeout? It looks like m_timeout is set to base_refresh_timeout on line 47. So on every subsequent lookup, it looks like m_timeout is set to base_refresh_timeout and then doubled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this, I broke it during the last refactoring pass. Will fix!

}
else // AF_INET6
{
std::array<uint32_t, 4> v6_ip;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a semi-standard (in that I use it elsewhere in the ipv6 related code for filterchecks, etc) type ipv6addr that you could use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was focused on having an object that provide an equality operator to use in dns_info::operator=, and you're right, ipv6addr should be better. Will do!

if(is_v6)
{
std::array<uint32_t, 4> v6_ip;
memcpy(v6_ip.data(), addr, 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initially confused because I thought addr was only an ipv6 addr due to the type being uint32_t. Can you make it a void * and cast it based on is_v6? Also, you could pass an address family instead of a bool is_v6. That would make it look a bit more like other libc functions that work on both ipv4 and ipv6 addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, will do

}
}

break;
case TYPE_SNET:
case TYPE_SERVERIP:
case TYPE_SERVERIP_NAME:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these cases only share a couple of lines (fetching the fdinfo), but are otherwise unrelated. Do you want to have a separate case for the _NAME ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to minimize the code duplication (already heavy in this file) but yes, in this case I suppose it's better as you are proposing. Will do.


if(m_cmpop == CO_IN)
{
for (uint16_t i=0; i < m_val_storages.size(); i )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to rely on the base class version of flt_compare here. For the _NAME filterchecks, you should just be able to extract them as a string and then do the normal comparison operators. This is especially important for IN as you can do a set membership test instead of looping over the values and comparing each value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolution goes from the domain name to the IP, not the other way around, so I have to call sinsp_dns_manager::match() at least once for every domain name in the list.
But, as optimization, after the domain name has been resolved I should be able to do what you're proposing because then sinsp_dns_manager::name_of will work.
I'll do that and then, if needed, we can talk more about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I see. The names are used as arguments to the matching function, not the values.

@mattpag
Copy link
Contributor Author

mattpag commented Sep 7, 2018

@mstemm PTAL

@mattpag
Copy link
Contributor Author

mattpag commented Sep 7, 2018

Rebased to pick up #1214.

sinsp_dns_manager is in charge of resolving a list of domain names
and, using a separate thread, keeping them in sync over time.
This new set of filters allows to match the related IP address
with a domain name, and to display a previously matched one.
 - Change sinsp_dns_manager:{match,name_of}() signatures to be
   more consistent with standard libc address functions
 - Separate handling of fd.{c,s}ip.name in
   sinsp_filter_check_fd::extract()
@mattpag
Copy link
Contributor Author

mattpag commented Sep 7, 2018

Rebased again to pick up #1218 too.

@mstemm
Copy link
Contributor

mstemm commented Sep 10, 2018

Ok the changes look good to me!

@mattpag mattpag merged commit 69ad6dd into dev Sep 11, 2018
@mattpag mattpag deleted the fqdn-filterchecks branch September 11, 2018 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants