-
Notifications
You must be signed in to change notification settings - Fork 53
Add interface binding to ICMP sockets #89
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
base: main
Are you sure you want to change the base?
Conversation
Add interface parameter to bind ICMP sockets to a specific interface Implement Unix-only binding via SO_BINDTODEVICE in ICMPSocket Propagate interface through ping, multiping, async_ping, traceroute Update docs and examples to demonstrate interface usage
Update docs, examples, and code to reflect that binding to a network interface is supported only on Linux. Bind operations are skipped on macOS and Windows.
| _ICMP_ECHO_REPLY = -1 | ||
|
|
||
| def __init__(self, address=None, privileged=True): | ||
| def __init__(self, address=None, privileged=True, interface=None): |
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.
Nit: you can force keyword arguments by inserting an asterisk.
| def __init__(self, address=None, privileged=True, interface=None): | |
| def __init__(self, address=None, *, privileged=True, interface=None): |
It makes it easier to read code by preventing someone to write ICMPSocket(address, False), instead forcing ICMPSocket(address, privilegied=False).
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, cool! I didn't know about that function.
I also prefer it when the keyword is included; it's annoying to have to check what it stands for or be forced to set it.
Since the whole library doesn't use it, would it be unclean to only use it here? And it would be a breaking change.
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.
Yes it's a breaking change so should start with internal functions and only do it for newly added keywords.
kalvdans
left a comment
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.
It's getting harder and harder to find things to complain on 😄
I have no merge rights, just bystander hoping that my approval will help a reviewer with write access to the repo.
| SO_BINDTODEVICE, | ||
| interface.encode("ascii"), | ||
| ) | ||
| except (OSError) as err: |
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.
Nit: unnecessary parantheses
| except (OSError) as err: | |
| except OSError as err: |
| interface.encode("ascii"), | ||
| ) | ||
| except (OSError) as err: | ||
| raise ICMPSocketError(f"Cannot bind to interface {interface}: {err}") |
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.
Nit: from None will completely replace the original exception instead of making two chained exceptions.
Chained exceptions will produce tracebacks saying "while handling this, another exception occurred". Since we already include the first exception text in the second exception text, there's no need to chain them.
| raise ICMPSocketError(f"Cannot bind to interface {interface}: {err}") | |
| raise ICMPSocketError(f"Cannot bind to interface {interface}: {err}") from None |
Summary
This PR adds the ability to bind ICMP sockets (only on linux) to specific network interfaces.
Changes
Core Implementation
Sockets: Added
interfaceparameter toICMPSocket,ICMPv4Socket, andICMPv6SocketclassesSO_BINDTODEVICEsocket option on Unix systems (requires root)Functions: Added
interfaceparameter to all public functions:ping()multiping()traceroute()async_ping()async_multiping()Documentation
Updated:
Use Cases
Our infrastructure nodes have private and public interfaces. We perform end-to-end testing to ensure that the private interface is functioning correctly; if not, we configure the routes to use the external interface instead. Without interface binding, icmplib always uses the working interface, making it impossible to test a specific interface independently.
This will also fix:
Platform Support
SO_BINDTODEVICEExample Usage
Backward Compatibility
This change is fully backward compatible. The
interfaceparameter is optional and defaults toNone.Testing