Replace netifaces by scapy #1489
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@tigercosmos Can you take a look at this? |
zhengfeihe
force-pushed
the
fix/remove-netifaces
branch
from
eafb916 to
4f4147f
Compare
|
@zhengfeihe overall it looks fine. could you resolve the CI errors? |
tigercosmos
reviewed
Jul 7, 2024
ci/run_tests/requirements.txt
Outdated
| @@ -1 +1 @@ | |||
| netifaces==0.11.0 | |||
| psutil==5.9.8 | |||
There was a problem hiding this comment.
why not use the latest 6.0.0
tigercosmos
reviewed
Jul 7, 2024
|
ummm, |
|
@zhengfeihe Seems that we need to build Alpine ourselves? |
|
also, for Windows: |
Seems we need to install python-dev ? giampaolo/psutil#1143 |
zhengfeihe
force-pushed
the
fix/remove-netifaces
branch
from
e754a22 to
2241be8
Compare
|
@zhengfeihe I used your code to run in #1486, but still got the error. seems not the issue of "python-dev". |
|
woo! I make it works. https://github.com/seladb/PcapPlusPlus/actions/runs/9827423291/job/27130086746 |
|
still try to debug windows though.. |
|
@zhengfeihe I see. for the linux part, we can refer #4783 and pick the necessary code. Your PR will be merged before #4783. |
|
btw, you can disable other tests for faster CI result, like what I did: 5f80c36 |
zhengfeihe
force-pushed
the
fix/remove-netifaces
branch
from
f2ceda3 to
e6474c0
Compare
tigercosmos
reviewed
Jul 15, 2024
| @@ -1 +1 @@ | |||
| netifaces==0.11.0 | |||
| scapy==2.5.0 | |||
There was a problem hiding this comment.
I would avoid using scapy, since we should not use a big library for just a small function.
There was a problem hiding this comment.
Let me try to find other alternative methods.
seladb
reviewed
Jul 16, 2024
ci/run_tests/run_tests_windows.py
Outdated
| @@ -65,7 +77,7 @@ def main(): | |||
| tcpreplay_interface, ip_address = find_interface() | |||
| if not tcpreplay_interface or not ip_address: | |||
| print("Cannot find an interface to run tests on!") | |||
| exit(1) | |||
| exit(1) | |||
There was a problem hiding this comment.
I think this was probably done by accident?
ci/run_tests/run_tests_windows.py
Outdated
Comment on lines
16
to
18
| if len(iface["ips"]) > 0: | ||
| # iface["ips"] will put ipv6 address first | ||
| return iface["ips"][1] |
There was a problem hiding this comment.
- Since we're taking the second element from
iface["ips"], don't we want to checkif len(iface["ips"]) > 1? - A nicer way to do it can be:
return iface["ips"][1] if len(iface["ips"]) > 1 else None
There was a problem hiding this comment.
Sure. I will change it.
|
@zhengfeihe @tigercosmos it seems we decided to replace |
|
Sorry, I missed this what was the remaining issue with psutil ? For Alpine we could either install the python3-dev or py3-psutil package if needed But for windows do you have a log of the issue ? |
Yes. Windows seems to be a little complicated. We get the guid of NIC other than the name. |
@zhengfeihe The scapy can be a temporarily solution if you cannot find another library. I would be suprised if there is no network tool for cross-platform support. If that is the case, maybe we should write one for ourself. |
|
@zhengfeihe Shall we finish the PR by this weekend? |
|
@seladb I think it's ready. The two failed tests seems not caused by my code's change, do you have ideas on fixing it? |
Dimi1010
approved these changes
Aug 7, 2024
seladb
reviewed
Aug 7, 2024
ci/run_tests/run_tests_windows.py
Outdated
| for iface in interfaces: | ||
| if iface["guid"] == guid: | ||
| # Return the second IP address if it exists, otherwise return None | ||
| return iface["ips"][1] if len(iface["ips"]) > 1 else None |
There was a problem hiding this comment.
Why do we always return the second address?
If we want to return the first IPv4 address, maybe we can do this instead:
def validate_ipv4_address(address):
try:
IPv4Address(address)
return True
except ValueError:
return False
def get_ip_by_guid(guid):
interfaces = scapy.arch.windows.get_windows_if_list()
for iface in interfaces:
if iface["guid"] == guid:
# Return the first IPv4 address if exists, otherwise return None
return next(filter(validate_ipv4_address, iface["ips"]), None)
# Return None if no matching interface is found
return NoneThere was a problem hiding this comment.
Because in the returnediface["ips"] list, the first element will be IPv6 address, and then the second element is the ipv4 address. One example is 'ips': ['fe80::ff8e:5110:73f1:e14d', '169.254.226.73']}.
I agree using check function do make more sense then just assign a fixed random number. I will change.
There was a problem hiding this comment.
@zhengfeihe It will be better to leave some comments in the code as well.
There was a problem hiding this comment.
@tigercosmos added
|
@seladb can you take a look at this again? |
tigercosmos
reviewed
Aug 11, 2024
ci/run_tests/run_tests_windows.py
Outdated
| interfaces = scapy.arch.windows.get_windows_if_list() | ||
| for iface in interfaces: | ||
| ips = iface.get("ips", []) | ||
| if not isinstance(ips, list): |
There was a problem hiding this comment.
I think this check is redundant, as you use iface.get("ips", []) already?
tigercosmos
reviewed
Aug 11, 2024
ci/run_tests/run_tests_windows.py
Outdated
| print(f"Unexpected format for 'ips' in interface: {iface}") | ||
| return None | ||
| # Use filter inside next to find the first valid IPv4 address | ||
| # The first element in the returned iface["ips"] list is set to be a ipv6 address; |
seladb
reviewed
Aug 12, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1489 +/- ##
==========================================
- Coverage 82.84% 82.84% -0.01%
==========================================
Files 273 273
Lines 46090 46090
Branches 9268 9253 -15
==========================================
- Hits 38185 38184 -1
+ Misses 7084 7082 -2
- Partials 821 824 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
seladb
approved these changes
Aug 14, 2024
|
Thank you @zhengfeihe for working on this fix, much appreciated!! 🙏 🙏 |
|
@zhengfeihe nice job! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solved issue #1384
Choose psutil as the new plan since it has a better maintenance record and more users compared to another option, ifaddr, used by some other projects.]Additionally, psutil supports all the platforms we use.Move to use scapy as the new solution, well support all the platform