[management] pass config to controller#4807
Conversation
WalkthroughController constructor now accepts and stores a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as startManagement / Tests
participant Server as Management Server
participant NMapCtor as network_map.NewController
participant Controller as NetworkMap Controller
participant GRPC as grpc.ToSyncResponse
Caller->>Server: startManagement(...)
Server->>NMapCtor: NewController(..., portForwardCtrl, config)
NMapCtor-->>Controller: returns Controller{..., config: cfg}
Controller->>GRPC: ToSyncResponse(ctx, cfg.HttpConfig, cfg.DeviceAuthorizationFlow, ...peer...)
GRPC-->>Controller: SyncResponse (includes HTTP/JWT settings)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
management/internals/controllers/network_map/controller/controller.go (2)
51-51: Consider adding a documentation comment.The
configfield stores the global configuration for sync response generation but lacks a comment explaining its purpose.Apply this diff to add documentation:
// dnsDomain is used for peer resolution. This is appended to the peer's name dnsDomain string + // config holds the global server configuration used in sync response generation config *config.Config
73-73: Validate that config is not nil.The constructor accepts a
configparameter but doesn't validate it's non-nil. Ifnilis passed,ToSyncResponsecalls will receivenilfor config, potentially reverting to the original bug.Apply this diff to add validation:
func NewController(ctx context.Context, store store.Store, metrics telemetry.AppMetrics, peersUpdateManager network_map.PeersUpdateManager, requestBuffer account.RequestBuffer, integratedPeerValidator integrated_validator.IntegratedValidator, settingsManager settings.Manager, dnsDomain string, proxyController port_forwarding.Controller, config *config.Config) *Controller { + if config == nil { + log.Fatal("config cannot be nil") + } nMetrics, err := newMetrics(metrics.UpdateChannelMetrics())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/internals/controllers/network_map/controller/controller.go(6 hunks)management/internals/server/controllers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
management/internals/controllers/network_map/controller/controller.go (1)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)
management/internals/server/controllers.go (4)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/internals/controllers/network_map/update_channel.go (1)
PeersUpdateManager(5-13)management/server/account_request_buffer.go (1)
AccountRequestBuffer(27-33)management/server/integrations/integrated_validator/interface.go (1)
IntegratedValidator(12-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: release_ui
🔇 Additional comments (4)
management/internals/controllers/network_map/controller/controller.go (3)
22-22: LGTM!The import addition is necessary for the config dependency injection.
211-211: LGTM!Correctly passes
c.configto enable config-driven behavior in sync response generation. This fixes the bug wherenilwas previously passed.
329-329: All ToSyncResponse call sites correctly pass the config object.Verification confirms all four call sites in the codebase pass a config object as the second parameter:
management/server/peer_test.go:1180,management/internals/controllers/network_map/controller/controller.go:211,management/internals/controllers/network_map/controller/controller.go:329, andmanagement/internals/shared/grpc/server.go:716. The change at line 329 is consistent with all other call sites.management/internals/server/controllers.go (1)
73-73: LGTM! Verification confirms proper initialization.
BaseServer.configis correctly initialized in theNewServerconstructor atmanagement/internals/server/server.go:71-73, where the passedconfigparameter is directly assigned to the struct field. The code change at line 73 is safe and correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
management/server/dns_test.go (1)
15-15: Config wiring for DNS tests looks correctImporting the config package and passing
&config.Config{}intoNewControlleris consistent with the updated constructor and should be sufficient for these DNS‑focused tests. If you ever start asserting on NetbirdConfig fields in update messages here, consider reusing a more realistic config (e.g., from testdata) and passing the same instance both toNewControllerandBuildManagerinstead ofnil.Also applies to: 226-226
management/server/account_test.go (1)
28-28: NewController usage is correct; consider aligning config with BuildManagerUsing
&config.Config{}here satisfies the updatedNewControllersignature and should work for these account tests, which don’t inspect NetbirdConfig details. If you later need config‑dependent behavior (e.g., STUN/TURN fields) in update messages, it may be worth threading a single, realistically initialised config instance into bothNewControllerandBuildManagerinstead of mixing a zero‑value struct withnil.Also applies to: 2962-2962
management/server/nameserver_test.go (1)
16-16: Nameserver tests correctly updated for the new controller signatureImporting
configand passing&config.Config{}intoNewControlleris consistent with the new API and should be fine for these nameserver‑centric tests. As with the account tests, if you ever need to validate config‑dependent fields in sync responses here, consider sharing a single initialised config instance betweenNewControllerandBuildManagerinstead of usingnilin the latter.Also applies to: 795-795
management/server/peer_test.go (1)
1293-1293: Peer‑related tests correctly inject config into the controllerAll updated
NewControllercall sites now pass&config.Config{}, which is consistent with the new constructor and enough for these registration/login and rollback tests that only inspect peer/account state and network maps. If you later extend these tests to assert on config‑driven fields (e.g., TURN/Signal in NetbirdConfig), it would be worth threading a realistic config (or shared helper) into bothNewControllerandBuildManagerinstead of mixing a zero‑value struct withnil.Also applies to: 1378-1378, 1531-1531, 1611-1611
management/server/http/testing/testing_tools/channel/channel.go (1)
13-13: Controller constructor update in HTTP black‑box tests is consistentImporting
internals/server/configand passing&config.Config{}intocontroller.NewControllercorrectly matches the updated constructor signature and keeps this helper self‑contained for its channel/update assertions. If future tests start asserting on Netbird config values, you can extend this config struct there without touching the wiring again.Also applies to: 75-75
management/server/route_test.go (1)
19-19: Route test controller wiring updated to match new signatureAdding the
configimport and passing&config.Config{}intocontroller.NewControllerbrings the route test helper in line with the new constructor API while keeping the setup minimal. Given these tests focus on routing behavior and peer updates, an empty config here is adequate and can be extended later if tests start depending on config‑driven fields.Also applies to: 1294-1294
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
client/cmd/testutil_test.go(1 hunks)client/internal/engine_test.go(1 hunks)client/server/server_test.go(1 hunks)management/server/account_test.go(2 hunks)management/server/dns_test.go(2 hunks)management/server/http/testing/testing_tools/channel/channel.go(2 hunks)management/server/management_proto_test.go(1 hunks)management/server/management_test.go(1 hunks)management/server/nameserver_test.go(2 hunks)management/server/peer_test.go(4 hunks)management/server/route_test.go(2 hunks)shared/management/client/client_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
management/server/peer_test.go (4)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrated_validator.go (6)
MockIntegratedValidator(125-128)MockIntegratedValidator(153-155)MockIntegratedValidator(157-159)MockIntegratedValidator(161-163)MockIntegratedValidator(165-167)MockIntegratedValidator(169-171)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)management/internals/server/config/config.go (1)
Config(37-60)
management/server/management_proto_test.go (3)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrated_validator.go (6)
MockIntegratedValidator(125-128)MockIntegratedValidator(153-155)MockIntegratedValidator(157-159)MockIntegratedValidator(161-163)MockIntegratedValidator(165-167)MockIntegratedValidator(169-171)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
management/server/http/testing/testing_tools/channel/channel.go (2)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
management/server/nameserver_test.go (3)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrated_validator.go (6)
MockIntegratedValidator(125-128)MockIntegratedValidator(153-155)MockIntegratedValidator(157-159)MockIntegratedValidator(161-163)MockIntegratedValidator(165-167)MockIntegratedValidator(169-171)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
management/server/dns_test.go (1)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)
management/server/account_test.go (2)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
client/internal/engine_test.go (3)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrated_validator.go (6)
MockIntegratedValidator(125-128)MockIntegratedValidator(153-155)MockIntegratedValidator(157-159)MockIntegratedValidator(161-163)MockIntegratedValidator(165-167)MockIntegratedValidator(169-171)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
shared/management/client/client_test.go (2)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
management/server/route_test.go (1)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)
management/server/management_test.go (3)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrated_validator.go (6)
MockIntegratedValidator(125-128)MockIntegratedValidator(153-155)MockIntegratedValidator(157-159)MockIntegratedValidator(161-163)MockIntegratedValidator(165-167)MockIntegratedValidator(169-171)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
client/server/server_test.go (1)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)
client/cmd/testutil_test.go (2)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/server/integrated_validator.go (6)
MockIntegratedValidator(125-128)MockIntegratedValidator(153-155)MockIntegratedValidator(157-159)MockIntegratedValidator(161-163)MockIntegratedValidator(165-167)MockIntegratedValidator(169-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (6)
shared/management/client/client_test.go (1)
28-28: Controller now uses the same parsed config as the rest of the stackPassing the JSON‑loaded
configintoNewController(and already intoBuildManager/gRPC server wiring) keeps the management and network‑map paths consistent and gives ToSyncResponse full access to the real server configuration. Looks good.Also applies to: 120-121
client/cmd/testutil_test.go (1)
22-22: Test utility now passes the real management config into NewControllerUsing the JSON‑loaded
configfor bothBuildManagerandcontroller.NewControllerkeeps the test wiring aligned with production and ensures ToSyncResponse can rely on the same configuration used elsewhere. No issues spotted.Also applies to: 119-119
management/server/management_proto_test.go (1)
366-366: Wiring controller with test config looks correctPassing
configintocontroller.NewControlleraligns the controller with the same*config.Configused for the rest of the test server stack, which is important for consistentSyncResponsecontent. This change looks correct and self‑contained here.management/server/management_test.go (1)
208-208: Consistent config injection into network map controllerBoth
startServerandstartManagementnow pass the same*config.Configinstance intocontroller.NewControllerthat is used to configure the management server itself. This keeps controller behavior aligned with the server config and matches the new constructor contract.Also applies to: 1627-1627
client/internal/engine_test.go (1)
1627-1627: startManagement helper correctly forwards config to controller
controller.NewControllernow receives the populatedconfigused to start the test management server, which is consistent with the updated API and ensures the controller has access to the full management configuration during engine tests.client/server/server_test.go (1)
319-319: Management test harness now passes config into controllerThe
startManagementhelper’snetworkMapControllernow receives the same*config.Configused elsewhere in the test server setup, which properly satisfies the extendedNewControllersignature and keeps configuration consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
management/server/peer_test.go (3)
1374-1380: Same config wiring pattern as earlier controller constructionThis is the same
NewControllercall pattern as inTest_RegisterPeerByUser; see that comment for details.
1527-1533: Repeated controller config patternThis matches the earlier
NewControllerusage with&config.Config{}; refer to the first comment for rationale and optional refactor suggestion.
1607-1613: Repeated controller config patternSame change as the other controller constructions in this file; see the initial comment around line 1292.
🧹 Nitpick comments (1)
management/server/peer_test.go (1)
1289-1295: Config injection into controller looks correct; consider centralizing test configPassing a non-nil
&config.Config{}intocontroller.NewControllermatches the new constructor signature and should prevent nil‑config paths. If more tests start depending on specific config fields, consider extracting a small helper (e.g.newTestConfig() *config.Config) so you can reuse and evolve a realistic test config in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/peer_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/peer_test.go (2)
management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)management/internals/server/config/config.go (1)
Config(37-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: release
- GitHub Check: JS / Lint
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
management/internals/shared/grpc/server.go (1)
640-654: UpdateprepareLoginResponseto match newtoPeerConfigsignature
toPeerConfignow expects(*nbconfig.HttpServerConfig, *nbconfig.DeviceAuthorizationFlow), butprepareLoginResponsestill passess.configas the last argument. In Go, this will not compile once the package is rebuilt.Update the call to pass the HTTP and device-flow sub-config:
- loginResp := &proto.LoginResponse{ - NetbirdConfig: toNetbirdConfig(s.config, nil, relayToken, nil), - PeerConfig: toPeerConfig(peer, netMap.Network, s.networkMapController.GetDNSDomain(settings), settings, s.config), - Checks: toProtocolChecks(ctx, postureChecks), - } + loginResp := &proto.LoginResponse{ + NetbirdConfig: toNetbirdConfig(s.config, nil, relayToken, nil), + PeerConfig: toPeerConfig( + peer, + netMap.Network, + s.networkMapController.GetDNSDomain(settings), + settings, + s.config.HttpConfig, + s.config.DeviceAuthorizationFlow, + ), + Checks: toProtocolChecks(ctx, postureChecks), + }management/internals/controllers/network_map/controller/controller.go (1)
171-216: Fix closure bug when merging proxy network maps in sendUpdateAccountPeersInside the goroutine you call
proxyNetworkMaps[peer.ID], butpeeris the loop variable from the outerforand is captured by reference. This creates a classic closure bug: when goroutines run,peermay already have advanced to another element, so you can read the wrong proxy map for a givenp.Use the function argument
pinstead:- proxyNetworkMap, ok := proxyNetworkMaps[peer.ID] + proxyNetworkMap, ok := proxyNetworkMaps[p.ID] if ok { remotePeerNetworkMap.Merge(proxyNetworkMap) }This aligns the map lookup with the peer actually being processed by the goroutine and removes the data race on the loop variable.
🧹 Nitpick comments (4)
management/server/peer_test.go (2)
1168-1185: TestToSyncResponse wiring matches new API; consider extending coverageThe
grpc.ToSyncResponseinvocation is correctly updated to passconfig,config.HttpConfig, andconfig.DeviceAuthorizationFlowper the new signature. Since bothHttpConfigandDeviceAuthorizationFloware nil in this test, the new JWT-related behavior isn’t exercised; if you want regression coverage for SSH/JWT config derived from these fields, a dedicated test setting them would be useful.
1288-1295: NewController calls are consistent; optional: factor out a small helperAll updated
controller.NewControllercall sites now pass a*config.Config(&config.Config{}), which matches the new constructor signature and preventsc.configfrom being nil in tests.If this pattern grows further, consider a small helper like
newTestNetworkMapController(...)that encapsulates repeated wiring (store, metrics, updateManager, requestBuffer, settingsMockManager, proxy controller, config) to reduce duplication and keep tests focused on behavior.Also applies to: 1373-1380, 1526-1533, 1606-1613
management/internals/controllers/network_map/controller/controller.go (2)
38-63: Controller now holds server config; consider enforcing non-nil or guarding accessesAdding
config *config.ConfigtoControllerand threading it viaNewController(...)looks fine and keeps configuration centralized.However, both
sendUpdateAccountPeersandUpdateAccountPeerlater dereferencec.config(viac.config.HttpConfig/.DeviceAuthorizationFlow) without a nil check. If any call site ever passes a nil config, those paths will panic.Two options:
- Treat
configas required: document it andpanicearly inNewControllerifconfig == nil.- Or make it optional: compute local
httpCfg/deviceFlowCfgwith a nil guard before callingToSyncResponse.Example of the latter:
-func NewController(..., config *config.Config) *Controller { +func NewController(..., config *config.Config) *Controller { + if config == nil { + config = &config.Config{} + } return &Controller{ // ... - config: config, + config: config, } }Also applies to: 73-108
207-213: ToSyncResponse calls correctly pass HTTP and device-flow config, but rely onc.configbeing non-nilBoth
sendUpdateAccountPeersandUpdateAccountPeernow call:grpc.ToSyncResponse(ctx, nil, c.config.HttpConfig, c.config.DeviceAuthorizationFlow, ...)This matches the new signature and lets sync responses pick up HTTP/device-flow settings, while still omitting the full
*config.Config(first arg) as before.Once
c.configis guaranteed non-nil (see previous comment), these calls look good. If you decide to allowconfigto be optional, ensure you guardc.configbefore dereferencing.Also applies to: 327-331
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
management/internals/controllers/network_map/controller/controller.go(6 hunks)management/internals/shared/grpc/conversion.go(4 hunks)management/internals/shared/grpc/server.go(1 hunks)management/server/peer_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
management/internals/controllers/network_map/controller/controller.go (1)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)
management/internals/shared/grpc/server.go (1)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)
management/server/peer_test.go (2)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)
management/internals/shared/grpc/conversion.go (1)
management/internals/server/config/config.go (1)
HttpServerConfig(93-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: iOS / Build
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
🔇 Additional comments (2)
management/internals/shared/grpc/server.go (1)
686-736: sendInitialSync: config propagation into ToSyncResponse looks correctPassing
s.config,s.config.HttpConfig, ands.config.DeviceAuthorizationFlowintoToSyncResponsematches the new signature and ensures sync responses see the full server config. No issues here.management/internals/shared/grpc/conversion.go (1)
86-105: toPeerConfig/ToSyncResponse wiring for HTTP & device-flow config looks consistentThe extended signatures:
toPeerConfig(..., httpConfig *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfig.DeviceAuthorizationFlow)ToSyncResponse(..., config *nbconfig.Config, httpConfig *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfig.DeviceAuthorizationFlow, ...)and the call:
PeerConfig: toPeerConfig(peer, networkMap.Network, dnsName, settings, httpConfig, deviceFlowConfig),are consistent with the design of deriving SSH JWT config from HTTP server and device authorization flow settings. No structural issues here; behavior will depend on
buildJWTConfig, see next comment.Also applies to: 107-120
| // buildJWTConfig constructs JWT configuration for SSH servers from management server config | ||
| func buildJWTConfig(config *nbconfig.Config) *proto.JWTConfig { | ||
| if config == nil { | ||
| return nil | ||
| } | ||
|
|
||
| if config.HttpConfig == nil || config.HttpConfig.AuthAudience == "" { | ||
| func buildJWTConfig(config *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfig.DeviceAuthorizationFlow) *proto.JWTConfig { | ||
| if config == nil || config.AuthAudience == "" { | ||
| return nil | ||
| } | ||
|
|
||
| issuer := strings.TrimSpace(config.HttpConfig.AuthIssuer) | ||
| if issuer == "" { | ||
| if config.DeviceAuthorizationFlow != nil { | ||
| if d := deriveIssuerFromTokenEndpoint(config.DeviceAuthorizationFlow.ProviderConfig.TokenEndpoint); d != "" { | ||
| issuer = d | ||
| } | ||
| issuer := strings.TrimSpace(config.AuthIssuer) | ||
| if issuer == "" || deviceFlowConfig != nil { | ||
| if d := deriveIssuerFromTokenEndpoint(deviceFlowConfig.ProviderConfig.TokenEndpoint); d != "" { | ||
| issuer = d | ||
| } | ||
| } | ||
| if issuer == "" { | ||
| return nil | ||
| } | ||
|
|
||
| keysLocation := strings.TrimSpace(config.HttpConfig.AuthKeysLocation) | ||
| keysLocation := strings.TrimSpace(config.AuthKeysLocation) | ||
| if keysLocation == "" { | ||
| keysLocation = strings.TrimSuffix(issuer, "/") + "/.well-known/jwks.json" | ||
| } | ||
|
|
||
| return &proto.JWTConfig{ | ||
| Issuer: issuer, | ||
| Audience: config.HttpConfig.AuthAudience, | ||
| Audience: config.AuthAudience, | ||
| KeysLocation: keysLocation, |
There was a problem hiding this comment.
❓ Verification inconclusive
Fix nil-pointer and issuer precedence in buildJWTConfig
buildJWTConfig currently has:
func buildJWTConfig(config *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfig.DeviceAuthorizationFlow) *proto.JWTConfig {
if config == nil || config.AuthAudience == "" {
return nil
}
issuer := strings.TrimSpace(config.AuthIssuer)
if issuer == "" || deviceFlowConfig != nil {
if d := deriveIssuerFromTokenEndpoint(deviceFlowConfig.ProviderConfig.TokenEndpoint); d != "" {
issuer = d
}
}
// ...
}Issues:
-
Nil-pointer risk:
Ifconfig.AuthIssuer == ""anddeviceFlowConfig == nil(common when device auth flow isn’t configured), theif issuer == "" || deviceFlowConfig != nilcondition is true anddeviceFlowConfig.ProviderConfig...dereferences a nil pointer. -
Surprising precedence:
As written, any non-nildeviceFlowConfigwill overwrite a non-emptyAuthIssuer. That’s a significant behavior change versus “use AuthIssuer if provided, otherwise fall back to token endpoint”.
A safer implementation is:
func buildJWTConfig(config *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfig.DeviceAuthorizationFlow) *proto.JWTConfig {
if config == nil || config.AuthAudience == "" {
return nil
}
- issuer := strings.TrimSpace(config.AuthIssuer)
- if issuer == "" || deviceFlowConfig != nil {
- if d := deriveIssuerFromTokenEndpoint(deviceFlowConfig.ProviderConfig.TokenEndpoint); d != "" {
- issuer = d
- }
- }
+ issuer := strings.TrimSpace(config.AuthIssuer)
+ // Only fall back to device flow token endpoint if issuer is not explicitly set
+ if issuer == "" && deviceFlowConfig != nil {
+ if d := deriveIssuerFromTokenEndpoint(deviceFlowConfig.ProviderConfig.TokenEndpoint); d != "" {
+ issuer = d
+ }
+ }
if issuer == "" {
return nil
}
keysLocation := strings.TrimSpace(config.AuthKeysLocation)
if keysLocation == "" {
keysLocation = strings.TrimSuffix(issuer, "/") + "/.well-known/jwks.json"
}
return &proto.JWTConfig{
Issuer: issuer,
- Audience: config.AuthAudience,
+ Audience: config.AuthAudience,
KeysLocation: keysLocation,
}
}This:
- Eliminates the nil dereference when
deviceFlowConfigis nil. - Treats
AuthIssueras authoritative when set, falling back to a derived issuer only when it’s empty.
Please double-check that this precedence (explicit issuer > derived-from-device-flow) matches your intended behavior.
I'll verify the review comment by examining the actual code and its context.
Now let me examine the actual code in the repository.
#!/bin/bash
# Find and display the buildJWTConfig function
rg -n -A 30 "func buildJWTConfig" --type=go[/run_scripts]
Fix nil-pointer dereference and issuer precedence in buildJWTConfig
The analysis in the original review comment is correct. The current condition if issuer == "" || deviceFlowConfig != nil has two critical issues:
-
Nil-pointer dereference: When
issuer == ""anddeviceFlowConfig == nil, the OR condition is true (first part is satisfied), causing the code to dereferencedeviceFlowConfig.ProviderConfig.TokenEndpointon a nil pointer, resulting in a runtime panic. -
Incorrect precedence: The OR condition allows
deviceFlowConfigto override a non-emptyAuthIssuer, which is likely unintended behavior.
The proposed fix to change the condition to if issuer == "" && deviceFlowConfig != nil is correct. This ensures:
- The dereferencing only occurs when
deviceFlowConfigis not nil AuthIssueris treated as authoritative and only falls back to the device flow token endpoint when explicitly empty
Apply the suggested fix as written.
🤖 Prompt for AI Agents
In management/internals/shared/grpc/conversion.go around lines 365 to 389,
change the conditional that attempts to derive the issuer so it does not
dereference deviceFlowConfig when nil and so AuthIssuer takes precedence:
replace the current OR condition with a check that only attempts to derive from
deviceFlowConfig when issuer is empty AND deviceFlowConfig is non-nil (i.e., use
issuer == "" && deviceFlowConfig != nil), leaving the rest of the logic
unchanged so the token endpoint is only used as a fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
management/internals/shared/grpc/server.go (2)
646-651: Ensure toPeerConfig does not leak sensitive DeviceAuthorizationFlow/HTTP configThreading
s.config.HttpConfigands.config.DeviceAuthorizationFlowintotoPeerConfigmakes sense for giving clients richer configuration, but it also means whatevertoPeerConfigserializes is now client-visible. Please double‑check that it only exposes safe fields (e.g., URLs, non‑secret identifiers) and not things like OAuth client secrets or internal‑only values. If needed, consider introducing a sanitized DTO type that is derived fromDeviceAuthorizationFlow/HttpConfigbefore passing it down.
716-716: Config propagation toToSyncResponselooks correct; verify nil‑safety and validationThe updated
ToSyncResponsecall wires(s.config, s.config.HttpConfig, s.config.DeviceAuthorizationFlow, …)in the expected order and matches the new signature. One thing to verify is thats.config,s.config.HttpConfig, ands.config.DeviceAuthorizationFloware either always non‑nil on a running server or are explicitly handled as nil inToSyncResponse/toPeerConfig(and any downstream config consumers), to avoid panics on misconfiguration. You might also want a startup‑time validation hook onnbconfig.Configto fail fast if required sub‑configs are missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/internals/shared/grpc/server.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
management/internals/shared/grpc/server.go (1)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: release
- GitHub Check: release_ui_darwin
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
management/server/peer_test.go (3)
1178-1180: ToSyncResponse arguments updated; consider asserting behavior of new config inputsThe additional configuration parameters are wired correctly into
grpc.ToSyncResponse. To better guard against regressions, consider extending this test to populateHttpConfig/DeviceAuthorizationFlowonconfigand add assertions that the resultingSyncResponsereflects those values as expected.
1230-1231: Clarify and scope the//nolintdirectiveA bare
//nolintsuppresses all linters for this line and can mask unrelated problems. Prefer narrowing it, e.g.//nolint:<linter>(reason), so future readers know what is being suppressed and why.
1293-1293: Shared controller construction with realistic config would improve test fidelityPassing
&config.Config{}intocontroller.NewControllersatisfies the new parameter but leaves all config fields at zero values and duplicates the same construction in several tests. To keep tests closer to production behavior and make future config changes easier to adopt, consider:
- Extracting a small helper (e.g.
newTestNetworkMapController(...)) that builds the controller for tests, and- Initializing the returned
*config.Configwith minimal but realistic values for any fields the controller depends on (such as HTTP / device-auth flow settings), so tests also exercise config-dependent logic.Also applies to: 1378-1378, 1531-1531, 1611-1611
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/internal/engine.go(1 hunks)management/server/peer_test.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/internal/engine.go
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/peer_test.go (3)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)management/internals/server/config/config.go (2)
DeviceAuthorizationFlow(127-130)Config(37-60)management/internals/controllers/network_map/controller/controller.go (1)
NewController(73-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: Linux
- GitHub Check: JS / Lint
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
3 participants
Describe your changes
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:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit