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

🐛 Fix handling of socketMap #1928

Merged
merged 1 commit into from
Oct 10, 2023
Merged

🐛 Fix handling of socketMap #1928

merged 1 commit into from
Oct 10, 2023

Conversation

czunker
Copy link
Contributor

@czunker czunker commented Sep 26, 2023

Fixes #1434

@czunker czunker requested a review from arlimus September 26, 2023 06:59
@czunker
Copy link
Contributor Author

czunker commented Sep 26, 2023

This PR basically moves code from the port resource to the process resource to have the logic for the socketMap in one place.

@czunker
Copy link
Contributor Author

czunker commented Sep 26, 2023

I tried this with a local, ssh, and running container connection. OS were Ubuntu 22.04 and Alpine 3.something.

@czunker
Copy link
Contributor Author

czunker commented Sep 26, 2023

@arlimus Is this how you thought it should be?

@arlimus
Copy link
Member

arlimus commented Oct 2, 2023

Sorry I'm late to respond @czunker !

I think it's good direction to move this to the process resource, as you are approaching it.

A few things:

  1. The port resource has changed and a rebase is necessary.
  2. The trickier part: The way the updated port resource works is it doesn't bug down the engine until the user requests anything related to processes. I.e. it is very fast simple checks that look for exposure and ports, without going into processes. I'd like to keep this around, even if we move resources, to keep things running fast. An alternative approach could be to find a more optimized solution for the the sockets (since we know most native tools do this in a split second, whereas the search takes a lot of effort).

No need to press this for v9, I have updated the tags.

@arlimus
Copy link
Member

arlimus commented Oct 2, 2023

@czunker Given that the primary issue for this PR is closed right now, I'd recommend we get to this after the other priority items are resolved first.

@czunker czunker force-pushed the christian/v9_fix_socketmap branch from 8510eed to ce289d7 Compare October 5, 2023 09:23
@czunker
Copy link
Contributor Author

czunker commented Oct 5, 2023

This PR now also fixes an issue, where I didn't see data with a debian 11.

@czunker
Copy link
Contributor Author

czunker commented Oct 5, 2023

Sorry I'm late to respond @czunker !

I think it's good direction to move this to the process resource, as you are approaching it.

A few things:

  1. The port resource has changed and a rebase is necessary.
  2. The trickier part: The way the updated port resource works is it doesn't bug down the engine until the user requests anything related to processes. I.e. it is very fast simple checks that look for exposure and ports, without going into processes. I'd like to keep this around, even if we move resources, to keep things running fast. An alternative approach could be to find a more optimized solution for the the sockets (since we know most native tools do this in a split second, whereas the search takes a lot of effort).

No need to press this for v9, I have updated the tags.

For the performance issue:
We need to find out, what the actual performance issue is. This PR uses the find command only for ssh connections. local connections get the data in a different way. The local connection iterates over the /proc filesystem which we disabled for ssh. From my very limited tests, find isn't super fast, but takes only hundreds of milliseconds for a run which takes 3s - 4s in total. local takes much longer, more like minutes when I run it on my laptop.

Fixes #1434

Signed-off-by: Christian Zunker <[email protected]>
Copy link
Member

@arlimus arlimus left a comment

Choose a reason for hiding this comment

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

Awesome improvements! This should be green once rebased, nice job 🤘

@czunker czunker force-pushed the christian/v9_fix_socketmap branch from ce289d7 to e925a28 Compare October 10, 2023 06:03
@czunker czunker merged commit d79989a into main Oct 10, 2023
10 checks passed
@czunker czunker deleted the christian/v9_fix_socketmap branch October 10, 2023 06:23
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify the way _socketsMap is populated
2 participants