-
Notifications
You must be signed in to change notification settings - Fork 39
Add TURN server support #253
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: develop
Are you sure you want to change the base?
Conversation
hub/router/webrtc.go
Outdated
| if !turnConfig.Enabled { | ||
| c.JSON(http.StatusPreconditionFailed, gin.H{ | ||
| "error": "TURN server not configured", | ||
| "message": "WebRTC requires TURN server to work in restricted networks. Please configure TURN server in Admin → Global Settings.", | ||
| "action": "configure_turn", | ||
| }) | ||
| return | ||
| } |
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.
Shouldn't TURN configuration be optional? I think we shouldn't be returning errors when its not enabled because many users will never do this. Also the checks for .Server, .SharedSecret and creation of ephemeral credentials should happen only when the TURN server is enabled, no?
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 should be optional. Fixed in commit
| return nil | ||
| } | ||
|
|
||
| func UpdateWebRTCTURNConfig(device *models.Device) error { |
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.
What exactly does this update on the Android app part? Shouldn't the turn config communication go through the usual flow Browser > Hub > Provider > Device without having to send something in advance?
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.
UpdateWebRTCTURNConfig(device) is only called during device provisioning. At that point, the provider retrieves the TURN server configuration from the database and sends it to the device, following the flow you just described, with the expectation that the TURN server will be registered as a new ICE candidate.
Do you think this implementation can be improved in any way, or is it good enough as it is?
common/auth/turn_credentials.go
Outdated
| func getTURNUsernameSuffix() string { | ||
| if suffix := os.Getenv("GADS_TURN_USERNAME_SUFFIX"); suffix != "" { | ||
| return suffix | ||
| } | ||
| return "gads" // Default fallback | ||
| } |
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.
If this is part of the authentication, exactly what kind of multi-tenant deployments are we targeting? We have one hub, I dont understand or like the need to use environment variables. Things like this should go either through configuration stored in Mongo or via start up flags on the CLI
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.
Done in commit
docs/swagger.json
Outdated
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 shouldn't be committed, we generate the swagger files in the pipeline on release
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.
Removed in commit
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 file is no longer versioned
docs/swagger.yaml
Outdated
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 shouldn't be committed, we generate the swagger files in the pipeline on release
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.
Removed in commit
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 file is no longer versioned
shamanec
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.
please review the comments
Add configurable TURN server support to improve WebRTC connectivity in restrictive network environments where direct P2P connections fail. - Add TURNConfig model with server, port, credentials, and enabled flag - Implement admin API endpoints for managing TURN configuration (GET/POST /admin/turn-config) - Add validation to ensure all TURN fields are provided when enabled - Integrate TURN config delivery to Android devices during setup via WebSocket - Update GADS Settings APK with TURN support in WebRTC service - Add ICE config endpoint for client-side WebRTC configuration TURN server configuration is optional and gracefully falls back to STUN-only mode if unavailable.
…ion (as originally intended)
5588eaf to
999d1f8
Compare
Add configurable TURN server support to improve WebRTC connectivity in restrictive network environments where direct P2P connections fail.