-
Notifications
You must be signed in to change notification settings - Fork 727
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
Domain name filterchecks #1213
Conversation
95aae31
to
9a3bb48
Compare
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.
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; |
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.
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.
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.
Thanks for catching this, I broke it during the last refactoring pass. Will fix!
userspace/libsinsp/dns_manager.cpp
Outdated
} | ||
else // AF_INET6 | ||
{ | ||
std::array<uint32_t, 4> v6_ip; |
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.
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.
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 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!
userspace/libsinsp/dns_manager.cpp
Outdated
if(is_v6) | ||
{ | ||
std::array<uint32_t, 4> v6_ip; | ||
memcpy(v6_ip.data(), addr, 16); |
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 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.
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 agree with you, will do
userspace/libsinsp/filterchecks.cpp
Outdated
} | ||
} | ||
|
||
break; | ||
case TYPE_SNET: | ||
case TYPE_SERVERIP: | ||
case TYPE_SERVERIP_NAME: |
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 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?
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 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 ) |
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 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.
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.
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.
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.
Ah right, I see. The names are used as arguments to the matching function, not the values.
@mstemm PTAL |
de851b5
to
3c217d5
Compare
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()
3c217d5
to
c0fbb82
Compare
Rebased again to pick up #1218 too. |
Ok the changes look good to me! |
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
.