Additional relay support.#6724
Conversation
|
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)
📝 WalkthroughWalkthroughThe combined server adds ChangesAdditional relay configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant YAML as Server YAML
participant LoadConfig
participant ApplySimplifiedDefaults
participant ManagementRelays
YAML->>LoadConfig: Parse additionalRelays
LoadConfig->>ApplySimplifiedDefaults: Apply relay defaults
ApplySimplifiedDefaults->>ManagementRelays: Set local and additional relay addresses
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
Adds support for server.additionalRelays in the combined server configuration so operators can advertise extra external relay endpoints while keeping the embedded relay (and embedded STUN) running.
Changes:
- Introduces
server.additionalRelaysand appends its entries after the auto-generated local relay address in client relay settings. - Updates the combined server config example to document the new setting.
- Adds focused unit tests to validate embedded relay/STUN behavior, relay ordering, override precedence, YAML loading, and authSecret validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| combined/config.yaml.example | Documents server.additionalRelays usage in the combined server example config. |
| combined/cmd/config.go | Adds AdditionalRelays to ServerConfig and appends them to the advertised relay list when auto-configuring client settings. |
| combined/cmd/config_test.go | Adds unit tests covering relay ordering, embedded relay/STUN enablement, override precedence, config loading, and validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // - Relay: Runs locally by default; server.additionalRelays adds more relay | ||
| // addresses, while server.relays disables the local relay and replaces them | ||
| // - STUN: Runs locally on port 3478 by default; disabled if server.stuns is set |
| # - Relay: Local by default; set 'additionalRelays' to advertise more relays, | ||
| # or set 'relays' to replace the local relay with external relays | ||
| # - STUN: Local on port 3478 by default; set 'stuns' to use external instead |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6724 +/- ##
==========================================
+ Coverage 34.28% 34.32% +0.03%
==========================================
Files 977 977
Lines 123295 123413 +118
==========================================
+ Hits 42274 42362 +88
- Misses 75793 75810 +17
- Partials 5228 5241 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
combined/cmd/config_test.go (1)
41-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the interaction between
additionalRelaysandrelays.addresses.When both are set simultaneously,
additionalRelaysshould be ignored. A test verifying this would guard against future regressions in the precedence logic.💡 Suggested test
func TestAdditionalRelaysIgnoredWhenRelayOverride(t *testing.T) { cfg := DefaultConfig() cfg.Server.ExposedAddress = "https://netbird.example.com:443" cfg.Server.AuthSecret = "shared-relay-secret" cfg.Server.Relays = RelaysConfig{ Addresses: []string{"rels://relay.example.com:443"}, Secret: "external-relay-secret", } cfg.Server.AdditionalRelays = []string{"rels://extra.example.com:443"} cfg.ApplySimplifiedDefaults() wantAddresses := []string{"rels://relay.example.com:443"} if !reflect.DeepEqual(cfg.Management.Relays.Addresses, wantAddresses) { t.Fatalf("relay addresses = %v, want %v (additionalRelays should be ignored)", cfg.Management.Relays.Addresses, wantAddresses) } }🤖 Prompt for 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. In `@combined/cmd/config_test.go` around lines 41 - 61, Add a regression test alongside TestApplySimplifiedDefaultsWithRelayOverride covering both Server.Relays.Addresses and Server.AdditionalRelays. After ApplySimplifiedDefaults, assert that Management.Relays.Addresses contains only the explicitly configured relay addresses, confirming additionalRelays is ignored when the relay override is present.
🤖 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.
Nitpick comments:
In `@combined/cmd/config_test.go`:
- Around line 41-61: Add a regression test alongside
TestApplySimplifiedDefaultsWithRelayOverride covering both
Server.Relays.Addresses and Server.AdditionalRelays. After
ApplySimplifiedDefaults, assert that Management.Relays.Addresses contains only
the explicitly configured relay addresses, confirming additionalRelays is
ignored when the relay override is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94f2122a-9487-46fa-bb76-605dfb6a05d5
📒 Files selected for processing (3)
combined/cmd/config.gocombined/cmd/config_test.gocombined/config.yaml.example
CoderSufiyan
left a comment
CoderSufiyan
left a comment
There was a problem hiding this comment.
Good feature. Before this, operators had to choose between local relay and external relays. Now they can have both. The three-way logic in autoConfigureClientSettings is clean: override takes priority, then additional relays augment the local relay, then fallback to local-only.
Tests cover all five states thoroughly. The YAML example doc is clear.
One minor suggestion: the reflect.DeepEqual assertions on slice order are fragile -- if order changes but semantics stay the same, these tests break. Consider ElementsMatch or Equal with sorting for the relay address assertions. Not a blocker.
LGTM.
|
@CoderSufiyan Thanks for the review, I implemented your suggestion |
|
Describe your changes
Add
server.additionalRelayssupport to the combined server so operators canadvertise external relay servers without disabling the embedded relay and STUN
service.
The automatically generated embedded relay address remains first in the relay
list, followed by the configured additional relay addresses. The existing
server.relaysreplacement behavior is unchanged and continues to takeprecedence when configured.
This change also:
combined/config.yaml.example;server.relaysoverride behavior is preserved;The focused unit tests, race detector,
go vet, and golangci-lint pass. Theconfiguration was also tested end-to-end with a combined server, two external
relays, and independent NetBird peers, including active-relay failover.
Issue ticket number and link
Closes #5351
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
netbirdio/docs#847
Summary by CodeRabbit
additionalRelayswhile keeping the embedded/local relay enabled.additionalRelays.additionalRelays(augment) andrelays(replace), including authSecret behavior and configuration examples.additionalRelaysare configured.relaysoverridesadditionalRelays(no augmentation).