[management]: Enhance relay and STUN service configuration logic#6715
[management]: Enhance relay and STUN service configuration logic#6715josark2005 wants to merge 7 commits into
Conversation
Bumps [github.com/pion/dtls/v3](https://github.com/pion/dtls) from 3.0.9 to 3.1.5. - [Release notes](https://github.com/pion/dtls/releases) - [Commits](pion/dtls@v3.0.9...v3.1.5) --- updated-dependencies: - dependency-name: github.com/pion/dtls/v3 dependency-version: 3.1.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesEmbedded service defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ApplySimplifiedDefaults
participant shouldStartEmbeddedRelay
participant autoConfigureClientSettings
ApplySimplifiedDefaults->>shouldStartEmbeddedRelay: evaluate authentication and external relay secret
shouldStartEmbeddedRelay-->>ApplySimplifiedDefaults: embedded relay eligibility
ApplySimplifiedDefaults->>autoConfigureClientSettings: configure client relay and STUN settings
autoConfigureClientSettings-->>ApplySimplifiedDefaults: updated management addresses and secret
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@combined/config.yaml.example`:
- Line 12: Update the STUN-related comments at the referenced configuration
entries to state that configuring stunPorts keeps local STUN active only when
the embedded local relay is enabled, requiring authSecret and, with external
relays, a matching relays.secret; clarify that stuns enables external STUN and
may coexist with local STUN rather than implying exclusive use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27255987-3e01-4fc2-aba6-cb8d89fb6c01
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
combined/cmd/config.gocombined/config.yaml.examplego.mod
josark2005
changed the title
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Problem
When external relays are configured via server.relays.addresses, the
embedded relay is unconditionally shut down. This prevents operators who
want both their own local relay and external relays from using the
combined-server deployment — they must run a separate standalone relay
process instead.
Similarly, configuring external STUN servers via server.stuns disables
the local STUN server, even though the two are independent.
Solution
When server.authSecret is set, the embedded relay stays active. If
external relays are also configured, the embedded relay remains active
only when server.authSecret matches relays.secret (they must share
the same secret for unified token authentication). The embedded relay
address is auto-added to the peer relay list — no need to declare it in
relays.addresses.
Local STUN (configured via server.stunPorts) is no longer dependent on
external STUN config. When stunPorts is set, the local STUN server
starts and its address is sent to peers alongside any external STUN
addresses.
CoderSufiyan
left a comment
CoderSufiyan
left a comment
There was a problem hiding this comment.
Good direction but a few things to address before merging:
1. Missing tests
The config logic changed significantly - shouldStartEmbeddedRelay, the else if to if split in autoConfigureClientSettings, STUN becoming additive. Zero test coverage for any of this. Add test cases for: local + external relay coexistence, STUN additive behavior, authSecret mismatch disabling embedded relay, authSecret empty skipping embedded relay.
2. Unrelated dependency bumps
The pion/dtls/v3 and pion/transport/v4 bumps in this PR are unrelated to relay/STUN config. Move these to a separate PR or at minimum explain why they are needed for this change.
3. STUN additive behavior change
Previously else if meant external STUN replaced local. Now it is plain if so both are advertised. This is probably the right behavior but it should be documented in the PR description since it changes existing user-facing behavior.
Otherwise the direction looks good.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@combined/cmd/config_test.go`:
- Around line 84-92: Update TestApplyRelayDefaults_StunEnabledWhenStunPortsSet
to also assert that c.Relay.Stun.Ports matches c.Server.StunPorts after
applyRelayDefaults, preserving the existing Enabled assertion.
- Around line 143-157: Extend
TestAutoConfigureClientSettings_StunAdditiveWithExternal to assert the local
STUN URIs at Management.Stuns indices 1 and 2, verifying they use
local.example.com with ports 3478 and 3479 respectively. Keep the existing count
and external-entry assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ea75e59-a5de-4d52-a28f-3b815298ee91
📒 Files selected for processing (2)
combined/cmd/config_test.gocombined/config.yaml.example
🚧 Files skipped from review as they are similar to previous changes (1)
- combined/config.yaml.example
|
|
All concerns from my earlier review are addressed:
The shouldStartEmbeddedRelay addition is clean. LGTM. |
2 participants
Describe your changes
Previously, setting
server.relays.addressesunconditionally disabled the embedded relay and local STUN. This change allows the embedded relay to remain active alongside external relays whenserver.authSecretis set.Changes
New predicate
shouldStartEmbeddedRelay()— determines whether theembedded relay should start: requiresauthSecretto be set, and whenexternal relays are configured,authSecretmust equalrelays.secretapplyRelayDefaults()— uses the new predicate instead of a simplehasExternalRelaycheck; logs a warning when secrets mismatch; STUN is no longer disabled by external STUN configautoConfigureClientSettings()— relay list: external and embedded addresses can coexist (embedded prepended for priority); STUN list: external and local STUN addresses can coexist (local appended)config.yaml.example— comments updated to reflect the new behaviorConfig behavior
STUN additive behavior
Previously, configuring
server.stuns(external STUN servers) replaced the local STUN with the external ones. Now, external STUN addresses are sent to peers alongside local STUN addresses (configured viaserver.stunPorts). Local STUN is only advertised when the embedded relay is active (i.e.,server.authSecretis set and matchesrelays.secret).This means peers will receive more STUN candidates, improving NAT traversal success. Local STUN only starts when
server.stunPortsis explicitly set (defaults to[3478]). To disable local STUN, setstunPortsto an empty list or simply omit the field:Relay additive behavior
Previously, configuring
server.relays.addresses(external relay servers) replaced the embedded relay entirely. Now, whenserver.authSecretis set and matchesrelays.secret, the embedded relay address is prepended to the external relay list sent to peers (embedded preferred for lower latency).This means operators can add external relay capacity without losing their local relay. The embedded relay address is auto-detected from
server.exposedAddress— no need to declare it inrelays.addresses.To use external relays without the embedded relay, simply omit
server.authSecret:Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#845
Summary by CodeRabbit
New Features
stunPortsare set, even alongside external STUN servers.Documentation
Tests