-
Notifications
You must be signed in to change notification settings - Fork 189
MapPortHandler Callback for container support #831
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: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #831 +/- ##
==========================================
+ Coverage 88.11% 88.26% +0.14%
==========================================
Files 43 43
Lines 5427 5444 +17
==========================================
+ Hits 4782 4805 +23
+ Misses 449 446 -3
+ Partials 196 193 -3
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:
|
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 so much, I like this direction let me know when it's ready for test and review.
agent_config.go
Outdated
| EnableUseCandidateCheckPriority bool | ||
|
|
||
| // MapPortHanlder is the handler used to compute mapped port for host candidate. | ||
| MapPortHanlder func(candidate Candidate) int |
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.
We've stopped adding new configurations to the AgentConfig struct since it's planned for deprecation in favor of a more Pion-aligned API. this feature should instead be exposed as an option, WithMapPortHandler, and used with the new NewAgentWithConfig API. Users who want to use this should migrate to the new API.
we did the same thing recently with the renomination API: #822 (comment)
candidate.go
Outdated
| Port() int | ||
|
|
||
| // Port mapping support for containers | ||
| MappedPort() int |
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.
question, do we have to make this public?
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.
Should we? Will webrtc care about this? I was thinking for design of separted ICE (maybe the future ion) this will be somehow used. What do you think?
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.
I think we should keep it private for now, unless we need it, or someone requests that we expose it later.
|
@xinze-zheng I'm sorry about the internal API change, we had to fix the options so we can upgrade webrtc to use them. |
|
I think it would be better if we can change the test to use the new Public api instead, |
|
@xinze-zheng hello, we merged the options API and added the new Address rewrite API if you want to continue working on this. |
|
@xinze-zheng I'll try to test and review this today or by tomorrow, thank you a lot :) |
|
@JoTurk Hi I've been looking into integrating this feature with webrtc. Seems we need to replace we just need to make GetMappedPort public in |
|
@xinze-zheng I think the only clean path if we go this route is to make a custom extension for mapped ports but i think it's maybe too much. I changed webrtc's candidates so it keeps ICE extensions when converting. Not sure about exposing mapped ports as a helper (but we can do this if no other solution). I'm maybe missing something, but why we can't overwrite candidates the same we do with addresses? |
Description
Expose a handler
func(candidate Candidate) inthandler that will be called uponaddCandidate. The handler will set themappedPortprivate field. During unmarshal, ifmappedPortis not 0, it will replace the port in ICE candidate string.Reference issue
pion/webrtc#3155