-
Notifications
You must be signed in to change notification settings - Fork 9
peer addresses and unconnected interfaces #23
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
Conversation
91b4758 to
eb56767
Compare
8ede74e to
226f45a
Compare
741d071 to
7a41244
Compare
e5b6180 to
e365d1d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
+ Coverage 60.16% 60.62% +0.45%
==========================================
Files 19 19
Lines 5879 5973 +94
==========================================
+ Hits 3537 3621 +84
- Misses 2342 2352 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
In line with some of my comments, can you split these two commits into two separate PRs?
I am happy to push through the /32 address support changes, but don't yet want to merge the unconnected interface changes until I can work on a proper solution for configuring parallel p2p connections.
EDIT: Ignore my previous comment, you can keep both of these commits in this PR.
munet/native.py
Outdated
| and ifaddr.network != oifaddr.network | ||
| ): | ||
| con.cmd_raises( | ||
| f"ip addr add {ifaddr} peer {oifaddr.ip} dev {ifname}" |
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 drop the peer's network mark here, which is the opposite of what you did in _set_p2p_addr (where you instead kept the peer's network mask and instead dropped the address's mask). Is this intentional?
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.
Good catch. I have made these consistent and use the peer's network in all cases. For /32 and /128 prefixes this is effectively no change. For shorter masks, the peer parameter is always a valid prefix.
munet/native.py
Outdated
| and ifaddr.network != oifaddr.network | ||
| ): | ||
| con.cmd_raises( | ||
| f"ip addr add {ifaddr} peer {oifaddr.ip} dev {ifname}" |
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.
same question as above
| @@ -0,0 +1,28 @@ | |||
| version: 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.
please move this test to the tests/config directory
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.
no problem - moved
| @@ -0,0 +1,31 @@ | |||
| version: 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.
please move this test to the tests/config directory
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.
also moved.
e365d1d to
6bbedb0
Compare
For p2p links with each endpoint in a different IP subnet, as would be found with IPv4 addresses and 32-bit subnet masks, configure the peer address. In Linux, adding an interface route with the peer's address as the destination is not sufficient; no other route will be able to use the peer's address as a next-hop.
An interface with no connection is currently omitted from the target network namespace. Instead, add a Linux "dummy" interface and assign IP addresses.
6bbedb0 to
58e72ab
Compare
liambrady
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.
LGTM
Here are a couple of changes to (1) handle interface addresses with /32 masks and (2) create a dummy for each unconnected interface.