-
Notifications
You must be signed in to change notification settings - Fork 64
Support dynamic IP registration #370
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #370 +/- ##
==========================================
+ Coverage 84.28% 84.34% +0.06%
==========================================
Files 41 41
Lines 3149 3194 +45
==========================================
+ Hits 2654 2694 +40
- Misses 356 359 +3
- Partials 139 141 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vnet/router.go
Outdated
| func (r *Router) AddIPToNIC(nic NIC, ip net.IP) error { | ||
| r.mutex.Lock() | ||
| defer r.mutex.Unlock() | ||
|
|
||
| if !r.ipv4Net.Contains(ip) { | ||
| return fmt.Errorf("%w: %s", errStaticIPisBeyondSubnet, r.ipv4Net.String()) | ||
| } | ||
| r.nics[ip.String()] = nic | ||
|
|
||
| return nil | ||
| } |
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.
This is a useful feature to add. but i don't think router is the right layer, In vnet's design addresses belong to the interface/net and the router should drive that from that state.
We also need clear semantics around: collision/overwrite behavior, or whether move ip vs alias. Also should this be called before or after start? and how the state will update or state consistent.
I think it should be somewhere in interface, with add / remove API. and update the child routers somehow.
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.
Fair point, just pushed a change that allows the addresses to be added through net which adds it to the nic.
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.
@JoTurk Just following up. Anything else we need to do here?
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.
Sorry i didn't notice that you updated the PR, i'll check when i wake up.
86602a5 to
dfd4524
Compare
dfd4524 to
37ee413
Compare
JoTurk
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.
Thank you.
Add the ability to register IP addresses with the router after Start() has been called.
For the vnet test in pion/webrtc#3323 we need to test gathering new ice candiates. We could do this by adding new interfaces on the fly, but I figured it might be easier to just change the address like dhcp.