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

[Linux] Connections in separate network namespaces are invisible #1691

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

davereikher
Copy link

This is a continuation of PR #1631 which was accidentally closed.
Please see #1611 for explanation of the problem and related discussion.

@davereikher
Copy link
Author

@giampaolo Is there a problem with AppVeyor builds? The errors there seem to be unrelated to my modifications.
If so, then this PR is ready for code review.

Copy link
Owner

@giampaolo giampaolo left a comment

Choose a reason for hiding this comment

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

I started looking into this PR. Here's some considerations.
This patch assumes that /proc/{PID}/ns/net (aka network namespaces) always exists. I suppose there may be circumstances (e.g. old Linux kernels) where this file may not exist, so I think the code should take that into account and fallback on using the old method (read /proc/net/tcp|udp).

Also, I suggest a refactoring: leave process_inet() and process_unix() methods alone. Instead of getting the namespace files in there, do that in retrieve() method and call process_inet() / process_unix() for each file.

Finally, but I don't think this is a problem for this PR, because we may implement #1581 sooner or later, the Python implementation will be altered. I cannot foresee how exactly that'll happen though, so ignore this part for now.

net_namespaces = defaultdict(list)
for pid in pids():
try:
ns = readlink("%s/%s/ns/net" % (get_procfs_path(), pid))
Copy link
Owner

Choose a reason for hiding this comment

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

use self._procfs_path

net_namespaces[ns].append(pid)
return net_namespaces

def process_inet(self, file_name, family, type_, inodes, filter_pid=None):
Copy link
Owner

Choose a reason for hiding this comment

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

This was a staticmethod

@@ -72,7 +72,7 @@
# --- globals
# =====================================================================


# TODO remove this comment
Copy link
Owner

Choose a reason for hiding this comment

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

?


def process_inet(self, file_name, family, type_, inodes, filter_pid=None):
"""Parse /proc/*/net/tcp* and /proc/*/net/udp* files."""
net_ns = Connections.network_namespaces()
Copy link
Owner

Choose a reason for hiding this comment

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

you can avoid calling this if filter_pid is None

giampaolo added a commit that referenced this pull request Feb 13, 2020
Added test that opens two processes. One behaves as if it's in the same
network namespace as the testing process, the other - as if it's another
network namespace. Psutil should detect a connection from the first but
not from the second process before the fix. After it should detect from
both.
If run with root privileges, will fetch all connections from all
network namespaces in the system. If run without root privileges, will
fetch connections only from processes accessible by this user, that is
will fetch connections from processes for which the path /proc/<pid>/ns
is accessible by this user.
Added a test checking the correct handling of missing /proc/PID/ns/net
file
@davereikher
Copy link
Author

Thanks, I've addressed the review in ec3760b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants